refactor(pr): simplify post-refactor PR command code

- Parallelize `pr list` discovery loop via buffer_unordered; thread RunId
  through the stream to drop the run_id.parse().expect(...) panic path.
- Skip computing the default model in create_run_pull_request when the
  request already supplies one (common path from `fabro pr create`).
- Delete the dead user_config::storage_dir wrapper (test-only, zero
  callers, stale deprecation note); point its tests at
  local_server::storage_dir directly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

lib/crates/fabro-cli/src/commands/pr/list.rs

@@ -1,6 +1,7 @@
 use anyhow::Result;
 use cli_table::format::{Border, Separator};
 use cli_table::{Cell, CellStruct, Color, Style, Table};
+use fabro_api::types::PullRequestDetail;
 use fabro_util::terminal::Styles;
 use futures::stream::{self, StreamExt};
 use serde::Serialize;
@@ -8,9 +9,24 @@ use tracing::info;
 
 use crate::args::PrListArgs;
 use crate::command_context::CommandContext;
-use crate::server_runs::ServerSummaryLookup;
+use crate::server_runs::{ServerRunSummaryInfo, ServerSummaryLookup};
 use crate::shared::{color_if, print_json_pretty};
 
+fn pr_display_state(detail: &PullRequestDetail) -> String {
+    if detail.merged {
+        "merged"
+    } else if detail.draft {
+        "draft"
+    } else {
+        match detail.state.as_str() {
+            "open" => "open",
+            "closed" => "closed",
+            _ => "unknown",
+        }
+    }
+    .to_string()
+}
+
 #[derive(Serialize)]
 struct PrRow {
     run_id: String,
@@ -26,14 +42,24 @@ pub(super) async fn list_command(args: PrListArgs, base_ctx: &CommandContext) ->
     let printer = ctx.printer();
     let lookup = ServerSummaryLookup::from_client(ctx.server().await?).await?;
 
-    let mut entries = Vec::new();
-    for run in lookup.runs() {
-        if let Ok(state) = lookup.client().get_run_state(&run.run_id()).await {
-            if let Some(record) = state.pull_request {
-                entries.push((run.run_id().to_string(), record));
+    let client = lookup.client().clone_for_reuse();
+    let run_ids: Vec<_> = lookup
+        .runs()
+        .iter()
+        .map(ServerRunSummaryInfo::run_id)
+        .collect();
+    let entries: Vec<_> = stream::iter(run_ids)
+        .map(|run_id| {
+            let client = client.clone_for_reuse();
+            async move {
+                let state = client.get_run_state(&run_id).await.ok()?;
+                state.pull_request.map(|record| (run_id, record))
             }
-        }
-    }
+        })
+        .buffer_unordered(10)
+        .filter_map(|entry| async move { entry })
+        .collect()
+        .await;
 
     if entries.is_empty() {
         if ctx.json_output() {
@@ -44,51 +70,34 @@ pub(super) async fn list_command(args: PrListArgs, base_ctx: &CommandContext) ->
         return Ok(());
     }
 
-    let client = lookup.client().clone_for_reuse();
     let all_rows = stream::iter(entries)
         .map(|(run_id, record)| {
             let client = client.clone_for_reuse();
             async move {
-                match client
-                    .get_run_pull_request(&run_id.parse().expect("run id should parse"))
-                    .await
-                {
-                    Ok(detail) => {
-                        let state = if detail.merged {
-                            "merged".to_string()
-                        } else if detail.draft {
-                            "draft".to_string()
-                        } else if detail.state == "open" {
-                            "open".to_string()
-                        } else if detail.state == "closed" {
-                            "closed".to_string()
-                        } else {
-                            "unknown".to_string()
-                        };
-                        Ok(PrRow {
-                            run_id,
-                            number: detail.number,
-                            state,
-                            merged: detail.merged,
-                            title: detail.title,
-                            url: detail.html_url,
-                        })
-                    }
+                match client.get_run_pull_request(&run_id).await {
+                    Ok(detail) => Ok(PrRow {
+                        run_id: run_id.to_string(),
+                        number: detail.number,
+                        state:  pr_display_state(&detail),
+                        merged: detail.merged,
+                        title:  detail.title,
+                        url:    detail.html_url,
+                    }),
                     Err(err) => {
                         let message = err.to_string();
                         if message.contains("GitHub integration unavailable on server.") {
                             return Err(err);
                         }
 
-                        tracing::warn!(run_id, error = %message, "Failed to fetch PR state");
+                        tracing::warn!(run_id = %run_id, error = %message, "Failed to fetch PR state");
                         Ok(PrRow {
-                            run_id,
+                            run_id: run_id.to_string(),
                             number: i64::try_from(record.number)
                                 .expect("stored pull request number should fit in i64"),
-                            state: "unknown".to_string(),
+                            state:  "unknown".to_string(),
                             merged: false,
-                            title: record.title,
-                            url: record.html_url,
+                            title:  record.title,
+                            url:    record.html_url,
                         })
                     }
                 }

lib/crates/fabro-cli/src/user_config.rs

@@ -1,6 +1,4 @@
 use std::path::Path;
-#[cfg(test)]
-use std::path::PathBuf;
 use std::str::FromStr;
 
 use anyhow::Result;
@@ -12,8 +10,6 @@ use fabro_util::version::FABRO_VERSION;
 use tracing::debug;
 
 use crate::args::ServerTargetArgs;
-#[cfg(test)]
-use crate::local_server;
 
 pub(crate) fn load_settings() -> anyhow::Result<SettingsLayer> {
     load_settings_with_config_and_storage_dir(None, None)
@@ -55,14 +51,6 @@ pub(crate) fn default_server_target() -> ServerTarget {
     ServerTarget::unix_socket_path(default_socket_path()).expect("default socket path is absolute")
 }
 
-#[deprecated(
-    note = "use local_server::storage_dir for lifecycle; PR commands must move to server-side API"
-)]
-#[cfg(test)]
-pub(crate) fn storage_dir(settings: &SettingsLayer) -> anyhow::Result<PathBuf> {
-    local_server::storage_dir(settings)
-}
-
 fn parse_server_target(value: &str) -> Result<ServerTarget> {
     ServerTarget::from_str(value)
 }
@@ -96,16 +84,15 @@ pub(crate) fn cli_http_client_builder() -> fabro_http::HttpClientBuilder {
 }
 
 #[cfg(test)]
-#[allow(
-    deprecated,
-    reason = "the storage_dir tests are exercising the deprecated helper by definition"
-)]
 mod tests {
+    use std::path::PathBuf;
+
     use fabro_config::parse_settings_layer;
     use fabro_config::user::default_storage_dir;
 
     use super::*;
     use crate::args::ServerTargetArgs;
+    use crate::local_server;
 
     fn server_target_args(value: Option<&str>) -> ServerTargetArgs {
         ServerTargetArgs {
@@ -225,7 +212,10 @@ url = "https://config.example.com"
     fn storage_dir_defaults_without_server_auth_methods() {
         let settings = SettingsLayer::default();
 
-        assert_eq!(storage_dir(&settings).unwrap(), default_storage_dir());
+        assert_eq!(
+            local_server::storage_dir(&settings).unwrap(),
+            default_storage_dir()
+        );
     }
 
     #[test]
@@ -239,7 +229,10 @@ root = "/srv/fabro"
 "#,
         );
 
-        assert_eq!(storage_dir(&settings).unwrap(), PathBuf::from("/srv/fabro"));
+        assert_eq!(
+            local_server::storage_dir(&settings).unwrap(),
+            PathBuf::from("/srv/fabro")
+        );
     }
 
     #[test]

lib/crates/fabro-server/src/server.rs

@@ -5317,13 +5317,15 @@ async fn create_run_pull_request(
         return missing_remote_branch_error(run_branch).into_response();
     }
 
-    let configured = configured_providers_from_process_env(Some(&state.vault)).await;
-    let model = body.model.unwrap_or_else(|| {
+    let model = if let Some(model) = body.model {
+        model
+    } else {
+        let configured = configured_providers_from_process_env(Some(&state.vault)).await;
         Catalog::builtin()
             .default_for_configured(&configured)
             .id
             .clone()
-    });
+    };
 
     let pull_request = match pull_request::maybe_open_pull_request(
         &creds,