Authenticated copilot client + 3 APIs #925
Conversation
WalkthroughWires a new Copilot FastAPI module into the desktop app startup, adds an authenticated Kiln client helper, and introduces comprehensive tests for both the Kiln client and Copilot endpoints. New endpoints handle payload mapping, async calls to Kiln Copilot, and explicit error responses. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI as "FastAPI App"
participant Handler as "copilot_api handler"
participant Config as "Config"
participant KilnClient as "AuthenticatedClient"
participant CopilotSvc as "Kiln Copilot Service"
Client->>FastAPI: POST /api/copilot/{clarify|refine|generate} (payload)
FastAPI->>Handler: route -> handler
Handler->>Config: _get_api_key()
Config-->>Handler: kiln_copilot_api_key
Handler->>KilnClient: get_authenticated_client(api_key)
KilnClient-->>Handler: AuthenticatedClient
Handler->>Handler: map payload -> Kiln SDK request
Handler->>CopilotSvc: <async> post method
alt success
CopilotSvc-->>Handler: Response (model objects)
Handler->>Handler: serialize .to_dict()
Handler-->>FastAPI: 200 JSON
FastAPI-->>Client: 200 response
else validation error (422)
CopilotSvc-->>Handler: HTTPValidationError
Handler-->>FastAPI: 422 JSON
FastAPI-->>Client: 422 response
else no response / unexpected
CopilotSvc-->>Handler: None / unexpected type
Handler-->>FastAPI: 500 JSON
FastAPI-->>Client: 500 response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Summary of ChangesHello @sfierro, 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 introduces the integration of the Kiln Copilot AI service into the desktop application. It achieves this by exposing three new API endpoints—clarify_spec, refine_spec, and generate_batch—through the desktop server. The changes also include the establishment of a secure, authenticated client for communication with the Kiln AI server and comprehensive testing to validate the reliability and proper functioning of these new features. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 new set of API endpoints to proxy requests to a Kiln Copilot service. It adds an authenticated client for this purpose, along with refactoring the existing client creation logic for better code reuse. The new endpoints are for clarifying a spec, refining a spec, and generating a batch of data. The changes are well-tested, including unit tests for the new client logic and integration tests for the new API endpoints.
My review focuses on improving the implementation of the new copilot_api.py file by leveraging more of Pydantic's features to reduce boilerplate code and improve maintainability. I've also suggested adding a couple of missing test cases to ensure full coverage of the error handling paths. Overall, this is a solid contribution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @app/desktop/studio_server/copilot_api.py:
- Around line 3-19: The imports for clarify_spec_v1_copilot_clarify_spec_post,
generate_batch_v1_copilot_generate_batch_post,
refine_spec_v1_copilot_refine_spec_post and the models ClarifySpecInput,
ClarifySpecOutput, GenerateBatchInput, GenerateBatchOutput, RefineSpecInput,
RefineSpecOutput are missing because the generated kiln_ai_server_client does
not include the copilot endpoints; regenerate the API client from the server
OpenAPI spec so the api.copilot module and those model classes are produced (or
update the client package/version to one that contains them), then update the
import block in copilot_api.py to reference the newly generated functions/models
(or remove/guard these imports if the copilot API is intentionally unavailable)
and run tests to confirm the new client code is importable and the referenced
symbols exist.
In @app/desktop/studio_server/test_copilot_api.py:
- Around line 4-13: The imports for ClarifySpecOutput, GenerateBatchOutput,
HTTPValidationError, and other copilot models are missing because the generated
API client lacks the copilot module; regenerate the client from the updated
OpenAPI spec so the kiln_ai_server_client.models package includes those model
classes, then update any import sites (e.g., the test that imports
ClarifySpecOutput/GenerateBatchOutput and the copilot_api.py implementation) to
use the newly generated module names, run the generation/build to publish the
updated package to your local environment, and re-run tests to verify imports
resolve.
🧹 Nitpick comments (3)
app/desktop/studio_server/test_copilot_api.py (2)
153-191: Consider adding validation error test forrefine_spec.The
TestClarifySpecclass includes atest_clarify_spec_validation_errortest, butTestRefineSpecis missing the corresponding validation error test case. Consider adding it for consistency.
193-254: Consider adding validation error test forgenerate_batch.Similarly,
TestGenerateBatchis missing a validation error test case that would verify the 422 response handling. Consider adding it for test completeness.app/desktop/studio_server/copilot_api.py (1)
70-72: Consider closing the authenticated client after use.Each endpoint creates a new
AuthenticatedClientbut doesn't explicitly close it. TheAuthenticatedClientclass supports context manager usage (async with). While the underlying httpx client may be lazily created and garbage collected, explicitly closing it ensures proper resource cleanup, especially for the async client.🔎 Proposed fix using async context manager
@app.post("/api/copilot/clarify_spec") async def clarify_spec(input: ClarifySpecApiInput) -> dict[str, Any]: api_key = _get_api_key() - client = get_authenticated_client(api_key) - - clarify_input = ClarifySpecInput(...) - - result = await clarify_spec_v1_copilot_clarify_spec_post.asyncio( - client=client, - body=clarify_input, - ) + async with get_authenticated_client(api_key) as client: + clarify_input = ClarifySpecInput(...) + + result = await clarify_spec_v1_copilot_clarify_spec_post.asyncio( + client=client, + body=clarify_input, + ) + # ... rest of handlingAlso applies to: 109-111, 153-155
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/desktop/desktop_server.pyapp/desktop/studio_server/api_client/kiln_server_client.pyapp/desktop/studio_server/api_client/test_kiln_server_client.pyapp/desktop/studio_server/copilot_api.pyapp/desktop/studio_server/test_copilot_api.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
**/*.py: Use asyncio for asynchronous operations in Python
Use Pydantic v2 (not v1) for data validation
**/*.py: Use Pydantic v2, not v1
Use asyncio for asynchronous operations in Python
Files:
app/desktop/studio_server/api_client/kiln_server_client.pyapp/desktop/studio_server/api_client/test_kiln_server_client.pyapp/desktop/studio_server/copilot_api.pyapp/desktop/studio_server/test_copilot_api.pyapp/desktop/desktop_server.py
app/desktop/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Python 3.13 for the desktop application (app/desktop)
Files:
app/desktop/studio_server/api_client/kiln_server_client.pyapp/desktop/studio_server/api_client/test_kiln_server_client.pyapp/desktop/studio_server/copilot_api.pyapp/desktop/studio_server/test_copilot_api.pyapp/desktop/desktop_server.py
{libs/server,app/desktop/studio_server}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use FastAPI for REST server implementation
Files:
app/desktop/studio_server/api_client/kiln_server_client.pyapp/desktop/studio_server/api_client/test_kiln_server_client.pyapp/desktop/studio_server/copilot_api.pyapp/desktop/studio_server/test_copilot_api.py
**/*test*.py
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use pytest for testing Python code
Files:
app/desktop/studio_server/api_client/test_kiln_server_client.pyapp/desktop/studio_server/test_copilot_api.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use pytest for testing in Python
Files:
app/desktop/studio_server/api_client/test_kiln_server_client.pyapp/desktop/studio_server/test_copilot_api.py
🧬 Code graph analysis (4)
app/desktop/studio_server/api_client/kiln_server_client.py (1)
app/desktop/studio_server/api_client/kiln_ai_server_client/client.py (1)
AuthenticatedClient(140-282)
app/desktop/studio_server/api_client/test_kiln_server_client.py (2)
app/desktop/studio_server/api_client/kiln_ai_server_client/client.py (1)
AuthenticatedClient(140-282)app/desktop/studio_server/api_client/kiln_server_client.py (1)
get_authenticated_client(43-50)
app/desktop/studio_server/test_copilot_api.py (1)
app/desktop/studio_server/copilot_api.py (1)
connect_copilot_api(68-189)
app/desktop/desktop_server.py (2)
app/desktop/studio_server/test_copilot_api.py (1)
app(20-23)app/desktop/studio_server/copilot_api.py (1)
connect_copilot_api(68-189)
🪛 GitHub Actions: Build and Test
app/desktop/studio_server/copilot_api.py
[error] 3-3: ModuleNotFoundError: No module named 'app.desktop.studio_server.api_client.kiln_ai_server_client.api.copilot'.
app/desktop/studio_server/test_copilot_api.py
[error] 4-4: ImportError: cannot import name 'ClarifySpecOutput' from 'app.desktop.studio_server.api_client.kiln_ai_server_client.models'.
🪛 GitHub Actions: Check API Bindings
app/desktop/studio_server/copilot_api.py
[error] 3-3: ModuleNotFoundError: No module named 'app.desktop.studio_server.api_client.kiln_ai_server_client.api.copilot'
🪛 GitHub Actions: Coverage Report
app/desktop/studio_server/copilot_api.py
[error] 3-3: ModuleNotFoundError: No module named 'app.desktop.studio_server.api_client.kiln_ai_server_client.api.copilot'
app/desktop/studio_server/test_copilot_api.py
[error] 4-4: ImportError: cannot import name 'ClarifySpecOutput' from 'app.desktop.studio_server.api_client.kiln_ai_server_client.models'
🪛 GitHub Actions: Format and Lint
app/desktop/studio_server/copilot_api.py
[error] 3-3: ty: unresolved-import Cannot resolve imported module 'app.desktop.studio_server.api_client.kiln_ai_server_client.api.copilot'.
[error] 9-9: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'ClarifySpecInput'.
[error] 10-10: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'ClarifySpecOutput'.
[error] 11-11: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'ExampleWithFeedback'.
[error] 12-12: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'GenerateBatchInput'.
[error] 13-13: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'GenerateBatchOutput'.
[error] 14-14: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'HTTPValidationError'.
[error] 15-15: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'RefineSpecInput'.
[error] 16-16: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'RefineSpecOutput'.
[error] 17-17: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'SpecInfo'.
[error] 18-18: ty: unresolved-import Module 'app.desktop.studio_server.api_client.kiln_ai_server_client.models' has no member 'TaskInfo'.
🔇 Additional comments (5)
app/desktop/desktop_server.py (1)
17-17: LGTM! Copilot API wiring follows established patterns.The import and registration of
connect_copilot_apifollows the same pattern as other API modules, and the placement beforeconnect_dev_toolsandconnect_webhostis appropriate.Note: This depends on resolving the import errors in
copilot_api.py(see pipeline failures in that file).Also applies to: 68-68
app/desktop/studio_server/api_client/test_kiln_server_client.py (1)
170-208: LGTM! Comprehensive test coverage for authenticated client.The test class follows the same pattern as
TestGetKilnServerClientand provides good coverage for:
- Instance type verification
- Base URL configuration with/without environment variable
- Token assignment
- Header configuration (User-Agent, Kiln-Desktop-App-Version)
- Timeout configuration (read: 300s, connect: 30s)
app/desktop/studio_server/api_client/kiln_server_client.py (1)
21-50: LGTM! Well-structured refactoring with proper DRY application.Good extraction of
_get_base_url()and_get_common_headers()helpers, which are now shared between the unauthenticated and authenticated client factories. Theget_authenticated_clientfunction is properly configured with:
- Environment-configurable base URL
- Token-based authentication
- Consistent headers
- Appropriate timeout (300s read for long-running copilot operations, 30s connect)
app/desktop/studio_server/copilot_api.py (2)
28-55: LGTM! Pydantic input models are well-defined.The input models use Pydantic v2 syntax correctly with
BaseModelandFieldfor defaults. The use ofdict[str, Any]for nested structures inRefineSpecApiInputis appropriate since these are passed through tofrom_dict()methods.
57-65: LGTM! API key validation is appropriate.The
_get_api_keyhelper properly returns 401 with a helpful message when the API key is not configured.
Not ready to merge until server sdk is updated
Summary by CodeRabbit
New Features
Security / Authentication
Tests
✏️ Tip: You can customize this high-level summary in your review settings.