Say when an interactive turn has gone silent, and keep the child's stderr

[realtonyyoung]

Closes #715 — AI-2384

What & why

A hosted Gemini turn ran 11m43s producing no transcript events and no error: the CLI was silently retrying a rate-limited model call in ~70s waves, writing the whole diagnosis to stderr while the ACP wire carried nothing. From the launcher it reads as a hang from launch. Two gaps made it invisible, and both are ours:

• AcpChildProcess drained stderr for deadlock safety and logged only line lengths unless the operator had opted into frame debugging, so the vendor's own explanation was thrown away. It now retains a bounded copy (4KB, dropped over cap, daemon-local) the way PiRpcProcess already does.
• Nothing bounded an interactive turn. After three minutes of zero envelopes the runtime now emits one system_note saying the agent is still running and that vendors go quiet this way, and logs a Warning carrying the retained stderr.

Where to look

The note never reaps — a silent turn is usually a retry the vendor wins, and killing it would lose work the user cannot see coming. It is withheld from review flows for a different reason than from reviewer launches with a first-output deadline, and the two conditions are separate on purpose: only some vendors carry that deadline, so deriving one from the other would put a "keep waiting" note into a transcript that is consumed as review output.

Verification

AcpTurnSilenceNoticeTests drives the real factory against FakeAcpAgent with the prompt response held open and a FakeTimeProvider: the note arrives, names the window, the runtime is still alive, a second window produces no second note, and a review-flow launch gets none at all. Daemon unit suite 2884 total, 0 failed. AOT publish of the daemon clean of IL3050/IL2026.

🤖 Generated with Claude Code

[linear-code]

AI-2384

[qodo-code-review]

PR Summary by Qodo

Surface silent ACP turns and retain child stderr

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Retains initial ACP child stderr diagnostics for launch and stalled-turn troubleshooting.
• Emits one non-terminating notice after three silent minutes for interactive turns.
• Excludes review and deadline-supervised flows, with fake-time coverage.

Diagram

graph TD
  TURN["ACP Turn"] --> FLOW{"Notice eligible?"}
  FLOW -->|No| EXISTING["Existing supervision"]
  FLOW -->|Yes| WATCHER["Silence watcher"] -->|Reads| PROCESS["ACP Process"] --> CAPTURE["stderr capture"]
  WATCHER -->|Three minutes quiet| NOTE["System note"]
  WATCHER -->|Diagnostics| LOG["Warning log"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Terminate silent turns

• ➕ Provides a hard upper bound for indefinitely stalled vendor calls.
• ➕ Releases daemon and child-process resources predictably.
• ➖ Destroys work from recoverable vendor retries.
• ➖ Conflicts with the user-facing promise that the agent remains running.

2. Emit periodic heartbeat notices

• ➕ Continually reassures users during very long retries.
• ➕ Makes extended silence duration visible without consulting logs.
• ➖ Can bury useful transcript content with repeated system messages.
• ➖ Adds recurring timers and lifecycle complexity per turn.

Recommendation: Keep the proposed one-shot, non-terminating notice. It explains likely recoverable vendor silence without discarding work or flooding transcripts, while existing deadlines continue governing reviewer launches.

Files changed (5) +259 / -0

Enhancement (1) +6 / -0

IAcpProcess.csExpose optional child-process diagnostics +6/-0

Expose optional child-process diagnostics

• Adds a default nullable diagnostics property so runtimes can inspect child stderr without requiring every process double to implement storage.

src/Capacitor.Cli.Daemon/Acp/IAcpProcess.cs

Bug fix (3) +96 / -0

AcpChildProcess.csRetain initial ACP child stderr diagnostics +27/-0

Retain initial ACP child stderr diagnostics

• Captures non-empty stderr lines independently of debug-frame logging and exposes the retained text safely across threads. Capture stops after reaching the diagnostic threshold so chatty long-lived children cannot continuously grow daemon memory.

src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs

AcpHostedAgentRuntime.csNotify users when interactive ACP turns remain silent +65/-0

Notify users when interactive ACP turns remain silent

• Counts emitted envelopes and arms a one-shot three-minute watcher around admitted turns. Eligible silent turns produce a system note and warning with retained stderr while leaving the child alive; review and first-output-deadline launches are excluded.

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

AcpHostedAgentRuntimeFactory.csPass review-flow status into ACP runtime supervision +4/-0

Pass review-flow status into ACP runtime supervision

• Propagates review-flow identity separately from the vendor-specific first-output deadline, preventing silence notices from contaminating review output.

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

Tests (1) +157 / -0

AcpTurnSilenceNoticeTests.csCover one-shot silence notices and review exclusions +157/-0

Cover one-shot silence notices and review exclusions

• Adds a factory-level fake-agent harness with controlled prompt completion and fake time. Tests verify the three-minute notice, continued process lifetime, no repeated notice, and suppression for review flows.

test/Capacitor.Cli.Daemon.Tests.Unit/Services/AcpTurnSilenceNoticeTests.cs

[qodo-code-review]

Code Review by Qodo

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

1. Ended turns emit silence notes ✓ Resolved 🐞 Bug ≡ Correctness

Description

ArmTurnSilenceNotice returns a CancellationTokenSource as an IDisposable, but disposing a CTS
does not cancel its token, so the watcher survives after SendPromptAsync completes. If no later
envelope changes the counter, it emits a false warning and transcript note three minutes after an
already-finished turn, potentially claiming an idle or exited agent is still running.

Code

src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[R2431-2434]

+        var cts = new CancellationTokenSource();
+        _ = WatchForTurnSilenceAsync(cts.Token);
+
+        return cts;

Evidence

The watcher exits early only when its token is canceled, while the turn scope merely disposes the
returned CTS. The turn's existing finally block explicitly describes the point where the turn is
settled, but no watcher cancellation occurs there.

src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[1290-1320]
src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[2428-2444]

Agent prompt

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

## Issue description
The turn-scoped `using` only disposes the `CancellationTokenSource`; it never cancels the delay, allowing a completed turn to emit a later false silence notice.

## Issue Context
Return a disposable registration whose `Dispose` cancels and then disposes the CTS, or explicitly cancel the watcher in the turn's `finally` block. Add a test that completes a silent prompt before three minutes, advances fake time past the window, and verifies no note is emitted.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[1290-1320]
- src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[2428-2444]
- test/Capacitor.Cli.Daemon.Tests.Unit/Services/AcpTurnSilenceNoticeTests.cs[100-156]

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

---

2. Warning bypasses stderr privacy gate ✓ Resolved 🐞 Bug ⛨ Security

Description

The silence path writes captured stderr verbatim at Warning even when KCAP_ACP_DEBUG_FRAMES is
disabled, although the drain code explicitly says stderr may contain paths, prompt fragments, or
error details and should be logged verbatim only after opt-in. Any silent turn can therefore copy
sensitive child output into normal daemon logs despite the configured privacy gate.

Code

src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[R2775-2776]

+    [LoggerMessage(Level = LogLevel.Warning, Message = "ACP turn for agent {AgentId} (vendor={Vendor}) has produced nothing for {Minutes} minutes; the child is still running. Its stderr so far: {Diagnostics}")]
+    partial void LogTurnSilent(string agentId, string vendor, double minutes, string diagnostics);

Evidence

The stderr drain documents and enforces an opt-in for verbatim logging because the data can be
sensitive, but the new silence warning receives the full Process.Diagnostics string and logs it at
Warning without checking that option.

src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[45-49]
src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[79-91]
src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[2448-2457]
src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[2775-2776]

Agent prompt

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

## Issue description
The new Warning log emits full stderr regardless of the debug-frames opt-in, bypassing the existing privacy control for verbatim child output.

## Issue Context
Keep diagnostics available in bounded daemon memory, but only place full text in logs when the same explicit debug setting permits it. In the default path, log a non-sensitive summary such as captured length or a safely redacted/classified reason.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[45-49]
- src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[79-91]
- src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[2448-2457]
- src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[2775-2776]

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

---

3. Capture comment narrates history ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The new comment preserves a past Gemini incident and change narrative (`reached a live daemon and
was thrown away`) rather than only documenting the current retention constraint. This violates the
requirement that comments remain understandable without historical review context.

Code

src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[R79-82]

+                // Retained whatever the log level does: the operator opts into per-line logging, not
+                // into being able to explain a stall after the fact, and the vendor writes its whole
+                // diagnosis here (gemini's "Attempt N failed: RATE_LIMIT_EXCEEDED … Retrying after
+                // 70650ms" reached a live daemon and was thrown away). Daemon-local only.

Evidence

PR Compliance ID 24 prohibits comments that narrate code evolution or preserve unnecessary
historical detail. The added comment quotes a specific Gemini retry and says it `reached a live
daemon and was thrown away`, which describes the motivating incident and former behavior rather than
only the present invariant.

CLAUDE.md: Comments Must Explain Current Non-Obvious Constraints Only
src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[79-82]

Agent prompt

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

## Issue description
The `Capture` call comment narrates a past Gemini incident and the previous behavior instead of stating only the current non-obvious constraint.

## Issue Context
Keep the useful rationale that stderr diagnostics must be retained independently of per-line logging, but remove the historical incident quotation and change narration.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[79-82]

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

---

4. Later stall diagnostics are dropped ✓ Resolved 🐞 Bug ◔ Observability

Description

The process-wide capture permanently stops accepting stderr once its first 4 KB is full, so a
long-lived child that logged earlier activity cannot retain a retry or authentication explanation
produced by a later silent turn. The silence warning will then print stale startup/previous-turn
text instead of the diagnostics this feature is intended to preserve.

Code

src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[R102-106]

+    void Capture(string line) {
+        lock (_diagnosticsGate) {
+            if (_diagnostics.Length >= DiagnosticsCap) return;
+
+            _diagnostics.Append(line).Append('\n');

Evidence

The code describes the child as long-lived and explicitly drops every line after the process-wide
buffer reaches its cap, while the silence consumer later treats that buffer as the stalled turn's
stderr. Thus diagnostics generated during a later turn are deterministically absent once earlier
output filled the buffer.

src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[35-39]
src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[102-107]
src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[2448-2457]

Agent prompt

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

## Issue description
A first-only process-lifetime buffer drops all diagnostics generated after it fills, including the retry message associated with a later stalled turn.

## Issue Context
Preserve the startup diagnostics needed for launch failures separately from a bounded rolling or turn-scoped recent-stderr buffer used by silence warnings. Keep both stores strictly bounded and thread-safe.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[30-42]
- src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[79-106]
- src/Capacitor.Cli.Daemon/Services/AcpHostedAgentRuntime.cs[2448-2457]

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

---

5. Diagnostics exceed capture cap ✓ Resolved 🐞 Bug ☼ Reliability

Description

Capture checks only the existing buffer length and then appends the entire incoming line, so one
long stderr line can retain far more than the advertised 4096 characters. A chatty or malformed
child can therefore leave a large string resident for the session despite this feature's
bounded-memory contract.

Code

src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[R104-106]

+            if (_diagnostics.Length >= DiagnosticsCap) return;
+
+            _diagnostics.Append(line).Append('\n');

Evidence

The class defines a 4096-character cap and the interface promises a bounded capture, but the append
operation has no per-line truncation or remaining-capacity calculation.

src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[35-42]
src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[102-107]
src/Capacitor.Cli.Daemon/Acp/IAcpProcess.cs[26-30]

Agent prompt

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

## Issue description
The diagnostics buffer can exceed `DiagnosticsCap` because an incoming line is appended without regard to the remaining capacity.

## Issue Context
Calculate the remaining capacity and append at most that many characters, accounting for the newline only when space remains. Add coverage with a single line larger than 4096 characters and assert the exposed diagnostics never exceeds the cap.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[35-42]
- src/Capacitor.Cli.Daemon/Acp/AcpChildProcess.cs[102-107]

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

---

Context sources

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[realtonyyoung]

Codex review flow — three findings, all taken.

• Cap checked before the append. Correct: one line longer than 4 KB was retained whole. Now truncated to the remaining room. The same defect is in PiRpcProcess.Capture, which this capture was modelled on — filed as PiRpcProcess diagnostics cap is checked before the append, so one long line escapes it #728 rather than reached into from here.
• A completed turn's watcher could still publish. Correct, and the token alone could not close it: the delay can complete and queue its continuation while the turn is ending. The watcher now re-checks the in-flight registration under _reconnectLock and identity-compares the turn it was armed for, so a note can only describe a turn that is still running.
• Silence measured over every envelope. Correct. The counter is now named positively — assistant text/thinking, tool calls and results, plans — so usage and session-info envelopes, which reach EmitEnvelope with no turn in flight at all, cannot mask a silent turn, and a metadata kind added later cannot start masking one by accident.

Daemon unit suite 2884 total, 0 failed; AOT clean.