Conversation
Updates argument parsing to support a `url timeout=<seconds>` format, allowing users to override the default extension timeout. Signed-off-by: Jazzcort <jason101011113@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR resolves a merge conflict for PR #6521, which adds support for custom timeout values for streamable HTTP extensions via a url timeout=<seconds> format in CLI arguments.
Changes:
- Adds
StreamableHttpOptionsstruct to capture URL and timeout - Implements
parse_streamable_http_extensionparser function - Updates type signatures to use
StreamableHttpOptionsinstead ofString - Applies code formatting changes in TypeScript files
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/goose-cli/src/cli.rs | Adds StreamableHttpOptions struct and parsing function for timeout parameter |
| crates/goose-cli/src/session/mod.rs | Imports StreamableHttpOptions type |
| crates/goose-cli/src/session/builder.rs | Updates StreamableHttpOptions type in SessionBuilderConfig and test |
| crates/goose-cli/src/commands/bench.rs | Converts string URLs to StreamableHttpOptions with default timeout |
| ui/desktop/src/utils/sessionCache.ts | Formatting change - multi-line error message |
| ui/desktop/src/utils/autoUpdater.ts | Formatting change - function call alignment |
| ui/desktop/src/components/sessions/SessionListView.tsx | Formatting change - multi-line toast error |
| ui/desktop/src/components/sessions/SessionHistoryView.tsx | Formatting change - function call alignment |
| ui/desktop/src/components/schedule/SchedulesView.tsx | Formatting change - function calls alignment |
| ui/desktop/src/components/alerts/AlertBox.tsx | Formatting change - window.alert alignment |
| ui/desktop/src/App.tsx | Formatting change - setFatalError alignment |
| ) | ||
| .await; | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove this extra blank line to maintain consistent code formatting.
| if let Some(url_str) = input_iter.next() { | ||
| url.push_str(url_str); | ||
| } |
There was a problem hiding this comment.
The url variable should be validated before being returned. If url_str is None (i.e., input is empty or only whitespace), an empty URL would be returned, which would cause issues downstream. Consider returning an error if the URL is missing or empty.
| let (key, value) = kv_pair.split_once('=').unwrap(); | ||
|
|
||
| // We Can have more keys here for setting other properties | ||
| if key == "timeout" { | ||
| if let Ok(seconds) = value.parse::<u64>() { | ||
| timeout = seconds; |
There was a problem hiding this comment.
Using unwrap() here is unsafe since we've already checked that the string contains '=' but there's still a guarantee from split_once. However, if the string is "key=" (no value), this will work but produce an empty value string. Consider handling the case where the value might be empty, or document that empty values are acceptable for timeout parameters.
| let (key, value) = kv_pair.split_once('=').unwrap(); | |
| // We Can have more keys here for setting other properties | |
| if key == "timeout" { | |
| if let Ok(seconds) = value.parse::<u64>() { | |
| timeout = seconds; | |
| if let Some((key, value)) = kv_pair.split_once('=') { | |
| // We Can have more keys here for setting other properties | |
| if key == "timeout" && !value.is_empty() { | |
| if let Ok(seconds) = value.parse::<u64>() { | |
| timeout = seconds; | |
| } |
| if let Ok(seconds) = value.parse::<u64>() { | ||
| timeout = seconds; |
There was a problem hiding this comment.
If the timeout value cannot be parsed as u64, the error is silently ignored and the default timeout is used. This could be confusing for users who provide an invalid timeout value. Consider returning an error or at least logging a warning when the timeout parsing fails.
| if let Ok(seconds) = value.parse::<u64>() { | |
| timeout = seconds; | |
| match value.parse::<u64>() { | |
| Ok(seconds) => { | |
| timeout = seconds; | |
| } | |
| Err(_) => { | |
| return Err(format!( | |
| "invalid timeout value '{}', expected an unsigned integer (seconds)", | |
| value | |
| )); | |
| } |
|
|
||
| let (key, value) = kv_pair.split_once('=').unwrap(); | ||
|
|
||
| // We Can have more keys here for setting other properties |
There was a problem hiding this comment.
Capitalize "Can" to "can" for consistency with standard comment style.
| // We Can have more keys here for setting other properties | |
| // We can have more keys here for setting other properties |
| fn parse_streamable_http_extension(input: &str) -> Result<StreamableHttpOptions, String> { | ||
| let mut input_iter = input.split_whitespace(); | ||
| let (mut url, mut timeout) = (String::new(), goose::config::DEFAULT_EXTENSION_TIMEOUT); | ||
|
|
||
| if let Some(url_str) = input_iter.next() { | ||
| url.push_str(url_str); | ||
| } | ||
|
|
||
| for kv_pair in input_iter { | ||
| if !kv_pair.contains('=') { | ||
| continue; | ||
| } | ||
|
|
||
| let (key, value) = kv_pair.split_once('=').unwrap(); | ||
|
|
||
| // We Can have more keys here for setting other properties | ||
| if key == "timeout" { | ||
| if let Ok(seconds) = value.parse::<u64>() { | ||
| timeout = seconds; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(StreamableHttpOptions { url, timeout }) | ||
| } |
There was a problem hiding this comment.
The new parse_streamable_http_extension function lacks test coverage. Consider adding tests to verify correct parsing of URL, timeout parameter, handling of invalid timeout values, missing URLs, and various edge cases.
Signed-off-by: Jazzcort <jason101011113@gmail.com> Co-authored-by: Jazzcort <jason101011113@gmail.com> Co-authored-by: Douwe Osinga <douwe@squareup.com> Signed-off-by: Elias Posen <elias@posen.ch>
2 participants
Summary
Merge conflict resolved for: #6521