Remove session_id from provider streaming trait methods - #9984
Conversation
DOsinga
left a comment
There was a problem hiding this comment.
Agree the third option (push this onto ApiClient / a transport-level context rather than the provider trait) seems best.
The reason is that session_id is really serving two different purposes here, and only one of them belongs in the provider layer:
-
The
agent-session-idHTTP header — this is tracking/routing for the API-client providers, and it's already anApiClientconcern (that's where it gets injected). It doesn't need to be in the trait signature at all; making it ambient is fine for this case but pushing it ontoApiClientis cleaner and removes most of the churn. -
claude-code / codex stream-json
session_id— this is genuinely functional state (it goes into the request body and the CLI keeps conversation context per session_id). For this case an ambient task-local that silently yieldsNone/""when a caller forgets the scope is a footgun: you could route a turn into the wrong conversation with no compile-time signal. This is the one place I'd keep it explicit.
So rather than making it all ambient, splitting the two — header onto ApiClient, and keep the explicit param only where it shapes the request body — gets you the signature cleanup without the silent-failure risk.
Approving so this isn't blocked, but flagging the above in case you want to take the ApiClient route.
There was a problem hiding this comment.
💡 Codex Review
After the complete override was removed, nothing ever scopes IS_AGENT_CALL to true, so Provider::complete(...) now falls through to the default implementation and this check always treats non-tool-response requests as user-initiated. For GitHub Copilot, internal non-streaming calls such as session naming, compaction, planner classification, and permission judging will now be sent with X-Initiator: user instead of agent, regressing the routing/accounting behavior the old override enforced.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
goose/crates/goose/src/session_context.rs
Lines 18 to 19 in 47c7874
None to clear inherited session IDs
When callers pass None, this branch simply awaits the future without creating a new task-local scope, so any outer SESSION_ID remains visible to session_id_request_builder(). A provider call that is intentionally made without a session while running inside another session scope will therefore send the outer agent-session-id; the old explicit None/empty session path removed that header. Scope None as well so optional session IDs can actually clear inherited context.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c4791366b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3526bca894
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@DOsinga I ended up re-working this to use the ApiClient after all. Now, callers can hook into the request builder to set the header. A task-scoped variable remains in the goose app layer. |
* main: chore: Remove legacy MCP-UI proxy support (#10086) Remove session_id from provider streaming trait methods (#9984) fix(cli): update help text for --session-id (#10077) task(acp): upgrade SDK and use new HTTP/WS crate (#10082) fix(providers): reject non-object tool-call arguments instead of panicking (#9832) docs: name the message field in the hooks payload guide (#9913) fix(cli): save /edit prompts to history (#10011) feat(i18n): add fr, de, it, pt, id, ms, vi, zh-TW desktop locales (#10072) feat (acp+): Use ACP permission manager for tool permissions (#10066) feat (ui): Remove ACP chat feature flag and turn on chat using ACP (#10062) fix(schedule): use session.message_count for schedule sessions listing (#10026) feat(acp): migrate getDictationConfig and transcribeDictation to ACP (#10048) fix: sync the sesion store after session provider and model update and also update thinking effort (#10060)
Introduces provider-specific mechanisms for using a scoped session id. For most this is done via a request builder that can be set on the ApiClient.
for #9803