Add SDK v1.23 agent-server API parity helpers#177
Conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solid implementation of SDK v1.23.0 API parity with comprehensive test coverage. A few code quality improvements would strengthen type safety and consistency.
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/26332377297
| export interface ConfirmationPolicyBase { | ||
| type: string; | ||
| kind?: string; | ||
| type?: string; |
There was a problem hiding this comment.
🟠 Important: Dual naming convention creates ambiguity. ConfirmationPolicyBase has both kind? and type? fields, forcing downstream code to check both with fallbacks like policy.type ?? policy.kind. This suggests an incomplete migration between naming conventions.
Suggestion: Pick one field name and deprecate the other, or document which field takes precedence and when each should be used.
| this._state.confirmationPolicy = policy as ConfirmationPolicy; | ||
| } else { | ||
| // Create a simple wrapper | ||
| const policyType = (policy as ConfirmationPolicyBase).type ?? policy.kind ?? 'never'; |
There was a problem hiding this comment.
🟡 Suggestion: Triple fallback policy.type ?? policy.kind ?? 'never' is fragile and masks the dual-naming inconsistency in ConfirmationPolicyBase. If the base type is fixed to use a single field, this simplifies to a single access.
| type: (policy as ConfirmationPolicyBase).type, | ||
| requiresConfirmation: () => (policy as ConfirmationPolicyBase).type === 'always', | ||
| type: policyType, | ||
| requiresConfirmation: () => policyType === 'always' || policyType === 'AlwaysConfirm', |
There was a problem hiding this comment.
🟡 Suggestion: String literal comparison against both 'always' and 'AlwaysConfirm' suggests another naming inconsistency. Consider defining these as const enums or union types to enforce consistency:
type ConfirmationPolicyKind = 'never' | 'always' | 'NeverConfirm' | 'AlwaysConfirm';Or normalize to a single convention (e.g., always use PascalCase or always use lowercase).
| policyOrRequest: ConfirmationPolicyBase | SetConfirmationPolicyRequest | ||
| ): Promise<Success> { | ||
| const request = | ||
| 'policy' in policyOrRequest ? policyOrRequest : ({ policy: policyOrRequest } as const); |
There was a problem hiding this comment.
🟡 Suggestion: Runtime check 'policy' in policyOrRequest works but bypasses TypeScript's type system. Consider a proper type guard:
function isSetConfirmationPolicyRequest(
value: unknown
): value is SetConfirmationPolicyRequest {
return typeof value === 'object' && value !== null && 'policy' in value;
}This makes the intent clearer and is reusable.
| conversationId: string, | ||
| securityAnalyzerOrRequest: unknown | SetSecurityAnalyzerRequest | null | ||
| ): Promise<Success> { | ||
| const request = isSetSecurityAnalyzerRequest(securityAnalyzerOrRequest) |
There was a problem hiding this comment.
🟡 Suggestion: The type guard isSetSecurityAnalyzerRequest is defined at the bottom of the file (line 292). Consider moving helper functions near the top of the file after imports, or co-locating them with the methods that use them for better discoverability.
| }); | ||
| } | ||
|
|
||
| async loadHooks(request: HooksRequest = {}): Promise<HooksResponse> { |
There was a problem hiding this comment.
🟡 Suggestion: New clients (HooksClient, MCPClient, CloudProxyClient) don't check agent-server version compatibility, unlike WorkspacesClient which calls assertAgentServerSupports(). If these endpoints are v1.23.0+, consider adding version checks or documenting that they'll gracefully fail with 404 on older servers.
| deleted: boolean; | ||
| } | ||
|
|
||
| export type SecretValueResponse = string; |
There was a problem hiding this comment.
🟡 Suggestion: SecretValueResponse = string is just a type alias for string. Unless you plan to make it a richer type later (e.g., { value: string, encrypted: boolean }), using string directly is simpler and reduces indirection.
| * @deprecated v1.23.0 agent servers use `url` and optional `headers`. | ||
| */ | ||
| source?: string; | ||
| /** | ||
| * @deprecated v1.23.0 agent servers use `url` and optional `headers`. | ||
| */ | ||
| key?: string; |
There was a problem hiding this comment.
🟢 Good: Clear deprecation notices for source and key fields in LookupSecret. The JSDoc comments are helpful for developers migrating from older code.
Additional Review Analysis[RISK ASSESSMENT]
This PR changes the request format for Key risk factors:
VERDICT: KEY INSIGHT:
|
neubig
force-pushed
the
codex/sdk-123-api-parity
branch
from
8024197 to
30d09ba
Compare
2 participants
Summary
Verification