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.
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Implementation is clean and follows existing patterns. Previous test coverage gap has been addressed. One minor type consistency issue remains.
[TESTING COVERAGE]
✅ All 5 methods now have test coverage (getOpenAISubscriptionModels test added)
✅ Token non-exposure verified
✅ Endpoint paths verified
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Adding new client methods to existing LLMMetadataClient with comprehensive test coverage. No breaking changes to existing functionality. Standard CRUD operations following established patterns.
VERDICT:
✅ Worth merging: Core implementation is solid, test coverage complete, only minor type style improvement suggested
KEY INSIGHT:
Previous review feedback has been addressed - missing test coverage added and type definitions improved from number | string | null to number | null.
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/26410852540
| device_code: string; | ||
| user_code: string; | ||
| verification_uri: string; | ||
| verification_uri_complete: string | null; |
There was a problem hiding this comment.
🟡 Suggestion: Use optional field syntax for idiomatic TypeScript.
Change:
verification_uri_complete: string | null;To:
verification_uri_complete?: string;Per OAuth 2.0 Device Authorization Grant (RFC 8628), verification_uri_complete is an optional field. TypeScript convention is to use ? for optional fields rather than | null. This makes the type more idiomatic and clearer to consumers.
Note: If the backend API explicitly returns null (rather than omitting the field), the current typing is technically correct but less idiomatic.
| device_code: string; | ||
| user_code: string; | ||
| verification_uri: string; | ||
| verification_uri_complete: string | null; |
There was a problem hiding this comment.
🟡 Suggestion: Use optional field syntax for idiomatic TypeScript.
Change:
verification_uri_complete: string | null;To:
verification_uri_complete?: string;Per OAuth 2.0 Device Authorization Grant (RFC 8628), verification_uri_complete is an optional field. TypeScript convention is to use ? for optional fields rather than | null. This makes the type more idiomatic and clearer to consumers.
Note: If the backend API explicitly returns null (rather than omitting the field), the current typing is technically correct but less idiomatic.
Summary
Tests
This PR was created by an AI agent (OpenHands) on behalf of the user.