chore(simplify): cleanup from review of recent commits

- Use FABRO_LOCAL_NO_AUTH_ENV const in start.rs and tests instead of
  the literal it was hoisted from.
- Preserve error chain in resolve_goal_override via anyhow::Error::from
  rather than stringifying through anyhow!.
- Drop {source} from ResolveGoalError::Io Display to avoid duplicate
  text under anyhow's chain formatter.
- Fail loud in setup_register when ConfigLayer reload or parent dir
  creation errors instead of silently leaving stale state.
- Promote resolve_goal_file_path to pub and call it from fabro-config
  to dedupe the absolute-or-base.join logic.
- Trim narrator-voice paragraphs from tls_config and web_auth comments.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-09 21:03:46 -04:00
parent 003b691de5
commit 2d4c0945bb
No known key found for this signature in database
7 changed files with 38 additions and 26 deletions

View file

@ -7,6 +7,7 @@ use chrono::Utc;
use fabro_config::Storage;
use fabro_config::user::default_socket_path;
use fabro_server::bind::{Bind, BindRequest};
use fabro_server::jwt_auth::FABRO_LOCAL_NO_AUTH_ENV;
use fabro_server::serve;
use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs};
use fabro_util::terminal::Styles;
@ -230,7 +231,7 @@ fn execute_daemon(
cmd.arg("--storage-dir").arg(storage_dir);
if matches!(bind, BindRequest::Unix(_)) {
cmd.env("FABRO_LOCAL_NO_AUTH", "1");
cmd.env(FABRO_LOCAL_NO_AUTH_ENV, "1");
}
cmd.env_remove("FABRO_JSON");

View file

@ -1,3 +1,4 @@
use fabro_server::jwt_auth::FABRO_LOCAL_NO_AUTH_ENV;
use fabro_test::{fabro_snapshot, test_context};
use std::process::Stdio;
use std::sync::{Arc, Barrier};
@ -148,7 +149,7 @@ fn start_with_tcp_host_only_bind_resolves_to_host_and_port() {
// startup explicitly.
let mut cmd = context.command();
cmd.env("FABRO_STORAGE_DIR", &storage_dir);
cmd.env("FABRO_LOCAL_NO_AUTH", "1");
cmd.env(FABRO_LOCAL_NO_AUTH_ENV, "1");
cmd.args(["server", "start", "--dry-run", "--bind", "127.0.0.1"]);
let output = cmd.output().expect("server start command should run");
assert!(
@ -211,7 +212,7 @@ fn start_with_tcp_host_only_bind_warns_and_falls_back_when_default_port_is_unava
// startup explicitly.
let mut cmd = context.command();
cmd.env("FABRO_STORAGE_DIR", &storage_dir);
cmd.env("FABRO_LOCAL_NO_AUTH", "1");
cmd.env(FABRO_LOCAL_NO_AUTH_ENV, "1");
cmd.args(["server", "start", "--dry-run", "--bind", "127.0.0.1"]);
fabro_snapshot!(filters, cmd, @"
success: true

View file

@ -13,6 +13,7 @@
use std::path::Path;
use anyhow::Context;
use fabro_types::settings::accessors::resolve_goal_file_path;
use fabro_types::settings::interp::InterpString;
use fabro_types::settings::run::RunGoalLayer;
use fabro_types::settings::{SettingsFile, parse_settings_file as parse_v2_settings_file};
@ -42,11 +43,10 @@ fn resolve_goal_file_paths(file: &mut SettingsFile, base_dir: &Path) {
return;
}
let literal = goal_file.as_source();
let path = Path::new(&literal);
if path.is_absolute() {
if Path::new(&literal).is_absolute() {
return;
}
let absolute = base_dir.join(path);
let absolute = resolve_goal_file_path(&literal, base_dir);
*goal_file = InterpString::parse(&absolute.to_string_lossy());
}

View file

@ -1,11 +1,8 @@
//! Resolved TLS material extracted from `[server.listen.tls]`.
//!
//! This module owns the `(cert, key, ca)` triple that the rustls config
//! builder in [`crate::tls`] consumes when the server is listening on TCP
//! with mTLS enabled. It lives outside `jwt_auth.rs` because TLS material
//! is a listen-side concern, not an authentication strategy — the auth
//! resolver only cares about *whether* TLS is present (for mTLS support),
//! not about its contents.
//! Owns the `(cert, key, ca)` triple that the rustls config builder in
//! [`crate::tls`] consumes when the server is listening on TCP with mTLS
//! enabled.
use std::path::PathBuf;

View file

@ -563,7 +563,13 @@ async fn setup_register(
// 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);
if let Err(err) = std::fs::create_dir_all(parent) {
error!(error = %err, path = %parent.display(), "Setup register failed: could not create settings parent directory");
return json_response(
StatusCode::INTERNAL_SERVER_ERROR,
json!({"error": format!("Failed to create settings directory: {err}")}),
);
}
}
let existing = std::fs::read_to_string(&settings_path).unwrap_or_default();
let mut doc: toml_edit::DocumentMut = if existing.is_empty() {
@ -622,12 +628,20 @@ async fn setup_register(
}
// Re-parse the freshly-written settings file and swap it into the
// in-memory state. Stage 6.6 may split this differently when the web
// setup flow is reworked, but for now a round-trip through
// `ConfigLayer::load` keeps the live state consistent with disk.
if let Ok(reloaded) = fabro_config::ConfigLayer::load(&settings_path) {
let mut shared = state.settings.write().expect("settings lock poisoned");
*shared = reloaded.into();
// in-memory state so subsequent OAuth requests see the new GitHub
// App credentials without a server restart.
match fabro_config::ConfigLayer::load(&settings_path) {
Ok(reloaded) => {
let mut shared = state.settings.write().expect("settings lock poisoned");
*shared = reloaded.into();
}
Err(err) => {
error!(error = %err, path = %settings_path.display(), "Setup register failed: could not reload written settings config");
return json_response(
StatusCode::INTERNAL_SERVER_ERROR,
json!({"error": format!("Failed to reload settings config after write: {err}")}),
);
}
}
info!(slug = %data.slug, app_id = %data.id, "GitHub App registered successfully");

View file

@ -465,7 +465,8 @@ impl SettingsFile {
/// Resolve a goal-file path string against `base_dir`. Absolute paths are
/// used as-is; relative paths are joined onto `base_dir`.
fn resolve_goal_file_path(path_str: &str, base_dir: &Path) -> PathBuf {
#[must_use]
pub fn resolve_goal_file_path(path_str: &str, base_dir: &Path) -> PathBuf {
let path = Path::new(path_str);
if path.is_absolute() {
path.to_path_buf()
@ -494,11 +495,9 @@ impl std::fmt::Display for ResolveGoalError {
f,
"failed to resolve run.goal.file: env var {var:?} referenced by ${{env.{var}}} is not set"
),
Self::Io { path, source } => write!(
f,
"failed to read run.goal.file at {}: {source}",
path.display()
),
Self::Io { path, .. } => {
write!(f, "failed to read run.goal.file at {}", path.display())
}
}
}
}

View file

@ -140,7 +140,7 @@ fn resolve_goal_override(
settings
.resolve_run_goal(working_directory)
.map(|opt| opt.map(|resolved| resolved.text))
.map_err(|err| anyhow::anyhow!(err))
.map_err(anyhow::Error::from)
}
#[cfg(test)]