From 3ac7ab9035a1489159b32c74fc3173771465d791 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 18:27:46 -0400 Subject: [PATCH] refactor(settings): stage 6.3b shrink server runtime types + delete Combine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prunes `fabro-types/src/settings/server.rs` down to just the three types that still have live consumers: - `ApiAuthStrategy` — used by `fabro-server::jwt_auth::resolve_auth_mode_with_lookup` - `TlsSettings` — used by `fabro-server::tls::*` and the mTLS integration test - `ApiSettings` — the shim struct built by `fabro-server::serve::build_legacy_api_settings` so the pre-v2 `resolve_auth_mode_with_lookup` signature still compiles Deletes the rest as dead code (all unreferenced in the workspace): `AuthProvider`, `AuthSettings`, `GitProvider`, `GitSettings`, `GitAuthorSettings`, `WebSettings`, `WebhookSettings`, `WebhookStrategy`, `SlackSettings`, `FeaturesSettings`, `LogSettings`, `ArtifactStorageBackend`, `ArtifactStorageSettings`. Trims the `ApiSettings` struct itself to just the two fields the auth resolver reads; drops the never-used `base_url` field and the `build_legacy_api_settings` lines that were computing it. Drops `pub use settings::{ArtifactStorageBackend, ArtifactStorageSettings}` from `fabro-types/src/lib.rs`. Also deletes the dead `Combine` trait machinery alongside its only remaining consumers: - `lib/crates/fabro-types/src/combine.rs` — deleted. - `pub mod combine;` / `pub use fabro_macros::Combine;` removed from `fabro-types/src/lib.rs`. - `#[proc_macro_derive(Combine)] fn derive_combine` — deleted from `fabro-macros/src/lib.rs` along with its `syn::{Data, DeriveInput, Fields}` imports. The `e2e_test` proc-macro is untouched. The seven legacy runtime type modules (`hook`, `mcp`, `project`, `run`, `sandbox`, `user`, plus now the bulk of `server`) are effectively all gone. Only a tiny `server.rs` remains as a transitional home for the three auth-resolver types until Stage 6.6g rewrites `resolve_auth_mode_with_lookup` to walk the v2 `server.auth.api` subtree directly. 3,758 workspace tests pass. `cargo fmt --check --all` and `cargo clippy --workspace -- -D warnings` are clean. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-macros/src/lib.rs | 53 +----- lib/crates/fabro-server/src/serve.rs | 9 - lib/crates/fabro-types/src/combine.rs | 61 ------- lib/crates/fabro-types/src/lib.rs | 3 - lib/crates/fabro-types/src/settings/mod.rs | 6 +- lib/crates/fabro-types/src/settings/server.rs | 172 ++---------------- 6 files changed, 17 insertions(+), 287 deletions(-) delete mode 100644 lib/crates/fabro-types/src/combine.rs diff --git a/lib/crates/fabro-macros/src/lib.rs b/lib/crates/fabro-macros/src/lib.rs index 2ca0fbfce..2d1344ab6 100644 --- a/lib/crates/fabro-macros/src/lib.rs +++ b/lib/crates/fabro-macros/src/lib.rs @@ -2,9 +2,7 @@ use proc_macro::TokenStream; use quote::quote; use syn::parse::{Parse, ParseStream}; use syn::punctuated::Punctuated; -use syn::{ - Data, DeriveInput, Fields, Ident, ItemFn, LitStr, Token, parenthesized, parse_macro_input, -}; +use syn::{Ident, ItemFn, LitStr, Token, parenthesized, parse_macro_input}; enum E2eRequirement { Twin, @@ -33,55 +31,6 @@ impl Parse for E2eRequirement { } } -#[proc_macro_derive(Combine)] -pub fn derive_combine(input: TokenStream) -> TokenStream { - let input = parse_macro_input!(input as DeriveInput); - let ident = input.ident; - let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); - - let body = match input.data { - Data::Struct(data) => match data.fields { - Fields::Named(fields) => { - let combined = fields.named.into_iter().map(|field| { - let ident = field.ident.expect("named field"); - quote! { - #ident: ::fabro_types::combine::Combine::combine(self.#ident, other.#ident) - } - }); - quote! { - Self { - #(#combined,)* - } - } - } - Fields::Unnamed(fields) => { - let combined = fields.unnamed.iter().enumerate().map(|(index, _)| { - let index = syn::Index::from(index); - quote! { - ::fabro_types::combine::Combine::combine(self.#index, other.#index) - } - }); - quote! { - Self(#(#combined),*) - } - } - Fields::Unit => quote!(Self), - }, - Data::Enum(_) | Data::Union(_) => { - quote!(self) - } - }; - - quote! { - impl #impl_generics ::fabro_types::combine::Combine for #ident #ty_generics #where_clause { - fn combine(self, other: Self) -> Self { - #body - } - } - } - .into() -} - #[proc_macro_attribute] pub fn e2e_test(attr: TokenStream, item: TokenStream) -> TokenStream { let requirements = diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index df6d75322..99806c727 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -116,14 +116,6 @@ fn build_legacy_api_settings(file: &SettingsFile) -> ApiSettings { authentication_strategies.push(ApiAuthStrategy::Mtls); } - let base_url = file - .server_api() - .and_then(|api| api.url.as_ref()) - .map_or_else( - || "http://localhost:3000/api/v1".to_string(), - InterpString::as_source, - ); - // TLS files now live under `server.listen.tls.{cert,key,ca}` in v2. // Build a legacy TlsSettings from the listen TLS subtree so the // existing rustls config path keeps working. @@ -147,7 +139,6 @@ fn build_legacy_api_settings(file: &SettingsFile) -> ApiSettings { }); ApiSettings { - base_url, authentication_strategies, tls, } diff --git a/lib/crates/fabro-types/src/combine.rs b/lib/crates/fabro-types/src/combine.rs deleted file mode 100644 index 6014cb957..000000000 --- a/lib/crates/fabro-types/src/combine.rs +++ /dev/null @@ -1,61 +0,0 @@ -use std::collections::HashMap; -use std::hash::Hash; -use std::path::PathBuf; - -pub trait Combine { - #[must_use] - fn combine(self, other: Self) -> Self; -} - -impl Combine for Option { - fn combine(self, other: Self) -> Self { - match (self, other) { - (Some(this), Some(other)) => Some(this.combine(other)), - (Some(this), None) => Some(this), - (None, Some(other)) => Some(other), - (None, None) => None, - } - } -} - -impl Combine for Vec { - fn combine(mut self, other: Self) -> Self { - self.extend(other); - self - } -} - -impl Combine for HashMap -where - K: Eq + Hash, - V: Combine, -{ - fn combine(mut self, other: Self) -> Self { - for (key, value) in other { - match self.remove(&key) { - Some(existing) => { - self.insert(key, existing.combine(value)); - } - None => { - self.insert(key, value); - } - } - } - - self - } -} - -macro_rules! impl_left_wins { - ($($ty:ty),* $(,)?) => { - $( - impl Combine for $ty { - fn combine(self, _other: Self) -> Self { - self - } - } - )* - }; -} - -impl_left_wins!(bool, i32, u16, u32, u64, usize, String, PathBuf,); diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index 3f00da289..7b522f786 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -3,7 +3,6 @@ extern crate self as fabro_types; pub mod billing; pub mod blob_ref; pub mod checkpoint; -pub mod combine; pub mod conclusion; pub mod failure_signature; pub mod graph; @@ -33,7 +32,6 @@ pub use blob_ref::{ }; pub use checkpoint::Checkpoint; pub use conclusion::{Conclusion, StageSummary}; -pub use fabro_macros::Combine; pub use failure_signature::FailureSignature; pub use graph::{AttrValue, Edge, Graph, Node, is_llm_handler_type, shape_to_handler_type}; pub use interview::{InterviewQuestionRecord, InterviewQuestionType}; @@ -53,7 +51,6 @@ pub use run_event::{EventBody, RunEvent, RunNoticeLevel}; pub use run_id::RunId; pub use run_id::fixtures; pub use sandbox_record::SandboxRecord; -pub use settings::{ArtifactStorageBackend, ArtifactStorageSettings}; pub use stage_id::StageId; pub use start::StartRecord; pub use status::{ diff --git a/lib/crates/fabro-types/src/settings/mod.rs b/lib/crates/fabro-types/src/settings/mod.rs index de5da8e98..38003a3ad 100644 --- a/lib/crates/fabro-types/src/settings/mod.rs +++ b/lib/crates/fabro-types/src/settings/mod.rs @@ -22,11 +22,7 @@ pub mod server; pub mod v2; -pub use server::{ - ApiAuthStrategy, ApiSettings, ArtifactStorageBackend, ArtifactStorageSettings, AuthProvider, - AuthSettings, FeaturesSettings, GitAuthorSettings, GitProvider, GitSettings, LogSettings, - SlackSettings, TlsSettings, WebSettings, WebhookSettings, WebhookStrategy, -}; +pub use server::{ApiAuthStrategy, ApiSettings, TlsSettings}; // v2 top-level re-exports. Stage 6.5 of the settings TOML redesign // promoted the v2 namespaced parse tree to be the primary API surface; // consumers can now write `fabro_types::settings::SettingsFile` / diff --git a/lib/crates/fabro-types/src/settings/server.rs b/lib/crates/fabro-types/src/settings/server.rs index b1f006932..7562290ff 100644 --- a/lib/crates/fabro-types/src/settings/server.rs +++ b/lib/crates/fabro-types/src/settings/server.rs @@ -1,34 +1,24 @@ +//! Transitional server-domain runtime types. +//! +//! Only the three types that the auth resolver (`jwt_auth.rs`) and the +//! TLS loader (`tls.rs`) still consume remain here. Stage 6.6g will +//! rewrite those consumers to walk the v2 `server.auth.api` / `server.listen.tls` +//! subtrees directly, at which point this file goes away and the +//! legacy runtime type module tree is fully gone. + use std::path::PathBuf; use serde::{Deserialize, Serialize}; -fn default_artifact_storage_prefix() -> String { - "artifacts".to_string() -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -#[serde(rename_all = "snake_case")] -pub enum AuthProvider { - #[default] - Github, - InsecureDisabled, -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize)] -pub struct AuthSettings { - #[serde(default)] - pub provider: AuthProvider, - #[serde(default)] - pub allowed_usernames: Vec, -} - -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize, crate::Combine)] +/// Authentication strategy flag consumed by `resolve_auth_mode_with_lookup`. +#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] #[serde(rename_all = "snake_case")] pub enum ApiAuthStrategy { Jwt, Mtls, } +/// mTLS material loaded by `fabro_server::tls::*`. #[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] pub struct TlsSettings { pub cert: PathBuf, @@ -36,144 +26,12 @@ pub struct TlsSettings { pub ca: PathBuf, } -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] +/// Shim `ApiSettings` that `fabro_server::serve::build_legacy_api_settings` +/// projects out of the v2 tree so the pre-v2 `resolve_auth_mode_with_lookup` +/// signature keeps working until Stage 6.6g rewrites it. +#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize)] pub struct ApiSettings { - #[serde(default = "default_base_url")] - pub base_url: String, #[serde(default)] pub authentication_strategies: Vec, pub tls: Option, } - -fn default_base_url() -> String { - "http://localhost:3000/api/v1".to_string() -} - -impl Default for ApiSettings { - fn default() -> Self { - Self { - base_url: default_base_url(), - authentication_strategies: Vec::new(), - tls: None, - } - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -#[serde(rename_all = "snake_case")] -pub enum GitProvider { - #[default] - Github, -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize)] -pub struct GitAuthorSettings { - pub name: Option, - pub email: Option, -} - -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize, crate::Combine)] -#[serde(rename_all = "snake_case")] -pub enum WebhookStrategy { - TailscaleFunnel, -} - -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] -pub struct WebhookSettings { - pub strategy: WebhookStrategy, -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize)] -pub struct GitSettings { - #[serde(default)] - pub provider: GitProvider, - pub app_id: Option, - pub client_id: Option, - pub slug: Option, - #[serde(default)] - pub author: GitAuthorSettings, - pub webhooks: Option, -} - -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] -pub struct WebSettings { - #[serde(default = "default_web_enabled")] - pub enabled: bool, - #[serde(default = "default_web_url")] - pub url: String, - #[serde(default)] - pub auth: AuthSettings, -} - -fn default_web_enabled() -> bool { - true -} - -fn default_web_url() -> String { - "http://localhost:3000".to_string() -} - -impl Default for WebSettings { - fn default() -> Self { - Self { - enabled: default_web_enabled(), - url: default_web_url(), - auth: AuthSettings::default(), - } - } -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct SlackSettings { - pub default_channel: Option, -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize)] -pub struct FeaturesSettings { - #[serde(default)] - pub session_sandboxes: bool, - #[serde(default)] - pub retros: bool, -} - -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] -pub struct LogSettings { - pub level: Option, -} - -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize, crate::Combine)] -#[serde(rename_all = "snake_case")] -pub enum ArtifactStorageBackend { - #[default] - Local, - S3, -} - -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize, crate::Combine)] -pub struct ArtifactStorageSettings { - #[serde(default)] - pub backend: ArtifactStorageBackend, - #[serde(default = "default_artifact_storage_prefix")] - pub prefix: String, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub bucket: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub region: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub endpoint: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub path_style: Option, -} - -impl Default for ArtifactStorageSettings { - fn default() -> Self { - Self { - backend: ArtifactStorageBackend::Local, - prefix: default_artifact_storage_prefix(), - bucket: None, - region: None, - endpoint: None, - path_style: None, - } - } -}