From 0244736b053d49f86712c70ac278f416b4795339 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Wed, 1 Jul 2026 10:31:26 -0400 Subject: [PATCH] Resolve run.prepare.steps env and interpolation at the run boundary (#530) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Per-step environment in `run.prepare.steps[].env` was parsed and then **dropped** before it reached the resolved run settings, so prepare steps could never see their declared env. This PR carries that env all the way through to the executor, resolves prepare-step interpolation at the run boundary, and fixes an argv-quoting bug. Three things: 1. **Per-step env is carried through.** `RunPrepareSettings` now holds `steps: Vec` (command plus per-step `env`) instead of a flat `commands: Vec`. The per-step env reaches `exec_command`, which already accepts per-command env vars, and is merged on top of the base sandbox environment. 2. **Interpolation resolves at the run boundary.** Prepare-step `script`/`command` and per-step `env` values are carried in source form out of the portable config resolve layer (so `fabro validate` stays portable and never requires env to be set). Their `{{ env.* }}` tokens resolve in the process that actually runs the steps, via `RunPrepareSettings::resolve_step_env` — mirroring the existing MCP transport env resolution. A missing env var is a **hard error** (fail-closed); there is no fallback to the unresolved literal. 3. **Argv is shell-quoted.** Argv-style prepare steps were assembled with `join(" ")`, so an argument containing spaces or quotes was re-split by the shell. They are now shell-quoted per element with the shared `shell_quote()` helper. `script` steps stay verbatim because they are raw shell snippets. ## How - `RunPrepareSettings.commands: Vec` becomes `RunPrepareSettings.steps: Vec` where `PreparedStep { command, env }`. The server-side `{{ vars.* }}` substitution pass now walks each step's command and env. - New `RunPrepareSettings::resolve_step_env(env_lookup)` resolves `{{ env.* }}` in each step's command and env values, returning a hard error on a missing var (and a loud `Unavailable` error for reserved `secrets`/`inputs` tokens). - The run boundary (`fabro_workflow::operations::start`) gains `runtime_setup_commands`, the prepare-step counterpart to `runtime_mcp_server`. `LifecycleOptions` now carries `Vec` (command + env), and the initialize phase passes each step's env to `exec_command`. - `resolve_prepare` shell-quotes each argv element and carries per-step env in source form. The stale lint suppression on the resolved fields is rewritten to describe the deliberate source preservation that now resolves at the run boundary. - The shell-quoting helper moves to a shared `fabro_util::shell` module (backed by `shlex`); `fabro_sandbox::shell_quote` delegates to it so the config resolve layer and sandbox code share one audited implementation. - The OpenAPI `RunPrepareSettings` schema and the generated TypeScript client are updated to the new `steps`/`PreparedStep` shape. ## Testing - `cargo build --workspace` - `cargo +nightly-2026-04-14 fmt --check --all` - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` - `cargo nextest run` for `fabro-util`, `fabro-types`, `fabro-config`, `fabro-sandbox`, `fabro-api`, `fabro-workflow`, `fabro-server`, `fabro-cli` (provider keys stripped) — all green. - `cd lib/packages/fabro-api-client && bun run typecheck` — clean. New tests cover: per-step env carried through resolution; script/command + env resolved at the run boundary; a missing env var is a hard error (in both the command and a per-step env value); reserved `secrets` tokens surface as `Unavailable`; argv elements are shell-quoted (an arg with spaces/quotes is correctly quoted) while a `script` stays verbatim; and an end-to-end check that per-step env reaches the executed setup command (with a negative control proving the success is attributable to the per-step env). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- Cargo.lock | 6 +- docs/public/api-reference/fabro-api.yaml | 51 ++- lib/crates/fabro-acp/Cargo.toml | 3 +- lib/crates/fabro-acp/src/command.rs | 14 +- lib/crates/fabro-cli/tests/it/cmd/attach.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/config.rs | 8 +- lib/crates/fabro-cli/tests/it/cmd/inspect.rs | 2 +- lib/crates/fabro-config/src/resolve/mod.rs | 8 +- lib/crates/fabro-config/src/resolve/run.rs | 159 ++++++-- lib/crates/fabro-dev/Cargo.toml | 2 +- lib/crates/fabro-dev/src/commands/mod.rs | 7 +- lib/crates/fabro-sandbox/Cargo.toml | 1 - lib/crates/fabro-sandbox/src/sandbox.rs | 9 +- lib/crates/fabro-server/src/demo/mod.rs | 19 +- lib/crates/fabro-server/src/run_manifest.rs | 14 +- lib/crates/fabro-types/Cargo.toml | 1 + lib/crates/fabro-types/src/settings/mod.rs | 10 +- lib/crates/fabro-types/src/settings/run.rs | 359 +++++++++++++++++- lib/crates/fabro-util/Cargo.toml | 1 + lib/crates/fabro-util/src/lib.rs | 1 + lib/crates/fabro-util/src/shell.rs | 65 ++++ lib/crates/fabro-workflow/Cargo.toml | 1 - .../fabro-workflow/src/handler/command.rs | 9 +- .../fabro-workflow/src/operations/start.rs | 38 +- .../src/pipeline/execute/tests.rs | 14 +- .../fabro-workflow/src/pipeline/initialize.rs | 54 ++- lib/crates/fabro-workflow/src/run_options.rs | 14 +- .../src/.openapi-generator/FILES | 3 + .../fabro-api-client/src/models/index.ts | 3 + .../src/models/prepared-command-step.ts | 27 ++ .../src/models/prepared-script-step.ts | 27 ++ .../src/models/prepared-step.ts | 27 ++ .../src/models/run-prepare-settings.ts | 5 +- 33 files changed, 854 insertions(+), 110 deletions(-) create mode 100644 lib/crates/fabro-util/src/shell.rs create mode 100644 lib/packages/fabro-api-client/src/models/prepared-command-step.ts create mode 100644 lib/packages/fabro-api-client/src/models/prepared-script-step.ts create mode 100644 lib/packages/fabro-api-client/src/models/prepared-step.ts diff --git a/Cargo.lock b/Cargo.lock index c2c438b6b..f5473abc4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2510,8 +2510,8 @@ dependencies = [ "csv", "fabro-config", "fabro-options-metadata", + "fabro-util", "quick-xml 0.36.2", - "shlex", "tempfile", "toml_edit", "tracing-subscriber", @@ -2881,7 +2881,6 @@ dependencies = [ "serde", "serde_json", "sha2 0.10.9", - "shlex", "strum 0.28.0", "tar", "tempfile", @@ -3158,6 +3157,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.10.9", + "shlex", "strum 0.28.0", "tempfile", "toml 0.8.23", @@ -3178,6 +3178,7 @@ dependencies = [ "rand 0.9.4", "serde", "serde_json", + "shlex", "tempfile", "termimad", "tokio", @@ -3284,7 +3285,6 @@ dependencies = [ "serde", "serde_json", "sha2 0.10.9", - "shlex", "tempfile", "thiserror 2.0.18", "tokio", diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 727c8cff5..d6e384405 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -13851,16 +13851,61 @@ components: RunPrepareSettings: type: object - required: [commands, timeout_ms] + required: [steps, timeout_ms] properties: - commands: + steps: type: array items: - type: string + $ref: "#/components/schemas/PreparedStep" timeout_ms: type: integer format: int64 + PreparedStep: + description: | + A single resolved prepare step. The runnable part preserves the + script-vs-argv distinction via the `type` discriminator: a `script` + is a raw shell snippet kept verbatim, while a `command` is an argv + whose elements are shell-quoted and joined at the run boundary (after + `{{ env.* }}` resolution) so an interpolated value cannot inject shell + syntax. Optional per-step `env` is shared by both shapes. + type: object + required: [type] + oneOf: + - $ref: "#/components/schemas/PreparedScriptStep" + - $ref: "#/components/schemas/PreparedCommandStep" + discriminator: + propertyName: type + mapping: + script: "#/components/schemas/PreparedScriptStep" + command: "#/components/schemas/PreparedCommandStep" + + PreparedScriptStep: + type: object + required: [type, script] + properties: + type: + type: string + enum: [script] + script: + type: string + env: + $ref: "#/components/schemas/StringMap" + + PreparedCommandStep: + type: object + required: [type, command] + properties: + type: + type: string + enum: [command] + command: + type: array + items: + type: string + env: + $ref: "#/components/schemas/StringMap" + RunExecutionSettings: type: object required: [mode, approval] diff --git a/lib/crates/fabro-acp/Cargo.toml b/lib/crates/fabro-acp/Cargo.toml index 3089b5c48..2fb62b2d9 100644 --- a/lib/crates/fabro-acp/Cargo.toml +++ b/lib/crates/fabro-acp/Cargo.toml @@ -11,7 +11,6 @@ default = ["runtime"] runtime = [ "dep:fabro-sandbox", "dep:fabro-types", - "dep:fabro-util", "dep:futures", "dep:tokio", "dep:tokio-util", @@ -30,7 +29,7 @@ agent-client-protocol.workspace = true agent-client-protocol-tokio.workspace = true fabro-sandbox = { path = "../fabro-sandbox", optional = true } fabro-types = { path = "../fabro-types", optional = true } -fabro-util = { path = "../fabro-util", optional = true } +fabro-util = { path = "../fabro-util" } serde_json.workspace = true shlex = "1" thiserror.workspace = true diff --git a/lib/crates/fabro-acp/src/command.rs b/lib/crates/fabro-acp/src/command.rs index 1ffae80cc..8e9d335dd 100644 --- a/lib/crates/fabro-acp/src/command.rs +++ b/lib/crates/fabro-acp/src/command.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use agent_client_protocol::schema::{McpServer, McpServerStdio}; use agent_client_protocol_tokio::AcpAgent; +use fabro_util::shell::shell_join; #[derive(Debug, Clone, PartialEq, Eq)] pub struct AcpProcessSpec { @@ -143,11 +144,7 @@ pub enum AcpCommandError { } fn render_command(program: &Path, args: &[String]) -> String { - std::iter::once(program.to_string_lossy().into_owned()) - .chain(args.iter().cloned()) - .map(|part| shell_quote(&part)) - .collect::>() - .join(" ") + shell_join(std::iter::once(program.to_string_lossy().into_owned()).chain(args.iter().cloned())) } fn parse_config_server(raw: &str) -> Result { @@ -171,13 +168,6 @@ fn parse_config_server(raw: &str) -> Result { serde_json::from_value(value).map_err(AcpCommandError::InvalidConfigJson) } -fn shell_quote(s: &str) -> String { - shlex::try_quote(s).map_or_else( - |_| format!("'{}'", s.replace('\'', "'\\''")), - |quoted| quoted.to_string(), - ) -} - #[cfg(test)] mod tests { use std::path::Path; diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index 509947bf8..6e7a6a051 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -995,7 +995,7 @@ fn attach_json_errors_without_prompting_for_human_input() { }, "notifications": {}, "prepare": { - "commands": [], + "steps": [], "timeout_ms": 300000 }, "pull_request": null, diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index 7aed153ca..ff36e155a 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -357,10 +357,12 @@ fn create_explicit_workflow_path_uses_project_config_relative_to_workflow() { run_spec["settings"]["run"]["model"]["name"].as_str(), Some("gpt-5.4-pro") ); - // v2 R30: run.prepare.steps replaces the whole ordered list across layers. + // run.prepare.steps replaces the whole ordered list across layers. A + // `script` step serializes with the `type` discriminator that preserves the + // script-vs-argv distinction in the run spec wire shape. assert_eq!( - run_spec["settings"]["run"]["prepare"]["commands"], - serde_json::json!(["workflow-setup"]) + run_spec["settings"]["run"]["prepare"]["steps"], + serde_json::json!([{ "type": "script", "script": "workflow-setup" }]) ); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/inspect.rs b/lib/crates/fabro-cli/tests/it/cmd/inspect.rs index a28f2b381..fef015390 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/inspect.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/inspect.rs @@ -139,7 +139,7 @@ fn inspect_resolves_selector_via_server_endpoint() { "author": null }, "prepare": { - "commands": [], + "steps": [], "timeout_ms": 300000 }, "execution": { diff --git a/lib/crates/fabro-config/src/resolve/mod.rs b/lib/crates/fabro-config/src/resolve/mod.rs index f0c6e5c47..083042604 100644 --- a/lib/crates/fabro-config/src/resolve/mod.rs +++ b/lib/crates/fabro-config/src/resolve/mod.rs @@ -73,10 +73,10 @@ pub(crate) fn default_string(path: impl AsRef) -> String { path.as_ref().to_string_lossy().into_owned() } -/// Warn when a field demoted out of the interpolation set (D2) still contains -/// claimed template tokens. These fields are plain `String` now — `{{ vars.* -/// }}` (which previously substituted via the run-scoped String pass, a -/// now-removed accident) and `{{ env.* }}` are both treated as literal text. +/// Warn when a field demoted out of the interpolation set still contains +/// claimed template tokens. These fields are plain `String` now, so +/// `{{ vars.* }}` (which previously substituted via the run-scoped String pass, +/// a now-removed accident) and `{{ env.* }}` are both treated as literal text. /// Other plain-`String` fields still substitute `{{ vars.* }}` until the /// String pass itself is retired in a later slice. Unclaimed `{{ ... }}` text /// (jq programs, Go templates) never interpolated and does not warn. diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index b22f101f9..55e883371 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -4,11 +4,11 @@ use fabro_types::settings::InterpString; use fabro_types::settings::run::{ ArtifactsSettings, GitAuthorSettings, HookDefinition, HookType, InterviewProviderSettings, McpServerSettings, McpTransport, MergeStrategy, NotificationProviderSettings, - NotificationRouteSettings, PullRequestSettings, ResolvedMcpEntry, RunAgentSettings, - RunBranchSettings, RunCheckpointSettings, RunCloneSettings, RunExecutionSettings, - RunGitSettings, RunGoal, RunIntegrationsGithubSettings, RunIntegrationsSettings, - RunInterviewsSettings, RunMetaBranchSettings, RunModelControls, RunModelSettings, RunNamespace, - RunPrepareSettings, RunScmSettings, ScmGitHubSettings, TlsMode, + NotificationRouteSettings, PreparedStep, PreparedStepRun, PullRequestSettings, + ResolvedMcpEntry, RunAgentSettings, RunBranchSettings, RunCheckpointSettings, RunCloneSettings, + RunExecutionSettings, RunGitSettings, RunGoal, RunIntegrationsGithubSettings, + RunIntegrationsSettings, RunInterviewsSettings, RunMetaBranchSettings, RunModelControls, + RunModelSettings, RunNamespace, RunPrepareSettings, RunScmSettings, ScmGitHubSettings, TlsMode, }; use super::{ResolveError, resolve_run_environment}; @@ -154,8 +154,9 @@ fn resolve_git(git: Option<&RunGitLayer>) -> RunGitSettings { #[expect( clippy::disallowed_methods, - reason = "known leak: prepare step templates collapse to raw source unresolved; strict \ - resolution scheduled for follow-up interpolation cleanup" + reason = "intentional source preservation: prepare step commands and per-step env are carried \ + in source form so `fabro validate` stays portable; their {{ env.* }} tokens resolve \ + at the run boundary in fabro_types::settings::run::RunPrepareSettings::resolve_step_env" )] fn resolve_prepare( prepare: Option<&RunPrepareLayer>, @@ -163,25 +164,44 @@ fn resolve_prepare( ) -> RunPrepareSettings { let prepare = prepare.expect("defaults.toml should provide run.prepare defaults"); - let mut commands = Vec::new(); + let mut steps = Vec::new(); for (index, step) in prepare.steps.iter().enumerate() { - match (&step.script, &step.command) { - (Some(script), None) => commands.push(script.as_source()), - (None, Some(argv)) => commands.push( - argv.iter() - .map(InterpString::as_source) - .collect::>() - .join(" "), - ), - (Some(_), Some(_)) | (None, None) => errors.push(ResolveError::Invalid { - path: format!("run.prepare.steps[{index}]"), - reason: "exactly one of script or command must be set".to_string(), - }), - } + let run = match (&step.script, &step.command) { + // A `script` is a raw shell snippet: carry it verbatim so the shell + // interprets it. Its `{{ env.* }}` tokens resolve at the run + // boundary. + (Some(script), None) => PreparedStepRun::Script { + script: script.as_source(), + }, + // A `command` is an argv: carry it as a vector of element source + // strings — neither pre-joined nor shell-quoted here. Each element's + // `{{ env.* }}` token resolves at the run boundary, and only the + // resolved value is shell-quoted (resolve-then-quote), so an + // interpolated value can never break out of its argument and inject + // shell syntax. + (None, Some(argv)) => PreparedStepRun::Command { + command: argv.iter().map(InterpString::as_source).collect(), + }, + (Some(_), Some(_)) | (None, None) => { + errors.push(ResolveError::Invalid { + path: format!("run.prepare.steps[{index}]"), + reason: "exactly one of script or command must be set".to_string(), + }); + continue; + } + }; + steps.push(PreparedStep { + run, + env: step + .env + .iter() + .map(|(key, value)| (key.clone(), value.as_source())) + .collect(), + }); } RunPrepareSettings { - commands, + steps, timeout_ms: prepare.timeout.map_or(300_000, |timeout| { u64::try_from(timeout.as_std().as_millis()).unwrap_or(u64::MAX) }), @@ -603,3 +623,98 @@ fn resolve_artifacts(artifacts: Option<&RunArtifactsLayer>) -> ArtifactsSettings .unwrap_or_default(), } } + +#[cfg(test)] +mod resolve_prepare_tests { + use std::collections::HashMap; + + use fabro_types::settings::InterpString; + use fabro_types::settings::run::PreparedStepRun; + + use super::{ResolveError, resolve_prepare}; + use crate::{PrepareStep, RunPrepareLayer}; + + fn resolve(layer: &RunPrepareLayer) -> Vec { + let mut errors: Vec = Vec::new(); + let settings = resolve_prepare(Some(layer), &mut errors); + assert!(errors.is_empty(), "unexpected resolve errors: {errors:?}"); + settings.steps + } + + #[test] + fn carries_per_step_env_through() { + let layer = RunPrepareLayer { + steps: vec![PrepareStep { + script: Some(InterpString::parse("setup")), + command: None, + env: HashMap::from([( + "TOKEN".to_string(), + InterpString::parse("{{ env.DEPLOY_TOKEN }}"), + )]), + }], + timeout: None, + }; + + let steps = resolve(&layer); + + assert_eq!(steps.len(), 1); + assert_eq!(steps[0].run, PreparedStepRun::Script { + script: "setup".to_string(), + }); + // The per-step env survives resolution in source form (its {{ env.* }} + // token resolves later, at the run boundary). + assert_eq!( + steps[0].env.get("TOKEN").map(String::as_str), + Some("{{ env.DEPLOY_TOKEN }}") + ); + } + + #[test] + fn argv_command_elements_are_carried_in_source_form() { + let layer = RunPrepareLayer { + steps: vec![PrepareStep { + script: None, + command: Some(vec![ + InterpString::parse("echo"), + InterpString::parse("hello world"), + InterpString::parse("{{ env.USER_INPUT }}"), + ]), + env: HashMap::new(), + }], + timeout: None, + }; + + let steps = resolve(&layer); + + // Argv elements are carried as separate source strings, NOT joined and + // NOT shell-quoted here. Quoting happens at the run boundary, after + // `{{ env.* }}` resolution, so the resolved value (not the source + // token) is what gets quoted. + assert_eq!(steps[0].run, PreparedStepRun::Command { + command: vec![ + "echo".to_string(), + "hello world".to_string(), + "{{ env.USER_INPUT }}".to_string(), + ], + }); + } + + #[test] + fn script_is_kept_verbatim() { + let layer = RunPrepareLayer { + steps: vec![PrepareStep { + script: Some(InterpString::parse("echo hello && ls -la")), + command: None, + env: HashMap::new(), + }], + timeout: None, + }; + + let steps = resolve(&layer); + + // A script is a raw shell snippet, not an argv: it is carried verbatim. + assert_eq!(steps[0].run, PreparedStepRun::Script { + script: "echo hello && ls -la".to_string(), + }); + } +} diff --git a/lib/crates/fabro-dev/Cargo.toml b/lib/crates/fabro-dev/Cargo.toml index 2c41028eb..7334acbf9 100644 --- a/lib/crates/fabro-dev/Cargo.toml +++ b/lib/crates/fabro-dev/Cargo.toml @@ -32,9 +32,9 @@ chrono.workspace = true clap.workspace = true csv = "1" fabro-config = { path = "../fabro-config" } +fabro-util = { path = "../fabro-util" } fabro-options-metadata.workspace = true quick-xml = "0.36" -shlex = "1" toml_edit.workspace = true tracing-subscriber.workspace = true walkdir.workspace = true diff --git a/lib/crates/fabro-dev/src/commands/mod.rs b/lib/crates/fabro-dev/src/commands/mod.rs index 007e7fc4b..a59142898 100644 --- a/lib/crates/fabro-dev/src/commands/mod.rs +++ b/lib/crates/fabro-dev/src/commands/mod.rs @@ -17,6 +17,7 @@ pub(crate) use bench_tests::{BenchTestsArgs, bench_tests}; pub(crate) use build::{BuildArgs, build}; pub(crate) use docker_build::{DockerBuildArgs, docker_build}; pub(crate) use docs::{DocsArgs, docs}; +use fabro_util::shell::shell_quote; pub(crate) use release::{ReleaseArgs, release}; pub(crate) use spa::{SpaArgs, spa}; @@ -188,11 +189,7 @@ fn is_cargo_build_env(key: &std::ffi::OsStr) -> bool { } pub(crate) fn shell_arg(arg: impl AsRef) -> String { - let arg = arg.as_ref(); - shlex::try_quote(arg).map_or_else( - |_| format!("'{}'", arg.replace('\'', "'\\''")), - std::borrow::Cow::into_owned, - ) + shell_quote(arg.as_ref()) } #[cfg(test)] diff --git a/lib/crates/fabro-sandbox/Cargo.toml b/lib/crates/fabro-sandbox/Cargo.toml index 3ccf6e5e3..dd90d2f0e 100644 --- a/lib/crates/fabro-sandbox/Cargo.toml +++ b/lib/crates/fabro-sandbox/Cargo.toml @@ -38,7 +38,6 @@ fabro-proc = { path = "../fabro-proc" } fabro-static.workspace = true fabro-util = { path = "../fabro-util" } fabro-redact.workspace = true -shlex = "1" # local glob = { version = "0.3" } diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index a1d2da18d..68801f8bb 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -8,6 +8,7 @@ use std::time::Duration; use async_trait::async_trait; use fabro_types::{CommandOutputStream, CommandTermination}; +use fabro_util::shell; use serde::{Deserialize, Serialize}; use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite}; use tokio::sync::Mutex as TokioMutex; @@ -1049,12 +1050,10 @@ pub(crate) fn resolve_path(path: &str, working_dir: &str) -> String { } /// Shell-quote a string using `shlex::try_quote`, with a fallback for edge -/// cases. +/// cases. Re-exported from [`fabro_util::shell::shell_quote`] so sandbox code +/// and the config resolve layer share one audited implementation. pub fn shell_quote(s: &str) -> String { - shlex::try_quote(s).map_or_else( - |_| format!("'{}'", s.replace('\'', "'\\''")), - |q| q.to_string(), - ) + shell::shell_quote(s) } /// Helper for sandbox implementations that manage git internally. diff --git a/lib/crates/fabro-server/src/demo/mod.rs b/lib/crates/fabro-server/src/demo/mod.rs index 080bd8db5..283435092 100644 --- a/lib/crates/fabro-server/src/demo/mod.rs +++ b/lib/crates/fabro-server/src/demo/mod.rs @@ -1088,8 +1088,8 @@ mod runs { use fabro_api::types::*; use fabro_types::settings::run::{ EnvironmentImageSettings, EnvironmentLifecycleSettings, EnvironmentProvider, - EnvironmentResourcesSettings, EnvironmentSettings, RunEnvironmentSettings, RunGoal, - RunModelSettings, RunNamespace, RunPrepareSettings, + EnvironmentResourcesSettings, EnvironmentSettings, PreparedStep, PreparedStepRun, + RunEnvironmentSettings, RunGoal, RunModelSettings, RunNamespace, RunPrepareSettings, }; use fabro_types::settings::{InterpString, ProjectNamespace, WorkflowNamespace}; use fabro_types::{ @@ -1802,7 +1802,20 @@ mod runs { ..RunModelSettings::default() }, prepare: RunPrepareSettings { - commands: vec!["bun install".into(), "bun run typecheck".into()], + steps: vec![ + PreparedStep { + run: PreparedStepRun::Script { + script: "bun install".to_string(), + }, + env: HashMap::new(), + }, + PreparedStep { + run: PreparedStepRun::Script { + script: "bun run typecheck".to_string(), + }, + env: HashMap::new(), + }, + ], timeout_ms: 120_000, }, environment: RunEnvironmentSettings::from_environment( diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index acbdbcbdc..d7794be48 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -566,7 +566,7 @@ async fn build_preflight_report( } fn base_preflight_checks(prepared: &PreparedManifest, graph: &Graph) -> Vec { - let setup_command_count = prepared.settings.run.prepare.commands.len(); + let setup_command_count = prepared.settings.run.prepare.steps.len(); let repo_summary = prepared.git.as_ref().map_or_else( || "unknown".to_string(), |git| { @@ -1940,11 +1940,13 @@ app_id = "fixture-app-id" let prepared = prepare_manifest(&server_settings, &manifest).unwrap(); let settings_json = serde_json::to_value(&prepared.settings).unwrap(); - // v2 merge matrix: run.prepare.steps replaces the whole list across - // layers, so the higher-precedence workflow layer wins over cli. - assert_eq!(prepared.settings.run.prepare.commands, vec![ - "workflow-setup".to_string() - ]); + // run.prepare.steps replaces the whole list across layers, so the + // higher-precedence workflow layer wins over cli. + assert_eq!(prepared.settings.run.prepare.steps.len(), 1); + assert_eq!( + prepared.settings.run.prepare.steps[0].to_shell_command(), + "workflow-setup" + ); assert!(settings_json.pointer("/server").is_none()); } diff --git a/lib/crates/fabro-types/Cargo.toml b/lib/crates/fabro-types/Cargo.toml index b7246024b..483e7d697 100644 --- a/lib/crates/fabro-types/Cargo.toml +++ b/lib/crates/fabro-types/Cargo.toml @@ -34,4 +34,5 @@ url.workspace = true [dev-dependencies] fabro-types = { path = ".", features = ["test-support"] } +shlex = "1" tempfile = "3" diff --git a/lib/crates/fabro-types/src/settings/mod.rs b/lib/crates/fabro-types/src/settings/mod.rs index aa2d7fb3e..872fa3519 100644 --- a/lib/crates/fabro-types/src/settings/mod.rs +++ b/lib/crates/fabro-types/src/settings/mod.rs @@ -38,11 +38,11 @@ pub use run::{ EnvironmentNetworkMode, EnvironmentNetworkSettings, EnvironmentProvider, EnvironmentResourcesSettings, EnvironmentSettings, GitAuthorSettings, HookDefinition, HookType, InterviewProviderSettings, McpServerRef, McpServerSettings, McpTransport, - NotificationProviderSettings, NotificationRouteSettings, PullRequestSettings, ResolvedMcpEntry, - RunAgentSettings, RunCheckpointSettings, RunEnvironmentSettings, RunExecutionSettings, - RunGitSettings, RunGoal, RunIntegrationsGithubSettings, RunIntegrationsSettings, - RunInterviewsSettings, RunModelControls, RunModelSettings, RunNamespace, RunPrepareSettings, - RunScmSettings, ScmGitHubSettings, TlsMode, + NotificationProviderSettings, NotificationRouteSettings, PreparedStep, PullRequestSettings, + ResolvedMcpEntry, RunAgentSettings, RunCheckpointSettings, RunEnvironmentSettings, + RunExecutionSettings, RunGitSettings, RunGoal, RunIntegrationsGithubSettings, + RunIntegrationsSettings, RunInterviewsSettings, RunModelControls, RunModelSettings, + RunNamespace, RunPrepareSettings, RunScmSettings, ScmGitHubSettings, TlsMode, }; pub use server::{ GithubIntegrationSettings, IntegrationWebhooksSettings, LogDestination, ObjectStoreSettings, diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index fe7f99178..efad179c8 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -10,6 +10,7 @@ use std::collections::HashMap; use std::path::PathBuf; use std::time::Duration as StdDuration; +use fabro_util::shell; use serde::de::{self, Deserializer}; use serde::ser::SerializeStruct; use serde::{Deserialize, Serialize}; @@ -104,9 +105,10 @@ impl RunNamespace { substitute_option(&mut slack.channel, &mut lookup)?; } substitute_map(&mut self.integrations.github.permissions, &mut lookup)?; - // run.scm.owner/repository were demoted and removed from this pass - // (D2): values stay literal. - substitute_string_vec(&mut self.prepare.commands, &mut lookup)?; + // run.scm.owner/repository are plain strings: values stay literal. + for step in &mut self.prepare.steps { + visit_prepared_step_strings(step, &mut |value| substitute_string(value, &mut lookup))?; + } // Only resolved inline servers carry substitutable templates; an // unresolved reference holds just an id + enabled flag. for entry in self.agent.mcps.values_mut() { @@ -262,6 +264,28 @@ where } } +/// Walk every interpolatable string in one prepare step — the runnable part +/// (a `script` snippet or each `command` argv element) and every per-step `env` +/// value. Both interpolation passes route through this one traversal so they +/// cannot drift as `PreparedStepRun` or `PreparedStep` grow fields: the +/// `{{ vars.* }}` pass ([`RunNamespace::substitute_variables`]) passes a +/// `substitute_string` visitor, the `{{ env.* }}` pass +/// ([`RunPrepareSettings::resolve_step_env`]) passes a `resolve_env_string` +/// visitor. Mirrors [`visit_mcp_transport_strings`]. +fn visit_prepared_step_strings( + step: &mut PreparedStep, + visitor: &mut F, +) -> Result<(), ResolveError> +where + F: FnMut(&mut String) -> Result<(), ResolveError>, +{ + match &mut step.run { + PreparedStepRun::Script { script } => visitor(script)?, + PreparedStepRun::Command { command } => visit_string_vec(command, visitor)?, + } + visit_string_map(&mut step.env, visitor) +} + fn visit_string_vec(values: &mut [String], visitor: &mut F) -> Result<(), ResolveError> where F: FnMut(&mut String) -> Result<(), ResolveError>, @@ -342,7 +366,7 @@ mod run_namespace_variable_substitution_tests { use super::{ ArtifactsSettings, DockerfileSource, EnvironmentImageSettings, EnvironmentNetworkMode, EnvironmentNetworkSettings, HookDefinition, HookEvent, HookType, InterpString, - McpHttpProtocol, McpServerSettings, McpTransport, RunCheckpointSettings, + McpHttpProtocol, McpServerSettings, McpTransport, PreparedStepRun, RunCheckpointSettings, RunEnvironmentSettings, RunGoal, RunNamespace, RunPrepareSettings, }; @@ -357,7 +381,19 @@ mod run_namespace_variable_substitution_tests { "deploy {{ vars.ENV }} in {{ env.REGION }}", ))), prepare: RunPrepareSettings { - commands: vec!["echo {{ vars.ENV }} {{ env.REGION }}".to_string()], + steps: vec![super::PreparedStep { + run: PreparedStepRun::Command { + command: vec![ + "echo".to_string(), + "{{ vars.ENV }}".to_string(), + "{{ env.REGION }}".to_string(), + ], + }, + env: HashMap::from([( + "STAGE".to_string(), + "{{ vars.ENV }}-{{ env.REGION }}".to_string(), + )]), + }], timeout_ms: 1_000, }, agent: super::RunAgentSettings { @@ -417,9 +453,21 @@ mod run_namespace_variable_substitution_tests { goal_source, Some("deploy prod in {{ env.REGION }}".to_string()) ); - assert_eq!(run.prepare.commands, vec![ - "echo prod {{ env.REGION }}".to_string() + assert_eq!(run.prepare.steps.len(), 1); + // `{{ vars.* }}` substitutes per argv element while `{{ env.* }}` is + // left for the run boundary. + let PreparedStepRun::Command { command } = &run.prepare.steps[0].run else { + panic!("expected command argv prepare step"); + }; + assert_eq!(command.as_slice(), [ + "echo".to_string(), + "prod".to_string(), + "{{ env.REGION }}".to_string(), ]); + assert_eq!( + run.prepare.steps[0].env.get("STAGE").map(String::as_str), + Some("prod-{{ env.REGION }}") + ); let mcp = run.agent.mcps["http"] .as_resolved() .expect("expected resolved inline mcp entry"); @@ -671,19 +719,105 @@ pub struct GitAuthorSettings { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunPrepareSettings { - pub commands: Vec, + pub steps: Vec, pub timeout_ms: u64, } impl Default for RunPrepareSettings { fn default() -> Self { Self { - commands: Vec::new(), + steps: Vec::new(), timeout_ms: 300_000, } } } +impl RunPrepareSettings { + /// Resolve `{{ env.* }}` tokens in every prepare step's runnable part and + /// per-step `env` values against `env_lookup`, returning a copy with the + /// tokens replaced and every other field preserved. A `script` step's + /// snippet resolves in place; a `command` step's argv resolves per element + /// (each element is shell-quoted later, in + /// [`PreparedStep::to_shell_command`], so quoting applies to the resolved + /// value rather than the source token). + /// + /// This is the late, use-time half of prepare-step interpolation, the + /// counterpart to the server-side `{{ vars.* }}` substitution in + /// [`RunNamespace::substitute_variables`]: `{{ vars.* }}` are substituted + /// earlier, server-side, while `{{ env.* }}` resolve here — in whichever + /// process actually runs the steps (the run worker for `fabro run`). + /// Carrying the source form out of the config resolve layer keeps + /// `fabro validate` portable (it never requires env to be set). + /// + /// A referenced env var that is unset is a hard error — no fallback to the + /// unresolved source. Reserved `secrets`/`inputs` tokens have no lookup + /// here and surface as a loud + /// [`super::interp::ResolveErrorKind::Unavailable`] error rather than + /// passing through as literal text. + pub fn resolve_step_env( + &self, + mut env_lookup: impl FnMut(&str) -> Option, + ) -> Result { + let mut resolved = self.clone(); + for step in &mut resolved.steps { + visit_prepared_step_strings(step, &mut |value| { + resolve_env_string(value, &mut env_lookup) + })?; + } + Ok(resolved) + } +} + +/// A single resolved prepare step: the thing to run plus the per-step +/// environment variables it should see. The runnable part keeps the +/// script-vs-argv distinction (see [`PreparedStepRun`]), and every string is +/// carried in source form out of the config resolve layer; their `{{ env.* }}` +/// tokens resolve at the run boundary via +/// [`RunPrepareSettings::resolve_step_env`]. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct PreparedStep { + #[serde(flatten)] + pub run: PreparedStepRun, + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub env: HashMap, +} + +/// The runnable part of a prepare step, preserving the script-vs-argv +/// distinction so each is treated correctly when assembled into the shell +/// command that runs via `bash -c`: +/// +/// - [`Script`](PreparedStepRun::Script) is a raw shell snippet kept verbatim +/// for the shell to interpret. +/// - [`Command`](PreparedStepRun::Command) is an argv: a vector of element +/// source strings, neither pre-joined nor shell-quoted at config time. Its +/// `{{ env.* }}` tokens resolve per element at the run boundary, and only +/// then is each *resolved* element shell-quoted and joined. Resolving before +/// quoting is what stops an interpolated env value from breaking out of its +/// argument and injecting shell syntax. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(tag = "type", rename_all = "snake_case")] +pub enum PreparedStepRun { + Script { script: String }, + Command { command: Vec }, +} + +impl PreparedStep { + /// Flatten this step's runnable part into the single shell string that runs + /// via `bash -c`. + /// + /// For a script, the snippet is returned verbatim. For an argv `command`, + /// each element is shell-quoted and joined with spaces so an argument that + /// contains spaces or shell metacharacters survives as a single token. This + /// must run *after* [`RunPrepareSettings::resolve_step_env`] so the quoting + /// applies to the resolved values, not the `{{ env.* }}` source. + pub fn to_shell_command(&self) -> String { + match &self.run { + PreparedStepRun::Script { script } => script.clone(), + PreparedStepRun::Command { command } => shell::shell_join(command), + } + } +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunExecutionSettings { pub mode: RunMode, @@ -1556,6 +1690,213 @@ mod resolve_transport_env_tests { } } +#[cfg(test)] +mod resolve_step_env_tests { + use std::collections::HashMap; + + use super::super::interp::ResolveErrorKind; + use super::{Namespace, PreparedStep, PreparedStepRun, RunPrepareSettings}; + + fn env_lookup( + pairs: &'static [(&'static str, &'static str)], + ) -> impl Fn(&str) -> Option + Copy { + move |name| { + pairs + .iter() + .find_map(|(key, value)| (*key == name).then(|| (*value).to_string())) + } + } + + fn script_step(script: &str, env: HashMap) -> PreparedStep { + PreparedStep { + run: PreparedStepRun::Script { + script: script.to_string(), + }, + env, + } + } + + fn command_step(argv: &[&str], env: HashMap) -> PreparedStep { + PreparedStep { + run: PreparedStepRun::Command { + command: argv.iter().map(|element| (*element).to_string()).collect(), + }, + env, + } + } + + #[test] + fn literal_step_passes_through() { + let settings = RunPrepareSettings { + steps: vec![script_step( + "echo hello", + HashMap::from([("STAGE".to_string(), "build".to_string())]), + )], + timeout_ms: 1_000, + }; + + let resolved = settings.resolve_step_env(env_lookup(&[])).unwrap(); + + assert_eq!(resolved.steps[0].to_shell_command(), "echo hello"); + assert_eq!( + resolved.steps[0].env.get("STAGE").map(String::as_str), + Some("build") + ); + } + + #[test] + fn script_resolves_verbatim() { + // A script is a raw shell snippet: its `{{ env.* }}` token resolves but + // the result is NOT shell-quoted — the shell interprets the snippet as + // written. + let settings = RunPrepareSettings { + steps: vec![script_step( + "deploy {{ env.REGION }} && echo done", + HashMap::new(), + )], + timeout_ms: 1_000, + }; + + let resolved = settings + .resolve_step_env(env_lookup(&[("REGION", "us-east-1")])) + .unwrap(); + + assert_eq!( + resolved.steps[0].to_shell_command(), + "deploy us-east-1 && echo done" + ); + } + + #[test] + fn command_and_env_resolve() { + let settings = RunPrepareSettings { + steps: vec![command_step( + &["deploy", "{{ env.REGION }}"], + HashMap::from([("TOKEN".to_string(), "{{ env.DEPLOY_TOKEN }}".to_string())]), + )], + timeout_ms: 1_000, + }; + + let resolved = settings + .resolve_step_env(env_lookup(&[ + ("REGION", "us-east-1"), + ("DEPLOY_TOKEN", "secret-token"), + ])) + .unwrap(); + + assert_eq!(resolved.steps[0].to_shell_command(), "deploy us-east-1"); + assert_eq!( + resolved.steps[0].env.get("TOKEN").map(String::as_str), + Some("secret-token") + ); + } + + #[test] + fn command_arg_with_space_stays_one_token() { + // A resolved argv element that contains a space must survive as a + // single shell word, not re-split into two. + let settings = RunPrepareSettings { + steps: vec![command_step(&["echo", "{{ env.MESSAGE }}"], HashMap::new())], + timeout_ms: 1_000, + }; + + let resolved = settings + .resolve_step_env(env_lookup(&[("MESSAGE", "hello world")])) + .unwrap(); + + let shell = resolved.steps[0].to_shell_command(); + let tokens = shlex::split(&shell).expect("resolved command should be valid shell"); + assert_eq!(tokens, vec!["echo".to_string(), "hello world".to_string()]); + } + + #[test] + fn command_arg_resolving_to_shell_metacharacters_is_not_injected() { + // Regression test for the command-injection defect: an `{{ env.* }}` + // value containing a single quote and `;` must be resolved THEN quoted + // so it stays a single argument and cannot break out to inject extra + // shell commands. Quoting the source token *before* resolving (the old + // behavior) lets the substituted value escape its quotes. + let malicious = "x'; touch PWNED; echo '"; + let settings = RunPrepareSettings { + steps: vec![command_step( + &["echo", "{{ env.USER_INPUT }}"], + HashMap::new(), + )], + timeout_ms: 1_000, + }; + + let resolved = settings + .resolve_step_env(|name| (name == "USER_INPUT").then(|| malicious.to_string())) + .unwrap(); + + let shell = resolved.steps[0].to_shell_command(); + // The flattened shell string round-trips to EXACTLY two tokens: the + // command and the verbatim payload as a single argument. Pre-fix, the + // value was substituted raw inside config-time quotes + // (`echo 'x'; touch PWNED; echo ''`), which `shlex::split` parses as + // several tokens / an injected `touch PWNED` command — so the round-trip + // equality below fails on the buggy code and passes once the value is + // resolved THEN quoted. + let tokens = shlex::split(&shell).expect("resolved command should be valid shell"); + assert_eq!(tokens, vec!["echo".to_string(), malicious.to_string()]); + assert_eq!( + tokens.len(), + 2, + "injected shell syntax leaked extra tokens: {shell}" + ); + } + + #[test] + fn missing_env_in_command_is_hard_error() { + let settings = RunPrepareSettings { + steps: vec![command_step( + &["deploy", "{{ env.REGION }}"], + HashMap::new(), + )], + timeout_ms: 1_000, + }; + + let err = settings.resolve_step_env(env_lookup(&[])).unwrap_err(); + + assert_eq!(err.namespace, Namespace::Env); + assert_eq!(err.name, "REGION"); + assert_eq!(err.kind, ResolveErrorKind::Missing); + } + + #[test] + fn missing_env_in_step_env_value_is_hard_error() { + let settings = RunPrepareSettings { + steps: vec![script_step( + "echo hi", + HashMap::from([("TOKEN".to_string(), "{{ env.DEPLOY_TOKEN }}".to_string())]), + )], + timeout_ms: 1_000, + }; + + let err = settings.resolve_step_env(env_lookup(&[])).unwrap_err(); + + assert_eq!(err.namespace, Namespace::Env); + assert_eq!(err.name, "DEPLOY_TOKEN"); + assert_eq!(err.kind, ResolveErrorKind::Missing); + } + + #[test] + fn reserved_secret_token_is_unavailable_not_leaked() { + let settings = RunPrepareSettings { + steps: vec![script_step( + "echo hi", + HashMap::from([("API_KEY".to_string(), "{{ secrets.API_KEY }}".to_string())]), + )], + timeout_ms: 1_000, + }; + + let err = settings.resolve_step_env(env_lookup(&[])).unwrap_err(); + + assert_eq!(err.namespace, Namespace::Secrets); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); + } +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "snake_case", deny_unknown_fields)] pub enum McpTransport { diff --git a/lib/crates/fabro-util/Cargo.toml b/lib/crates/fabro-util/Cargo.toml index 7c98d9fb0..67372946c 100644 --- a/lib/crates/fabro-util/Cargo.toml +++ b/lib/crates/fabro-util/Cargo.toml @@ -16,6 +16,7 @@ workspace = true console.workspace = true fabro-static.workspace = true rand.workspace = true +shlex = "1" termimad.workspace = true serde_json.workspace = true serde.workspace = true diff --git a/lib/crates/fabro-util/src/lib.rs b/lib/crates/fabro-util/src/lib.rs index 5993f2e39..26adaeec8 100644 --- a/lib/crates/fabro-util/src/lib.rs +++ b/lib/crates/fabro-util/src/lib.rs @@ -11,6 +11,7 @@ pub mod path; pub mod printer; pub mod run_log; pub mod session_secret; +pub mod shell; pub mod terminal; pub mod text; pub mod time; diff --git a/lib/crates/fabro-util/src/shell.rs b/lib/crates/fabro-util/src/shell.rs new file mode 100644 index 000000000..de13ce25f --- /dev/null +++ b/lib/crates/fabro-util/src/shell.rs @@ -0,0 +1,65 @@ +//! Shell-quoting helpers. +//! +//! A single audited place that turns an arbitrary string into a token that is +//! safe to interpolate into a `/bin/sh` command line. Use this anywhere a +//! user-controlled value (path, branch name, URL, env var, glob, argv element) +//! is assembled into a shell script — never hand-roll the escaping. + +/// Shell-quote a string using `shlex::try_quote`, with a fallback for edge +/// cases (`shlex` rejects strings containing a NUL byte, which can never appear +/// in a real argv anyway, so the fallback simply single-quotes defensively). +#[must_use] +pub fn shell_quote(s: &str) -> String { + shlex::try_quote(s).map_or_else( + |_| format!("'{}'", s.replace('\'', "'\\''")), + std::borrow::Cow::into_owned, + ) +} + +/// Shell-quote each element of an argv and join them with spaces into a single +/// `/bin/sh` command line. Quoting is applied per element via [`shell_quote`], +/// so an argument containing spaces or shell metacharacters survives as one +/// shell word instead of being re-split. Use this for any `program + args` +/// vector that is assembled into a shell script. +#[must_use] +pub fn shell_join(parts: impl IntoIterator>) -> String { + parts + .into_iter() + .map(|part| shell_quote(part.as_ref())) + .collect::>() + .join(" ") +} + +#[cfg(test)] +mod tests { + use super::{shell_join, shell_quote}; + + #[test] + fn plain_word_is_unquoted() { + assert_eq!(shell_quote("setup"), "setup"); + } + + #[test] + fn spaces_are_quoted() { + assert_eq!(shell_quote("hello world"), "'hello world'"); + } + + #[test] + fn single_quotes_are_escaped() { + // shlex double-quotes a token that contains a single quote. + assert_eq!(shell_quote("it's"), r#""it's""#); + } + + #[test] + fn join_quotes_each_element() { + assert_eq!( + shell_join(["echo", "hello world", "a;b"]), + r"echo 'hello world' 'a;b'" + ); + } + + #[test] + fn join_empty_is_empty() { + assert_eq!(shell_join(Vec::::new()), ""); + } +} diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index 48306375b..0389c7e99 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -63,7 +63,6 @@ hex.workspace = true sha2 = { workspace = true } mime_guess.workspace = true miette.workspace = true -shlex = "1" git2.workspace = true tokio-util.workspace = true tracing.workspace = true diff --git a/lib/crates/fabro-workflow/src/handler/command.rs b/lib/crates/fabro-workflow/src/handler/command.rs index 66838ae63..146324013 100644 --- a/lib/crates/fabro-workflow/src/handler/command.rs +++ b/lib/crates/fabro-workflow/src/handler/command.rs @@ -4,6 +4,7 @@ use async_trait::async_trait; use fabro_agent::CommandOutputCallback; use fabro_graphviz::graph::{Graph, Node}; use fabro_types::{CommandTermination, StageTiming}; +use fabro_util::shell::shell_quote; use super::{EngineServices, Handler, NodeTimeoutPolicy}; use crate::command_log::CommandLogRecorder; @@ -16,14 +17,6 @@ fn timeout_ms(node: &Node) -> Option { node.timeout().map(crate::millis_u64) } -/// Shell-escape a string using `shlex::try_quote` (POSIX-safe). -fn shell_quote(s: &str) -> String { - shlex::try_quote(s).map_or_else( - |_| format!("'{}'", s.replace('\'', "'\\''")), - |q| q.to_string(), - ) -} - /// Executes an external script configured via node attributes. pub struct CommandHandler; diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index e92e2ac0e..f24ec8d43 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -18,7 +18,7 @@ use fabro_static::EnvVars; use fabro_types::settings::run::{ ApprovalMode, McpServerSettings as ResolvedMcpServerSettings, PullRequestSettings, ResolvedMcpEntry, RunMode, RunModelSettings as ResolvedRunModelSettings, - RunNamespace as ResolvedRunSettings, + RunNamespace as ResolvedRunSettings, RunPrepareSettings as ResolvedRunPrepareSettings, }; use fabro_types::settings::{ModelRegistry, ResolvedModelRef}; use fabro_types::{ManifestPath, RunId, RunRunnableSource, SandboxProviderKind}; @@ -44,7 +44,7 @@ use crate::pipeline::{ use crate::records::Checkpoint; use crate::run_control::RunControlState; use crate::run_metadata::metadata_branch_name; -use crate::run_options::{GitCheckpointOptions, LifecycleOptions, RunOptions}; +use crate::run_options::{GitCheckpointOptions, LifecycleOptions, RunOptions, SetupCommand}; use crate::run_status::{FailureReason, RunStatus}; use crate::runtime_store::RunStoreHandle; use crate::services::FabroRunToolServices; @@ -471,7 +471,7 @@ impl RunSession { steering_hub: services.steering_hub, on_node: services.on_node, lifecycle: LifecycleOptions { - setup_commands: resolved.prepare.commands.clone(), + setup_commands: runtime_setup_commands(&resolved.prepare)?, setup_command_timeout_ms: resolved.prepare.timeout_ms, }, hooks: fabro_hooks::HookSettings { @@ -703,6 +703,38 @@ fn runtime_mcp_server( }) } +/// Build the launch-time setup (prepare) commands from resolved settings, +/// resolving any `{{ env.* }}` tokens in each step's command and per-step env +/// against the worker process environment — the run boundary where the steps +/// actually run. +/// +/// The resolution itself lives on the type +/// ([`ResolvedRunPrepareSettings::resolve_step_env`]) so prepare-step env +/// resolution shares one resolver with the rest of the run-boundary +/// interpolation. Prepare-step commands and env are carried in source form out +/// of the config resolve layer so `fabro validate` stays portable (it never +/// requires env to be set), and a referenced env var that is unset is a hard +/// error — no fallback to the unresolved source. +fn runtime_setup_commands( + prepare: &ResolvedRunPrepareSettings, +) -> Result, Error> { + let resolved = prepare + .resolve_step_env(process_env_var) + .map_err(|err| Error::engine_with_source("failed to resolve prepare step", err))?; + Ok(resolved + .steps + .into_iter() + .map(|step| SetupCommand { + // Flatten the runnable part into the shell string AFTER env + // resolution: an argv `command` is shell-quoted per resolved + // element here so an interpolated value stays a single token; a + // `script` is kept verbatim. + command: step.to_shell_command(), + env: step.env, + }) + .collect()) +} + impl RunSession { /// Shared engine: initialize, execute, finalize, pull_request. async fn run( diff --git a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs index a1ee4e9f9..e2b96ed37 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs @@ -33,7 +33,7 @@ use crate::outcome::{Outcome, OutcomeExt, StageOutcome}; use crate::pipeline::initialize; use crate::pipeline::types::{InitOptions, LlmSpec, Persisted, SandboxEnvSpec}; use crate::records::RunSpec; -use crate::run_options::{GitCheckpointOptions, LifecycleOptions, RunOptions}; +use crate::run_options::{GitCheckpointOptions, LifecycleOptions, RunOptions, SetupCommand}; use crate::test_support::run_graph; fn local_env() -> Arc { @@ -175,9 +175,15 @@ fn persisted_workflow(graph: Graph, source: String, run_dir: &Path, run_id: RunI ) } -fn test_lifecycle(setup_commands: Vec) -> LifecycleOptions { +fn test_lifecycle(setup_commands: Vec<&str>) -> LifecycleOptions { LifecycleOptions { - setup_commands, + setup_commands: setup_commands + .into_iter() + .map(|command| SetupCommand { + command: command.to_string(), + env: std::collections::HashMap::new(), + }) + .collect(), setup_command_timeout_ms: 300_000, } } @@ -1202,7 +1208,7 @@ async fn run_with_lifecycle_emits_initialize_and_setup_events() { local_env(), &simple_graph(), test_run_options(dir.path(), "order-test"), - test_lifecycle(vec!["echo ok".to_string()]), + test_lifecycle(vec!["echo ok"]), ) .await .unwrap(); diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index 2bfc711f4..54a6f1f45 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -525,19 +525,21 @@ pub async fn initialize( command_count: options.lifecycle.setup_commands.len(), }); let setup_start = Instant::now(); - for (index, command) in options.lifecycle.setup_commands.iter().enumerate() { + for (index, setup) in options.lifecycle.setup_commands.iter().enumerate() { + let command = &setup.command; options.emitter.emit(&Event::SetupCommandStarted { command: command.clone(), index, }); let cmd_start = Instant::now(); let cancel_token = options.run_options.cancel_token.child_token(); + let step_env = (!setup.env.is_empty()).then_some(&setup.env); let result = sandbox .exec_command( command, options.lifecycle.setup_command_timeout_ms, None, - None, + step_env, Some(cancel_token.clone()), ) .await @@ -665,6 +667,13 @@ mod tests { fixtures::RUN_1 } + fn setup_cmd(command: &str) -> crate::run_options::SetupCommand { + crate::run_options::SetupCommand { + command: command.to_string(), + env: HashMap::new(), + } + } + fn test_catalog() -> Arc { Arc::new(Catalog::from_builtin().expect("default catalog should build")) } @@ -783,6 +792,12 @@ mod tests { async fn initialize_with_setup_command( command: &str, + ) -> (crate::error::Result, Vec) { + initialize_with_setup_step(setup_cmd(command)).await + } + + async fn initialize_with_setup_step( + setup: crate::run_options::SetupCommand, ) -> (crate::error::Result, Vec) { let temp = tempfile::tempdir().unwrap(); let run_dir = temp.path().join("run"); @@ -820,7 +835,7 @@ mod tests { steering_hub: Arc::new(crate::steering_hub::SteeringHub::new(emitter.clone())), catalog: test_catalog(), lifecycle: crate::run_options::LifecycleOptions { - setup_commands: vec![command.to_string()], + setup_commands: vec![setup], setup_command_timeout_ms: 1_000, }, run_options: test_settings(&run_dir), @@ -1196,7 +1211,7 @@ mod tests { steering_hub: Arc::new(crate::steering_hub::SteeringHub::new(emitter.clone())), catalog: test_catalog(), lifecycle: crate::run_options::LifecycleOptions { - setup_commands: vec!["true".to_string()], + setup_commands: vec![setup_cmd("true")], setup_command_timeout_ms: 1_000, }, run_options: test_settings(&run_dir), @@ -1230,6 +1245,35 @@ mod tests { ); } + #[tokio::test] + async fn initialize_passes_per_step_env_to_setup_command() { + // The command only succeeds when the per-step env var is visible to the + // shell, so a green run proves the env reached `exec_command`. + let setup = crate::run_options::SetupCommand { + command: "test \"$PREPARE_STAGE\" = build".to_string(), + env: HashMap::from([("PREPARE_STAGE".to_string(), "build".to_string())]), + }; + + let (result, events) = initialize_with_setup_step(setup).await; + + assert!(result.is_ok(), "setup with per-step env should succeed"); + assert!( + events + .iter() + .any(|event| event.event_name() == "setup.completed") + ); + } + + #[tokio::test] + async fn initialize_setup_command_without_step_env_does_not_see_it() { + // Negative control: the same command without the per-step env fails, + // confirming the success above is attributable to the per-step env. + let (result, _events) = + initialize_with_setup_command("test \"$PREPARE_STAGE\" = build").await; + + assert!(result.is_err(), "setup should fail without per-step env"); + } + #[tokio::test] async fn initialize_setup_failure_preserves_stderr_and_adds_exec_tail() { let (result, events) = @@ -1310,7 +1354,7 @@ mod tests { steering_hub: Arc::new(crate::steering_hub::SteeringHub::new(emitter.clone())), catalog: test_catalog(), lifecycle: crate::run_options::LifecycleOptions { - setup_commands: vec!["sleep 5".to_string()], + setup_commands: vec![setup_cmd("sleep 5")], setup_command_timeout_ms: 5_000, }, run_options, diff --git a/lib/crates/fabro-workflow/src/run_options.rs b/lib/crates/fabro-workflow/src/run_options.rs index 23282d644..e3817e5f0 100644 --- a/lib/crates/fabro-workflow/src/run_options.rs +++ b/lib/crates/fabro-workflow/src/run_options.rs @@ -74,8 +74,18 @@ impl RunOptions { /// Options for sandbox lifecycle management within the engine. pub struct LifecycleOptions { - /// Setup commands to run inside the sandbox after initialization. - pub setup_commands: Vec, + /// Setup commands to run inside the sandbox after initialization, each with + /// its own environment. + pub setup_commands: Vec, /// Timeout in milliseconds for each setup command. pub setup_command_timeout_ms: u64, } + +/// A single setup (prepare) command and the per-step environment it runs with. +/// Both the command string and the env values are already fully resolved (their +/// `{{ env.* }}` tokens replaced at the run boundary) by the time they reach +/// the sandbox. +pub struct SetupCommand { + pub command: String, + pub env: std::collections::HashMap, +} diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index e9a380137..6c4bc5e4e 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -280,6 +280,9 @@ models/preflight-check-result.ts models/preflight-check-section.ts models/preflight-response.ts models/preflight-workflow-summary.ts +models/prepared-command-step.ts +models/prepared-script-step.ts +models/prepared-step.ts models/preview-url-request.ts models/preview-url-response.ts models/principal-agent.ts diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index 40cf68be5..7b51ee78c 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -250,6 +250,9 @@ export * from './preflight-check-result'; export * from './preflight-check-section'; export * from './preflight-response'; export * from './preflight-workflow-summary'; +export * from './prepared-command-step'; +export * from './prepared-script-step'; +export * from './prepared-step'; export * from './preview-url-request'; export * from './preview-url-response'; export * from './principal'; diff --git a/lib/packages/fabro-api-client/src/models/prepared-command-step.ts b/lib/packages/fabro-api-client/src/models/prepared-command-step.ts new file mode 100644 index 000000000..a707858dc --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/prepared-command-step.ts @@ -0,0 +1,27 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +export interface PreparedCommandStep { + 'type': PreparedCommandStepTypeEnum; + 'command': Array; + 'env'?: { [key: string]: string; }; +} + +export const PreparedCommandStepTypeEnum = { + COMMAND: 'command' +} as const; + +export type PreparedCommandStepTypeEnum = typeof PreparedCommandStepTypeEnum[keyof typeof PreparedCommandStepTypeEnum]; diff --git a/lib/packages/fabro-api-client/src/models/prepared-script-step.ts b/lib/packages/fabro-api-client/src/models/prepared-script-step.ts new file mode 100644 index 000000000..16a88d436 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/prepared-script-step.ts @@ -0,0 +1,27 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + + +export interface PreparedScriptStep { + 'type': PreparedScriptStepTypeEnum; + 'script': string; + 'env'?: { [key: string]: string; }; +} + +export const PreparedScriptStepTypeEnum = { + SCRIPT: 'script' +} as const; + +export type PreparedScriptStepTypeEnum = typeof PreparedScriptStepTypeEnum[keyof typeof PreparedScriptStepTypeEnum]; diff --git a/lib/packages/fabro-api-client/src/models/prepared-step.ts b/lib/packages/fabro-api-client/src/models/prepared-step.ts new file mode 100644 index 000000000..24daf4f94 --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/prepared-step.ts @@ -0,0 +1,27 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + +// May contain unused imports in some cases +// @ts-ignore +import type { PreparedCommandStep } from './prepared-command-step'; +// May contain unused imports in some cases +// @ts-ignore +import type { PreparedScriptStep } from './prepared-script-step'; + +/** + * @type PreparedStep + * A single resolved prepare step. The runnable part preserves the script-vs-argv distinction via the `type` discriminator: a `script` is a raw shell snippet kept verbatim, while a `command` is an argv whose elements are shell-quoted and joined at the run boundary (after `{{ env.* }}` resolution) so an interpolated value cannot inject shell syntax. Optional per-step `env` is shared by both shapes. + */ +export type PreparedStep = { type: 'command' } & PreparedCommandStep | { type: 'script' } & PreparedScriptStep; diff --git a/lib/packages/fabro-api-client/src/models/run-prepare-settings.ts b/lib/packages/fabro-api-client/src/models/run-prepare-settings.ts index d1f3a1911..48a4b75fa 100644 --- a/lib/packages/fabro-api-client/src/models/run-prepare-settings.ts +++ b/lib/packages/fabro-api-client/src/models/run-prepare-settings.ts @@ -13,8 +13,11 @@ */ +// May contain unused imports in some cases +// @ts-ignore +import type { PreparedStep } from './prepared-step'; export interface RunPrepareSettings { - 'commands': Array; + 'steps': Array; 'timeout_ms': number; }