Conversation
|
Claude finished @binoy14's task in 1m 4s —— View job Code Review Complete
Review FindingsThe PR successfully fixes the token lookup logic. The code and tests look solid with 100% coverage. No issues found that require fixing. The fix correctly addresses the problem where Test coverage is comprehensive and includes all critical paths including edge cases for both client types, error scenarios, and middleware behavior. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
caf890c to
b5ecfcf
Compare
rexxars
left a comment
There was a problem hiding this comment.
To me this technically already "works as intended" - sometimes you want to send unauthenticated requests, and with this that is no longer a possibility, I think?
In a way yes, but this is different from the current CLI logic and it breaks the telemetry calls. The problem is that telemetry has |
getCliClient()

Fixes issue with the token and requireUser logic which broke telemetry reporting (mostly).
The issue was that if you have
requireUser: falsethen it does not look up stored token and without token API call to check telemetry consent fails. This fixes that logic to use provided or look it up always.Also added extensive tests for apiClient