fix(agent): auto-approve CLI tool permissions & improve error visibility - #321

@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

Claude finished @axisrow's task in 1m 39s —— View job

Code Review

• Read changed files
• Analyze for critical issues
• Post review findings

Overall the changes are correct and address real bugs. No critical security vulnerabilities or data loss risks. Two minor issues worth noting:

Issue 1 — _api_request_count not reset between retry attempts (minor)

src/agent/manager.py:434 — _api_request_count is initialized once before the retry loop (line 571). On a retry, the counter continues from where it left off. So if attempt #1 emitted 3 API request statuses and then timed out, attempt #2 would show "API #4", "API #5", etc. instead of starting from "API #1".

Not a functional bug, but could confuse diagnostics.

Fix this →

Issue 2 — [api:request] early return silently drops is_error (theoretical, practically safe)

src/agent/manager.py:483-492 — The early return for [api:request] lines exits _on_stderr before the is_error surfacing block runs. This means if an [api:request] line were also classified as an error (e.g., contained [error]), it would be logged but not surfaced to the user's queue.

In practice, [api:request] lines are always [INFO] level, so is_error would always be False here. Practically a non-issue, but the code is slightly misleading — it reads as if the early return might be skipping important work.

_auto_approve_tool design — intentional, no issue

The function unconditionally returns PermissionResultAllow() regardless of tool_name/tool_input. This is correct given that:

1. The allowed_tools list passed to ClaudeAgentOptions already restricts which tools Claude can call.
2. When no gate is active, filter_allowed_tools() narrows the list further.
3. can_use_tool is a secondary CLI runtime check that was crashing the tool call. Auto-approving here does not widen permissions beyond what allowed_tools already grants.

The comment in the function documents this rationale well.

UserMessage handling — correct

Updating _last_activity and doing nothing else is the right approach. UserMessage signals the SDK sent tool results back to Claude — it's a heartbeat, not a displayable event.

Summary: No bugs that could break functionality in production, no security issues, no data loss risks. The two issues above are cosmetic/theoretical. The core fixes (can_use_tool callback, UserMessage heartbeat, deduplicated API counter) are sound.

Re: Issue 2 ([api:request] early return) — SKIP. [api:request] lines are always [INFO] level status events from the SDK, never error lines. The early return is intentional (unique counter, must not be deduped). The theoretical risk of dropping a real error here is negligible.

@claude review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

Claude finished @axisrow's task in 1m 35s —— View job

Code Review

• Read changed files
• Analyze for critical issues
• Post review findings

The previous review found two issues. Here's the current status:

Issue 1 — _api_request_count not reset on retry: ✅ Fixed in eca9542. _api_request_count[0] = 0 is now at line 581, inside the for attempt in range(2) loop, so each retry starts the counter fresh.

Issue 2 — [api:request] early return drops is_error block: ✅ Accepted as SKIP by author — [api:request] lines are always [INFO] level, is_error would never be True here.

No critical issues found in the current diff

All four changes are correct:

• _auto_approve_tool (manager.py:354): correctly auto-approves CLI can_use_tool requests. Safe because allowed_tools passed to ClaudeAgentOptions is the real permission gate — this callback is only a secondary CLI runtime check that was crashing tool execution.

• _api_request_count counter (manager.py:481–489): early-return path is correct. Each [api:request] event gets a unique numbered label, bypassing the deduplication guard (_last_emitted). Reset at line 581 per retry.

• UserMessage handling (manager.py:726): updating _last_activity[0] is the right behavior — it's a heartbeat signaling tool results were forwarded to Claude, not a displayable event.

• Unhandled message logging (manager.py:751): warning-level log is appropriate for diagnostics.

Tests

Three new tests cover all three behavioral changes. Test logic is sound:

• test_chat_stream_options_have_can_use_tool — asserts can_use_tool is set on options
• test_stderr_api_request_counter_not_deduped — verifies 3 unique API #N status labels
• test_user_message_does_not_break_stream — verifies stream completes with text and done when UserMessage is interspersed

The filter "text" in p and "type" not in p in the last test is correct — text-delta payloads in this codebase are emitted as {"text": "..."} (no type key, see manager.py:690), unlike status/tool events.

No blocking issues. Ready to merge.