@codex review

Codex Review: Didn't find any major issues. Delightful!

@claude review

Claude finished @farion1231's task in 1m 51s —— View job

Code Review

• Gather context and read changed files
• Analyze code for bugs, security issues, and correctness
• Post review feedback

File: src-tauri/src/proxy/providers/streaming.rs

Summary

The PR introduces two helper functions (default_anthropic_usage_json and build_message_delta_event) to eliminate duplicated inline message_delta JSON construction at two call sites, and ensures the usage field is always present in message_delta events even when upstream providers don't send usage data.

Positive Observations

• Clean DRY refactor. The two call sites that built message_delta inline were identical; extracting them into build_message_delta_event is a clear improvement.
• Correct fix for the core issue. Previously, if usage_json was None, the message_delta event was emitted without a usage key. Downstream Anthropic-protocol consumers (e.g., Claude Code) expect usage to always be present; this fix makes that guarantee hold.
• Good test coverage. test_message_delta_includes_zero_usage_when_stream_has_no_usage correctly exercises a realistic tool-call stream that produces no usage payload, and asserts all three relevant properties (event count, stop_reason, and both token fields).

Issues and Observations

1. Semantic ambiguity: zero tokens vs. unknown usage

default_anthropic_usage_json returns { "input_tokens": 0, "output_tokens": 0 }. This is indistinguishable from a response that genuinely used zero tokens. Downstream consumers tracking cost or billing will silently undercount rather than being able to distinguish "no data available." This is likely an acceptable tradeoff given that the upstream simply didn't send usage data, but it's worth documenting or logging:

fn default_anthropic_usage_json() -> Value {
    // Upstream did not report usage; fall back to zeros to satisfy the protocol.
    log::debug!("[Claude/OpenRouter] No usage in stream; emitting zero-usage fallback");
    json!({ "input_tokens": 0, "output_tokens": 0 })
}

2. The .filter(|usage| usage.is_object()) guard is effectively dead code

usage_json at the call sites is always Option<Value> produced by build_anthropic_usage_json, which always returns a json!({...}) object. So is_object() will always be true when Some. The filter never fires in practice and slightly obscures intent. Either remove it (it adds no protection) or add a comment explaining what non-object case you're guarding against:

// Lines 123-125
let usage = usage_json
    .filter(|usage| usage.is_object())  // <- always true in practice
    .unwrap_or_else(default_anthropic_usage_json);

3. latest_usage is not used as an intermediate fallback

The stream tracks latest_usage (updated on every chunk), and separately updates the pending_message_delta's usage field when a chunk arrives with usage data. The new fallback to zeros skips latest_usage entirely. In the common case both are equivalent, but if the usage arrived on an earlier chunk before the finish_reason chunk set pending_message_delta, there's a path where latest_usage is Some but pending_message_delta's usage field remains None. Looking at the existing logic around line 212-214:

if let Some(usage_json) = &chunk_usage_json {
    latest_usage = Some(usage_json.clone());
    if let Some((_, pending_usage)) = pending_message_delta.as_mut() {
        *pending_usage = Some(usage_json.clone());
    }
}

This only updates pending_message_delta's usage if the pending_message_delta tuple already exists (i.e., a finish_reason was already seen). If usage arrives on the same chunk as finish_reason, both get set correctly. But if usage arrives on a later chunk (after finish_reason), the pending_message_delta usage IS updated. If usage arrives on an earlier chunk (before finish_reason), latest_usage captures it but pending_message_delta doesn't exist yet to be updated—so it will have None usage and fall back to zeros even though token data was available. This is a pre-existing issue, not introduced by this PR, but the new fallback makes it silently lose real data in that edge case.

4. Test model name is fictional

Minor: the test uses "gpt-5.5" which doesn't exist. "gpt-test" or "test-model" would make the test intent clearer and avoid reader confusion.

Overall Assessment

The fix is correct for its stated goal and the refactoring is solid. Issues 1 and 4 are minor/informational. Issue 2 is a dead-code nitpick. Issue 3 is a pre-existing edge case worth being aware of but is out of scope for this PR.

The PR is good to merge. Issues 1 and 3 could be addressed as follow-up if needed.

@claude review

(Re-running with the updated review prompt for comparison.)

Claude finished @farion1231's task in 1m 5s —— View job

Code Review (re-run with updated prompt)

• Read changed file and diff
• Analyze for high-signal issues
• Post review

LGTM.

Small, well-scoped change: extracts build_message_delta_event to dedupe two identical inline blocks and guarantees message_delta.usage is always present (zeros when upstream didn't report usage). Regression test exercises the tool-call-without-usage path and checks the relevant fields.

No blocking issues found under the strict criteria. Items raised in the prior re-review (zero-vs-unknown semantics, the always-true is_object() filter, the latest_usage ordering edge case, fictional test model name) are sub-threshold — style, dead code, or pre-existing — and not worth blocking the merge.
• fix/streaming-missing-usage

@codex review

Codex Review: Didn't find any major issues. 🎉