fix: expose constructor headers via Client.headers property

[thomasht86]

Summary

• Headers passed to Client() constructor were not accessible via .headers property (always returned empty dict)
• Root cause: constructor headers were used for default_headers() but self.headers was initialized empty
• Fix: Store the constructor headers in self.headers instead of creating empty HeaderMap

Test plan

• Added test_constructor_headers_accessible - verifies headers passed to constructor are accessible
• Added test_constructor_headers_with_cookies - verifies cookies are excluded from headers getter
• Added test_constructor_cookies_only - verifies cookies-only constructor doesn't expose Cookie header
• Added test_setter_overwrites_constructor_headers - verifies setter correctly overwrites constructor headers
• All 112 unit tests pass

🤖 Generated with Claude Code

[claude]

Pull Request Review

Summary

This PR fixes a bug where headers passed to the Client() constructor were not accessible via the .headers property. The fix correctly stores constructor headers in self.headers instead of initializing an empty HeaderMap.

---

✅ Code Quality & Best Practices

Strengths:

• Clean fix: The solution is minimal and directly addresses the root cause
• Proper cloning: Uses .clone() at line 159 (src/lib.rs:159) to preserve headers for both default_headers() and the instance field
• Maintains immutability: Doesn't modify the existing headers_headermap after cloning
• Comprehensive test coverage: 4 well-designed tests covering different scenarios

Test Coverage Assessment:

• ✅ test_constructor_headers_accessible: Verifies basic functionality
• ✅ test_constructor_headers_with_cookies: Confirms Cookie header exclusion behavior
• ✅ test_constructor_cookies_only: Edge case for cookies-only initialization
• ✅ test_setter_overwrites_constructor_headers: Validates setter behavior

---

🔍 Potential Issues

1. Minor Memory Overhead (Low Priority)
The .clone() at src/lib.rs:159 creates a copy of the HeaderMap to store in both:

• client_builder.default_headers() (inside the reqwest Client)
• self.headers (instance field)

This is necessary but worth noting. The reqwest client already stores default headers internally, so we're storing headers twice. However, this is the correct approach given that:

• Users expect to read/modify headers via .headers property
• The reqwest client's headers are encapsulated and not accessible
• The performance impact is negligible for typical header counts

2. Test Consistency (Suggestion)
The test test_setter_overwrites_constructor_headers verifies setter behavior, but there's no test ensuring that constructor headers are actually sent in requests. Consider adding an integration test like:

def test_constructor_headers_sent_in_request(base_url_ssl, ca_bundle):
    """Verify that constructor headers are actually sent in HTTP requests."""
    client = httpr.Client(
        headers={"X-Custom-Header": "test-value"},
        ca_cert_file=ca_bundle
    )
    response = client.get(f"{base_url_ssl}/headers")
    assert response.json()["headers"]["X-Custom-Header"] == "test-value"
    client.close()

This would verify the end-to-end flow from constructor → stored headers → request headers.

---

🔒 Security Considerations

✅ No security concerns identified:

• Cookie exclusion from .headers getter is preserved (correctly implemented at src/lib.rs:257)
• No new attack surface introduced
• Headers are properly validated through existing to_headermap() conversion

---

⚡ Performance Considerations

✅ Performance impact is minimal:

• One additional .clone() operation during client initialization (one-time cost)
• No impact on per-request performance
• The clone happens on a HeaderMap which is typically small (< 20 headers)

---

🧪 Testing Recommendation

The tests are well-structured and pass (per PR description). Consider adding:

1. An integration test verifying headers are sent in actual HTTP requests (mentioned above)
2. A test for the edge case where headers are set to None or empty dict in constructor

---

📝 Documentation

The code changes are self-documenting, but consider:

• No docstring updates needed (behavior was always intended to work this way)
• The fix restores expected behavior rather than adding new functionality

---

✅ Final Verdict

LGTM with minor suggestions

This is a solid bug fix that:

• ✅ Correctly addresses the root cause
• ✅ Maintains backward compatibility
• ✅ Has comprehensive test coverage
• ✅ Introduces no security or performance issues
• ✅ Follows the codebase conventions (per CLAUDE.md)

Recommended Actions:

1. Optional: Add integration test for headers being sent in requests
2. Merge once CI passes

Great work identifying and fixing this bug! 🎉