test(guard): stabilize spawn-race fixtures

[eggrollofchaos]

Summary

• model mocked guard processes as live in claim-focused contention tests
• keep fresh dead-PID protection covered by a deterministic unit test

Verification

• uv run --frozen --with pytest pytest tests/test_guard_race_2026_05_18.py::TestR1_DaemonProcessRace::test_two_processes_one_winner tests/test_spawn_lock.py::TestThreeProcessContention::test_three_process_contention tests/test_spawn_lock.py::TestV4TenProcessContention::test_ten_process_contention_30x tests/test_spawn_lock.py::TestFreshClaimProtection::test_fresh_dead_pid_is_not_reclaimed -q

Review

Required review tiers: tier-1,tier-2,tier-3

Approval authority: requester

[eggrollofchaos]

Findings

High

1. tests/test_spawn_lock.py:455-463 — The new "fresh dead PID" regression test is not deterministic under the supported COZEMPIC_PIDFILE_FRESH_SECONDS configuration. The implementation accepts any finite value in (0, 300], but the test advances its mocked clock by a fixed 1.0 second. With the valid setting COZEMPIC_PIDFILE_FRESH_SECONDS=0.1, the file is correctly classified as stale, the claim is reclaimed, and this new test fails. Reproducer: COZEMPIC_PIDFILE_FRESH_SECONDS=0.1 env -u COZEMPIC_NO_AUTO_INIT uv run --frozen --with pytest pytest tests/test_spawn_lock.py::TestFreshClaimProtection::test_fresh_dead_pid_is_not_reclaimed -q. Fix by patching _FRESH_PIDFILE_SECONDS to a known value for this unit or deriving a mocked age strictly inside the active imported window.

Low

2. tests/test_spawn_lock.py:457-463 — The new exception assertion leaves two calls in the expected-exception region and triggers the sole Sonar annotation on this PR. Construct the claim outside the assertion, then put only claim.__enter__() (or an equivalent single helper call) inside assertRaises; assertRaises already fails if no exception is raised, so the explicit self.fail(...) is unnecessary.

Verification

• Packet command: 4 passed in 20.81s.
• Valid short-window probe above: 1 failed, reproducing the High finding.
• Valid long-window probe (COZEMPIC_PIDFILE_FRESH_SECONDS=300): 1 passed.
• git diff --check fd41e95b85f283286eaca9419c19825abb6d053b..605aa1341c06859f186d198dd89c061de04f26d5: passed.
• PR context checked at head 605aa1341c06859f186d198dd89c061de04f26d5: 1 top-level bot comment, 0 inline comments, 0 review threads (0 active), 0 prior reviews. The Sonar quality gate passed with the one Low annotation described above.

Counts: 0 Critical, 1 High, 0 Medium, 1 Low, 0 Nit.

Verdict: Critical/High findings remain.

[eggrollofchaos]

Self-review verdict: LGTM — approve.

The v2 continuation closes both prior findings:

• The fresh-dead-PID test now derives its mocked age from the active _FRESH_PIDFILE_SECONDS value, so it remains strictly inside every supported positive window. Independent probes passed at 0.1, 1e-9, and 300 seconds.
• The expected-exception block now contains only claim.__enter__(), removing the multi-call assertion ambiguity and the prior static-analysis concern.

I also re-read both changed test files in full and traced the fixtures through start_guard_daemon, DaemonSpawnClaim, _is_process_alive, and the shared freshness gate. Treating the synthetic positive PIDs as alive correctly models the peer hand-off and makes the two-, three-, and ten-process contention fixtures independent of host PID state without weakening the production assertion.

Findings: none.

Verification:

• Exact-head targeted suite: 4 passed in 16.84s.
• Fresh-window probes: 0.1, 1e-9, and 300 each passed.
• Exact head: b6fea7251c191f2c422a794d639f79cbec12a3ce over base fd41e95b85f283286eaca9419c19825abb6d053b.
• PR context checked immediately before verdict: SonarCloud passed with 0 new issues; 1 top-level bot comment; 0 inline comments; 0 review threads; no unresolved current-head feedback; merge state CLEAN.

Counts: 0 Critical, 0 High, 0 Medium, 0 Low, 0 Nit.

Merge blocker: none from this review. This is a neutral confirmation only; no approval or merge action was performed.

[eggrollofchaos]

Findings

High

1. src/cozempic/guard.py:2663 — _open_guard_log() opens the predictable /tmp/cozempic_guard_<slug>.log path with blocking O_WRONLY and never verifies that the opened object is a regular file. A pre-created FIFO with no reader blocks start_guard_daemon() before it can return a structured failure or start the guard. Reproducer: create a FIFO at the log path, then call _open_guard_log; it blocks. Open with O_NONBLOCK and reject non-regular descriptors with fstat before returning the handle.

2. src/cozempic/guard.py:3177 and src/cozempic/guard.py:3254 — read_armed() and _reload_rate_exceeded() still use Path.read_text() on predictable reload-state paths. A FIFO blocks the guard loop, while a symlink lets unrelated JSON drive the armed-warning and reload-rate decisions. The current hardening only protects the reload-lock metadata. Use the same no-follow, non-blocking, regular-file reader for both state files, treating every read/parse failure as fail-closed.

3. tests/test_guard_polish_pr93.py:685 — the target head makes the existing TestPolishPR93_PidfileEACCES.test_is_pidfile_fresh_returns_true_on_permission_error fail. DaemonSpawnClaim._is_pidfile_fresh() now calls pidfile_is_fresh(), which calls os.stat, so the test's Path.stat PermissionError probe no longer reaches the implementation. Update the test to patch the active probe (or restore the tested seam) so the full suite remains green.

Verification

• Packet-targeted suite: 244 passed, 21 subtests passed.

• Additional changed-file suite: 84 passed, 5 skipped, 10 subtests passed, 1 failed (the High finding above).

• FIFO probes: guard-log open, armed sentinel read, and reload-ledger read each blocked as described.

• git diff --check fd41e95b85f283286eaca9419c19825abb6d053b..ea60b0b4de89b0fc91f5e156e1c8d8d66edc3359: passed.

• PR context: head ea60b0b4de89b0fc91f5e156e1c8d8d66edc3359; 1 top-level bot comment, 0 current-head inline comments, 0 active threads; checks passed.

• Review tier: tier-3

• Required review tiers: tier-1,tier-2,tier-3

• Approval authority: requester

• Cleared base: fd41e95

• Cleared target: ea60b0b

• Counts: Critical 0 | High 3 | Medium 0 | Low 0 | Nit 0

• Verdict: Critical/High findings remain.

• Final-head clearance: blocked: tier-3 has 3 High findings

• Approval workflow: not-authority

[eggrollofchaos]

Correction: review #4784824467 was submitted before a canonical packet claim. It is non-clearing and must not be used as review evidence.

[eggrollofchaos]

Reviewed at ea60b0b. Summary: 0 Critical, 0 High, 1 Medium, 1 Low.

Verified

Centralizing PID parsing on spawn_lock._parse_pidfile_pid is the right shape — watchdog._read_pid and the guard.py shutdown paths all delegate to it, so the hardening applies at one chokepoint rather than per call site. The ordering is TOCTOU-correct: O_NOFOLLOW on open, then S_ISREG against the already-held fd via fstat, rather than a pre-open lstat.

I exercised the primitive directly against each attack shape:

input	result
regular pidfile	parsed
symlink → valid pidfile	0 (refused)
FIFO	0 (refused, no hang)
non-UTF-8 bytes	0 (fail closed)
directory / missing	0

The symlink→valid case is the important one: it would otherwise redirect a SIGTERM at an arbitrary PID, and it is refused. Malformed input failing closed to 0 is the correct direction, since callers read 0 as "no live pid" and never signal. Termination is additionally gated behind _is_cozempic_guard_process, so a recycled PID is not signalled.

Reproduced the stated suite: 244 passed, 21 subtests.

Medium — _open_guard_log can hang on a pre-planted FIFO

guard.py:_open_guard_log opens with O_CREAT|O_WRONLY|O_APPEND|O_NOFOLLOW. O_NOFOLLOW rejects a symlink but not a FIFO, and without O_NONBLOCK a write-only open on a FIFO blocks until a reader appears. Since the log path is deterministic, a FIFO planted there stalls the guard at startup. Confirmed by probe, with the hardened read shape as a control:

_open_guard_log shape   (FIFO planted): *** BLOCKED (hang) ***
hardened read shape (FIFO, +O_NONBLOCK): opened (no block)

The two O_RDWR lock opens (_stale_reclaim_lock, doctor._is_lock_held) do not block, and the O_CREAT|O_EXCL creators are fine by construction — so this is the single remaining instance, not a class. Suggested fix: add O_NONBLOCK and an S_ISREG check on the opened fd, matching what _parse_pidfile_pid, _read_regular_metadata, and _read_regular_log_tail already do. This also closes the last gap against the stated "reject symlinks and FIFOs" goal.

Low — the O_NOFOLLOW platform caveat is noted at one of nine sites

Nine call sites guard with hasattr(os, "O_NOFOLLOW"); on a platform lacking it the flag is silently dropped and the open follows the symlink, after which S_ISREG passes if the target is a regular file. Only reload_lock.py:272 records this ("not on Windows"); the other eight read as unconditionally symlink-safe. Behavior is consistent, documentation is not. Extracting one shared open-regular-no-follow helper would state the caveat once, prevent the flag sets from drifting apart, and give a single place to add a fallback later.

Not approving — leaving that to the PR owner.

[eggrollofchaos]

Tier 3 Review — PR #178 (guard race-test stability)

Review tier: tier-3
Required review tiers: tier-1,tier-2,tier-3
Approval authority: requester
Cleared base: not-applicable
Cleared target: not-applicable
Counts: Critical 0, High 1, Medium 0, Low 0, Nit 0
Verdict: CHANGES_REQUESTED
Approval workflow: not-authority

Findings

High — concurrent test invocations share one global PID path and fail each other

tests/test_spawn_lock.py:108 fixes SESSION_ID to fade1234-5678-9abc-def0-2026051811cc, which maps every invocation to /tmp/cozempic_guard_fade1234-567.pid. Two normal concurrent invocations of this test therefore contend on the same production-style claim. Each correctly sees the other invocation's fresh fake PID and returns already_running, but each test still requires one local started=True result. A concurrent run failed both copies (11/20 and 12/20 iterations respectively), so parallel CI or another local test run makes this claimed stability gate flaky.

Give each top-level test invocation a unique session ID (or isolate its PID path under a per-test temporary directory), while retaining the shared ID only within that invocation's three worker processes. Clean the matching persistent reclaim-lock path as part of that fixture cleanup.

Verification

• git diff --check fd41e95b85f283286eaca9419c19825abb6d053b 8e94e4133da82740cc248d1f3e39fdc55980c15a — passed.
• A single invocation of TestThreeProcessContention passed on three retries.
• Two simultaneous normal invocations of that exact test each failed because of the fixed shared /tmp PID path.

[eggrollofchaos]

Findings

High

1. tests/test_spawn_lock.py:122-170 — the claimed race-stability regression test is still nondeterministically red at this head. Two invocations of TestThreeProcessContention::test_three_process_contention failed: one recorded 3 bad iterations out of 20 and the next 10 out of 20, each with all three workers returning already_running=True against a freshly reset PID path. This removes the test's exactly-one-winner guarantee and makes the safety gate flaky. Isolate the fixture's per-iteration runtime artifacts and/or correct the claim handoff so every iteration deterministically yields one started=True result.

Verification

• Focused 367-test command stopped at TestThreeProcessContention::test_three_process_contention (3/20 bad iterations).
• Exact re-run of that test failed (10/20 bad iterations).
• TestV4TenProcessContention::test_ten_process_contention_30x passed.
• git diff --check passed.

Review tier: tier-3
Required review tiers: tier-1,tier-2,tier-3
Approval authority: requester
Cleared base: not-applicable
Cleared target: not-applicable
Counts: Critical 0 | High 1 | Medium 0 | Low 0 | Nit 0
Verdict: Critical/High findings remain.
Approval workflow: not-authority

[eggrollofchaos]

Findings

Critical

1. src/cozempic/guard.py:3790-3826,3853-3857; 4252-4277 — When PID handoff fails after Popen and the attempted child cleanup cannot stop the child, the code records orphaned_guard_pid but leaves claim.handed_off false. The finally block then removes the PID claim, and reload_self_daemon() retries with no live claim visible. That retry can start a second guard for the same session while the first remains alive, allowing concurrent prune/reload activity against one transcript.

   Reproduced by simulating a PID-handoff os.replace error and a child that survives terminate/kill waits: the first call returned orphaned_pid=712345; the retry started PID 712346 for the same session. When cleanup cannot stop the child, atomically publish its real PID, retain the claim, and treat it as already running instead of retrying. Add a regression test for this path.

Verification

• Focused guard/lock suite: 146 passed, 21 subtests passed.
• Two simultaneous normal invocations of TestThreeProcessContention::test_three_process_contention: both passed.
• git diff --check fd41e95b85f283286eaca9419c19825abb6d053b...954d6ea6470ade52278fc70b1d47f17b81f374f3: passed.

Counts: 1 Critical, 0 High, 0 Medium, 0 Low, 0 Nit.

Verdict: Critical/High findings remain.

[eggrollofchaos]

Reviewed at 954d6ea. 0 Critical, 0 High, 2 Medium, 0 Nit-blocking — LGTM. Reviewed the full range since the last clean pass (ea60b0b4..954d6ea6, 11 commits), since the intermediate head was never separately reviewed.

The previously reported blocking-open issue is fixed. _open_guard_log and doctor._is_lock_held both take O_NONBLOCK plus an S_ISREG check now, which closes the FIFO hang and keeps the non-regular case fail-safe (_is_lock_held returns "held", so the file is left alone). O_NONBLOCK on the surviving regular-file path is a no-op for read/write, so the buffered writer is unaffected.

Test isolation verified empirically, not just read. Ran the contention suite from a clean tree: 159 passed, 31 subtests; pgrep -f cozempic unchanged at 189 before and after; zero new /tmp/cozempic_* artifacts in the run window; git status clean afterwards. The reaping refactor is also correctly ordered — assertFalse(errors) precedes the sum(result["claimed"] ...), so a queue timeout fails with diagnostics rather than a KeyError, and the try/finally guarantees reaping even when a worker never reports.

Medium — the read_armed validation admits bools and skips two fields

isinstance(True, int) is True in Python, so a JSON bool satisfies the new numeric check. Writing a sentinel with {"tier": true, "armed_at": true, "projected_pct": "sixty", "warned": "nope"} and reading it back:

armed_at survives validation: True   (value True)
time.time() - armed_at        = 1785152217.4
_grace_ok (>= grace)          : True     <- grace clock defeated
tier kept                     : True (bool)
projected_pct                 : 'sixty'  <- unvalidated
warned truthy                 : True     <- unvalidated

Two independent paths reach the same outcome. armed_at: true passes validation, and time.time() - True yields ~1.8e9 seconds, so the grace gate is always satisfied. Separately, warned is never type-checked, and the consumer does bool(_armed.get("warned")), so any truthy value short-circuits the same gate. Either one lets a reload proceed without the warning the grace clock exists to deliver.

projected_pct is also unvalidated. cli.py defends itself with an isinstance check at the display site, but write_armed carries the bad value forward via existing.get("projected_pct", 0.0), so a corrupt value persists across re-arms instead of being neutralized.

Corrupt sentinels are the threat model this function is being hardened for — the atomic writer means cozempic itself will not produce these, so reaching it takes external tampering or a foreign writer. That is what keeps this Medium rather than higher. Suggested fix: reject bools explicitly (isinstance(x, (int, float)) and not isinstance(x, bool)), extend the same treatment to warned (bool) and projected_pct (numeric), and reset rather than carry forward in write_armed.

Note the existing test pins the current behavior including this hole: test_read_armed_drops_nonnumeric_grace_fields passes with armed_at: true because a bool is numeric by isinstance.

Medium — two of the three new guards are not pinned by any test

Mutation-checked each hardening change against the full suite rather than a filtered subset:

Guard removed	Result
_open_guard_log S_ISREG	1944 passed — uncovered
doctor._is_lock_held S_ISREG	1944 passed — uncovered
read_armed field sanitization	1 failed (test_read_armed_drops_nonnumeric_grace_fields) — covered

The _open_guard_log guard is the fix for the blocking-open issue reported last round, and nothing in 1944 tests notices its removal — so a future refactor can silently reintroduce the hang with a green suite. A test that points the guard log at a FIFO and asserts the open raises rather than blocks would pin both it and the doctor equivalent.

Nits

• _reap_processes is now duplicated verbatim in tests/test_spawn_lock.py and tests/test_guard_race_2026_05_18.py. A shared helper keeps the two from drifting, which matters because the reaping policy is the thing under test.
• TestStaleClaimContention got a try/finally around its result-collection loop so reaping always runs; TestV4TenProcessContention has the identical shape but no try/finally. Nothing in that window raises today, so it is not a live leak — but the sibling fix was not applied symmetrically, and a future edit that raises there would leak 10 workers per iteration.

Not approving — leaving that to the PR owner.

[eggrollofchaos]

Correction to my earlier review at this head

My previous review at 954d6ea6 gave an approve verdict. It was issued without triaging an unresolved Critical that had already been reported against this same commit 16 minutes earlier. I am withdrawing that approve verdict — it should not be treated as clearing this head.

Critical (still unresolved at this head)

src/cozempic/guard.py — PID-handoff failure path leaves a live child with no claim.

When the PID-file os.replace fails after Popen and the child survives both terminate() and kill(), the handler sets orphaned_guard_pid = proc.pid, records the orphan, and re-raises. claim.handed_off = True sits after that inner except BaseException block, so it is unreachable on this path. The claim's __exit__ then unlinks the .pid file — as the code's own comment states — while the child is still running. A subsequent reload retry sees no live claim and can start a second guard for the same session, allowing concurrent prune/reload activity against one transcript.

Verified at 954d6ea6 by reading the emitted blob: the handed_off assignment is unreachable on the orphan path, and the orphan branch is entered precisely when the child could not be stopped.

In scope for this PR — the diff modifies this region (guard.py hunks at 3794, 3827, 3834, 3994, 4253, 4261, 4277).

Suggested direction is unchanged from the original report: when cleanup cannot stop the child, atomically publish its real PID and retain the claim, treating it as already-running rather than retrying. Worth a regression test that forces a handoff failure with an unkillable child and asserts no second guard starts.

Still open from my prior pass (non-blocking)

• Medium — read_armed field validation admits bools (isinstance(True, int) is True), and does not validate warned or projected_pct at all. An armed_at: true record satisfies the grace gate.
• Medium — two of the three new S_ISREG guards are unpinned: removing either leaves the full suite green, so the blocking-open regression can return silently.
• Nit — _reap_processes duplicated verbatim across tests/test_spawn_lock.py and tests/test_guard_race_2026_05_18.py.
• Nit — try/finally reaping applied in TestStaleClaimContention but not in TestV4TenProcessContention.

Counts: 1 Critical, 0 High, 2 Medium, 0 Low, 2 Nit.

Verdict: Critical/High findings remain.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.1% Duplication on New Code

See analysis details on SonarQube Cloud

[eggrollofchaos]

Review notes on d23cae3. The O_CREAT|O_EXCL claim rewrite looks sound — single winner, no observable placeholder PID across the parent→daemon hand-off, and _parse_pidfile_pid's O_NOFOLLOW/O_NONBLOCK/S_ISREG hardening correctly neutralises a planted FIFO or symlink at the pidfile path. Two things worth a look before merge.

1. _stale_reclaim_lock can block forever. The persistent companion lock is the right fix for the flock-unlink race, but the acquisition is unbounded:

fcntl.flock(fd, fcntl.LOCK_EX)   # no LOCK_NB, no timeout

This is on the SessionStart path, so any stalled holder wedges every subsequent session start with no error and no diagnostic. The lock file is also opened O_CREAT|O_RDWR|O_NOFOLLOW without O_EXCL and without an S_ISREG check, at a predictable /tmp path — on a shared host another local process can pre-create it and hold the lock. Suggest LOCK_EX | LOCK_NB in a bounded retry loop, treating exhaustion as DaemonAlreadyStarting (matches the module's existing conservative posture), plus the S_ISREG check for parity with the pidfile reader.

2. A pre-existing test reaches real tmux send-keys. Not introduced here — it reproduces on fd41e95 and the file isn't in this diff — but it surfaced while running the suite, so flagging it. tests/test_guard_transient_race.py patches _spawn_reload_watcher, _is_claude_process, _wait_for_exit, os.kill and time.sleep, but not subprocess.run, so guard.py:2308 executes for real. With a logging tmux stub on PATH:

send-keys -t %53 /exit Enter
send-keys -t %53 cd '/tmp/fake_project' && claude --resume abcdef012345678901234567890abcde Enter

Running the suite inside tmux puts the developer's own pane into the exit-confirmation dialog, then tries to resume a fixture UUID from a directory that doesn't exist. Note pane = os.environ.get("TMUX_PANE", "") and *(["-t", pane] if pane else []) — when TMUX_PANE is unset the -t is dropped entirely and send-keys targets whichever pane is currently focused, so it isn't limited to the person running the tests. (target on the preceding line is computed and never used, which suggests the fallback wasn't deliberate.)

One-line fix on the test side: add patch("cozempic.guard.subprocess.run") to that context stack — test_guard_hardening.py already does this throughout; this file is the outlier. Separately, it may be worth having the production path refuse to send when no pane resolves rather than retargeting to the focused one.

The changed test files in this PR are clean on that score: with the same stub, test_guard_robustness, test_spawn_lock, test_guard_race_2026_05_18, test_guard_hardening, test_interactive_guard, test_guard_watchdog and test_doctor ran 250 passed / 21 subtests with zero tmux invocations.

Smaller items: reclaim-lock files accumulate per session slug in /tmp and nothing ages them out (doctor seems the natural owner); record_orphan fsyncs the file but not the parent directory, unlike the pidfile hand-off a few lines below; and the test(guard): title undersells a change that touches eight source files (~780 added lines in src/).

Verification: full suite at head is 7 failed / 1941 passed / 5 skipped / 281 subtests. Five of those failures reproduce identically on fd41e95 (test_auto_init_global_skip ×4, test_guard_transient_race ×1) — pre-existing. The other two are wall-clock assertions (...under_500ms, ...quadratic_input_bounded) that varied between runs on a loaded machine.