fix(staged): honor preferred agent in action detection - #838
Merged
Conversation
Automatic action detection (first-time worktree setup and the project-MCP add_project_repo path) passes provider_id=None. On that path the backend fell back to AcpAiProvider::new() -> find_acp_agent(), which returns the first installed agent in KNOWN_AGENTS order (Goose) and never consulted the user's recentAgents preference, so detection silently ran as Goose regardless of the selected agent whenever Goose was installed. Resolve the None case the same way #823 fixed repo badges: when no provider is supplied, compute select_preferred_provider over discover_providers() and read_recent_agent_ids() and build the provider with that id, only falling back to first-installed when no agent is available at all. The new build_action_provider helper centralizes this for both detect_actions_for_repo_context and the autodetect poller, covering every None caller (including future ones) at once. Add resolver unit tests mirroring #823's badge_provider_id_* tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: 6b11f8cf6e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| provider_id: Option<&str>, | ||
| working_dir: PathBuf, | ||
| ) -> Result<AcpAiProvider> { | ||
| let available_ids: Vec<String> = crate::agent::discover_providers() |
There was a problem hiding this comment.
When the caller passes an explicit provider, this still runs discover_providers() before resolve_action_provider_id can return that id. Discovery shells out through login-shell lookups for every known agent, so a slow or hanging shell init or unrelated provider lookup can now block action detection even though the selected provider could be constructed directly; this regresses the old explicit path, which only called AcpAiProvider::with_agent(id, ...). Short-circuit a nonblank provider_id before discovery and only read recents/discover when it is absent.
Useful? React with 👍 / 👎.
Action detection, repo badges, and (loosely) auto review each carried their own `provider: None` fallback. `resolve_action_provider_id` in actions/commands was byte-for-byte identical to `badge_provider_id` in lib, and `build_action_provider` duplicated the `discover_providers` + `read_recent_agent_ids` scaffolding from `find_badge_agent`, so the fallback logic could drift between call sites. Hoist the shared shape into `session_commands` alongside `select_preferred_provider`: - `resolve_preferred_provider_id(provider, available_ids, recent_ids)` is the pure, unit-tested trim -> filter-empty -> or_else(select_preferred_provider) resolver. - `discover_preferred_provider_id(provider)` bundles the discover_providers + read_recent_agent_ids scaffolding on top of it. `build_action_provider` and `find_badge_agent` now both call `discover_preferred_provider_id`, and the duplicated `badge_provider_id` / `resolve_action_provider_id` resolvers (plus their parallel test sets) are removed in favor of one set of resolver tests next to the helper. No behavior change: clippy clean, all Rust tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
discover_preferred_provider_id always ran agent::discover_providers() plus read_recent_agent_ids() before resolving, even when the caller passed a non-blank explicit provider id that resolve_preferred_provider_id short-circuits via .or_else() — so the discovery results were computed and immediately discarded. discover_providers() probes every KNOWN_AGENT through find_via_login_shell, each spawning a login shell that sources the user's full profile, and the action-detection poller calls build_action_provider (hence this helper) on every poll iteration. The pre-refactor poller went straight to AcpAiProvider::with_agent(id) with a single probe; the consolidation regressed that to N login-shell probes per poll. Short-circuit the explicit, non-blank provider before touching discover_providers()/read_recent_agent_ids(): only the provider: None fallback paths now pay the discovery cost. Extract the shared trim -> filter-empty parsing into explicit_provider_id so resolve_preferred_provider_id and the new short-circuit use one definition and can't drift; the fallback path still routes through resolve_preferred_provider_id so the unit-tested resolver stays the single source of resolution truth. No behavior change: clippy clean, all Rust tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
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
When action detection is triggered without an explicit
provider_id— which happens during automatic first-touch worktree setup and via the project-MCPadd_project_repopath — the code previously fell back toAcpAiProvider::new, which silently picks the first installed agent inKNOWN_AGENTSorder (Goose). This ignored the user's actual agent preference.This change introduces a shared
build_action_providerhelper that mirrors the frontend'sgetPreferredAgentlogic viaselect_preferred_provider, so detection honors the user's most-recently-used available agent. It only falls back to first-installed when no agent can be resolved at all (e.g. nothing installed). This matches thebadge_provider_idfallback added in #823.Changes
resolve_action_provider_id/build_action_providerinactions/commands.rs: an explicitprovider_idalways wins; otherwise resolve against discovered providers + recent agent ids.detect_actions_for_repo_contextandrun_detectorthroughbuild_action_provider, removing the duplicatedwith_agent/newbranching.