From 65d86ce97dafe571ae4101346ea8cbf4da8eca5f Mon Sep 17 00:00:00 2001 From: ITSGJY <133723203+ITSGJY@users.noreply.github.com> Date: Fri, 25 Sep 2026 15:41:14 +0800 Subject: [PATCH] refactor(validation): unify read-only execution planning --- crates/webcodex-core/src/runner_protocol.rs | 4 +- .../webcodex-core/src/runner_protocol/job.rs | 12 +- crates/webcodex-validation/src/adapters/go.rs | 31 +- .../webcodex-validation/src/adapters/mod.rs | 70 +++- .../webcodex-validation/src/adapters/rust.rs | 103 +++--- crates/webcodex-validation/src/lib.rs | 4 +- .../webcodex-validation/src/profile_tests.rs | 26 ++ src/tool_runtime/cargo.rs | 328 +++++------------- 8 files changed, 284 insertions(+), 294 deletions(-) diff --git a/crates/webcodex-core/src/runner_protocol.rs b/crates/webcodex-core/src/runner_protocol.rs index 05a54e534..74ead4281 100644 --- a/crates/webcodex-core/src/runner_protocol.rs +++ b/crates/webcodex-core/src/runner_protocol.rs @@ -4669,8 +4669,8 @@ mod filter_canonical_tests { #[test] fn cargo_value_contract_normalizes_exactly_once_and_fails_closed() { - // The shared normalization contract used by both the synchronous - // command builders and the structured Job argv builder. + // The shared normalization contract used by adapter-owned validation + // plans and Runner-side canonical-step validation. assert_eq!( normalize_cargo_value("serde").unwrap(), Some("serde".to_string()) diff --git a/crates/webcodex-core/src/runner_protocol/job.rs b/crates/webcodex-core/src/runner_protocol/job.rs index 0d7737616..fe6b1b59e 100644 --- a/crates/webcodex-core/src/runner_protocol/job.rs +++ b/crates/webcodex-core/src/runner_protocol/job.rs @@ -300,9 +300,9 @@ fn is_canonical_cargo_test_args(args: &[&str]) -> bool { } /// Normalize and validate one value-taking Cargo argument (`--features`, -/// `-p`). This is the single shared contract used by the synchronous command -/// builders and the structured long-Job argv builder, so a given request runs -/// identical arguments no matter how long it takes. +/// `-p`). This is the single shared normalization contract used by validation +/// planning and Runner-side canonical-step validation, so command-text and +/// structured-argv projections preserve identical argument semantics. /// /// Applies exactly one leading/trailing whitespace trim, then rejects values /// that are NUL/control-containing, longer than [`CARGO_VALUE_MAX_BYTES`], @@ -523,9 +523,9 @@ fn node_script_allowed(kind: &str, script: &str) -> bool { /// Normalize and validate the single argv value that may follow `cargo test`: /// a libtest name substring, never a Cargo option. This is the shared contract -/// used by the planner (`safe_rust_filter`), the synchronous command builder, -/// and the structured long-Job argv builder, so a given filter runs identically -/// regardless of runtime path. +/// used by the recipe planner (`safe_rust_filter`), adapter-owned validation +/// planning, and Runner-side canonical-step validation, so a given filter keeps +/// identical semantics regardless of runtime path. /// /// Applies exactly one leading/trailing trim and rejects control bytes, /// over-long values, and anything that begins with `-` after trimming, so a diff --git a/crates/webcodex-validation/src/adapters/go.rs b/crates/webcodex-validation/src/adapters/go.rs index 6909c61af..2790c1660 100644 --- a/crates/webcodex-validation/src/adapters/go.rs +++ b/crates/webcodex-validation/src/adapters/go.rs @@ -1,6 +1,8 @@ -use super::{ValidationAdapter, ValidationCommandOptions, ValidationFailureEvidence}; +use super::{ + read_only_validation_plan, ReadOnlyValidationPlan, ValidationAdapter, ValidationCommandOptions, + ValidationFailureEvidence, ValidationPlanArg, +}; use webcodex_core::runner_protocol::normalize_go_test_packages; -use webcodex_core::shell_quote::shell_escape_simple; use webcodex_core::validation_evidence::{parse_go_test_diagnostics, ValidationDiagnostics}; struct GoTestValidationAdapter; @@ -20,7 +22,10 @@ impl ValidationAdapter for GoTestValidationAdapter { "go_test" } - fn build_command(&self, options: ValidationCommandOptions) -> Result { + fn build_readonly_plan( + &self, + options: ValidationCommandOptions, + ) -> Result { if options.check || options.filter.is_some() || options.lib.is_some() @@ -33,14 +38,20 @@ impl ValidationAdapter for GoTestValidationAdapter { { return Err("go_test does not accept Cargo validation command options".to_string()); } - let Some(packages) = options.go_packages.as_deref() else { - return Ok("go test -json ./...".to_string()); - }; - let packages = normalize_go_test_packages(Some(packages)) + let explicit_packages = options.go_packages.is_some(); + let packages = normalize_go_test_packages(options.go_packages.as_deref()) .map_err(|reason| format!("packages {reason}"))?; - let mut command = vec!["go".to_string(), "test".to_string(), "-json".to_string()]; - command.extend(packages.iter().map(|package| shell_escape_simple(package))); - Ok(command.join(" ")) + let mut args = vec![ + ValidationPlanArg::Literal("test"), + ValidationPlanArg::Literal("-json"), + ]; + if explicit_packages { + args.extend(packages.into_iter().map(ValidationPlanArg::Value)); + } else { + debug_assert_eq!(packages.as_slice(), ["./..."]); + args.push(ValidationPlanArg::Literal("./...")); + } + read_only_validation_plan("test", "go", args) } fn parse( diff --git a/crates/webcodex-validation/src/adapters/mod.rs b/crates/webcodex-validation/src/adapters/mod.rs index 84c7c43a9..1042304b1 100644 --- a/crates/webcodex-validation/src/adapters/mod.rs +++ b/crates/webcodex-validation/src/adapters/mod.rs @@ -3,6 +3,8 @@ mod go; mod rust; +use webcodex_core::runner_protocol::ShellJobValidationStep; +use webcodex_core::shell_quote::shell_escape_simple; use webcodex_core::validation_evidence::ValidationDiagnostics; use webcodex_core::workflow_session_contract::ExecutionPurpose; @@ -24,6 +26,64 @@ pub struct ValidationCommandOptions { pub go_packages: Option>, } +/// Adapter-owned, read-only validation plan. +/// +/// `structured_step` is the canonical execution representation. The command +/// string is only the compatibility projection required by the existing +/// synchronous capture path; it is derived from the same typed arguments and +/// must never be parsed back into execution authority. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ReadOnlyValidationPlan { + pub compatibility_command: String, + pub structured_step: ShellJobValidationStep, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum ValidationPlanArg { + Literal(&'static str), + Value(String), +} + +impl ValidationPlanArg { + fn raw(&self) -> String { + match self { + Self::Literal(value) => (*value).to_string(), + Self::Value(value) => value.clone(), + } + } + + fn rendered(&self) -> String { + match self { + Self::Literal(value) => (*value).to_string(), + Self::Value(value) => shell_escape_simple(value), + } + } +} + +pub(super) fn read_only_validation_plan( + name: &'static str, + program: &'static str, + args: Vec, +) -> Result { + let structured_step = ShellJobValidationStep { + name: name.to_string(), + program: program.to_string(), + args: args.iter().map(ValidationPlanArg::raw).collect(), + env: Vec::new(), + }; + if !structured_step.is_canonical() { + return Err("structured validation step is not canonical".to_string()); + } + let compatibility_command = std::iter::once(program.to_string()) + .chain(args.iter().map(ValidationPlanArg::rendered)) + .collect::>() + .join(" "); + Ok(ReadOnlyValidationPlan { + compatibility_command, + structured_step, + }) +} + pub struct ValidationFailureEvidence<'a> { pub success: bool, pub reported_failure_kind: Option<&'a str>, @@ -38,7 +98,15 @@ pub trait ValidationAdapter: Sync { fn tool_identity(&self) -> &'static str; - fn build_command(&self, options: ValidationCommandOptions) -> Result; + fn build_readonly_plan( + &self, + options: ValidationCommandOptions, + ) -> Result; + + fn build_command(&self, options: ValidationCommandOptions) -> Result { + self.build_readonly_plan(options) + .map(|plan| plan.compatibility_command) + } fn parse( &self, diff --git a/crates/webcodex-validation/src/adapters/rust.rs b/crates/webcodex-validation/src/adapters/rust.rs index e5e24d1d4..99076df30 100644 --- a/crates/webcodex-validation/src/adapters/rust.rs +++ b/crates/webcodex-validation/src/adapters/rust.rs @@ -1,8 +1,10 @@ -use super::{ValidationAdapter, ValidationCommandOptions, ValidationFailureEvidence}; +use super::{ + read_only_validation_plan, ReadOnlyValidationPlan, ValidationAdapter, ValidationCommandOptions, + ValidationFailureEvidence, ValidationPlanArg, +}; use webcodex_core::runner_protocol::{ normalize_cargo_packages, normalize_cargo_value, normalize_rust_test_filter, }; -use webcodex_core::shell_quote::shell_escape_simple; use webcodex_core::validation_evidence::{ parse_cargo_check_diagnostics, parse_cargo_test_diagnostics, ValidationDiagnostics, }; @@ -54,17 +56,44 @@ impl ValidationAdapter for RustValidationAdapter { self.tool_identity } - fn build_command(&self, options: ValidationCommandOptions) -> Result { + fn build_readonly_plan( + &self, + options: ValidationCommandOptions, + ) -> Result { if options.go_packages.is_some() { return Err("Cargo validation does not accept go_test packages".to_string()); } match self.kind { - RustAdapterKind::Format => Ok(cargo_fmt_command(options.check)), - RustAdapterKind::Check => cargo_check_command(options), - RustAdapterKind::Test => cargo_test_command(options), + RustAdapterKind::Format => { + if !options.check { + return Err("cargo_fmt mutation is not read-only validation".to_string()); + } + read_only_validation_plan( + "format", + "cargo", + vec![ + ValidationPlanArg::Literal("fmt"), + ValidationPlanArg::Literal("--"), + ValidationPlanArg::Literal("--check"), + ], + ) + } + RustAdapterKind::Check => cargo_check_plan(options), + RustAdapterKind::Test => cargo_test_plan(options), } } + fn build_command(&self, options: ValidationCommandOptions) -> Result { + if self.kind == RustAdapterKind::Format && !options.check { + if options.go_packages.is_some() { + return Err("Cargo validation does not accept go_test packages".to_string()); + } + return Ok("cargo fmt".to_string()); + } + self.build_readonly_plan(options) + .map(|plan| plan.compatibility_command) + } + fn parse( &self, stdout_excerpt: &str, @@ -148,83 +177,75 @@ impl ValidationAdapter for RustValidationAdapter { } } -fn cargo_fmt_command(check: bool) -> String { - if check { - "cargo fmt -- --check".to_string() - } else { - "cargo fmt".to_string() - } -} - -fn cargo_check_command(options: ValidationCommandOptions) -> Result { +fn cargo_check_plan(options: ValidationCommandOptions) -> Result { let features = validate_arg("features", options.features)?; let packages = normalize_cargo_packages( options.package.as_deref(), options.cargo_packages.as_deref(), ) .map_err(|reason| format!("packages {reason}"))?; - let mut args = vec!["cargo".to_string(), "check".to_string()]; + let mut args = vec![ValidationPlanArg::Literal("check")]; if options.all_targets.unwrap_or(true) { - args.push("--all-targets".to_string()); + args.push(ValidationPlanArg::Literal("--all-targets")); } if options.all_features.unwrap_or(false) { - args.push("--all-features".to_string()); + args.push(ValidationPlanArg::Literal("--all-features")); } if options.no_default_features.unwrap_or(false) { - args.push("--no-default-features".to_string()); + args.push(ValidationPlanArg::Literal("--no-default-features")); } if let Some(features) = features { - args.push("--features".to_string()); - args.push(shell_escape_simple(&features)); + args.push(ValidationPlanArg::Literal("--features")); + args.push(ValidationPlanArg::Value(features)); } for package in packages.into_iter().flatten() { - args.push("-p".to_string()); - args.push(shell_escape_simple(&package)); + args.push(ValidationPlanArg::Literal("-p")); + args.push(ValidationPlanArg::Value(package)); } - Ok(args.join(" ")) + read_only_validation_plan("check", "cargo", args) } -fn cargo_test_command(options: ValidationCommandOptions) -> Result { +fn cargo_test_plan(options: ValidationCommandOptions) -> Result { if options.cargo_packages.is_some() { return Err("cargo_test does not accept cargo_check packages".to_string()); } let filter = validate_filter(options.filter)?; let features = validate_arg("features", options.features)?; let package = validate_arg("package", options.package)?; - let mut args = vec!["cargo".to_string(), "test".to_string()]; + let mut args = vec![ValidationPlanArg::Literal("test")]; if let Some(filter) = filter { - args.push(shell_escape_simple(&filter)); + args.push(ValidationPlanArg::Value(filter)); } if options.lib.unwrap_or(false) { - args.push("--lib".to_string()); + args.push(ValidationPlanArg::Literal("--lib")); } if options.all_targets.unwrap_or(false) { - args.push("--all-targets".to_string()); + args.push(ValidationPlanArg::Literal("--all-targets")); } if options.all_features.unwrap_or(false) { - args.push("--all-features".to_string()); + args.push(ValidationPlanArg::Literal("--all-features")); } if options.no_default_features.unwrap_or(false) { - args.push("--no-default-features".to_string()); + args.push(ValidationPlanArg::Literal("--no-default-features")); } if let Some(features) = features { - args.push("--features".to_string()); - args.push(shell_escape_simple(&features)); + args.push(ValidationPlanArg::Literal("--features")); + args.push(ValidationPlanArg::Value(features)); } if let Some(package) = package { - args.push("-p".to_string()); - args.push(shell_escape_simple(&package)); + args.push(ValidationPlanArg::Literal("-p")); + args.push(ValidationPlanArg::Value(package)); } if options.no_run.unwrap_or(false) { - args.push("--no-run".to_string()); + args.push(ValidationPlanArg::Literal("--no-run")); } - Ok(args.join(" ")) + read_only_validation_plan("test", "cargo", args) } -/// Normalize one value-taking Cargo option using the same shared contract the -/// structured Job argv builder uses, so a request produces identical -/// arguments whether it runs synchronously or as a long Job. The error label -/// is mapped to the tool-facing message. +/// Normalize one value-taking Cargo option before the adapter emits its single +/// read-only validation plan. The command-text and structured-argv projections +/// are then derived from that same normalized value. The error label is mapped +/// to the tool-facing message. fn validate_arg(label: &str, value: Option) -> Result, String> { match value { Some(raw) => match normalize_cargo_value(&raw) { diff --git a/crates/webcodex-validation/src/lib.rs b/crates/webcodex-validation/src/lib.rs index 0174a323b..a2338666c 100644 --- a/crates/webcodex-validation/src/lib.rs +++ b/crates/webcodex-validation/src/lib.rs @@ -17,8 +17,8 @@ mod profile_tests; mod recipe_tests; pub use adapters::{ - execution_purpose_for_validation_kind, validation_adapter_for_tool, ValidationAdapter, - ValidationCommandOptions, ValidationFailureEvidence, + execution_purpose_for_validation_kind, validation_adapter_for_tool, ReadOnlyValidationPlan, + ValidationAdapter, ValidationCommandOptions, ValidationFailureEvidence, }; pub use evidence::{ current_validation_evidence_for_session, event_is_job_acceptance_only, diff --git a/crates/webcodex-validation/src/profile_tests.rs b/crates/webcodex-validation/src/profile_tests.rs index 26e26ff08..1ee7fcebe 100644 --- a/crates/webcodex-validation/src/profile_tests.rs +++ b/crates/webcodex-validation/src/profile_tests.rs @@ -70,6 +70,32 @@ fn rust_profile_selects_cargo_fmt_adapter_and_preserves_command() { ); } +#[test] +fn cargo_fmt_read_only_plan_excludes_mutating_format() { + let adapter = validation_adapter_for_tool("cargo_fmt").expect("cargo_fmt adapter"); + let readonly = adapter + .build_readonly_plan(ValidationCommandOptions { + check: true, + ..ValidationCommandOptions::default() + }) + .unwrap(); + assert_eq!(readonly.compatibility_command, "cargo fmt -- --check"); + assert_eq!(readonly.structured_step.name, "format"); + assert_eq!(readonly.structured_step.program, "cargo"); + assert_eq!(readonly.structured_step.args, ["fmt", "--", "--check"]); + assert!(readonly.structured_step.is_canonical()); + + assert!(adapter + .build_readonly_plan(ValidationCommandOptions::default()) + .is_err()); + assert_eq!( + adapter + .build_command(ValidationCommandOptions::default()) + .unwrap(), + "cargo fmt" + ); +} + #[test] fn rust_profile_selects_cargo_check_adapter_and_preserves_command() { let adapter = validation_adapter_for_tool("cargo_check").expect("cargo_check adapter"); diff --git a/src/tool_runtime/cargo.rs b/src/tool_runtime/cargo.rs index 7e96a2aac..d2505b709 100644 --- a/src/tool_runtime/cargo.rs +++ b/src/tool_runtime/cargo.rs @@ -998,8 +998,8 @@ impl ToolRuntime { no_run: request.no_run, go_packages: request.go_packages, }; - let command = match adapter.build_command(options.clone()) { - Ok(command) => command, + let plan = match adapter.build_readonly_plan(options.clone()) { + Ok(plan) => plan, Err(e) => { return ToolResult::err(command_rejected_message( e, @@ -1011,6 +1011,8 @@ impl ToolRuntime { )) } }; + let command = plan.compatibility_command; + let step = plan.structured_step; let requires_multi_package_cargo_check = tool_name == "cargo_check" && crate::runner_protocol::normalize_cargo_packages( options.package.as_deref(), @@ -1083,7 +1085,7 @@ impl ToolRuntime { cwd.as_deref(), &command, adapter, - options, + step, purpose, timeout_secs, sync_wait_secs, @@ -1157,7 +1159,7 @@ impl ToolRuntime { cwd: Option<&str>, command: &str, adapter: &'static dyn ValidationAdapter, - options: ValidationCommandOptions, + step: ShellJobValidationStep, purpose: ExecutionPurpose, timeout_secs: u64, sync_wait_secs: u64, @@ -1183,19 +1185,9 @@ impl ToolRuntime { let resolved_cwd = super::helpers::project_relative_runner_cwd(config, &effective_cwd) .unwrap_or_else(|_| ".".to_string()); let actual_shell = "configured"; - // The validation step is derived from the same options the tool would - // have run synchronously, so the promoted Job executes the identical - // command (not a re-constructed string). Passing the step in the job - // metadata makes the request kind `start_validation_job`, so the - // Runner runs the cargo program+argv directly (never the raw command - // through a shell). - let step = validation_step(tool_name, &options); - let Ok(step) = step else { - return ToolResult::err(command_rejected_message( - "could not encode structured validation step", - "fix the structured validation argument format, then retry.", - )); - }; + // The adapter owns the canonical read-only execution plan. The same + // structured step that produced the compatibility command text is + // carried into the Job, so sync and promoted paths cannot drift. let dispatched_command = match serde_json::to_string(std::slice::from_ref(&step)) { Ok(command) => command, Err(_) => { @@ -1816,117 +1808,6 @@ fn validation_handoff_failure_result( result } -/// Build the canonical structured validation step for a read-only validation tool -/// from the same options the synchronous adapter would have used. -fn validation_step( - tool_name: &str, - options: &ValidationCommandOptions, -) -> Result { - if tool_name != "go_test" && options.go_packages.is_some() { - return Err("only go_test accepts Go package patterns".to_string()); - } - let (name, program, args) = match tool_name { - "cargo_fmt" => ( - "format", - "cargo", - vec!["fmt".to_string(), "--".to_string(), "--check".to_string()], - ), - "cargo_check" => { - let packages = crate::runner_protocol::normalize_cargo_packages( - options.package.as_deref(), - options.cargo_packages.as_deref(), - ) - .map_err(|reason| format!("packages {reason}"))?; - let mut args = vec!["check".to_string()]; - if options.all_targets.unwrap_or(true) { - args.push("--all-targets".to_string()); - } - if options.all_features.unwrap_or(false) { - args.push("--all-features".to_string()); - } - if options.no_default_features.unwrap_or(false) { - args.push("--no-default-features".to_string()); - } - push_paired_arg(&mut args, "--features", options.features.as_deref())?; - for package in packages.into_iter().flatten() { - push_paired_arg(&mut args, "-p", Some(&package))?; - } - ("check", "cargo", args) - } - "cargo_test" => { - if options.cargo_packages.is_some() { - return Err("cargo_test does not accept cargo_check packages".to_string()); - } - let mut args = vec!["test".to_string()]; - if let Some(filter) = options.filter.as_deref() { - // Whitespace-only filter means "no filter", matching the - // synchronous path. Option-like filters are rejected by the - // shared filter contract before any argv is built. - if let Some(normalized) = - crate::runner_protocol::normalize_rust_test_filter(filter)? - { - args.push(normalized); - } - } - if options.lib.unwrap_or(false) { - args.push("--lib".to_string()); - } - if options.all_targets.unwrap_or(false) { - args.push("--all-targets".to_string()); - } - if options.all_features.unwrap_or(false) { - args.push("--all-features".to_string()); - } - if options.no_default_features.unwrap_or(false) { - args.push("--no-default-features".to_string()); - } - push_paired_arg(&mut args, "--features", options.features.as_deref())?; - push_paired_arg(&mut args, "-p", options.package.as_deref())?; - if options.no_run.unwrap_or(false) { - args.push("--no-run".to_string()); - } - ("test", "cargo", args) - } - "go_test" => { - let packages = - crate::runner_protocol::normalize_go_test_packages(options.go_packages.as_deref()) - .map_err(|reason| format!("packages {reason}"))?; - let mut args = vec!["test".to_string(), "-json".to_string()]; - args.extend(packages); - ("test", "go", args) - } - _ => return Err("unknown validation tool".to_string()), - }; - let step = ShellJobValidationStep { - name: name.to_string(), - program: program.to_string(), - args, - env: Vec::new(), - }; - if !step.is_canonical() { - return Err("structured validation step is not canonical".to_string()); - } - Ok(step) -} - -/// Append a value-taking Cargo flag with its already-normalized value. -/// Normalization is the shared `normalize_cargo_value` contract (a single -/// trim, non-empty, not `-`-prefixed, NUL/control-free, bounded), so the -/// structured argv matches what the synchronous path would have built. Values -/// are normalized here and written normalized into argv, never passed through -/// raw. -fn push_paired_arg(args: &mut Vec, flag: &str, value: Option<&str>) -> Result<(), String> { - let Some(value) = value else { - return Ok(()); - }; - let Some(normalized) = crate::runner_protocol::normalize_cargo_value(value)? else { - return Ok(()); - }; - args.push(flag.to_string()); - args.push(normalized); - Ok(()) -} - fn apply_validation_projection_fields(payload: &mut Value, projection: &Value) { for field in [ "passed", @@ -2090,14 +1971,12 @@ mod structured_cargo_arg_parity_tests { } } - /// The structured Job argv builder must normalize a value-taking Cargo - /// argument identically to the synchronous command builder, so the same - /// request produces the same effective arguments no matter how long it - /// runs. Whitespace-padded inputs are normalized in both paths; invalid - /// values fail closed before any argv is produced. + /// One adapter-owned plan must preserve both the legacy command projection + /// and the canonical Job argv. Exact expected values stay independent of + /// the planner so a shared producer bug cannot make this test self-fulfilling. #[test] - fn job_and_sync_paths_produce_the_same_normalized_values() { - for (tool_name, options) in [ + fn read_only_plan_preserves_exact_command_and_structured_argv() { + for (tool_name, options, expected_command, expected_args) in [ ( "cargo_check", ValidationCommandOptions { @@ -2108,6 +1987,17 @@ mod structured_cargo_arg_parity_tests { package: Some(" my-crate ".to_string()), ..ValidationCommandOptions::default() }, + "cargo check --all-targets --all-features --no-default-features --features 'serde' -p 'my-crate'", + vec![ + "check", + "--all-targets", + "--all-features", + "--no-default-features", + "--features", + "serde", + "-p", + "my-crate", + ], ), ( "cargo_test", @@ -2122,86 +2012,59 @@ mod structured_cargo_arg_parity_tests { no_run: Some(true), ..ValidationCommandOptions::default() }, + "cargo test 'module::nested::test' --lib --all-targets --all-features --no-default-features --features 'a b' -p 'my-crate' --no-run", + vec![ + "test", + "module::nested::test", + "--lib", + "--all-targets", + "--all-features", + "--no-default-features", + "--features", + "a b", + "-p", + "my-crate", + "--no-run", + ], ), ] { - // Sync path: build_command normalizes via the shared contract and - // shell-escapes. The parsed argv words after `cargo ` must - // contain the normalized values. let adapter = validation_adapter_for_tool(tool_name).unwrap(); - let sync = adapter - .build_command(options.clone()) - .unwrap_or_else(|error| panic!("{tool_name} sync build: {error}")); - assert!( - sync.contains("serde") || sync.contains("a b"), - "{tool_name} sync command missing normalized feature: {sync}" - ); - assert!(sync.contains("my-crate"), "{tool_name} sync: {sync}"); - if tool_name == "cargo_test" { - assert!( - sync.contains("module::nested::test"), - "cargo_test sync missing normalized filter: {sync}" - ); - assert!( - sync.contains("--lib"), - "cargo_test sync missing --lib: {sync}" - ); - assert!( - sync.contains("--all-targets") && sync.contains("--no-run"), - "cargo_test sync must preserve native lib/all-targets/no-run composition: {sync}" - ); - } - - // Job path: validation_step writes normalized values into the - // structured argv, never the raw padded strings. - let step = validation_step(tool_name, &options).unwrap(); - let joined = step.args.join(" "); - assert!( - !joined.contains(" serde") && !joined.contains("serde "), - "{tool_name} job argv must contain normalized feature: {joined:?}" + let plan = adapter + .build_readonly_plan(options) + .unwrap_or_else(|error| panic!("{tool_name} plan build: {error}")); + assert_eq!(plan.compatibility_command, expected_command, "{tool_name}"); + assert_eq!( + plan.structured_step.args, + expected_args + .into_iter() + .map(str::to_string) + .collect::>(), + "{tool_name}" ); - assert!( - joined.contains("serde") || joined.contains("a b"), - "{tool_name} job argv missing feature: {joined:?}" - ); - assert!( - !joined.contains(" my-crate") && !joined.contains("my-crate "), - "{tool_name} job argv must contain normalized package: {joined:?}" - ); - assert!(joined.contains("my-crate"), "{tool_name} job: {joined:?}"); - if tool_name == "cargo_test" { - assert!( - step.args.iter().any(|arg| arg == "module::nested::test"), - "cargo_test job argv must contain the normalized filter: {joined:?}" - ); - assert!(step.args.iter().any(|arg| arg == "--lib")); - assert!(step.args.iter().any(|arg| arg == "--all-targets")); - assert!(step.args.iter().any(|arg| arg == "--no-run")); - } - assert!(step.is_canonical(), "{tool_name} step must be canonical"); + assert!(plan.structured_step.is_canonical(), "{tool_name} step must be canonical"); } } #[test] - fn cargo_test_lib_false_and_omission_are_command_equivalent() { + fn cargo_test_lib_false_and_omission_share_one_plan() { let adapter = validation_adapter_for_tool("cargo_test").unwrap(); - let omitted = ValidationCommandOptions::default(); - let explicit_false = ValidationCommandOptions { - lib: Some(false), - ..ValidationCommandOptions::default() - }; - - assert_eq!( - adapter.build_command(omitted.clone()).unwrap(), - adapter.build_command(explicit_false.clone()).unwrap() - ); - let omitted_step = validation_step("cargo_test", &omitted).unwrap(); - let false_step = validation_step("cargo_test", &explicit_false).unwrap(); - assert_eq!(omitted_step.args, false_step.args); - assert!(!omitted_step.args.iter().any(|arg| arg == "--lib")); + let omitted = adapter + .build_readonly_plan(ValidationCommandOptions::default()) + .unwrap(); + let explicit_false = adapter + .build_readonly_plan(ValidationCommandOptions { + lib: Some(false), + ..ValidationCommandOptions::default() + }) + .unwrap(); + + assert_eq!(omitted, explicit_false); + assert_eq!(omitted.compatibility_command, "cargo test"); + assert_eq!(omitted.structured_step.args, vec!["test"]); } #[test] - fn go_test_job_and_sync_paths_share_normalized_package_scope() { + fn go_test_read_only_plan_preserves_package_projection() { let options = ValidationCommandOptions { go_packages: Some(vec![ "./internal/control".to_string(), @@ -2210,24 +2073,24 @@ mod structured_cargo_arg_parity_tests { ..ValidationCommandOptions::default() }; let adapter = validation_adapter_for_tool("go_test").unwrap(); + let plan = adapter.build_readonly_plan(options).unwrap(); assert_eq!( - adapter.build_command(options.clone()).unwrap(), + plan.compatibility_command, "go test -json './internal/control' './internal/node'" ); - let step = validation_step("go_test", &options).unwrap(); - assert_eq!(step.name, "test"); - assert_eq!(step.program, "go"); + assert_eq!(plan.structured_step.name, "test"); + assert_eq!(plan.structured_step.program, "go"); assert_eq!( - step.args, + plan.structured_step.args, vec!["test", "-json", "./internal/control", "./internal/node"] ); - assert!(step.env.is_empty()); - assert!(step.is_structured_go_test_json()); - assert!(step.is_canonical()); + assert!(plan.structured_step.env.is_empty()); + assert!(plan.structured_step.is_structured_go_test_json()); + assert!(plan.structured_step.is_canonical()); } #[test] - fn invalid_cargo_values_fail_closed_on_both_paths() { + fn invalid_cargo_values_fail_before_a_read_only_plan_exists() { for (tool_name, invalid) in [ ( "cargo_check", @@ -2272,23 +2135,21 @@ mod structured_cargo_arg_parity_tests { }, ), ] { - // Sync path: build_command must reject before any command string. let adapter = validation_adapter_for_tool(tool_name).unwrap(); assert!( - adapter.build_command(invalid.clone()).is_err(), - "{tool_name} sync must reject {invalid:?}" + adapter.build_readonly_plan(invalid.clone()).is_err(), + "{tool_name} plan must reject {invalid:?}" ); - // Job path: validation_step must reject before any argv is built. assert!( - validation_step(tool_name, &invalid).is_err(), - "{tool_name} job must reject {invalid:?}" + adapter.build_command(invalid.clone()).is_err(), + "{tool_name} compatibility command must reject {invalid:?}" ); } } #[test] - fn whitespace_only_values_mean_option_omitted_on_both_paths() { - for (tool_name, options) in [ + fn whitespace_only_values_are_omitted_from_both_plan_projections() { + for (tool_name, options, expected_command, expected_args) in [ ( "cargo_check", ValidationCommandOptions { @@ -2296,6 +2157,8 @@ mod structured_cargo_arg_parity_tests { package: Some(" ".to_string()), ..ValidationCommandOptions::default() }, + "cargo check --all-targets", + vec!["check", "--all-targets"], ), ( "cargo_test", @@ -2304,24 +2167,25 @@ mod structured_cargo_arg_parity_tests { features: Some(" ".to_string()), ..ValidationCommandOptions::default() }, + "cargo test", + vec!["test"], ), ] { let adapter = validation_adapter_for_tool(tool_name).unwrap(); - let sync = adapter.build_command(options.clone()).unwrap(); - assert!( - !sync.contains("--features") && !sync.contains(" -p "), - "{tool_name} whitespace-only values must be omitted: {sync}" + let plan = adapter.build_readonly_plan(options).unwrap(); + assert_eq!(plan.compatibility_command, expected_command, "{tool_name}"); + assert_eq!( + plan.structured_step.args, + expected_args + .into_iter() + .map(str::to_string) + .collect::>(), + "{tool_name}" ); - let step = validation_step(tool_name, &options).unwrap(); assert!( - !step - .args - .iter() - .any(|arg| arg == "--features" || arg == "-p"), - "{tool_name} whitespace-only values must be omitted: {:?}", - step.args + plan.structured_step.is_canonical(), + "{tool_name} step must be canonical" ); - assert!(step.is_canonical(), "{tool_name} step must be canonical"); } } }