scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-15 23:32:46 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

refactor(settings): stage 6.3 delete dead Settings helpers + v2 install TOML

Browse source 
Stage 6.3 closes out the dead code that Stage 6.1 left behind:

fabro-types
- Delete the inherent helpers on the legacy flat `Settings` struct
  (`app_id`, `slug`, `client_id`, `git_author`, `sandbox_settings`,
  `setup_settings`, `setup_commands`, `setup_timeout_ms`,
  `preserve_sandbox_enabled`, `github_permissions`, `mcp_server_entries`,
  `verbose_enabled`, `prevent_idle_sleep_enabled`, `upgrade_check_enabled`,
  `dry_run_enabled`, `auto_approve_enabled`, `no_retro_enabled`,
  `storage_dir`, `slack_settings`). Nothing reads them anymore --
  consumers now use `SettingsFile` accessors (`github_app_id_str()`,
  `run_sandbox()`, `dry_run_enabled()`, `storage_dir()`, etc.). The
  `Settings` struct itself stays alive for the remaining legacy
  OpenAPI response path and a handful of demo-route payloads; Stage
  6.6 finishes the deletion alongside the OpenAPI spec rewrite.
- Delete the `#[cfg(test)] mod tests` block that only covered the
  deleted `storage_dir()` helper.

fabro-cli/commands/install.rs
- `merge_server_settings` now writes a v2 TOML file (with
  `[server.{api,listen.tls,web,auth.api.{jwt,mtls},auth.web}]` stanzas)
  instead of the legacy v1 top-level `[web]`/`[api]`/`[git]` shape.
  The generated file previously failed to parse as v2 on next startup;
  now it round-trips through `ConfigLayer::parse`.
- Tests rewritten to parse the generated TOML through
  `fabro_config::ConfigLayer::parse` and assert against the v2 tree
  (`server.auth.web.allowed_usernames`, `server.auth.api.{jwt,mtls}.enabled`,
  `server.listen.tls.{cert,key,ca}`). The `merge_server_settings_preserves_existing_*`
  tests collapsed into a single `preserves_existing_top_level_sections`
  test since the old tests were asserting v1 `[git]` / `[api]` keys
  that no longer make sense.

Build, clippy, fmt, and tests all green: 3756 / 3756 pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-09 16:20:26 -04:00
parent ea206e0e40
commit 34a481cd44
No known key found for this signature in database
2 changed files with 118 additions and 196 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

209
lib/crates/fabro-cli/src/commands/install.rs
View file

@ -204,50 +204,53 @@ fn ensure_table<'a>(table: &'a mut toml::Table, key: &str) -> Result<&'a mut tom
fn merge_server_settings(doc: &mut toml::Value, username: &str) -> Result<()> {
let root = root_table_mut(doc)?;
let web = ensure_table(root, "web")?;
root.insert("_version".to_string(), toml::Value::Integer(1));
let server = ensure_table(root, "server")?;
let api = ensure_table(server, "api")?;
api.insert(
"url".to_string(),
toml::Value::String("https://localhost:3000/api/v1".to_string()),
);
let listen = ensure_table(server, "listen")?;
listen.insert("type".to_string(), toml::Value::String("tcp".to_string()));
let listen_tls = ensure_table(listen, "tls")?;
let certs_dir = fabro_util::Home::from_env().certs_dir();
listen_tls.insert(
"cert".to_string(),
toml::Value::String(certs_dir.join("server.crt").to_string_lossy().to_string()),
);
listen_tls.insert(
"key".to_string(),
toml::Value::String(certs_dir.join("server.key").to_string_lossy().to_string()),
);
listen_tls.insert(
"ca".to_string(),
toml::Value::String(certs_dir.join("ca.crt").to_string_lossy().to_string()),
);
let web = ensure_table(server, "web")?;
web.insert("enabled".to_string(), toml::Value::Boolean(true));
web.insert(
"url".to_string(),
toml::Value::String("http://localhost:3000".to_string()),
);
let auth = ensure_table(web, "auth")?;
auth.insert(
"provider".to_string(),
toml::Value::String("github".to_string()),
);
auth.insert(
let auth = ensure_table(server, "auth")?;
let auth_api = ensure_table(auth, "api")?;
let jwt = ensure_table(auth_api, "jwt")?;
jwt.insert("enabled".to_string(), toml::Value::Boolean(true));
let mtls = ensure_table(auth_api, "mtls")?;
mtls.insert("enabled".to_string(), toml::Value::Boolean(true));
let auth_web = ensure_table(auth, "web")?;
auth_web.insert(
"allowed_usernames".to_string(),
toml::Value::Array(vec![toml::Value::String(username.to_string())]),
);
let api = ensure_table(root, "api")?;
api.insert(
"base_url".to_string(),
toml::Value::String("https://localhost:3000/api/v1".to_string()),
);
api.insert(
"authentication_strategies".to_string(),
toml::Value::Array(vec![
toml::Value::String("jwt".to_string()),
toml::Value::String("mtls".to_string()),
]),
);
let tls = ensure_table(api, "tls")?;
let certs_dir = fabro_util::Home::from_env().certs_dir();
tls.insert(
"cert".to_string(),
toml::Value::String(certs_dir.join("server.crt").to_string_lossy().to_string()),
);
tls.insert(
"key".to_string(),
toml::Value::String(certs_dir.join("server.key").to_string_lossy().to_string()),
);
tls.insert(
"ca".to_string(),
toml::Value::String(certs_dir.join("ca.crt").to_string_lossy().to_string()),
);
Ok(())
}
@ -1021,102 +1024,114 @@ mod tests {
#[test]
fn config_toml_roundtrips() {
use fabro_types::settings::v2::SettingsFile;
let toml_str = format_config_toml("brynary");
let settings: fabro_types::Settings =
toml::from_str(&toml_str).expect("config should parse");
assert_eq!(
settings.web.unwrap().auth.allowed_usernames,
vec!["brynary"]
);
let cfg: SettingsFile = fabro_config::ConfigLayer::parse(&toml_str)
.expect("generated config should parse as v2")
.into();
let allowed = cfg
.server
.as_ref()
.and_then(|s| s.auth.as_ref())
.and_then(|a| a.web.as_ref())
.map(|w| w.allowed_usernames.clone())
.expect("server.auth.web.allowed_usernames should be set");
assert_eq!(allowed, vec!["brynary".to_string()]);
}
#[test]
fn config_toml_has_auth_strategies() {
use fabro_types::settings::v2::SettingsFile;
let toml_str = format_config_toml("alice");
let settings: fabro_types::Settings = toml::from_str(&toml_str).unwrap();
assert_eq!(
settings.api.unwrap().authentication_strategies,
vec![
fabro_config::server::ApiAuthStrategy::Jwt,
fabro_config::server::ApiAuthStrategy::Mtls,
]
let cfg: SettingsFile = fabro_config::ConfigLayer::parse(&toml_str).unwrap().into();
let auth_api = cfg
.server
.as_ref()
.and_then(|s| s.auth.as_ref())
.and_then(|a| a.api.as_ref())
.expect("server.auth.api should be set");
assert!(
auth_api
.jwt
.as_ref()
.is_some_and(|jwt| jwt.enabled.unwrap_or(false))
);
assert!(
auth_api
.mtls
.as_ref()
.is_some_and(|mtls| mtls.enabled.unwrap_or(false))
);
}
#[test]
fn config_toml_has_tls_paths() {
use fabro_types::settings::v2::SettingsFile;
use fabro_types::settings::v2::server::ServerListenLayer;
let toml_str = format_config_toml("bob");
let settings: fabro_types::Settings = toml::from_str(&toml_str).unwrap();
let tls = settings.api.unwrap().tls.expect("tls should be set");
let cfg: SettingsFile = fabro_config::ConfigLayer::parse(&toml_str).unwrap().into();
let listen = cfg
.server
.as_ref()
.and_then(|s| s.listen.as_ref())
.expect("server.listen should be set");
let tls = match listen {
ServerListenLayer::Tcp { tls, .. } => tls.as_ref().expect("server.listen.tls"),
ServerListenLayer::Unix { .. } => panic!("expected tcp listen"),
};
let certs_dir = fabro_util::Home::from_env().certs_dir();
assert_eq!(tls.cert, certs_dir.join("server.crt"));
assert_eq!(tls.key, certs_dir.join("server.key"));
assert_eq!(tls.ca, certs_dir.join("ca.crt"));
assert_eq!(
tls.cert.as_ref().map(|c| c.as_source()),
Some(certs_dir.join("server.crt").to_string_lossy().into_owned())
);
assert_eq!(
tls.key.as_ref().map(|c| c.as_source()),
Some(certs_dir.join("server.key").to_string_lossy().into_owned())
);
assert_eq!(
tls.ca.as_ref().map(|c| c.as_source()),
Some(certs_dir.join("ca.crt").to_string_lossy().into_owned())
);
}
#[test]
fn merge_server_settings_preserves_existing_git_table() {
fn merge_server_settings_preserves_existing_top_level_sections() {
let mut doc: toml::Value = toml::from_str(
r#"
[git]
app_id = "123"
_version = 1
[git.author]
name = "fabro"
email = "fabro@example.com"
[project]
name = "custom"
"#,
)
.unwrap();
merge_server_settings(&mut doc, "alice").unwrap();
let git = doc.get("git").and_then(toml::Value::as_table).unwrap();
assert_eq!(git.get("app_id").and_then(toml::Value::as_str), Some("123"));
let author = git.get("author").and_then(toml::Value::as_table).unwrap();
// Existing top-level [project] stays.
assert_eq!(
author.get("name").and_then(toml::Value::as_str),
Some("fabro")
);
assert_eq!(
author.get("email").and_then(toml::Value::as_str),
Some("fabro@example.com")
);
assert_eq!(
doc.get("web")
doc.get("project")
.and_then(toml::Value::as_table)
.and_then(|web| web.get("auth"))
.and_then(|p| p.get("name"))
.and_then(toml::Value::as_str),
Some("custom")
);
// New server.auth.web.allowed_usernames is added.
assert_eq!(
doc.get("server")
.and_then(toml::Value::as_table)
.and_then(|auth| auth.get("allowed_usernames"))
.and_then(|s| s.get("auth"))
.and_then(toml::Value::as_table)
.and_then(|a| a.get("web"))
.and_then(toml::Value::as_table)
.and_then(|w| w.get("allowed_usernames"))
.and_then(toml::Value::as_array)
.and_then(|allowed| allowed.first())
.and_then(|u| u.first())
.and_then(toml::Value::as_str),
Some("alice")
);
}
#[test]
fn merge_server_settings_preserves_existing_api_nested_keys() {
let mut doc: toml::Value = toml::from_str(
r#"
[api]
base_url = "https://example.com/api/v1"
[api.extra]
mode = "keep-me"
"#,
)
.unwrap();
merge_server_settings(&mut doc, "alice").unwrap();
let api = doc.get("api").and_then(toml::Value::as_table).unwrap();
let extra = api.get("extra").and_then(toml::Value::as_table).unwrap();
assert_eq!(
extra.get("mode").and_then(toml::Value::as_str),
Some("keep-me")
);
}
// -- GitHub App manifest --
#[test]

105
lib/crates/fabro-types/src/settings/mod.rs
View file

@ -129,102 +129,9 @@ pub struct Settings {
pub fabro: Option<ProjectSettings>,
}
impl Settings {
pub fn app_id(&self) -> Option<&str> {
self.git.as_ref().and_then(|g| g.app_id.as_deref())
}
pub fn slug(&self) -> Option<&str> {
self.git.as_ref().and_then(|g| g.slug.as_deref())
}
pub fn client_id(&self) -> Option<&str> {
self.git.as_ref().and_then(|g| g.client_id.as_deref())
}
pub fn git_author(&self) -> Option<&GitAuthorSettings> {
self.git.as_ref().map(|g| &g.author)
}
pub fn sandbox_settings(&self) -> Option<&SandboxSettings> {
self.sandbox.as_ref()
}
pub fn setup_settings(&self) -> Option<&SetupSettings> {
self.setup.as_ref()
}
pub fn setup_commands(&self) -> &[String] {
self.setup
.as_ref()
.map_or(&[], |setup| setup.commands.as_slice())
}
pub fn setup_timeout_ms(&self) -> Option<u64> {
self.setup.as_ref().and_then(|setup| setup.timeout_ms)
}
pub fn preserve_sandbox_enabled(&self) -> bool {
self.sandbox
.as_ref()
.and_then(|sandbox| sandbox.preserve)
.unwrap_or(false)
}
pub fn github_permissions(&self) -> Option<&HashMap<String, String>> {
self.github
.as_ref()
.and_then(|github| (!github.permissions.is_empty()).then_some(&github.permissions))
}
pub fn mcp_server_entries(&self) -> &HashMap<String, McpServerEntry> {
&self.mcp_servers
}
pub fn verbose_enabled(&self) -> bool {
self.verbose.unwrap_or(false)
}
pub fn prevent_idle_sleep_enabled(&self) -> bool {
self.prevent_idle_sleep.unwrap_or(false)
}
pub fn upgrade_check_enabled(&self) -> bool {
self.upgrade_check.unwrap_or(true)
}
pub fn dry_run_enabled(&self) -> bool {
self.dry_run.unwrap_or(false)
}
pub fn auto_approve_enabled(&self) -> bool {
self.auto_approve.unwrap_or(false)
}
pub fn no_retro_enabled(&self) -> bool {
self.no_retro.unwrap_or(false)
}
pub fn storage_dir(&self) -> PathBuf {
self.storage_dir
.clone()
.unwrap_or_else(|| fabro_util::Home::from_env().storage_dir())
}
pub fn slack_settings(&self) -> Option<&SlackSettings> {
self.slack.as_ref()
}
}
#[cfg(test)]
mod tests {
use super::Settings;
#[test]
fn storage_dir_defaults_to_home_storage_subdir() {
assert_eq!(
Settings::default().storage_dir(),
fabro_util::Home::from_env().storage_dir()
);
}
}
// All inherent helpers on `Settings` are gone -- the v2 `SettingsFile`
// accessors in `settings::v2::accessors` are the single source of truth
// for reading merged configuration. The flat `Settings` struct itself
// lingers for the OpenAPI legacy `ServerSettings` response shape and a
// handful of demo-route payloads; Stage 6.6 finishes the deletion once
// the OpenAPI spec is rewritten to return v2 DTOs.