Add LLM subscription client methods#178
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Implementation is clean and follows existing patterns, but has missing test coverage and type inconsistencies that should be addressed.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/typescript-client/actions/runs/26336719042
| ); | ||
| }); | ||
|
|
||
| it('LLMMetadataClient calls OpenAI subscription endpoints without exposing tokens', async () => { |
There was a problem hiding this comment.
🟠 Important: Missing test coverage for getOpenAISubscriptionModels().
The implementation adds 5 methods but this test only exercises 4. Add a test case for getOpenAISubscriptionModels() to verify it correctly calls GET /api/llm/subscription/openai/models and returns the models array.
| it('LLMMetadataClient calls OpenAI subscription endpoints without exposing tokens', async () => { | |
| it('LLMMetadataClient.getOpenAISubscriptionModels() returns models array', async () => { | |
| global.fetch = jest.fn().mockResolvedValue( | |
| new Response(JSON.stringify({ vendor: 'openai', models: ['gpt-4', 'gpt-3.5-turbo'] }), { | |
| status: 200, | |
| headers: { 'content-type': 'application/json' }, | |
| }) | |
| ); | |
| const client = new LLMMetadataClient({ host: 'http://example.com', apiKey: 'secret' }); | |
| const models = await client.getOpenAISubscriptionModels(); | |
| expect(models).toEqual(['gpt-4', 'gpt-3.5-turbo']); | |
| expect(global.fetch).toHaveBeenCalledWith( | |
| 'http://example.com/api/llm/subscription/openai/models', | |
| expect.objectContaining({ method: 'GET' }) | |
| ); | |
| }); | |
| it('LLMMetadataClient calls OpenAI subscription endpoints without exposing tokens', async () => { |
| export interface LLMSubscriptionStatusResponse { | ||
| vendor: string; | ||
| connected: boolean; | ||
| account_email?: string | null; | ||
| expires_at?: number | string | null; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Type inconsistencies in optional fields.
-
expires_at: number | string | null- Mixing number and string types creates ambiguity. Pick one:- If it's always a timestamp milliseconds:
expires_at?: number | null - If it's always an ISO date string:
expires_at?: string | null
- If it's always a timestamp milliseconds:
-
Optional + nullable redundancy - Using both
?:and| nullis redundant:account_email?: string | nullmeans "optional, and when present can be null or string"- Better:
account_email: string | null(always present but nullable) ORaccount_email?: string(optional but never explicitly null)
Check what the agent-server actually returns and match that exactly.
| export interface LLMSubscriptionDeviceStartResponse { | ||
| device_code: string; | ||
| user_code: string; | ||
| verification_uri: string; | ||
| verification_uri_complete?: string | null; | ||
| expires_at: number | string; | ||
| interval_seconds: number; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Same type inconsistency as above.
verification_uri_complete?: string | null- Choose one: optional OR nullable, not bothexpires_at: number | string- Pick a single type (number for timestamp milliseconds, or string for ISO date)
Consistency across these subscription response types will prevent runtime type confusion.
Summary
Tests
This PR was created by an AI agent (OpenHands) on behalf of the user.