Conversation
baxen
requested review from
DOsinga,
angiejones and
blackgirlbytes
as code owners
![chatgpt-codex-connector[bot]](https://avatars.gh.umua.top/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31544799b9
ℹ️ 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".
| export type UpsertConfigRequest = { | ||
| key: string; | ||
| isSecret: boolean; | ||
| }; |
There was a problem hiding this comment.
value field to UpsertConfigRequest type
The generated TypeScript request type for _goose/config/upsert omits the required value property, even though the server-side request schema requires it. As generated, strongly typed callers cannot construct a valid request without type assertions, and a caller following this type literally will send { key, isSecret } and get a runtime invalid-params error from the backend.
Useful? React with 👍 / 👎.
angiejones
left a comment
angiejones
left a comment
There was a problem hiding this comment.
approving doc updates
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a8f8b725d
ℹ️ 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: d0b248a919
ℹ️ 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".
crates/goose/src/acp/provider.rs
Outdated
| let notify = { | ||
| let mut notifiers = self.session_config_notifiers.lock().await; | ||
| notifiers | ||
| .entry(acp_session_id.clone()) | ||
| .or_insert_with(|| Arc::new(Notify::new())) | ||
| .clone() | ||
| }; | ||
|
|
||
| tokio::time::timeout(DEFERRED_INITIAL_CONFIG_TIMEOUT, notify.notified()) | ||
| .await |
There was a problem hiding this comment.
There is a race in response_with_initial_config: if a config update arrives after the initial cache check but before this waiter registers/blocks, handle_deferred_config_update stores the update without notifying (no notifier existed yet), and this path can then wait until timeout despite data already being available. In that case model discovery fails with a timeout, and deferred config delivery can be dropped for ACP sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fcaef48ff
ℹ️ 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".
Signed-off-by: Bradley Axen <baxen@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08d6b6ec61
ℹ️ 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".
| .insert(session_id.to_string(), new_model.clone()); | ||
| } | ||
| } | ||
| let session_id = self.acp_session_id(); |
There was a problem hiding this comment.
This change hard-wires every prompt to a single ACP session (self.acp_session_id()), ignoring the session_id provided by Goose. In this codebase, provider instances are reused across multiple Goose session IDs (for example via cloned Arc<dyn Provider> in subagent/orchestrator flows), so this causes cross-session context bleed: prompts and config updates from different Goose sessions end up in the same remote ACP conversation. Before this commit the provider kept a Goose↔ACP session map; removing that mapping makes multi-session behavior incorrect for ACP-backed providers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
there was exactly one user - orchestrator platform extension that is still hidden - that does this and i updated it. this is not correct otherwise
…de warnings - Orchestrator now calls providers::create() for child agents instead of sharing the parent's Arc<dyn Provider>, matching the summon/delegate pattern and avoiding shared session state with ACP providers. - Suppress unused variable warnings for telemetry-gated code in agent.rs and scheduler.rs. - Add #[allow(dead_code)] on shared test modules in goose-acp tests, since each test binary only uses a subset of the common functions. Signed-off-by: Bradley Axen <baxen@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78aeee1d16
ℹ️ 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".
|
|
||
| let model_state = build_model_state(&*provider).await?; | ||
| let mode_state = build_mode_state(self.goose_mode)?; | ||
| let provider_selection = session_provider_selection(&goose_session).to_string(); |
There was a problem hiding this comment.
on_new_session builds config_options from the pre-init goose_session object, but init_provider has already called agent.update_provider(...), which persists the resolved concrete provider/model into session storage. In the common case where _meta.provider is not set, goose_session.provider_name is still None, so the response advertises provider "goose"; later build_config_update/load_session reads the stored session and reports the concrete provider (for example openai), so the provider config option can flip without any user change.
Useful? React with 👍 / 👎.
| let current_provider = agent.provider().await.map_err(|e| { | ||
| sacp::Error::internal_error().data(format!("Failed to get provider: {}", e)) | ||
| })?; | ||
| let provider_name = current_provider.get_name().to_string(); |
There was a problem hiding this comment.
on_set_model always derives provider_name from the active concrete provider and then calls agent.update_provider(...), which persists that concrete name. If a session was intentionally set to the default-provider sentinel ("goose") via update_provider, changing only the model through session/setConfigOption or session/setModel silently rewrites the session away from the sentinel, so future restores no longer follow the global default-provider behavior.
Useful? React with 👍 / 👎.
Signed-off-by: Bradley Axen <baxen@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9fcac6a59
ℹ️ 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".
| } else if is_changing_provider { | ||
| ACP_CURRENT_MODEL.to_string() |
There was a problem hiding this comment.
In update_provider, omitting model while switching to a different provider always sets default_model to ACP_CURRENT_MODEL ("current"). That sentinel is only resolved by ACP-backed providers in AcpProvider::connect; non-ACP providers keep model_name="current", so later prompts can be sent with an invalid model id and fail at runtime. This branch should choose a concrete model for the target provider (for example, its registry default) instead of forcing current for all provider changes.
Useful? React with 👍 / 👎.
| if let Some(ref provider_name) = requested_provider { | ||
| self.session_manager | ||
| .update(&goose_session.id) | ||
| .provider_name(provider_name) | ||
| .apply() |
There was a problem hiding this comment.
on_new_session persists _meta.provider directly into the newly created session without checking that the provider exists. If a client sends an unknown provider id, init_provider fails afterward and the request errors, but the session row created earlier remains stored, producing orphan ACP sessions that can accumulate from repeated bad requests. Validate the provider before writing it (or clean up the session on init failure).
Useful? React with 👍 / 👎.
|
I think this makes sense direction wise from what I know. The main acp consume was the TUI, will that need to be updated (I may have missed it in here) cc @alexhancock ? @baxen looks like syntax error in docs, grok live test was a server outage it looks like. |
this is backwards compatible for the TUI! |
3 participants
Motivation
This moves more ACP session and provider control into
goose serveso clients like the new desktop app can behave as thin transports instead of owning backend lifecycle themselves. The main change is that Goose ACP now exposes the provider operations a client needs over ACP custom requests, including provider selection at session creation, provider switching, reset-to-default behavior, and provider discovery.Alongside the new features, this significantly simplifies the ACP provider internals by removing abstractions that were unused within the codebase (CLI, desktop, TUI).
New features
_goose/session/provider/update— switch provider (and optionally model/context limit) for a session_goose/providers/list— discover available providers_goose/config/read,_goose/config/upsert,_goose/config/remove— CRUD for goose config valuesconfig_options, so ACP clients can switch providers via the samesession/setConfigOptionflow_meta.provideronnew_session— clients can choose a provider up front, avoiding a double-create (session + immediate provider switch)"goose") — resetting to the default provider reads from config and clears any session-level model overrideamp-acpandpi-acpproviders — new ACP-backed providers following the same pattern as claude-acp/codex-acp/copilot-acp, registered as non-visiblemanages_own_context()for ACP providers — skips goose-side context compaction and tool-pair summarization, matching the existing pattern for claude_code and gemini_cliSimplifications
PermissionMappingremovedThe old code let each provider definition (claude-acp, codex-acp, copilot-acp) specify preferred option IDs for permission responses (e.g.
allow_option_id: Some("allow")). The new code matches byPermissionOptionKind(AllowOnce, AllowAlways, etc.) directly. This is correct because the ACP protocol defines option kinds precisely — matching by kind is the canonical approach. The preferred-ID override was never actually needed: copilot already usedPermissionMapping::default()(kind-based fallback), and the IDs specified by claude/codex just happened to match their kinds anyway.Callers updated:
claude_acp.rs,codex_acp.rs,copilot_acp.rsall had theirpermission_mappingfield removed. No other code referencedPermissionMapping.Multi-session tracking removed (
goose_to_acp_id,acp_to_goose_id,session_model)The old code maintained bidirectional maps between goose session IDs and ACP session IDs, supporting multiple ACP sessions per provider instance. In practice, goose creates one provider instance per session — the multi-session machinery was unused complexity. The new code creates a single ACP session eagerly during
connect()and stores it directly as anAcpSessionstruct.Removed with it:
ensure_session,has_session,resolve_acp_session_id,map_sessions_to_goose_ids, and all thegoose_to_acp_id/acp_to_goose_id/session_modellocking.rejected_tool_callsmade local tostream()Was an
Arc<TokioMutex<HashSet<String>>>shared across the provider. Since rejected tool call tracking is only needed within a singlestream()call (tool calls don't span prompts), this is now a plainHashSetlocal to the stream closure. Simpler and avoids unnecessary async mutex overhead.init_session/OnceCellremovedThe old code lazily created a throwaway session just to discover the current model name (for
ACP_CURRENT_MODELresolution), then closed it if the agent supported close. The new code resolves the model from the eagerly-created session — no throwaway needed.send_set_model,send_untyped,delete_session,list_sessions,close_session_by_acp_idremovedThese were public methods on
AcpProviderwith no callers outside the struct itself (and its tests).send_set_modelwas used for mid-stream model forwarding which is now unnecessary with single-session semantics.send_untypedwas only used bydelete_session.list_sessionswas only used by tests.close_session_by_acp_idwas only used byinit_session. The correspondingClientRequestvariants (SetModel,Untyped,CloseSession,ListSessions) are also removed.auth_methodsfield removedWas stored on the provider from the init response but never read by any caller in the codebase.
update_modesimplifiedThe old code had a "pre-initialization" guard that skipped the ACP call when no sessions existed yet (storing mode only in the shared mutex). With eager session creation, this guard is unnecessary. Also,
mode_mapping[&mode](which would panic on missing key) is replaced with.get(&mode).unwrap_or_else(...)— strictly safer.Other changes
AcpProviderFactorytype replacesProviderConstructorin goose-acp, adding a provider name parameter to support runtime provider switchingProviderEntry::metadata()accessor addedSessionUpdateBuilder::clear_provider_name/clear_model_configadded for the reset-to-default flowEnabledExtensionsStateused in goose-acp server to pass session extensions when creating providersClientCapabilitiessent during ACP initialize