feat: add Python SDK, OpenAPI spec, and build tooling#420
feat: add Python SDK, OpenAPI spec, and build tooling#420KooshaPari wants to merge 1 commit intomainfrom
Conversation
…cleanups - Add api/openapi.yaml — OpenAPI spec for core endpoints - Add .github/workflows/generate-sdks.yaml — Python/TypeScript SDK generation - Add sdk/python/cliproxy/api.py — comprehensive Python SDK with native classes - Update .gitignore — add build artifacts (cliproxyapi++, .air/, logs/) Cherry-picked from fix/test-cleanups (commits a4e4c2b, ad78f86, 05242f0) before closing superseded PR #409. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Summary of ChangesHello @KooshaPari, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to integrate valuable additions from a previously unmergeable PR, focusing on enhancing the developer experience. It primarily introduces a comprehensive Python SDK, providing native Python classes for seamless API interaction. Additionally, it updates the .gitignore file to properly manage build artifacts. While the PR description also mentions the addition of an OpenAPI specification and an SDK generation workflow, these specific file changes are not included in the provided patch content. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Python SDK, an OpenAPI specification, and updates to build tooling. The Python SDK is a valuable addition, but it has some areas for improvement regarding type safety, security, and maintainability. A critical issue was found in .gitignore where merge conflict markers have been included, which must be resolved before merging.
| <<<<<<< HEAD | ||
| ======= | ||
| server | ||
| cli-proxy-api-plus-integration-test | ||
|
|
||
| boardsync | ||
| releasebatch | ||
| .cache | ||
| >>>>>>> a4e4c2b8 (chore: add build artifacts to .gitignore) | ||
|
|
||
| # Build artifacts (cherry-picked from fix/test-cleanups) | ||
| cliproxyapi++ | ||
| .air/ | ||
| boardsync | ||
| releasebatch | ||
| .cache | ||
| logs/ |
There was a problem hiding this comment.
This change introduces merge conflict markers which should be resolved. It also adds duplicate entries. Please resolve the conflicts and remove duplicates.
# Build artifacts (cherry-picked from fix/test-cleanups)
cli-proxy-api-plus-integration-test
cliproxyapi++
.air/
boardsync
releasebatch
.cache
logs/
| timeout: int = 30, | ||
| ): | ||
| self.base_url = base_url.rstrip("/") | ||
| self.api_key = api_key or os.getenv("CLIPROXY_API_KEY", "8317") |
There was a problem hiding this comment.
Using a hardcoded default value of "8317" for the API key is a security risk. This value appears to be the default port, which could be confusing and lead to insecure deployments if a user forgets to set a proper key. It's better to default to None if the environment variable is not set. The _request method should then be updated to only add the Authorization header if an API key is present.
| self.api_key = api_key or os.getenv("CLIPROXY_API_KEY", "8317") | |
| self.api_key = api_key or os.getenv("CLIPROXY_API_KEY") |
| headers = {"Authorization": f"Bearer {self.api_key}"} | ||
| headers.update(kwargs.pop("headers", {})) |
There was a problem hiding this comment.
To handle cases where self.api_key might be None, the Authorization header should only be added if a key is present.
| headers = {"Authorization": f"Bearer {self.api_key}"} | |
| headers.update(kwargs.pop("headers", {})) | |
| headers = {} | |
| if self.api_key: | |
| headers["Authorization"] = f"Bearer {self.api_key}" | |
| headers.update(kwargs.pop("headers", {})) |
| """ | ||
|
|
||
| import httpx | ||
| from dataclasses import dataclass, field |
There was a problem hiding this comment.
To use asdict for dataclass serialization, it needs to be imported from the dataclasses module.
| from dataclasses import dataclass, field | |
| from dataclasses import dataclass, field, asdict |
| from dataclasses import dataclass, field | ||
| from typing import Any, Optional | ||
| from enum import Enum | ||
| import os |
There was a problem hiding this comment.
To avoid hardcoding the default model name in multiple places, it's good practice to define it as a constant.
| import os | |
| import os | |
| DEFAULT_MODEL = "claude-3-5-sonnet-20241022" |
| class ChatChoice: | ||
| """Single chat choice.""" | ||
| index: int | ||
| message: dict |
There was a problem hiding this comment.
The message field is typed as a dict. For better type safety and consistency, it should be typed as ChatMessage. This requires updating the parsing logic in _parse_completion.
| message: dict | |
| message: ChatMessage |
| def first_choice(self) -> str: | ||
| if self.choices and self.choices[0].message: | ||
| return self.choices[0].message.get("content", "") | ||
| return "" |
There was a problem hiding this comment.
With message being a ChatMessage object, you can now access its content attribute directly for cleaner code.
| def first_choice(self) -> str: | |
| if self.choices and self.choices[0].message: | |
| return self.choices[0].message.get("content", "") | |
| return "" | |
| def first_choice(self) -> str: | |
| if self.choices and self.choices[0].message: | |
| return self.choices[0].message.content or "" | |
| return "" |
| def chat( | ||
| self, | ||
| messages: list[ChatMessage], | ||
| model: str = "claude-3-5-sonnet-20241022", |
There was a problem hiding this comment.
Use the DEFAULT_MODEL constant here and in other places where this model name is hardcoded (lines 164, 273) to improve maintainability.
| model: str = "claude-3-5-sonnet-20241022", | |
| model: str = DEFAULT_MODEL, |
|
|
||
| def auth_add(self, auth: AuthEntry) -> dict: | ||
| """Add auth entry - native Python.""" | ||
| return self.management_request("POST", "/v0/management/auth", json=auth.__dict__) |
There was a problem hiding this comment.
Using __dict__ to serialize a dataclass is not recommended. The dataclasses.asdict() function should be used instead, as it's the official and safer way to convert a dataclass instance to a dictionary, and it correctly handles nested dataclasses.
| return self.management_request("POST", "/v0/management/auth", json=auth.__dict__) | |
| return self.management_request("POST", "/v0/management/auth", json=asdict(auth)) |
|
|
||
| def _parse_completion(self, resp: dict) -> ChatCompletion: | ||
| """Parse completion response to Python object.""" | ||
| choices = [ChatChoice(**c) for c in resp.get("choices", [])] |
There was a problem hiding this comment.
To support ChatChoice.message being a ChatMessage object, the parsing logic needs to be updated to explicitly construct ChatMessage objects from the response data.
| choices = [ChatChoice(**c) for c in resp.get("choices", [])] | |
| choices = [ | |
| ChatChoice( | |
| index=c.get("index", 0), | |
| message=ChatMessage(**c.get("message", {})), | |
| finish_reason=c.get("finish_reason") | |
| ) for c in resp.get("choices", []) | |
| ] |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Superseded by layered CI-fix PR #509. |
Fixed: preserve Responses computer tool passthrough
1 participant
Cherry-picked valuable additions from #409 (fix/test-cleanups) before closing the superseded PR.
Changes
.gitignore— add build artifacts (cliproxyapi++, .air/, logs/)sdk/python/cliproxy/api.py— comprehensive Python SDK with native classesapi/openapi.yaml— OpenAPI spec for core endpoints.github/workflows/generate-sdks.yaml— Python/TypeScript SDK generation workflowContext
PR #409 could not be merged due to unresolvable history divergence (was built on clean-main before rename). This PR captures only the new additions and applies them cleanly on top of current main.
Closes #409