Add Go SDK for Jules Agent API #5
Conversation
Summary of ChangesHello @sheing-google, 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 initial Go SDK for the Jules Agent API. It establishes the core client infrastructure, defines all necessary data models for interacting with the API's resources such as sessions, activities, and sources, and provides concrete methods for performing common operations. The SDK aims to offer a robust and idiomatic Go interface for developers to integrate with the Jules Agent platform. Highlights
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 Go SDK for the Jules Agent API. The overall structure is solid, leveraging idiomatic Go patterns like functional options for client configuration. The API coverage for sessions, activities, and sources is a great start. My review focuses on improving robustness, error handling, and test quality. I've identified areas where URL construction can be made safer, error handling can be more complete, and tests can be more thorough. I've also noted a potential issue with the client options pattern that could lead to surprising behavior.
| bodyBytes, _ := io.ReadAll(resp.Body) | ||
| return fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, string(bodyBytes)) |
There was a problem hiding this comment.
The error returned from io.ReadAll is being ignored by using the blank identifier _. If reading the response body fails, the error message will be incomplete (e.g., showing an empty body string), making debugging harder. You should always handle errors returned from I/O operations.
bodyBytes, err := io.ReadAll(resp.Body)
if err != nil {
return fmt.Errorf("API request failed with status %d, and could not read response body: %w", resp.StatusCode, err)
}
return fmt.Errorf("API request failed with status %d: %s", resp.StatusCode, string(bodyBytes))| // WithTimeout sets the timeout for the client. | ||
| func WithTimeout(timeout time.Duration) ClientOption { | ||
| return func(c *Client) { | ||
| c.httpClient.Timeout = timeout | ||
| } |
There was a problem hiding this comment.
The WithTimeout option directly modifies the Timeout field of the httpClient. If a user provides their own http.Client via WithHTTPClient, this option will mutate that client. This can be surprising behavior, especially since the order of options matters (WithHTTPClient followed by WithTimeout will modify the provided client, but the reverse order will not). Consider documenting this behavior or changing the implementation to avoid mutating a user-provided client. A safer approach might be to only set the timeout on the default client if no custom client is provided.
| u, err := url.Parse(c.baseURL + path) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse URL: %w", err) | ||
| } |
There was a problem hiding this comment.
Using string concatenation to build URLs can be fragile. For instance, it might not correctly handle cases where c.baseURL already has a path component or if the path component is not properly escaped. A more robust approach is to use url.Parse and url.URL.ResolveReference. This ensures that paths are joined correctly according to URL semantics and provides better error handling.
u, err := url.Parse(c.baseURL)
if err != nil {
return fmt.Errorf("failed to parse base URL: %w", err)
}
pathURL, err := url.Parse(path)
if err != nil {
return fmt.Errorf("failed to parse request path: %w", err)
}
u = u.ResolveReference(pathURL)| if len(resp.Sessions) != 2 { | ||
| t.Errorf("Expected 2 sessions, got %d", len(resp.Sessions)) | ||
| } |
There was a problem hiding this comment.
The assertion here only checks the number of sessions. For a more robust test, you should check the content of the sessions as well. This ensures that the response is not just the correct size, but also contains the expected data. Using more specific assertions provides better failure messages and test coverage.
if len(resp.Sessions) != 2 {
t.Fatalf("Expected 2 sessions, got %d", len(resp.Sessions))
}
if resp.Sessions[0].ID != "s1" {
t.Errorf("Expected session 0 ID to be 's1', got %q", resp.Sessions[0].ID)
}
if resp.Sessions[1].ID != "s2" {
t.Errorf("Expected session 1 ID to be 's2', got %q", resp.Sessions[1].ID)
}| data, err := json.Marshal(activity) | ||
| if err != nil { | ||
| t.Fatalf("Failed to marshal activity: %v", err) | ||
| } | ||
|
|
||
| var unmarshaled Activity | ||
| if err := json.Unmarshal(data, &unmarshaled); err != nil { | ||
| t.Fatalf("Failed to unmarshal activity: %v", err) | ||
| } | ||
|
|
||
| // cmp.Diff might have issues with map[string]any due to types (float64 vs int when unmarshaling JSON numbers) | ||
| // For simplicity in this basic test, we'll just check a few fields explicitly if cmp fails hard, | ||
| // but let's try cmp first as it's robust. | ||
| // Note: json.Unmarshal unmarshals numbers to float64 by default for interface{}, so we might need to handle that if we were strict. | ||
| // For this test, we'll rely on basic structural equality. | ||
| } |
There was a problem hiding this comment.
This test function TestActivityJSON performs a JSON marshal/unmarshal cycle but lacks any assertions to verify the result. A test without assertions is not useful as it can't fail even if the logic is broken (e.g., if fields are not marshaled or unmarshaled correctly). You should add assertions to check that the unmarshaled object is equal to the original.
data, err := json.Marshal(activity)
if err != nil {
t.Fatalf("Failed to marshal activity: %v", err)
}
var unmarshaled Activity
if err := json.Unmarshal(data, &unmarshaled); err != nil {
t.Fatalf("Failed to unmarshal activity: %v", err)
}
if diff := cmp.Diff(activity, unmarshaled); diff != "" {
t.Errorf("Activity mismatch after JSON roundtrip (-want +got):\n%s", diff)
}
}| if pollInterval == 0 { | ||
| pollInterval = 5 * time.Second | ||
| } |
There was a problem hiding this comment.
The check pollInterval == 0 sets a default, but it doesn't handle negative durations. A negative duration would cause time.NewTicker to panic. It's safer to check for pollInterval <= 0. Additionally, defining the default interval as a named constant improves readability and maintainability.
| if pollInterval == 0 { | |
| pollInterval = 5 * time.Second | |
| } | |
| const defaultPollInterval = 5 * time.Second | |
| if pollInterval <= 0 { | |
| pollInterval = defaultPollInterval | |
| } |
1 participant
No description provided.