scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-13 23:14:17 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

feat(server): preserve comments in setup_register TOML edits

Browse source 
`setup_register` in `web_auth.rs` used to round-trip the user's
settings file through `toml::Value` + `toml::to_string_pretty`, which
strips every comment, blank line, and explicit key ordering on the
way out. A user who'd hand-commented their `~/.fabro/settings.toml`
would see all of that lost on the next GitHub App registration.

Switches the edit path to `toml_edit::DocumentMut`, which preserves
prefix decoration (comments, blank lines) on every key. Adds
`toml_edit = "0.22"` as a workspace dependency (already pulled in
transitively via `toml 0.8`) and declares it in `fabro-server`.

Implementation notes:

- New `ensure_nested_table(doc, &["server", "web"])` walks a dotted
  path and `or_insert`s missing intermediate tables without touching
  existing ones.
- New `set_preserving_decor(table, key, value)` replaces an entry's
  value while copying the old key's `leaf_decor` forward. Without
  that workaround, `toml_edit::Table::insert` drops the prefix
  decoration of the replaced key -- which would strip a top-of-file
  comment attached to `_version = 1` or any other value we update.
- `_version` is only inserted when missing; it's always `1` today, so
  rewriting it every time is unnecessary and would trample its decor.
- `merge_settings_keys` now takes `&mut toml_edit::DocumentMut`
  instead of `&mut toml::Value`. The flow in `setup_register` parses
  the file on disk into a `DocumentMut`, applies the merge, and
  writes `doc.to_string()` back.

Adds a new test
`merge_settings_keys_preserves_comments_and_unrelated_keys` that
round-trips a fixture file containing:

- A top-of-file comment attached to `_version`
- A comment above `[server.storage]`
- A comment above a pre-existing `[server.integrations.slack]` table
- Unrelated keys in `[server.storage]`, `[server.integrations.slack]`,
  and `[run.model]`

and asserts that every comment and every unrelated key survives the
merge, that the new GitHub App keys are present, and that the final
output still parses as a valid v2 `SettingsFile` via
`fabro_config::ConfigLayer::parse`.

Also strengthens the existing
`merge_settings_keys_writes_v2_server_integrations_github` test with
a round-trip parse of the emitted TOML through `ConfigLayer::parse`
to ensure the output is real v2 config, not just a JSON-shaped blob.

3,765 workspace tests pass (+1 new). `cargo fmt --check --all` and
`cargo clippy --workspace -- -D warnings` are clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-09 19:18:10 -04:00
parent d4fb73d614
commit fac0b10244
No known key found for this signature in database
4 changed files with 177 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock generated
View file

@ -1929,6 +1929,7 @@ dependencies = [
"tokio-rustls",
"tokio-stream",
"toml 0.8.23",
"toml_edit",
"tower",
"tower-http",
"tower-service",

1
Cargo.toml
View file

@ -53,6 +53,7 @@ mime_guess = "2"
indicatif = "0.18"
termimad = "0.34"
toml = "0.8"
toml_edit = "0.22"
jsonwebtoken = { version = "10", features = ["aws_lc_rs"] }
hmac = "0.12"
sha2 = "0.10"

1
lib/crates/fabro-server/Cargo.toml
View file

@ -58,6 +58,7 @@ serde_yaml = "0.9"
anyhow.workspace = true
clap.workspace = true
toml.workspace = true
toml_edit.workspace = true
tracing.workspace = true
ulid.workspace = true
uuid.workspace = true

241
lib/crates/fabro-server/src/web_auth.rs
View file

@ -559,18 +559,20 @@ async fn setup_register(
let settings_path = state.config_path.clone();
// Build a v2 settings_path edit in place. This used to bridge back to
// the legacy flat shape and emit v1 TOML; the v2 parser hard-rejects
// the v1 top-level keys, so this was already broken. Write v2 TOML
// using `merge_settings_keys` against the raw TOML document.
// Edit the settings file in place via `toml_edit::DocumentMut`, which
// preserves existing comments, whitespace, and key ordering. The value-
// tree parser (`toml::Value`) would strip all of that on round-trip.
if let Some(parent) = settings_path.parent() {
let _ = std::fs::create_dir_all(parent);
}
let existing = std::fs::read_to_string(&settings_path).unwrap_or_default();
let mut doc: toml::Value = if existing.is_empty() {
toml::Value::Table(toml::Table::default())
let mut doc: toml_edit::DocumentMut = if existing.is_empty() {
toml_edit::DocumentMut::new()
} else {
match toml::from_str(&existing).context("failed to parse existing settings config") {
match existing
.parse::<toml_edit::DocumentMut>()
.context("failed to parse existing settings config")
{
Ok(doc) => doc,
Err(err) => {
error!(error = %err, path = %settings_path.display(), "Setup register failed: could not parse settings config");
@ -588,10 +590,7 @@ async fn setup_register(
json!({"error": format!("Failed to update settings config: {err}")}),
);
}
if let Err(err) = std::fs::write(
&settings_path,
toml::to_string_pretty(&doc).unwrap_or_default(),
) {
if let Err(err) = std::fs::write(&settings_path, doc.to_string()) {
error!(error = %err, path = %settings_path.display(), "Setup register failed: could not write settings config");
return json_response(
StatusCode::INTERNAL_SERVER_ERROR,
@ -635,52 +634,76 @@ async fn setup_register(
Json(json!({"ok": true})).into_response()
}
fn root_table_mut(doc: &mut toml::Value) -> anyhow::Result<&mut toml::Table> {
doc.as_table_mut()
.ok_or_else(|| anyhow!("settings config root is not a table"))
/// Walk dotted `path` into `doc`, creating missing intermediate tables,
/// and return a mutable reference to the terminal table.
///
/// Uses `toml_edit`'s [`toml_edit::Entry::or_insert`] so existing tables
/// keep their comments, ordering, and any sibling keys untouched.
fn ensure_nested_table<'a>(
doc: &'a mut toml_edit::DocumentMut,
path: &[&str],
) -> anyhow::Result<&'a mut toml_edit::Table> {
let mut current: &mut toml_edit::Table = doc.as_table_mut();
for segment in path {
let next = current
.entry(segment)
.or_insert(toml_edit::Item::Table(toml_edit::Table::new()));
current = next
.as_table_mut()
.ok_or_else(|| anyhow!("settings config [{segment}] is not a table"))?;
}
Ok(current)
}
fn ensure_table<'a>(table: &'a mut toml::Table, key: &str) -> anyhow::Result<&'a mut toml::Table> {
table
.entry(key.to_string())
.or_insert_with(|| toml::Value::Table(toml::Table::default()))
.as_table_mut()
.ok_or_else(|| anyhow!("settings config [{key}] is not a table"))
/// Set a scalar value on `table[key]`, preserving any key-level decor
/// (leading comments, blank lines) that was attached to the existing entry.
///
/// `toml_edit`'s default `Table::insert` replaces the entry wholesale and
/// drops its prefix decoration, which would strip a top-of-file comment
/// attached to a key we're updating. Copying the decor forward keeps the
/// user's formatting intact.
fn set_preserving_decor(table: &mut toml_edit::Table, key: &str, value: toml_edit::Item) {
let preserved_decor = table.key(key).map(|existing| existing.leaf_decor().clone());
table.insert(key, value);
if let (Some(decor), Some(mut updated)) = (preserved_decor, table.key_mut(key)) {
*updated.leaf_decor_mut() = decor;
}
}
fn merge_settings_keys(
doc: &mut toml::Value,
doc: &mut toml_edit::DocumentMut,
data: &GitHubManifestConversion,
origin: Option<&str>,
) -> anyhow::Result<()> {
let web_url = origin.map_or_else(|| "http://localhost:3000".to_string(), str::to_string);
let root = root_table_mut(doc)?;
// Make sure the freshly-written file is a valid v2 file.
root.insert("_version".to_string(), toml::Value::Integer(1));
// Make sure the freshly-written file is a valid v2 file. `_version` is
// always `1` at the moment, so skip the write entirely if it's already
// there -- otherwise we'd trample any top-of-file comment attached to
// the key.
let root = doc.as_table_mut();
if !root.contains_key("_version") {
root.insert("_version", toml_edit::value(1_i64));
}
let server = ensure_table(root, "server")?;
let web = ensure_table(server, "web")?;
web.insert("enabled".to_string(), toml::Value::Boolean(true));
web.insert("url".to_string(), toml::Value::String(web_url));
let web = ensure_nested_table(doc, &["server", "web"])?;
set_preserving_decor(web, "enabled", toml_edit::value(true));
set_preserving_decor(web, "url", toml_edit::value(web_url));
let auth = ensure_table(server, "auth")?;
let auth_web = ensure_table(auth, "web")?;
let _ = auth_web;
let auth_api = ensure_table(auth, "api")?;
let _jwt = ensure_table(auth_api, "jwt")?;
// Ensure the auth subtrees exist so a freshly-registered GitHub App
// resolves `[server.auth.web]` / `[server.auth.api.jwt]` strategies on
// the next startup without a second manual edit.
ensure_nested_table(doc, &["server", "auth", "web"])?;
ensure_nested_table(doc, &["server", "auth", "api", "jwt"])?;
let integrations = ensure_table(server, "integrations")?;
let github = ensure_table(integrations, "github")?;
github.insert(
"app_id".to_string(),
toml::Value::String(data.id.to_string()),
let github = ensure_nested_table(doc, &["server", "integrations", "github"])?;
set_preserving_decor(github, "app_id", toml_edit::value(data.id.to_string()));
set_preserving_decor(
github,
"client_id",
toml_edit::value(data.client_id.clone()),
);
github.insert(
"client_id".to_string(),
toml::Value::String(data.client_id.clone()),
);
github.insert("slug".to_string(), toml::Value::String(data.slug.clone()));
set_preserving_decor(github, "slug", toml_edit::value(data.slug.clone()));
Ok(())
}
@ -700,38 +723,122 @@ mod tests {
}
}
fn parse_doc(source: &str) -> toml_edit::DocumentMut {
source
.parse::<toml_edit::DocumentMut>()
.expect("fixture should parse as TOML")
}
#[test]
fn merge_settings_keys_writes_v2_server_integrations_github() {
let mut doc: toml::Value =
toml::from_str("_version = 1\n").expect("empty v2 doc should parse");
let mut doc = parse_doc("_version = 1\n");
merge_settings_keys(&mut doc, &sample_conversion(), Some("https://example.test")).unwrap();
let github = doc
.get("server")
.and_then(toml::Value::as_table)
.and_then(|s| s.get("integrations"))
.and_then(toml::Value::as_table)
.and_then(|i| i.get("github"))
.and_then(toml::Value::as_table)
let github = doc["server"]["integrations"]["github"]
.as_table()
.expect("server.integrations.github should exist");
assert_eq!(
github.get("app_id").and_then(toml::Value::as_str),
Some("123")
);
assert_eq!(
github.get("slug").and_then(toml::Value::as_str),
Some("fabro")
);
assert_eq!(github["app_id"].as_str(), Some("123"));
assert_eq!(github["slug"].as_str(), Some("fabro"));
assert_eq!(github["client_id"].as_str(), Some("abc"));
let web = doc
.get("server")
.and_then(toml::Value::as_table)
.and_then(|s| s.get("web"))
.and_then(toml::Value::as_table)
let web = doc["server"]["web"]
.as_table()
.expect("server.web should exist");
assert_eq!(web["url"].as_str(), Some("https://example.test"));
assert_eq!(web["enabled"].as_bool(), Some(true));
// Re-parse the emitted document to prove it round-trips into a
// valid v2 `SettingsFile`.
let emitted = doc.to_string();
let file = fabro_config::ConfigLayer::parse(&emitted)
.expect("merged output should parse as a v2 SettingsFile");
let server = file
.as_v2()
.server
.as_ref()
.expect("[server] should be present");
let integrations = server
.integrations
.as_ref()
.expect("[server.integrations] should be present");
let github = integrations
.github
.as_ref()
.expect("[server.integrations.github] should be present");
assert_eq!(
web.get("url").and_then(toml::Value::as_str),
Some("https://example.test")
github.app_id.as_ref().map(|s| s.as_source()),
Some("123".to_string())
);
}
#[test]
fn merge_settings_keys_preserves_comments_and_unrelated_keys() {
let existing = r##"# Top-of-file comment explaining the settings layout.
_version = 1
# Storage root comment — should survive the edit.
[server.storage]
root = "/srv/fabro-data"
# A pre-existing integration that is NOT github.
[server.integrations.slack]
default_channel = "#ops"
[run.model]
provider = "anthropic"
name = "claude-sonnet"
"##;
let mut doc = parse_doc(existing);
merge_settings_keys(
&mut doc,
&sample_conversion(),
Some("https://fabro.example"),
)
.unwrap();
let emitted = doc.to_string();
// Comments must survive the round-trip.
assert!(
emitted.contains("# Top-of-file comment explaining the settings layout."),
"top-of-file comment was stripped:\n{emitted}"
);
assert!(
emitted.contains("# Storage root comment — should survive the edit."),
"inline table comment was stripped:\n{emitted}"
);
assert!(
emitted.contains("# A pre-existing integration that is NOT github."),
"sibling-table comment was stripped:\n{emitted}"
);
// Unrelated keys must still be intact.
assert!(
emitted.contains(r#"root = "/srv/fabro-data""#),
"server.storage.root was lost:\n{emitted}"
);
assert!(
emitted.contains(r##"default_channel = "#ops""##),
"server.integrations.slack.default_channel was lost:\n{emitted}"
);
assert!(
emitted.contains(r#"provider = "anthropic""#),
"run.model.provider was lost:\n{emitted}"
);
// And the new keys must be present.
assert!(
emitted.contains(r#"app_id = "123""#),
"server.integrations.github.app_id missing:\n{emitted}"
);
assert!(
emitted.contains(r#"url = "https://fabro.example""#),
"server.web.url missing:\n{emitted}"
);
// Finally, the whole thing must still parse as a valid v2
// SettingsFile.
fabro_config::ConfigLayer::parse(&emitted)
.expect("merged output should still parse as v2 after the edit");
}
}