[realtonyyoung]

Fixes two kcap-cli test flakes. Linear: AI-910, AI-1516.

AI-910 — ClaudeHookStdoutTests stdout-capture contamination

The three tests captured hook stdout by redirecting the process-global Console.Out to a StringWriter. A concurrently-running test whose SUT writes to Console.Out (e.g. CodexHookCommand emitting {"continue":true}) leaked a second JSON object into the capture → JsonReaderException: '{' is invalid after a single JSON value.

Fix (issue's preferred option 1): thread an optional TextWriter? stdout through ClaudeHookCommand.Handle → HandleWithDeps → HandleCore (default Console.Out). The SessionStart hookSpecificOutput envelope and the auth-lapsed systemMessage nudge now write to that writer. Each test passes its own StringWriter — no Console.SetOut, so nothing another test writes to Console can contaminate it. This is the durable fix for the class (a keyed [NotInParallel] couldn't prevent it; the writer does, regardless of scheduling).

[NotInParallel] is kept: the injection ends the contamination flake, but these tests still exercise process-global state (KCAP_CONFIG_DIR config + the in-process auth-provider cache) and real hook HTTP timing. (Verified that fully parallelizing them exposes an unrelated empty-envelope flake under load — out of scope.)

AI-1516 — HandleStopAgent reap-by-pid-record confirmation race

Surfaced on PR #360's first ubuntu run: HandleStopAgent_unknown_id_reaps_by_pid_record_where_env_is_readable intermittently failed with the PID record still present after the process had exited.

Root cause: ProcessReaper.KillConfirmAsync's Classify reads IsAlive (kill(0)) then the start token (/proc/{pid}/stat). A SIGTERM'd, proven-ours process in its death transition reads alive=true but its token momentarily unreadable → Ambiguous. The grace loop returned false on that transient Ambiguous, so the reaper reported unconfirmed even though the process was (about to be) dead → the record was stranded (ReapByRecordAsync deletes only on confirmed-gone). The single fixed 250ms post-SIGKILL check had the same fragility under OS-reflection lag.

Fix: both confirmation waits (grace + post-SIGKILL) poll via a shared PollForConfirmedDeathAsync for a definitive Dead/Recycled, treating a transient Ambiguous as keep polling (we've already proven ownership and signalled). The pre-signal and pre-SIGKILL gates still spare on Ambiguous — the safety invariant is unchanged: only a proven-ours leader is ever signalled, and a persistently-ambiguous target is still retained for the next sweep.

Test correction (see the Linear comment). The test's macOS branch asserted "always spares (HasExited == false)", but tracing showed that's a race, not a real spare: on macOS 26.5.2 Classify = Ours (identity is readable) so the reaper does SIGTERM the dummy — the old code only "passed" by returning on the first transient Ambiguous before the SIGTERM landed, leaving the process dead-but-record-retained (the exact bug). The macOS branch now asserts the real invariant — record deleted IFF the process was confirmed gone — making it a true regression guard (fails on the pre-fix dead-but-retained state, passes on the fix) and robust across macOS versions.

Verification (local, macOS 26.5.2)

• ClaudeHookStdoutTests 3/3; full Capacitor.Cli.Tests.Integration 144/144; ClaudeHookCommandTests 35/35; SpoolOutageRecoveryTests 2/2.
• Reaper test 4/4; AgentOrchestratorVendorTests 84/84; OrphanReaperTests 9/9.
• Build clean; check-linear-ids.sh passes.
• The Linux reap+confirm path (the CI flake) is validated by the ubuntu CI leg.

🤖 Generated with Claude Code

[linear-code]

AI-910

AI-1516

[qodo-code-review]

PR Summary by Qodo

Fix kcap-cli parallel-load flakes in hook stdout capture and process reaping

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Inject per-call stdout writer into ClaudeHookCommand to avoid Console.Out test contamination.
• Make ProcessReaper death confirmation resilient to transient Ambiguous states under load.
• Update integration/unit tests to use writer injection and assert correct record-retention
 invariants.

Diagram

graph TD
  subgraph "AI-910: Hook stdout"
    T1["ClaudeHookStdoutTests"] --> C1["ClaudeHookCommand"] --> W1["Injected TextWriter"]
    C1 --> CO1["Console.Out"]
  end
  subgraph "AI-1516: StopAgent reap"
    T2["StopAgentPidFallbackTests"] --> PR1["ProcessReaper"] --> PI1["ProcessIdentity/Classify"] --> OS1{{"OS /proc"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Globally serialize stdout-writing tests

• ➕ Avoids production code signature changes
• ➖ Brittle: requires perfect coverage of every Console.Out writer across the suite
• ➖ Slows CI by reducing parallelism
• ➖ Doesn’t address non-test concurrent stdout usage (still process-global)

2. Use a single fixed post-signal delay (increase 250ms)

• ➕ Minimal code change
• ➖ Still flaky under variable OS scheduling/procfs reflection lag
• ➖ Either too short (flakes) or too long (slows stop/reap paths)

3. Treat Ambiguous as confirmed-dead after signaling

• ➕ Would prevent record stranding
• ➖ Risks false confirmation when OS visibility is persistently ambiguous (safety invariant erosion)
• ➖ Could delete durable tracking for a still-alive process

Recommendation: The PR’s approach is the most robust: (1) stdout writer injection removes reliance on process-global Console.Out while keeping behavior identical for production callers, and (2) polling for a definitive Dead/Recycled after signaling preserves the safety invariant (ambiguity never kills/never confirms) while eliminating transient-Ambiguous timing races that strand PID records.

Files changed (4) +107 / -79

Bug fix (2) +59 / -30

ProcessReaper.csPoll for definitive process death after TERM/KILL to avoid transient Ambiguous races +40/-21

Poll for definitive process death after TERM/KILL to avoid transient Ambiguous races

• Introduces a shared PollForConfirmedDeathAsync loop used during the SIGTERM grace window and after SIGKILL. This treats transient Ambiguous states (e.g., /proc stat unreadable mid-exit or slow OS reflection) as ‘keep waiting’ rather than immediate failure, while preserving the pre-escalation safety gate that still spares on Ambiguous.

src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs

ClaudeHookCommand.csAdd optional stdout TextWriter to isolate hook output from process-global Console.Out +19/-9

Add optional stdout TextWriter to isolate hook output from process-global Console.Out

• Threads an optional TextWriter through Handle → HandleWithDeps → HandleCore and uses it for SessionStart hook envelope and auth-lapsed systemMessage output. Defaults to Console.Out when not provided, maintaining existing runtime behavior while enabling test-safe capture without Console.SetOut.

src/Capacitor.Cli/Commands/ClaudeHookCommand.cs

Tests (2) +48 / -49

ClaudeHookStdoutTests.csCapture hook stdout via injected writer instead of Console.SetOut +34/-40

Capture hook stdout via injected writer instead of Console.SetOut

• Removes process-global Console.Out redirection and replaces it with per-test StringWriter injection via ClaudeHookCommand.Handle(..., stdout: writer). Updates documentation/comments to explain the contamination flake and why [NotInParallel] remains for other shared-state reasons.

test/Capacitor.Cli.Tests.Integration/ClaudeHookStdoutTests.cs

StopAgentPidFallbackTests.csRelax macOS expectations and assert record deletion matches confirmed process death +14/-9

Relax macOS expectations and assert record deletion matches confirmed process death

• Updates the stop-agent PID-record fallback test to accept OS-version-dependent macOS behavior (Ambiguous spare vs provable ownership). Asserts the invariant that the PID record is retained iff the process was not confirmed gone, aligning the test with the fixed confirmation semantics.

test/Capacitor.Cli.Tests.Unit/Daemon/StopAgentPidFallbackTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

1. Verbose KillConfirmAsync comments ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New multi-line narrative comments around KillConfirmAsync and PollForConfirmedDeathAsync are
overly verbose and restate behavior already expressed by the code. This increases maintenance cost
and can obscure the actual invariants during future changes.

Code

src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[R117-127]

+            // Grace poll. The leader was proven ours immediately above, so a TRANSIENT Ambiguous here is
+            // the death transition — kill(pid,0) still answers for an instant after the process has begun
+            // exiting while /proc/{pid}/stat has already gone, so the start-token read is momentarily
+            // unreadable — NOT a reason to spare. Keep polling for the definitive Dead/Recycled; only
+            // those confirm within the window (see PollForConfirmedDeathAsync).
+            if (await PollForConfirmedDeathAsync(pid, expectedIdentity, GraceBeforeKill, ct)) return true;
+
+            // Grace elapsed without a definitive gone verdict. Re-gate before the hard kill: never SIGKILL
+            // a pid recycled to a foreign process, or one we can no longer prove is ours — ambiguity spares
+            // BEFORE we escalate, so the safety invariant ("only a proven-ours leader is ever signalled")
+            // is unchanged.

Evidence

PR Compliance ID 6 requires avoiding overly verbose comments. The added comment blocks in
ProcessReaper.cs are multi-line, narrative explanations (including emphasized wording like
TRANSIENT, NOT, ONLY) that primarily restate the polling and safety-gating behavior already
present in PollForConfirmedDeathAsync and the surrounding control flow.

CLAUDE.md: Avoid overly verbose comments; prefer self-explanatory code
src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[117-127]
src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[153-158]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New/modified comments are overly verbose and largely re-describe what the code already makes clear.

## Issue Context
Compliance requires keeping comments concise and preferring self-explanatory code; long narrative explanations are harder to keep correct over time.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[117-127]
- src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[153-158]
- src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[136-138]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Windows hard-kill wait ✓ Resolved 🐞 Bug ➹ Performance

Description

On Windows, KillConfirmAsync can now wait up to ConfirmAfterKill (5s) after the hard-kill step even
though SignalGroup(hard:true) performs no kill on Windows. This can increase StopAgent/orphan-reap
latency for a non-terminating pid by up to an additional 5s compared to the previous single-tick
post-kill check.

Code

src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[R133-144]

            SignalGroup(pid, hard: true); // SIGKILL the group while the leader is still proven ours
-            await Task.Delay(250, ct);

-            switch (Classify(pid, expectedIdentity)) {
-                case LeaderState.Dead:      return true;
-                case LeaderState.Recycled:  return true;
-                case LeaderState.Ambiguous: return false; // became uncomparable → unconfirmed, retain
-                default:
-                    logger.LogWarning("ProcessReaper: pid {Pid} (agent {AgentId}) still alive after SIGKILL — retained for next sweep", pid, agentId);
-                    return false;
-            }
+            // SIGKILL WILL terminate it; the OS just needs a moment to reflect the death (reap the zombie /
+            // update /proc), which under heavy parallel load can exceed a single tick. Poll the same way
+            // rather than checking once, so a slow-to-reflect kill isn't misreported as still-alive.
+            if (await PollForConfirmedDeathAsync(pid, expectedIdentity, ConfirmAfterKill, ct)) return true;
+
+            logger.LogWarning(
+                "ProcessReaper: pid {Pid} (agent {AgentId}) not confirmed gone within {Grace}s+{Kill}s — retained for next sweep",
+                pid, agentId, GraceBeforeKill.TotalSeconds, ConfirmAfterKill.TotalSeconds);
+            return false;

Evidence

The PR adds a 5s polling confirmation window after the hard-kill call in KillConfirmAsync, but on
Windows the hard-kill path in SignalGroup does nothing (only the soft path kills the process tree).
Therefore, the extra polling can extend the time spent per pid without any additional termination
action having occurred on Windows.

src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[32-34]
src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[107-145]
src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[175-185]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
On Windows, `SignalGroup(pid, hard: true)` is a no-op, but `KillConfirmAsync` still performs a full `ConfirmAfterKill` polling window afterward. This can add up to 5 seconds of extra latency per pid in the failure/unconfirmed path.

## Issue Context
- Windows kill behavior is implemented in `SignalGroup`: only `hard:false` does `Process.Kill(entireProcessTree: true)`; `hard:true` returns without acting.
- `KillConfirmAsync` always executes the post-"hard kill" poll (`ConfirmAfterKill`) regardless of OS.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[107-145]
- src/Capacitor.Cli.Daemon/Services/ProcessReaper.cs[175-189]

## Suggested fix
- Make the post-hard-kill confirmation window OS-aware, e.g.:
 - On Windows: either (a) skip the `ConfirmAfterKill` poll entirely and do a single `Classify` check, or (b) set the window to a much smaller value, or (c) repeat the `entireProcessTree` kill for `hard:true`.
- Keep the Unix behavior unchanged (where SIGKILL is meaningful and OS reflection lag is expected).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Verbose test rationale comments ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The updated test documentation blocks include extensive rationale that is longer than necessary to
understand the tests. This violates the guideline to keep comments concise and raises ongoing
maintenance burden when implementation details change.

Code

test/Capacitor.Cli.Tests.Integration/ClaudeHookStdoutTests.cs[R15-27]

+/// Stdout is captured by passing an explicit <see cref="TextWriter"/> to
+/// <c>Handle</c> (its <c>stdout</c> parameter) instead of redirecting the
+/// process-global <c>Console.Out</c>. The capture is private to each test, so a
+/// concurrently-running test whose SUT writes to <c>Console.Out</c> (e.g.
+/// <c>CodexHookCommand</c> emitting <c>{"continue":true}</c>) can't leak into it
+/// — the contamination flake this removes (a keyed <c>[NotInParallel]</c>
+/// couldn't, since a differently-keyed test still ran concurrently).
///
-/// Config isolation is provided by <see cref="IntegrationGlobalSetup"/>,
-/// which points <c>KCAP_CONFIG_DIR</c> at a fresh temp directory before
-/// any test code touches <c>PathHelpers</c>. Without it, a developer-side
-/// <c>excluded_paths</c> entry covering the test <c>cwd</c> would silently
-/// short-circuit <c>ClaudeHookCommand</c> and make these tests pass for
-/// the wrong reason.
+/// The tests stay <c>[NotInParallel]</c>: they still exercise process-global
+/// state (the <c>KCAP_CONFIG_DIR</c> config + the in-process auth-provider
+/// discovery cache) and real hook HTTP timing that isn't parallel-safe. The
+/// injected writer is the durable fix for the capture-contamination class —
+/// it holds even if the serialization is ever relaxed.

Evidence

PR Compliance ID 6 requires avoiding overly verbose comments. The PR adds/expands long explanatory
comment blocks in tests (including multi-paragraph rationale and detailed failure-mode narration)
that could be shortened to a brief statement of intent and constraints.

CLAUDE.md: Avoid overly verbose comments; prefer self-explanatory code
test/Capacitor.Cli.Tests.Integration/ClaudeHookStdoutTests.cs[15-27]
test/Capacitor.Cli.Tests.Unit/Daemon/StopAgentPidFallbackTests.cs[30-43]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New/modified comments in tests are overly long and provide deep narrative context that can drift from reality.

## Issue Context
Compliance requires concise comments; keep only the minimum needed to understand why the test is structured this way.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Integration/ClaudeHookStdoutTests.cs[15-27]
- test/Capacitor.Cli.Tests.Unit/Daemon/StopAgentPidFallbackTests.cs[30-43]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[realtonyyoung]

Addressed all three Qodo findings in 898246b:

1. Windows hard-kill wait (Bug/Perf) — good catch. On Windows SignalGroup(hard:true) is a no-op (the soft signal already Process.Kill(entireProcessTree:true) and the grace poll gave it the full window), so the post-SIGKILL confirmation poll is now capped at a single 250ms tick on Windows instead of the 5s ConfirmAfterKill window — no added latency for a non-terminating pid. Unix keeps the full poll (where SIGKILL is real and OS-reflection lag is expected).
2. Verbose KillConfirmAsync/PollForConfirmedDeathAsync comments — trimmed to concise intent.
3. Verbose test rationale comments — trimmed in both ClaudeHookStdoutTests and StopAgentPidFallbackTests.

Re-verified locally: AgentOrchestratorVendorTests 84/84, the reaper test 3/3, ClaudeHookStdoutTests 3/3; check-linear-ids.sh ✓.