fix: harden SDK validation#2
Merged
Merged
Conversation
Reviewer's GuideHarden SDK-side validation and request shaping across workspace construction, pagination, dialectic chat, and file upload, while tightening base URL/workspace ID validation, switching workspace reads to POST get-or-create semantics, and expanding tests and CI coverage (including MSRV). Sequence diagram for new workspace metadata/configuration POST flowsequenceDiagram
participant Honcho
participant HttpClient
participant Routes
participant Server
Honcho->>Routes: workspaces()
Routes-->>Honcho: /workspaces
Honcho->>Honcho: build WorkspaceCreate { id, metadata: None, configuration: None }
Honcho->>HttpClient: post(/workspaces, WorkspaceCreate, headers)
HttpClient->>Server: POST /workspaces { id, metadata, configuration }
Server-->>HttpClient: 200 Workspace
HttpClient-->>Honcho: Workspace
Honcho->>Honcho: extract metadata/configuration
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 updates the honcho-ai SDK to version 0.1.2, introducing centralized validation for workspace IDs, base URLs, and pagination parameters. Key changes include switching workspace metadata and configuration retrieval from GET to POST to support upsert-like behavior, adding an is_retryable helper to the error type, and expanding documentation for file uploads and peer management. Feedback identifies a potential regression where the new POST requests might fail with a 409 Conflict error on existing workspaces, and notes that the new hard limit of 100 for pagination page sizes may be a breaking change for some users.
Comment on lines
243
to
253
| pub async fn get_metadata(&self) -> Result<HashMap<String, Value>> { | ||
| let body = crate::types::workspace::WorkspaceCreate { | ||
| id: self.inner.workspace_id.clone(), | ||
| metadata: None, | ||
| configuration: None, | ||
| }; | ||
| let ws: Workspace = self | ||
| .inner | ||
| .http | ||
| .get(&routes::workspace(self.workspace_id())?, &[]) | ||
| .post(&routes::workspaces(), Some(&body), &[]) | ||
| .await?; |
There was a problem hiding this comment.
The change from GET to POST for fetching workspace metadata (and configuration in subsequent methods) introduces a potential regression. While ensure_workspace (line 194) explicitly handles 409 Conflict as a success case, these methods do not. If the workspace already exists and the server returns 409 on a POST request, these methods will fail with a Conflict error instead of returning the data.
If the server is guaranteed to return 200 OK with the workspace object on POST even if it exists, then the 409 handling in ensure_workspace is redundant. Otherwise, these methods should handle the 409 case (e.g., by falling back to a GET request) to ensure they work correctly for existing workspaces.
Comment on lines
+435
to
+439
| if !(1..=100).contains(&size) { | ||
| return Err(HonchoError::Validation( | ||
| "size must be between 1 and 100".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The validate_pagination function enforces a hard limit of 100 for the page size. This is a breaking change that restricts users from requesting larger pages even if the server supports them. If this limit is intended to match server-side constraints, it should be documented in the public API; otherwise, consider allowing larger values or making the limit configurable.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
src/client.rs,normalize_base_urlis now called directly but is defined insrc/http/client.rsaspub(crate); consider either importing it explicitly fromcrate::http::clientor calling it ascrate::http::client::normalize_base_urlto avoid a compilation error. - You removed the
size == 0validation inListConclusionsBuilder::sendbut only added the newvalidate_paginationcheck inpaginate_post; consider reusingvalidate_pagination(or equivalent bounds checking) for conclusions pagination as well so page/size are consistently validated across the SDK.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `src/client.rs`, `normalize_base_url` is now called directly but is defined in `src/http/client.rs` as `pub(crate)`; consider either importing it explicitly from `crate::http::client` or calling it as `crate::http::client::normalize_base_url` to avoid a compilation error.
- You removed the `size == 0` validation in `ListConclusionsBuilder::send` but only added the new `validate_pagination` check in `paginate_post`; consider reusing `validate_pagination` (or equivalent bounds checking) for conclusions pagination as well so page/size are consistently validated across the SDK.
## Individual Comments
### Comment 1
<location path="src/client.rs" line_range="96" />
<code_context>
- ));
- }
+ validate_workspace_id(workspace_id)?;
+ let url = normalize_base_url(base_url)?;
let http =
- HttpClient::from_params(HttpClient::builder().base_url(base_url.to_string()).build())?;
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid double-normalizing and reparsing the base URL when constructing `HttpClient`.
`normalize_base_url` already parses and validates `base_url` into a `Url`, but we then convert it to `String` and pass it into `HttpClient::from_params`, which calls `normalize_base_url` again (same in `from_env`). This does redundant parsing/validation on every client construction. It would be better if `HttpClient::builder`/`HttpClientParams` could accept a `Url` directly, or if there were a constructor that takes an already-normalized `Url` to avoid the extra work.
Suggested implementation:
```rust
pub fn new(base_url: &str, workspace_id: &str) -> Result<Self> {
validate_workspace_id(workspace_id)?;
let url = normalize_base_url(base_url)?;
let http = HttpClient::from_url(url)?;
Ok(Self {
inner: Arc::new(Inner {
http,
```
To fully implement the optimization and avoid double-normalization/reparsing you will also need to:
1. **Add a constructor to `HttpClient` that accepts a pre-normalized `Url`:**
- In the module where `HttpClient` is defined, implement something like:
```rust
impl HttpClient {
pub fn from_url(url: Url) -> Result<Self> {
// construct HttpClient directly from a Url without calling `normalize_base_url` again
let params = HttpClientParams::builder().base_url_url(url).build();
HttpClient::from_params(params)
}
}
```
- The exact implementation will depend on how `HttpClientParams`/`builder()` are structured.
2. **Extend `HttpClientParams` / `HttpClient::builder()` to accept a `Url` directly (if it doesn’t already):**
- For example, add a method like:
```rust
impl HttpClientParamsBuilder {
pub fn base_url_url(mut self, url: Url) -> Self {
self.base_url = url;
self
}
}
```
- And change `HttpClientParams`’s `base_url` field to be a `Url` instead of a `String` if it isn’t already, or otherwise ensure that `from_url` does not re-call `normalize_base_url`.
3. **Update other call sites (such as `from_env`) that currently do `normalize_base_url(...)?` and then `HttpClient::from_params(HttpClient::builder().base_url(url.to_string()).build())` to instead use `HttpClient::from_url(url)` so they also benefit from avoiding redundant normalization.**
</issue_to_address>
### Comment 2
<location path=".github/workflows/ci.yml" line_range="49-50" />
<code_context>
- - run: cargo test --test '*_types'
- - run: cargo test --test wire_format_peers
- - run: cargo test --test compile_assertions
+ - run: cargo test --all-features
+ - run: cargo test --doc --all-features
+
+ msrv:
</code_context>
<issue_to_address>
**suggestion (testing):** Running both `cargo test --all-features` and `cargo test --doc --all-features` likely executes doc tests twice.
`cargo test` already includes doctests unless you filter them out (e.g. with `--lib`, `--test`, etc.), so this setup will run doctests in both commands, slowing CI without added coverage. To avoid duplication while still running everything else, you could either change the first command to `cargo test --all-features --lib --tests --bins --examples` or remove the separate `--doc` invocation.
Suggested implementation:
```
- uses: Swatinem/rust-cache@42dc69e1aa15d09112580998cf2ef0119e2e91ae # v2
- run: cargo test --all-features --lib --tests --bins --examples
```
None required, as long as there are no other workflow steps elsewhere that depend specifically on `cargo test --doc`. If there are, consider aligning those with this pattern as well.
</issue_to_address>
### Comment 3
<location path="tests/pagination.rs" line_range="671-680" />
<code_context>
}
#[tokio::test]
-async fn gets_workspace_metadata_by_id() {
+async fn gets_workspace_metadata_by_post_get_or_create() {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a boundary test for the minimum allowed page size
Current tests cover rejection of invalid sizes and acceptance of `size = 100`, but not the lower bound. Since `validate_pagination` permits `size = 1`, please add a test that calls `paginate_post` with `page = 1, size = 1` and asserts the request is sent with `size=1` and succeeds, confirming the intended `1..=100` inclusive range.
</issue_to_address>
### Comment 4
<location path="README.md" line_range="144" />
<code_context>
session.remove_peers(["bob"]).await?;
let peers = session.peers().await?;
let cfg = session.get_peer_configuration("alice").await?;
+// alice must already be present; add_peer/add_peers/set_peers above satisfies this.
session.set_peer_configuration("alice", &cfg).await?;
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adjusting verb agreement in this sentence ("satisfy" instead of "satisfies").
Because `add_peer/add_peers/set_peers` form a compound subject, the verb should be plural: `add_peer/add_peers/set_peers above satisfy this.`
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+671
to
+680
| #[tokio::test] | ||
| async fn paginate_post_rejects_invalid_page_and_size_before_request() { | ||
| use honcho_ai::http::client::HttpClient; | ||
| use honcho_ai::types::pagination::paginate_post; | ||
| use wiremock::MockServer; | ||
|
|
||
| let server = MockServer::start().await; | ||
| let http = HttpClient::from_params( | ||
| HttpClient::builder() | ||
| .base_url(server.uri()) |
There was a problem hiding this comment.
suggestion (testing): Consider adding a boundary test for the minimum allowed page size
Current tests cover rejection of invalid sizes and acceptance of size = 100, but not the lower bound. Since validate_pagination permits size = 1, please add a test that calls paginate_post with page = 1, size = 1 and asserts the request is sent with size=1 and succeeds, confirming the intended 1..=100 inclusive range.
Implements robust client-side validation for various parameters, including workspace IDs, pagination values, and dialectic queries, ensuring invalid requests are caught early. Enhances URL normalization and validation during client construction for consistent and reliable base URL handling. Updates relevant documentation to clearly state these new constraints.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Harden SDK validation and workspace handling, add preflight input checks, and tighten HTTP/base URL behavior while expanding tests and CI coverage.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: