refactor(settings): remove bridge shims and restore contracts

Drop the dead sandbox and hook bridge helpers that no longer have runtime
callers, and move the run settings serde coverage into fabro-types where the
wire types live. Add the missing /api/v1/runs/:id/settings contract test so the
outward sparse settings shape stays covered after the refactor.
This commit is contained in:
Bryan Helmkamp 2026-04-10 08:35:08 -04:00
parent 9a43606759
commit 1dfb8fc272
No known key found for this signature in database
7 changed files with 324 additions and 287 deletions

View file

@ -1,7 +1,5 @@
use fabro_config::effective_settings::{EffectiveSettingsLayers, EffectiveSettingsMode};
use fabro_config::parse_settings_layer;
use fabro_types::run::RunRecord;
use fabro_types::run_event::run::RunCreatedProps;
use fabro_types::settings::SettingsLayer;
fn parse(source: &str) -> SettingsLayer {
@ -115,87 +113,3 @@ name = "gpt-5"
Some("gpt-5".to_string())
);
}
#[test]
fn run_record_round_trips_templated_settings() {
let settings = parse(
r#"
_version = 1
[server.storage]
root = "${env.FABRO_STORAGE}"
"#,
);
let record = RunRecord {
run_id: fabro_types::fixtures::RUN_1,
settings,
graph: fabro_types::graph::Graph::new("test"),
workflow_slug: Some("demo".to_string()),
working_directory: std::path::PathBuf::from("/tmp/project"),
host_repo_path: Some("/tmp/project".to_string()),
repo_origin_url: None,
base_branch: Some("main".to_string()),
labels: std::collections::HashMap::new(),
provenance: None,
manifest_blob: None,
definition_blob: None,
};
let json = serde_json::to_value(&record).expect("record should serialize");
let round_trip: RunRecord = serde_json::from_value(json).expect("record should deserialize");
assert_eq!(
round_trip
.settings
.server
.as_ref()
.and_then(|server| server.storage.as_ref())
.and_then(|storage| storage.root.as_ref())
.map(|value| value.as_source()),
Some("${env.FABRO_STORAGE}".to_string())
);
}
#[test]
fn run_created_props_round_trips_templated_settings() {
let settings = parse(
r#"
_version = 1
[server.integrations.github]
app_id = "${env.GITHUB_APP_ID}"
"#,
);
let event = RunCreatedProps {
settings,
graph: fabro_types::graph::Graph::new("test"),
workflow_source: Some("digraph test { start -> exit }".to_string()),
workflow_config: None,
labels: std::collections::BTreeMap::new(),
run_dir: "/tmp/run".to_string(),
working_directory: "/tmp/project".to_string(),
host_repo_path: Some("/tmp/project".to_string()),
repo_origin_url: None,
base_branch: Some("main".to_string()),
workflow_slug: Some("demo".to_string()),
db_prefix: None,
provenance: None,
manifest_blob: None,
};
let json = serde_json::to_value(&event).expect("event should serialize");
let round_trip: RunCreatedProps =
serde_json::from_value(json).expect("event should deserialize");
assert_eq!(
round_trip
.settings
.server
.as_ref()
.and_then(|server| server.integrations.as_ref())
.and_then(|integrations| integrations.github.as_ref())
.and_then(|github| github.app_id.as_ref())
.map(|value| value.as_source()),
Some("${env.GITHUB_APP_ID}".to_string())
);
}

View file

@ -6,17 +6,9 @@
//! behavior methods (`is_blocking`, `timeout`, `resolved_hook_type`,
//! `runs_in_sandbox`, `effective_name`) are runtime concerns owned by the
//! executor.
//!
//! [`bridge_hook`] converts a v2 `HookEntry` into the runtime
//! [`HookDefinition`] and lives here (not in `fabro-types`) so the runtime
//! shape stays owned by this crate.
use std::borrow::Cow;
use fabro_types::settings::InterpString;
use fabro_types::settings::run::{
HookAgentMarker, HookEntry, HookEvent as V2HookEvent, HookTlsMode as V2HookTlsMode,
};
use serde::{Deserialize, Serialize};
/// Lifecycle events that can trigger user-defined hooks.
@ -244,107 +236,3 @@ impl HookSettings {
Self { hooks }
}
}
/// Convert a v2 [`HookEntry`] into the runtime [`HookDefinition`] shape
/// this crate's executor consumes.
#[must_use]
pub fn bridge_hook(hook: &HookEntry) -> HookDefinition {
let hook_type = resolve_hook_type(hook);
// If the hook is a script/command form, emit via the shorthand so
// HookDefinition.command holds the full command and
// HookDefinition.hook_type stays None. This avoids the duplicate
// `command` key that would otherwise appear under `#[serde(flatten)]`.
let command = if let Some(script) = &hook.script {
Some(interp_to_string(script))
} else {
hook.command.as_ref().map(|command| {
command
.iter()
.map(interp_to_string)
.collect::<Vec<_>>()
.join(" ")
})
};
HookDefinition {
name: hook.name.clone().or_else(|| hook.id.clone()),
event: bridge_hook_event(hook.event),
command,
hook_type,
matcher: hook.matcher.clone(),
blocking: hook.blocking,
timeout_ms: hook
.timeout
.map(|d| u64::try_from(d.as_std().as_millis()).unwrap_or(u64::MAX)),
sandbox: hook.sandbox,
}
}
fn resolve_hook_type(hook: &HookEntry) -> Option<HookType> {
if hook.script.is_some() || hook.command.is_some() {
return None;
}
if let Some(url) = &hook.url {
let headers = if hook.headers.is_empty() {
None
} else {
Some(
hook.headers
.iter()
.map(|(k, v)| (k.clone(), interp_to_string(v)))
.collect(),
)
};
let tls = match hook.tls {
Some(V2HookTlsMode::Verify) => TlsMode::Verify,
Some(V2HookTlsMode::NoVerify) => TlsMode::NoVerify,
Some(V2HookTlsMode::Off) => TlsMode::Off,
None => TlsMode::default(),
};
return Some(HookType::Http {
url: interp_to_string(url),
headers,
allowed_env_vars: hook.allowed_env_vars.clone(),
tls,
});
}
if matches!(hook.agent, Some(HookAgentMarker::Enabled)) {
return Some(HookType::Agent {
prompt: hook
.prompt
.as_ref()
.map(interp_to_string)
.unwrap_or_default(),
model: hook.model.as_ref().map(interp_to_string),
max_tool_rounds: hook.max_tool_rounds,
});
}
hook.prompt.as_ref().map(|prompt| HookType::Prompt {
prompt: interp_to_string(prompt),
model: hook.model.as_ref().map(interp_to_string),
})
}
fn bridge_hook_event(event: V2HookEvent) -> HookEvent {
match event {
V2HookEvent::RunStart => HookEvent::RunStart,
V2HookEvent::RunComplete => HookEvent::RunComplete,
V2HookEvent::RunFailed => HookEvent::RunFailed,
V2HookEvent::StageStart => HookEvent::StageStart,
V2HookEvent::StageComplete => HookEvent::StageComplete,
V2HookEvent::StageFailed => HookEvent::StageFailed,
V2HookEvent::StageRetrying => HookEvent::StageRetrying,
V2HookEvent::EdgeSelected => HookEvent::EdgeSelected,
V2HookEvent::ParallelStart => HookEvent::ParallelStart,
V2HookEvent::ParallelComplete => HookEvent::ParallelComplete,
V2HookEvent::SandboxReady => HookEvent::SandboxReady,
V2HookEvent::SandboxCleanup => HookEvent::SandboxCleanup,
V2HookEvent::CheckpointSaved => HookEvent::CheckpointSaved,
V2HookEvent::PreToolUse => HookEvent::PreToolUse,
V2HookEvent::PostToolUse => HookEvent::PostToolUse,
V2HookEvent::PostToolUseFailure => HookEvent::PostToolUseFailure,
}
}
fn interp_to_string(value: &InterpString) -> String {
value.as_source()
}

View file

@ -1,8 +1,6 @@
//! Sandbox configuration runtime types.
//!
//! These types are the runtime shape that the sandbox providers consume.
//! The v2 parse tree lives in `fabro_types::settings::run::RunSandboxLayer`.
//! Conversion from the v2 shape lives in [`bridge_sandbox`].
//!
//! The `DaytonaSettings`/`DaytonaSnapshotSettings` names are kept for
//! backward compatibility with the old import path; [`crate::daytona`]
@ -11,10 +9,7 @@
use std::collections::HashMap;
use fabro_types::settings::InterpString;
use fabro_types::settings::run::{
DaytonaDockerfileLayer, DaytonaNetworkLayer, RunSandboxLayer, WorktreeMode as V2WorktreeMode,
};
use fabro_types::settings::run::WorktreeMode as V2WorktreeMode;
use serde::de::{self, MapAccess, Visitor};
use serde::{Deserialize, Serialize};
@ -139,80 +134,6 @@ pub enum WorktreeMode {
Never,
}
#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
pub struct LocalSandboxSettings {
#[serde(default)]
pub worktree_mode: WorktreeMode,
}
#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)]
pub struct SandboxSettings {
pub provider: Option<String>,
pub preserve: Option<bool>,
pub devcontainer: Option<bool>,
pub local: Option<LocalSandboxSettings>,
pub daytona: Option<DaytonaSettings>,
pub env: Option<HashMap<String, String>>,
}
/// Convert a v2 [`RunSandboxLayer`] into the runtime [`SandboxSettings`] shape.
#[must_use]
pub fn bridge_sandbox(sb: &RunSandboxLayer) -> SandboxSettings {
SandboxSettings {
provider: sb.provider.clone(),
preserve: sb.preserve,
devcontainer: sb.devcontainer,
local: sb.local.as_ref().map(|local| LocalSandboxSettings {
worktree_mode: local
.worktree_mode
.map(bridge_worktree_mode)
.unwrap_or_default(),
}),
daytona: sb.daytona.as_ref().map(|d| DaytonaSettings {
auto_stop_interval: d.auto_stop_interval,
labels: if d.labels.is_empty() {
None
} else {
Some(d.labels.clone())
},
snapshot: d.snapshot.as_ref().and_then(|s| {
s.name.as_ref().map(|name| DaytonaSnapshotSettings {
name: name.clone(),
cpu: s.cpu,
memory: s.memory.map(|sz| size_to_gb_i32(sz.as_bytes())),
disk: s.disk.map(|sz| size_to_gb_i32(sz.as_bytes())),
dockerfile: s.dockerfile.as_ref().map(|d| match d {
DaytonaDockerfileLayer::Inline(text) => {
DockerfileSource::Inline(text.clone())
}
DaytonaDockerfileLayer::Path { path } => {
DockerfileSource::Path { path: path.clone() }
}
}),
})
}),
network: d.network.as_ref().map(|n| match n {
DaytonaNetworkLayer::Block => DaytonaNetwork::Block,
DaytonaNetworkLayer::AllowAll => DaytonaNetwork::AllowAll,
DaytonaNetworkLayer::AllowList { allow_list } => {
DaytonaNetwork::AllowList(allow_list.clone())
}
}),
skip_clone: d.skip_clone.unwrap_or(false),
}),
env: if sb.env.is_empty() {
None
} else {
Some(
sb.env
.iter()
.map(|(k, v)| (k.clone(), interp_to_string(v)))
.collect(),
)
},
}
}
/// Convert a v2 [`V2WorktreeMode`] into the runtime [`WorktreeMode`].
#[must_use]
pub fn bridge_worktree_mode(m: V2WorktreeMode) -> WorktreeMode {
@ -223,12 +144,3 @@ pub fn bridge_worktree_mode(m: V2WorktreeMode) -> WorktreeMode {
V2WorktreeMode::Never => WorktreeMode::Never,
}
}
fn interp_to_string(value: &InterpString) -> String {
value.as_source()
}
fn size_to_gb_i32(bytes: u64) -> i32 {
let gb = bytes / 1_000_000_000;
i32::try_from(gb).unwrap_or(i32::MAX)
}

View file

@ -1,5 +1,6 @@
#[cfg(target_os = "linux")]
mod mtls;
mod routing;
mod runs;
mod settings;
mod system;

View file

@ -0,0 +1,132 @@
use axum::body::Body;
use axum::http::{Request, StatusCode};
use fabro_config::parse_settings_layer;
use fabro_server::jwt_auth::AuthMode;
use fabro_server::server::build_router;
use tower::ServiceExt;
use crate::helpers::{
MINIMAL_DOT, api, body_json, minimal_manifest_json, test_app_state_with_options,
};
#[tokio::test]
async fn retrieve_run_settings_preserves_templates_and_redacts_sensitive_fields() {
let storage_dir = tempfile::tempdir().unwrap();
let settings = parse_settings_layer(&format!(
r#"
_version = 1
[server.listen]
type = "tcp"
address = "127.0.0.1:32276"
[server.listen.tls]
cert = "/etc/fabro/tls/cert.pem"
key = "/etc/fabro/tls/key.pem"
ca = "/etc/fabro/tls/ca.pem"
[server.auth.api.jwt]
enabled = true
issuer = "https://auth.example.com"
audience = "${{env.JWT_AUDIENCE}}"
[server.auth.api.mtls]
enabled = true
ca = "/etc/fabro/tls/ca.pem"
[server.auth.web.providers.github]
enabled = true
client_id = "Iv1.abcdef"
client_secret = "${{env.GITHUB_OAUTH_SECRET}}"
[server.storage]
root = "{}"
[server.scheduler]
max_concurrent_runs = 9
[server.integrations.github]
app_id = "${{env.GITHUB_APP_ID}}"
client_id = "Iv1.github"
slug = "fabro-app"
"#,
storage_dir.path().display()
))
.expect("settings fixture should parse");
let app = build_router(test_app_state_with_options(settings, 5), AuthMode::Disabled);
let mut manifest = minimal_manifest_json(MINIMAL_DOT);
manifest["configs"] = serde_json::json!([{
"type": "user",
"path": "/tmp/home/.fabro/settings.toml",
"source": r#"
_version = 1
[run]
goal = "Ship it"
[cli.output]
verbosity = "verbose"
[features]
session_sandboxes = true
"#
}]);
let create_request = Request::builder()
.method("POST")
.uri(api("/runs"))
.header("content-type", "application/json")
.body(Body::from(serde_json::to_string(&manifest).unwrap()))
.unwrap();
let create_response = app.clone().oneshot(create_request).await.unwrap();
let create_status = create_response.status();
let create_body = body_json(create_response.into_body()).await;
assert_eq!(create_status, StatusCode::CREATED, "{create_body}");
let run_id = create_body["id"]
.as_str()
.expect("run ID should be present");
let get_request = Request::builder()
.method("GET")
.uri(api(&format!("/runs/{run_id}/settings")))
.body(Body::empty())
.unwrap();
let response = app.oneshot(get_request).await.unwrap();
assert_eq!(response.status(), StatusCode::OK);
let body = body_json(response.into_body()).await;
assert_eq!(body["_version"], 1);
assert_eq!(body["run"]["goal"], "Ship it");
assert_eq!(body["cli"]["output"]["verbosity"], "verbose");
assert_eq!(body["features"]["session_sandboxes"], true);
assert_eq!(
body["server"]["storage"]["root"],
storage_dir.path().display().to_string()
);
assert_eq!(body["server"]["scheduler"]["max_concurrent_runs"], 9);
assert_eq!(
body["server"]["integrations"]["github"]["app_id"],
"${env.GITHUB_APP_ID}"
);
assert_eq!(
body["server"]["auth"]["api"]["jwt"]["enabled"],
serde_json::json!(true)
);
assert_eq!(
body["server"]["auth"]["api"]["mtls"]["enabled"],
serde_json::json!(true)
);
assert_eq!(
body["server"]["auth"]["web"]["providers"]["github"]["client_id"],
"Iv1.abcdef"
);
assert!(body.pointer("/server/listen").is_none());
assert!(body.pointer("/server/auth/api/jwt/issuer").is_none());
assert!(body.pointer("/server/auth/api/jwt/audience").is_none());
assert!(body.pointer("/server/auth/api/mtls/ca").is_none());
assert!(
body.pointer("/server/auth/web/providers/github/client_secret")
.is_none()
);
}

View file

@ -0,0 +1,95 @@
use std::collections::BTreeMap;
use fabro_types::graph::Graph;
use fabro_types::run_event::run::RunCreatedProps;
use fabro_types::settings::run::{RunGoalLayer, RunLayer};
use fabro_types::settings::server::{
GithubIntegrationLayer, ServerIntegrationsLayer, ServerLayer, ServerStorageLayer,
};
use fabro_types::settings::{InterpString, SettingsLayer};
fn templated_settings() -> SettingsLayer {
SettingsLayer {
version: Some(1),
run: Some(RunLayer {
goal: Some(RunGoalLayer::Inline(InterpString::parse(
"Ship ${env.TASK}",
))),
..RunLayer::default()
}),
server: Some(ServerLayer {
storage: Some(ServerStorageLayer {
root: Some(InterpString::parse("${env.FABRO_STORAGE}")),
}),
integrations: Some(ServerIntegrationsLayer {
github: Some(GithubIntegrationLayer {
app_id: Some(InterpString::parse("${env.GITHUB_APP_ID}")),
..GithubIntegrationLayer::default()
}),
..ServerIntegrationsLayer::default()
}),
..ServerLayer::default()
}),
..SettingsLayer::default()
}
}
#[test]
fn run_created_props_round_trip_templated_settings() {
let props = RunCreatedProps {
settings: templated_settings(),
graph: Graph::new("ship"),
workflow_source: Some("digraph Ship { start -> exit }".to_string()),
workflow_config: Some("[run]\ngoal = \"Ship ${env.TASK}\"".to_string()),
labels: BTreeMap::from([("team".to_string(), "platform".to_string())]),
run_dir: "/tmp/run".to_string(),
working_directory: "/tmp/project".to_string(),
host_repo_path: Some("/tmp/project".to_string()),
repo_origin_url: Some("https://github.com/fabro-sh/fabro.git".to_string()),
base_branch: Some("main".to_string()),
workflow_slug: Some("demo".to_string()),
db_prefix: Some("run_".to_string()),
provenance: None,
manifest_blob: None,
};
let json = serde_json::to_value(&props).expect("props should serialize");
let round_trip: RunCreatedProps =
serde_json::from_value(json.clone()).expect("props should deserialize");
assert_eq!(
serde_json::to_value(&round_trip).expect("round-trip should serialize"),
json
);
assert_eq!(
round_trip
.settings
.run
.as_ref()
.and_then(|run| run.goal.as_ref()),
Some(&RunGoalLayer::Inline(InterpString::parse(
"Ship ${env.TASK}"
)))
);
assert_eq!(
round_trip
.settings
.server
.as_ref()
.and_then(|server| server.storage.as_ref())
.and_then(|storage| storage.root.as_ref())
.map(|value| value.as_source()),
Some("${env.FABRO_STORAGE}".to_string())
);
assert_eq!(
round_trip
.settings
.server
.as_ref()
.and_then(|server| server.integrations.as_ref())
.and_then(|integrations| integrations.github.as_ref())
.and_then(|github| github.app_id.as_ref())
.map(|value| value.as_source()),
Some("${env.GITHUB_APP_ID}".to_string())
);
}

View file

@ -0,0 +1,95 @@
use std::collections::HashMap;
use std::path::PathBuf;
use fabro_types::fixtures;
use fabro_types::graph::Graph;
use fabro_types::run::RunRecord;
use fabro_types::settings::run::{RunGoalLayer, RunLayer};
use fabro_types::settings::server::{
GithubIntegrationLayer, ServerIntegrationsLayer, ServerLayer, ServerStorageLayer,
};
use fabro_types::settings::{InterpString, SettingsLayer};
fn templated_settings() -> SettingsLayer {
SettingsLayer {
version: Some(1),
run: Some(RunLayer {
goal: Some(RunGoalLayer::Inline(InterpString::parse(
"Ship ${env.TASK}",
))),
..RunLayer::default()
}),
server: Some(ServerLayer {
storage: Some(ServerStorageLayer {
root: Some(InterpString::parse("${env.FABRO_STORAGE}")),
}),
integrations: Some(ServerIntegrationsLayer {
github: Some(GithubIntegrationLayer {
app_id: Some(InterpString::parse("${env.GITHUB_APP_ID}")),
..GithubIntegrationLayer::default()
}),
..ServerIntegrationsLayer::default()
}),
..ServerLayer::default()
}),
..SettingsLayer::default()
}
}
#[test]
fn run_record_round_trips_templated_settings() {
let record = RunRecord {
run_id: fixtures::RUN_1,
settings: templated_settings(),
graph: Graph::new("ship"),
workflow_slug: Some("demo".to_string()),
working_directory: PathBuf::from("/tmp/project"),
host_repo_path: Some("/tmp/project".to_string()),
repo_origin_url: Some("https://github.com/fabro-sh/fabro.git".to_string()),
base_branch: Some("main".to_string()),
labels: HashMap::from([("team".to_string(), "platform".to_string())]),
provenance: None,
manifest_blob: None,
definition_blob: None,
};
let json = serde_json::to_value(&record).expect("record should serialize");
let round_trip: RunRecord =
serde_json::from_value(json.clone()).expect("record should deserialize");
assert_eq!(
serde_json::to_value(&round_trip).expect("round-trip should serialize"),
json
);
assert_eq!(
round_trip
.settings
.run
.as_ref()
.and_then(|run| run.goal.as_ref()),
Some(&RunGoalLayer::Inline(InterpString::parse(
"Ship ${env.TASK}"
)))
);
assert_eq!(
round_trip
.settings
.server
.as_ref()
.and_then(|server| server.storage.as_ref())
.and_then(|storage| storage.root.as_ref())
.map(|value| value.as_source()),
Some("${env.FABRO_STORAGE}".to_string())
);
assert_eq!(
round_trip
.settings
.server
.as_ref()
.and_then(|server| server.integrations.as_ref())
.and_then(|integrations| integrations.github.as_ref())
.and_then(|github| github.app_id.as_ref())
.map(|value| value.as_source()),
Some("${env.GITHUB_APP_ID}".to_string())
);
}