Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions apps/staged/src-tauri/src/actions/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use builderbot_actions::{
ActionDetector, ActionExecutor, ActionMetadata, ActionType, FileExplorationMode,
RunDetectionMode, StopOptions, SuggestedAction,
};
use std::path::PathBuf;
use std::sync::{Arc, Mutex};
use tauri::{AppHandle, State};
use tokio::sync::watch;
Expand Down Expand Up @@ -34,6 +35,31 @@ struct DetectingActionsEvent {
detecting: bool,
}

/// Build an [`AcpAiProvider`] for action detection, honoring the user's
/// preferred agent when `provider_id` is `None`.
///
/// An explicit `provider_id` always wins. When it is `None` — which the
/// automatic first-touch worktree setup and the project-MCP `add_project_repo`
/// path both pass — detection resolves the user's most-recently-used available
/// agent via
/// [`discover_preferred_provider_id`](crate::session_commands::discover_preferred_provider_id),
/// the shared helper behind the badge and action-detection fallbacks, instead
/// of silently picking the first installed agent in `KNOWN_AGENTS` order
/// (Goose).
///
/// Falls back to [`AcpAiProvider::new`] (first installed agent) only when no
/// provider can be resolved at all — i.e. no agents are installed, in which
/// case construction would fail regardless.
pub(crate) fn build_action_provider(
provider_id: Option<&str>,
working_dir: PathBuf,
) -> Result<AcpAiProvider> {
match crate::session_commands::discover_preferred_provider_id(provider_id) {
Some(id) => AcpAiProvider::with_agent(&id, working_dir),
None => AcpAiProvider::new(working_dir),
}
}

pub(crate) async fn detect_actions_for_repo_context(
github_repo: &str,
subpath: Option<&str>,
Expand Down Expand Up @@ -65,11 +91,8 @@ pub(crate) async fn detect_actions_for_repo_context(
None => std::env::temp_dir(),
};

let provider = match provider_id {
Some(id) => AcpAiProvider::with_agent(id, provider_dir.clone()),
None => AcpAiProvider::new(provider_dir.clone()),
}
.map_err(|e| format!("Failed to create AI provider: {e}"))?;
let provider = build_action_provider(provider_id, provider_dir.clone())
.map_err(|e| format!("Failed to create AI provider: {e}"))?;

let detector = ActionDetector::new(Box::new(provider));

Expand Down
10 changes: 5 additions & 5 deletions apps/staged/src-tauri/src/actions/run_detector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use tauri::AppHandle;
use tokio::sync::watch;
use tokio::time::{self, Duration};

use builderbot_actions::{AcpAiProvider, AiProvider, RunDetectionMode};
use builderbot_actions::{AiProvider, RunDetectionMode};

use super::events::emit_run_phase_changed;
use super::events::RunPhaseChangedEvent;
Expand Down Expand Up @@ -275,10 +275,10 @@ If still building, set regex and has_endpoint_capture to null/false."#,
);

let ai_response = {
let provider_result = match provider_id.as_deref() {
Some(id) => AcpAiProvider::with_agent(id, working_dir.clone()),
None => AcpAiProvider::new(working_dir.clone()),
};
let provider_result = super::commands::build_action_provider(
provider_id.as_deref(),
working_dir.clone(),
);
let provider = match provider_result {
Ok(p) => p,
Err(e) => {
Expand Down
53 changes: 2 additions & 51 deletions apps/staged/src-tauri/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1433,18 +1433,6 @@ fn build_badge_prompt(
prompt
}

fn badge_provider_id(
provider: Option<&str>,
available_ids: &[String],
recent_ids: &[String],
) -> Option<String> {
provider
.map(str::trim)
.filter(|provider| !provider.is_empty())
.map(ToOwned::to_owned)
.or_else(|| session_commands::select_preferred_provider(available_ids, recent_ids))
}

fn find_badge_agent(provider: Option<&str>) -> Option<acp_client::AcpAgent> {
if let Some(provider) = provider
.map(str::trim)
Expand All @@ -1457,16 +1445,7 @@ fn find_badge_agent(provider: Option<&str>) -> Option<acp_client::AcpAgent> {
return agent;
}

let available_ids: Vec<String> = agent::discover_providers()
.into_iter()
.map(|provider| provider.id)
.collect();
let provider = badge_provider_id(
None,
&available_ids,
&session_commands::read_recent_agent_ids(),
)?;

let provider = session_commands::discover_preferred_provider_id(None)?;
acp_client::find_acp_agent_by_id(&provider)
}

Expand Down Expand Up @@ -2390,14 +2369,10 @@ pub fn run() {

#[cfg(test)]
mod tests {
use super::{badge_provider_id, cleanup_project_branches_best_effort};
use super::cleanup_project_branches_best_effort;
use crate::store::{Branch, BranchType};
use std::collections::HashMap;

fn ids(values: &[&str]) -> Vec<String> {
values.iter().map(|value| value.to_string()).collect()
}

fn remote_branch(
project_id: &str,
id: &str,
Expand All @@ -2409,30 +2384,6 @@ mod tests {
branch
}

#[test]
fn badge_provider_id_uses_explicit_provider() {
assert_eq!(
badge_provider_id(Some("codex"), &ids(&["goose", "claude"]), &ids(&["claude"])),
Some("codex".to_string())
);
}

#[test]
fn badge_provider_id_uses_recent_available_provider() {
assert_eq!(
badge_provider_id(None, &ids(&["goose", "claude"]), &ids(&["codex", "claude"])),
Some("claude".to_string())
);
}

#[test]
fn badge_provider_id_falls_back_to_first_available_provider() {
assert_eq!(
badge_provider_id(None, &ids(&["goose", "claude"]), &ids(&["codex"])),
Some("goose".to_string())
);
}

#[test]
fn delete_project_cleanup_retries_branch_row_deletes_and_re_sweeps_remote_workspaces() {
let branches = vec![
Expand Down
109 changes: 109 additions & 0 deletions apps/staged/src-tauri/src/session_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2143,6 +2143,63 @@ pub(crate) fn select_preferred_provider(
available_ids.first().cloned()
}

/// Resolve a provider id from an optional explicit selection.
///
/// A non-blank explicit `provider` always wins (after trimming whitespace);
/// blank or whitespace-only values are ignored. When no usable explicit
/// provider is given — the `provider: None` path taken by repo badges, action
/// detection, and any future caller — fall back to the user's preferred
/// available agent via [`select_preferred_provider`]. Returns `None` only when
/// nothing can be resolved at all (no explicit provider and no available
/// agent).
///
/// This is the single shared shape behind every `provider: None` resolution
/// path so the fallback logic can't drift between call sites over time.
pub(crate) fn resolve_preferred_provider_id(
provider: Option<&str>,
available_ids: &[String],
recent_ids: &[String],
) -> Option<String> {
explicit_provider_id(provider).or_else(|| select_preferred_provider(available_ids, recent_ids))
}

/// The usable explicit provider id, if any: the trimmed `provider` when it is
/// non-blank, otherwise `None`. Shared by [`resolve_preferred_provider_id`] and
/// [`discover_preferred_provider_id`] so the "non-blank explicit id wins"
/// parsing has a single definition and can't drift between them.
fn explicit_provider_id(provider: Option<&str>) -> Option<String> {
provider
.map(str::trim)
.filter(|id| !id.is_empty())
.map(ToOwned::to_owned)
}

/// [`resolve_preferred_provider_id`] sourcing `available_ids` from the installed
/// providers and `recent_ids` from the saved `recent-agents` preference.
///
/// Centralizes the `discover_providers` + `read_recent_agent_ids` scaffolding
/// shared by the badge and action-detection callers so they all discover
/// providers and consult the preference the same way.
///
/// A non-blank explicit `provider` short-circuits before any discovery: it
/// always wins regardless of what's installed, so probing every known agent
/// (each spawns a login shell that sources the user's full profile) and reading
/// the preferences file would only compute results that are immediately
/// discarded. This matters because the action-detection poller resolves the
/// provider on every poll iteration; only the `provider: None` fallback paths
/// pay the discovery cost.
pub(crate) fn discover_preferred_provider_id(provider: Option<&str>) -> Option<String> {
if let Some(id) = explicit_provider_id(provider) {
return Some(id);
}

let available_ids: Vec<String> = agent::discover_providers()
.into_iter()
.map(|p| p.id)
.collect();
resolve_preferred_provider_id(None, &available_ids, &read_recent_agent_ids())
}

fn missing_review_provider_error(is_remote: bool) -> String {
if is_remote {
"No remote ACP provider is configured for review sessions.".to_string()
Expand Down Expand Up @@ -4375,6 +4432,58 @@ mod tests {
);
}

#[test]
fn resolve_preferred_provider_id_uses_explicit_provider() {
assert_eq!(
resolve_preferred_provider_id(
Some("codex"),
&ids(&["goose", "claude"]),
&ids(&["claude"])
),
Some("codex".to_string())
);
}

#[test]
fn resolve_preferred_provider_id_uses_recent_available_provider() {
// Goose is first in KNOWN_AGENTS order, but the user's recent preference
// is `claude` — the resolver must pick the preference, not first-installed.
assert_eq!(
resolve_preferred_provider_id(
None,
&ids(&["goose", "claude"]),
&ids(&["codex", "claude"])
),
Some("claude".to_string())
);
}

#[test]
fn resolve_preferred_provider_id_falls_back_to_first_available_provider() {
// No recent agent is available, so fall back to the first available.
assert_eq!(
resolve_preferred_provider_id(None, &ids(&["goose", "claude"]), &ids(&["codex"])),
Some("goose".to_string())
);
}

#[test]
fn resolve_preferred_provider_id_ignores_blank_explicit_provider() {
assert_eq!(
resolve_preferred_provider_id(
Some(" "),
&ids(&["goose", "claude"]),
&ids(&["claude"])
),
Some("claude".to_string())
);
}

#[test]
fn resolve_preferred_provider_id_returns_none_when_nothing_available() {
assert_eq!(resolve_preferred_provider_id(None, &[], &[]), None);
}

#[test]
fn resolve_provider_from_ids_rejects_unavailable_provider() {
let available = ids(&["goose", "claude"]);
Expand Down