code-coach review: ReActActor guard rollout (P0-1) — 5 advisory suggestions

[psmon]

Source

• Pre-commit code-coach Mode 2 review on commit pending (ReActActor guard 3-set + 5 Lite refinements)
• Adoption spec: Docs/agent-origin/03-adoption-recommendations.md §P0-1
• Full review log: harness/logs/code-coach/2026-04-27-1545-precommit-react-actor-guards.md

Verdict

Commit was approved — 0 must-fix, 0 should-fix. The 5 items below are advisory suggestions that did not block the merge but warrant follow-up. Per harness policy (code-coach review yielding Suggestion or higher gets tracked as an issue), filing here for visibility.

Suggestions

S-1 — Property accessor pattern consistency

File: Project/ZeroCommon/Llm/Tools/ToolLoopGuards.cs:31-32

BlockedRepeats uses { get; private set; } while LlmRetries and ConsecutiveBlocks use backing field + expression-bodied getter. All three are externally read-only and modified through a single internal entry point.

Proposed:
```csharp
private int _blockedRepeats;
public int BlockedRepeats => _blockedRepeats;
// CheckRepeat: _blockedRepeats++;
```

Behavioral diff: 0. Pure consistency cleanup.

S-2 — Recent-buffer collection choice

File: Project/ZeroCommon/Llm/Tools/ToolLoopGuards.cs:32

LinkedList<RecentAttempt> chosen for O(1) RemoveFirst(). With RecentBufferSize = 5 the linked-list node overhead (~80 B/node) outweighs the O(n=5) shift of List<>.RemoveAt(0) or Queue<>.Dequeue().

Proposed:
```csharp
private readonly Queue _recent = new();
// AddLast → Enqueue, RemoveFirst → Dequeue
```

Micro-optimization scale; correctness unchanged.

S-3 — Switch-style transient classifier

File: Project/ZeroCommon/Llm/Tools/ToolLoopGuards.cs:118-129

Cleaner with a list pattern:

```csharp
if (ex is HttpRequestException { StatusCode: var sc }
&& sc is HttpStatusCode.RequestTimeout
or HttpStatusCode.TooManyRequests
or HttpStatusCode.BadGateway
or HttpStatusCode.ServiceUnavailable
or HttpStatusCode.GatewayTimeout)
return true;
```

Same behavior, less ceremony. Still falls through to message-keyword check below for typed-less exceptions.

S-5 — Document session-scope guard intent

File: Project/ZeroCommon/Llm/Tools/AgentToolLoop.cs:73-76

AgentToolLoop reuses one instance across multiple RunAsync calls (via UserSendCount). A new ToolLoopGuards per call is intentional — each user request starts with a clean repeat counter so a long-running workspace session doesn't accumulate false positives across unrelated tasks. The intent isn't obvious from the code alone.

Proposed (1-2 line comment):
```csharp
// New guards per RunAsync so each user request starts with a clean
// repeat counter — long-running sessions don't accumulate false
// positives across unrelated tasks.
var guards = new ToolLoopGuards();
```

Highest ROI of all suggestions — single-line doc fix, prevents future debugging confusion.

S-6 — Integration test coverage (follow-up task)

File: Project/ZeroCommon.Tests/AgentToolLoopTests.cs (new)

Current tests exercise ToolLoopGuards standalone (24 unit cases). No end-to-end check that AgentToolLoop + MockAgentToolHost triggers the guard under a real-but-mocked LLM.

Proposed — add 1-2 [SkippableFact] cases:

• 5 forced `read_terminal` calls → `session.FailureReason` contains "blocked"
• `session.GuardStats.BlockedRepeats > 0`

LLM mock setup is non-trivial (force model to repeat exact call); track separately.

Recommendation

Apply S-5 first (single-line, no test re-run needed), defer S-1 / S-2 / S-3 to a future cleanup PR, and split S-6 into a dedicated testing task once the LLM-mock harness is available.

Closes-when

This issue closes when:

• S-1, S-2, S-3, S-5 applied (or explicitly declined with reason)
• S-6 promoted to its own task with a tracking issue