diff --git a/docs/public/changelog/2026-08-31.mdx b/docs/public/changelog/2026-08-31.mdx new file mode 100644 index 000000000..fbfeb4e0d --- /dev/null +++ b/docs/public/changelog/2026-08-31.mdx @@ -0,0 +1,38 @@ +--- +title: "Immutable workflow versions for CLI runs" +date: "2026-08-31" +--- + +`fabro run` and `fabro create` now resolve and package local workflows, +register their immutable workflow versions and dependencies, and create runs +by intent. Source parsing or packaging failures remain local and stop before +registration, while full effective-intent validation is authoritative at +server admission. Use `fabro preflight` for explicit local validation without +creating a run. `fabro create` leaves the run submitted; `fabro run` starts it +with a separate request. + +Workflows can still be selected by project name, from user workflow storage, +from another local checkout, or as loose local files. The workflow source is +independent from the execution target: local environments use the directory +where Fabro was invoked, while Docker and Daytona derive a GitHub target from +that directory. Clone-based runs invoked from a Git checkout require a +non-bare checkout with an attached branch and at least one commit; detached +HEAD and unborn checkouts fail before workflow registration instead of being +treated as empty workspaces. A directory outside Git still selects an empty +workspace. Remote Git workflow acquisition is not included. + +CLI input overrides retain string, Boolean, integer, and finite-float types, +and Boolean flags remain sparse. `--goal-file` is read locally and sent by +value as a per-run override; goal files referenced by `workflow.toml` remain +part of the immutable workflow. + +Project and CLI-machine `[run]` and `[environments]` settings are no longer +transmitted during intent creation. Fabro warns with only the affected file and +key names. Move workflow behavior—including `[run.pull_request]`—to +`workflow.toml`, and configure placement with server-managed environments. +Malformed or unreadable active configuration still fails before creation. + +New repositories now place their default automatic pull-request policy in the +starter workflow rather than `.fabro/project.toml`. Intent-created runs also +derive their display slug from the immutable workflow entrypoint. These +changes require no API schema update. diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index 1c848b28e..7db919dba 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -3,12 +3,32 @@ title: "Run Configuration" description: "Configure workflow runs with TOML files" --- -A run config is a TOML file that bundles a workflow graph with all the settings needed to execute it — the goal, model, sandbox, prepare steps, inputs, and hooks. Instead of passing a dozen CLI flags, you check a `.toml` file into version control and launch with a single command: +A workflow config is a TOML file that bundles a workflow graph with its +execution behavior — the goal, model, prepare steps, inputs, hooks, and other +workflow-owned settings. Instead of passing a dozen CLI flags, check +`workflow.toml` into version control and launch it with a single command: ```bash fabro run run.toml ``` +`fabro run` and `fabro create` resolve and package the workflow locally, +register its immutable workflow version and dependencies, and then ask the +server to admit the intent and create a run from that version. Source parsing +or packaging failures stop locally before registration; full effective-intent +validation is authoritative at server admission. Use `fabro preflight` for +explicit local validation without creating a run. `fabro create` stops with +the run in the submitted state; `fabro run` performs the same create operation +and then starts the run separately. + +The workflow can be selected by name from the current project or user workflow +storage, by a path in another local checkout, or as a loose local file. Its +source location does not choose the execution workspace: the directory where +you invoke Fabro remains the target source. Clone-based environments derive a +GitHub target from that caller directory, while a local environment receives +the canonical caller directory directly. Fetching workflow definitions from a +remote Git URL is not part of these commands. + ## Minimal example A run config needs at minimum a schema version and a goal: @@ -29,7 +49,10 @@ goal = "Implement the login feature" | `[workflow].graph` | No | Path to the Graphviz workflow file, relative to the TOML file's directory. Defaults to `workflow.fabro`. | | `[run].goal` | No | What the workflow should accomplish. Passed to agents and available via `--goal` CLI flag or Graphviz graph `goal` attribute. | -Goal precedence: CLI `--goal` > `[run].goal` > Graphviz graph attribute. +Goal precedence: CLI `--goal` or `--goal-file` > `[run].goal` > Graphviz graph +attribute. A CLI `--goal-file` is read on the invoking machine and sent as a +per-run value; a goal file referenced by `workflow.toml` remains immutable +workflow content. ## Full example @@ -58,32 +81,6 @@ script = "git clone https://github.com/fabro-sh/fabro repo" [[run.prepare.steps]] script = "cd repo && npm install" -[run.environment] -id = "cloud" - -[environments.cloud] -provider = "daytona" - -[environments.cloud.lifecycle] -preserve = false -auto_stop = "60m" - -[environments.cloud.labels] -project = "fabro" -env = "ci" - -[environments.cloud.image] -dockerfile = "FROM node:20-slim\nRUN apt-get update && apt-get install -y git" - -[environments.cloud.resources] -cpu = 4 -memory = "8GB" -disk = "20GB" - -[environments.cloud.env] -API_KEY = "{{ secrets.MY_API_KEY }}" -NODE_ENV = "production" - [run.integrations.github.permissions] contents = "write" pull_requests = "write" @@ -281,15 +278,16 @@ push = true | `enabled` | When `false`, Fabro skips metadata branch snapshots. | | `push` | When `false`, Fabro writes metadata snapshots locally but does not push `fabro/meta/` to the remote. | -### `[run.environment]` and `[environments.]` +### `[run.environment]` and server-managed environments -Runs select a reusable named environment by slug. Environment catalogs can be -defined in `settings.toml`, `.fabro/project.toml`, or `workflow.toml`. - -```toml title="run.toml" -[run.environment] -id = "ci" +Runs select a reusable server-managed environment by slug. For `fabro run` and +`fabro create`, use `--environment ` to select it; omitting the flag +selects `default`. Configure the catalog on the server rather than relying on +the CLI machine's `settings.toml` or the source checkout's +`.fabro/project.toml`, because those `environments` tables are not transmitted +during intent creation. +```toml title="server settings.toml" [environments.ci] provider = "docker" # local | docker | daytona @@ -308,8 +306,8 @@ stop_on_terminal = true NODE_ENV = "production" ``` -Sparse run-level overrides live under `[run.environment.*]` and apply to the -selected environment only: +Workflow-owned sparse overrides can live under `[run.environment.*]` and apply +to the selected server environment: ```toml [run.environment.resources] @@ -367,7 +365,9 @@ issues = "read" Only requested permissions are included. The upper bound is the permission set granted to the installed GitHub App, and Fabro logs a warning and continues without `GITHUB_TOKEN` if the app is not configured or is not installed on the repository. -This table follows the normal settings precedence order. A higher-precedence layer can set `permissions = {}` to clear inherited permissions and run without a GitHub token. +This table follows the workflow settings merge rules. A higher-precedence +workflow or CLI override can set `permissions = {}` to clear inherited +permissions and run without a GitHub token. ### `[run.integrations.github].additional_repositories` @@ -619,15 +619,15 @@ Absolute paths are used as-is. ## Precedence -Settings can come from multiple sources. Fabro resolves them in this order (first match wins): +For runs created by `fabro run` and `fabro create`, the CLI transmits sparse +flags and immutable workflow content, not machine or project run defaults. +Fabro resolves workflow behavior in this order (first match wins): | Source | Priority | |---|---| | Node-level [stylesheet](/workflows/stylesheets) | Highest | | CLI flags (`--model`, `--provider`, `--environment`) | | | Run config TOML (`workflow.toml` or equivalent) | | -| Project defaults (`.fabro/project.toml`) | | -| Machine defaults (`~/.fabro/settings.toml`) | | | Graphviz graph attributes (`default_model`, `default_provider`) | | | Built-in defaults | Lowest | @@ -635,31 +635,32 @@ Settings can come from multiple sources. Fabro resolves them in this order (firs Stylesheet rules on individual nodes always take priority over run config values. -### Project defaults (`.fabro/project.toml`) +### Project and machine settings -The `.fabro/project.toml` project config can set default values for any of the `[run.*]` sections described above. These defaults apply to all runs in the project unless the workflow config overrides them: +`fabro run` and `fabro create` do not transmit `[run]` or `[environments]` +from `.fabro/project.toml` or the CLI machine's `~/.fabro/settings.toml`. +When either key is present, the CLI warns with the affected file and key names, +but never includes the values in the warning or request. Move workflow-owned +behavior into each `workflow.toml`, and configure placement in server-managed +environments. -```toml title=".fabro/project.toml" +In particular, automatic pull-request behavior for CLI-created runs belongs in +the workflow: + +```toml title="workflow.toml" _version = 1 -[run.model] -name = "claude-sonnet-4-5" +[workflow] +graph = "workflow.fabro" -[run.environment] -id = "cloud" - -[environments.cloud] -provider = "daytona" - -[environments.cloud.image] -dockerfile = { path = "Dockerfile" } +[run.pull_request] +enabled = true +draft = false ``` -Project defaults and workflow config values merge per the normative merge matrix: most fields merge by field (higher-precedence wins per key), TOML `run.inputs` tables replace wholesale, CLI input flags merge per key at highest precedence, environment `env` and `labels` merge by key, and `run.prepare.steps` replaces whole-list. - -### Machine defaults - -When running locally, the machine defaults at `~/.fabro/settings.toml` can set run-scoped defaults too. Same merge rules apply. +There is no compatibility field in the create request and no global +pull-request default supplied by the CLI. A server may still apply its own +active configuration independently; the warning does not claim otherwise. ## Validation @@ -669,6 +670,10 @@ Fabro validates the run config when it loads: - **Unknown keys** — Any top-level key not in `[project]`, `[workflow]`, `[run]`, `[cli]`, `[server]`, or `_version` is rejected with a targeted rename hint pointing at the v2 replacement path. - **Variable check** — Undefined workflow or prompt template variables produce diagnostics. `fabro validate` reports them as warnings; run-style commands treat them as errors before creating or starting a run. +The CLI also continues to parse and validate its active machine settings and a +discovered source-project config before creation. Malformed or unreadable files +remain hard local failures even though their run values are not transmitted. + Use `fabro preflight` to validate a run config without executing it: ```bash diff --git a/docs/public/reference/cli.mdx b/docs/public/reference/cli.mdx index a5b826da6..a24a8b389 100644 --- a/docs/public/reference/cli.mdx +++ b/docs/public/reference/cli.mdx @@ -70,7 +70,7 @@ fabro [OPTIONS] [COMMAND] | `fabro attach` | Attach to a running or finished workflow run | | `fabro auth` | Manage CLI authentication state | | `fabro completion` | Generate shell completions | -| `fabro create` | Create a workflow run (allocate run dir, persist spec) | +| `fabro create` | Register a local workflow version and create a submitted run | | `fabro deny` | Deny pending workflow runs | | `fabro discord` | Open the Discord community in the browser | | `fabro docs` | Open the docs website in the browser | @@ -92,7 +92,7 @@ fabro [OPTIONS] [COMMAND] | `fabro resume` | Resume an interrupted workflow run | | `fabro rewind` | Rewind a workflow run to an earlier checkpoint | | `fabro rm` | Remove one or more workflow runs | -| `fabro run` | Launch a workflow run | +| `fabro run` | Register a local workflow version, create a run, and start it | | `fabro sandbox` | Sandbox operations (cp, ssh, preview) | | `fabro secret` | Manage server-owned secrets | | `fabro server` | Server operations | @@ -332,7 +332,7 @@ fabro completion [OPTIONS] ### `fabro create` -Create a workflow run (allocate run dir, persist spec) +Register a local workflow version and create a submitted run ```bash fabro create [OPTIONS] @@ -342,7 +342,7 @@ fabro create [OPTIONS] | Name | Description | | --- | --- | -| `WORKFLOW` | Path to a .fabro workflow file or .toml task config | +| `WORKFLOW` | Local workflow name, checkout path, .fabro file, or workflow TOML | #### Options @@ -353,7 +353,7 @@ fabro create [OPTIONS] | `--dry-run` | Execute with simulated LLM backend | | `--environment ` | Named environment for agent tools | | `--goal ` | Override the workflow goal (available as {{ goal }} in prompts) | -| `--goal-file ` | Read the workflow goal from a file | +| `--goal-file ` | Read a per-run goal value from a local file | | `--label ` | Attach a label to this run (repeatable, format: KEY=VALUE) | | `--model ` | Override default LLM model | | `--parent ` | Link this run to an existing orchestration parent run | @@ -1061,7 +1061,7 @@ fabro rm [OPTIONS] ... ### `fabro run` -Launch a workflow run +Register a local workflow version, create a run, and start it ```bash fabro run [OPTIONS] @@ -1071,7 +1071,7 @@ fabro run [OPTIONS] | Name | Description | | --- | --- | -| `WORKFLOW` | Path to a .fabro workflow file or .toml task config | +| `WORKFLOW` | Local workflow name, checkout path, .fabro file, or workflow TOML | #### Options @@ -1082,7 +1082,7 @@ fabro run [OPTIONS] | `--dry-run` | Execute with simulated LLM backend | | `--environment ` | Named environment for agent tools | | `--goal ` | Override the workflow goal (available as {{ goal }} in prompts) | -| `--goal-file ` | Read the workflow goal from a file | +| `--goal-file ` | Read a per-run goal value from a local file | | `--label ` | Attach a label to this run (repeatable, format: KEY=VALUE) | | `--model ` | Override default LLM model | | `--parent ` | Link this run to an existing orchestration parent run | diff --git a/docs/public/reference/user-configuration.mdx b/docs/public/reference/user-configuration.mdx index e68f36896..853f6a549 100644 --- a/docs/public/reference/user-configuration.mdx +++ b/docs/public/reference/user-configuration.mdx @@ -34,24 +34,27 @@ Files that omit `_version` are treated as version `1`. The legacy top-level `ver | Scope | Examples | |---|---| | CLI-only | `[cli.target]`, `[cli.auth]`, `[cli.exec]`, `[cli.output]`, `[cli.updates]`, `[cli.logging]` | -| Shared run defaults | `[run.model]`, `[run.environment]`, `[environments.]`, `[run.checkpoint]`, `[run.inputs]`, `[run.prepare]`, `[run.pull_request]`, `[run.integrations.github]`, `[run.hooks]`, `[run.agent.mcps]` | +| Server-side run policy | `[run.model]`, `[run.environment]`, `[environments.]`, `[run.checkpoint]`, `[run.inputs]`, `[run.prepare]`, `[run.pull_request]`, `[run.integrations.github]`, `[run.hooks]`, `[run.agent.mcps]` | | Shared LLM catalog | `[llm.providers.]`, provider-scoped `[llm.providers..models.]` offerings, limits, features, controls, and costs | | Server-only | `[server.listen]`, `[server.api]`, `[server.web]`, `[server.auth]`, `[server.storage]`, `[server.artifacts]`, `[server.slatedb]`, `[server.scheduler]`, `[server.logging]`, `[server.integrations]` | `[cli.*]` and `[server.*]` stanzas are owner-specific: they are only consumed from `~/.fabro/settings.toml` (plus process-local flags and env overrides). The same stanzas in `.fabro/project.toml` or `workflow.toml` remain schema-valid but runtime-inert. +`fabro run` and `fabro create` do not copy the CLI machine's `[run]` or +`[environments]` tables into an intent request. They warn with only the file +path and affected key names when those tables are present. Put workflow-owned +behavior in `workflow.toml` and configure placement in environments managed by +the target server. The server may still use its own active `settings.toml` +independently. + See [Server Configuration](/administration/server-configuration) for the server-owned sections. ## Precedence -Shared layered domains (`[project]`, `[workflow]`, `[run]`) use this override order: - -1. **CLI flags** — always win -2. **Environment overrides** — Fabro-defined override channels -3. **`workflow.toml`** — per-workflow overrides -4. **`.fabro/project.toml`** — project defaults -5. **`~/.fabro/settings.toml`** — machine defaults -6. **Built-in defaults** +For CLI-created workflow runs, sparse CLI flags override immutable +`workflow.toml` behavior. The CLI does not layer local project or machine run +defaults into the request. A target server resolves its own server-side policy +and environment catalog during admission. Owner-specific domains (`[cli.*]`, `[server.*]`) use a narrower trust boundary — only CLI flags, env overrides, `~/.fabro/settings.toml`, and built-in defaults apply. @@ -116,39 +119,17 @@ output_cost_per_mtok = 8.00 input_cost_per_mtok = 3.00 output_cost_per_mtok = 16.00 -[run.model] -name = "claude-sonnet-4-5" - -[run.git.author] -name = "fabro-bot" -email = "fabro-bot@company.com" - -[run.pull_request] -enabled = true - -[run.integrations.github.permissions] -contents = "write" -pull_requests = "write" - -[run.agent.mcps.filesystem] -type = "stdio" -command = ["npx", "-y", "@modelcontextprotocol/server-filesystem", "/workspace"] -startup_timeout = "15s" -tool_timeout = "90s" - -[run.agent.mcps.filesystem.env] -NODE_ENV = "production" - -[run.agent.mcps.sentry] -type = "http" -url = "https://mcp.sentry.dev/mcp" - -[run.agent.mcps.sentry.headers] -Authorization = "Bearer sk-xxx" ``` All fields are optional. Include only the sections and keys you want to override. A single file can still include both CLI and server sections when you run both processes on one machine, but explicit remote targets do not read remote server state from the local machine. + +The `[run]` schemas below remain valid for a server's own active configuration, +but they are not CLI-side defaults for `fabro run` or `fabro create`. Put +automatic pull-request settings and other workflow-owned behavior in +`workflow.toml`; there is no compatibility request field or CLI global default. + + {/* generated:options */} ## `[cli.target]` diff --git a/lib/apps/fabro-cli/src/args.rs b/lib/apps/fabro-cli/src/args.rs index 4b564ddb4..b9790885d 100644 --- a/lib/apps/fabro-cli/src/args.rs +++ b/lib/apps/fabro-cli/src/args.rs @@ -232,7 +232,7 @@ pub(crate) struct RunArgs { #[command(flatten)] pub(crate) inputs: InputOverrideArgs, - /// Path to a .fabro workflow file or .toml task config + /// Local workflow name, checkout path, .fabro file, or workflow TOML #[arg(required = true)] pub(crate) workflow: Option, @@ -248,7 +248,7 @@ pub(crate) struct RunArgs { #[arg(long)] pub(crate) goal: Option, - /// Read the workflow goal from a file + /// Read a per-run goal value from a local file #[arg(long, conflicts_with = "goal")] pub(crate) goal_file: Option, @@ -1136,9 +1136,9 @@ pub(crate) struct UpgradeArgs { #[derive(Subcommand)] pub(crate) enum RunCommands { - /// Launch a workflow run + /// Register a local workflow version, create a run, and start it Run(RunArgs), - /// Create a workflow run (allocate run dir, persist spec) + /// Register a local workflow version and create a submitted run Create(RunArgs), /// Start a created workflow run on the server Start(StartArgs), diff --git a/lib/apps/fabro-cli/src/command_context.rs b/lib/apps/fabro-cli/src/command_context.rs index b3318ce15..bbd0b6d59 100644 --- a/lib/apps/fabro-cli/src/command_context.rs +++ b/lib/apps/fabro-cli/src/command_context.rs @@ -17,7 +17,7 @@ use crate::args::{ ServerConnectionArgs, ServerTargetArgs, printer_from_verbosity, require_no_json_override, }; use crate::server_client::Client; -use crate::user_config::LoadedSettings; +use crate::user_config::{LoadedSettings, RunSettingsKeyPresence}; use crate::{server_client, user_config}; #[derive(Clone, Debug)] @@ -33,24 +33,19 @@ pub(crate) enum ServerMode { } pub(crate) struct CommandContext { - printer: Printer, + printer: Printer, process_local_json: bool, - cwd: PathBuf, - base_config_path: PathBuf, - cli_layer: CliLayer, - storage_dir: PathBuf, - run_settings: std::result::Result, - user_settings: UserSettings, - server_mode: ServerMode, - server: OnceCell>, - llm_source: OnceCell>, - catalog: OnceLock>, -} - -struct ResolvedCommandSettings { - storage_dir: PathBuf, - run_settings: std::result::Result, + cwd: PathBuf, + base_config_path: PathBuf, + cli_layer: CliLayer, + storage_dir: PathBuf, + run_settings: std::result::Result, user_settings: UserSettings, + run_settings_key_presence: RunSettingsKeyPresence, + server_mode: ServerMode, + server: OnceCell>, + llm_source: OnceCell>, + catalog: OnceLock>, } impl CommandContext { @@ -69,6 +64,7 @@ impl CommandContext { storage_dir: resolved_settings.storage_dir, run_settings: resolved_settings.run_settings, user_settings: resolved_settings.user_settings, + run_settings_key_presence: resolved_settings.run_settings_key_presence, server_mode: ServerMode::None, server: OnceCell::new(), llm_source: OnceCell::new(), @@ -119,6 +115,10 @@ impl CommandContext { &self.user_settings } + pub(crate) fn run_settings_key_presence(&self) -> &RunSettingsKeyPresence { + &self.run_settings_key_presence + } + pub(crate) fn base_config_path(&self) -> &Path { &self.base_config_path } @@ -219,6 +219,7 @@ impl CommandContext { storage_dir: resolved_settings.storage_dir, run_settings: resolved_settings.run_settings, user_settings: resolved_settings.user_settings, + run_settings_key_presence: resolved_settings.run_settings_key_presence, server_mode, server: OnceCell::new(), llm_source: OnceCell::new(), @@ -227,13 +228,10 @@ impl CommandContext { } } -fn load_merged_settings( - cli_layer: &CliLayer, - server_mode: &ServerMode, -) -> Result { - let loaded_settings = match server_mode { +fn load_merged_settings(cli_layer: &CliLayer, server_mode: &ServerMode) -> Result { + match server_mode { ServerMode::None | ServerMode::ByTarget { .. } => { - user_config::load_resolved_settings(None, None, Some(cli_layer))? + user_config::load_resolved_settings(None, None, Some(cli_layer)) } ServerMode::ByStorageDir { storage_dir_override, @@ -242,16 +240,7 @@ fn load_merged_settings( None, storage_dir_override.as_deref(), Some(cli_layer), - )?, - }; - Ok(resolve_command_settings(loaded_settings)) -} - -fn resolve_command_settings(loaded_settings: LoadedSettings) -> ResolvedCommandSettings { - ResolvedCommandSettings { - storage_dir: loaded_settings.storage_dir, - run_settings: loaded_settings.run_settings, - user_settings: loaded_settings.user_settings, + ), } } @@ -265,7 +254,7 @@ mod tests { use fabro_util::printer::Printer; use tokio::sync::OnceCell; - use super::{CommandContext, ServerMode, resolve_command_settings}; + use super::{CommandContext, ServerMode}; use crate::user_config; fn cli_layer_with_json_and_verbose() -> CliLayer { @@ -280,10 +269,9 @@ mod tests { fn synthetic_context(process_local_json: bool, printer: Printer) -> CommandContext { let cli_layer = cli_layer_with_json_and_verbose(); - let resolved_settings = resolve_command_settings( + let resolved_settings = user_config::load_resolved_settings_from_toml("_version = 1\n", None, Some(&cli_layer)) - .expect("settings should resolve"), - ); + .expect("settings should resolve"); CommandContext { printer, process_local_json, @@ -293,6 +281,7 @@ mod tests { storage_dir: resolved_settings.storage_dir, run_settings: resolved_settings.run_settings, user_settings: resolved_settings.user_settings, + run_settings_key_presence: resolved_settings.run_settings_key_presence, server_mode: ServerMode::None, server: OnceCell::new(), llm_source: OnceCell::new(), @@ -316,32 +305,28 @@ mod tests { #[test] fn storage_dir_override_only_changes_storage_root_in_merged_settings() { let cli_layer = cli_layer_with_json_and_verbose(); - let base_settings = resolve_command_settings( - user_config::load_resolved_settings_from_toml( - r#" + let base_settings = user_config::load_resolved_settings_from_toml( + r#" _version = 1 [server.storage] root = "/srv/fabro/default" "#, - None, - Some(&cli_layer), - ) - .expect("base settings should resolve"), - ); - let connection_settings = resolve_command_settings( - user_config::load_resolved_settings_from_toml( - r#" + None, + Some(&cli_layer), + ) + .expect("base settings should resolve"); + let connection_settings = user_config::load_resolved_settings_from_toml( + r#" _version = 1 [server.storage] root = "/srv/fabro/default" "#, - Some(std::path::Path::new("/srv/fabro/override")), - Some(&cli_layer), - ) - .expect("connection settings should resolve"), - ); + Some(std::path::Path::new("/srv/fabro/override")), + Some(&cli_layer), + ) + .expect("connection settings should resolve"); assert_eq!( base_settings.user_settings, @@ -385,27 +370,24 @@ root = "/srv/fabro" "fixture intentionally omits [server.auth] so server_settings should fail to resolve" ); - let resolved = resolve_command_settings(loaded); - assert_eq!(resolved.storage_dir, PathBuf::from("/srv/fabro")); - assert!(resolved.run_settings.is_ok()); + assert_eq!(loaded.storage_dir, PathBuf::from("/srv/fabro")); + assert!(loaded.run_settings.is_ok()); } #[test] fn run_settings_include_run_agent_mcps() { - let resolved = resolve_command_settings( - user_config::load_resolved_settings_from_toml( - r#" + let resolved = user_config::load_resolved_settings_from_toml( + r#" _version = 1 [run.agent.mcps.demo] type = "stdio" command = ["demo-mcp"] "#, - None, - Some(&CliLayer::default()), - ) - .expect("settings should resolve"), - ); + None, + Some(&CliLayer::default()), + ) + .expect("settings should resolve"); let run_settings = resolved.run_settings.expect("run settings should resolve"); assert!(run_settings.agent.mcps.contains_key("demo")); diff --git a/lib/apps/fabro-cli/src/commands/repo/init.rs b/lib/apps/fabro-cli/src/commands/repo/init.rs index 48bf0f49f..71c644b72 100644 --- a/lib/apps/fabro-cli/src/commands/repo/init.rs +++ b/lib/apps/fabro-cli/src/commands/repo/init.rs @@ -61,12 +61,6 @@ pub(crate) async fn run_init( # https://docs.fabro.computer/getting-started/quick-start _version = 1 - -# Auto-create pull requests on successful workflow runs. -[run.pull_request] -enabled = true -draft = true -# auto_merge = true ", ) .with_context(|| format!("failed to write {}", project_toml.display()))?; @@ -122,7 +116,18 @@ draft = true let toml_path = workflow_dir.join("workflow.toml"); std::fs::write( &toml_path, - "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + "\ +_version = 1 + +[workflow] +graph = \"workflow.fabro\" + +# Auto-create pull requests on successful workflow runs. +[run.pull_request] +enabled = true +draft = true +# auto_merge = true +", ) .with_context(|| format!("failed to write {}", toml_path.display()))?; created.push(".fabro/workflows/hello/workflow.toml".to_string()); diff --git a/lib/apps/fabro-cli/src/commands/run/command.rs b/lib/apps/fabro-cli/src/commands/run/command.rs index 4c7f7a93b..2ec25f636 100644 --- a/lib/apps/fabro-cli/src/commands/run/command.rs +++ b/lib/apps/fabro-cli/src/commands/run/command.rs @@ -15,7 +15,7 @@ pub(crate) async fn execute(mut args: RunArgs, base_ctx: &CommandContext) -> Res let quiet = args.detach; let prevent_idle_sleep = ctx.user_settings().cli.exec.prevent_idle_sleep; - let created_run = Box::pin(super::create::create_run(&ctx, &args, styles, quiet)).await?; + let created_run = Box::pin(super::create::create_run(&ctx, &args, styles)).await?; if !quiet { fabro_util::printerr!( diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index b8bde173f..184c19816 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -1,83 +1,115 @@ -use anyhow::{Context as _, bail}; -use fabro_config::RunLayer; -use fabro_config::user::active_settings_path; -use fabro_manifest::{ManifestBuildInput, build_run_manifest}; -use fabro_server::manifest_validation; -use fabro_types::RunId; +use std::path::Path; + +use anyhow::{Context as _, anyhow, bail}; +use fabro_config::project; +use fabro_environment::DEFAULT_ENVIRONMENT_ID; +use fabro_types::settings::run::EnvironmentProvider; +use fabro_types::{DirtyStatus, RunId, RunIntent, RunTarget}; use fabro_util::terminal::Styles; -use super::output::{api_diagnostics_to_local, print_workflow_summary}; -use super::overrides::run_args_overrides; +use super::overrides::prepare_intent_overrides; use crate::args::RunArgs; use crate::command_context::CommandContext; use crate::commands::resolve_run_id; -use crate::manifest_args::run_manifest_args; +use crate::user_config::{RunSettingsKeyPresence, read_project_run_settings_key_presence}; pub(crate) struct CreatedRun { pub(crate) run_id: RunId, } -/// Create a workflow run: allocate run directory, persist RunSpec, return -/// (run_id, run_dir). +/// Register the local workflow version closure with the server and create a +/// run from an immutable workflow intent, leaving it in the submitted state. /// -/// This does NOT execute the workflow — it only prepares the run directory. +/// This does NOT start the workflow — starting is a separate request. pub(crate) async fn create_run( ctx: &CommandContext, args: &RunArgs, styles: &Styles, - quiet: bool, ) -> anyhow::Result { let workflow_path = args .workflow .as_ref() .ok_or_else(|| anyhow::anyhow!("--workflow is required"))?; - let cli_args_config = run_args_overrides(args)?; - let cwd = ctx.cwd().to_path_buf(); - let mut built = build_run_manifest(ManifestBuildInput { - workflow: workflow_path.clone(), - cwd, - run_overrides: cli_args_config.run, - cli_overrides: cli_args_config.cli, - input_overrides: cli_args_config.input_overrides, - args: run_manifest_args(args), - environment_defaults: fabro_environment::seeded_catalog_layer(), - user_settings_path: Some(active_settings_path(None)), + let canonical_cwd = ctx.cwd().canonicalize().with_context(|| { + format!( + "failed to canonicalize caller working directory {}", + ctx.cwd().display() + ) })?; + let user_workflows_root = fabro_util::Home::from_env().workflows_dir(); + let package = fabro_manifest::resolve_local_workflow_package( + workflow_path, + &canonical_cwd, + Some(&user_workflows_root), + )?; + let prepared = prepare_intent_overrides(args, &canonical_cwd).await?; - let client = if let Some(parent_selector) = args.parent.as_deref() { - let client = ctx.server().await?; - let parent_id = resolve_run_id(client.as_ref(), parent_selector).await?; - built.manifest.parent_id = Some(parent_id.to_string()); - Some(client) - } else { - None - }; - - let mut validation = - manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; - manifest_validation::promote_template_undefined_variables_to_errors(&mut validation); - let diagnostics = api_diagnostics_to_local(&validation.workflow.diagnostics); - if !quiet { - print_workflow_summary( - &validation.workflow, - Some(&built.target_path), + warn_untransmitted_settings( + ctx, + styles, + ctx.base_config_path(), + *ctx.run_settings_key_presence(), + ); + let project_config = project::discover_project_config(&package.workflow_location().dir)?; + if let Some(path) = project_config.as_deref() { + warn_untransmitted_settings( + ctx, styles, - ctx.printer(), + path, + read_project_run_settings_key_presence(path).await?, ); } - if diagnostics - .iter() - .any(|diagnostic| diagnostic.severity == fabro_validate::Severity::Error) - { - bail!("Validation failed"); - } - let client = match client { - Some(client) => client, - None => ctx.server().await?, - }; + let client = ctx.server().await?; + let environment_id = args + .environment + .as_deref() + .unwrap_or(DEFAULT_ENVIRONMENT_ID); + let (parent_id, environment) = tokio::try_join!( + async { + match args.parent.as_deref() { + Some(parent_selector) => Ok(Some( + resolve_run_id(client.as_ref(), parent_selector).await?, + )), + None => Ok(None), + } + }, + async { + client + .retrieve_environment(environment_id) + .await + .with_context(|| format!("could not retrieve environment `{environment_id}`")) + }, + )?; + let (target, dirty_worktree) = + run_target_for_environment(environment.settings.provider, &canonical_cwd)?; + if dirty_worktree { + fabro_util::printerr!( + ctx.printer(), + "{} the caller Git working tree is dirty; uncommitted changes are not included in the run target.", + styles.yellow.apply_to("Warning:"), + ); + } + let workflow_version_id = package.closure().root_id(); + client + .register_workflow_versions( + package + .closure() + .versions() + .map(|(_, validated)| validated.version()), + ) + .await + .context("could not register workflow versions")?; let created_run_id = client - .create_run_from_manifest(built.manifest) + .create_run_from_intent(RunIntent { + workflow_version_id, + target, + args: prepared.intent_args, + environment_id: Some(environment.id.to_string()), + parent_id, + title: None, + goal: prepared.goal, + }) .await .context("could not create run")?; @@ -85,3 +117,105 @@ pub(crate) async fn create_run( run_id: created_run_id, }) } + +fn warn_untransmitted_settings( + ctx: &CommandContext, + styles: &Styles, + path: &Path, + presence: RunSettingsKeyPresence, +) { + let keys = presence.key_paths(); + if keys.is_empty() { + return; + } + fabro_util::printerr!( + ctx.printer(), + "{} {} contains {}; `fabro run` and `fabro create` do not transmit these settings. Move workflow-owned run behavior, including `run.pull_request`, to `workflow.toml`; configure placement with server-managed environments.", + styles.yellow.apply_to("Warning:"), + path.display(), + keys.join(", "), + ); +} + +/// Derives the run target from the caller directory for the environment's +/// provider. Returns the target plus whether a clone-based observation found a +/// dirty Git worktree, so the caller can warn about it. +fn run_target_for_environment( + provider: EnvironmentProvider, + canonical_cwd: &Path, +) -> anyhow::Result<(RunTarget, bool)> { + if !provider.is_clone_based() { + let path = canonical_cwd.to_str().ok_or_else(|| { + anyhow!( + "caller working directory is not valid UTF-8: {}", + canonical_cwd.display() + ) + })?; + return Ok(( + RunTarget::Folder { + path: path.to_string(), + }, + false, + )); + } + let Some(observation) = fabro_manifest::observe_git_run_target(canonical_cwd, None) else { + return Ok((none_target_for_unversioned_directory(canonical_cwd)?, false)); + }; + let dirty = observation.legacy_git_context.dirty == DirtyStatus::Dirty; + let target = observation.run_target.ok_or_else(|| { + anyhow!("the caller Git checkout cannot be represented as a canonical GitHub run target") + })?; + if target.sha.is_none() { + bail!( + "the exact local Git commit could not be made available from the canonical GitHub origin; push the commit and try again" + ); + } + Ok((RunTarget::Git(target), dirty)) +} + +fn none_target_for_unversioned_directory(canonical_cwd: &Path) -> anyhow::Result { + let repository = match git2::Repository::discover(canonical_cwd) { + Ok(repository) => repository, + Err(source) if source.code() == git2::ErrorCode::NotFound => return Ok(RunTarget::None {}), + Err(source) => { + return Err(anyhow::Error::new(source)).with_context(|| { + format!( + "failed to inspect caller working directory {} for Git metadata", + canonical_cwd.display() + ) + }); + } + }; + + if repository.is_bare() { + bail!( + "the caller directory resolves to a bare Git repository; clone-based runs require a non-bare checkout with an attached branch" + ); + } + match repository.head() { + Err(source) + if matches!( + source.code(), + git2::ErrorCode::UnbornBranch | git2::ErrorCode::NotFound + ) => + { + bail!( + "the caller Git checkout has no commits; create a commit before using a clone-based environment" + ); + } + Err(source) => { + return Err(anyhow::Error::new(source)) + .context("failed to inspect the caller Git checkout HEAD"); + } + Ok(head) if !head.is_branch() => { + bail!( + "the caller Git checkout has a detached HEAD; check out a branch before using a clone-based environment" + ); + } + Ok(_) => {} + } + + bail!( + "the caller Git checkout does not have a usable attached branch for a clone-based run target" + ) +} diff --git a/lib/apps/fabro-cli/src/commands/run/mod.rs b/lib/apps/fabro-cli/src/commands/run/mod.rs index 859659c35..7d0c2e3ed 100644 --- a/lib/apps/fabro-cli/src/commands/run/mod.rs +++ b/lib/apps/fabro-cli/src/commands/run/mod.rs @@ -42,7 +42,7 @@ pub(crate) async fn dispatch( RunCommands::Create(args) => { let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); let ctx = base_ctx.with_target(&args.target)?; - let created_run = Box::pin(create::create_run(&ctx, &args, styles, true)).await?; + let created_run = Box::pin(create::create_run(&ctx, &args, styles)).await?; if ctx.json_output() { print_json_pretty(&serde_json::json!({ "run_id": created_run.run_id }))?; } else { diff --git a/lib/apps/fabro-cli/src/commands/run/overrides.rs b/lib/apps/fabro-cli/src/commands/run/overrides.rs index bd154ad87..3c0ffca45 100644 --- a/lib/apps/fabro-cli/src/commands/run/overrides.rs +++ b/lib/apps/fabro-cli/src/commands/run/overrides.rs @@ -1,13 +1,15 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use anyhow::{Result, anyhow}; +use anyhow::{Context as _, Result, anyhow}; use fabro_config::{ CliLayer, CliOutputLayer, RunGoalLayer, RunLayer, parse_input_overrides, parse_labels, }; use fabro_manifest::{RunOverrideInput, build_run_overrides}; +use fabro_types::RunIntentArgs; use fabro_types::settings::cli::OutputVerbosity; use fabro_types::settings::interp::InterpString; +use tokio::fs; use crate::args::{PreflightArgs, RunArgs}; @@ -18,6 +20,12 @@ pub(crate) struct ManifestSettingsOverrides { pub(crate) input_overrides: HashMap, } +#[derive(Debug)] +pub(super) struct PreparedIntentOverrides { + pub(super) intent_args: RunIntentArgs, + pub(super) goal: Option, +} + fn sparse_flag(value: bool) -> Option { value.then_some(true) } @@ -67,25 +75,60 @@ fn current_dir_or_dot() -> PathBuf { std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")) } -pub(crate) fn run_args_overrides(args: &RunArgs) -> Result { - let cwd = current_dir_or_dot(); - let goal = goal_layer_from_args(args.goal.as_deref(), args.goal_file.as_deref(), &cwd)?; - let mut run = build_run_overrides(RunOverrideInput { - goal: None, - model: args.model.as_deref(), - provider: args.provider.as_deref(), - environment: args.environment.as_deref(), - preserve_sandbox: sparse_flag(args.preserve_sandbox), - dry_run: sparse_flag(args.dry_run), - auto_approve: sparse_flag(args.auto_approve), - labels: parse_labels(&args.label), - }); - run.goal = goal; +async fn intent_goal_from_args( + goal: Option<&str>, + goal_file: Option<&Path>, + cwd: &Path, +) -> Result> { + match (goal, goal_file) { + (Some(_), Some(_)) => Err(anyhow!( + "--goal and --goal-file are mutually exclusive; use exactly one" + )), + (Some(text), None) => Ok(Some(text.to_owned())), + (None, Some(path)) => { + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + cwd.join(path) + }; + Ok(Some(fs::read_to_string(&absolute).await.with_context( + || format!("failed to read goal file {}", absolute.display()), + )?)) + } + (None, None) => Ok(None), + } +} - Ok(ManifestSettingsOverrides { - run: Some(run), - cli: cli_layer_for_verbose(args.verbose), - input_overrides: parse_input_overrides(&args.inputs.values)?, +pub(super) async fn prepare_intent_overrides( + args: &RunArgs, + cwd: &Path, +) -> Result { + let goal = intent_goal_from_args(args.goal.as_deref(), args.goal_file.as_deref(), cwd).await?; + let input_overrides = parse_input_overrides(&args.inputs.values)?; + let inputs = input_overrides + .iter() + .map(|(key, value)| { + let value = fabro_types::toml_scalar_to_json_value(value) + .map_err(anyhow::Error::new) + .with_context(|| format!("failed to convert input override `{key}`"))?; + Ok((key.clone(), value)) + }) + .collect::>>()?; + let labels = parse_labels(&args.label); + let dry_run = sparse_flag(args.dry_run); + let auto_approve = sparse_flag(args.auto_approve); + let preserve_sandbox = sparse_flag(args.preserve_sandbox); + Ok(PreparedIntentOverrides { + intent_args: RunIntentArgs { + model: args.model.clone(), + provider: args.provider.clone(), + inputs, + labels, + dry_run, + auto_approve, + preserve_sandbox, + }, + goal, }) } @@ -118,6 +161,149 @@ pub(crate) fn preflight_args_overrides(args: &PreflightArgs) -> Result RunArgs { + RunArgs { + target: ServerTargetArgs::default(), + inputs: InputOverrideArgs::default(), + workflow: Some(PathBuf::from("workflow.fabro")), + dry_run: false, + auto_approve: false, + goal: None, + goal_file: None, + model: None, + provider: None, + verbose: false, + environment: None, + label: Vec::new(), + parent: None, + preserve_sandbox: false, + detach: false, + } + } + + #[tokio::test] + async fn intent_overrides_preserve_typed_values_and_sparse_flags() { + let mut args = run_args(); + args.inputs.values = vec![ + "string=hello".to_string(), + "boolean=true".to_string(), + "integer=42".to_string(), + "float=1.25".to_string(), + ]; + args.goal = Some("Ship it".to_string()); + args.model = Some("gpt-5".to_string()); + args.provider = Some("openai".to_string()); + args.environment = Some("cloud".to_string()); + args.label = vec!["team=cli".to_string()]; + args.dry_run = true; + args.auto_approve = true; + args.preserve_sandbox = true; + args.verbose = true; + + let PreparedIntentOverrides { intent_args, goal } = + prepare_intent_overrides(&args, Path::new("/caller")) + .await + .unwrap(); + + assert_eq!(goal.as_deref(), Some("Ship it")); + assert_eq!( + intent_args.inputs, + HashMap::from([ + ("string".to_string(), serde_json::json!("hello")), + ("boolean".to_string(), serde_json::json!(true)), + ("integer".to_string(), serde_json::json!(42)), + ("float".to_string(), serde_json::json!(1.25)), + ]) + ); + assert_eq!(intent_args.model.as_deref(), Some("gpt-5")); + assert_eq!(intent_args.provider.as_deref(), Some("openai")); + assert_eq!(intent_args.labels.get("team"), Some(&"cli".to_string())); + assert_eq!(intent_args.dry_run, Some(true)); + assert_eq!(intent_args.auto_approve, Some(true)); + assert_eq!(intent_args.preserve_sandbox, Some(true)); + assert!( + !serde_json::to_value(&intent_args) + .unwrap() + .as_object() + .unwrap() + .contains_key("verbose") + ); + } + + #[tokio::test] + async fn intent_overrides_leave_false_flags_absent() { + let prepared = prepare_intent_overrides(&run_args(), Path::new("/caller")) + .await + .unwrap(); + + assert_eq!(prepared.intent_args.dry_run, None); + assert_eq!(prepared.intent_args.auto_approve, None); + assert_eq!(prepared.intent_args.preserve_sandbox, None); + } + + #[tokio::test] + async fn intent_goal_files_are_read_by_value_from_relative_and_absolute_paths() { + let dir = tempfile::tempdir().unwrap(); + let relative = PathBuf::from("goals/task.md"); + fs::create_dir_all(dir.path().join("goals")).await.unwrap(); + fs::write(dir.path().join(&relative), "Goal from file") + .await + .unwrap(); + + for goal_file in [relative, dir.path().join("goals/task.md")] { + let mut args = run_args(); + args.goal_file = Some(goal_file); + let PreparedIntentOverrides { + intent_args: _, + goal, + } = prepare_intent_overrides(&args, dir.path()).await.unwrap(); + + assert_eq!(goal.as_deref(), Some("Goal from file")); + } + } + + #[tokio::test] + async fn intent_goal_file_read_errors_preserve_the_resolved_path_and_source() { + let mut args = run_args(); + args.goal_file = Some(PathBuf::from("missing.md")); + + let error = prepare_intent_overrides(&args, Path::new("/caller")) + .await + .unwrap_err(); + assert!(error.to_string().contains("/caller/missing.md")); + assert!(error.source().is_some()); + } + + #[tokio::test] + async fn intent_goal_and_goal_file_together_are_rejected_defensively() { + let mut args = run_args(); + args.goal = Some("inline".to_string()); + args.goal_file = Some(PathBuf::from("goal.md")); + + let error = prepare_intent_overrides(&args, Path::new("/caller")) + .await + .unwrap_err(); + assert!(error.to_string().contains("mutually exclusive")); + } + + #[tokio::test] + async fn intent_overrides_reject_non_finite_float_with_input_key() { + let mut args = run_args(); + args.inputs.values = vec!["temperature=nan".to_string()]; + + let error = prepare_intent_overrides(&args, Path::new("/caller")) + .await + .unwrap_err(); + assert!(error.to_string().contains("temperature")); + assert!(format!("{error:#}").contains("finite")); + assert!(error.chain().any(|cause| { + cause + .downcast_ref::() + .is_some() + })); + } #[test] fn goal_and_goal_file_together_is_rejected() { diff --git a/lib/apps/fabro-cli/src/landing.rs b/lib/apps/fabro-cli/src/landing.rs index ef6f87d9c..866f4487d 100644 --- a/lib/apps/fabro-cli/src/landing.rs +++ b/lib/apps/fabro-cli/src/landing.rs @@ -46,7 +46,7 @@ pub(crate) fn print() { &[ ("validate", "Validate a workflow"), ("preflight", "Validate run configuration without executing"), - ("run", "Launch a workflow run"), + ("run", "Register and run a local workflow"), ], cmd_width, ); diff --git a/lib/apps/fabro-cli/src/main.rs b/lib/apps/fabro-cli/src/main.rs index 6cb9f9788..9ab51b332 100644 --- a/lib/apps/fabro-cli/src/main.rs +++ b/lib/apps/fabro-cli/src/main.rs @@ -1312,20 +1312,6 @@ destination = "{destination}" } } - #[test] - fn run_manifest_args_preserves_input_only_manifest_args() { - let cli = Cli::try_parse_from(["fabro", "run", "workflow.toml", "-I", "foo=bar"]) - .expect("should parse"); - match *cli.command.unwrap() { - Commands::RunCmd(RunCommands::Run(args)) => { - let manifest_args = manifest_args::run_manifest_args(&args) - .expect("input-only args should be retained"); - assert_eq!(manifest_args.input, vec!["foo=bar"]); - } - _ => panic!("unexpected command variant"), - } - } - #[test] fn parse_create_input_long_flag() { let cli = Cli::try_parse_from(["fabro", "create", "workflow.toml", "--input", "foo=bar"]) diff --git a/lib/apps/fabro-cli/src/manifest_args.rs b/lib/apps/fabro-cli/src/manifest_args.rs index e7e5ab052..e3857ceb3 100644 --- a/lib/apps/fabro-cli/src/manifest_args.rs +++ b/lib/apps/fabro-cli/src/manifest_args.rs @@ -1,21 +1,6 @@ use fabro_api::types; -use crate::args::{PreflightArgs, RunArgs}; - -pub(crate) fn run_manifest_args(args: &RunArgs) -> Option { - let payload = types::ManifestArgs { - auto_approve: args.auto_approve.then_some(true), - dry_run: args.dry_run.then_some(true), - label: args.label.clone(), - model: args.model.clone(), - preserve_sandbox: args.preserve_sandbox.then_some(true), - provider: args.provider.clone(), - environment: args.environment.clone(), - input: args.inputs.values.clone(), - verbose: args.verbose.then_some(true), - }; - (!fabro_manifest::manifest_args_is_empty(&payload)).then_some(payload) -} +use crate::args::PreflightArgs; pub(crate) fn preflight_manifest_args(args: &PreflightArgs) -> Option { let payload = types::ManifestArgs { diff --git a/lib/apps/fabro-cli/src/user_config.rs b/lib/apps/fabro-cli/src/user_config.rs index f93227ca9..e842a389e 100644 --- a/lib/apps/fabro-cli/src/user_config.rs +++ b/lib/apps/fabro-cli/src/user_config.rs @@ -3,10 +3,12 @@ use std::str::FromStr; use anyhow::{Context, Result, anyhow}; pub(crate) use fabro_client::ServerTarget; +use fabro_config::parse::{SettingsSource, validate_settings_source}; pub(crate) use fabro_config::user::{active_settings_path, default_storage_dir}; use fabro_config::user::{default_settings_path, default_socket_path}; use fabro_config::{ - CliLayer, LogFilter, ParseError, RunSettingsBuilder, ServerSettingsBuilder, UserSettingsBuilder, + CliLayer, LogFilter, ParseError, RunSettingsBuilder, ServerSettingsBuilder, SettingsLayer, + UserSettingsBuilder, }; use fabro_static::EnvVars; use fabro_types::settings::RunNamespace; @@ -15,18 +17,42 @@ use fabro_types::settings::server::LogDestination; use fabro_types::{ServerSettings, UserSettings}; use fabro_util::error::SharedError; use fabro_util::version::FABRO_VERSION; +use tokio::fs; use toml_edit::{DocumentMut, Item, Table, value}; use tracing::debug; use crate::args::ServerTargetArgs; +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(crate) struct RunSettingsKeyPresence { + pub(crate) run: bool, + pub(crate) environments: bool, +} + +impl RunSettingsKeyPresence { + fn from_document(document: &toml::Value) -> Self { + Self { + run: document.get("run").is_some(), + environments: document.get("environments").is_some(), + } + } + + pub(crate) fn key_paths(self) -> Vec<&'static str> { + [(self.run, "run"), (self.environments, "environments")] + .into_iter() + .filter_map(|(present, key)| present.then_some(key)) + .collect() + } +} + pub(crate) struct LoadedSettings { - pub(crate) storage_dir: PathBuf, - pub(crate) config_log_level: Option, - pub(crate) config_log_destination: Option, - pub(crate) run_settings: std::result::Result, - pub(crate) server_settings: std::result::Result, - pub(crate) user_settings: UserSettings, + pub(crate) storage_dir: PathBuf, + pub(crate) config_log_level: Option, + pub(crate) config_log_destination: Option, + pub(crate) run_settings: std::result::Result, + pub(crate) server_settings: std::result::Result, + pub(crate) user_settings: UserSettings, + pub(crate) run_settings_key_presence: RunSettingsKeyPresence, } pub(crate) fn load_resolved_settings( @@ -35,6 +61,7 @@ pub(crate) fn load_resolved_settings( cli_layer: Option<&CliLayer>, ) -> anyhow::Result { let document = load_settings_document(config_path)?; + let run_settings_key_presence = RunSettingsKeyPresence::from_document(&document); let storage_override = storage_dir.map(Path::to_path_buf); let storage_dir = storage_dir_from_document(&document, storage_dir); let pre_tracing_config = pre_tracing_config_from_document(&document)?; @@ -54,9 +81,25 @@ pub(crate) fn load_resolved_settings( run_settings, server_settings, user_settings, + run_settings_key_presence, }) } +pub(crate) async fn read_project_run_settings_key_presence( + path: &Path, +) -> anyhow::Result { + let parse_error = + |source| fabro_config::Error::parse_file("Failed to parse settings file", path, source); + let source = fs::read_to_string(path) + .await + .map_err(|source| fabro_config::Error::read_file(path, source))?; + let document: toml::Value = toml::from_str(&source) + .map_err(|source| parse_error(ParseError::Toml(source.to_string())))?; + let layer = source.parse::().map_err(parse_error)?; + validate_settings_source(&layer, SettingsSource::Project).map_err(parse_error)?; + Ok(RunSettingsKeyPresence::from_document(&document)) +} + fn load_settings_document(config_path: Option<&Path>) -> anyhow::Result { load_settings_document_with_lookup(config_path, process_env_var_os) } @@ -333,6 +376,7 @@ pub(crate) fn load_resolved_settings_from_toml( cli_layer: Option<&CliLayer>, ) -> anyhow::Result { let document: toml::Value = toml::from_str(source).context("failed to parse settings file")?; + let run_settings_key_presence = RunSettingsKeyPresence::from_document(&document); let storage_override = storage_dir.map(Path::to_path_buf); let storage_dir = storage_dir_from_document(&document, storage_dir); let pre_tracing_config = pre_tracing_config_from_document(&document)?; @@ -359,6 +403,7 @@ pub(crate) fn load_resolved_settings_from_toml( run_settings, server_settings, user_settings, + run_settings_key_presence, }) } @@ -383,6 +428,73 @@ mod tests { UserSettingsBuilder::from_toml(source).expect("fixture should resolve") } + #[test] + fn run_settings_key_presence_distinguishes_absent_empty_and_populated_keys() { + let absent: toml::Value = toml::from_str("_version = 1\n").unwrap(); + let empty_run: toml::Value = toml::from_str("_version = 1\n\n[run]\n").unwrap(); + let empty_environments: toml::Value = + toml::from_str("_version = 1\n\n[environments]\n").unwrap(); + let populated: toml::Value = toml::from_str( + "_version = 1\n\n[run.model]\nname = \"gpt-5\"\n\n[environments.cloud]\nprovider = \"docker\"\n", + ) + .unwrap(); + + assert_eq!( + RunSettingsKeyPresence::from_document(&absent), + RunSettingsKeyPresence::default() + ); + assert_eq!( + RunSettingsKeyPresence::from_document(&empty_run), + RunSettingsKeyPresence { + run: true, + environments: false, + } + ); + assert_eq!( + RunSettingsKeyPresence::from_document(&empty_environments), + RunSettingsKeyPresence { + run: false, + environments: true, + } + ); + assert_eq!( + RunSettingsKeyPresence::from_document(&populated), + RunSettingsKeyPresence { + run: true, + environments: true, + } + ); + } + + #[tokio::test] + async fn project_run_settings_key_presence_validates_the_same_source() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("project.toml"); + fs::write(&path, "_version = 1\n\n[run]\n\n[environments]\n") + .await + .unwrap(); + + assert_eq!( + read_project_run_settings_key_presence(&path).await.unwrap(), + RunSettingsKeyPresence { + run: true, + environments: true, + } + ); + + fs::write( + &path, + "_version = 1\n\n[environments.cloud]\ncwd = \"/tmp\"\n", + ) + .await + .unwrap(); + let error = read_project_run_settings_key_presence(&path) + .await + .unwrap_err(); + assert!(error.to_string().contains(&path.display().to_string())); + assert!(error.source().is_some()); + } + #[test] fn exec_has_no_server_target_by_default() { assert_eq!(exec_server_target(&server_target_args(None)).unwrap(), None); diff --git a/lib/apps/fabro-cli/tests/it/cmd/attach.rs b/lib/apps/fabro-cli/tests/it/cmd/attach.rs index 93cd58c83..4d279727d 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/attach.rs @@ -364,6 +364,8 @@ fn attach_replays_completed_detached_run() { "--dry-run", "--auto-approve", "--detach", + "--environment", + "local", workflow.to_str().unwrap(), ]) .assert() @@ -884,7 +886,6 @@ fn attach_json_errors_without_prompting_for_human_input() { } } }, - "manifest_blob": "[BLOB_HASH]", "provenance": { "client": { "name": "fabro-cli", @@ -1014,10 +1015,15 @@ fn attach_json_errors_without_prompting_for_human_input() { }, "source_directory": "[TEMP_DIR]", "spec_blob": "[BLOB_HASH]", + "target": { + "kind": "folder", + "path": "[TEMP_DIR]" + }, "title": "Wait for approval", "web_url": "http://localhost:3000/runs/[ULID]", "workflow_slug": "human-gate", - "workflow_source": "digraph HumanGate {/n graph [goal=\"Wait for approval\"]/n start [shape=Mdiamond, label=\"Start\"]/n exit [shape=Msquare, label=\"Exit\"]/n approve [shape=hexagon, label=\"Approve?\"]/n ship [shape=parallelogram, script=\"echo shipped\"]/n revise [shape=parallelogram, script=\"echo revised\"]/n start -> approve/n approve -> ship [label=\"[A] Approve\"]/n approve -> revise [label=\"[R] Revise\"]/n ship -> exit/n revise -> exit/n}/n" + "workflow_source": "digraph HumanGate {/n graph [goal=\"Wait for approval\"]/n start [shape=Mdiamond, label=\"Start\"]/n exit [shape=Msquare, label=\"Exit\"]/n approve [shape=hexagon, label=\"Approve?\"]/n ship [shape=parallelogram, script=\"echo shipped\"]/n revise [shape=parallelogram, script=\"echo revised\"]/n start -> approve/n approve -> ship [label=\"[A] Approve\"]/n approve -> revise [label=\"[R] Revise\"]/n ship -> exit/n revise -> exit/n}/n", + "workflow_version_id": "fc1611d3be115f2db472e4ac05a5034f449743089259566b18f204ff961a0c18" }, "run_id": "[ULID]", "ts": "[TIMESTAMP]" diff --git a/lib/apps/fabro-cli/tests/it/cmd/config.rs b/lib/apps/fabro-cli/tests/it/cmd/config.rs index daa58168b..663c727d9 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/config.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/config.rs @@ -303,7 +303,7 @@ script = "workflow-setup" // --------------------------------------------------------------------------- #[test] -fn create_explicit_workflow_path_uses_project_config_relative_to_workflow() { +fn create_explicit_workflow_warns_without_transmitting_machine_or_project_run_settings() { let mut context = test_context!(); let (project, storage_dir) = setup_external_workflow_fixture(&mut context); context.ensure_home_server_auth_methods(); @@ -320,10 +320,20 @@ fn create_explicit_workflow_path_uses_project_config_relative_to_workflow() { "--dry-run", "--model", "gpt-5.4-pro", + "--environment", + "local", workflow.to_str().unwrap(), ]) .assert() .success(); + let stderr = String::from_utf8_lossy(&create.get_output().stderr); + assert!(stderr.contains("settings.toml contains run"), "{stderr}"); + assert!(stderr.contains("project.toml contains run"), "{stderr}"); + assert!( + stderr.contains("do not transmit these settings"), + "{stderr}" + ); + assert!(stderr.contains("workflow.toml"), "{stderr}"); let run_id = created_run_id(create.get_output()); let runs_dir = storage_dir.join("scratch"); @@ -348,6 +358,8 @@ fn create_explicit_workflow_path_uses_project_config_relative_to_workflow() { let run_spec = serde_json::to_value(&state.spec).unwrap(); assert_eq!( run_spec["settings"]["run"]["execution"]["approval"].as_str(), + // The CLI did not transmit its machine setting. The in-process server + // may still apply its own active configuration independently. Some("auto") ); assert_eq!( diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index 48be32af7..655ed1570 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -1,11 +1,21 @@ +#![expect( + clippy::disallowed_methods, + reason = "integration tests stage temporary workflow and Git fixtures with synchronous APIs" +)] + +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::{Arc, Mutex}; + use fabro_test::{fabro_json_snapshot, fabro_snapshot, test_context}; -use httpmock::MockServer; +use httpmock::{HttpMockResponse, Mock, MockServer}; use insta::assert_snapshot; use serde_json::json; use super::support::{ - created_run_id, fixture, output_stdout, remote_run_summary_json, resolve_run, - run_count_for_test_case, run_state, + created_run_id, environment_json, fixture, mock_environment, + mock_workflow_version_registrations, mock_workflow_version_registrations_recording, + output_stderr, output_stdout, remote_run_summary_json, resolve_run, run_count_for_test_case, + run_git, run_state, }; use crate::support::unique_run_id; @@ -28,6 +38,46 @@ fn run_status_response(run_id: &str, status: &str) -> serde_json::Value { ) } +fn mock_intent_create<'a>( + server: &'a MockServer, + run_id: &str, + requests: Arc>>, +) -> Mock<'a> { + let response = run_status_response(run_id, "submitted").to_string(); + server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.respond_with(move |request| { + requests.lock().unwrap().push( + serde_json::from_slice(request.body_ref()) + .expect("run-intent request body should be valid JSON"), + ); + HttpMockResponse::builder() + .status(201) + .header("content-type", "application/json") + .body(response.clone()) + .build() + }); + }) +} + +fn write_workflow(root: &std::path::Path, directory: &str, graph_name: &str) -> std::path::PathBuf { + let directory = root.join(directory); + std::fs::create_dir_all(&directory).expect("workflow fixture directory should be created"); + std::fs::write( + directory.join("workflow.toml"), + "_version = 1\n\n[workflow]\ngraph = \"workflow.fabro\"\n", + ) + .expect("workflow fixture manifest should be written"); + std::fs::write( + directory.join("workflow.fabro"), + format!( + "digraph {graph_name} {{ start [shape=Mdiamond] exit [shape=Msquare] start -> exit }}" + ), + ) + .expect("workflow fixture graph should be written"); + directory.join("workflow.toml") +} + #[test] fn help() { let context = test_context!(); @@ -37,12 +87,12 @@ fn help() { success: true exit_code: 0 ----- stdout ----- - Create a workflow run (allocate run dir, persist spec) + Register a local workflow version and create a submitted run Usage: fabro create [OPTIONS] Arguments: - Path to a .fabro workflow file or .toml task config + Local workflow name, checkout path, .fabro file, or workflow TOML Options: --json Output as JSON [env: FABRO_JSON=] @@ -54,7 +104,7 @@ fn help() { --auto-approve Auto-approve all human gates --quiet Suppress non-essential output [env: FABRO_QUIET=] --goal Override the workflow goal (available as {{ goal }} in prompts) - --goal-file Read the workflow goal from a file + --goal-file Read a per-run goal value from a local file --model Override default LLM model --provider Override default LLM provider -v, --verbose Enable verbose output @@ -73,6 +123,8 @@ fn create_uses_explicit_server_target_and_prints_remote_run_id() { let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let mock = server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); then.status(201) @@ -97,6 +149,8 @@ fn create_uses_explicit_server_target_and_prints_remote_run_id() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + environment_mock.assert(); + version_mock.assert(); mock.assert(); assert_eq!(output_stdout(&output).trim(), run_id.as_str()); } @@ -106,10 +160,12 @@ fn create_defers_provider_validation_to_the_server() { let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "default", "docker"); + let registered_versions = Arc::new(Mutex::new(Vec::new())); + let version_mock = + mock_workflow_version_registrations_recording(&server, Arc::clone(®istered_versions)); let mock = server.mock(|when, then| { - when.method("POST") - .path("/api/v1/runs") - .body_includes(r#"provider=\"server-only\""#); + when.method("POST").path("/api/v1/runs"); then.status(201) .header("Content-Type", "application/json") .body(run_status_response(run_id.as_str(), "submitted").to_string()); @@ -131,7 +187,14 @@ fn create_defers_provider_validation_to_the_server() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + environment_mock.assert(); + version_mock.assert(); mock.assert(); + assert!( + serde_json::to_string(®istered_versions.lock().unwrap()[0]) + .unwrap() + .contains("server-only") + ); assert_eq!(output_stdout(&output).trim(), run_id.as_str()); } @@ -140,6 +203,8 @@ fn create_uses_configured_server_target_without_server_flag() { let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let mock = server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); then.status(201) @@ -160,17 +225,21 @@ fn create_uses_configured_server_target_without_server_flag() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + environment_mock.assert(); + version_mock.assert(); mock.assert(); assert_eq!(output_stdout(&output).trim(), run_id.as_str()); } #[test] -fn create_parent_resolves_parent_and_sends_parent_id_in_manifest() { +fn create_parent_resolves_parent_and_sends_parent_id_in_intent() { let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); let parent_id = unique_run_id(); let resolve_mock = super::support::mock_resolved_run(&server, "nightly-parent", &parent_id); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let create_mock = server.mock(|when, then| { when.method("POST") .path("/api/v1/runs") @@ -200,6 +269,8 @@ fn create_parent_resolves_parent_and_sends_parent_id_in_manifest() { String::from_utf8_lossy(&output.stderr) ); resolve_mock.assert(); + environment_mock.assert(); + version_mock.assert(); create_mock.assert(); assert_eq!(output_stdout(&output).trim(), run_id.as_str()); } @@ -237,6 +308,8 @@ fn create_cli_server_target_overrides_configured_server_target() { }); let cli_server = MockServer::start(); let run_id = unique_run_id(); + let environment_mock = mock_environment(&cli_server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&cli_server); let cli_mock = cli_server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); then.status(201) @@ -262,11 +335,745 @@ fn create_cli_server_target_overrides_configured_server_target() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + environment_mock.assert(); + version_mock.assert(); cli_mock.assert(); config_mock.assert_calls(0); assert_eq!(output_stdout(&output).trim(), run_id.as_str()); } +#[test] +fn create_sends_one_sparse_typed_intent_with_local_caller_target() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let parent_id = unique_run_id(); + let environment_mock = mock_environment(&server, "local", "local"); + let version_mock = mock_workflow_version_registrations(&server); + let resolve_mock = super::support::mock_resolved_run(&server, "parent", &parent_id); + let requests = Arc::new(Mutex::new(Vec::::new())); + let create_mock = mock_intent_create(&server, &run_id, Arc::clone(&requests)); + let caller = tempfile::tempdir().unwrap(); + let checkout = tempfile::tempdir().unwrap(); + let workflow_dir = checkout.path().join(".fabro/workflows/exact"); + std::fs::create_dir_all(&workflow_dir).unwrap(); + std::fs::write( + checkout.path().join(".fabro/project.toml"), + r#"_version = 1 + +[run.model] +name = "project-value-must-not-cross-admission" + +[environments.project-only] +provider = "local" +"#, + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.toml"), + r#"_version = 1 + +[workflow] +graph = "workflow.fabro" + +[run.pull_request] +enabled = true +draft = false +"#, + ) + .unwrap(); + std::fs::write( + workflow_dir.join("workflow.fabro"), + r#"digraph Exact { + start [shape=Mdiamond] + exit [shape=Msquare] + task [shape=parallelogram, script="echo {{ inputs.string }}"] + start -> task -> exit +}"#, + ) + .unwrap(); + std::fs::write(caller.path().join("goal.md"), "Goal read from caller cwd").unwrap(); + context.write_home( + ".fabro/settings.toml", + r#"_version = 1 + +[run.model] +name = "machine-value-must-not-cross-admission" + +[environments.machine-only] +provider = "local" +"#, + ); + let workflow = workflow_dir.join("workflow.toml"); + let expected_package = + fabro_manifest::resolve_local_workflow_package(&workflow, caller.path(), None).unwrap(); + let expected_id = expected_package.closure().root_id(); + + let output = context + .create_cmd() + .current_dir(caller.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--environment", + "local", + "--parent", + "parent", + "--goal-file", + "goal.md", + "--model", + "gpt-5", + "--provider", + "openai", + "--label", + "team=cli", + "--input", + "string=hello", + "--input", + "boolean=true", + "--input", + "integer=42", + "--input", + "float=1.25", + "--dry-run", + "--auto-approve", + "--preserve-sandbox", + "--verbose", + workflow.to_str().unwrap(), + ]) + .output() + .expect("command should execute"); + + assert!( + output.status.success(), + "command failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + environment_mock.assert(); + version_mock.assert(); + resolve_mock.assert(); + create_mock.assert_calls(1); + let requests = requests.lock().unwrap(); + let [intent] = requests.as_slice() else { + panic!("expected exactly one intent request: {requests:?}"); + }; + assert_eq!(intent["workflow_version_id"], expected_id.to_string()); + assert_eq!( + intent["target"], + json!({ + "kind": "folder", + "path": caller.path().canonicalize().unwrap(), + }) + ); + assert_eq!(intent["environment_id"], "local"); + assert_eq!(intent["parent_id"], parent_id.clone()); + assert_eq!(intent["goal"], "Goal read from caller cwd"); + assert_eq!(intent["args"]["model"], "gpt-5"); + assert_eq!(intent["args"]["provider"], "openai"); + assert_eq!( + intent["args"]["inputs"], + json!({ + "string": "hello", + "boolean": true, + "integer": 42, + "float": 1.25, + }) + ); + assert_eq!(intent["args"]["labels"]["team"], "cli"); + assert_eq!( + intent["args"]["labels"]["fabro_test_run"], + context.test_run_id() + ); + assert_eq!( + intent["args"]["labels"]["fabro_test_case"], + context.test_case_id() + ); + assert_eq!(intent["args"]["dry_run"], true); + assert_eq!(intent["args"]["auto_approve"], true); + assert_eq!(intent["args"]["preserve_sandbox"], true); + let wire = serde_json::to_string(intent).unwrap(); + for absent in [ + "goal.md", + "machine-value-must-not-cross-admission", + "project-value-must-not-cross-admission", + "workflow.toml", + "workflow.fabro", + "verbose", + ] { + assert!(!wire.contains(absent), "intent leaked {absent}: {wire}"); + } + let stderr = output_stderr(&output); + assert_eq!( + stderr + .lines() + .filter(|line| line.contains("do not transmit these settings")) + .count(), + 2, + "{stderr}" + ); + assert!(stderr.contains("contains run, environments"), "{stderr}"); + assert!(!stderr.contains("machine-value-must-not-cross-admission")); + assert!(!stderr.contains("project-value-must-not-cross-admission")); +} + +#[test] +fn create_preserves_named_user_other_checkout_and_loose_file_selection() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "local", "local"); + let version_mock = mock_workflow_version_registrations(&server); + let requests = Arc::new(Mutex::new(Vec::new())); + let create_mock = mock_intent_create(&server, &run_id, Arc::clone(&requests)); + let project = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let other_checkout = tempfile::tempdir().unwrap(); + run_git(project.path(), &["init", "--quiet"]); + let project_workflow = write_workflow(project.path(), ".fabro/workflows/hello", "ProjectHello"); + let user_root = context.home_dir.join(".fabro/workflows"); + let user_workflow = write_workflow(&user_root, "hello", "UserHello"); + let other_workflow = write_workflow( + other_checkout.path(), + ".fabro/workflows/other", + "OtherCheckout", + ); + let loose_workflow = outside.path().join("loose.fabro"); + std::fs::write( + &loose_workflow, + "digraph Loose { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", + ) + .unwrap(); + + let project_expected = fabro_manifest::resolve_local_workflow_package( + std::path::Path::new("hello"), + project.path(), + Some(&user_root), + ) + .unwrap(); + assert_eq!( + project_expected.workflow_location().toml.as_ref(), + Some(&project_workflow.canonicalize().unwrap()) + ); + let user_expected = fabro_manifest::resolve_local_workflow_package( + std::path::Path::new("hello"), + outside.path(), + Some(&user_root), + ) + .unwrap(); + assert_eq!( + user_expected.workflow_location().toml.as_ref(), + Some(&user_workflow.canonicalize().unwrap()) + ); + let other_expected = fabro_manifest::resolve_local_workflow_package( + &other_workflow, + outside.path(), + Some(&user_root), + ) + .unwrap(); + let loose_expected = fabro_manifest::resolve_local_workflow_package( + &loose_workflow, + outside.path(), + Some(&user_root), + ) + .unwrap(); + let expected = [ + project_expected.closure().root_id(), + user_expected.closure().root_id(), + other_expected.closure().root_id(), + loose_expected.closure().root_id(), + ]; + + for (cwd, workflow) in [ + (project.path(), std::path::Path::new("hello")), + (outside.path(), std::path::Path::new("hello")), + (outside.path(), other_workflow.as_path()), + (outside.path(), loose_workflow.as_path()), + ] { + let output = context + .create_cmd() + .current_dir(cwd) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--environment", + "local", + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!( + output.status.success(), + "create failed for {}:\n{}", + workflow.display(), + output_stderr(&output) + ); + } + + environment_mock.assert_calls(4); + version_mock.assert_calls(4); + create_mock.assert_calls(4); + let requests = requests.lock().unwrap(); + assert_eq!(requests.len(), expected.len()); + for (request, expected_id) in requests.iter().zip(expected) { + assert_eq!(request["workflow_version_id"], expected_id.to_string()); + } + assert_eq!( + requests[0]["target"]["path"], + project.path().canonicalize().unwrap().to_str().unwrap() + ); + for request in &requests[1..] { + assert_eq!( + request["target"]["path"], + outside.path().canonicalize().unwrap().to_str().unwrap() + ); + } +} + +#[test] +fn create_clone_targets_require_exact_git_observations() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let environment_calls = Arc::new(AtomicUsize::new(0)); + let environment_calls_for_mock = Arc::clone(&environment_calls); + let environment_mock = server.mock(|when, then| { + when.method("GET").path("/api/v1/environments/default"); + then.respond_with(move |_| { + let provider = match environment_calls_for_mock.fetch_add(1, Ordering::SeqCst) { + 0 | 2 => "docker", + 1 => "daytona", + call => panic!("unexpected environment retrieval {call}"), + }; + HttpMockResponse::builder() + .status(200) + .header("content-type", "application/json") + .body(environment_json("default", provider).to_string()) + .build() + }); + }); + let version_mock = mock_workflow_version_registrations(&server); + let requests = Arc::new(Mutex::new(Vec::new())); + let create_mock = mock_intent_create(&server, &run_id, Arc::clone(&requests)); + let fixture_root = tempfile::tempdir().unwrap(); + let workflow = write_workflow(fixture_root.path(), "workflow", "CloneTarget"); + + let exact = tempfile::tempdir().unwrap(); + let bare = fixture_root.path().join("origin.git"); + run_git(fixture_root.path(), &[ + "init", + "--bare", + "--quiet", + bare.to_str().unwrap(), + ]); + run_git(exact.path(), &[ + "-c", + "init.defaultBranch=feature", + "init", + "--quiet", + ]); + std::fs::write(exact.path().join("tracked.txt"), "tracked").unwrap(); + run_git(exact.path(), &["add", "tracked.txt"]); + run_git(exact.path(), &[ + "-c", + "user.name=test", + "-c", + "user.email=test@example.com", + "commit", + "--quiet", + "-m", + "initial", + ]); + run_git(exact.path(), &[ + "remote", + "add", + "origin", + "https://github.com/acme/widgets.git", + ]); + let local_url = format!("file://{}", bare.display()); + run_git(exact.path(), &[ + "remote", "set-url", "--push", "origin", &local_url, + ]); + std::fs::write(exact.path().join("dirty.txt"), "not committed").unwrap(); + let exact_output = context + .create_cmd() + .current_dir(exact.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!( + exact_output.status.success(), + "{}", + output_stderr(&exact_output) + ); + assert_eq!( + output_stderr(&exact_output) + .lines() + .filter(|line| line.contains("working tree is dirty")) + .count(), + 1 + ); + + let branch_only = tempfile::tempdir().unwrap(); + run_git(branch_only.path(), &[ + "-c", + "init.defaultBranch=topic", + "init", + "--quiet", + ]); + std::fs::write(branch_only.path().join("tracked.txt"), "tracked").unwrap(); + run_git(branch_only.path(), &["add", "tracked.txt"]); + run_git(branch_only.path(), &[ + "-c", + "user.name=test", + "-c", + "user.email=test@example.com", + "commit", + "--quiet", + "-m", + "initial", + ]); + run_git(branch_only.path(), &[ + "remote", + "add", + "origin", + "https://github.com/acme/missing.git", + ]); + let missing_url = format!("file://{}/missing.git", fixture_root.path().display()); + run_git(branch_only.path(), &[ + "remote", + "set-url", + "--push", + "origin", + &missing_url, + ]); + let branch_output = context + .create_cmd() + .current_dir(branch_only.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!(!branch_output.status.success()); + let branch_stderr = output_stderr(&branch_output); + assert!( + branch_stderr.contains( + "the exact local Git commit could not be made available from the canonical GitHub origin" + ), + "{branch_stderr}" + ); + assert!(!branch_stderr.contains("file://")); + assert!(!branch_stderr.contains("No such file or directory")); + + let no_repository = tempfile::tempdir().unwrap(); + let none_output = context + .create_cmd() + .current_dir(no_repository.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!( + none_output.status.success(), + "{}", + output_stderr(&none_output) + ); + + environment_mock.assert_calls(3); + version_mock.assert_calls(2); + create_mock.assert_calls(2); + let requests = requests.lock().unwrap(); + assert_eq!(requests[0]["args"]["dry_run"], true); + assert_eq!( + requests[0]["target"], + json!({ + "kind": "git", + "repo": "acme/widgets", + "branch": "feature", + "sha": run_git(exact.path(), &["rev-parse", "HEAD"]), + }) + ); + assert_eq!(requests[1]["target"], json!({ "kind": "none" })); +} + +#[test] +fn create_rejects_unusable_git_checkouts_instead_of_sending_an_empty_target() { + let context = test_context!(); + let server = MockServer::start(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); + let run_id = unique_run_id(); + let requests = Arc::new(Mutex::new(Vec::new())); + let create_mock = mock_intent_create(&server, &run_id, requests); + let fixture_root = tempfile::tempdir().unwrap(); + let workflow = write_workflow(fixture_root.path(), "workflow", "UnusableCheckout"); + + let detached = tempfile::tempdir().unwrap(); + run_git(detached.path(), &[ + "-c", + "init.defaultBranch=feature", + "init", + "--quiet", + ]); + std::fs::write(detached.path().join("tracked.txt"), "tracked").unwrap(); + run_git(detached.path(), &["add", "tracked.txt"]); + run_git(detached.path(), &[ + "-c", + "user.name=test", + "-c", + "user.email=test@example.com", + "commit", + "--quiet", + "-m", + "initial", + ]); + run_git(detached.path(), &["checkout", "--detach", "--quiet"]); + + let unborn = tempfile::tempdir().unwrap(); + run_git(unborn.path(), &[ + "-c", + "init.defaultBranch=feature", + "init", + "--quiet", + ]); + + for (working_directory, expected_error) in [ + ( + detached.path(), + "the caller Git checkout has a detached HEAD", + ), + (unborn.path(), "the caller Git checkout has no commits"), + ] { + let output = context + .create_cmd() + .current_dir(working_directory) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + + assert!(!output.status.success()); + let stderr = output_stderr(&output); + assert!(stderr.contains(expected_error), "{stderr}"); + } + + environment_mock.assert_calls(2); + version_mock.assert_calls(0); + create_mock.assert_calls(0); +} + +#[test] +fn create_rejects_an_unsupported_attached_origin_before_upload() { + let context = test_context!(); + let server = MockServer::start(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); + let run_id = unique_run_id(); + let requests = Arc::new(Mutex::new(Vec::new())); + let create_mock = mock_intent_create(&server, &run_id, requests); + let fixture_root = tempfile::tempdir().unwrap(); + let workflow = write_workflow(fixture_root.path(), "workflow", "UnsupportedOrigin"); + let workspace = tempfile::tempdir().unwrap(); + let bare = fixture_root.path().join("unsupported.git"); + run_git(fixture_root.path(), &[ + "init", + "--bare", + "--quiet", + bare.to_str().unwrap(), + ]); + run_git(workspace.path(), &[ + "-c", + "init.defaultBranch=feature", + "init", + "--quiet", + ]); + std::fs::write(workspace.path().join("tracked.txt"), "tracked").unwrap(); + run_git(workspace.path(), &["add", "tracked.txt"]); + run_git(workspace.path(), &[ + "-c", + "user.name=test", + "-c", + "user.email=test@example.com", + "commit", + "--quiet", + "-m", + "initial", + ]); + run_git(workspace.path(), &[ + "remote", + "add", + "origin", + bare.to_str().unwrap(), + ]); + + let output = context + .create_cmd() + .current_dir(workspace.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + + assert!(!output.status.success()); + assert!( + output_stderr(&output).contains("cannot be represented as a canonical GitHub run target") + ); + environment_mock.assert(); + version_mock.assert_calls(0); + create_mock.assert_calls(0); +} + +#[test] +fn create_registers_dependencies_before_the_root_and_then_creates_once() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "local", "local"); + let registrations = Arc::new(Mutex::new(Vec::new())); + let version_mock = + mock_workflow_version_registrations_recording(&server, Arc::clone(®istrations)); + let requests = Arc::new(Mutex::new(Vec::::new())); + let requests_for_mock = Arc::clone(&requests); + let registrations_for_create = Arc::clone(®istrations); + let create_response = run_status_response(run_id.as_str(), "submitted").to_string(); + let create_mock = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.respond_with(move |request| { + assert_eq!( + registrations_for_create.lock().unwrap().len(), + 2, + "intent create arrived before dependency and root registration" + ); + requests_for_mock + .lock() + .unwrap() + .push(serde_json::from_slice(request.body_ref()).unwrap()); + HttpMockResponse::builder() + .status(201) + .header("content-type", "application/json") + .body(create_response.clone()) + .build() + }); + }); + let project = tempfile::tempdir().unwrap(); + run_git(project.path(), &["init", "--quiet"]); + write_workflow(project.path(), ".fabro/workflows/root", "Root"); + write_workflow(project.path(), ".fabro/workflows/child", "Child"); + std::fs::write( + project.path().join(".fabro/workflows/root/workflow.fabro"), + r#"digraph Root { + start [shape=Mdiamond] + exit [shape=Msquare] + child [shape=house, stack.child_workflow="../child/workflow.fabro"] + start -> child -> exit +}"#, + ) + .unwrap(); + let expected = fabro_manifest::resolve_local_workflow_package( + std::path::Path::new("root"), + project.path(), + None, + ) + .unwrap(); + + let output = context + .create_cmd() + .current_dir(project.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--environment", + "local", + "root", + ]) + .output() + .unwrap(); + + assert!(output.status.success(), "{}", output_stderr(&output)); + environment_mock.assert(); + version_mock.assert_calls(2); + create_mock.assert_calls(1); + let registered_entrypoints: Vec = registrations + .lock() + .unwrap() + .iter() + .map(|version| version["entrypoint"].as_str().unwrap().to_string()) + .collect(); + assert_eq!(registered_entrypoints, [ + ".fabro/workflows/child/workflow.fabro", + ".fabro/workflows/root/workflow.fabro", + ]); + assert_eq!( + requests.lock().unwrap()[0]["workflow_version_id"], + expected.closure().root_id().to_string() + ); +} + +#[test] +fn create_stops_before_intent_create_when_registration_fails() { + let context = test_context!(); + let server = MockServer::start(); + let environment_mock = mock_environment(&server, "local", "local"); + let version_mock = server.mock(|when, then| { + when.method("POST").path("/api/v1/workflow-versions"); + then.status(409) + .header("content-type", "text/plain") + .body("workflow version rejected by fixture"); + }); + let run_id = unique_run_id(); + let requests = Arc::new(Mutex::new(Vec::new())); + let create_mock = mock_intent_create(&server, &run_id, requests); + let source = tempfile::tempdir().unwrap(); + let caller = tempfile::tempdir().unwrap(); + let workflow = write_workflow(source.path(), "workflow", "RegistrationFailure"); + + let output = context + .create_cmd() + .current_dir(caller.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--environment", + "local", + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + + assert!(!output.status.success()); + environment_mock.assert(); + version_mock.assert(); + create_mock.assert_calls(0); + let stderr = output_stderr(&output); + assert!( + stderr.contains("could not register workflow versions"), + "{stderr}" + ); + assert!(stderr.contains("index 0"), "{stderr}"); + assert!( + stderr.contains("workflow version rejected by fixture"), + "{stderr}" + ); + assert!(stderr.contains("409 Conflict"), "{stderr}"); +} + #[test] fn create_persists_directory_workflow_slug_and_cached_graph() { let context = test_context!(); @@ -290,6 +1097,8 @@ digraph BarBaz { "create", "--dry-run", "--auto-approve", + "--environment", + "local", workflow_path.to_str().unwrap(), ]) .assert() @@ -308,7 +1117,7 @@ digraph BarBaz { }), @r#" { - "workflow_slug": "sluggy", + "workflow_slug": "workflow", "graph_name": "BarBaz", "cached_graph_lines": [ "digraph BarBaz {", @@ -345,6 +1154,8 @@ digraph FooWorkflow { "create", "--dry-run", "--auto-approve", + "--environment", + "local", workflow_path.to_str().unwrap(), ]) .assert() @@ -398,7 +1209,7 @@ fn create_persists_requested_overrides_into_store() { "--provider", "openai", "--environment", - "default", + "local", "--label", "env=dev", "--label", @@ -465,8 +1276,8 @@ fn create_persists_requested_overrides_into_store() { "provider": "openai" }, "environment": { - "id": "default", - "provider": "docker", + "id": "local", + "provider": "local", "preserve": true } }, @@ -485,7 +1296,14 @@ fn create_json_does_not_imply_auto_approve() { let workflow = fixture("simple.fabro"); let output = context .command() - .args(["--json", "create", "--dry-run", workflow.to_str().unwrap()]) + .args([ + "--json", + "create", + "--dry-run", + "--environment", + "local", + workflow.to_str().unwrap(), + ]) .output() .expect("command should execute"); @@ -514,17 +1332,20 @@ fn create_json_does_not_imply_auto_approve() { #[test] fn create_invalid_workflow_fails_without_creating_run() { let context = test_context!(); + let caller = tempfile::tempdir().unwrap(); let workflow = fixture("invalid.fabro"); let initial_run_count = run_count_for_test_case(&context); let mut cmd = context.create_cmd(); - cmd.arg(workflow.to_str().unwrap()); + cmd.current_dir(caller.path()) + .args(["--quiet", workflow.to_str().unwrap()]); fabro_snapshot!(context.filters(), cmd, @" success: false exit_code: 1 ----- stdout ----- ----- stderr ----- - × Validation failed + × could not create run + ╰─▶ run intent could not be compiled: Validation failed "); let run_count = run_count_for_test_case(&context); @@ -537,17 +1358,20 @@ fn create_invalid_workflow_fails_without_creating_run() { #[test] fn create_rejects_unbound_template_inputs_without_creating_run() { let context = test_context!(); + let caller = tempfile::tempdir().unwrap(); let workflow = fixture("templated_unbound.fabro"); let initial_run_count = run_count_for_test_case(&context); let mut cmd = context.create_cmd(); - cmd.arg(workflow.to_str().unwrap()); + cmd.current_dir(caller.path()) + .args(["--quiet", workflow.to_str().unwrap()]); fabro_snapshot!(context.filters(), cmd, @" success: false exit_code: 1 ----- stdout ----- ----- stderr ----- - × Validation failed + × could not create run + ╰─▶ run intent could not be compiled: Validation failed "); let run_count = run_count_for_test_case(&context); @@ -556,3 +1380,198 @@ fn create_rejects_unbound_template_inputs_without_creating_run() { "invalid create should not persist a run for this test case" ); } + +#[test] +fn create_registers_package_before_surfacing_server_admission_rejection() { + let context = test_context!(); + let server = MockServer::start(); + let environment_mock = mock_environment(&server, "local", "local"); + let registered_versions = Arc::new(Mutex::new(Vec::new())); + let version_mock = + mock_workflow_version_registrations_recording(&server, Arc::clone(®istered_versions)); + let registered_versions_for_create = Arc::clone(®istered_versions); + let create_mock = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.respond_with(move |_| { + assert_eq!( + registered_versions_for_create.lock().unwrap().len(), + 1, + "the workflow version must be registered before server admission" + ); + HttpMockResponse::builder() + .status(422) + .header("content-type", "text/plain") + .body("server-authoritative workflow rejection") + .build() + }); + }); + let output = context + .create_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--environment", + "local", + fixture("templated_unbound.fabro").to_str().unwrap(), + ]) + .output() + .unwrap(); + assert!(!output.status.success()); + environment_mock.assert(); + version_mock.assert(); + create_mock.assert(); + let stderr = output_stderr(&output); + assert!(stderr.contains("could not create run"), "{stderr}"); + assert!( + stderr.contains("server-authoritative workflow rejection"), + "{stderr}" + ); + assert!(!stderr.contains("Validation failed"), "{stderr}"); +} + +#[test] +fn create_does_not_warn_for_client_only_settings() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "local", "local"); + let version_mock = mock_workflow_version_registrations(&server); + let requests = Arc::new(Mutex::new(Vec::new())); + let create_mock = mock_intent_create(&server, &run_id, requests); + let project = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(project.path().join(".fabro")).unwrap(); + std::fs::write( + project.path().join(".fabro/project.toml"), + "_version = 1\n\n[cli.output]\nverbosity = \"quiet\"\n", + ) + .unwrap(); + let workflow = write_workflow(project.path(), "workflow", "ClientOnly"); + context.write_home( + ".fabro/settings.toml", + "_version = 1\n\n[cli.output]\nverbosity = \"quiet\"\n", + ); + + let output = context + .create_cmd() + .current_dir(project.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--environment", + "local", + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + + assert!(output.status.success(), "{}", output_stderr(&output)); + environment_mock.assert(); + version_mock.assert(); + create_mock.assert(); + assert!(!output_stderr(&output).contains("do not transmit these settings")); +} + +#[test] +fn create_rejects_malformed_discovered_project_config_before_server_access() { + let context = test_context!(); + let server = MockServer::start(); + let any_request = server.mock(|when, then| { + when.any_request(); + then.status(500).body("server must remain untouched"); + }); + let project = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(project.path().join(".fabro")).unwrap(); + let project_config = project.path().join(".fabro/project.toml"); + std::fs::write(&project_config, "_version = 1\nrun = [").unwrap(); + let workflow = write_workflow(project.path(), "workflow", "MalformedProject"); + + let output = context + .create_cmd() + .current_dir(project.path()) + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + workflow.to_str().unwrap(), + ]) + .output() + .unwrap(); + + assert!(!output.status.success()); + let stderr = output_stderr(&output); + assert!( + stderr.contains(project_config.to_str().unwrap()), + "{stderr}" + ); + assert!( + stderr.contains("TOML") || stderr.contains("toml"), + "{stderr}" + ); + any_request.assert_calls(0); +} + +#[test] +fn create_uses_workflow_owned_pull_request_settings_only() { + let context = test_context!(); + context.ensure_home_server_auth_methods(); + let project = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(project.path().join(".fabro")).unwrap(); + std::fs::write( + project.path().join(".fabro/project.toml"), + r"_version = 1 + +[run.pull_request] +enabled = true +draft = false +", + ) + .unwrap(); + let workflow = write_workflow(project.path(), "workflow", "PullRequestAuthority"); + + let project_only = context + .create_cmd() + .current_dir(project.path()) + .args(["--environment", "local", workflow.to_str().unwrap()]) + .assert() + .success(); + assert!( + String::from_utf8_lossy(&project_only.get_output().stderr) + .contains("project.toml contains run") + ); + let project_only_id = created_run_id(project_only.get_output()); + let project_only_state = run_state(&context.find_run_dir(&project_only_id)); + assert_eq!( + project_only_state.spec.settings.run.pull_request, None, + "project-only pull-request settings must not cross intent admission" + ); + + std::fs::write( + &workflow, + r#"_version = 1 + +[workflow] +graph = "workflow.fabro" + +[run.pull_request] +enabled = true +draft = false +"#, + ) + .unwrap(); + let workflow_owned = context + .create_cmd() + .current_dir(project.path()) + .args(["--environment", "local", workflow.to_str().unwrap()]) + .assert() + .success(); + let workflow_owned_id = created_run_id(workflow_owned.get_output()); + let workflow_owned_state = run_state(&context.find_run_dir(&workflow_owned_id)); + let pull_request = workflow_owned_state + .spec + .settings + .run + .pull_request + .as_ref() + .expect("workflow.toml should configure pull-request behavior"); + assert!(pull_request.enabled); + assert!(!pull_request.draft); +} diff --git a/lib/apps/fabro-cli/tests/it/cmd/dump.rs b/lib/apps/fabro-cli/tests/it/cmd/dump.rs index cd8aff482..532257f18 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/dump.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/dump.rs @@ -168,7 +168,7 @@ fn dump_exports_blob_refs_and_artifacts_together() { ) .unwrap(); fs::write( - workspace_dir.join("run.toml"), + workspace_dir.join("workflow.toml"), r#"_version = 1 [workflow] @@ -189,7 +189,7 @@ include = ["assets/**"] let mut run_cmd = context.run_cmd(); run_cmd.current_dir(&workspace_dir); run_cmd.timeout(Duration::from_secs(30)); - run_cmd.args(["--environment", "local", "run.toml"]); + run_cmd.args(["--environment", "local", "workflow.toml"]); let run_output = run_cmd.output().expect("command should execute"); assert!( run_output.status.success(), diff --git a/lib/apps/fabro-cli/tests/it/cmd/fabro.rs b/lib/apps/fabro-cli/tests/it/cmd/fabro.rs index d4950c196..1151e285f 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/fabro.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/fabro.rs @@ -12,8 +12,8 @@ fn help() { Usage: fabro [OPTIONS] [COMMAND] Commands: - run Launch a workflow run - create Create a workflow run (allocate run dir, persist spec) + run Register a local workflow version, create a run, and start it + create Register a local workflow version and create a submitted run start Start a created workflow run on the server attach Attach to a running or finished workflow run events View the event log of a workflow run @@ -96,7 +96,7 @@ fn no_args_prints_curated_landing() { fabro validate Validate a workflow fabro preflight Validate run configuration without executing - fabro run Launch a workflow run + fabro run Register and run a local workflow Inspect runs diff --git a/lib/apps/fabro-cli/tests/it/cmd/repo_init.rs b/lib/apps/fabro-cli/tests/it/cmd/repo_init.rs index f1a5fd719..ce0d3d9a8 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/repo_init.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/repo_init.rs @@ -63,12 +63,6 @@ fn repo_init_creates_project_toml_and_hello_workflow() { # https://docs.fabro.computer/getting-started/quick-start _version = 1 - - # Auto-create pull requests on successful workflow runs. - [run.pull_request] - enabled = true - draft = true - # auto_merge = true "### ); assert_snapshot!( @@ -96,6 +90,12 @@ fn repo_init_creates_project_toml_and_hello_workflow() { [workflow] graph = "workflow.fabro" + + # Auto-create pull requests on successful workflow runs. + [run.pull_request] + enabled = true + draft = true + # auto_merge = true "### ); } diff --git a/lib/apps/fabro-cli/tests/it/cmd/run.rs b/lib/apps/fabro-cli/tests/it/cmd/run.rs index c630c6711..5f1752b38 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/run.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/run.rs @@ -10,7 +10,8 @@ use httpmock::MockServer; use serde_json::Value; use super::support::{ - created_run_id, output_stderr, remote_run_summary_json, run_state, wait_for_event_names, + created_run_id, mock_environment, mock_workflow_version_registrations, output_stderr, + remote_run_summary_json, run_state, wait_for_event_names, }; use crate::support::{LightweightCli, run_output_filters, run_projection_json, unique_run_id}; @@ -118,12 +119,12 @@ fn help() { success: true exit_code: 0 ----- stdout ----- - Launch a workflow run + Register a local workflow version, create a run, and start it Usage: fabro run [OPTIONS] Arguments: - Path to a .fabro workflow file or .toml task config + Local workflow name, checkout path, .fabro file, or workflow TOML Options: --json Output as JSON [env: FABRO_JSON=] @@ -135,7 +136,7 @@ fn help() { --auto-approve Auto-approve all human gates --quiet Suppress non-essential output [env: FABRO_QUIET=] --goal Override the workflow goal (available as {{ goal }} in prompts) - --goal-file Read the workflow goal from a file + --goal-file Read a per-run goal value from a local file --model Override default LLM model --provider Override default LLM provider -v, --verbose Enable verbose output @@ -154,6 +155,8 @@ fn detach_uses_explicit_server_target_and_prints_remote_run_id() { let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let create_mock = server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); then.status(201) @@ -188,6 +191,8 @@ fn detach_uses_explicit_server_target_and_prints_remote_run_id() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + environment_mock.assert(); + version_mock.assert(); create_mock.assert(); start_mock.assert(); assert_eq!(output_stderr(&output), ""); @@ -198,12 +203,14 @@ fn detach_uses_explicit_server_target_and_prints_remote_run_id() { } #[test] -fn run_parent_resolves_parent_and_sends_parent_id_in_manifest() { +fn run_parent_resolves_parent_and_sends_parent_id_in_intent() { let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); let parent_id = unique_run_id(); let resolve_mock = super::support::mock_resolved_run(&server, "nightly-parent", &parent_id); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let create_mock = server.mock(|when, then| { when.method("POST") .path("/api/v1/runs") @@ -243,6 +250,8 @@ fn run_parent_resolves_parent_and_sends_parent_id_in_manifest() { String::from_utf8_lossy(&output.stderr) ); resolve_mock.assert(); + environment_mock.assert(); + version_mock.assert(); create_mock.assert(); start_mock.assert(); assert_eq!( @@ -256,6 +265,8 @@ fn detach_uses_configured_server_target_without_server_flag() { let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let create_mock = server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); then.status(201) @@ -289,6 +300,8 @@ fn detach_uses_configured_server_target_without_server_flag() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + environment_mock.assert(); + version_mock.assert(); create_mock.assert(); start_mock.assert(); assert_eq!( @@ -301,6 +314,8 @@ fn detach_uses_configured_server_target_without_server_flag() { fn run_create_failure_shows_action_context_and_response_body() { let context = test_context!(); let server = MockServer::start(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let create_mock = server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); then.status(422) @@ -328,6 +343,8 @@ fn run_create_failure_shows_action_context_and_response_body() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + environment_mock.assert(); + version_mock.assert(); create_mock.assert(); let stderr = output_stderr(&output); @@ -479,6 +496,8 @@ fn detach_cli_server_target_overrides_configured_server_target() { }); let cli_server = MockServer::start(); let run_id = unique_run_id(); + let environment_mock = mock_environment(&cli_server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&cli_server); let cli_create = cli_server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); then.status(201) @@ -514,6 +533,8 @@ fn detach_cli_server_target_overrides_configured_server_target() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + environment_mock.assert(); + version_mock.assert(); cli_create.assert(); cli_start.assert(); config_create.assert_calls(0); @@ -529,6 +550,8 @@ fn remote_foreground_run_consumes_paginated_events_and_prints_server_backed_summ let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let preflight = server.mock(|when, then| { when.method("POST").path("/api/v1/preflight"); then.status(500) @@ -626,6 +649,8 @@ fn remote_foreground_run_consumes_paginated_events_and_prints_server_backed_summ String::from_utf8_lossy(&output.stderr) ); preflight.assert_calls(0); + environment_mock.assert(); + version_mock.assert(); first_page.assert(); second_page.assert(); @@ -644,14 +669,16 @@ fn remote_foreground_run_consumes_paginated_events_and_prints_server_backed_summ } #[test] -fn run_rejects_unbound_template_inputs_before_creating_remote_run() { +fn run_surfaces_server_rejection_for_unbound_template_inputs() { let context = test_context!(); let server = MockServer::start(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let create = server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); - then.status(500) - .header("Content-Type", "application/json") - .body(serde_json::json!({ "error": "run should not be created" }).to_string()); + then.status(422) + .header("Content-Type", "text/plain") + .body("server-authoritative unbound-input rejection"); }); let workflow = context.install_fixture("templated_unbound.fabro"); @@ -671,32 +698,29 @@ fn run_rejects_unbound_template_inputs_before_creating_remote_run() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); - create.assert_calls(0); + environment_mock.assert(); + version_mock.assert(); + create.assert(); let stderr = output_stderr(&output); + assert!(stderr.contains("could not create run"), "{stderr}"); assert!( - stderr.contains("inputs.app_dir"), - "stderr should name the unbound variable: {stderr}" - ); - assert!( - stderr.contains("templated_unbound.fabro"), - "stderr should name the workflow source: {stderr}" - ); - assert!( - !stderr.contains(""), - "stderr should not expose MiniJinja's generic source name: {stderr}" + stderr.contains("server-authoritative unbound-input rejection"), + "{stderr}" ); } #[test] -fn foreground_run_rejects_invalid_workflow_before_creating_remote_run() { +fn foreground_run_surfaces_server_rejection_for_invalid_workflow() { let context = test_context!(); let server = MockServer::start(); + let environment_mock = mock_environment(&server, "default", "docker"); + let version_mock = mock_workflow_version_registrations(&server); let create = server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); - then.status(500) - .header("Content-Type", "application/json") - .body(serde_json::json!({ "error": "run should not be created" }).to_string()); + then.status(422) + .header("Content-Type", "text/plain") + .body("server-authoritative invalid-workflow rejection"); }); let workflow = context.install_fixture("invalid.fabro"); @@ -716,15 +740,16 @@ fn foreground_run_rejects_invalid_workflow_before_creating_remote_run() { String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); - create.assert_calls(0); + environment_mock.assert(); + version_mock.assert(); + create.assert(); let stderr = output_stderr(&output); - assert!(stderr.contains("Workflow: Invalid"), "{stderr}"); + assert!(stderr.contains("could not create run"), "{stderr}"); assert!( - stderr.contains("Pipeline must have exactly one start node"), + stderr.contains("server-authoritative invalid-workflow rejection"), "{stderr}" ); - assert!(stderr.contains("Validation failed"), "{stderr}"); } #[test] @@ -743,7 +768,7 @@ fn local_foreground_run_prints_artifact_paths_from_server_artifact_list() { "#, ); context.write_temp( - "artifact-summary/run.toml", + "artifact-summary/workflow.toml", r#"_version = 1 [workflow] @@ -770,7 +795,7 @@ include = ["assets/**"] "local", "--provider", "openai", - "run.toml", + "workflow.toml", ]) .output() .expect("command should execute"); @@ -802,10 +827,6 @@ fn dry_run_simple() { exit_code: 0 ----- stdout ----- ----- stderr ----- - Workflow: Simple (4 nodes, 3 edges) - Graph: [GRAPH_PATH] - Goal: Run tests and report results - Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] Sandbox: local (ready in [TIME]) @@ -827,8 +848,8 @@ fn dry_run_simple() { #[test] fn dry_run_with_goal_file_reads_contents_into_goal() { // Regression test for the `--goal-file` flag that was previously - // being silently ignored in the v2 path. The file content must end - // up in the effective goal displayed in the workflow summary. + // being silently ignored in the v2 path. The file content must reach + // the server-authoritative run specification. let context = test_context!(); let goal_dir = tempfile::tempdir().unwrap(); @@ -847,10 +868,10 @@ fn dry_run_with_goal_file_reads_contents_into_goal() { "run should succeed:\nstderr:\n{}", String::from_utf8_lossy(&output.stderr) ); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!( - stderr.contains("Ship the rate-limiting feature end to end."), - "goal file content should appear in workflow summary, got:\n{stderr}" + assert_eq!( + run_state(&context.single_run_dir()).spec.graph.goal(), + "Ship the rate-limiting feature end to end.\n", + "goal file content should reach the admitted run" ); } @@ -1101,6 +1122,8 @@ fn detach_creates_run_dir_with_detach_log() { "--detach", "--dry-run", "--auto-approve", + "--environment", + "local", workflow.to_str().unwrap(), ]) .assert() diff --git a/lib/apps/fabro-cli/tests/it/cmd/start.rs b/lib/apps/fabro-cli/tests/it/cmd/start.rs index b2e587a60..e24bd4811 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/start.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/start.rs @@ -46,6 +46,8 @@ fn start_by_run_id_starts_created_run() { "create", "--dry-run", "--auto-approve", + "--environment", + "local", workflow.to_str().unwrap(), ]) .assert() @@ -96,6 +98,8 @@ fn start_by_run_id_starts_created_run_without_run_json_or_status_json() { "create", "--dry-run", "--auto-approve", + "--environment", + "local", workflow.to_str().unwrap(), ]) .assert() diff --git a/lib/apps/fabro-cli/tests/it/cmd/support.rs b/lib/apps/fabro-cli/tests/it/cmd/support.rs index c5cd16788..51e179298 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/support.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/support.rs @@ -12,6 +12,7 @@ use std::path::{Path, PathBuf}; use std::process::Output; use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; use base64::Engine as _; @@ -23,7 +24,7 @@ use fabro_store::EventEnvelope; use fabro_test::{TestContext, expect_reqwest_status}; use fabro_types::test_support::test_principal; use fabro_types::{RunId, StageId}; -use httpmock::{Mock, MockServer}; +use httpmock::{HttpMockResponse, Mock, MockServer}; use serde_json::Value; use shlex::try_quote; @@ -141,6 +142,90 @@ pub(crate) fn mock_resolved_run<'a>( }) } +/// Canonical environment response body for mock servers, matching the +/// `GET /api/v1/environments/{id}` shape the run-intent create path reads. +pub(crate) fn environment_json(id: &str, provider: &str) -> Value { + serde_json::json!({ + "id": id, + "revision": "0".repeat(64), + "provider": provider, + "image": { "docker": null, "dockerfile": null }, + "resources": { "cpu": null, "memory": null, "disk": null }, + "network": { "mode": "allow_all", "allow": [] }, + "lifecycle": { + "preserve": false, + "stop_on_terminal": true, + "auto_stop": null + }, + "labels": {}, + "env": {} + }) +} + +pub(crate) fn mock_environment<'a>(server: &'a MockServer, id: &str, provider: &str) -> Mock<'a> { + server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/environments/{id}")); + then.status(200) + .header("content-type", "application/json") + .json_body(environment_json(id, provider)); + }) +} + +pub(crate) fn mock_workflow_version_registrations(server: &MockServer) -> Mock<'_> { + mock_workflow_version_registrations_recording(server, Arc::new(Mutex::new(Vec::new()))) +} + +/// Accepts `POST /api/v1/workflow-versions`, echoing each version's +/// content-derived ID back, and records every request body into +/// `registrations` for later assertions. +pub(crate) fn mock_workflow_version_registrations_recording( + server: &MockServer, + registrations: Arc>>, +) -> Mock<'_> { + server.mock(|when, then| { + when.method("POST").path("/api/v1/workflow-versions"); + then.respond_with(move |request| { + let body: Value = serde_json::from_slice(request.body_ref()) + .expect("workflow-version request body should be valid JSON"); + let version: fabro_types::WorkflowVersion = serde_json::from_value(body.clone()) + .expect("workflow-version request body should be a workflow version"); + registrations.lock().unwrap().push(body); + HttpMockResponse::builder() + .status(201) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "workflow_version_id": version + .id() + .expect("mocked workflow version should have a valid ID") + }) + .to_string(), + ) + .build() + }); + }) +} + +/// Runs a `git` command in `path` for fixture setup, panicking on failure and +/// returning trimmed stdout. +pub(crate) fn run_git(path: &Path, args: &[&str]) -> String { + let output = std::process::Command::new("git") + .args(args) + .current_dir(path) + .output() + .expect("Git fixture command should execute"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout) + .expect("Git fixture output should be UTF-8") + .trim() + .to_string() +} + /// Snapshot filter that scrubs short (12-char) ULID suffixes from output, used /// when the CLI prints abbreviated run IDs. pub(crate) fn ulid_filter() -> (String, String) { @@ -397,7 +482,7 @@ pub(crate) fn setup_local_sandbox_run(context: &TestContext) -> WorkspaceRunSetu "#, ); write_text_file( - &workspace_dir.join("run.toml"), + &workspace_dir.join("workflow.toml"), r#"_version = 1 [workflow] @@ -412,7 +497,7 @@ id = "local" "#, ); - let run = run_local_workflow(context, &workspace_dir, "run.toml"); + let run = run_local_workflow(context, &workspace_dir, "workflow.toml"); assert!(run_state(&run.run_dir).sandbox.is_some()); WorkspaceRunSetup { run, workspace_dir } diff --git a/lib/apps/fabro-cli/tests/it/scenario/lifecycle.rs b/lib/apps/fabro-cli/tests/it/scenario/lifecycle.rs index 9f8e7b7a7..4d805e088 100644 --- a/lib/apps/fabro-cli/tests/it/scenario/lifecycle.rs +++ b/lib/apps/fabro-cli/tests/it/scenario/lifecycle.rs @@ -124,6 +124,8 @@ fn dry_run_create_start_attach_works_with_default_run_lookup() { "create", "--dry-run", "--auto-approve", + "--environment", + "local", workflow.to_str().unwrap(), ]) .assert() @@ -219,7 +221,7 @@ fn dry_run_detach_attach_works_with_default_run_lookup() { } #[test] -fn completed_run_can_be_attached_by_workflow_slug() { +fn completed_run_can_be_attached_by_entrypoint_slug() { let context = test_context!(); context.ensure_home_server_auth_methods(); let project = tempfile::tempdir().unwrap(); @@ -246,6 +248,8 @@ digraph BarBaz { "create", "--dry-run", "--auto-approve", + "--environment", + "local", workflow_path.to_str().unwrap(), ]) .assert() @@ -254,7 +258,7 @@ digraph BarBaz { context .command() .current_dir(project.path()) - .args(["start", "sluggy"]) + .args(["start", "workflow"]) .assert() .success(); context @@ -267,7 +271,7 @@ digraph BarBaz { context .command() .current_dir(project.path()) - .args(["attach", "sluggy"]) + .args(["attach", "workflow"]) .timeout(std::time::Duration::from_secs(10)) .assert() .success(); @@ -314,6 +318,8 @@ digraph FooWorkflow { "create", "--dry-run", "--auto-approve", + "--environment", + "local", workflow_path.to_str().unwrap(), ]) .assert() diff --git a/lib/apps/fabro-cli/tests/it/workflow/acp.rs b/lib/apps/fabro-cli/tests/it/workflow/acp.rs index 9cd0845ae..f01472e03 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/acp.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/acp.rs @@ -175,7 +175,7 @@ fn acp_artifacts_are_listed_when_touched_file_mtime_precedes_attempt_start() { ), ); context.write_temp( - "run.toml", + "workflow.toml", r#"_version = 1 [workflow] @@ -196,7 +196,7 @@ include = ["verification-artifacts/**"] context .run_cmd() .args(["--auto-approve", "--environment", "local"]) - .arg(context.temp_dir.join("run.toml")) + .arg(context.temp_dir.join("workflow.toml")) .assert() .success(); diff --git a/lib/apps/fabro-cli/tests/it/workflow/artifacts.rs b/lib/apps/fabro-cli/tests/it/workflow/artifacts.rs index 71f57ade6..0e1f24ec3 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/artifacts.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/artifacts.rs @@ -29,7 +29,7 @@ fn unchanged_matching_artifact_is_captured_once_across_stages() { "#, ); context.write_temp( - "run.toml", + "workflow.toml", r#"_version = 1 [workflow] @@ -50,7 +50,7 @@ include = ["assets/**"] context .run_cmd() .args(["--auto-approve", "--environment", "local"]) - .arg(context.temp_dir.join("run.toml")) + .arg(context.temp_dir.join("workflow.toml")) .assert() .success(); diff --git a/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs b/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs index 496a7de04..a6facf8d0 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs @@ -14,12 +14,6 @@ fn dry_run_branching() { exit_code: 0 ----- stdout ----- ----- stderr ----- - Workflow: Branch (6 nodes, 6 edges) - Graph: [GRAPH_PATH] - Goal: Implement and validate a feature - - warning [node: implement]: Node 'implement' has goal_gate=true but no retry_target or fallback_retry_target (goal_gate_has_retry) - fix: Add retry_target or fallback_retry_target attribute Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] Sandbox: local (ready in [TIME]) @@ -52,10 +46,6 @@ fn dry_run_conditions() { exit_code: 0 ----- stdout ----- ----- stderr ----- - Workflow: Conditions (5 nodes, 5 edges) - Graph: [GRAPH_PATH] - Goal: Test condition evaluation with OR and parentheses - Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] Sandbox: local (ready in [TIME]) @@ -88,10 +78,6 @@ fn dry_run_parallel() { exit_code: 0 ----- stdout ----- ----- stderr ----- - Workflow: Parallel (7 nodes, 7 edges) - Graph: [GRAPH_PATH] - Goal: Test parallel and fan-in execution - Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] Sandbox: local (ready in [TIME]) @@ -125,10 +111,6 @@ fn dry_run_styled() { exit_code: 0 ----- stdout ----- ----- stderr ----- - Workflow: Styled (5 nodes, 4 edges) - Graph: [GRAPH_PATH] - Goal: Build a styled pipeline - Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] Sandbox: local (ready in [TIME]) @@ -160,10 +142,6 @@ fn dry_run_inferred_command() { exit_code: 0 ----- stdout ----- ----- stderr ----- - Workflow: InferredCommand (3 nodes, 2 edges) - Graph: [GRAPH_PATH] - Goal: Verify a shapeless script node runs as a command - Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] Sandbox: local (ready in [TIME]) diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index ef7056fb1..46acefeba 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -665,8 +665,12 @@ pub(crate) fn sandbox_provider_policy_error( }) } +pub(crate) fn configured_sandbox_provider(settings: &RunNamespace) -> SandboxProviderKind { + SandboxProviderKind::from(settings.environment.provider) +} + pub(crate) fn effective_sandbox_provider(settings: &RunNamespace) -> SandboxProviderKind { - SandboxProviderKind::from(settings.environment.provider).effective_for(settings.execution.mode) + configured_sandbox_provider(settings).effective_for(settings.execution.mode) } fn resolve_daytona_config(settings: &RunNamespace) -> DaytonaConfig { diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 2864c4bcf..236e0ec63 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -18,7 +18,7 @@ use fabro_api::types::{ BoardColumn, ManifestConfigType, ManifestGoalType, RunIntent, RunManifest, SubmitAnswerRequest, UpdateRunParentRequest, UpdateRunRequest, }; -use fabro_config::{CliLayer, RunLayer, Storage}; +use fabro_config::{CliLayer, RunLayer, Storage, project}; use fabro_environment::{DEFAULT_ENVIRONMENT_ID, EnvironmentId}; use fabro_interview::AnswerSubmission; use fabro_llm::client::Client as LlmClient; @@ -704,6 +704,7 @@ pub(crate) async fn create_run_from_intent( }); let entrypoint = lowered.entrypoint.clone(); + let workflow_slug = project::workflow_slug_from_path(entrypoint.as_path()); let raw_compiler_input = RawRunCompilerInput { workflow_bundle: lowered.workflow_bundle, entrypoint: lowered.entrypoint, @@ -729,7 +730,7 @@ pub(crate) async fn create_run_from_intent( // admission via `with_target_and_git`; the compiler never reads them. git: None, storage_root: state.server_storage_dir(), - workflow_slug: None, + workflow_slug, workflow_version_id: Some(intent.workflow_version_id), target: None, provenance: run_provenance(&headers, &actor), @@ -1093,23 +1094,24 @@ async fn validate_intent_environment( settings: &fabro_types::WorkflowSettings, target: &RunTarget, ) -> Result<(), EnvironmentSelectionError> { - let provider = run_manifest::effective_sandbox_provider(&settings.run); + let configured_provider = run_manifest::configured_sandbox_provider(&settings.run); + let effective_provider = run_manifest::effective_sandbox_provider(&settings.run); let image = &settings.run.environment.image; - let image_incompatible = match provider { + let image_incompatible = match effective_provider { SandboxProviderKind::Docker => image.docker.is_none() && image.dockerfile.is_some(), SandboxProviderKind::Local | SandboxProviderKind::Daytona => false, }; let (target_incompatible, detail) = match target { RunTarget::Git(_) => ( - provider == SandboxProviderKind::Local || !settings.run.clone.enabled, + configured_provider == SandboxProviderKind::Local || !settings.run.clone.enabled, "Git targets require a compatible clone-enabled Docker or Daytona environment", ), RunTarget::None {} => ( - provider == SandboxProviderKind::Local, + configured_provider == SandboxProviderKind::Local, "none targets require a compatible Docker or Daytona environment", ), RunTarget::Folder { .. } => ( - provider != SandboxProviderKind::Local, + configured_provider != SandboxProviderKind::Local, "folder targets require a Local environment", ), }; @@ -1117,17 +1119,20 @@ async fn validate_intent_environment( return Err(EnvironmentSelectionError::TargetUnsupported { detail }); } if let Some(detail) = - run_manifest::sandbox_provider_policy_error(&state.server_settings(), provider) + run_manifest::sandbox_provider_policy_error(&state.server_settings(), effective_provider) { - return Err(EnvironmentSelectionError::ProviderDisabled { provider, detail }); + return Err(EnvironmentSelectionError::ProviderDisabled { + provider: effective_provider, + detail, + }); } - if provider == SandboxProviderKind::Daytona { + if effective_provider == SandboxProviderKind::Daytona { match state.vault_secret(EnvVars::DAYTONA_API_KEY).await { Ok(Some(key)) if !key.trim().is_empty() => {} Ok(_) => { return Err(EnvironmentSelectionError::MissingCredential { - provider, - name: EnvVars::DAYTONA_API_KEY, + provider: effective_provider, + name: EnvVars::DAYTONA_API_KEY, }); } Err(source) => { diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 9533e0d2f..7f23d1989 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -3564,10 +3564,17 @@ async fn store_workflow_version( graph: &str, workflow_toml: Option<&str>, ) -> fabro_types::WorkflowVersionId { - let mut files = std::collections::BTreeMap::from([( - fabro_types::WorkflowPath::new("workflow.fabro").unwrap(), - graph.to_string(), - )]); + store_workflow_version_with_entrypoint(state, "workflow.fabro", graph, workflow_toml).await +} + +async fn store_workflow_version_with_entrypoint( + state: &AppState, + entrypoint: &str, + graph: &str, + workflow_toml: Option<&str>, +) -> fabro_types::WorkflowVersionId { + let entrypoint = fabro_types::WorkflowPath::new(entrypoint).unwrap(); + let mut files = std::collections::BTreeMap::from([(entrypoint.clone(), graph.to_string())]); if let Some(workflow_toml) = workflow_toml { files.insert( fabro_types::WorkflowPath::new("workflow.toml").unwrap(), @@ -3582,12 +3589,9 @@ async fn store_workflow_version( "FROM alpine:3".to_string(), ); } - let version = fabro_types::WorkflowVersion::new( - fabro_types::WorkflowPath::new("workflow.fabro").unwrap(), - files, - std::collections::BTreeMap::new(), - ) - .unwrap(); + let version = + fabro_types::WorkflowVersion::new(entrypoint, files, std::collections::BTreeMap::new()) + .unwrap(); let version = fabro_workflow_version::ValidatedWorkflowVersion::new(version).unwrap(); let blobs = state.store_ref().blobs(); fabro_workflow_version::WorkflowVersionStore::new(blobs) @@ -3596,6 +3600,48 @@ async fn store_workflow_version( .unwrap() } +#[tokio::test] +async fn post_runs_run_intent_derives_workflow_slug_from_immutable_entrypoint() { + let state = test_app_state(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + + for (entrypoint, expected_slug) in [ + ("deploy/workflow.fabro", "deploy"), + ("workflow.fabro", "workflow"), + ] { + let workflow_version_id = + store_workflow_version_with_entrypoint(&state, entrypoint, MINIMAL_DOT, None).await; + let body = post_run_manifest( + &app, + json!({ + "workflow_version_id": workflow_version_id, + "target": { "kind": "none" }, + "args": {} + }), + ) + .await; + let run_id = body["id"].as_str().unwrap().parse::().unwrap(); + let projection = state + .stores + .runs + .open_run_reader(&run_id) + .await + .unwrap() + .state() + .await + .unwrap(); + + assert_eq!( + projection.spec.workflow_slug.as_deref(), + Some(expected_slug) + ); + assert_eq!( + projection.spec.workflow_version_id, + Some(workflow_version_id) + ); + } +} + #[tokio::test] async fn post_runs_run_intent_persists_tagged_exact_git_target_without_starting() { let state = test_app_state(); @@ -3814,6 +3860,234 @@ async fn post_runs_run_intent_args_true_override_resolved_settings_without_start assert!(projection.spec.settings.run.environment.lifecycle.preserve); } +#[tokio::test] +async fn post_runs_run_intent_dry_run_uses_configured_target_provider() { + let folder = tempfile::tempdir().unwrap(); + let folder_path = folder + .path() + .canonicalize() + .unwrap() + .to_string_lossy() + .into_owned(); + let cases = [ + ( + test_app_state(), + None, + json!({ + "kind": "git", + "repo": "fabro-sh/fabro", + "branch": "main" + }), + json!({ "dry_run": true }), + ), + ( + test_app_state(), + None, + json!({ "kind": "none" }), + json!({ "dry_run": true }), + ), + ( + TestAppStateBuilder::new() + .default_environment_provider(Some(EnvironmentProvider::Daytona)) + .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) + .build(), + Some("_version = 1\n[run.execution]\nmode = \"dry_run\"\n"), + json!({ "kind": "none" }), + json!({}), + ), + ( + TestAppStateBuilder::new() + .default_environment_provider(Some(EnvironmentProvider::Daytona)) + .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) + .build(), + Some("_version = 1\n[run.execution]\nmode = \"dry_run\"\n"), + json!({ + "kind": "git", + "repo": "fabro-sh/fabro", + "branch": "main" + }), + json!({}), + ), + ( + TestAppStateBuilder::new() + .runtime_settings( + default_test_server_settings(), + manifest_run_defaults_from_toml("[run.execution]\nmode = \"dry_run\"\n"), + ) + .default_environment_provider(Some(EnvironmentProvider::Local)) + .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) + .build(), + None, + json!({ "kind": "folder", "path": folder_path }), + json!({}), + ), + ]; + + for (state, workflow_toml, target, args) in cases { + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, workflow_toml).await; + let body = post_run_manifest( + &app, + json!({ + "workflow_version_id": workflow_version_id, + "target": target, + "args": args + }), + ) + .await; + let run_id = body["id"].as_str().unwrap().parse::().unwrap(); + let projection = state + .stores + .runs + .open_run_reader(&run_id) + .await + .unwrap() + .state() + .await + .unwrap(); + + assert_eq!(projection.spec.settings.run.execution.mode, RunMode::DryRun); + assert_eq!( + serde_json::to_value(projection.spec.target.unwrap()).unwrap(), + target + ); + } +} + +#[tokio::test] +async fn post_runs_run_intent_dry_run_rejects_configured_target_mismatches() { + let states_and_targets = [ + ( + local_test_app_state(), + json!({ + "kind": "git", + "repo": "fabro-sh/fabro", + "branch": "main" + }), + ), + (local_test_app_state(), json!({ "kind": "none" })), + ( + test_app_state(), + json!({ "kind": "folder", "path": "/path-that-must-not-be-read" }), + ), + ( + TestAppStateBuilder::new() + .default_environment_provider(Some(EnvironmentProvider::Daytona)) + .vault_entries([(fabro_static::EnvVars::OPENAI_API_KEY, "test-openai-api-key")]) + .build(), + json!({ "kind": "folder", "path": "/path-that-must-not-be-read" }), + ), + ]; + + for (state, target) in states_and_targets { + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; + let response = post_run_intent_response( + &app, + json!({ + "workflow_version_id": workflow_version_id, + "target": target, + "args": { "dry_run": true } + }), + ) + .await; + let body = response_json!(response, StatusCode::UNPROCESSABLE_ENTITY).await; + + assert_eq!(body["errors"][0]["code"], "target_environment_unsupported"); + assert!( + state + .stores + .run_summaries + .list_identities() + .await + .unwrap() + .is_empty() + ); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn post_runs_run_intent_dry_run_starts_in_isolated_scratch_workspace() { + let source = r#" +_version = 1 + +[server.auth] +methods = ["dev-token"] + +[[run.prepare.steps]] +script = "pwd > setup-working-directory.txt" +"#; + let state = test_app_state_with_settings_and_registry_factory( + server_settings_from_toml(source), + manifest_run_defaults_from_toml(source), + |interviewer| fabro_workflow::handler::default_registry(interviewer, || None), + ); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let external_target = tempfile::tempdir().unwrap(); + let external_sentinel = external_target.path().join("existing-target-file.txt"); + tokio::fs::write(&external_sentinel, b"must remain unchanged") + .await + .unwrap(); + let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; + let body = post_run_manifest( + &app, + json!({ + "workflow_version_id": workflow_version_id, + "target": { + "kind": "git", + "repo": "fabro-sh/fabro", + "branch": "main" + }, + "args": { "dry_run": true } + }), + ) + .await; + let run_id = body["id"].as_str().unwrap().parse::().unwrap(); + + let response = app + .oneshot( + Request::builder() + .method("POST") + .uri(api(&format!("/runs/{run_id}/start"))) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + response_json!(response, StatusCode::OK).await; + + execute_run(Arc::clone(&state), run_id).await; + + let run_store = state.stores.runs.open_run_reader(&run_id).await.unwrap(); + assert_eq!( + run_store.state().await.unwrap().status, + RunStatus::Succeeded { + reason: SuccessReason::Completed, + } + ); + let scratch_workspace = Storage::new(state.server_storage_dir()) + .run_scratch(&run_id) + .root() + .join("dry-run-workspace") + .canonicalize() + .unwrap(); + let setup_working_directory = + tokio::fs::read_to_string(scratch_workspace.join("setup-working-directory.txt")) + .await + .unwrap(); + assert_eq!(Path::new(setup_working_directory.trim()), scratch_workspace); + assert_eq!( + tokio::fs::read(&external_sentinel).await.unwrap(), + b"must remain unchanged" + ); + assert!( + !external_target + .path() + .join("setup-working-directory.txt") + .exists() + ); +} + #[tokio::test] async fn post_runs_run_intent_args_false_are_distinct_from_omitted_overrides() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 20edc4800..520e09a18 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -403,7 +403,14 @@ impl RunSession { .state() .await .map_err(|err| Error::engine(err.to_string()))?; - let git = git_checkpoint_options_from_start(settings, &record.run_id, state.start); + let dry_run_clone_target = settings.run.execution.mode == RunMode::DryRun + && matches!( + record.target.as_ref(), + Some(RunTarget::Git(_) | RunTarget::None {}) + ); + let git = (!dry_run_clone_target) + .then(|| git_checkpoint_options_from_start(settings, &record.run_id, state.start)) + .flatten(); let definition_blob = state.spec.definition_blob; let accepted_definition = match definition_blob { Some(blob_hash) => { @@ -418,12 +425,27 @@ impl RunSession { accepted_definition.map(|definition| Arc::new(definition.workflow_bundle())); let resolved = &settings.run; - let sandbox_provider = - resolve_sandbox_provider(resolved).effective_for(resolved.execution.mode); - let clone_source = clone_source_for_run(record)?; - // An empty-workspace run has no repository for PR creation or the - // sandbox environment, regardless of any persisted Git metadata. - let runtime_origin_url = (!clone_source.skip_clone) + let configured_sandbox_provider = resolve_sandbox_provider(resolved); + let sandbox_provider = configured_sandbox_provider.effective_for(resolved.execution.mode); + let clone_source = if dry_run_clone_target { + CloneSourceForRun { + origin_url: None, + branch: None, + tag: None, + commit_sha: None, + skip_clone: true, + } + } else { + clone_source_for_run(record)? + }; + // Clone avoidance and repository identity are independent for Local + // folder targets: their files are already present, but GitHub tokens + // and pull-request publication still need the persisted origin. Only + // an explicit empty target or a clone-target dry-run uses a repository- + // free scratch workspace. + let repository_free_workspace = + dry_run_clone_target || matches!(record.target.as_ref(), Some(RunTarget::None {})); + let runtime_origin_url = (!repository_free_workspace) .then(|| record.repo_origin_url().map(str::to_string)) .flatten(); let catalog = Arc::clone(&services.catalog); @@ -463,14 +485,26 @@ impl RunSession { }) .collect::, _>>()?; - if sandbox_provider != SandboxProviderKind::Local + if configured_sandbox_provider != SandboxProviderKind::Local && matches!(record.target, Some(RunTarget::Folder { .. })) { return Err(Error::engine( "persisted folder run targets require the Local sandbox provider", )); } + if configured_sandbox_provider == SandboxProviderKind::Local { + if let Some(target @ (RunTarget::Git(_) | RunTarget::None {})) = record.target.as_ref() + { + return Err(Error::engine(format!( + "persisted {} run targets require a clone-based sandbox provider", + target.kind_name() + ))); + } + } let sandbox = match sandbox_provider { + SandboxProviderKind::Local if dry_run_clone_target => SandboxSpec::Local { + working_directory: dry_run_workspace_for_target(persisted).await?, + }, SandboxProviderKind::Local => match record.target.as_ref() { Some(target @ (RunTarget::Git(_) | RunTarget::None {})) => { return Err(Error::engine(format!( @@ -652,6 +686,16 @@ async fn folder_working_directory_from_record( Ok(canonical) } +async fn dry_run_workspace_for_target(persisted: &Persisted) -> Result { + let workspace = persisted.run_dir().join("dry-run-workspace"); + fs::create_dir_all(&workspace).await.map_err(|source| { + Error::engine_with_source("failed to create dry-run target workspace", source) + })?; + fs::canonicalize(&workspace).await.map_err(|source| { + Error::engine_with_source("failed to canonicalize dry-run target workspace", source) + }) +} + fn clone_source_for_run(record: &RunSpec) -> Result { let Some(target) = &record.target else { return Ok(CloneSourceForRun { @@ -1276,8 +1320,8 @@ mod tests { RunMode, RunPrepareSettings, }; use fabro_types::{ - BilledModelUsage, ManifestPath, RunTarget, StageTiming, WorkflowSettings, fixtures, - test_support, + BilledModelUsage, GitContext, ManifestPath, RunTarget, StageTiming, WorkflowSettings, + fixtures, test_support, }; use fabro_vault::SecretType; use object_store::memory::InMemory; @@ -2017,7 +2061,125 @@ reasoning = false } #[tokio::test] - async fn run_session_new_folder_target_uses_canonical_path_over_environment_cwd() { + async fn run_session_new_dry_run_clone_targets_use_isolated_local_workspace() { + for target in [ + RunTarget::None {}, + RunTarget::Git(GitRunTarget { + repo: "fabro-sh/fabro".to_string(), + branch: "main".to_string(), + tag: None, + sha: Some("0123456789abcdef0123456789abcdef01234567".to_string()), + }), + ] { + let temp = tempfile::tempdir().unwrap(); + let (storage_root, run_dir) = storage_root_and_run_dir(&temp); + let mut settings = settings_from_run_layer(RunLayer { + execution: Some(RunExecutionLayer { + mode: Some(RunMode::DryRun), + ..RunExecutionLayer::default() + }), + ..RunLayer::default() + }); + settings.run.environment.provider = EnvironmentProvider::Docker; + settings.run.environment.image.docker = Some("buildpack-deps:noble".to_string()); + let (persisted, store) = persisted_workflow_with_settings_and_target( + MINIMAL_DOT, + &storage_root, + settings, + Some(target.clone()), + ) + .await; + assert_eq!(persisted.run_spec().target, Some(target.clone())); + let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); + let registry = Arc::new(test_registry()); + + let session = RunSession::new( + &persisted, + test_start_services(&store, &storage_root, emitter, registry).await, + ) + .await + .unwrap(); + + let SandboxSpec::Local { working_directory } = session.sandbox else { + panic!("clone target dry-run should execute in a Local scratch sandbox"); + }; + assert_eq!( + working_directory, + run_dir.join("dry-run-workspace").canonicalize().unwrap() + ); + assert_eq!(session.sandbox_env.origin_url, None); + assert_eq!(session.pr_origin_url, None); + assert!(session.git.is_none()); + assert_eq!(persisted.run_spec().target, Some(target)); + } + } + + #[tokio::test] + async fn run_session_new_dry_run_rejects_configured_target_mismatches() { + let temp = tempfile::tempdir().unwrap(); + let (storage_root, _run_dir) = storage_root_and_run_dir(&temp); + let mut local_settings = settings_from_run_layer(RunLayer { + execution: Some(RunExecutionLayer { + mode: Some(RunMode::DryRun), + ..RunExecutionLayer::default() + }), + ..RunLayer::default() + }); + local_settings.run.environment.provider = EnvironmentProvider::Local; + let (persisted, store) = persisted_workflow_with_settings_and_target( + MINIMAL_DOT, + &storage_root, + local_settings, + Some(RunTarget::None {}), + ) + .await; + let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); + let registry = Arc::new(test_registry()); + let Err(error) = RunSession::new( + &persisted, + test_start_services(&store, &storage_root, emitter, registry).await, + ) + .await + else { + panic!("Local configured provider must reject none even in dry-run"); + }; + assert!(error.to_string().contains("none run targets require")); + + let temp = tempfile::tempdir().unwrap(); + let (storage_root, _run_dir) = storage_root_and_run_dir(&temp); + let (_, canonical_text) = canonical_folder(&temp); + let mut docker_settings = settings_from_run_layer(RunLayer { + execution: Some(RunExecutionLayer { + mode: Some(RunMode::DryRun), + ..RunExecutionLayer::default() + }), + ..RunLayer::default() + }); + docker_settings.run.environment.provider = EnvironmentProvider::Docker; + let (persisted, store) = + persisted_workflow_with_settings(MINIMAL_DOT, &storage_root, docker_settings).await; + let persisted = persisted_with_target_projection( + persisted, + RunTarget::Folder { + path: canonical_text.clone(), + }, + Some(canonical_text), + ); + let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); + let registry = Arc::new(test_registry()); + let Err(error) = RunSession::new( + &persisted, + test_start_services(&store, &storage_root, emitter, registry).await, + ) + .await + else { + panic!("Docker configured provider must reject folder even in dry-run"); + }; + assert!(error.to_string().contains("folder run targets require")); + } + + #[tokio::test] + async fn run_session_new_folder_target_uses_canonical_path_and_preserves_git_identity() { let temp = tempfile::tempdir().unwrap(); let (storage_root, _run_dir) = storage_root_and_run_dir(&temp); let (canonical_folder, canonical_text) = canonical_folder(&temp); @@ -2035,6 +2197,13 @@ reasoning = false }, Some(canonical_text), ); + let origin_url = "https://github.com/acme/widgets"; + let persisted = persisted_with_git_projection(persisted, GitContext { + origin_url: origin_url.to_string(), + branch: "feature".to_string(), + sha: Some("0123456789abcdef0123456789abcdef01234567".to_string()), + dirty: fabro_types::DirtyStatus::Clean, + }); let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); let registry = Arc::new(test_registry()); @@ -2050,6 +2219,8 @@ reasoning = false }; assert_eq!(working_directory, canonical_folder); assert_ne!(working_directory, environment_cwd); + assert_eq!(session.sandbox_env.origin_url.as_deref(), Some(origin_url)); + assert_eq!(session.pr_origin_url.as_deref(), Some(origin_url)); } #[tokio::test] @@ -2340,6 +2511,12 @@ reasoning = false Persisted::new(graph, source, diagnostics, run_dir, run_spec) } + fn persisted_with_git_projection(persisted: Persisted, git: GitContext) -> Persisted { + let (graph, source, diagnostics, run_dir, mut run_spec) = persisted.into_parts(); + run_spec.git = Some(git); + Persisted::new(graph, source, diagnostics, run_dir, run_spec) + } + /// Create `folder-target` under `temp` and return its canonical path and /// the UTF-8 text a persisted folder target would carry. fn canonical_folder(temp: &tempfile::TempDir) -> (PathBuf, String) { diff --git a/lib/foundation/fabro-client/src/client.rs b/lib/foundation/fabro-client/src/client.rs index 93fd868f4..9ed8e88c1 100644 --- a/lib/foundation/fabro-client/src/client.rs +++ b/lib/foundation/fabro-client/src/client.rs @@ -701,6 +701,17 @@ impl Client { self.submit_create_run(manifest.into()).await } + /// Retrieves one canonical server-managed environment by ID. + pub async fn retrieve_environment(&self, id: &str) -> Result { + let response = self + .send_api(|client| { + let id = id.to_string(); + async move { client.retrieve_environment().id(id).send().await } + }) + .await?; + Ok(response.into_inner()) + } + /// Registers one workflow version and verifies the server assigned the /// content-derived id, so a mismatched response fails loudly here rather /// than being trusted downstream. @@ -2326,6 +2337,7 @@ mod tests { use chrono::Duration as ChronoDuration; use fabro_types::WorkflowPath; + use fabro_types::settings::run::EnvironmentProvider; use fabro_util::exit; use httpmock::Method::{GET, POST}; use httpmock::{HttpMockResponse, MockServer}; @@ -2410,6 +2422,72 @@ mod tests { .build() } + fn environment_json(id: &str, provider: &str) -> serde_json::Value { + json!({ + "id": id, + "revision": "0".repeat(64), + "provider": provider, + "image": { "docker": null, "dockerfile": null }, + "resources": { "cpu": null, "memory": null, "disk": null }, + "network": { "mode": "allow_all", "allow": [] }, + "lifecycle": { + "preserve": false, + "stop_on_terminal": true, + "auto_stop": null + }, + "labels": {}, + "env": {} + }) + } + + #[tokio::test] + async fn retrieve_environment_returns_the_canonical_environment() { + let server = MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(GET).path("/api/v1/environments/local"); + then.status(200) + .header("content-type", "application/json") + .json_body(environment_json("local", "local")); + }) + .await; + let client = Client::new_no_proxy(&server.url("")).unwrap(); + + let environment = client.retrieve_environment("local").await.unwrap(); + + mock.assert_async().await; + assert_eq!(environment.id.as_str(), "local"); + assert_eq!(environment.settings.provider, EnvironmentProvider::Local); + } + + #[tokio::test] + async fn retrieve_environment_preserves_api_failure_metadata() { + let server = MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method(GET).path("/api/v1/environments/missing"); + then.status(404) + .header("content-type", "application/json") + .json_body(json!({ + "errors": [{ + "status": "404", + "title": "Not Found", + "detail": "environment not found", + "code": "environment_not_found" + }] + })); + }) + .await; + let client = Client::new_no_proxy(&server.url("")).unwrap(); + + let error = client.retrieve_environment("missing").await.unwrap_err(); + + mock.assert_async().await; + let failure = api_failure_for(&error).expect("API failure metadata should be preserved"); + assert_eq!(failure.status, fabro_http::StatusCode::NOT_FOUND); + assert_eq!(failure.code.as_deref(), Some("environment_not_found")); + } + #[tokio::test] async fn create_workflow_version_posts_exact_version_and_returns_server_id() { let server = MockServer::start_async().await; diff --git a/lib/foundation/fabro-types/src/input_scalar.rs b/lib/foundation/fabro-types/src/input_scalar.rs index 512b9d407..726233dab 100644 --- a/lib/foundation/fabro-types/src/input_scalar.rs +++ b/lib/foundation/fabro-types/src/input_scalar.rs @@ -17,6 +17,46 @@ pub enum JsonScalarToTomlError { NumberOutOfRange, } +/// The reason a parsed TOML value cannot be represented as a JSON scalar. +#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] +pub enum TomlScalarToJsonError { + /// The TOML float is not finite, and JSON numbers must be finite. + #[error("must be a finite float")] + NonFiniteFloat, + /// TOML datetimes are outside the scalar-only conversion contract. + #[error("must be a scalar value")] + Datetime, + /// TOML arrays are outside the scalar-only conversion contract. + #[error("must be a scalar value")] + Array, + /// TOML tables are outside the scalar-only conversion contract. + #[error("must be a scalar value")] + Table, +} + +/// Converts an already-parsed TOML scalar into a JSON value. +/// +/// The inverse of [`json_scalar_to_toml_value`]: strings, booleans, and +/// integers map directly, and finite floats become JSON numbers. +/// +/// # Errors +/// +/// Returns [`TomlScalarToJsonError`] for TOML datetimes, arrays, tables, or a +/// non-finite float (JSON numbers must be finite). +pub fn toml_scalar_to_json_value(value: &toml::Value) -> Result { + match value { + toml::Value::String(value) => Ok(Value::String(value.clone())), + toml::Value::Integer(value) => Ok(Value::Number((*value).into())), + toml::Value::Float(value) => serde_json::Number::from_f64(*value) + .map(Value::Number) + .ok_or(TomlScalarToJsonError::NonFiniteFloat), + toml::Value::Boolean(value) => Ok(Value::Bool(*value)), + toml::Value::Datetime(_) => Err(TomlScalarToJsonError::Datetime), + toml::Value::Array(_) => Err(TomlScalarToJsonError::Array), + toml::Value::Table(_) => Err(TomlScalarToJsonError::Table), + } +} + /// Converts an already-parsed JSON scalar into a TOML value. /// /// Numbers are converted to a signed integer first and then to a float. As a @@ -50,7 +90,81 @@ pub fn json_scalar_to_toml_value(value: &Value) -> Result Result<(), TomlScalarToJsonError> { + let cases = [ + (toml::Value::String("hello".to_string()), json!("hello")), + (toml::Value::Integer(42), json!(42)), + (toml::Value::Float(1.25), json!(1.25)), + (toml::Value::Boolean(true), json!(true)), + ]; + + for (input, expected) in cases { + assert_eq!(toml_scalar_to_json_value(&input)?, expected); + } + + Ok(()) + } + + #[test] + fn rejects_non_scalar_toml_values_with_typed_errors() { + let datetime: toml::Value = "value = 1979-05-27T07:32:00Z" + .parse::() + .unwrap() + .remove("value") + .unwrap(); + let cases = [ + (datetime, TomlScalarToJsonError::Datetime), + ( + toml::Value::Array(vec![toml::Value::Integer(1)]), + TomlScalarToJsonError::Array, + ), + ( + toml::Value::Table(toml::Table::new()), + TomlScalarToJsonError::Table, + ), + ]; + + for (input, expected) in cases { + assert_eq!(toml_scalar_to_json_value(&input), Err(expected)); + } + } + + #[test] + fn rejects_non_finite_toml_floats() { + for input in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert_eq!( + toml_scalar_to_json_value(&toml::Value::Float(input)), + Err(TomlScalarToJsonError::NonFiniteFloat) + ); + } + } + + #[test] + fn toml_scalars_round_trip_through_json() -> Result<(), TomlScalarToJsonError> { + let scalars = [ + toml::Value::String("hello".to_string()), + toml::Value::Integer(i64::MIN), + toml::Value::Integer(i64::MAX), + toml::Value::Float(1.25), + toml::Value::Boolean(false), + ]; + + for input in scalars { + let json = toml_scalar_to_json_value(&input)?; + assert_eq!( + json_scalar_to_toml_value(&json).expect("round trip should stay scalar"), + input + ); + } + + Ok(()) + } #[test] fn converts_strings_to_toml_strings() -> Result<(), JsonScalarToTomlError> { diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index e412be73b..3e15fc39f 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -81,7 +81,10 @@ pub use graph::{ AttrValue, AttributeScope, ContextKeyAttr, Edge, Graph, KNOWN_HANDLER_TYPES, Node, OnFailure, ResolvedOnFailure, is_known_handler_type, is_llm_handler_type, shape_to_handler_type, }; -pub use input_scalar::{JsonScalarToTomlError, json_scalar_to_toml_value}; +pub use input_scalar::{ + JsonScalarToTomlError, TomlScalarToJsonError, json_scalar_to_toml_value, + toml_scalar_to_json_value, +}; pub use interview::{ InterviewQuestionRecord, QuestionType, ReviewTarget, ReviewTargetError, ReviewTargetKind, };