Skip to content

Conversation

@ltawfik
Copy link
Collaborator

@ltawfik ltawfik commented Jun 17, 2025

Summary

  • Makes cost_usd and total_cost_usd fields optional in ResultMessage
  • Updates client to use safe dictionary access with .get()
  • Adds fallback logic for field name compatibility

Problem

Users are experiencing KeyError and validation errors when using the Python SDK because:

  1. The CLI doesn't always include cost_usd in its response
  2. The field name changed from total_cost to total_cost_usd
  3. These fields were marked as required in the SDK

Solution

This PR makes both cost fields optional with None defaults and uses safe dictionary access to handle missing fields gracefully.

Test plan

  • Verify the sample chat app runs without errors
  • Test with responses that include/exclude cost fields
  • Confirm compatibility with both field names

🤖 Generated with Claude Code

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a thing anymore -- the field is total_cost_usd

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude can you kill the cost_usd fields and replace them with total_cost_usd?

The CLI only sends total_cost_usd, not cost_usd. This PR:

- Removes the non-existent cost_usd field from ResultMessage
- Makes total_cost_usd optional since it may be missing
- Handles GeneratorExit gracefully in subprocess transport
- Removes unnecessary cancel scope call

Fixes the error users were experiencing:
- KeyError: 'cost_usd'
- Field required [type=missing, input_value=...]

The anyio cancel scope error still appears in logs but doesn't affect functionality.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@ltawfik ltawfik force-pushed the fix-optional-cost-fields branch from 2444c44 to 6cc5b34 Compare June 17, 2025 23:39
@ltawfik ltawfik requested a review from bcherny June 17, 2025 23:40
- Remove cost_usd field from test expectations
- Update to use total_cost_usd instead of total_cost
- All 30 tests now pass

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@claude
Copy link
Contributor

claude bot commented Jun 18, 2025

Claude finished @ltawfik's task —— View job


PR Review: Fix Optional Cost Fields

Todo List:

  • Read repository guidelines (CLAUDE.md) - No repo-specific guidelines found
  • Analyze changed files and implementation approach
  • Check handling of cost field optionality
  • Address bcherny's review comment about cost_usd vs total_cost_usd
  • Evaluate test coverage
  • Provide comprehensive code review feedback

Code Review Summary

This PR addresses an important compatibility issue by making cost fields optional, but there's one critical issue that needs to be fixed.

🚨 Critical Issue

examples/quick_start.py:63 - Still references message.cost_usd instead of message.total_cost_usd

# Current (incorrect):
elif isinstance(message, ResultMessage) and message.cost_usd > 0:

# Should be:
elif isinstance(message, ResultMessage) and message.total_cost_usd and message.total_cost_usd > 0:

✅ Code Quality & Best Practices

Good implementations:

  1. Safe dictionary access in client.py:91 - Uses data.get("total_cost_usd") to handle missing fields gracefully
  2. Proper type annotations - total_cost_usd: float | None = None correctly makes the field optional
  3. Consistent field handling - All cost-related code now uses safe access patterns

✅ Bug Prevention

The changes successfully prevent KeyError exceptions by:

  • Making total_cost_usd optional with None default in ResultMessage
  • Using .get() method for safe dictionary access
  • Proper None checking in the client code

✅ Performance Considerations

  • No performance impact - the changes are minimal and use efficient dictionary access
  • Optional fields with defaults don't add overhead

✅ Security Concerns

  • No security issues identified
  • The changes are defensive programming that improve robustness

✅ Test Coverage

The test suite has been properly updated:

  • test_client.py:96 - Mock data includes total_cost_usd field
  • test_integration.py:51,73,116 - All test scenarios use the correct field name
  • test_types.py:53 - Type tests verify the optional field works correctly

📋 Workflow Files

The added GitHub workflows are well-configured:

  • claude-code-review.yml - Automated PR reviews
  • claude.yml - Manual trigger for @claude mentions

Recommendation

APPROVE with one required fix: Update examples/quick_start.py:63 to use total_cost_usd instead of cost_usd and add proper None checking.

After that fix, this PR will successfully resolve the validation errors while maintaining backward compatibility.


@ltawfik ltawfik force-pushed the fix-optional-cost-fields branch from 732e15c to b3448f4 Compare June 18, 2025 00:01
@bcherny bcherny changed the title Fix: Make cost fields optional to prevent validation errors Fix: Remove unused cost_usd field Jun 18, 2025
@ltawfik ltawfik merged commit 9b1387a into main Jun 18, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants