From a4ee62a8a4845d5e400b289b1b688bbced8bb43f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Apr 2026 18:10:31 -0400 Subject: [PATCH] refactor(dev): dedupe shared helpers, remove check-boundary Consolidate workspace_root, PlannedCommand, shell_arg, markdown_cell, and replace_generated_region into commands/mod.rs; unify fabro_dev/output_text/ write_file/read_file into tests/it/main.rs. Drop the abandoned check-boundary subcommand. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../fabro-dev/src/commands/check_boundary.rs | 194 ------------------ .../src/commands/check_spa_budgets.rs | 10 +- .../fabro-dev/src/commands/docker_build.rs | 58 +----- .../src/commands/generate_cli_reference.rs | 62 +----- .../commands/generate_options_reference.rs | 62 +----- lib/crates/fabro-dev/src/commands/mod.rs | 123 ++++++++++- .../fabro-dev/src/commands/refresh_spa.rs | 10 +- lib/crates/fabro-dev/src/commands/release.rs | 92 +-------- lib/crates/fabro-dev/src/main.rs | 3 - .../fabro-dev/tests/it/check_boundary.rs | 169 --------------- lib/crates/fabro-dev/tests/it/docker_build.rs | 8 +- .../tests/it/generate_cli_reference.rs | 28 +-- .../tests/it/generate_options_reference.rs | 28 +-- lib/crates/fabro-dev/tests/it/main.rs | 43 ++-- lib/crates/fabro-dev/tests/it/release.rs | 21 +- lib/crates/fabro-dev/tests/it/spa.rs | 21 +- 16 files changed, 181 insertions(+), 751 deletions(-) delete mode 100644 lib/crates/fabro-dev/src/commands/check_boundary.rs delete mode 100644 lib/crates/fabro-dev/tests/it/check_boundary.rs diff --git a/lib/crates/fabro-dev/src/commands/check_boundary.rs b/lib/crates/fabro-dev/src/commands/check_boundary.rs deleted file mode 100644 index 4c4950e35..000000000 --- a/lib/crates/fabro-dev/src/commands/check_boundary.rs +++ /dev/null @@ -1,194 +0,0 @@ -use std::fs; -use std::path::{Path, PathBuf}; - -use anyhow::{Context, Result, bail}; -use clap::Args; -use regex::Regex; -use walkdir::WalkDir; - -const FABRO_CLI_SRC: &str = "lib/crates/fabro-cli/src"; -const EXEMPTION_MARKER: &str = "boundary-exempt(pr-api): remove with follow-up #1"; - -const SERVER_SYMBOL_ALLOWLIST: &[&str] = &[ - "lib/crates/fabro-cli/src/local_server.rs", - "lib/crates/fabro-cli/src/commands/install.rs", - "lib/crates/fabro-cli/src/commands/run/runner.rs", -]; - -const STORAGE_ALLOWLIST: &[&str] = &[ - "lib/crates/fabro-cli/src/command_context.rs", - "lib/crates/fabro-cli/src/commands/install.rs", - "lib/crates/fabro-cli/src/commands/uninstall.rs", - "lib/crates/fabro-cli/src/commands/run/runner.rs", -]; - -const DEPRECATED_HELPER_ALLOWLIST: &[&str] = &["lib/crates/fabro-cli/src/user_config.rs"]; - -const TEMPORARY_EXEMPTIONS: &[&str] = &[ - "lib/crates/fabro-cli/src/commands/pr/mod.rs", - "lib/crates/fabro-cli/src/commands/pr/create.rs", -]; - -#[derive(Debug, Args)] -pub(crate) struct CheckBoundaryArgs { - /// Workspace root to check. - #[arg(long, value_name = "ROOT", default_value = ".")] - root: PathBuf, -} - -struct BoundaryRule { - name: &'static str, - pattern: Regex, - allowlist: &'static [&'static str], -} - -struct SourceFile { - relative: String, - contents: String, -} - -#[expect( - clippy::print_stdout, - clippy::print_stderr, - reason = "dev check command reports pass/fail diagnostics directly" -)] -pub(crate) fn check_boundary(args: CheckBoundaryArgs) -> Result<()> { - let failures = BoundaryChecker::new(args.root).check()?; - - if failures.is_empty() { - println!("CLI/server boundary checks passed."); - return Ok(()); - } - - for failure in failures { - eprintln!("{failure}"); - } - bail!("CLI/server boundary checks failed") -} - -struct BoundaryChecker { - root: PathBuf, -} - -impl BoundaryChecker { - fn new(root: PathBuf) -> Self { - Self { root } - } - - fn check(&self) -> Result> { - let rules = boundary_rules(); - let mut failures = Vec::new(); - - for file in self.source_files()? { - let has_marker = marker_lines(&file.contents).next().is_some(); - let has_valid_exemption = - has_marker && TEMPORARY_EXEMPTIONS.contains(&file.relative.as_str()); - - for line_number in marker_lines(&file.contents) { - if !TEMPORARY_EXEMPTIONS.contains(&file.relative.as_str()) { - failures.push(format!( - "boundary check failed: unexpected temporary exemption marker in {}:{}", - file.relative, line_number - )); - } - } - - for rule in &rules { - if rule.allowlist.contains(&file.relative.as_str()) || has_valid_exemption { - continue; - } - - for (line_number, line) in file.contents.lines().enumerate() { - if rule.pattern.is_match(line) { - failures.push(format!( - "boundary check failed: {} used outside allowlist: {}:{}", - rule.name, - file.relative, - line_number + 1 - )); - } - } - } - } - - Ok(failures) - } - - #[expect( - clippy::disallowed_methods, - reason = "dev boundary checker synchronously scans source files outside Tokio paths" - )] - fn source_files(&self) -> Result> { - let source_root = self.root.join(FABRO_CLI_SRC); - if !source_root.exists() { - return Ok(Vec::new()); - } - - let mut files = Vec::new(); - for entry in WalkDir::new(&source_root) { - let entry = entry.context("walking fabro-cli source tree")?; - let path = entry.path(); - if !entry.file_type().is_file() - || path.extension().and_then(|ext| ext.to_str()) != Some("rs") - { - continue; - } - - let contents = fs::read_to_string(path) - .with_context(|| format!("reading source file {}", path.display()))?; - files.push(SourceFile { - relative: relative_path(&self.root, path)?, - contents, - }); - } - - files.sort_by(|left, right| left.relative.cmp(&right.relative)); - Ok(files) - } -} - -fn boundary_rules() -> [BoundaryRule; 3] { - [ - BoundaryRule { - name: "gated server symbol", - pattern: Regex::new( - r"fabro_config::resolve_server_from_file|fabro_config::resolve_server\b|fabro_config::ServerSettings::from_layer\b|fabro_config::ServerSettings::resolve\b|ServerSettings::from_layer\b|ServerSettings::resolve\b", - ) - .expect("server symbol regex should compile"), - allowlist: SERVER_SYMBOL_ALLOWLIST, - }, - BoundaryRule { - name: "Storage::new", - pattern: Regex::new(r"Storage::new").expect("storage regex should compile"), - allowlist: STORAGE_ALLOWLIST, - }, - BoundaryRule { - name: "deprecated user_config::storage_dir", - pattern: Regex::new(r"user_config::storage_dir") - .expect("deprecated helper regex should compile"), - allowlist: DEPRECATED_HELPER_ALLOWLIST, - }, - ] -} - -fn marker_lines(contents: &str) -> impl Iterator + '_ { - contents.lines().enumerate().filter_map(|(index, line)| { - if line.contains(EXEMPTION_MARKER) { - Some(index + 1) - } else { - None - } - }) -} - -fn relative_path(root: &Path, path: &Path) -> Result { - let relative = path - .strip_prefix(root) - .with_context(|| format!("{} is not under {}", path.display(), root.display()))?; - - Ok(relative - .components() - .map(|component| component.as_os_str().to_string_lossy()) - .collect::>() - .join("/")) -} diff --git a/lib/crates/fabro-dev/src/commands/check_spa_budgets.rs b/lib/crates/fabro-dev/src/commands/check_spa_budgets.rs index 53f3b4183..a7dab7a49 100644 --- a/lib/crates/fabro-dev/src/commands/check_spa_budgets.rs +++ b/lib/crates/fabro-dev/src/commands/check_spa_budgets.rs @@ -5,6 +5,8 @@ use anyhow::{Context, Result, bail}; use clap::Args; use walkdir::WalkDir; +use super::workspace_root; + const DEFAULT_ASSET_BUDGET_BYTES: u64 = 15 * 1024 * 1024; const DEFAULT_PAYLOAD_BUDGET_BYTES: u64 = 5 * 1024 * 1024; @@ -127,11 +129,3 @@ fn ensure_gzip_success(file: &Path, output: &Output) -> Result { Ok(output.stdout.len() as u64) } - -fn workspace_root() -> PathBuf { - let mut root = Path::new(env!("CARGO_MANIFEST_DIR")).to_path_buf(); - root.pop(); - root.pop(); - root.pop(); - root -} diff --git a/lib/crates/fabro-dev/src/commands/docker_build.rs b/lib/crates/fabro-dev/src/commands/docker_build.rs index 86d3614af..6e012b0fd 100644 --- a/lib/crates/fabro-dev/src/commands/docker_build.rs +++ b/lib/crates/fabro-dev/src/commands/docker_build.rs @@ -1,10 +1,11 @@ use std::fmt; -use std::path::{Path, PathBuf}; -use std::process::Command; +use std::path::PathBuf; use anyhow::{Context, Result, bail}; use clap::{Args, ValueEnum}; +use super::{PlannedCommand, command, shell_arg, workspace_root}; + const ZIG_VERSION: &str = "0.13.0"; #[derive(Debug, Args)] @@ -199,13 +200,8 @@ impl DockerBuildPlan { .arg(".") } - #[expect( - clippy::disallowed_methods, - reason = "dev docker-build intentionally runs synchronous Docker subprocesses" - )] fn run_command(&self, planned: &PlannedCommand) -> Result<()> { - let status = Command::new(&planned.program) - .args(&planned.args) + let status = command(planned) .current_dir(&self.workspace_root) .status() .with_context(|| format!("running {}", planned.to_shell_line()))?; @@ -228,32 +224,6 @@ impl DockerBuildPlan { } } -struct PlannedCommand { - program: String, - args: Vec, -} - -impl PlannedCommand { - fn new(program: impl Into) -> Self { - Self { - program: program.into(), - args: Vec::new(), - } - } - - fn arg(mut self, arg: impl Into) -> Self { - self.args.push(arg.into()); - self - } - - fn to_shell_line(&self) -> String { - std::iter::once(shell_arg(&self.program)) - .chain(self.args.iter().map(shell_arg)) - .collect::>() - .join(" ") - } -} - fn build_script(target: &str, zig_arch: &str) -> String { format!( "set -e; \ @@ -269,23 +239,3 @@ fn build_script(target: &str, zig_arch: &str) -> String { cargo zigbuild --release -p fabro-cli --target {target}" ) } - -fn shell_arg(arg: impl AsRef) -> String { - let arg = arg.as_ref(); - if arg - .chars() - .all(|ch| ch.is_ascii_alphanumeric() || "_-./:=@".contains(ch)) - { - return arg.to_string(); - } - - format!("'{}'", arg.replace('\'', "'\\''")) -} - -fn workspace_root() -> PathBuf { - let mut root = Path::new(env!("CARGO_MANIFEST_DIR")).to_path_buf(); - root.pop(); - root.pop(); - root.pop(); - root -} diff --git a/lib/crates/fabro-dev/src/commands/generate_cli_reference.rs b/lib/crates/fabro-dev/src/commands/generate_cli_reference.rs index a31b0ae37..a097f21af 100644 --- a/lib/crates/fabro-dev/src/commands/generate_cli_reference.rs +++ b/lib/crates/fabro-dev/src/commands/generate_cli_reference.rs @@ -1,9 +1,11 @@ use std::ffi::OsStr; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use anyhow::{Context, Result, bail}; use clap::{Arg, ArgAction, Command}; +use super::{markdown_cell, replace_generated_region, workspace_root}; + const CLI_REFERENCE_PATH: &str = "docs/reference/cli.mdx"; const FENCE_START: &str = ""; const FENCE_END: &str = ""; @@ -29,7 +31,13 @@ pub(crate) fn generate_cli_reference(args: GenerateCliReferenceArgs) -> Result<( let current = std::fs::read_to_string(&path).with_context(|| format!("reading {}", path.display()))?; let generated = render_cli_reference(fabro_cli::command_for_reference()); - let updated = replace_generated_region(¤t, &generated)?; + let updated = replace_generated_region( + ¤t, + &generated, + CLI_REFERENCE_PATH, + FENCE_START, + FENCE_END, + )?; if args.check { if current != updated { @@ -254,53 +262,3 @@ fn os_str_to_markdown_code(value: &OsStr) -> Option { let value = value.to_str()?; (!value.is_empty()).then(|| format!("`{}`", markdown_cell(value))) } - -fn markdown_cell(value: &str) -> String { - value - .replace('|', "\\|") - .replace('\n', "
") - .trim() - .to_string() -} - -fn replace_generated_region(current: &str, generated: &str) -> Result { - let start = current - .find(FENCE_START) - .with_context(|| format!("{CLI_REFERENCE_PATH} is missing {FENCE_START}"))?; - let content_start = start + FENCE_START.len(); - let relative_end = current[content_start..] - .find(FENCE_END) - .with_context(|| format!("{CLI_REFERENCE_PATH} is missing {FENCE_END}"))?; - let end = content_start + relative_end; - - let before = ¤t[..content_start]; - let after = ¤t[end..]; - Ok(format!("{before}\n{generated}\n{after}")) -} - -fn workspace_root() -> PathBuf { - let mut root = Path::new(env!("CARGO_MANIFEST_DIR")).to_path_buf(); - root.pop(); - root.pop(); - root.pop(); - root -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn replace_generated_region_preserves_manual_content() { - let updated = replace_generated_region( - "before\n\nstale\n\nafter\n", - "fresh", - ) - .expect("generated region should be replaced"); - - assert_eq!( - updated, - "before\n\nfresh\n\nafter\n" - ); - } -} diff --git a/lib/crates/fabro-dev/src/commands/generate_options_reference.rs b/lib/crates/fabro-dev/src/commands/generate_options_reference.rs index eb288a1d4..87b308daa 100644 --- a/lib/crates/fabro-dev/src/commands/generate_options_reference.rs +++ b/lib/crates/fabro-dev/src/commands/generate_options_reference.rs @@ -1,9 +1,11 @@ use std::collections::BTreeMap; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use anyhow::{Context, Result, bail}; use fabro_options_metadata::{OptionField, OptionSet, Visit}; +use super::{markdown_cell, replace_generated_region, workspace_root}; + const OPTIONS_REFERENCE_PATH: &str = "docs/reference/user-configuration.mdx"; const FENCE_START: &str = ""; const FENCE_END: &str = ""; @@ -30,7 +32,13 @@ pub(crate) fn generate_options_reference(args: GenerateOptionsReferenceArgs) -> let current = std::fs::read_to_string(&path).with_context(|| format!("reading {}", path.display()))?; let generated = render_options_reference(); - let updated = replace_generated_region(¤t, &generated)?; + let updated = replace_generated_region( + ¤t, + &generated, + OPTIONS_REFERENCE_PATH, + FENCE_START, + FENCE_END, + )?; if args.check { if current != updated { @@ -276,53 +284,3 @@ See [MCP](/agents/mcp) for transport-specific examples. fn normalize_doc(doc: &str) -> String { doc.trim().trim_end_matches('.').to_string() } - -fn markdown_cell(value: &str) -> String { - value - .replace('|', "\\|") - .replace('\n', "
") - .trim() - .to_string() -} - -fn replace_generated_region(current: &str, generated: &str) -> Result { - let start = current - .find(FENCE_START) - .with_context(|| format!("{OPTIONS_REFERENCE_PATH} is missing {FENCE_START}"))?; - let content_start = start + FENCE_START.len(); - let relative_end = current[content_start..] - .find(FENCE_END) - .with_context(|| format!("{OPTIONS_REFERENCE_PATH} is missing {FENCE_END}"))?; - let end = content_start + relative_end; - - let before = ¤t[..content_start]; - let after = ¤t[end..]; - Ok(format!("{before}\n{generated}\n{after}")) -} - -fn workspace_root() -> PathBuf { - let mut root = Path::new(env!("CARGO_MANIFEST_DIR")).to_path_buf(); - root.pop(); - root.pop(); - root.pop(); - root -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn replace_generated_region_preserves_manual_content() { - let updated = replace_generated_region( - "before\n\nstale\n\nafter\n", - "fresh", - ) - .expect("generated region should be replaced"); - - assert_eq!( - updated, - "before\n\nfresh\n\nafter\n" - ); - } -} diff --git a/lib/crates/fabro-dev/src/commands/mod.rs b/lib/crates/fabro-dev/src/commands/mod.rs index 50d0210d8..36ac119f1 100644 --- a/lib/crates/fabro-dev/src/commands/mod.rs +++ b/lib/crates/fabro-dev/src/commands/mod.rs @@ -1,4 +1,3 @@ -mod check_boundary; mod check_spa_budgets; mod docker_build; mod generate_cli_reference; @@ -6,7 +5,10 @@ mod generate_options_reference; mod refresh_spa; mod release; -pub(crate) use check_boundary::{CheckBoundaryArgs, check_boundary}; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use anyhow::{Context, Result}; pub(crate) use check_spa_budgets::{CheckSpaBudgetsArgs, check_spa_budgets}; pub(crate) use docker_build::{DockerBuildArgs, docker_build}; pub(crate) use generate_cli_reference::{GenerateCliReferenceArgs, generate_cli_reference}; @@ -15,3 +17,120 @@ pub(crate) use generate_options_reference::{ }; pub(crate) use refresh_spa::{RefreshSpaArgs, refresh_spa}; pub(crate) use release::{ReleaseArgs, release}; + +pub(crate) fn workspace_root() -> PathBuf { + let mut root = Path::new(env!("CARGO_MANIFEST_DIR")).to_path_buf(); + root.pop(); + root.pop(); + root.pop(); + root +} + +pub(crate) fn markdown_cell(value: &str) -> String { + value + .replace('|', "\\|") + .replace('\n', "
") + .trim() + .to_string() +} + +pub(crate) fn replace_generated_region( + current: &str, + generated: &str, + doc_path: &str, + fence_start: &str, + fence_end: &str, +) -> Result { + let start = current + .find(fence_start) + .with_context(|| format!("{doc_path} is missing {fence_start}"))?; + let content_start = start + fence_start.len(); + let relative_end = current[content_start..] + .find(fence_end) + .with_context(|| format!("{doc_path} is missing {fence_end}"))?; + let end = content_start + relative_end; + + let before = ¤t[..content_start]; + let after = ¤t[end..]; + Ok(format!("{before}\n{generated}\n{after}")) +} + +pub(crate) struct PlannedCommand { + pub(crate) program: String, + pub(crate) args: Vec, + pub(crate) unset_env: Vec, + pub(crate) env: Vec<(String, String)>, +} + +impl PlannedCommand { + pub(crate) fn new(program: impl Into) -> Self { + Self { + program: program.into(), + args: Vec::new(), + unset_env: Vec::new(), + env: Vec::new(), + } + } + + pub(crate) fn arg(mut self, arg: impl Into) -> Self { + self.args.push(arg.into()); + self + } + + pub(crate) fn env(mut self, key: impl Into, value: impl Into) -> Self { + self.env.push((key.into(), value.into())); + self + } + + pub(crate) fn env_remove(mut self, key: impl Into) -> Self { + self.unset_env.push(key.into()); + self + } + + pub(crate) fn to_shell_line(&self) -> String { + let mut parts = Vec::new(); + + if !self.unset_env.is_empty() { + parts.push("unset".to_string()); + parts.extend(self.unset_env.iter().map(shell_arg)); + parts.push("&&".to_string()); + } + + parts.extend( + self.env + .iter() + .map(|(key, value)| format!("{}={}", shell_arg(key), shell_arg(value))), + ); + parts.push(shell_arg(&self.program)); + parts.extend(self.args.iter().map(shell_arg)); + parts.join(" ") + } +} + +#[expect( + clippy::disallowed_methods, + reason = "fabro-dev intentionally builds synchronous subprocess commands" +)] +pub(crate) fn command(planned: &PlannedCommand) -> Command { + let mut command = Command::new(&planned.program); + command.args(&planned.args); + for key in &planned.unset_env { + command.env_remove(key); + } + for (key, value) in &planned.env { + command.env(key, value); + } + command +} + +pub(crate) fn shell_arg(arg: impl AsRef) -> String { + let arg = arg.as_ref(); + if arg + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || "_-./:=@".contains(ch)) + { + return arg.to_string(); + } + + format!("'{}'", arg.replace('\'', "'\\''")) +} diff --git a/lib/crates/fabro-dev/src/commands/refresh_spa.rs b/lib/crates/fabro-dev/src/commands/refresh_spa.rs index 5b8eec041..b6f739027 100644 --- a/lib/crates/fabro-dev/src/commands/refresh_spa.rs +++ b/lib/crates/fabro-dev/src/commands/refresh_spa.rs @@ -5,6 +5,8 @@ use anyhow::{Context, Result, bail}; use clap::Args; use walkdir::WalkDir; +use super::workspace_root; + #[derive(Debug, Args)] pub(crate) struct RefreshSpaArgs { /// Repository root containing apps/fabro-web and lib/crates/fabro-spa. @@ -101,11 +103,3 @@ fn mirror_dist(dist_dir: &Path, asset_dir: &Path) -> Result<()> { Ok(()) } - -fn workspace_root() -> PathBuf { - let mut root = Path::new(env!("CARGO_MANIFEST_DIR")).to_path_buf(); - root.pop(); - root.pop(); - root.pop(); - root -} diff --git a/lib/crates/fabro-dev/src/commands/release.rs b/lib/crates/fabro-dev/src/commands/release.rs index 785611fc9..f6bcb8283 100644 --- a/lib/crates/fabro-dev/src/commands/release.rs +++ b/lib/crates/fabro-dev/src/commands/release.rs @@ -1,11 +1,13 @@ use std::fmt; use std::path::{Path, PathBuf}; -use std::process::{Command, Output}; +use std::process::Output; use anyhow::{Context, Result, bail}; use chrono::{Local, NaiveDate}; use clap::{Args, ValueEnum}; +use super::{PlannedCommand, command, workspace_root}; + const RELEASE_EPOCH: &str = "2026-01-01"; const RELEASE_TEST_SEGMENT_WRITE_KEY: &str = "fake-for-local-smoke"; @@ -353,91 +355,3 @@ fn update_version(cargo_toml: &Path, current_version: &str, new_version: &str) - std::fs::write(cargo_toml, contents.replacen(&needle, &replacement, 1)) .with_context(|| format!("writing {}", cargo_toml.display())) } - -#[expect( - clippy::disallowed_methods, - reason = "dev release builds synchronous subprocess commands" -)] -fn command(planned: &PlannedCommand) -> Command { - let mut command = Command::new(&planned.program); - command.args(&planned.args); - for key in &planned.unset_env { - command.env_remove(key); - } - for (key, value) in &planned.env { - command.env(key, value); - } - command -} - -struct PlannedCommand { - program: String, - args: Vec, - unset_env: Vec, - env: Vec<(String, String)>, -} - -impl PlannedCommand { - fn new(program: impl Into) -> Self { - Self { - program: program.into(), - args: Vec::new(), - unset_env: Vec::new(), - env: Vec::new(), - } - } - - fn arg(mut self, arg: impl Into) -> Self { - self.args.push(arg.into()); - self - } - - fn env(mut self, key: impl Into, value: impl Into) -> Self { - self.env.push((key.into(), value.into())); - self - } - - fn env_remove(mut self, key: impl Into) -> Self { - self.unset_env.push(key.into()); - self - } - - fn to_shell_line(&self) -> String { - let mut parts = Vec::new(); - - if !self.unset_env.is_empty() { - parts.push("unset".to_string()); - parts.extend(self.unset_env.iter().map(shell_arg)); - parts.push("&&".to_string()); - } - - parts.extend( - self.env - .iter() - .map(|(key, value)| format!("{}={}", shell_arg(key), shell_arg(value))), - ); - parts.push(shell_arg(&self.program)); - parts.extend(self.args.iter().map(shell_arg)); - parts.join(" ") - } -} - -fn shell_arg(arg: impl AsRef) -> String { - let arg = arg.as_ref(); - if arg - .chars() - .all(|ch| ch.is_ascii_alphanumeric() || "_-./:=@".contains(ch)) - { - return arg.to_string(); - } - - format!("'{}'", arg.replace('\'', "'\\''")) -} - -fn workspace_root() -> PathBuf { - let mut root = Path::new(env!("CARGO_MANIFEST_DIR")).to_path_buf(); - root.pop(); - root.pop(); - root.pop(); - root -} diff --git a/lib/crates/fabro-dev/src/main.rs b/lib/crates/fabro-dev/src/main.rs index 47a85773d..476b54ed6 100644 --- a/lib/crates/fabro-dev/src/main.rs +++ b/lib/crates/fabro-dev/src/main.rs @@ -18,8 +18,6 @@ struct Cli { #[derive(Debug, Subcommand)] enum Command { - /// Check source boundary rules. - CheckBoundary(commands::CheckBoundaryArgs), /// Build Fabro Docker images with the release pipeline layout. DockerBuild(commands::DockerBuildArgs), /// Generate docs/reference/cli.mdx from the Fabro clap command tree. @@ -37,7 +35,6 @@ enum Command { impl Command { fn run(self) -> Result<()> { match self { - Self::CheckBoundary(args) => commands::check_boundary(args), Self::DockerBuild(args) => commands::docker_build(args), Self::GenerateCliReference(args) => commands::generate_cli_reference(args), Self::GenerateOptionsReference(args) => commands::generate_options_reference(args), diff --git a/lib/crates/fabro-dev/tests/it/check_boundary.rs b/lib/crates/fabro-dev/tests/it/check_boundary.rs deleted file mode 100644 index 47fee0d07..000000000 --- a/lib/crates/fabro-dev/tests/it/check_boundary.rs +++ /dev/null @@ -1,169 +0,0 @@ -use std::fs; -use std::path::{Path, PathBuf}; - -fn fabro_dev() -> assert_cmd::Command { - assert_cmd::cargo::cargo_bin_cmd!("fabro-dev") -} - -fn workspace_root() -> PathBuf { - let mut root = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - root.pop(); - root.pop(); - root.pop(); - root -} - -#[expect( - clippy::disallowed_methods, - reason = "integration tests stage temporary Rust source fixtures with sync std::fs::write" -)] -fn write_file(root: &Path, path: &str, contents: &str) { - let path = root.join(path); - fs::create_dir_all(path.parent().expect("fixture path should have parent")) - .expect("creating fixture parent directory"); - fs::write(path, contents).expect("writing fixture file"); -} - -fn check_boundary(root: &Path) -> assert_cmd::Command { - let mut cmd = fabro_dev(); - cmd.args(["check-boundary", "--root"]).arg(root); - cmd -} - -fn output_text(bytes: &[u8]) -> String { - String::from_utf8(bytes.to_vec()).expect("command output should be valid utf-8") -} - -#[test] -fn current_tree_passes_boundary_check() { - let output = check_boundary(&workspace_root()) - .assert() - .success() - .get_output() - .clone(); - - assert!( - output_text(&output.stdout).contains("CLI/server boundary checks passed."), - "success output should report boundary check pass" - ); -} - -#[test] -fn empty_workspace_passes() { - let fixture = tempfile::tempdir().expect("creating fixture"); - - check_boundary(fixture.path()).assert().success(); -} - -#[test] -fn allowed_symbols_pass() { - let fixture = tempfile::tempdir().expect("creating fixture"); - write_file( - fixture.path(), - "lib/crates/fabro-cli/src/local_server.rs", - "fn resolve() { fabro_config::resolve_server(); }\n", - ); - write_file( - fixture.path(), - "lib/crates/fabro-cli/src/command_context.rs", - "fn storage() { Storage::new(path); }\n", - ); - - check_boundary(fixture.path()).assert().success(); -} - -#[test] -fn temporary_exemption_marker_allows_listed_file() { - let fixture = tempfile::tempdir().expect("creating fixture"); - write_file( - fixture.path(), - "lib/crates/fabro-cli/src/commands/pr/mod.rs", - "// boundary-exempt(pr-api): remove with follow-up #1\nfn storage() { Storage::new(path); }\n", - ); - - check_boundary(fixture.path()).assert().success(); -} - -#[test] -fn offending_file_reports_path_and_line() { - let fixture = tempfile::tempdir().expect("creating fixture"); - write_file( - fixture.path(), - "lib/crates/fabro-cli/src/commands/bad.rs", - "fn storage() { Storage::new(path); }\n", - ); - - let output = check_boundary(fixture.path()) - .assert() - .failure() - .code(1) - .get_output() - .clone(); - let stderr = output_text(&output.stderr); - - assert!( - stderr.contains( - "boundary check failed: Storage::new used outside allowlist: \ - lib/crates/fabro-cli/src/commands/bad.rs:1" - ), - "stderr should name offending file and line:\n{stderr}" - ); -} - -#[test] -fn multiple_offending_files_are_all_reported() { - let fixture = tempfile::tempdir().expect("creating fixture"); - write_file( - fixture.path(), - "lib/crates/fabro-cli/src/commands/bad_storage.rs", - "fn storage() { Storage::new(path); }\n", - ); - write_file( - fixture.path(), - "lib/crates/fabro-cli/src/commands/bad_server.rs", - "fn server() { ServerSettings::resolve(layer); }\n", - ); - - let output = check_boundary(fixture.path()) - .assert() - .failure() - .code(1) - .get_output() - .clone(); - let stderr = output_text(&output.stderr); - - assert!( - stderr.contains("bad_storage.rs:1"), - "stderr should report storage offender:\n{stderr}" - ); - assert!( - stderr.contains("bad_server.rs:1"), - "stderr should report server-settings offender:\n{stderr}" - ); -} - -#[test] -fn unexpected_temporary_exemption_marker_fails() { - let fixture = tempfile::tempdir().expect("creating fixture"); - write_file( - fixture.path(), - "lib/crates/fabro-cli/src/commands/bad.rs", - "// boundary-exempt(pr-api): remove with follow-up #1\n", - ); - - let output = check_boundary(fixture.path()) - .assert() - .failure() - .code(1) - .get_output() - .clone(); - let stderr = output_text(&output.stderr); - - assert!( - stderr.contains( - "boundary check failed: unexpected temporary exemption marker in \ - lib/crates/fabro-cli/src/commands/bad.rs:1" - ), - "stderr should report unexpected marker:\n{stderr}" - ); -} diff --git a/lib/crates/fabro-dev/tests/it/docker_build.rs b/lib/crates/fabro-dev/tests/it/docker_build.rs index fcf16901c..461de29c9 100644 --- a/lib/crates/fabro-dev/tests/it/docker_build.rs +++ b/lib/crates/fabro-dev/tests/it/docker_build.rs @@ -1,10 +1,4 @@ -fn fabro_dev() -> assert_cmd::Command { - assert_cmd::cargo::cargo_bin_cmd!("fabro-dev") -} - -fn output_text(bytes: &[u8]) -> String { - String::from_utf8(bytes.to_vec()).expect("command output should be valid utf-8") -} +use super::{fabro_dev, output_text}; #[test] fn help_lists_docker_build_flags() { diff --git a/lib/crates/fabro-dev/tests/it/generate_cli_reference.rs b/lib/crates/fabro-dev/tests/it/generate_cli_reference.rs index 334f868e0..f06597f6a 100644 --- a/lib/crates/fabro-dev/tests/it/generate_cli_reference.rs +++ b/lib/crates/fabro-dev/tests/it/generate_cli_reference.rs @@ -1,32 +1,6 @@ -use std::fs; use std::path::Path; -fn fabro_dev() -> assert_cmd::Command { - assert_cmd::cargo::cargo_bin_cmd!("fabro-dev") -} - -fn output_text(bytes: &[u8]) -> String { - String::from_utf8(bytes.to_vec()).expect("command output should be valid utf-8") -} - -#[expect( - clippy::disallowed_methods, - reason = "integration tests stage temporary CLI reference fixtures with sync std::fs::write" -)] -fn write_file(root: &Path, path: &str, contents: &str) { - let path = root.join(path); - fs::create_dir_all(path.parent().expect("fixture path should have parent")) - .expect("creating fixture parent directory"); - fs::write(path, contents).expect("writing fixture file"); -} - -#[expect( - clippy::disallowed_methods, - reason = "integration tests inspect generated CLI reference fixtures with sync std::fs::read_to_string" -)] -fn read_file(root: &Path, path: &str) -> String { - fs::read_to_string(root.join(path)).expect("reading fixture file") -} +use super::{fabro_dev, output_text, read_file, write_file}; fn cli_reference(root: &Path) -> assert_cmd::Command { let mut cmd = fabro_dev(); diff --git a/lib/crates/fabro-dev/tests/it/generate_options_reference.rs b/lib/crates/fabro-dev/tests/it/generate_options_reference.rs index 99ffe7bf3..7d9da4e80 100644 --- a/lib/crates/fabro-dev/tests/it/generate_options_reference.rs +++ b/lib/crates/fabro-dev/tests/it/generate_options_reference.rs @@ -1,32 +1,6 @@ -use std::fs; use std::path::Path; -fn fabro_dev() -> assert_cmd::Command { - assert_cmd::cargo::cargo_bin_cmd!("fabro-dev") -} - -fn output_text(bytes: &[u8]) -> String { - String::from_utf8(bytes.to_vec()).expect("command output should be valid utf-8") -} - -#[expect( - clippy::disallowed_methods, - reason = "integration tests stage temporary options reference fixtures with sync std::fs::write" -)] -fn write_file(root: &Path, path: &str, contents: &str) { - let path = root.join(path); - fs::create_dir_all(path.parent().expect("fixture path should have parent")) - .expect("creating fixture parent directory"); - fs::write(path, contents).expect("writing fixture file"); -} - -#[expect( - clippy::disallowed_methods, - reason = "integration tests inspect generated options reference fixtures with sync std::fs::read_to_string" -)] -fn read_file(root: &Path, path: &str) -> String { - fs::read_to_string(root.join(path)).expect("reading fixture file") -} +use super::{fabro_dev, output_text, read_file, write_file}; fn options_reference(root: &Path) -> assert_cmd::Command { let mut cmd = fabro_dev(); diff --git a/lib/crates/fabro-dev/tests/it/main.rs b/lib/crates/fabro-dev/tests/it/main.rs index d269bcda2..c0f3e0375 100644 --- a/lib/crates/fabro-dev/tests/it/main.rs +++ b/lib/crates/fabro-dev/tests/it/main.rs @@ -1,7 +1,6 @@ -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::{Command, Output}; -mod check_boundary; mod docker_build; mod generate_cli_reference; mod generate_options_reference; @@ -24,6 +23,25 @@ fn output_text(bytes: &[u8]) -> String { String::from_utf8(bytes.to_vec()).expect("command output should be valid utf-8") } +#[expect( + clippy::disallowed_methods, + reason = "integration tests stage temporary fixture files with sync std::fs::write" +)] +fn write_file(root: &Path, path: &str, contents: impl AsRef<[u8]>) { + let path = root.join(path); + std::fs::create_dir_all(path.parent().expect("fixture path should have parent")) + .expect("creating fixture parent directory"); + std::fs::write(path, contents).expect("writing fixture file"); +} + +#[expect( + clippy::disallowed_methods, + reason = "integration tests inspect fixture files with sync std::fs::read_to_string" +)] +fn read_file(root: &Path, path: &str) -> String { + std::fs::read_to_string(root.join(path)).expect("reading fixture file") +} + #[expect( clippy::disallowed_methods, reason = "integration test intentionally shells out to Cargo to verify the cargo dev alias" @@ -48,7 +66,6 @@ fn help_lists_scaffolded_commands() { let stdout = output_text(&output.stdout); for command in [ - "check-boundary", "docker-build", "generate-cli-reference", "generate-options-reference", @@ -76,29 +93,11 @@ fn cargo_dev_alias_resolves_to_fabro_dev_help() { let stdout = output_text(&output.stdout); assert!( - stdout.contains("check-boundary"), + stdout.contains("docker-build"), "cargo dev help should come from fabro-dev:\n{stdout}" ); } -#[test] -fn check_boundary_help_succeeds() { - let output = cargo_dev(&["check-boundary", "--help"]); - - assert!( - output.status.success(), - "cargo dev check-boundary --help failed\nstdout:\n{}\nstderr:\n{}", - output_text(&output.stdout), - output_text(&output.stderr) - ); - - let stdout = output_text(&output.stdout); - assert!( - stdout.contains("Check source boundary rules"), - "check-boundary help should describe the subcommand:\n{stdout}" - ); -} - #[test] fn unknown_subcommand_exits_with_clap_usage_error() { let output = fabro_dev() diff --git a/lib/crates/fabro-dev/tests/it/release.rs b/lib/crates/fabro-dev/tests/it/release.rs index c7abbb93c..193c3c3fe 100644 --- a/lib/crates/fabro-dev/tests/it/release.rs +++ b/lib/crates/fabro-dev/tests/it/release.rs @@ -1,21 +1,7 @@ use std::path::Path; use std::process::Command; -fn fabro_dev() -> assert_cmd::Command { - assert_cmd::cargo::cargo_bin_cmd!("fabro-dev") -} - -fn output_text(bytes: &[u8]) -> String { - String::from_utf8(bytes.to_vec()).expect("command output should be valid utf-8") -} - -#[expect( - clippy::disallowed_methods, - reason = "integration tests stage temporary release fixture repositories with sync std::fs::write" -)] -fn write_file(path: &Path, contents: &str) { - std::fs::write(path, contents).expect("writing fixture file"); -} +use super::{fabro_dev, output_text, write_file}; #[expect( clippy::disallowed_methods, @@ -39,7 +25,8 @@ fn git(root: &Path, args: &[&str]) { fn release_fixture() -> tempfile::TempDir { let fixture = tempfile::tempdir().expect("creating fixture"); write_file( - &fixture.path().join("Cargo.toml"), + fixture.path(), + "Cargo.toml", r#"[workspace] members = [] @@ -198,7 +185,7 @@ fn dry_run_reports_skip_tests_without_running_release_tests() { #[test] fn dirty_worktree_errors_unless_dry_run() { let fixture = release_fixture(); - write_file(&fixture.path().join("dirty.txt"), "dirty\n"); + write_file(fixture.path(), "dirty.txt", "dirty\n"); let output = fabro_dev() .args([ diff --git a/lib/crates/fabro-dev/tests/it/spa.rs b/lib/crates/fabro-dev/tests/it/spa.rs index bed328285..ced259598 100644 --- a/lib/crates/fabro-dev/tests/it/spa.rs +++ b/lib/crates/fabro-dev/tests/it/spa.rs @@ -1,23 +1,4 @@ -use std::path::Path; - -fn fabro_dev() -> assert_cmd::Command { - assert_cmd::cargo::cargo_bin_cmd!("fabro-dev") -} - -fn output_text(bytes: &[u8]) -> String { - String::from_utf8(bytes.to_vec()).expect("command output should be valid utf-8") -} - -#[expect( - clippy::disallowed_methods, - reason = "integration tests stage temporary SPA fixture files with sync std::fs::write" -)] -fn write_file(root: &Path, path: &str, contents: &[u8]) { - let path = root.join(path); - std::fs::create_dir_all(path.parent().expect("fixture path should have parent")) - .expect("creating fixture parent directory"); - std::fs::write(path, contents).expect("writing fixture file"); -} +use super::{fabro_dev, output_text, write_file}; #[test] fn refresh_spa_mirrors_dist_and_removes_source_maps() {