-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(goose): only send agent-session-id when a session exists #6657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| // | ||
| /// # Parameters | ||
| /// - `session_id`: Use `None` only for configuration or pre-session tasks. | ||
| async fn complete_with_model( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the key place though some others had to switch to Option to support pre-session tasks
Keep session headers 1:1 with session manager records so we avoid confusing or conflicting IDs. Backfill propagation across API clients and provider requests (including Bedrock's AWS runtime path) so OpenAI-compatible Envoy AI Gateway clones also see agent-session-id. Keep session_id required for normal agent flows; allow None only for configuration and pre-session paths like model discovery, provider detection, and auth probes. Signed-off-by: Adrian Cole <adrian@tetrate.io>
355230c to
5b1f5c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Stops propagating agent-session-id unless a real goose session exists, while ensuring non-OpenAI-compatible providers can still receive the header for correlation when a session does exist.
Changes:
- Updates the provider completion path to accept
session_id: Option<&str>and only injectagent-session-idwhenSome(non_empty). - Removes session-id requirements from model discovery/config/probing paths (
fetch_supported_models/fetch_recommended_models) and updates call sites accordingly. - Adds/updates tests to cover header injection/removal behavior and MCP session propagation edge cases.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/goose/tests/mcp_integration_test.rs | Updates mock provider signature to accept optional session id. |
| crates/goose/tests/agent.rs | Adapts test provider to optional session id for completion. |
| crates/goose/src/providers/xai.rs | Threads optional session id through completion requests. |
| crates/goose/src/providers/venice.rs | Removes session requirement for model fetch; optional session for completion. |
| crates/goose/src/providers/tetrate.rs | Removes session requirement for model fetch; optional session for completion. |
| crates/goose/src/providers/testprovider.rs | Propagates optional session id through wrapped provider calls. |
| crates/goose/src/providers/snowflake.rs | Threads optional session id through requests. |
| crates/goose/src/providers/sagemaker_tgi.rs | Conditionally propagates session id via SageMaker custom attributes. |
| crates/goose/src/providers/openrouter.rs | Removes session requirement for model fetch; conditional session propagation via payload/user + header. |
| crates/goose/src/providers/openai.rs | Removes session requirement for model fetch; optional session for completion calls. |
| crates/goose/src/providers/ollama.rs | Removes session requirement for model fetch; optional session for completion. |
| crates/goose/src/providers/litellm.rs | Removes session requirement for model fetch; optional session for completion + cache-control detection. |
| crates/goose/src/providers/lead_worker.rs | Updates delegation to use complete_with_model with optional session id; removes session from model fetch. |
| crates/goose/src/providers/google.rs | Removes session requirement for model fetch; optional session propagation for requests/streaming. |
| crates/goose/src/providers/githubcopilot.rs | Removes session requirement for model fetch; optional session propagation for requests. |
| crates/goose/src/providers/gemini_cli.rs | Updates provider signature to optional session id (still ignored for CLI). |
| crates/goose/src/providers/gcpvertexai.rs | Conditionally injects session header for Vertex AI requests; removes session from model fetch. |
| crates/goose/src/providers/databricks.rs | Removes pre-session UUID generation for probing; removes session requirement for model fetch; optional session for completion. |
| crates/goose/src/providers/cursor_agent.rs | Updates provider signature to optional session id (still ignored for CLI). |
| crates/goose/src/providers/codex.rs | Updates provider signature to optional session id (still ignored for CLI). |
| crates/goose/src/providers/claude_code.rs | Updates provider signature to optional session id (still ignored for CLI constraints). |
| crates/goose/src/providers/chatgpt_codex.rs | Conditionally injects agent-session-id header for streaming requests; removes session from model fetch. |
| crates/goose/src/providers/canonical/build_canonical_models.rs | Removes pre-session UUID generation for provider probing/model fetch. |
| crates/goose/src/providers/bedrock.rs | Conditionally injects agent-session-id header into AWS Bedrock requests. |
| crates/goose/src/providers/base.rs | Changes provider trait APIs to use optional session id for completion and removes session from model fetch APIs. |
| crates/goose/src/providers/azure.rs | Threads optional session id through requests. |
| crates/goose/src/providers/auto_detect.rs | Removes pre-session UUID usage; uses sessionless model fetch for provider detection. |
| crates/goose/src/providers/api_client.rs | Makes request builder session-aware (optional) and adds tests for header set/replace/remove behavior. |
| crates/goose/src/providers/anthropic.rs | Removes session requirement for model fetch; optional session for completion; ensures streaming still passes session. |
| crates/goose/src/context_mgmt/mod.rs | Updates test provider signature to optional session id. |
| crates/goose/src/agents/reply_parts.rs | Updates test provider signature to optional session id. |
| crates/goose/src/agents/mcp_client.rs | Removes connection-scoped fallback session id; makes MCP session id propagation optional and removable. |
| crates/goose/examples/image_tool.rs | Updates example to use complete_with_model(None, ...) for pre-session usage. |
| crates/goose/examples/databricks_oauth.rs | Updates example to use complete_with_model(None, ...) for pre-session usage. |
| crates/goose-server/src/routes/config_management.rs | Removes pre-session UUID generation; uses sessionless model fetch/provider detect. |
| crates/goose-cli/src/commands/configure.rs | Removes pre-session UUID generation; switches config checks to sessionless model fetch or complete_with_model(None, ...). |
Comments suppressed due to low confidence (1)
crates/goose/examples/image_tool.rs:71
&provider.get_model_config()borrows a temporaryModelConfigacross.await, which will not compile; assignlet model_config = provider.get_model_config();before the call and pass&model_config.
let (response, usage) = provider
.complete_with_model(
None,
&provider.get_model_config(),
"You are a helpful assistant. Please describe any text you see in the image.",
&messages,
&[Tool::new("view_image", "View an image", input_schema)],
)
Summary
Stop adding the
agent-session-idheader unless there is a current session. This keeps headers 1:1 with session manager records and avoids confusing or conflicting session IDs.Noneis for configuration or pre-session paths (model discovery, provider detection, auth probes, canonical model checks).This also backfills propagation of non-basic OpenAI apis so they can also see
agent-session-id(e.g., Anthropic, Bedrock, Vertex etc). This is important because proxies like Envoy AI Gateway and Ollama started supporting non-OpenAI endpoints recently. With this, they can do log correlation on goose session ID.Type of Change
AI Assistance
Testing
Added some edge case tests.
Related Issues
Tuneup of #6626