fix(ci): resolve workspace test and lint regressions

This commit is contained in:
Bryan Helmkamp 2026-04-23 20:49:30 -04:00
parent d380c8f496
commit 64dbdf2500
No known key found for this signature in database
15 changed files with 190 additions and 77 deletions

View file

@ -199,16 +199,16 @@ fn load_merged_settings(
Some(cli_layer),
)?,
};
resolve_command_settings(loaded_settings)
Ok(resolve_command_settings(loaded_settings))
}
fn resolve_command_settings(loaded_settings: LoadedSettings) -> Result<ResolvedCommandSettings> {
Ok(ResolvedCommandSettings {
fn resolve_command_settings(loaded_settings: LoadedSettings) -> ResolvedCommandSettings {
ResolvedCommandSettings {
storage_dir: loaded_settings.storage_dir,
run_settings: loaded_settings.run_settings,
server_settings: loaded_settings.server_settings,
user_settings: loaded_settings.user_settings,
})
}
}
#[cfg(test)]
@ -238,8 +238,7 @@ mod tests {
let resolved_settings = resolve_command_settings(
user_config::load_resolved_settings_from_toml("_version = 1\n", None, Some(&cli_layer))
.expect("settings should resolve"),
)
.expect("settings should merge");
);
CommandContext {
printer,
process_local_json,
@ -283,8 +282,7 @@ root = "/srv/fabro/default"
Some(&cli_layer),
)
.expect("base settings should resolve"),
)
.expect("base settings should merge");
);
let connection_settings = resolve_command_settings(
user_config::load_resolved_settings_from_toml(
r#"
@ -297,8 +295,7 @@ root = "/srv/fabro/default"
Some(&cli_layer),
)
.expect("connection settings should resolve"),
)
.expect("connection settings should merge");
);
assert_eq!(
base_settings.user_settings,
@ -339,8 +336,7 @@ root = "/srv/fabro"
Some(&CliLayer::default()),
)
.expect("settings should resolve"),
)
.expect("settings should merge");
);
assert_eq!(resolved.storage_dir, PathBuf::from("/srv/fabro"));
assert!(resolved.run_settings.is_ok());
@ -362,8 +358,7 @@ command = ["demo-mcp"]
Some(&CliLayer::default()),
)
.expect("settings should resolve"),
)
.expect("settings should merge");
);
let run_settings = resolved.run_settings.expect("run settings should resolve");
assert!(run_settings.agent.mcps.contains_key("demo"));

View file

@ -304,12 +304,12 @@ pub(crate) async fn execute(mut args: ExecArgs, ctx: &CommandContext) -> AnyResu
// v2 MCPs live under `cli.exec.agent.mcps` (owner-specific) or
// `run.agent.mcps`. For `fabro exec` we use the cli.exec path, falling
// back to run.agent.mcps if unset.
let mcp_servers: Vec<McpServerSettings> = if !cli.exec.agent.mcps.is_empty() {
cli.exec.agent.mcps.values().cloned().collect()
} else {
let mcp_servers: Vec<McpServerSettings> = if cli.exec.agent.mcps.is_empty() {
ctx.run_settings()
.map(|settings| settings.agent.mcps.values().cloned().collect())
.unwrap_or_default()
} else {
cli.exec.agent.mcps.values().cloned().collect()
};
if let Some(target) = server_target {
tracing::info!(transport = "server", "Agent session starting");

View file

@ -59,8 +59,9 @@ pub(crate) async fn run_uninstall(args: &UninstallArgs, ctx: &CommandContext) ->
let storage_dir = local_server::LocalServerConfig::load_with_storage_dir(None)
.ok()
.map(|settings| settings.storage_dir().to_path_buf())
.unwrap_or_else(user_config::default_storage_dir);
.map_or_else(user_config::default_storage_dir, |settings| {
settings.storage_dir().to_path_buf()
});
let inventory = build_inventory(&home_root, &storage_dir)?;

View file

@ -4,6 +4,8 @@ use std::path::{Path, PathBuf};
use anyhow::Result;
use fabro_config::bind::BindRequest;
use fabro_config::user::default_storage_dir;
use fabro_server::serve::resolve_bind_request_from_server_settings;
use fabro_types::ServerSettings;
use fabro_types::settings::{InterpString, ServerAuthMethod};
@ -58,7 +60,7 @@ impl LocalServerConfig {
.server_settings
.as_ref()
.map_err(|err| anyhow::anyhow!("{err}"))?;
fabro_server::serve::resolve_bind_request_from_server_settings(settings, cli_override)
resolve_bind_request_from_server_settings(settings, cli_override)
}
}
@ -72,11 +74,10 @@ fn storage_dir_from_toml_with_lookup(
) -> Result<PathBuf> {
let document: toml::Value = toml::from_str(source)
.map_err(|err| anyhow::anyhow!("failed to parse settings file: {err}"))?;
let storage_root = string_at_path(&document, &["server", "storage", "root"])
.map(|root| InterpString::parse(&root))
.unwrap_or_else(|| {
InterpString::parse(&fabro_config::user::default_storage_dir().to_string_lossy())
});
let storage_root = string_at_path(&document, &["server", "storage", "root"]).map_or_else(
|| InterpString::parse(&default_storage_dir().to_string_lossy()),
|root| InterpString::parse(&root),
);
let resolved_root = storage_root
.resolve(lookup)
.map_err(|err| anyhow::anyhow!("failed to resolve {}: {err}", storage_root.as_source()))?;
@ -95,6 +96,8 @@ fn string_at_path(document: &toml::Value, path: &[&str]) -> Option<String> {
mod tests {
use std::path::PathBuf;
use fabro_config::user::default_storage_dir;
use super::{storage_dir_from_toml, storage_dir_from_toml_with_lookup};
#[test]
@ -116,7 +119,7 @@ root = "/srv/fabro"
fn storage_dir_from_toml_defaults_without_auth_methods() {
let path = storage_dir_from_toml("_version = 1\n").expect("default storage dir");
assert_eq!(path, fabro_config::user::default_storage_dir());
assert_eq!(path, default_storage_dir());
}
#[test]

View file

@ -352,7 +352,7 @@ fn collect_workflow_config_files(
.map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))?;
let run = document
.remove("run")
.map(|value| value.try_into::<RunLayer>())
.map(toml::Value::try_into::<RunLayer>)
.transpose()
.map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))?
.unwrap_or_default();

View file

@ -3,10 +3,10 @@ use std::str::FromStr;
use anyhow::Result;
pub(crate) use fabro_client::ServerTarget;
use fabro_config::user::default_socket_path;
pub(crate) use fabro_config::user::{active_settings_path, default_storage_dir};
pub(crate) use fabro_config::user::{FABRO_CONFIG_ENV, active_settings_path, default_storage_dir};
use fabro_config::user::{default_settings_path, default_socket_path};
use fabro_config::{
CliLayer, RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder, load_config_file,
CliLayer, ParseError, RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder,
};
use fabro_types::settings::cli::CliTargetSettings;
use fabro_types::settings::{CliNamespace, InterpString, RunNamespace};
@ -52,7 +52,40 @@ pub(crate) fn load_resolved_settings(
}
fn load_settings_document(config_path: Option<&Path>) -> anyhow::Result<toml::Value> {
let table: toml::Table = load_config_file(config_path, "settings.toml")?;
load_settings_document_with_lookup(config_path, |name| std::env::var_os(name))
}
#[expect(
clippy::disallowed_methods,
reason = "sync settings load during CLI startup; not on a Tokio path"
)]
fn load_settings_document_with_lookup(
config_path: Option<&Path>,
lookup: impl Fn(&str) -> Option<std::ffi::OsString>,
) -> anyhow::Result<toml::Value> {
let config_path = config_path
.map(Path::to_path_buf)
.or_else(|| lookup(FABRO_CONFIG_ENV).map(PathBuf::from));
let path = if let Some(path) = config_path {
path
} else {
let default_path = default_settings_path();
if !default_path.is_file() {
return Ok(toml::Value::Table(toml::Table::new()));
}
default_path
};
let contents = std::fs::read_to_string(&path)
.map_err(|source| fabro_config::Error::read_file(&path, source))?;
let table: toml::Table = toml::from_str(&contents).map_err(|source| {
fabro_config::Error::parse_file(
"Failed to parse settings file",
&path,
ParseError::Toml(source.to_string()),
)
})?;
Ok(toml::Value::Table(table))
}
@ -104,9 +137,10 @@ fn storage_dir_from_document_with_lookup(
return Ok(dir.to_path_buf());
}
let storage_root = string_at_path(document, &["server", "storage", "root"])
.map(|root| InterpString::parse(&root))
.unwrap_or_else(|| InterpString::parse(&default_storage_dir().to_string_lossy()));
let storage_root = string_at_path(document, &["server", "storage", "root"]).map_or_else(
|| InterpString::parse(&default_storage_dir().to_string_lossy()),
|root| InterpString::parse(&root),
);
let resolved_root = storage_root.resolve(lookup)?;
Ok(PathBuf::from(resolved_root.value))
}
@ -378,4 +412,34 @@ root = "{{ env.FABRO_STORAGE_ROOT }}"
temp.path()
);
}
#[test]
#[expect(
clippy::disallowed_methods,
reason = "unit test writes a temporary settings fixture with sync std::fs::write"
)]
fn load_settings_document_uses_fabro_config_env_for_storage_root() {
let dir = tempfile::tempdir().unwrap();
let config_path = dir.path().join("settings.toml");
std::fs::write(
&config_path,
r#"
_version = 1
[server.storage]
root = "/srv/fabro"
"#,
)
.unwrap();
let document = load_settings_document_with_lookup(None, |_| {
Some(config_path.clone().into_os_string())
})
.expect("settings document should load");
assert_eq!(
storage_dir_from_document(&document, None).unwrap(),
PathBuf::from("/srv/fabro")
);
}
}

View file

@ -283,7 +283,7 @@ impl WorkflowSettingsBuilder {
#[must_use]
pub(crate) fn args_layer(mut self, layer: SettingsLayer) -> Self {
self.args = layer;
self.args = layer.combine(self.args);
self
}
@ -458,9 +458,14 @@ fn finish_dense_result<T>(
#[cfg(test)]
mod tests {
use fabro_types::settings::run::RunMode;
use std::collections::HashMap;
use super::RunSettingsBuilder;
use fabro_types::settings::InterpString;
use fabro_types::settings::cli::OutputVerbosity;
use fabro_types::settings::run::{ApprovalMode, RunMode};
use super::{RunSettingsBuilder, WorkflowSettingsBuilder};
use crate::{CliLayer, CliOutputLayer, ReplaceMap, RunExecutionLayer, RunLayer, RunModelLayer};
#[test]
fn run_settings_builder_resolves_run_namespace() {
@ -481,4 +486,58 @@ command = ["demo-mcp"]
assert_eq!(settings.execution.mode, RunMode::DryRun);
assert!(settings.agent.mcps.contains_key("demo"));
}
#[test]
fn workflow_builder_preserves_run_overrides_when_cli_overrides_are_added() {
let settings = WorkflowSettingsBuilder::new()
.run_overrides(RunLayer {
metadata: ReplaceMap::from(HashMap::from([("env".to_string(), "cli".to_string())])),
model: Some(RunModelLayer {
provider: Some(InterpString::parse("openai")),
name: Some(InterpString::parse("gpt-5")),
fallbacks: Vec::new(),
}),
execution: Some(RunExecutionLayer {
mode: Some(RunMode::DryRun),
approval: Some(ApprovalMode::Auto),
retros: Some(false),
}),
..RunLayer::default()
})
.cli_overrides(CliLayer {
output: Some(CliOutputLayer {
verbosity: Some(OutputVerbosity::Verbose),
..CliOutputLayer::default()
}),
..CliLayer::default()
})
.build()
.expect("settings should resolve");
assert_eq!(
settings.run.metadata.get("env").map(String::as_str),
Some("cli")
);
assert_eq!(
settings
.run
.model
.provider
.as_ref()
.map(InterpString::as_source),
Some("openai".to_string())
);
assert_eq!(
settings
.run
.model
.name
.as_ref()
.map(InterpString::as_source),
Some("gpt-5".to_string())
);
assert_eq!(settings.run.execution.mode, RunMode::DryRun);
assert_eq!(settings.run.execution.approval, ApprovalMode::Auto);
assert!(!settings.run.execution.retros);
}
}

View file

@ -55,6 +55,7 @@ struct ManifestSettingsOverrides {
cli: Option<CliLayer>,
}
#[cfg(test)]
pub(crate) fn manifest_run_defaults(run: Option<&RunLayer>) -> RunLayer {
run.cloned().unwrap_or_default()
}
@ -226,7 +227,7 @@ fn root_workflow_run_layer(
.map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))?;
let mut run = document
.remove("run")
.map(|value| value.try_into::<RunLayer>())
.map(toml::Value::try_into::<RunLayer>)
.transpose()
.map_err(|err| anyhow!("Failed to parse run config TOML: {err}"))?
.unwrap_or_default();
@ -975,7 +976,7 @@ mod tests {
let mut document: toml::Table = source.parse().expect("v2 fixture should parse");
document
.remove("run")
.map(|value| value.try_into::<RunLayer>())
.map(toml::Value::try_into::<RunLayer>)
.transpose()
.expect("run settings should parse")
.unwrap_or_default()

View file

@ -1067,7 +1067,7 @@ mod tests {
let mut document: toml::Table = source.parse().expect("v2 fixture should parse");
document
.remove("run")
.map(|value| value.try_into::<fabro_config::RunLayer>())
.map(toml::Value::try_into::<fabro_config::RunLayer>)
.transpose()
.expect("run settings should parse")
.unwrap_or_default()

View file

@ -2658,10 +2658,6 @@ pub(crate) fn create_test_app_state_with_runtime_settings_and_session_key(
}
#[cfg(test)]
#[expect(
clippy::disallowed_methods,
reason = "test helper writes a fixture server.env with sync std::fs::write"
)]
pub(crate) fn create_test_app_state_with_session_key(
server_settings: ServerSettings,
manifest_run_defaults: RunLayer,
@ -7491,7 +7487,7 @@ mod tests {
let mut document: toml::Table = source.parse().expect("run defaults should parse");
document
.remove("run")
.map(|value| value.try_into::<fabro_config::RunLayer>())
.map(toml::Value::try_into::<fabro_config::RunLayer>)
.transpose()
.expect("run defaults should parse")
.unwrap_or_default()
@ -11045,11 +11041,8 @@ timeout = "30s"
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn concurrency_limit_respected() {
let state = create_app_state_with_options(
default_test_server_settings(),
RunLayer::default(),
1,
);
let state =
create_app_state_with_options(default_test_server_settings(), RunLayer::default(), 1);
let app = test_app_with_scheduler(Arc::clone(&state));
// Create and start two runs with max_concurrent_runs=1

View file

@ -106,14 +106,11 @@ async fn spawn_served_listener(
.await
});
let bind = match rx.await {
Ok(bind) => bind,
Err(_) => {
let result = handle
.await
.expect("server task should not panic before reporting readiness");
panic!("server should report its bind address: {result:?}");
}
let Ok(bind) = rx.await else {
let result = handle
.await
.expect("server task should not panic before reporting readiness");
panic!("server should report its bind address: {result:?}");
};
(handle, bind, tempdir)
}

View file

@ -62,7 +62,7 @@ pub(crate) fn settings_from_toml(source: &str) -> TestAppSettings {
ensure_test_auth_methods(&mut document);
let manifest_run_defaults = document
.remove("run")
.map(|value| value.try_into::<RunLayer>())
.map(toml::Value::try_into::<RunLayer>)
.transpose()
.expect("test run settings should parse")
.unwrap_or_default();
@ -93,17 +93,18 @@ pub(crate) fn test_app_state_with_options(
}
pub(crate) fn test_settings() -> TestAppSettings {
let mut settings = TestAppSettings::default();
settings.manifest_run_defaults = RunLayer {
sandbox: Some(RunSandboxLayer {
local: Some(LocalSandboxLayer {
worktree_mode: Some(WorktreeMode::Never),
TestAppSettings {
manifest_run_defaults: RunLayer {
sandbox: Some(RunSandboxLayer {
local: Some(LocalSandboxLayer {
worktree_mode: Some(WorktreeMode::Never),
}),
..RunSandboxLayer::default()
}),
..RunSandboxLayer::default()
}),
..RunLayer::default()
};
settings
..RunLayer::default()
},
..TestAppSettings::default()
}
}
pub(crate) fn test_app_with_scheduler(state: Arc<AppState>) -> axum::Router {

View file

@ -647,7 +647,7 @@ mod tests {
let state: RunProjection = serde_json::from_value(serde_json::json!({
"spec": {
"run_id": "01JW6A7VNFZSFF0SKXJG29Z2M3",
"settings": { "_version": 1 },
"settings": WorkflowSettings::default(),
"graph": { "name": "ship", "nodes": {}, "edges": [], "attrs": {} },
"workflow_slug": "demo",
"working_directory": "/tmp/project",

View file

@ -7,7 +7,7 @@ use std::path::{Path, PathBuf};
use std::sync::Arc;
use anyhow::Context;
use fabro_config::project::resolve_working_directory_from_run;
use fabro_config::project::{resolve_workflow_path, resolve_working_directory_from_run};
use fabro_config::run::resolve_run_goal_from_namespace;
use fabro_types::WorkflowSettings;
@ -65,8 +65,7 @@ fn workflow_slug_from_path(workflow_path: &Path) -> Option<String> {
pub(crate) fn resolve_workflow(request: ResolveWorkflowInput) -> anyhow::Result<ResolvedWorkflow> {
match request.workflow {
WorkflowInput::Path(workflow_path) => {
let resolution =
fabro_config::project::resolve_workflow_path(&workflow_path, &request.cwd)?;
let resolution = resolve_workflow_path(&workflow_path, &request.cwd)?;
let settings = request.settings;
let raw_source = std::fs::read_to_string(&resolution.dot_path)
.with_context(|| format!("Failed to read {}", resolution.dot_path.display()))?;

View file

@ -311,7 +311,7 @@ impl RunSession {
let resolved = &settings.run;
let sandbox_provider = resolve_sandbox_provider(&resolved)?;
let sandbox_provider = resolve_sandbox_provider(resolved)?;
let sandbox_provider =
if resolved.execution.mode == RunMode::DryRun && !sandbox_provider.is_local() {
SandboxProvider::Local
@ -366,7 +366,7 @@ impl RunSession {
None => None,
};
SandboxSpec::Daytona {
config: resolve_daytona_config(&resolved).unwrap_or_default(),
config: resolve_daytona_config(resolved).unwrap_or_default(),
github_app: services.github_app.clone(),
run_id: Some(record.run_id),
clone_branch: detected_base_branch.or_else(|| record.base_branch.clone()),
@ -434,7 +434,7 @@ impl RunSession {
artifact_sink: services.artifact_sink,
git,
github_app: services.github_app.clone(),
worktree_mode: Some(resolve_worktree_mode(&resolved)),
worktree_mode: Some(resolve_worktree_mode(resolved)),
registry_override: services.registry_override,
retro_enabled: resolved.execution.retros && project_config::is_retro_enabled(),
preserve_sandbox: resolved.sandbox.preserve,
@ -971,8 +971,8 @@ mod tests {
use chrono::Utc;
use fabro_config::{RunExecutionLayer, RunLayer, WorkflowSettingsBuilder};
use fabro_store::Database;
use fabro_types::{WorkflowSettings, fixtures};
use fabro_types::settings::run::RunMode;
use fabro_types::{WorkflowSettings, fixtures};
use object_store::memory::InMemory;
use super::*;