diff --git a/docs/internal/mcp-server-qa-test-plan.md b/docs/internal/mcp-server-qa-test-plan.md index c5d7b8e74..920ff0754 100644 --- a/docs/internal/mcp-server-qa-test-plan.md +++ b/docs/internal/mcp-server-qa-test-plan.md @@ -1,5 +1,11 @@ # Fabro MCP Server — QA Test Plan +> Historical manual QA results. The run-create source selectors and flat settings +> below predate the registered-version contract. Current calls register file contents +> with `fabro_workflow_version_create`, then pass `workflow_version_id`, an explicit +> standalone `target`, and nested `args` to `fabro_run_create`. See +> [the current MCP guide](../public/agents/mcp.mdx) for the supported contract. + One-time manual QA pass for the 5 tools exposed by `fabro-mcp-server`. Source of truth: `lib/apps/fabro-mcp-server/src/run_tools/`. This plan is **not** a template for adding automated test coverage — it exists to drive a single hands-on sweep against a real running server. Tick boxes as scenarios pass; add notes inline for failures or surprising behavior. Open bugs/PRs for issues found; do not port these scenarios into the Rust test suite. diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index da356b8a5..98f1de65d 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -86,84 +86,58 @@ Registration resolves no runtime secrets, selects no environment, and starts no execution. It requires a user credential or a worker token with `agent:run_tools`; ordinary worker tokens and same-run Ask Fabro sessions cannot register versions. If an upload fails, retry the same contents: previously registered immutable -versions remain reusable. The existing `fabro_run_create` input is unchanged. +versions remain reusable. Use the returned ID with `fabro_run_create`. ### Create runs -For a simple create call, `fabro_run_create` accepts a workflow selector string. Selectors are compatible only when the MCP process and Fabro operation share the native filesystem: - -```json -{ "runs": ["sleeper"] } -``` - -Use the object form when you need create options: +Call `fabro_run_create` with a registered workflow version ID and an independent +workspace target. You can reuse the same ID for multiple runs without uploading +again: ```json { - "runs": [ - { - "workflow": "sleeper", - "auto_approve": true, - "dry_run": true, - "goal_file": "plans/ship-it.md", + "runs": [{ + "workflow_version_id": "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + "target": { "kind": "git", "repo": "acme/widgets", "branch": "main" }, + "environment_id": "production", + "goal": "Review the checkout implementation.", + "args": { + "inputs": { "component": "checkout" }, "labels": { "source": "mcp" }, - "start": true - } - ] + "auto_approve": false + }, + "start": false + }] } ``` -For agent-authored content or callers without a shared filesystem, send an inline package by value. `entrypoint` and every `files` key are portable paths inside that package: +Standalone MCP calls require an explicit `target`: Git coordinates, `kind: none` +for an empty workspace, or a server-local folder for a compatible Local environment. +A folder path refers to the server's filesystem, not the MCP client's filesystem. +Workflow content does not determine the target. Native workflow-agent calls may +omit `target` to inherit the parent's execution target; see [Child Runs](/execution/child-runs). +Supplying `parent_id` in standalone MCP does not enable that inheritance. -```json -{ - "runs": [ - { - "workflow": { - "kind": "inline", - "entrypoint": "workflows/review/workflow.fabro", - "files": { - "workflows/review/workflow.fabro": "digraph Review { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - } - }, - "target": { - "kind": "git", - "repo": "acme/widgets", - "branch": "feature/checkout" - } - } - ] -} -``` +`args` uses the same fields as RunIntent: `inputs`, `labels`, `model`, `provider`, +`dry_run`, `auto_approve`, and `preserve_sandbox`. Omitted boolean overrides stay +omitted; explicit `false` is preserved. `environment_id` selects a server environment; +omission uses the server default. `title`, literal `goal`, and an exact `parent_id` +are optional. Caller/project/machine run settings are not read by the tool. +Workflow-owned settings remain in the registered `workflow.toml`. -You can also reuse an exact immutable workflow version without uploading content again: + +Run creation accepts registered IDs only. Replace workflow strings and inline +sources with a preceding `fabro_workflow_version_create` call. Move flat run +options into `args`, use `environment_id` instead of `environment`, and send goal +text instead of `goal_file`. Obtain local or remote files using the caller's own +shell/read tools. Neither Fabro tool fetches remote sources or reads a caller path. + -```json -{ - "runs": [ - { - "workflow": { - "kind": "stored", - "workflow_version_id": "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" - }, - "target": { "kind": "none" }, - "start": false - } - ] -} -``` - -Workflow source and run target are independent. Object-form calls can provide an explicit Git, `none`, or folder target. A workflow agent inherits its parent run's canonical target when `target` is omitted; for a Git parent that means its repository and current execution branch (normally `fabro/run/`). The original input branch and pinned commit/tag are not inherited. Push the parent's changes before creating the child. If run branches are disabled, the child follows the original input branch; if an enabled run branch is not available yet, provide an explicit target. - -Folder targets are available only to standalone MCP and Local workflow agents with a shared host filesystem; Docker and Daytona parents cannot select a server-host folder, even by naming a Local child environment. - -Standalone MCP calls using a stored workflow ID must supply an explicit `target`: the stored repository configuration is not available for implicit derivation. For selector and inline sources, when `target` is omitted, standalone MCP derives it from the selected environment the way `fabro run` does: a Local environment targets the working directory as a folder, and a Docker or Daytona environment targets the attached GitHub checkout only when it can prove that the exact local HEAD is available from the canonical origin (a directory with no Git metadata runs with no workspace); otherwise, push the commit or provide an explicit target. Docker and Daytona agents must send inline files or a stored version ID—never send a sandbox path as a selector, because the run worker cannot use that path to read the sandbox's files. - -Use `goal` for inline goal text or `goal_file` to read the run goal from a file. They are mutually exclusive. Relative `goal_file` paths resolve from the run's `cwd`, or from the MCP server working directory when `cwd` is omitted. Like selectors, `goal_file` requires a native/shared filesystem; Docker and Daytona agents must send `goal` text by value. - -Creation and execution remain separate operations. `fabro_run_create` first creates a durable run, then requests one start by default. Set `start: false` to leave the new run submitted without requesting execution. - -`fabro_run_create` transmits only the values in the request. The MCP server's `~/.fabro/settings.toml` `[run]` defaults, and any project or machine run settings, are not applied to the created run, matching `fabro run`. Keep workflow-owned behavior in `workflow.toml`, and pass explicit `auto_approve`, `dry_run`, `model`, `provider`, `environment`, or `preserve_sandbox` values in the spec when a run needs them. +Creation persists a submitted run. A separate start request follows by default; +set `start: false` to start later. Batches contain 1–50 items and stop at the first +failure. If creation succeeded before a start, summary lookup, or later item +failed, the error includes the already-created run IDs. Inspect those runs before +retrying to avoid creating duplicates. Run summaries returned by the MCP server include parent metadata. Use `parent_id` on `fabro_run_create` to create a child run, `parent_id` on `fabro_run_search` to list direct children, and the `link_parent` or `unlink_parent` actions on `fabro_run_interact` to change an existing run's parent. See [Child Runs](/execution/child-runs) for the orchestration model. diff --git a/docs/public/execution/child-runs.mdx b/docs/public/execution/child-runs.mdx index 752cb62e4..95881df18 100644 --- a/docs/public/execution/child-runs.mdx +++ b/docs/public/execution/child-runs.mdx @@ -32,6 +32,7 @@ This exposes the same Fabro run tools available through [MCP](/agents/mcp): | Tool | Purpose | |---|---| +| `fabro_workflow_version_create` | Register supplied workflow files and return an immutable version ID | | `fabro_run_create` | Create one or more child runs, starting them by default | | `fabro_run_search` | Search runs, including direct children by `parent_id` | | `fabro_run_get` | Inspect a run without mutating it | @@ -46,101 +47,69 @@ When a workflow agent calls `fabro_run_create`, Fabro always parents the created ## Create child runs -The simplest `fabro_run_create` call names a workflow: +Acquire workflow files with the agent's shell/read tools, then call +`fabro_workflow_version_create` with their contents: ```json { - "runs": ["implement-and-test"] + "entrypoint": "workflow.fabro", + "files": { + "workflow.fabro": "digraph Child { start [shape=Mdiamond] work [prompt=\"@prompt.md\"] exit [shape=Msquare] start -> work -> exit }", + "prompt.md": "Implement the requested change and run the relevant tests." + } } ``` -Use the object form to pass run options: +Use the returned `workflow_version_id` in `fabro_run_create`: ```json { - "runs": [ - { - "workflow": "implement-and-test", - "goal": "Implement the checkout page refactor and run the test suite.", - "labels": { - "lane": "checkout", - "source": "parent-run" - }, - "start": true - } - ] + "runs": [{ + "workflow_version_id": "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + "goal": "Implement the checkout page refactor and run its tests.", + "args": { "labels": { "lane": "checkout", "source": "parent-run" } }, + "start": true + }] } ``` -The parent can create several children in one call: +A version can be reused for several children, each with its own goal, target, and +`args`. When the version already exists, skip registration. Read goal files with +the agent's read tool and pass literal `goal` text. Workflow names, paths, inline +source objects, and `goal_file` are no longer run-create inputs. -```json -{ - "runs": [ - { - "workflow": "implementation", - "goal_file": "plans/api.md", - "labels": { "lane": "api" } - }, - { - "workflow": "implementation", - "goal_file": "plans/web.md", - "labels": { "lane": "web" } - }, - { - "workflow": "review", - "goal": "Review the current branch for security and data-integrity risks.", - "labels": { "lane": "review" } - } - ] -} -``` +This flow works in Local, Docker, and Daytona environments. The agent can clone a +remote workflow in its sandbox and submit the resulting contents. Fabro's native +tool handler executes outside the sandbox, so registration treats file-map keys +as virtual relative paths and never reads sandbox paths from the worker host. +Include the workflow's referenced configuration, prompts, and child workflows in +the supplied tree. See [MCP](/agents/mcp) for package limits. -Selector strings and selector paths are available only when the parent agent runs in a genuinely shared Local filesystem context. Docker and Daytona agents must send the workflow bytes inline or reuse an exact stored workflow version; a path inside their sandbox is not a usable selector on the worker host. +Workflow content and workspace target are independent. If `target` is omitted, +a native child inherits the parent's canonical target: `none` or folder as-is, +and a Git target's repository and current execution branch (normally +`fabro/run/`). Push parent changes before creating the child; a child +clone sees that branch's remote HEAD. The parent's original pinned SHA/tag is +not inherited. If run branches are disabled, the original input branch is used; +if an enabled execution branch is unavailable, send an explicit target. -Send agent-authored workflow content with a strict inline source: +An explicit Git, `none`, or folder target overrides inheritance while the current +run remains the forced parent. Set `sha` on an explicit Git target to pin a child. +Server admission enforces folder access: Docker/Daytona parents cannot select a +server-host folder, including by requesting a Local child environment. +Standalone MCP always requires an explicit target, even with `parent_id`. -```json -{ - "runs": [ - { - "workflow": { - "kind": "inline", - "entrypoint": "child/workflow.fabro", - "files": { - "child/workflow.fabro": "digraph Child { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - } - }, - "goal": "Run the workflow content supplied by this parent." - } - ] -} -``` +Use `environment_id` to choose a server environment; omission uses the server +default, not the parent's environment. Run overrides belong in canonical `args`: +`inputs`, `labels`, `model`, `provider`, `auto_approve`, `dry_run`, and +`preserve_sandbox`. Omission preserves workflow/server defaults; explicit `false` +is not omitted. The tool does not apply caller, project, or machine run settings. +Keep workflow-owned configuration in the registered `workflow.toml`. -Reuse content already registered with Fabro by supplying its exact immutable ID: - -```json -{ - "runs": [ - { - "workflow": { - "kind": "stored", - "workflow_version_id": "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" - }, - "target": { "kind": "none" }, - "start": false - } - ] -} -``` - -Workflow content and workspace target are separate choices. If `target` is omitted, a child inherits the parent's canonical target: a `none` or folder target as-is, and a Git target's repository and current execution branch (normally `fabro/run/`). Push the parent's changes before creating the child; the child checks out that branch's current remote HEAD. The original input branch and pinned commit/tag are not carried over. If run branches are disabled, the child follows the original input branch. If an enabled run branch is not available yet, provide an explicit target. Pass an explicit Git target with a `sha` to pin a child. An explicit Git, `none`, or folder target overrides that inheritance while the current run remains the forced parent. Folder targets require a Local parent with a shared host filesystem; Docker and Daytona parents cannot select a server-host folder, even by naming a Local child environment. - -`goal_file` is also a shared-filesystem feature. Local agents can read it relative to the operation `cwd`; Docker and Daytona agents must send the resolved `goal` text by value. - -Like `fabro run`, `fabro_run_create` transmits only what the request names. Run defaults from the caller's `~/.fabro/settings.toml` `[run]` table, and project or machine run settings, are not applied to the created run. Put workflow-owned behavior in the workflow's `workflow.toml`, and pass explicit `auto_approve`, `dry_run`, `model`, `provider`, `environment`, or `preserve_sandbox` values in the spec when a child needs them. See [Run Configuration](/execution/run-configuration) for the settings layering. - -By default, `fabro_run_create` requests start for each created run. Set `"start": false` when the parent should create the child now and start it later. +Creation and start remain separate operations. Set `start: false` to leave a +child submitted. A batch stops on its first failure; if any runs have already +been created, their IDs appear in the error so the parent can inspect them +instead of recreating them blindly. ## Start and approval diff --git a/lib/apps/fabro-cli/src/commands/mcp/mod.rs b/lib/apps/fabro-cli/src/commands/mcp/mod.rs index ef5725f72..c6a63a5e1 100644 --- a/lib/apps/fabro-cli/src/commands/mcp/mod.rs +++ b/lib/apps/fabro-cli/src/commands/mcp/mod.rs @@ -34,7 +34,6 @@ fn server_settings( let user_settings = connection_ctx.user_settings().clone(); let storage_dir = connection_ctx.storage_dir().to_path_buf(); let base_config_path = connection_ctx.base_config_path().to_path_buf(); - let config_path = base_config_path.clone(); let client_factory: fabro_mcp_server::FabroClientFactory = std::sync::Arc::new(move || { let target = target.clone(); let user_settings = user_settings.clone(); @@ -51,11 +50,7 @@ fn server_settings( }); future }); - Ok(fabro_mcp_server::FabroMcpServerSettings { - client_factory, - config_path, - cwd: base_ctx.cwd().to_path_buf(), - }) + Ok(fabro_mcp_server::FabroMcpServerSettings { client_factory }) } fn init_settings(args: &McpInitArgs) -> Result { diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index 9e600cf16..10365ca4a 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -77,7 +77,7 @@ pub(crate) async fn create_run( target, dirty_worktree, } = fabro_manifest::derive_run_target_for_provider( - environment.settings.provider, + &environment.settings.provider, &canonical_cwd, configured_repo_origin_url.as_deref(), )?; diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index cd01459f2..e96388091 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -6,7 +6,6 @@ use std::time::Duration; use anyhow::{Context, Result, anyhow}; use async_trait::async_trait; use fabro_client::ServerTarget; -use fabro_config::user::default_workflows_dir; use fabro_config::{ServerSettingsBuilder, Storage}; use fabro_interview::{ AnswerSubmission, ControlInterviewer, WORKER_CONTROL_INVALID_CURSOR_REASON, @@ -15,13 +14,10 @@ use fabro_interview::{ WorkerControlMessage, }; use fabro_manifest::SuppliedWorkflowVersionPackager; -use fabro_server::run_tool_create::ServerRunCreateAdapter; use fabro_store::{EventEnvelope, RunProjection, RunProjectionReducer}; use fabro_tool::fabro_client::ClientBackend; -use fabro_types::settings::run::{EnvironmentProvider, RunMode, RunNamespace}; -use fabro_types::{ - ArtifactUpload, BlobHash, EventBody, FailureReason, Principal, RunEvent, RunId, -}; +use fabro_types::settings::run::{RunMode, RunNamespace}; +use fabro_types::{ArtifactUpload, BlobHash, EventBody, FailureReason, Principal, RunEvent, RunId}; use fabro_vault::{SecretStore, Vault}; use fabro_workflow::artifact_upload::{ArtifactSink, StageArtifactUploader}; use fabro_workflow::event::{Emitter, RunEventSink}; @@ -99,14 +95,7 @@ pub(crate) async fn execute( worker_token.to_owned(), ))); let fabro_run_tools = if fabro_run_tools_enabled_from_worker_token(worker_token) { - build_fabro_run_tool_services( - worker_token, - client.clone_for_reuse(), - run_id, - run_spec.settings.run.environment.provider, - run_spec.source_directory.as_deref(), - &run_dir, - ) + build_fabro_run_tool_services(worker_token, client.clone_for_reuse(), run_id) } else { None }; @@ -232,24 +221,15 @@ fn build_fabro_run_tool_services( worker_token: &str, client: fabro_client::Client, current_run_id: RunId, - provider: EnvironmentProvider, - source_directory: Option<&str>, - run_dir: &Path, ) -> Option { if worker_token.trim().is_empty() { return None; } let backend = ClientBackend::new(Arc::new(client)) - .with_run_create_adapter(Arc::new(ServerRunCreateAdapter::worker( - provider, - current_run_id, - Some(default_workflows_dir()), - ))) .with_workflow_version_packager(Arc::new(SuppliedWorkflowVersionPackager)); Some(FabroRunToolServices { backend: Arc::new(backend), current_run_id, - base_cwd: source_directory.map_or_else(|| run_dir.to_path_buf(), PathBuf::from), }) } @@ -1189,7 +1169,6 @@ fn install_signal_handlers( reason = "This test module prefers explicit type paths over extra imports." )] mod tests { - use std::path::Path; use std::sync::Arc; use std::time::Duration; @@ -1199,14 +1178,13 @@ mod tests { use fabro_interview::{ AnswerValue, ControlInterviewer, Interviewer, Question, WorkerControlEnvelope, }; - use fabro_server::run_tool_create::ServerRunCreateAdapter; use fabro_types::run_event::{ InterviewCompletedProps, InterviewStartedProps, RunCompletedProps, RunControlEffectProps, RunFailedProps, RunStatusTransitionProps, }; use fabro_types::{ AuthMethod, EventBody, FailureCategory, FailureDetail, FailureReason, IdpIdentity, - Principal, QuestionType, RunFailure, RunTarget, SuccessReason, WorkflowVersionId, fixtures, + Principal, QuestionType, RunFailure, SuccessReason, fixtures, }; use fabro_vault::{SecretType, Vault}; use fabro_workflow::event::RunEventSink; @@ -1264,45 +1242,6 @@ mod tests { )); } - #[tokio::test] - async fn fabro_run_create_worker_adapter_preserves_explicit_target_without_host_reads() { - use fabro_tool::{FabroRunCreateParams, RunCreateAdapter, ValidatedCreateRuns}; - use fabro_types::settings::run::EnvironmentProvider; - - let temp = tempfile::tempdir().unwrap(); - let workflow_version_id: WorkflowVersionId = - fabro_types::BlobHash::new(b"stored workflow").into(); - let inherited = RunTarget::None {}; - let params: FabroRunCreateParams = serde_json::from_value(serde_json::json!({ - "runs": [{ - "workflow": { - "kind": "stored", - "workflow_version_id": workflow_version_id - }, - "target": inherited - }] - })) - .unwrap(); - let spec = ValidatedCreateRuns::try_from(params) - .unwrap() - .runs - .remove(0); - let adapter = ServerRunCreateAdapter::worker( - EnvironmentProvider::Docker, - fabro_types::RunId::new(), - Some(temp.path().join("workflows")), - ); - let client = fabro_client::Client::new_no_proxy("http://127.0.0.1:9").unwrap(); - - let prepared = adapter - .prepare(&client, &spec, Path::new("/host/that-must-not-be-read")) - .await - .unwrap(); - - assert_eq!(prepared.workflow_version_id, workflow_version_id); - assert_eq!(prepared.target, inherited); - } - fn worker_token_with_claims(claims: &serde_json::Value) -> String { jsonwebtoken::encode( &jsonwebtoken::Header::new(jsonwebtoken::Algorithm::HS256), diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index 49f511e4c..246e7b99b 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -645,7 +645,7 @@ async fn stdio_server_initializes_and_lists_run_tools() { .find(|(name, _, _)| name == "fabro_run_create") .map(|(_, _, schema)| schema) .expect("fabro_run_create tool should be listed"); - assert_create_schema_accepts_string_and_object_specs(create_schema); + assert_create_schema_requires_version_ids(create_schema); client .shutdown() .await @@ -737,16 +737,15 @@ async fn mcp_create_and_search_manage_real_runs_with_cli_auth() { let client = spawn_mcp_client(&context, &["--server", &target_url]).await; + let workflow_version_id = register_mcp_workflow(&client, &workflow).await; let create = call_tool_json( &client, "fabro_run_create", serde_json::json!({ "runs": [{ - "workflow": workflow, + "workflow_version_id": workflow_version_id, "target": { "kind": "none" }, - "dry_run": true, - "auto_approve": true, - "labels": { "source": "mcp-test" } + "args": {"dry_run": true, "auto_approve": true, "labels": { "source": "mcp-test" }} }] }), ) @@ -805,16 +804,15 @@ async fn mcp_run_tools_use_default_local_server_without_server_flag() { let workflow = context.install_fixture("simple.fabro"); let client = spawn_mcp_client(&context, &[]).await; + let workflow_version_id = register_mcp_workflow(&client, &workflow).await; let create = call_tool_json( &client, "fabro_run_create", serde_json::json!({ "runs": [{ - "workflow": workflow, + "workflow_version_id": workflow_version_id, "target": { "kind": "none" }, - "dry_run": true, - "auto_approve": true, - "labels": { "source": "mcp-default-server-test" }, + "args": {"dry_run": true, "auto_approve": true, "labels": { "source": "mcp-default-server-test" }}, "start": false }] }), @@ -1876,7 +1874,9 @@ async fn mcp_create_validation_errors_happen_before_auth_or_network() { let context = test_context!(); let client = spawn_mcp_client(&context, &["--server", "http://127.0.0.1:9"]).await; let too_many = (0..51) - .map(|index| serde_json::json!({ "workflow": format!("wf-{index}.fabro") })) + .map( + |_| serde_json::json!({"workflow_version_id":"a".repeat(64), "target":{"kind":"none"}}), + ) .collect::>(); let empty = call_tool_error_text( @@ -1896,13 +1896,14 @@ async fn mcp_create_validation_errors_happen_before_auth_or_network() { "fabro_run_create", serde_json::json!({ "runs": [{ - "workflow": "simple.fabro", - "inputs": { "decision": null } + "workflow_version_id": "a".repeat(64), + "target": {"kind":"none"}, + "args": {"inputs": { "decision": null }} }] }), ) .await; - let conflicting_goal_sources = call_tool_error_text( + let conflicting_goal_sources = call_tool_parameter_error( &client, "fabro_run_create", serde_json::json!({ @@ -1919,7 +1920,7 @@ async fn mcp_create_validation_errors_happen_before_auth_or_network() { assert!(many.contains("runs"), "{many}"); assert!(null.contains("decision"), "{null}"); assert!( - conflicting_goal_sources.contains("goal and goal_file are mutually exclusive"), + conflicting_goal_sources.contains("fabro_workflow_version_create"), "{conflicting_goal_sources}" ); assert_mcp_run_tool_count(&client).await; @@ -1930,93 +1931,24 @@ async fn mcp_create_validation_errors_happen_before_auth_or_network() { } #[tokio::test(flavor = "multi_thread")] -async fn mcp_create_string_shorthand_deserializes_before_auth() { +async fn mcp_create_requires_explicit_target_and_rejects_old_shorthand_before_auth() { let context = test_context!(); - let harness = - RealAuthHarness::start_with_dev_token(fabro_test::GitHubAppState::default()).await; - let target_url = harness.api_target(); - let workflow = context.install_fixture("simple.fabro"); - context.git_init(); - run_git(&context.temp_dir, &["config", "user.name", "Fabro Test"]); - run_git(&context.temp_dir, &[ - "config", - "user.email", - "fabro@example.com", - ]); - run_git(&context.temp_dir, &["add", "simple.fabro"]); - run_git(&context.temp_dir, &["commit", "--quiet", "-m", "fixture"]); - run_git(&context.temp_dir, &[ - "remote", - "add", - "origin", - "https://github.com/fabro-sh/fabro.git", - ]); - let test_origin = context.temp_dir.join("origin.git"); - run_git(&context.temp_dir, &[ - "init", - "--bare", - "--quiet", - test_origin - .to_str() - .expect("test origin path should be UTF-8"), - ]); - let push_url = format!("file://{}", test_origin.display()); - run_git(&context.temp_dir, &[ - "remote", "set-url", "--push", "origin", &push_url, - ]); - let workflow_version_id = - fabro_manifest::collect_workflow_versions(&workflow, &context.temp_dir) - .expect("workflow fixture should package") - .root_id(); - let client = spawn_mcp_client(&context, &["--server", &target_url]).await; - - let result = client - .call_tool( - "fabro_run_create", - serde_json::json!({ "runs": [workflow] }), - std::time::Duration::from_secs(30), - ) - .await - .expect("string shorthand should deserialize and return a tool-level auth error"); - assert_eq!(result.is_error, Some(true), "tool should return error"); - let error = result - .content - .first() - .and_then(|content| serde_json::to_value(content).ok()) - .and_then(|content| content["text"].as_str().map(ToOwned::to_owned)) - .expect("tool error should include text"); - assert!(!error.contains("CreateRunSpec"), "{error}"); - assert!( - error.contains("Run `fabro auth login` to authenticate."), - "{error}" - ); - assert!( - harness - .api_requests - .contains("GET /api/v1/environments/default"), - "the shorthand request should reach environment lookup authentication" - ); - assert!( - !harness - .api_requests - .contains("POST /api/v1/workflow-versions"), - "an unauthenticated shorthand request must not attempt registration" - ); - assert!( - !harness.workflow_version_exists(workflow_version_id).await, - "an unauthenticated shorthand request must not register a workflow version" - ); - assert!( - !harness.api_requests.contains("POST /api/v1/runs"), - "an unauthenticated shorthand request must not reach run creation" - ); - assert_mcp_run_tool_count(&client).await; - - client - .shutdown() - .await - .expect("MCP client should shut down"); - harness.shutdown().await; + let client = spawn_mcp_client(&context, &["--server", "http://127.0.0.1:9"]).await; + let error = call_tool_parameter_error( + &client, + "fabro_run_create", + serde_json::json!({"runs":["simple.fabro"]}), + ) + .await; + assert!(error.contains("fabro_workflow_version_create"), "{error}"); + let error = call_tool_error_text( + &client, + "fabro_run_create", + serde_json::json!({"runs":[{"workflow_version_id":"a".repeat(64)}]}), + ) + .await; + assert!(error.contains("explicit target"), "{error}"); + client.shutdown().await.unwrap(); } #[tokio::test(flavor = "multi_thread")] @@ -2824,6 +2756,18 @@ async fn call_tool_json( .expect("tool result should include structured content") } +async fn call_tool_parameter_error( + client: &McpClient, + name: &str, + arguments: serde_json::Value, +) -> String { + let error = client + .call_tool(name, arguments, std::time::Duration::from_secs(30)) + .await + .expect_err("invalid parameters should produce an MCP protocol error"); + error.to_string() +} + async fn call_tool_error_text( client: &McpClient, name: &str, @@ -2842,50 +2786,62 @@ async fn call_tool_error_text( .expect("tool error should include text") } -fn assert_create_schema_accepts_string_and_object_specs(schema: &serde_json::Value) { - let variants = schema - .pointer("/properties/runs/items/anyOf") - .and_then(serde_json::Value::as_array) - .expect("fabro_run_create runs items should use anyOf"); - - assert!( - variants.iter().any(|variant| variant["type"] == "string"), - "fabro_run_create should advertise workflow string shorthand: {schema}" - ); - let object_variant = variants - .iter() - .find(|variant| variant["type"] == "object") - .unwrap_or_else(|| { - panic!("fabro_run_create should advertise object create specs: {schema}") +fn assert_create_schema_requires_version_ids(schema: &serde_json::Value) { + let item = &schema["properties"]["runs"]["items"]; + let item = item + .get("$ref") + .and_then(serde_json::Value::as_str) + .map_or(item, |reference| { + schema + .pointer( + reference + .strip_prefix('#') + .expect("schema reference should be local"), + ) + .expect("schema reference should resolve") }); + assert_eq!(item["type"], "object"); + assert_eq!(item["additionalProperties"], false); assert!( - object_variant.pointer("/properties/workflow").is_some(), - "object create spec should include workflow property: {schema}" - ); - assert!( - object_variant.pointer("/properties/goal_file").is_some(), - "object create spec should include goal_file property: {schema}" - ); - assert!( - object_variant - .get("required") - .and_then(serde_json::Value::as_array) - .is_some_and(|required| required.iter().any(|field| field == "workflow")), - "object create spec should require workflow: {schema}" + item["required"] + .as_array() + .expect("create schema should require fields") + .iter() + .any(|field| field == "workflow_version_id") ); + assert!(item["properties"].get("args").is_some()); + assert!(item["properties"].get("workflow").is_none()); + assert!(item["properties"].get("goal_file").is_none()); +} + +async fn register_mcp_workflow(client: &McpClient, workflow: &Path) -> serde_json::Value { + let entrypoint = workflow + .file_name() + .expect("fixture should have a filename") + .to_str() + .expect("fixture filename should be UTF-8"); + let content = fs::read_to_string(workflow).expect("workflow fixture should be readable"); + let result = call_tool_json( + client, + "fabro_workflow_version_create", + serde_json::json!({ + "entrypoint": entrypoint, "files": {entrypoint: content} + }), + ) + .await; + result["workflow_version_id"].clone() } async fn create_mcp_run(client: &McpClient, workflow: PathBuf, start: bool) -> String { + let workflow_version_id = register_mcp_workflow(client, &workflow).await; let create = call_tool_json( client, "fabro_run_create", serde_json::json!({ "runs": [{ - "workflow": workflow, + "workflow_version_id": workflow_version_id, "target": { "kind": "none" }, - "dry_run": true, - "auto_approve": true, - "labels": { "source": "mcp-test" }, + "args": {"dry_run": true, "auto_approve": true, "labels": { "source": "mcp-test" }}, "start": start }] }), @@ -2897,19 +2853,6 @@ async fn create_mcp_run(client: &McpClient, workflow: PathBuf, start: bool) -> S .to_string() } -fn run_git(cwd: &Path, args: &[&str]) { - let output = Command::new("git") - .args(args) - .current_dir(cwd) - .output() - .expect("git command should run"); - assert!( - output.status.success(), - "git {args:?} failed: {}", - String::from_utf8_lossy(&output.stderr) - ); -} - fn seed_oauth_auth( home_dir: &Path, target: &fabro_client::ServerTarget, diff --git a/lib/apps/fabro-cli/tests/it/support/auth_harness.rs b/lib/apps/fabro-cli/tests/it/support/auth_harness.rs index d9fb34389..315465096 100644 --- a/lib/apps/fabro-cli/tests/it/support/auth_harness.rs +++ b/lib/apps/fabro-cli/tests/it/support/auth_harness.rs @@ -25,9 +25,7 @@ use fabro_server::jwt_auth::resolve_auth_mode_with_lookup; use fabro_server::server::{RouterOptions, build_router_with_options}; use fabro_server::test_support::TestAppStateBuilder; use fabro_static::EnvVars; -use fabro_store::Database; use fabro_test::{GitHubAppState, TestContext, apply_test_isolation}; -use fabro_types::WorkflowVersionId; use serde_json::Value; use tokio::net::TcpListener; use tokio::sync::oneshot; @@ -44,7 +42,6 @@ pub(crate) const TEST_DEV_TOKEN: &str = pub(crate) struct RealAuthHarness { pub(crate) api_base_url: String, api_server: RunningHttpServer, - store: Arc, twin: fabro_test::TwinGitHub, pub(crate) api_requests: ListenerRequestLog, } @@ -87,13 +84,11 @@ impl RealAuthHarness { if let Some(token) = dev_token.clone() { secrets.insert("FABRO_DEV_TOKEN".to_string(), token); } - let (store, artifact_store) = fabro_server::test_support::test_store_bundle(); let state = TestAppStateBuilder::new() .runtime_settings(settings, RunLayer::default()) .max_concurrent_runs(5) .env_lookup(|_| None) .server_secret_env(secrets) - .store_bundle(Arc::clone(&store), artifact_store) .vault_entries([ ("GITHUB_APP_CLIENT_SECRET", github_client_secret.as_str()), (EnvVars::OPENAI_API_KEY, "test-openai-api-key"), @@ -118,7 +113,6 @@ impl RealAuthHarness { Self { api_base_url, api_server, - store, twin, api_requests, } @@ -128,15 +122,6 @@ impl RealAuthHarness { format!("{}/api/v1", self.api_base_url) } - pub(crate) async fn workflow_version_exists(&self, id: WorkflowVersionId) -> bool { - let blob_hash = id.into(); - self.store - .blobs() - .exists(&blob_hash) - .await - .expect("workflow-version blob lookup should succeed") - } - pub(crate) async fn shutdown(self) { self.api_server.shutdown().await; self.twin.shutdown().await; diff --git a/lib/apps/fabro-mcp-server/src/lib.rs b/lib/apps/fabro-mcp-server/src/lib.rs index ff555322c..6a58b29b8 100644 --- a/lib/apps/fabro-mcp-server/src/lib.rs +++ b/lib/apps/fabro-mcp-server/src/lib.rs @@ -23,8 +23,6 @@ pub type FabroClientFactory = Arc FabroClientFuture + Send + Sync>; #[derive(Clone)] pub struct FabroMcpServerSettings { pub client_factory: FabroClientFactory, - pub config_path: PathBuf, - pub cwd: PathBuf, } impl std::fmt::Debug for FabroMcpServerSettings { @@ -32,8 +30,6 @@ impl std::fmt::Debug for FabroMcpServerSettings { formatter .debug_struct("FabroMcpServerSettings") .field("client_factory", &"") - .field("config_path", &self.config_path) - .field("cwd", &self.cwd) .finish() } } diff --git a/lib/apps/fabro-mcp-server/src/server.rs b/lib/apps/fabro-mcp-server/src/server.rs index b20fd4bb4..ee98aa6b0 100644 --- a/lib/apps/fabro-mcp-server/src/server.rs +++ b/lib/apps/fabro-mcp-server/src/server.rs @@ -1,11 +1,8 @@ -use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; use anyhow::Result; -use fabro_config::user; use fabro_manifest::SuppliedWorkflowVersionPackager; -use fabro_server::run_tool_create::ServerRunCreateAdapter; use fabro_tool::fabro_client::ClientBackend; use fabro_tool::{self as run_tools, FabroToolBackend}; use fabro_util::version::FABRO_VERSION; @@ -26,7 +23,6 @@ use crate::{FabroMcpServerSettings, SERVER_NAME}; pub(crate) struct FabroMcpServer { settings: Arc, backend: Arc>>, - cwd: PathBuf, tool_router: ToolRouter, } @@ -83,11 +79,9 @@ impl ServerHandler for FabroMcpServer { #[tool_router(router = tool_router)] impl FabroMcpServer { pub(crate) fn new(settings: Arc) -> Self { - let cwd = settings.cwd.clone(); Self { settings, backend: Arc::new(OnceCell::new()), - cwd, tool_router: Self::tool_router(), } } @@ -116,21 +110,21 @@ impl FabroMcpServer { #[tool( name = "fabro_run_create", - description = "Create one or more Fabro workflow runs from a native selector, inline files, or an exact stored workflow version, optionally under a parent run and starting them by default." + description = "Create runs from registered workflow_version_id values and canonical RunIntent settings. Register contents with fabro_workflow_version_create first. Standalone calls require an explicit target; native workers may inherit the parent target. Starts runs by default." )] async fn fabro_run_create( &self, params: Parameters, ) -> Result { - let params = match run_tools::ValidatedCreateRuns::try_from(params.0) { - Ok(params) => params, - Err(err) => return Ok(error_result(&err)), - }; + let params = params.0; + if let Err(err) = params.validate(run_tools::CreateRunOptions::default()) { + return Ok(error_result(&err)); + } let backend = match self.backend().await { Ok(backend) => backend, Err(err) => return Ok(error_result(&err)), }; - match run_tools::create_runs(backend, &self.cwd, params).await { + match run_tools::create_runs(backend, params).await { Ok(result) => success_result(&result, run_tools::create_runs_text(&result)), Err(err) => Ok(error_result(&err)), } @@ -275,15 +269,9 @@ impl FabroMcpServer { .await .map(|client| { Arc::new( - ClientBackend::new(Arc::new(client)) - .with_run_create_adapter(Arc::new( - ServerRunCreateAdapter::standalone(Some( - user::default_workflows_dir(), - )), - )) - .with_workflow_version_packager(Arc::new( - SuppliedWorkflowVersionPackager, - )), + ClientBackend::new(Arc::new(client)).with_workflow_version_packager( + Arc::new(SuppliedWorkflowVersionPackager), + ), ) as Arc }) .map_err(|err| run_tools::ToolError::from_anyhow(&err)) @@ -315,7 +303,6 @@ fn error_result(err: &run_tools::ToolError) -> CallToolResult { #[cfg(test)] mod tests { use std::collections::BTreeMap; - use std::path::PathBuf; use std::sync::Arc; use serde_json::Value; @@ -344,8 +331,6 @@ mod tests { .await; let url = mock.url(""); let server = FabroMcpServer::new(Arc::new(FabroMcpServerSettings { - cwd: PathBuf::from("/does-not-exist"), - config_path: PathBuf::from("/does-not-exist"), client_factory: Arc::new(move || { let url = url.clone(); Box::pin(async move { fabro_client::Client::new_no_proxy(&url) }) @@ -383,8 +368,6 @@ mod tests { #[test] fn server_info_reports_fabro_version() { let settings = FabroMcpServerSettings { - cwd: PathBuf::from("."), - config_path: PathBuf::from("fabro.toml"), client_factory: Arc::new(|| { Box::pin(async { panic!("client should not be constructed while reading info") }) }), @@ -399,8 +382,6 @@ mod tests { #[test] fn fabro_run_pair_tool_is_registered_with_stage_based_schema() { let settings = FabroMcpServerSettings { - cwd: PathBuf::from("."), - config_path: PathBuf::from("fabro.toml"), client_factory: Arc::new(|| { Box::pin(async { panic!("client should not be constructed while listing tools") }) }), @@ -426,10 +407,8 @@ mod tests { } #[test] - fn fabro_run_create_tool_advertises_complete_workflow_source_and_target_grammar() { + fn fabro_run_create_tool_advertises_canonical_version_contract() { let settings = FabroMcpServerSettings { - cwd: PathBuf::from("."), - config_path: PathBuf::from("fabro.toml"), client_factory: Arc::new(|| { Box::pin(async { panic!("client should not be constructed while listing tools") }) }), @@ -441,59 +420,15 @@ mod tests { .find(|tool| tool.name.as_ref() == "fabro_run_create") .expect("fabro_run_create should be registered"); let schema = Value::Object(tool.input_schema.as_ref().clone()); - let variants = schema - .pointer("/properties/runs/items/anyOf") - .and_then(Value::as_array) - .expect("runs items should advertise string and object variants"); - - assert!( - variants.iter().any(|variant| variant["type"] == "string"), - "runs items should include workflow string shorthand: {schema}" - ); - let object_variant = variants + let definition = run_tools::tool_definitions() .iter() - .find(|variant| variant["type"] == "object") - .unwrap_or_else(|| { - panic!("runs items should include object create spec variant: {schema}") - }); - assert_eq!(object_variant["additionalProperties"], false); - assert!( - object_variant.pointer("/properties/workflow").is_some(), - "object create spec should expose workflow property: {schema}" - ); - assert!( - object_variant.pointer("/properties/goal_file").is_some(), - "object create spec should expose goal_file property: {schema}" - ); - assert!( - object_variant - .get("required") - .and_then(Value::as_array) - .is_some_and(|required| required.iter().any(|name| name == "workflow")), - "object create spec should require workflow: {schema}" - ); - let workflow_variants = object_variant - .pointer("/properties/workflow/anyOf") - .and_then(Value::as_array) - .expect("workflow should advertise selector, inline, and stored sources"); - assert!( - workflow_variants - .iter() - .any(|variant| variant["type"] == "string") - ); - for kind in ["inline", "stored"] { - assert!(workflow_variants.iter().any(|variant| { - variant.pointer("/properties/kind/const") == Some(&Value::from(kind)) - })); - } - let target_variants = object_variant - .pointer("/properties/target/anyOf") - .and_then(Value::as_array) - .expect("target should advertise the canonical target union"); - for kind in ["git", "none", "folder"] { - assert!(target_variants.iter().any(|variant| { - variant.pointer("/properties/kind/const") == Some(&Value::from(kind)) - })); - } + .find(|definition| definition.name == "fabro_run_create") + .unwrap(); + let mut expected = definition.parameters.clone(); + expected.as_object_mut().unwrap().remove("$schema"); + let mut actual = schema; + actual.as_object_mut().unwrap().remove("$schema"); + assert_eq!(actual, expected); + assert_eq!(tool.description.as_deref(), Some(definition.description)); } } diff --git a/lib/apps/fabro-server/src/lib.rs b/lib/apps/fabro-server/src/lib.rs index 14d9addc1..5b9ee5447 100644 --- a/lib/apps/fabro-server/src/lib.rs +++ b/lib/apps/fabro-server/src/lib.rs @@ -41,7 +41,8 @@ mod run_intent; mod run_manifest; mod run_selector; mod run_title_generation; -pub mod run_tool_create; +#[cfg(test)] +mod run_tool_create; pub mod security_headers; pub mod serve; pub mod server; diff --git a/lib/apps/fabro-server/src/run_tool_create.rs b/lib/apps/fabro-server/src/run_tool_create.rs index a59229866..503236f61 100644 --- a/lib/apps/fabro-server/src/run_tool_create.rs +++ b/lib/apps/fabro-server/src/run_tool_create.rs @@ -1,1228 +1,229 @@ -use std::path::{Path, PathBuf}; +// Integration regression for the native run tool using production Git setup. +use std::collections::HashMap; +use std::path::Path; +use std::process::Command; +use std::sync::{Arc, Mutex}; -use anyhow::{Context, Result, bail}; -use async_trait::async_trait; -use fabro_environment::DEFAULT_ENVIRONMENT_ID; -use fabro_manifest::{ - CollectedWorkflowClosure, DerivedRunTarget, collect_inline_workflow_versions, - configured_repo_origin_url_for_location, configured_repo_origin_url_from_workflow_toml, - derive_run_target_for_provider, resolve_local_workflow_package, +use chrono::{TimeZone, Utc}; +use fabro_tool::fabro_client::ClientBackend; +use fabro_types::{ + GitRunTarget, Run, RunId, RunLifecycle, RunLinks, RunOrigin, RunProjection, RunStatus, + RunTarget, RunTimestamps, WorkflowRef, WorkflowVersionId, test_support, }; -use fabro_tool::{ - CreateRunWorkflowSource, PreparedRunCreate, RunCreateAdapter, ValidatedCreateRunSpec, -}; -use fabro_types::settings::run::EnvironmentProvider; -use fabro_types::{RunId, RunProjection, RunTarget}; -use tokio::{fs, task}; - -#[derive(Clone, Debug)] -pub struct ServerRunCreateAdapter { - mode: RunCreateMode, +use httpmock::Method::{GET, POST}; +use httpmock::{HttpMockRequest, HttpMockResponse, MockServer}; +use serde_json::json; +use tokio::fs; +#[expect( + clippy::disallowed_methods, + reason = "test fixture setup uses the Git CLI against an isolated temporary repository" +)] +fn run_git(cwd: &Path, args: &[&str]) { + let output = Command::new("git") + .args(args) + .current_dir(cwd) + .output() + .expect("git command should run"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); } -#[derive(Clone, Debug)] -enum RunCreateMode { - Standalone { - user_workflows_root: Option, - }, - Worker { - provider: EnvironmentProvider, - parent_run_id: RunId, - user_workflows_root: Option, - }, +fn test_parent(target: Option) -> RunProjection { + let mut spec = fabro_types::test_support::test_run_spec(); + spec.target = target; + RunProjection::new(String::new(), spec, chrono::Utc::now()) } -impl ServerRunCreateAdapter { - #[must_use] - pub fn standalone(user_workflows_root: Option) -> Self { - Self { - mode: RunCreateMode::Standalone { - user_workflows_root, - }, - } - } - - #[must_use] - pub fn worker( - provider: EnvironmentProvider, - parent_run_id: RunId, - user_workflows_root: Option, - ) -> Self { - Self { - mode: RunCreateMode::Worker { - provider, - parent_run_id, - user_workflows_root, - }, - } - } - - fn has_shared_filesystem(&self) -> bool { - match self.mode { - RunCreateMode::Standalone { .. } => true, - RunCreateMode::Worker { provider, .. } => provider.is_local(), - } - } - - fn user_workflows_root(&self) -> Option<&Path> { - match &self.mode { - RunCreateMode::Standalone { - user_workflows_root, - } - | RunCreateMode::Worker { - user_workflows_root, - .. - } => user_workflows_root.as_deref(), - } - } - - async fn resolve_goal( - &self, - spec: &ValidatedCreateRunSpec, - cwd: &Path, - ) -> Result> { - if let Some(goal) = &spec.goal { - return Ok(Some(goal.clone())); - } - let Some(goal_file) = &spec.goal_file else { - return Ok(None); - }; - if !self.has_shared_filesystem() { - bail!( - "goal_file requires a shared Local filesystem; Docker and Daytona callers must send goal text by value" - ); - } - let path = cwd.join(goal_file); - fs::read_to_string(&path) - .await - .with_context(|| format!("failed to read goal file {}", path.display())) - .map(Some) - } - - async fn resolve_target( - &self, - client: &fabro_client::Client, - spec: &ValidatedCreateRunSpec, - cwd: &Path, - configured_repo_origin_url: Option<&str>, - ) -> Result { - if let Some(target) = &spec.target { - if matches!(target, RunTarget::Folder { .. }) && !self.has_shared_filesystem() { - bail!( - "folder targets require a shared Local filesystem; Docker and Daytona parents cannot select server-host folders" - ); - } - return Ok(ResolvedTarget { - target: target.clone(), - warnings: Vec::new(), - }); - } - - match &self.mode { - RunCreateMode::Worker { parent_run_id, .. } => { - let parent = client - .get_run_state(parent_run_id) - .await - .context("could not retrieve the parent run's current execution state")?; - Ok(ResolvedTarget { - target: inherit_parent_target(&parent)?, - warnings: Vec::new(), - }) - } - RunCreateMode::Standalone { .. } => { - // Standalone callers derive the target the same way `fabro run` - // does: from the selected environment's provider. Local - // environments run against the caller folder; clone-based - // environments need a provably published GitHub checkout. - let environment_id = spec - .environment - .as_deref() - .unwrap_or(DEFAULT_ENVIRONMENT_ID); - let environment = client - .retrieve_environment(environment_id) - .await - .with_context(|| { - format!( - "could not retrieve environment `{environment_id}` to derive the run target" - ) - })?; - let provider = environment.settings.provider; - let canonical_cwd = fs::canonicalize(cwd).await.with_context(|| { - format!("failed to canonicalize run directory {}", cwd.display()) - })?; - let configured_repo_origin_url = configured_repo_origin_url.map(str::to_owned); - let DerivedRunTarget { - target, - dirty_worktree, - } = task::spawn_blocking(move || { - derive_run_target_for_provider( - provider, - &canonical_cwd, - configured_repo_origin_url.as_deref(), - ) - }) - .await - .context("run target derivation task failed")??; - let mut warnings = Vec::new(); - if dirty_worktree { - warnings.push( - "the local checkout has uncommitted changes; those changes are excluded from the run target" - .to_string(), - ); - } - Ok(ResolvedTarget { target, warnings }) - } - } - } - - async fn collect_selector(&self, selector: &str, cwd: &Path) -> Result { - if !self.has_shared_filesystem() { - bail!( - "workflow selectors require a shared Local filesystem; send inline files or an exact stored workflow version ID from Docker or Daytona" - ); - } - let selector = PathBuf::from(selector); - let cwd = cwd.to_path_buf(); - let user_workflows_root = self.user_workflows_root().map(Path::to_path_buf); - task::spawn_blocking(move || { - let package = - resolve_local_workflow_package(&selector, &cwd, user_workflows_root.as_deref())?; - let configured_repo_origin_url = - configured_repo_origin_url_for_location(package.workflow_location())?; - Ok(CollectedWorkflow { - closure: package.into_closure(), - configured_repo_origin_url, - }) +async fn mock_parent<'a>(server: &'a MockServer, parent: &RunProjection) -> httpmock::Mock<'a> { + let path = format!("/api/v1/runs/{}/state", parent.spec.id()); + let body = serde_json::to_value(parent).unwrap(); + server + .mock_async(move |when, then| { + when.method(GET).path(path); + then.status(200).json_body(body); }) .await - .context("workflow package collection task failed")? - } } -/// A packaged workflow closure plus the `run.scm` repository its config names, -/// which standalone target derivation honors over the checkout's own origin. -struct CollectedWorkflow { - closure: CollectedWorkflowClosure, - configured_repo_origin_url: Option, -} - -#[async_trait] -impl RunCreateAdapter for ServerRunCreateAdapter { - async fn prepare( - &self, - client: &fabro_client::Client, - spec: &ValidatedCreateRunSpec, - cwd: &Path, - ) -> Result { - let goal = self.resolve_goal(spec, cwd).await?; - let CollectedWorkflow { - closure, - configured_repo_origin_url, - } = match &spec.workflow { - CreateRunWorkflowSource::Stored { - workflow_version_id, - } => { - // Stored configuration is not available through the client - // API. Never substitute the caller's checkout for a workflow's - // configured repository merely because it was supplied by ID. - if matches!(self.mode, RunCreateMode::Standalone { .. }) && spec.target.is_none() { - bail!( - "standalone stored workflow sources require an explicit target; the stored workflow's repository configuration is not available for target derivation" - ); - } - let resolved_target = self.resolve_target(client, spec, cwd, None).await?; - return Ok(PreparedRunCreate { - workflow_version_id: *workflow_version_id, - target: resolved_target.target, - goal, - warnings: resolved_target.warnings, - }); - } - CreateRunWorkflowSource::Selector(selector) => { - self.collect_selector(selector, cwd).await? - } - CreateRunWorkflowSource::Inline(source) => collect_inline_workflow(source).await?, - }; - let resolved_target = self - .resolve_target(client, spec, cwd, configured_repo_origin_url.as_deref()) - .await?; - let versions = closure - .versions() - .map(|(_, version)| version.version()) - .collect::>(); - client.register_workflow_versions(versions).await?; - - Ok(PreparedRunCreate { - workflow_version_id: closure.root_id(), - target: resolved_target.target, - goal, - warnings: resolved_target.warnings, - }) - } -} - -struct ResolvedTarget { - target: RunTarget, - warnings: Vec, -} - -/// Follow the parent's execution branch, not the branch it originally cloned. -/// A missing execution branch must not silently substitute unrelated source -/// content. When run branches are disabled, execution stays on the input -/// branch. -fn inherit_parent_target(parent: &RunProjection) -> Result { - let target = parent.spec.target.as_ref().context( - "the parent run has no canonical target; send an explicit target for this child run", - )?; - Ok(match target { - RunTarget::Git(git) => { - let branch = if parent.spec.settings.run.run_branch.enabled { - parent.start.as_ref().and_then(|start| start.run_branch.as_ref()).context( - "the parent run has no execution branch yet; send an explicit target for this child run" - )? - } else { - &git.branch - }; - RunTarget::Git(fabro_types::GitRunTarget { - repo: git.repo.clone(), - branch: branch.clone(), - tag: None, - sha: None, - }) - } - RunTarget::None {} | RunTarget::Folder { .. } => target.clone(), - }) -} - -/// Collect an inline workflow from its supplied bytes. The entrypoint is an -/// exact key of the file map, never a checkout selector. -async fn collect_inline_workflow( - source: &fabro_tool::InlineWorkflowSource, -) -> Result { - let entrypoint = source.entrypoint.clone(); - let files = source.files.clone(); - task::spawn_blocking(move || { - let closure = collect_inline_workflow_versions(&entrypoint, &files)?; - // The inline config is either the entrypoint itself or the - // `workflow.toml` beside the entrypoint graph. - let config_path = if Path::new(entrypoint.as_str()) - .extension() - .is_some_and(|ext| ext == "toml") - { - Some(entrypoint.clone()) - } else { - entrypoint.resolve_reference("workflow.toml").ok() - }; - let configured_repo_origin_url = config_path - .and_then(|path| files.get(&path)) - .map(|source| configured_repo_origin_url_from_workflow_toml(source)) - .transpose()? - .flatten(); - Ok(CollectedWorkflow { - closure, - configured_repo_origin_url, - }) +#[tokio::test] +async fn run_create_child_checkout_contains_the_parents_pushed_work() { + let temp = tempfile::tempdir().unwrap(); + let workspace = temp.path().join("parent"); + let origin = temp.path().join("origin.git"); + fs::create_dir(&workspace).await.unwrap(); + run_git(temp.path(), &[ + "init", + "--bare", + "--quiet", + origin.to_str().unwrap(), + ]); + run_git(&workspace, &["init", "--quiet", "--initial-branch", "main"]); + run_git(&workspace, &["config", "user.name", "Fabro Test"]); + run_git(&workspace, &["config", "user.email", "fabro@example.com"]); + fs::write(workspace.join("result.txt"), "original") + .await + .unwrap(); + run_git(&workspace, &["add", "."]); + run_git(&workspace, &["commit", "--quiet", "-m", "initial"]); + run_git(&workspace, &[ + "remote", + "add", + "origin", + origin.to_str().unwrap(), + ]); + run_git(&workspace, &["push", "--quiet", "origin", "main"]); + let base_sha = fabro_workflow::git::head_sha(&workspace).unwrap(); + let mut parent = test_parent(Some(RunTarget::Git(GitRunTarget { + repo: "acme/widgets".to_owned(), + branch: "main".to_owned(), + tag: Some("v1.0.0".to_owned()), + sha: Some(base_sha), + }))); + let sandbox = fabro_sandbox::local_sandbox(&workspace).await.unwrap(); + // Docker and Daytona use this same setup operation to create the run branch. + let git = fabro_sandbox::setup_git(&sandbox, &fabro_sandbox::GitSetupIntent::NewRun { + run_id: parent.spec.id().to_string(), }) .await - .context("inline workflow package collection task failed")? -} - -#[cfg(test)] -mod tests { - use std::collections::BTreeMap; - use std::process::Command; - use std::sync::{Arc, Mutex}; - - use fabro_tool::fabro_client::ClientBackend; - use fabro_tool::{FabroRunCreateParams, FabroToolBackend as _, ValidatedCreateRuns}; - use fabro_types::{GitRunTarget, WorkflowVersion, WorkflowVersionId}; - use httpmock::Method::{GET, POST}; - use httpmock::{HttpMockRequest, HttpMockResponse, MockServer}; - use serde_json::json; - - use super::*; - - /// Canonical `GET /api/v1/environments/{id}` body for mock servers. - 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": {} - }) - } - - async fn mock_environment<'a>( - server: &'a MockServer, - id: &str, - provider: &str, - ) -> httpmock::Mock<'a> { - let path = format!("/api/v1/environments/{id}"); - let body = environment_json(id, provider); - server - .mock_async(move |when, then| { - when.method(GET).path(path); - then.status(200) + .unwrap(); + parent.start = Some(fabro_types::StartRecord { + start_time: chrono::Utc::now(), + run_branch: Some(git.run_branch.clone()), + base_sha: Some(git.base_sha), + }); + fs::write(workspace.join("result.txt"), "parent implementation") + .await + .unwrap(); + run_git(&workspace, &["add", "."]); + run_git(&workspace, &["commit", "--quiet", "-m", "implement"]); + run_git(&workspace, &["push", "--quiet", "origin", &git.run_branch]); + let server = MockServer::start_async().await; + let state_request = mock_parent(&server, &parent).await; + let client = fabro_client::Client::new_no_proxy(&server.url("")).unwrap(); + let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); + let child_id = RunId::new(); + let submitted = Arc::new(Mutex::new(Vec::::new())); + let captured = Arc::clone(&submitted); + server + .mock_async(move |when, then| { + when.method(POST).path("/api/v1/runs"); + then.respond_with(move |request: &HttpMockRequest| { + captured + .lock() + .unwrap() + .push(serde_json::from_str(&request.body_string()).unwrap()); + HttpMockResponse::builder() + .status(201) .header("content-type", "application/json") - .json_body(body); - }) - .await - } + .body(serde_json::to_string(&run(child_id, None, 0)).unwrap()) + .build() + }); + }) + .await; + server + .mock_async(|when, then| { + when.method(GET).path(format!("/api/v1/runs/{child_id}")); + then.status(200) + .json_body_obj(&run(child_id, Some(parent.spec.id()), 0)); + }) + .await; + let params = serde_json::from_value( + json!({"runs":[{"workflow_version_id":workflow_version_id,"start":false}]}), + ) + .unwrap(); + fabro_tool::create_runs_with_options( + Arc::new(ClientBackend::new(Arc::new(client))), + params, + fabro_tool::CreateRunOptions { + forced_parent_id: Some(parent.spec.id()), + }, + ) + .await + .unwrap(); + state_request.assert_calls_async(1).await; + let intents = submitted.lock().unwrap().clone(); + assert_eq!(intents.len(), 1); + assert_eq!(intents[0].workflow_version_id, workflow_version_id); + assert_eq!(intents[0].parent_id, Some(parent.spec.id())); + let RunTarget::Git(target) = &intents[0].target else { + panic!("child should have a Git target") + }; + assert_eq!(target.repo, "acme/widgets"); + assert_eq!(target.sha, None); + assert_eq!(target.tag, None); + let child = temp.path().join("child"); + run_git(temp.path(), &[ + "clone", + "--quiet", + "--branch", + &target.branch, + origin.to_str().unwrap(), + child.to_str().unwrap(), + ]); + assert_eq!( + fs::read_to_string(child.join("result.txt")).await.unwrap(), + "parent implementation" + ); +} - fn validated_spec(value: &serde_json::Value) -> ValidatedCreateRunSpec { - let params: FabroRunCreateParams = serde_json::from_value(json!({ "runs": [value] })) - .expect("create input should deserialize"); - ValidatedCreateRuns::try_from(params) - .expect("create input should validate") - .runs - .remove(0) - } +fn run(run_id: RunId, parent_id: Option, children_count: u64) -> Run { + run_with_status(run_id, parent_id, children_count, RunStatus::Submitted) +} - fn no_proxy_client(base_url: &str) -> fabro_client::Client { - fabro_client::Client::new_no_proxy(base_url).expect("test client should build") - } - - #[expect( - clippy::disallowed_methods, - reason = "test fixture setup uses the Git CLI against an isolated temporary repository" - )] - fn run_git(cwd: &Path, args: &[&str]) { - let output = Command::new("git") - .args(args) - .current_dir(cwd) - .output() - .expect("git command should run"); - assert!( - output.status.success(), - "git {args:?} failed: {}", - String::from_utf8_lossy(&output.stderr) - ); - } - - async fn dynamic_version_registration_mock( - server: &MockServer, - registered: Arc>>, - ) -> httpmock::Mock<'_> { - server - .mock_async(move |when, then| { - when.method(POST).path("/api/v1/workflow-versions"); - then.respond_with(move |request: &HttpMockRequest| { - let version: WorkflowVersion = serde_json::from_str(&request.body_string()) - .expect("registration request should contain a workflow version"); - let id = version.id().expect("registered version should have an ID"); - registered.lock().unwrap().push(version); - HttpMockResponse::builder() - .status(201) - .header("content-type", "application/json") - .body(json!({ "workflow_version_id": id }).to_string()) - .build() - }); - }) - .await - } - - #[tokio::test] - async fn workflow_version_inline_create_registers_exact_dependency_first_bytes() { - let server = MockServer::start_async().await; - let registered = Arc::new(Mutex::new(Vec::new())); - let registration = - dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; - let client = no_proxy_client(&server.url("")); - let spec = validated_spec(&json!({ - "workflow": { - "kind": "inline", - "entrypoint": "root/workflow.fabro", - "files": { - "root/workflow.fabro": r#"digraph Root { - start [shape=Mdiamond] - prompt [prompt="@prompt.md"] - child [stack.child_workflow="../child/workflow.fabro"] - exit [shape=Msquare] - start -> prompt -> child -> exit - }"#, - "root/prompt.md": "runtime-authored root bytes", - "child/workflow.fabro": r#"digraph Child { - start [shape=Mdiamond] - task [prompt="@support.md"] - exit [shape=Msquare] - start -> task -> exit - }"#, - "child/support.md": "runtime-authored child bytes" - } - }, - "target": { "kind": "none" }, - "start": false - })); - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Docker, RunId::new(), None); - - let prepared = adapter - .prepare(&client, &spec, Path::new("/host/that-must-not-be-read")) - .await - .expect("inline workflow should prepare"); - - registration.assert_calls_async(2).await; - let registered = registered.lock().unwrap(); - assert_eq!(registered.len(), 2); - let child_id = registered[0].id().unwrap(); - let root_id = registered[1].id().unwrap(); - assert_eq!(prepared.workflow_version_id, root_id); - assert_eq!(prepared.target, RunTarget::None {}); - assert_eq!( - registered[1].workflow_dependencies(), - &BTreeMap::from([( - fabro_types::WorkflowPath::new("child/workflow.fabro").unwrap(), - child_id, - )]) - ); - assert_eq!( - registered[0] - .files() - .get(&fabro_types::WorkflowPath::new("child/support.md").unwrap()) - .map(String::as_str), - Some("runtime-authored child bytes") - ); - assert_eq!( - registered[1] - .files() - .get(&fabro_types::WorkflowPath::new("root/prompt.md").unwrap()) - .map(String::as_str), - Some("runtime-authored root bytes") - ); - } - - #[tokio::test] - async fn workflow_version_inline_entrypoint_is_exact_even_without_an_extension() { - let server = MockServer::start_async().await; - let registered = Arc::new(Mutex::new(Vec::new())); - let registration = - dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; - let client = no_proxy_client(&server.url("")); - let spec = validated_spec(&json!({ - "workflow": { - "kind": "inline", - "entrypoint": "review", - "files": { - "review": "digraph Review { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - } - }, - "target": { "kind": "none" } - })); - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Docker, RunId::new(), None); - - let prepared = adapter - .prepare(&client, &spec, Path::new("/host/that-must-not-be-read")) - .await - .expect("an extensionless inline entrypoint names a supplied file, not a selector"); - - registration.assert_calls_async(1).await; - let registered = registered.lock().unwrap(); - assert_eq!(prepared.workflow_version_id, registered[0].id().unwrap()); - assert_eq!(registered[0].entrypoint().as_str(), "review"); - } - - fn test_parent(target: Option) -> RunProjection { - let mut spec = fabro_types::test_support::test_run_spec(); - spec.target = target; - RunProjection::new(String::new(), spec, chrono::Utc::now()) - } - - async fn mock_parent<'a>(server: &'a MockServer, parent: &RunProjection) -> httpmock::Mock<'a> { - let path = format!("/api/v1/runs/{}/state", parent.spec.id()); - let body = serde_json::to_value(parent).unwrap(); - server - .mock_async(move |when, then| { - when.method(GET).path(path); - then.status(200).json_body(body); - }) - .await - } - - #[tokio::test] - async fn workflow_version_child_checkout_contains_the_parents_pushed_work() { - let temp = tempfile::tempdir().unwrap(); - let workspace = temp.path().join("parent"); - let origin = temp.path().join("origin.git"); - fs::create_dir(&workspace).await.unwrap(); - run_git(temp.path(), &[ - "init", - "--bare", - "--quiet", - origin.to_str().unwrap(), - ]); - run_git(&workspace, &["init", "--quiet", "--initial-branch", "main"]); - run_git(&workspace, &["config", "user.name", "Fabro Test"]); - run_git(&workspace, &["config", "user.email", "fabro@example.com"]); - fs::write(workspace.join("result.txt"), "original") - .await - .unwrap(); - run_git(&workspace, &["add", "."]); - run_git(&workspace, &["commit", "--quiet", "-m", "initial"]); - run_git(&workspace, &[ - "remote", - "add", - "origin", - origin.to_str().unwrap(), - ]); - run_git(&workspace, &["push", "--quiet", "origin", "main"]); - let base_sha = fabro_workflow::git::head_sha(&workspace).unwrap(); - let mut parent = test_parent(Some(RunTarget::Git(GitRunTarget { - repo: "acme/widgets".to_owned(), - branch: "main".to_owned(), - tag: Some("v1.0.0".to_owned()), - sha: Some(base_sha), - }))); - // Construct the adapter before execution starts, exactly as the worker does. - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Docker, parent.spec.id(), None); - let sandbox = fabro_sandbox::LocalSandbox::new(workspace.clone()); - // Docker and Daytona use this same setup operation to create the run branch. - let git = - fabro_sandbox::setup_git_via_exec(&sandbox, &fabro_sandbox::GitSetupIntent::NewRun { - run_id: parent.spec.id().to_string(), - }) - .await - .unwrap(); - parent.start = Some(fabro_types::StartRecord { - start_time: chrono::Utc::now(), - run_branch: Some(git.run_branch.clone()), - base_sha: Some(git.base_sha), - }); - fs::write(workspace.join("result.txt"), "parent implementation") - .await - .unwrap(); - run_git(&workspace, &["add", "."]); - run_git(&workspace, &["commit", "--quiet", "-m", "implement"]); - run_git(&workspace, &["push", "--quiet", "origin", &git.run_branch]); - let server = MockServer::start_async().await; - let state_request = mock_parent(&server, &parent).await; - let client = no_proxy_client(&server.url("")); - let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); - let spec = validated_spec( - &json!({ "workflow": { "kind": "stored", "workflow_version_id": workflow_version_id } }), - ); - let prepared = adapter - .prepare(&client, &spec, Path::new("/must-not-be-read")) - .await - .unwrap(); - state_request.assert_calls_async(1).await; - assert_eq!(prepared.workflow_version_id, workflow_version_id); - let RunTarget::Git(target) = prepared.target else { - panic!("child should have a Git target") - }; - assert_eq!(target.repo, "acme/widgets"); - assert_eq!(target.sha, None); - assert_eq!(target.tag, None); - let child = temp.path().join("child"); - run_git(temp.path(), &[ - "clone", - "--quiet", - "--branch", - &target.branch, - origin.to_str().unwrap(), - child.to_str().unwrap(), - ]); - assert_eq!( - fs::read_to_string(child.join("result.txt")).await.unwrap(), - "parent implementation" - ); - } - - #[test] - fn inherited_target_requires_execution_state_unless_run_branches_are_disabled() { - let mut parent = test_parent(Some(RunTarget::Git(GitRunTarget { - repo: "acme/widgets".to_owned(), - branch: "main".to_owned(), - tag: None, - sha: None, - }))); - assert!( - inherit_parent_target(&parent) - .unwrap_err() - .to_string() - .contains("no execution branch yet") - ); - parent.spec.settings.run.run_branch.enabled = false; - assert_eq!( - inherit_parent_target(&parent).unwrap(), - parent.spec.target.clone().unwrap() - ); - for target in [RunTarget::None {}, RunTarget::Folder { - path: "/shared/workspace".to_owned(), - }] { - parent.spec.target = Some(target.clone()); - assert_eq!(inherit_parent_target(&parent).unwrap(), target); - } - } - - #[tokio::test] - async fn workflow_version_standalone_stored_source_requires_target_without_reading_cwd() { - let client = no_proxy_client("http://127.0.0.1:9"); - let adapter = ServerRunCreateAdapter::standalone(None); - let workflow_version_id: WorkflowVersionId = - fabro_types::BlobHash::new(b"stored with repository config").into(); - let mut spec = validated_spec( - &json!({ "workflow": { "kind": "stored", "workflow_version_id": workflow_version_id } }), - ); - let error = adapter - .prepare(&client, &spec, Path::new("/must-not-be-read")) - .await - .unwrap_err(); - assert!( - error - .to_string() - .contains("stored workflow sources require an explicit target") - ); - spec.target = Some(RunTarget::Git(GitRunTarget { - repo: "acme/upstream".to_owned(), - branch: "main".to_owned(), - tag: None, - sha: None, - })); - let prepared = adapter - .prepare(&client, &spec, Path::new("/must-not-be-read")) - .await - .unwrap(); - assert_eq!(prepared.target, spec.target.unwrap()); - assert_eq!(prepared.workflow_version_id, workflow_version_id); - } - - #[tokio::test] - async fn workflow_version_selector_uses_resolved_package_root_not_operation_cwd() { - let temp = tempfile::tempdir().unwrap(); - let operation_cwd = temp.path().join("nested/operation"); - let workflow_dir = temp.path().join(".fabro/workflows/demo"); - fs::create_dir_all(&operation_cwd).await.unwrap(); - fs::create_dir_all(&workflow_dir).await.unwrap(); - fs::write(temp.path().join(".fabro/project.toml"), "_version = 1\n") - .await - .unwrap(); - fs::write( - workflow_dir.join("workflow.toml"), - "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", - ) - .await - .unwrap(); - fs::write( - workflow_dir.join("workflow.fabro"), - "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", - ) - .await - .unwrap(); - let server = MockServer::start_async().await; - let registered = Arc::new(Mutex::new(Vec::new())); - let registration = - dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; - let client = no_proxy_client(&server.url("")); - let spec = validated_spec(&json!({ - "workflow": "demo", - "target": { "kind": "none" } - })); - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Local, RunId::new(), None); - - let prepared = adapter - .prepare(&client, &spec, &operation_cwd) - .await - .unwrap(); - - registration.assert_calls_async(1).await; - let registered = registered.lock().unwrap(); - assert_eq!(prepared.workflow_version_id, registered[0].id().unwrap()); - assert_eq!( - registered[0].entrypoint().as_str(), - ".fabro/workflows/demo/workflow.fabro" - ); - } - - #[tokio::test] - async fn workflow_version_worker_capabilities_gate_selector_and_goal_file_before_reads() { - let temp = tempfile::tempdir().unwrap(); - let workflow = temp.path().join("same-name.fabro"); - fs::write( - &workflow, - "digraph HostCopy { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", - ) - .await - .unwrap(); - fs::write( - temp.path().join("goal.md"), - "host goal that must not be read", - ) - .await - .unwrap(); - let client = no_proxy_client("http://127.0.0.1:9"); - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Daytona, RunId::new(), None); - - let selector = validated_spec(&json!({ - "workflow": "same-name.fabro", - "target": { "kind": "none" } - })); - let selector_error = adapter - .prepare(&client, &selector, temp.path()) - .await - .expect_err("Daytona worker must reject host selectors"); - assert!( - selector_error - .to_string() - .contains("inline files or an exact stored") - ); - - let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); - let goal_file = validated_spec(&json!({ - "workflow": { - "kind": "stored", - "workflow_version_id": workflow_version_id - }, - "target": { "kind": "none" }, - "goal_file": "goal.md" - })); - let goal_error = adapter - .prepare(&client, &goal_file, temp.path()) - .await - .expect_err("Daytona worker must reject host goal files"); - assert!(goal_error.to_string().contains("send goal text by value")); - } - - #[tokio::test] - async fn workflow_version_clone_based_workers_reject_explicit_folder_targets() { - let client = no_proxy_client("http://127.0.0.1:9"); - let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); - let spec = validated_spec(&json!({ - "workflow": { - "kind": "stored", - "workflow_version_id": workflow_version_id - }, - "target": { "kind": "folder", "path": "/srv/server-workspace" } - })); - - for provider in [EnvironmentProvider::Docker, EnvironmentProvider::Daytona] { - let adapter = ServerRunCreateAdapter::worker(provider, RunId::new(), None); - let error = adapter - .prepare(&client, &spec, Path::new("/ignored")) - .await - .expect_err("clone-based workers must not select server-host folders"); - - assert!( - error - .to_string() - .contains("cannot select server-host folders"), - "unexpected error for {provider:?}: {error:#}" - ); - } - } - - #[tokio::test] - async fn workflow_version_local_worker_accepts_explicit_folder_target() { - let client = no_proxy_client("http://127.0.0.1:9"); - let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); - let target = RunTarget::Folder { - path: "/srv/server-workspace".to_string(), - }; - let spec = validated_spec(&json!({ - "workflow": { - "kind": "stored", - "workflow_version_id": workflow_version_id - }, - "target": target - })); - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Local, RunId::new(), None); - - let prepared = adapter - .prepare(&client, &spec, Path::new("/ignored")) - .await - .unwrap(); - - assert_eq!(prepared.target, target); - } - - #[tokio::test] - async fn workflow_version_is_registered_before_server_admission_rejection() { - let server = MockServer::start_async().await; - let registered = Arc::new(Mutex::new(Vec::new())); - let registration = - dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; - let admission = server - .mock_async(|when, then| { - when.method(POST).path("/api/v1/runs"); - then.status(422) - .header("content-type", "text/plain") - .body("server-authoritative workflow rejection"); - }) - .await; - let client = Arc::new(no_proxy_client(&server.url(""))); - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Docker, RunId::new(), None); - let backend = - ClientBackend::new(Arc::clone(&client)).with_run_create_adapter(Arc::new(adapter)); - let spec = validated_spec(&json!({ - "workflow": { - "kind": "inline", - "entrypoint": "workflow.fabro", - "files": { - "workflow.fabro": r#"digraph W { - start [shape=Mdiamond] - task [prompt="@prompt.md"] - exit [shape=Msquare] - start -> task -> exit - }"#, - "prompt.md": "Hello {{ inputs.owner }}" - } - }, - "target": { "kind": "none" } - })); - let error = backend - .create_run_from_spec(&spec, Path::new("/ignored"), None) - .await - .expect_err("the server should reject the semantically invalid workflow"); - - registration.assert_calls_async(1).await; - admission.assert_calls_async(1).await; - assert_eq!(registered.lock().unwrap().len(), 1); - assert!( - error - .to_string() - .contains("server-authoritative workflow rejection"), - "unexpected error: {error:#}" - ); - } - - #[tokio::test] - async fn workflow_version_target_failure_precedes_registration() { - let parent = test_parent(None); - let server = MockServer::start_async().await; - let state_request = mock_parent(&server, &parent).await; - let client = no_proxy_client(&server.url("")); - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Docker, parent.spec.id(), None); - let spec = validated_spec(&json!({ - "workflow": { - "kind": "inline", - "entrypoint": "workflow.fabro", - "files": { - "workflow.fabro": "digraph W { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - } - } - })); - - let error = adapter - .prepare(&client, &spec, Path::new("/ignored")) - .await - .expect_err("missing inherited target should fail before registration"); - - state_request.assert_calls_async(1).await; - assert!( - error - .to_string() - .contains("parent run has no canonical target"), - "unexpected error: {error:#}" - ); - } - - #[tokio::test] - async fn workflow_version_shared_goal_file_and_explicit_target_are_preserved() { - let temp = tempfile::tempdir().unwrap(); - fs::write(temp.path().join("goal.md"), "goal from shared filesystem") - .await - .unwrap(); - let client = no_proxy_client("http://127.0.0.1:9"); - let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); - let spec = validated_spec(&json!({ - "workflow": { - "kind": "stored", - "workflow_version_id": workflow_version_id - }, - "target": { "kind": "none" }, - "goal_file": "goal.md" - })); - let adapter = - ServerRunCreateAdapter::worker(EnvironmentProvider::Local, RunId::new(), None); - - let prepared = adapter.prepare(&client, &spec, temp.path()).await.unwrap(); - - assert_eq!(prepared.target, RunTarget::None {}); - assert_eq!( - prepared.goal.as_deref(), - Some("goal from shared filesystem") - ); - } - - #[tokio::test] - async fn workflow_version_standalone_rejects_unavailable_head_before_registration_or_create() { - let temp = tempfile::tempdir().unwrap(); - let workspace = temp.path().join("workspace"); - fs::create_dir(&workspace).await.unwrap(); - run_git(&workspace, &[ - "init", - "--quiet", - "--initial-branch", - "feature", - ]); - run_git(&workspace, &["config", "user.name", "Fabro Test"]); - run_git(&workspace, &["config", "user.email", "fabro@example.com"]); - fs::write(workspace.join("tracked.txt"), "committed") - .await - .unwrap(); - run_git(&workspace, &["add", "tracked.txt"]); - run_git(&workspace, &["commit", "--quiet", "-m", "initial"]); - run_git(&workspace, &[ - "remote", - "add", - "origin", - "https://github.com/acme/widgets.git", - ]); - let missing = format!("file://{}/missing.git", temp.path().display()); - run_git(&workspace, &[ - "remote", "set-url", "--push", "origin", &missing, - ]); - let spec = validated_spec(&json!({ - "workflow": { - "kind": "inline", - "entrypoint": "workflow.fabro", - "files": { - "workflow.fabro": "digraph W { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - } - } - })); - let server = MockServer::start_async().await; - mock_environment(&server, "default", "docker").await; - let registered = Arc::new(Mutex::new(Vec::new())); - let registration = - dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; - let create = server - .mock_async(|when, then| { - when.method(POST).path("/api/v1/runs"); - then.status(500); - }) - .await; - let client = Arc::new(no_proxy_client(&server.url(""))); - let adapter = ServerRunCreateAdapter::standalone(None); - let backend = - ClientBackend::new(Arc::clone(&client)).with_run_create_adapter(Arc::new(adapter)); - - let error = backend - .create_run_from_spec(&spec, &workspace, None) - .await - .expect_err("an unavailable local HEAD must not degrade to a branch-only target"); - - registration.assert_calls_async(0).await; - create.assert_calls_async(0).await; - assert!(registered.lock().unwrap().is_empty()); - assert!( - error - .to_string() - .contains("exact local Git commit could not be made available"), - "unexpected error: {error:#}" - ); - } - - #[tokio::test] - async fn workflow_version_standalone_preserves_dirty_warning_for_exact_target() { - let temp = tempfile::tempdir().unwrap(); - let workspace = temp.path().join("workspace"); - let origin = temp.path().join("origin.git"); - fs::create_dir(&workspace).await.unwrap(); - run_git(temp.path(), &[ - "init", - "--bare", - "--quiet", - origin.to_str().unwrap(), - ]); - run_git(&workspace, &[ - "init", - "--quiet", - "--initial-branch", - "feature", - ]); - run_git(&workspace, &["config", "user.name", "Fabro Test"]); - run_git(&workspace, &["config", "user.email", "fabro@example.com"]); - fs::write(workspace.join("tracked.txt"), "committed") - .await - .unwrap(); - run_git(&workspace, &["add", "tracked.txt"]); - run_git(&workspace, &["commit", "--quiet", "-m", "initial"]); - run_git(&workspace, &[ - "remote", - "add", - "origin", - "https://github.com/acme/widgets.git", - ]); - let push_url = format!("file://{}", origin.display()); - run_git(&workspace, &[ - "remote", "set-url", "--push", "origin", &push_url, - ]); - fs::write(workspace.join("dirty.txt"), "uncommitted") - .await - .unwrap(); - - let spec = validated_spec(&json!({ - "workflow": { - "kind": "inline", "entrypoint": "main.fabro", "files": { - "main.fabro": "digraph W { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - } - }, - "environment": "sandbox" - })); - let server = MockServer::start_async().await; - let environment = mock_environment(&server, "sandbox", "daytona").await; - let registration = - dynamic_version_registration_mock(&server, Arc::new(Mutex::new(Vec::new()))).await; - let client = no_proxy_client(&server.url("")); - let adapter = ServerRunCreateAdapter::standalone(None); - - let prepared = adapter.prepare(&client, &spec, &workspace).await.unwrap(); - - environment.assert_calls_async(1).await; - registration.assert_calls_async(1).await; - let RunTarget::Git(target) = prepared.target else { - panic!("standalone attached Git checkout should derive a Git target"); - }; - assert_eq!(target.repo, "acme/widgets"); - assert_eq!(target.branch, "feature"); - assert!(target.sha.is_some()); - assert!( - prepared - .warnings - .iter() - .any(|warning| warning.contains("uncommitted changes")) - ); - } - - #[tokio::test] - async fn workflow_version_standalone_local_environment_targets_the_caller_folder() { - let temp = tempfile::tempdir().unwrap(); - let workspace = temp.path().join("plain"); - fs::create_dir(&workspace).await.unwrap(); - let spec = validated_spec(&json!({ - "workflow": { "kind": "inline", "entrypoint": "main.fabro", "files": { - "main.fabro": "digraph W { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - }}, - "environment": "local" - })); - let server = MockServer::start_async().await; - mock_environment(&server, "local", "local").await; - let registration = - dynamic_version_registration_mock(&server, Arc::new(Mutex::new(Vec::new()))).await; - let client = no_proxy_client(&server.url("")); - let adapter = ServerRunCreateAdapter::standalone(None); - - let prepared = adapter.prepare(&client, &spec, &workspace).await.unwrap(); - registration.assert_calls_async(1).await; - - let expected = workspace.canonicalize().unwrap(); - assert_eq!(prepared.target, RunTarget::Folder { - path: expected.to_str().unwrap().to_string(), - }); - assert!(prepared.warnings.is_empty()); - } - - #[tokio::test] - async fn workflow_version_standalone_clone_environment_without_git_metadata_runs_empty() { - let temp = tempfile::tempdir().unwrap(); - let workspace = temp.path().join("plain"); - fs::create_dir(&workspace).await.unwrap(); - let spec = validated_spec(&json!({ - "workflow": { - "kind": "inline", "entrypoint": "main.fabro", "files": { - "main.fabro": "digraph W { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" - } - } - })); - let server = MockServer::start_async().await; - mock_environment(&server, "default", "docker").await; - let registration = - dynamic_version_registration_mock(&server, Arc::new(Mutex::new(Vec::new()))).await; - let client = no_proxy_client(&server.url("")); - let adapter = ServerRunCreateAdapter::standalone(None); - - let prepared = adapter.prepare(&client, &spec, &workspace).await.unwrap(); - registration.assert_calls_async(1).await; - - assert_eq!(prepared.target, RunTarget::None {}); - } - - #[tokio::test] - async fn workflow_version_standalone_honors_configured_scm_repository_over_checkout_origin() { - let temp = tempfile::tempdir().unwrap(); - let workspace = temp.path().join("workspace"); - let origin = temp.path().join("origin.git"); - fs::create_dir(&workspace).await.unwrap(); - run_git(temp.path(), &[ - "init", - "--bare", - "--quiet", - origin.to_str().unwrap(), - ]); - run_git(&workspace, &[ - "init", - "--quiet", - "--initial-branch", - "feature", - ]); - run_git(&workspace, &["config", "user.name", "Fabro Test"]); - run_git(&workspace, &["config", "user.email", "fabro@example.com"]); - let workflow_dir = workspace.join(".fabro/workflows/demo"); - fs::create_dir_all(&workflow_dir).await.unwrap(); - fs::write(workspace.join(".fabro/project.toml"), "_version = 1\n") - .await - .unwrap(); - fs::write( - workflow_dir.join("workflow.toml"), - "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.scm]\nowner = \"acme\"\nrepository = \"widgets\"\n", - ) - .await - .unwrap(); - fs::write( - workflow_dir.join("workflow.fabro"), - "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", - ) - .await - .unwrap(); - run_git(&workspace, &["add", "."]); - run_git(&workspace, &["commit", "--quiet", "-m", "initial"]); - // The checkout is a fork; the workflow names the upstream repository. - run_git(&workspace, &[ - "remote", - "add", - "origin", - "https://github.com/alice/widgets.git", - ]); - let push_url = format!("file://{}", origin.display()); - run_git(&workspace, &[ - "remote", "set-url", "--push", "origin", &push_url, - ]); - let server = MockServer::start_async().await; - mock_environment(&server, "default", "docker").await; - let registered = Arc::new(Mutex::new(Vec::new())); - let registration = - dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; - let client = no_proxy_client(&server.url("")); - let spec = validated_spec(&json!({ "workflow": "demo" })); - let adapter = ServerRunCreateAdapter::standalone(None); - - let error = adapter - .prepare(&client, &spec, &workspace) - .await - .expect_err("a fork checkout must not silently become the run's repository"); - - registration.assert_calls_async(0).await; - assert!( - error - .to_string() - .contains("run.scm repository that is not the local checkout's origin"), - "unexpected error: {error:#}" - ); +fn run_with_status( + run_id: RunId, + parent_id: Option, + children_count: u64, + status: RunStatus, +) -> Run { + Run { + id: run_id, + parent_id, + children_count, + title: "Test run".to_string(), + goal: "Test run".to_string(), + workflow: WorkflowRef { + slug: Some("simple".to_string()), + name: Some("Simple".to_string()), + graph_name: None, + node_count: 0, + edge_count: 0, + }, + automation: None, + repository: None, + created_by: test_support::test_principal(), + origin: RunOrigin::default(), + labels: HashMap::new(), + lifecycle: RunLifecycle { + status, + approval: None, + pending_control: None, + queue_position: None, + error: None, + archived: false, + archived_at: None, + }, + sandbox: None, + models: Vec::new(), + source_directory: Some("/srv/repo".to_string()), + timestamps: RunTimestamps { + created_at: Utc.with_ymd_and_hms(2026, 4, 5, 12, 0, 0).unwrap(), + started_at: None, + last_event_at: None, + completed_at: None, + }, + timing: None, + billing: None, + size: fabro_types::RunSize::default(), + ask_fabro: fabro_types::AskFabro::default(), + diff: None, + pull_request: None, + current_question: None, + superseded_by: None, + retried_from: None, + links: RunLinks { web: None }, } } diff --git a/lib/apps/fabro-server/src/server/handler/sessions.rs b/lib/apps/fabro-server/src/server/handler/sessions.rs index 1e7c1f67c..9b3a01f58 100644 --- a/lib/apps/fabro-server/src/server/handler/sessions.rs +++ b/lib/apps/fabro-server/src/server/handler/sessions.rs @@ -1,7 +1,6 @@ use std::collections::HashMap; use std::convert::Infallible; use std::fmt::Write as _; -use std::path::PathBuf; use std::sync::Arc; use axum::extract::{Path, Query, State}; @@ -751,7 +750,6 @@ async fn build_agent( let services = FabroRunToolServices { backend: Arc::new(backend), current_run_id: run_id, - base_cwd: PathBuf::new(), }; let run_tools = register_named_fabro_run_tools(&services, ASK_FABRO_RUN_TOOL_NAMES); let selector = format!("{provider_id}/{model}"); diff --git a/lib/apps/fabro-server/src/server/handler/workflow_versions.rs b/lib/apps/fabro-server/src/server/handler/workflow_versions.rs index e8f8b85a4..cd8daafc7 100644 --- a/lib/apps/fabro-server/src/server/handler/workflow_versions.rs +++ b/lib/apps/fabro-server/src/server/handler/workflow_versions.rs @@ -120,7 +120,6 @@ mod tests { }; use crate::server; use crate::test_support::{self, TestAppStateBuilder}; - use crate::worker_token::{WorkerScopeSet, issue_worker_token, issue_worker_token_with_scopes}; const GRAPH: &str = "digraph W { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }"; @@ -162,79 +161,6 @@ mod tests { assert_eq!(response.status(), StatusCode::UNAUTHORIZED); } - #[tokio::test] - async fn workflow_version_registration_authorization_is_narrow() { - let state = TestAppStateBuilder::new().build(); - let app = server::build_router(Arc::clone(&state), test_support::test_auth_mode()); - let run_id = fabro_types::RunId::new(); - let scoped = issue_worker_token_with_scopes( - state.worker_token_keys(), - &run_id, - WorkerScopeSet::run_worker_with_agent_run_tools(), - ) - .unwrap(); - let ordinary = issue_worker_token(state.worker_token_keys(), &run_id).unwrap(); - - let authorized = app - .clone() - .oneshot( - Request::builder() - .method(Method::POST) - .uri("/api/v1/workflow-versions") - .header(header::CONTENT_TYPE, "application/json") - .header(header::AUTHORIZATION, format!("Bearer {scoped}")) - .body(Body::from(serde_json::to_vec(&version(GRAPH)).unwrap())) - .unwrap(), - ) - .await - .unwrap(); - fabro_test::expect_axum_status( - authorized, - StatusCode::CREATED, - "scoped worker POST /api/v1/workflow-versions", - ) - .await; - - let forbidden = app - .clone() - .oneshot( - Request::builder() - .method(Method::POST) - .uri("/api/v1/workflow-versions") - .header(header::CONTENT_TYPE, "application/json") - .header(header::AUTHORIZATION, format!("Bearer {ordinary}")) - .body(Body::from(serde_json::to_vec(&version(GRAPH)).unwrap())) - .unwrap(), - ) - .await - .unwrap(); - fabro_test::expect_axum_status( - forbidden, - StatusCode::FORBIDDEN, - "ordinary worker POST /api/v1/workflow-versions", - ) - .await; - - let malformed = app - .oneshot( - Request::builder() - .method(Method::POST) - .uri("/api/v1/workflow-versions") - .header(header::CONTENT_TYPE, "application/json") - .header(header::AUTHORIZATION, "Bearer not-a-worker-token") - .body(Body::from(serde_json::to_vec(&version(GRAPH)).unwrap())) - .unwrap(), - ) - .await - .unwrap(); - fabro_test::expect_axum_status( - malformed, - StatusCode::UNAUTHORIZED, - "malformed worker POST /api/v1/workflow-versions", - ) - .await; - } - #[tokio::test] async fn valid_and_equivalent_requests_return_the_same_id() { let state = TestAppStateBuilder::new().build(); diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 3dfadcf15..1b86f0c90 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -19927,3 +19927,57 @@ async fn workflow_version_registration_requires_user_or_run_tools_capability() { .is_none() ); } + +#[tokio::test] +async fn run_tools_worker_registers_contents_then_creates_by_version_id() { + let (state, app) = jwt_auth_app(); + let parent_id = create_run_with_bearer(&app, &issue_test_user_jwt()).await; + let token = issue_test_run_tools_worker_token(&parent_id); + let response = app + .clone() + .oneshot(json_bearer_request( + Method::POST, + "/workflow-versions", + &token, + &json!({ + "entrypoint": "child.fabro", + "files": {"child.fabro": MINIMAL_DOT}, + "workflow_dependencies": {} + }), + )) + .await + .unwrap(); + let registered = response_json!(response, StatusCode::CREATED).await; + let response = app + .oneshot(json_bearer_request( + Method::POST, + "/runs", + &token, + &json!({ + "workflow_version_id": registered["workflow_version_id"], + "target": {"kind": "none"}, + "args": {"dry_run": true}, + "parent_id": parent_id, + "goal": "A child created from sandbox-supplied contents" + }), + )) + .await + .unwrap(); + let child = response_json!(response, StatusCode::CREATED).await; + assert_eq!(child["parent_id"], parent_id.to_string()); + assert_eq!(child["lifecycle"]["status"]["kind"], "submitted"); + let child_id = child["id"].as_str().unwrap().parse::().unwrap(); + let projection = state + .stores + .runs + .load_run_projection(&child_id) + .await + .unwrap() + .unwrap(); + assert_eq!( + serde_json::to_value(projection.spec.workflow_version_id).unwrap(), + registered["workflow_version_id"] + ); + assert_eq!(projection.spec.target, Some(RunTarget::None {})); + assert!(projection.start.is_none()); +} diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index 532bc2e0e..43a028dd1 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -31,12 +31,10 @@ use fabro_graphviz::parser; use fabro_template::validate_static_reference; use fabro_types::graph::ReferenceKind; use fabro_types::settings::interp::InterpString; -use fabro_types::settings::run::{ - ApprovalMode, EnvironmentProvider, ResolvedGoalSource, ResolvedRunGoal, RunMode, -}; +use fabro_types::settings::run::{ApprovalMode, ResolvedGoalSource, ResolvedRunGoal, RunMode}; use fabro_types::{ DirtyStatus, GitContext, GitHubRepositorySlug, GitRunTarget, ManifestPath, RunTarget, - WorkflowSettings, + SandboxProviderKind, WorkflowSettings, }; use fabro_workflow::git::{self, GitSyncStatus}; pub use fabro_workflow_version::CollectedWorkflowClosure; @@ -49,7 +47,6 @@ use crate::workflow_bundler::WorkflowBundler; pub use crate::workflow_version_collector::{ MAX_WORKFLOW_VERSION_DEPTH, WorkflowVersionCollectError, collect_workflow_versions, collect_workflow_versions_at_location, - collect_inline_workflow_versions, }; pub use crate::workflow_version_packager::SuppliedWorkflowVersionPackager; @@ -418,7 +415,7 @@ pub enum RunTargetDerivationError { )] Unverified, #[error( - "the workflow configures a run.scm repository that is not the local checkout's origin; run from a checkout of the configured repository or pass an explicit target" + "the workflow configures a run.scm repository that is not the local checkout's origin; run from a checkout of the configured repository" )] ConfiguredOriginMismatch, #[error("failed to inspect caller working directory {} for Git metadata", path.display())] @@ -462,13 +459,13 @@ pub enum RunTargetDerivationError { /// # Errors /// /// Returns the reason a clone-based target could not be derived; every -/// variant's message is written for the caller of the run tool or CLI. +/// variant's message is written for the CLI caller. pub fn derive_run_target_for_provider( - provider: EnvironmentProvider, + provider: &SandboxProviderKind, canonical_cwd: &Path, configured_repo_origin_url: Option<&str>, ) -> std::result::Result { - if !provider.is_clone_based() { + if !provider.clones_workspace() { let path = canonical_cwd .to_str() .ok_or_else(|| RunTargetDerivationError::NonUtf8Path { @@ -542,21 +539,6 @@ fn none_target_for_unversioned_directory( outcome } -/// The workflow's configured `run.scm` GitHub repository as a normalized -/// origin URL, read from `workflow.toml` source text. `None` when the config -/// names no GitHub repository. -/// -/// # Errors -/// -/// Returns an error when the source cannot be parsed or resolved. -pub fn configured_repo_origin_url_from_workflow_toml(source: &str) -> Result> { - let run = parse_run_layer_from_settings_toml(source) - .context("failed to parse run settings from workflow.toml")?; - Ok(configured_repo_origin_url_from_scm_layer( - &run.scm.unwrap_or_default(), - )) -} - /// The configured `run.scm` GitHub repository for a resolved local workflow. /// `workflow.toml` values take precedence over the discovered /// `.fabro/project.toml`, field by field, matching run settings layering. @@ -885,28 +867,6 @@ pub(crate) mod test_fixtures { #[cfg(test)] mod tests { - #[test] - fn configured_repo_origin_url_reads_run_scm_from_workflow_toml() { - let configured = super::configured_repo_origin_url_from_workflow_toml( - "_version = 1\n[run.scm]\nowner = \"acme\"\nrepository = \"widgets\"\n", - ) - .unwrap(); - assert_eq!( - configured.as_deref(), - Some("https://github.com/acme/widgets") - ); - - let unconfigured = - super::configured_repo_origin_url_from_workflow_toml("_version = 1\n").unwrap(); - assert_eq!(unconfigured, None); - - let other_provider = super::configured_repo_origin_url_from_workflow_toml( - "_version = 1\n[run.scm]\nprovider = \"gitlab\"\nowner = \"acme\"\nrepository = \"widgets\"\n", - ) - .unwrap(); - assert_eq!(other_provider, None); - } - use fabro_workflow::git::head_sha; use super::*; diff --git a/lib/components/fabro-manifest/src/local_workflow_package.rs b/lib/components/fabro-manifest/src/local_workflow_package.rs index 0e9270676..5c32a4412 100644 --- a/lib/components/fabro-manifest/src/local_workflow_package.rs +++ b/lib/components/fabro-manifest/src/local_workflow_package.rs @@ -66,11 +66,6 @@ impl ResolvedLocalWorkflowPackage { pub fn closure(&self) -> &CollectedWorkflowClosure { &self.closure } - - #[must_use] - pub fn into_closure(self) -> CollectedWorkflowClosure { - self.closure - } } /// Resolve producer-readable workflow bytes under one stable local source diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index a02ccec96..131f9990c 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -186,79 +186,6 @@ pub fn collect_workflow_versions_at_location( VersionAssembler::new(collected).assemble() } -/// Package a workflow whose bytes arrive by value. `entrypoint` is an exact -/// key of `files`; unlike a checkout selector, an extensionless entrypoint -/// is never rewritten to a `.fabro/workflows//workflow.toml` lookup. -/// The files are staged in a private temporary root only for the duration of -/// collection, so the resulting closure's paths are rooted at the file map. -/// -/// # Errors -/// -/// Returns a collision error before touching the filesystem when two paths -/// cannot coexist on one filesystem; staging and collection failures are -/// reported against the entrypoint. -pub fn collect_inline_workflow_versions( - entrypoint: &WorkflowPath, - files: &BTreeMap, -) -> Result { - if !files.contains_key(entrypoint) { - return Err(WorkflowVersionCollectError::MissingWorkflow { - path: entrypoint.to_string(), - }); - } - fabro_types::validate_workflow_source_paths(files.keys()).map_err(|error| match error { - WorkflowVersionShapeError::PathCollision { second, .. } => { - WorkflowVersionCollectError::PathCollision { - entrypoint: entrypoint.clone(), - path: second, - } - } - source => WorkflowVersionCollectError::InvalidShape { - entrypoint: entrypoint.clone(), - source, - }, - })?; - let collect_error = |source: anyhow::Error| WorkflowVersionCollectError::Collect { - path: PathBuf::from(entrypoint.as_str()), - source, - }; - let root = tempfile::tempdir().map_err(|source| { - collect_error( - anyhow::Error::new(source).context("failed to create private inline workflow root"), - ) - })?; - for (path, content) in files { - let destination = root.path().join(path.as_str()); - if let Some(parent) = destination.parent() { - std::fs::create_dir_all(parent).map_err(|source| { - collect_error(anyhow::Error::new(source).context(format!( - "failed to create inline workflow directory for `{path}`" - ))) - })?; - } - std::fs::write(&destination, content).map_err(|source| { - collect_error( - anyhow::Error::new(source) - .context(format!("failed to write inline workflow file `{path}`")), - ) - })?; - } - let package_root = root.path().canonicalize().map_err(|source| { - collect_error( - anyhow::Error::new(source).context("failed to canonicalize the inline workflow root"), - ) - })?; - let location = WorkflowLocation::from_exact_path(Path::new(entrypoint.as_str()), &package_root) - .map_err(|source| collect_error(source.into()))?; - let location = canonicalize_location(location, |path, source| { - collect_error(anyhow::Error::new(source).context(format!( - "failed to canonicalize inline workflow path {}", - path.display() - ))) - })?; - collect_workflow_versions_at_location(&location, &package_root, Path::new(entrypoint.as_str())) -} - fn repository_workflow_path(workflow: &Path) -> PathBuf { if workflow.is_relative() && workflow.extension().is_none() { Path::new(".fabro/workflows") @@ -498,43 +425,6 @@ dockerfile = { path = "Dockerfile" } ); } - #[test] - fn inline_collection_uses_the_exact_entrypoint_and_rejects_collisions_before_staging() { - let files = BTreeMap::from([ - ( - WorkflowPath::new("review").unwrap(), - "digraph Review {}".to_string(), - ), - ( - WorkflowPath::new("notes/detail.md").unwrap(), - "detail".to_string(), - ), - ]); - let closure = - collect_inline_workflow_versions(&WorkflowPath::new("review").unwrap(), &files) - .unwrap(); - let (_, root) = closure.versions().next().unwrap(); - assert_eq!(root.version().entrypoint().as_str(), "review"); - - for (file, descendant) in [ - ("a", "a/b.md"), - ("A", "a/b.md"), - ("dir/File", "DIR/file/child.md"), - ("Prompt.md", "prompt.md"), - ] { - let entrypoint = WorkflowPath::new(file).unwrap(); - let colliding = BTreeMap::from([ - (entrypoint.clone(), "digraph A {}".to_string()), - (WorkflowPath::new(descendant).unwrap(), "b".to_string()), - ]); - let error = collect_inline_workflow_versions(&entrypoint, &colliding).unwrap_err(); - assert!( - matches!(error, WorkflowVersionCollectError::PathCollision { .. }), - "unexpected error: {error:#}" - ); - } - } - #[test] fn packages_named_workflow_without_project_config() { let temp = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-tool/src/common.rs b/lib/components/fabro-tool/src/common.rs index 9cebd72d2..7b92b3a09 100644 --- a/lib/components/fabro-tool/src/common.rs +++ b/lib/components/fabro-tool/src/common.rs @@ -1,5 +1,4 @@ use std::collections::HashMap; -use std::path::Path; use std::sync::LazyLock; use async_trait::async_trait; @@ -7,7 +6,7 @@ use chrono::{DateTime, NaiveDate, Utc}; use fabro_api::types; use fabro_types::{ PairId, PairMessageRecord, PairMessageRequest, PairRecord, PairTranscriptResponse, Run, RunId, - RunPairStatusResponse, RunTarget, StageId, WorkflowVersionId, + RunPairStatusResponse, StageId, }; use fabro_util::exit::{self, ExitClass}; use schemars::JsonSchema; @@ -46,37 +45,6 @@ impl std::error::Error for ToolError {} pub type ToolResult = Result; -#[derive(Debug)] -pub struct PreparedRunCreate { - pub workflow_version_id: WorkflowVersionId, - pub target: RunTarget, - pub goal: Option, - pub warnings: Vec, -} - -#[derive(Debug)] -pub struct CreateRunSubmission { - pub run_id: RunId, - pub warnings: Vec, -} - -/// Trusted producer-local preparation for one tool-created run. -/// -/// Implementations acquire permitted workflow bytes and goal files, enforce -/// producer capabilities and package structure, resolve the independent -/// target, and register immutable workflow versions before returning -/// intent-ready fields. The server remains authoritative for semantic run -/// admission. -#[async_trait] -pub trait RunCreateAdapter: Send + Sync { - async fn prepare( - &self, - client: &fabro_client::Client, - spec: &crate::ValidatedCreateRunSpec, - cwd: &Path, - ) -> anyhow::Result; -} - #[async_trait] pub trait FabroToolBackend: Send + Sync { async fn create_workflow_version( @@ -86,12 +54,8 @@ pub trait FabroToolBackend: Send + Sync { Err(workflow_version_tool_unavailable_error()) } - async fn create_run_from_spec( - &self, - spec: &crate::ValidatedCreateRunSpec, - cwd: &Path, - parent_id: Option, - ) -> anyhow::Result; + async fn create_run_from_intent(&self, intent: fabro_types::RunIntent) + -> anyhow::Result; async fn resolve_run(&self, selector: &str) -> anyhow::Result; async fn retrieve_run(&self, run_id: &RunId) -> anyhow::Result; @@ -222,7 +186,7 @@ static TOOL_DEFINITIONS: LazyLock> = LazyLock::new(|| { ), tool_definition::( FABRO_RUN_CREATE_TOOL_NAME, - "Create one or more Fabro workflow runs from a native selector, inline files, or an exact stored workflow version, optionally under a parent run and starting them by default.", + "Create runs from registered workflow_version_id values and canonical RunIntent settings. Register contents with fabro_workflow_version_create first. Standalone calls require an explicit target; native workers may inherit the parent target. Starts runs by default.", ), tool_definition::( FABRO_RUN_SEARCH_TOOL_NAME, @@ -350,7 +314,6 @@ mod tests { use fabro_types::{ RunLifecycle, RunLinks, RunOrigin, RunStatus, RunTimestamps, WorkflowRef, test_support, }; - use serde_json::Value; use super::*; @@ -390,44 +353,6 @@ mod tests { ); } - #[test] - fn create_tool_definition_advertises_complete_workflow_source_and_target_grammar() { - let definition = tool_definitions() - .iter() - .find(|definition| definition.name == FABRO_RUN_CREATE_TOOL_NAME) - .expect("create tool should be in the shared catalog"); - let variants = definition - .parameters - .pointer("/properties/runs/items/anyOf") - .and_then(Value::as_array) - .expect("run item union should be present"); - assert!(variants.iter().any(|variant| variant["type"] == "string")); - let object = variants - .iter() - .find(|variant| variant["type"] == "object") - .expect("object create specification should be present"); - assert_eq!(object["additionalProperties"], false); - let workflow = object - .pointer("/properties/workflow/anyOf") - .and_then(Value::as_array) - .expect("workflow source union should be present"); - assert!(workflow.iter().any(|variant| variant["type"] == "string")); - for kind in ["inline", "stored"] { - assert!(workflow.iter().any(|variant| { - variant.pointer("/properties/kind/const") == Some(&Value::from(kind)) - })); - } - let target = object - .pointer("/properties/target/anyOf") - .and_then(Value::as_array) - .expect("target union should be present"); - for kind in ["git", "none", "folder"] { - assert!(target.iter().any(|variant| { - variant.pointer("/properties/kind/const") == Some(&Value::from(kind)) - })); - } - } - #[test] fn pair_tool_definition_exposes_pair_schema() { let definition = tool_definitions() diff --git a/lib/components/fabro-tool/src/create.rs b/lib/components/fabro-tool/src/create.rs index 4aa13ab43..21449d610 100644 --- a/lib/components/fabro-tool/src/create.rs +++ b/lib/components/fabro-tool/src/create.rs @@ -1,137 +1,234 @@ -use std::borrow::Cow; -use std::collections::{BTreeMap, HashMap}; -use std::path::{Path, PathBuf}; use std::sync::Arc; -use fabro_types::{RunId, RunTarget, WorkflowPath, WorkflowVersionId}; +use anyhow::Context as _; +use fabro_types::{RunId, RunIntent, RunIntentArgs, RunProjection, RunTarget, WorkflowVersionId}; use schemars::{JsonSchema, Schema, SchemaGenerator, json_schema}; use serde::{Deserialize, Deserializer, Serialize, de}; -use serde_json::Value; use super::common::{self, FabroToolBackend, ToolError, ToolResult}; use super::manifest; #[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct FabroRunCreateParams { - pub runs: Vec, + #[serde(deserialize_with = "deserialize_runs")] + #[schemars(length(min = 1, max = 50))] + pub runs: Vec, } -#[derive(Debug)] -pub enum CreateRunSpecInput { - Workflow(String), - Spec(Box), +fn deserialize_runs<'de, D: Deserializer<'de>>( + deserializer: D, +) -> Result, D::Error> { + Vec::::deserialize(deserializer).map_err(|error| { + de::Error::custom(format!( + "fabro_run_create requires workflow_version_id and canonical RunIntent fields; register file contents with fabro_workflow_version_create first: {error}" + )) + }) } -impl<'de> Deserialize<'de> for CreateRunSpecInput { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let value = Value::deserialize(deserializer)?; - match value { - Value::String(workflow) => Ok(Self::Workflow(workflow)), - Value::Object(_) => CreateRunSpec::deserialize(value) - .map(Box::new) - .map(Self::Spec) - .map_err(de::Error::custom), - other => Err(de::Error::custom(format!( - "expected workflow string shorthand or create spec object, got {}", - json_value_kind(&other) - ))), +/// RunIntent fields plus tool-only parent/target defaults and a separate start +/// request. +#[derive(Debug, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct CreateRunSpec { + #[schemars(with = "String")] + pub workflow_version_id: WorkflowVersionId, + #[serde(default)] + #[schemars(schema_with = "run_target_schema")] + pub target: Option, + #[serde(default)] + #[schemars(schema_with = "run_args_schema")] + pub args: RunIntentArgs, + pub environment_id: Option, + #[schemars(with = "Option")] + pub parent_id: Option, + pub title: Option, + pub goal: Option, + pub start: Option, +} + +#[derive(Debug, Clone, Copy, Default)] +pub struct CreateRunOptions { + /// Set only by trusted native worker dispatch, never from tool JSON. + pub forced_parent_id: Option, +} + +impl FabroRunCreateParams { + pub fn validate(&self, options: CreateRunOptions) -> ToolResult<()> { + common::validate_len("runs", self.runs.len(), 1, 50)?; + for spec in &self.runs { + if let Some(parent) = options.forced_parent_id { + if spec.parent_id.is_some_and(|id| id != parent) { + return Err(ToolError::message(format!( + "parent_id must be omitted or match the current run {parent}" + ))); + } + } else if spec.target.is_none() { + return Err(ToolError::message( + "standalone fabro_run_create requires an explicit target; use kind: none for an empty workspace", + )); + } + if let Some(target) = &spec.target { + target + .clone() + .validate() + .map_err(|error| ToolError::from_anyhow(&error.into()))?; + } + for (key, value) in &spec.args.inputs { + manifest::json_to_toml_value(key, value)?; + } + } + Ok(()) + } +} + +#[derive(Debug, Serialize, JsonSchema)] +pub struct CreateRunsResult { + pub runs: Vec, +} + +#[derive(Debug, Serialize, JsonSchema)] +pub struct CreatedRunResult { + pub run_id: String, + pub parent_id: Option, + pub children_count: u64, + #[schemars(with = "String")] + pub workflow_version_id: WorkflowVersionId, + pub start_requested: bool, + pub status: String, +} + +pub async fn create_runs( + backend: Arc, + params: FabroRunCreateParams, +) -> ToolResult { + create_runs_with_options(backend, params, CreateRunOptions::default()).await +} + +pub async fn create_runs_with_options( + backend: Arc, + params: FabroRunCreateParams, + options: CreateRunOptions, +) -> ToolResult { + params.validate(options)?; + let mut runs = Vec::with_capacity(params.runs.len()); + let mut created_ids = Vec::new(); + for spec in params.runs { + let result: anyhow::Result = async { + let parent_id = options.forced_parent_id.or(spec.parent_id); + let target = if let Some(target) = spec.target { + target + } else { + let parent = options + .forced_parent_id + .context("an explicit target is required")?; + inherit_parent_target(&backend.get_run_state(&parent).await?)? + }; + let intent = RunIntent { + workflow_version_id: spec.workflow_version_id, + target, + args: spec.args, + environment_id: spec.environment_id, + parent_id, + title: spec.title, + goal: spec.goal, + }; + let run_id = backend.create_run_from_intent(intent).await?; + created_ids.push(run_id); + let start_requested = spec.start.unwrap_or(true); + let summary = if start_requested { + backend.start_run(&run_id, false).await.with_context(|| { + format!("run {run_id} was created but its start request failed") + })? + } else { + backend.retrieve_run(&run_id).await.with_context(|| { + format!("run {run_id} was created but retrieving its summary failed") + })? + }; + Ok(CreatedRunResult { + run_id: run_id.to_string(), + parent_id: summary.parent_id.map(|id| id.to_string()), + children_count: summary.children_count, + workflow_version_id: spec.workflow_version_id, + start_requested, + status: summary.lifecycle.status.kind().to_string(), + }) + } + .await; + match result { + Ok(run) => runs.push(run), + Err(error) => { + let error = if created_ids.is_empty() { + error + } else { + error.context(format!( + "already created run IDs: {}; inspect these runs before retrying", + created_ids + .iter() + .map(ToString::to_string) + .collect::>() + .join(", ") + )) + }; + return Err(ToolError::from_anyhow(&error)); + } } } + Ok(CreateRunsResult { runs }) } -fn json_value_kind(value: &Value) -> &'static str { - match value { - Value::Null => "null", - Value::Bool(_) => "boolean", - Value::Number(_) => "number", - Value::String(_) => "string", - Value::Array(_) => "array", - Value::Object(_) => "object", - } +fn inherit_parent_target(parent: &RunProjection) -> anyhow::Result { + let target = parent.spec.target.as_ref().context( + "the parent run has no canonical target; send an explicit target for this child run", + )?; + Ok(match target { + RunTarget::Git(git) => { + let branch = if parent.spec.settings.run.run_branch.enabled { + parent.start.as_ref().and_then(|start| start.run_branch.as_ref()) + .filter(|branch| !branch.trim().is_empty()).context( + "the parent run has no execution branch yet; send an explicit target for this child run" + )? + } else { + &git.branch + }; + RunTarget::Git(fabro_types::GitRunTarget { + repo: git.repo.clone(), + branch: branch.clone(), + tag: None, + sha: None, + }) + } + RunTarget::None {} | RunTarget::Folder { .. } => target.clone(), + }) } -impl From for CreateRunSpecInput { - fn from(spec: CreateRunSpec) -> Self { - Self::Spec(Box::new(spec)) - } +pub fn create_runs_text(result: &CreateRunsResult) -> String { + let start_requested = result.runs.iter().filter(|run| run.start_requested).count(); + format!( + "created {} Fabro run(s), start requested for {start_requested}", + result.runs.len() + ) } -impl JsonSchema for CreateRunSpecInput { - fn inline_schema() -> bool { - true - } - - fn schema_name() -> Cow<'static, str> { - "CreateRunSpecInput".into() - } - - fn json_schema(generator: &mut SchemaGenerator) -> Schema { - json_schema!({ - "description": "Fabro run create specification. Use a workflow string shorthand, or an object when setting create options.", - "anyOf": [ - { - "type": "string", - "description": "Workflow selector shorthand. Equivalent to an object with only the workflow field set." - }, - generator.subschema_for::() - ] - }) - } +// Schema metadata for canonical types owned by fabro-types. Runtime values use +// those types directly; parity tests below prevent tool-schema drift. +fn run_args_schema(_: &mut SchemaGenerator) -> Schema { + json_schema!({ + "type": "object", "additionalProperties": false, + "properties": { + "model": {"type": ["string", "null"]}, + "provider": {"type": ["string", "null"]}, + "inputs": {"type": "object", "additionalProperties": {"type": ["string", "boolean", "number"]}}, + "labels": {"type": "object", "additionalProperties": {"type": "string"}}, + "dry_run": {"type": ["boolean", "null"]}, + "auto_approve": {"type": ["boolean", "null"]}, + "preserve_sandbox": {"type": ["boolean", "null"]} + } + }) } - -impl JsonSchema for CreateRunWorkflowSource { - fn inline_schema() -> bool { - true - } - - fn schema_name() -> Cow<'static, str> { - "CreateRunWorkflowSource".into() - } - - fn json_schema(_: &mut SchemaGenerator) -> Schema { - json_schema!({ - "description": "Workflow content source. Selector strings require a proven shared filesystem; inline files and exact stored IDs are portable.", - "anyOf": [ - { - "type": "string", - "description": "Workflow selector, such as a workflow name or workflow file path." - }, - { - "type": "object", - "required": ["kind", "entrypoint", "files"], - "additionalProperties": false, - "properties": { - "kind": { "const": "inline" }, - "entrypoint": { "type": "string" }, - "files": { - "type": "object", - "additionalProperties": { "type": "string" } - } - } - }, - { - "type": "object", - "required": ["kind", "workflow_version_id"], - "additionalProperties": false, - "properties": { - "kind": { "const": "stored" }, - "workflow_version_id": { "type": "string" } - } - } - ] - }) - } -} - -/// Schema for the optional canonical run target. `RunTarget` is an internally -/// tagged serde enum whose variants deny unknown fields, so the union is -/// spelled out here and pinned by the serde parity test below. fn run_target_schema(_: &mut SchemaGenerator) -> Schema { json_schema!({ - "description": "Canonical run workspace target. Worker calls inherit the parent target when omitted (a Git parent contributes its repository and current execution branch from run state, not its original input branch or pinned commit/tag; changes must be pushed before creating the child); standalone stored-ID calls require an explicit target; standalone selector and inline calls derive it from the selected environment: Local environments target the working directory folder, and clone-based environments require an attached GitHub checkout whose exact local HEAD is available from the canonical origin. Folder targets require a standalone or Local-worker filesystem context.", + "description": "Canonical workspace target. Required for standalone calls; native workers inherit the parent execution target when omitted.", "anyOf": [ { "type": "null" }, { @@ -177,1198 +274,324 @@ fn run_target_schema(_: &mut SchemaGenerator) -> Schema { }) } -#[derive(Debug, Clone)] -pub enum CreateRunWorkflowSource { - Selector(String), - Inline(InlineWorkflowSource), - Stored { - workflow_version_id: WorkflowVersionId, - }, -} - -impl CreateRunWorkflowSource { - #[must_use] - pub fn display(&self) -> String { - match self { - Self::Selector(selector) => selector.clone(), - Self::Inline(source) => source.entrypoint.to_string(), - Self::Stored { - workflow_version_id, - } => workflow_version_id.to_string(), - } - } -} - -impl<'de> Deserialize<'de> for CreateRunWorkflowSource { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - #[derive(Deserialize)] - #[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] - enum TaggedSource { - Inline { - entrypoint: WorkflowPath, - files: BTreeMap, - }, - Stored { - workflow_version_id: WorkflowVersionId, - }, - } - - match Value::deserialize(deserializer)? { - Value::String(selector) => Ok(Self::Selector(selector)), - value @ Value::Object(_) => { - match serde_json::from_value::(value).map_err(de::Error::custom)? { - TaggedSource::Inline { entrypoint, files } => { - Ok(Self::Inline(InlineWorkflowSource { entrypoint, files })) - } - TaggedSource::Stored { - workflow_version_id, - } => Ok(Self::Stored { - workflow_version_id, - }), - } - } - other => Err(de::Error::custom(format!( - "expected workflow selector string or tagged source object, got {}", - json_value_kind(&other) - ))), - } - } -} - -#[derive(Debug, Clone)] -pub struct InlineWorkflowSource { - pub entrypoint: WorkflowPath, - pub files: BTreeMap, -} - -/// Full create-run specification. -/// -/// The advertised MCP schema is derived from this struct, so adding a field -/// here publishes it to clients automatically; `deny_unknown_fields` and the -/// derived `additionalProperties: false` stay in lockstep. -#[derive(Debug, Deserialize, JsonSchema)] -#[serde(deny_unknown_fields)] -#[schemars(inline)] -pub struct CreateRunSpec { - pub workflow: CreateRunWorkflowSource, - /// Working directory used to resolve relative workflow paths. - pub cwd: Option, - /// Optional parent run id or selector. - pub parent_id: Option, - #[schemars(schema_with = "run_target_schema")] - pub target: Option, - /// Optional goal override for the run. - pub goal: Option, - /// Read the run goal from a file. Mutually exclusive with goal. Relative - /// paths are resolved from the run cwd. - pub goal_file: Option, - /// Workflow input overrides keyed by input name. - #[serde(default)] - pub inputs: HashMap, - /// Labels to attach to the created run. - #[serde(default)] - pub labels: HashMap, - /// Whether the run should use dry-run mode. - pub dry_run: Option, - /// Whether agent approval prompts should be auto-approved. - pub auto_approve: Option, - /// Model override for the run. - pub model: Option, - /// Provider override for the run. - pub provider: Option, - /// Named environment slug override for the run. - pub environment: Option, - /// Whether to preserve the sandbox after the run. - pub preserve_sandbox: Option, - /// Whether to start the run immediately after creation. Defaults to true. - pub start: Option, -} - -#[derive(Debug, Deserialize)] -#[serde(transparent)] -pub struct RunInputValue(Value); - -impl From for RunInputValue { - fn from(value: Value) -> Self { - Self(value) - } -} - -impl RunInputValue { - pub(crate) fn into_inner(self) -> Value { - self.0 - } -} - -impl JsonSchema for RunInputValue { - fn inline_schema() -> bool { - true - } - - fn schema_name() -> Cow<'static, str> { - "RunInputValue".into() - } - - fn json_schema(_: &mut SchemaGenerator) -> Schema { - json_schema!({ - "description": "Run input override value. Inputs are TOML-compatible scalar values: string, boolean, integer, or float.", - "anyOf": [ - { "type": "string" }, - { "type": "boolean" }, - { "type": "integer" }, - { "type": "number" } - ] - }) - } -} - -#[derive(Debug)] -pub struct ValidatedCreateRuns { - pub runs: Vec, -} - -#[derive(Debug)] -pub struct ValidatedCreateRunSpec { - pub workflow: CreateRunWorkflowSource, - pub cwd: Option, - pub parent_id: Option, - pub target: Option, - pub goal: Option, - pub goal_file: Option, - pub inputs: HashMap, - pub labels: HashMap, - pub dry_run: Option, - pub auto_approve: Option, - pub model: Option, - pub provider: Option, - pub environment: Option, - pub preserve_sandbox: Option, - pub start: Option, -} - -#[derive(Debug)] -pub struct ValidatedRunInputValue { - json: Value, -} - -impl ValidatedRunInputValue { - #[must_use] - pub fn json(&self) -> &Value { - &self.json - } -} - -impl TryFrom for ValidatedCreateRuns { - type Error = ToolError; - - fn try_from(params: FabroRunCreateParams) -> Result { - common::validate_len("runs", params.runs.len(), 1, 50)?; - let runs = params - .runs - .into_iter() - .map(ValidatedCreateRunSpec::try_from) - .collect::, _>>()?; - Ok(Self { runs }) - } -} - -impl TryFrom for ValidatedCreateRunSpec { - type Error = ToolError; - - fn try_from(spec: CreateRunSpecInput) -> Result { - match spec { - CreateRunSpecInput::Workflow(workflow) => Self::try_from(CreateRunSpec { - workflow: CreateRunWorkflowSource::Selector(workflow), - cwd: None, - parent_id: None, - target: None, - goal: None, - goal_file: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: None, - auto_approve: None, - model: None, - provider: None, - environment: None, - preserve_sandbox: None, - start: None, - }), - CreateRunSpecInput::Spec(spec) => Self::try_from(*spec), - } - } -} - -impl TryFrom for ValidatedCreateRunSpec { - type Error = ToolError; - - fn try_from(spec: CreateRunSpec) -> Result { - let workflow = validate_workflow_source(spec.workflow)?; - let parent_id = spec - .parent_id - .as_deref() - .map(str::trim) - .filter(|parent_id| !parent_id.is_empty()) - .map(ToOwned::to_owned); - if spec.parent_id.is_some() && parent_id.is_none() { - return Err(ToolError::message("parent_id must not be blank")); - } - if spec.goal.is_some() && spec.goal_file.is_some() { - return Err(ToolError::message( - "goal and goal_file are mutually exclusive; use exactly one", - )); - } - if spec - .goal_file - .as_ref() - .is_some_and(|path| path.as_os_str().is_empty()) - { - return Err(ToolError::message("goal_file must not be blank")); - } - let inputs = spec - .inputs - .into_iter() - .map(|(key, value)| { - let json = value.into_inner(); - manifest::json_to_toml_value(&key, &json)?; - Ok((key, ValidatedRunInputValue { json })) - }) - .collect::>>()?; - let target = spec - .target - .map(|target| { - target - .validate() - .map(|validated| validated.target) - .map_err(|err| ToolError::message(format!("invalid run target: {err}"))) - }) - .transpose()?; - Ok(Self { - workflow, - cwd: spec.cwd, - parent_id, - target, - goal: spec.goal, - goal_file: spec.goal_file, - inputs, - labels: spec.labels, - dry_run: spec.dry_run, - auto_approve: spec.auto_approve, - model: spec.model, - provider: spec.provider, - environment: spec.environment, - preserve_sandbox: spec.preserve_sandbox, - start: spec.start, - }) - } -} - -fn validate_workflow_source( - source: CreateRunWorkflowSource, -) -> ToolResult { - match source { - CreateRunWorkflowSource::Selector(selector) => { - let selector = selector.trim(); - if selector.is_empty() { - return Err(ToolError::message("workflow selector must not be blank")); - } - Ok(CreateRunWorkflowSource::Selector(selector.to_string())) - } - CreateRunWorkflowSource::Inline(source) => { - if source.files.len() > fabro_types::MAX_WORKFLOW_VERSION_FILES { - return Err(ToolError::message(format!( - "inline workflow contains more than {} files", - fabro_types::MAX_WORKFLOW_VERSION_FILES - ))); - } - if !source.files.contains_key(&source.entrypoint) { - return Err(ToolError::message(format!( - "inline workflow entrypoint `{}` is missing from files", - source.entrypoint - ))); - } - fabro_types::validate_workflow_source_paths(source.files.keys()) - .map_err(|err| ToolError::message(format!("inline workflow {err}")))?; - let mut total_bytes = 0usize; - for (path, content) in &source.files { - let bytes = content.len(); - if bytes > fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES { - return Err(ToolError::message(format!( - "inline workflow file `{path}` exceeds {} KiB", - fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES / 1024 - ))); - } - total_bytes = total_bytes - .checked_add(bytes) - .ok_or_else(|| ToolError::message("inline workflow content size overflowed"))?; - if total_bytes > fabro_types::MAX_WORKFLOW_VERSION_BYTES { - return Err(ToolError::message(format!( - "inline workflow content exceeds {} MiB in aggregate", - fabro_types::MAX_WORKFLOW_VERSION_BYTES / (1024 * 1024) - ))); - } - } - Ok(CreateRunWorkflowSource::Inline(source)) - } - stored @ CreateRunWorkflowSource::Stored { .. } => Ok(stored), - } -} - -#[derive(Debug, Serialize, JsonSchema)] -pub struct CreateRunsResult { - pub runs: Vec, -} - -#[derive(Debug, Serialize, JsonSchema)] -pub struct CreatedRunResult { - pub run_id: String, - pub parent_id: Option, - pub children_count: u64, - pub workflow: String, - pub start_requested: bool, - pub status: String, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - #[schemars(default, skip_serializing_if = "Vec::is_empty")] - pub warnings: Vec, -} - -#[derive(Debug, Clone, Copy, Default)] -pub struct CreateRunOptions { - pub forced_parent_id: Option, -} - -pub async fn create_runs( - backend: Arc, - base_cwd: &Path, - params: ValidatedCreateRuns, -) -> ToolResult { - create_runs_with_options(backend, base_cwd, params, CreateRunOptions::default()).await -} - -pub async fn create_runs_with_options( - backend: Arc, - base_cwd: &Path, - params: ValidatedCreateRuns, - options: CreateRunOptions, -) -> ToolResult { - let mut created = Vec::with_capacity(params.runs.len()); - let mut parent_id_cache = HashMap::::new(); - for spec in params.runs { - let workflow = spec.workflow.display(); - let cwd = spec.cwd.clone().unwrap_or_else(|| base_cwd.to_path_buf()); - let parent_id = if let Some(forced_parent_id) = options.forced_parent_id { - Some(forced_parent_id) - } else if let Some(parent_selector) = spec.parent_id.as_deref() { - Some( - resolve_parent_run_id(backend.as_ref(), &mut parent_id_cache, parent_selector) - .await?, - ) - } else { - None - }; - let submission = backend - .create_run_from_spec(&spec, &cwd, parent_id) - .await - .map_err(|err| ToolError::from_anyhow(&err))?; - let run_id = submission.run_id; - let start_requested = spec.start.unwrap_or(true); - let summary = if start_requested { - backend - .start_run(&run_id, false) - .await - .map_err(|err| ToolError::from_anyhow(&err))? - } else { - backend - .retrieve_run(&run_id) - .await - .map_err(|err| ToolError::from_anyhow(&err))? - }; - created.push(CreatedRunResult { - run_id: summary.id.to_string(), - parent_id: summary.parent_id.map(|parent_id| parent_id.to_string()), - children_count: summary.children_count, - workflow, - start_requested, - status: summary.lifecycle.status.kind().to_string(), - warnings: submission.warnings, - }); - } - Ok(CreateRunsResult { runs: created }) -} - -async fn resolve_parent_run_id( - backend: &dyn FabroToolBackend, - parent_id_cache: &mut HashMap, - parent_selector: &str, -) -> ToolResult { - if let Ok(parent_id) = parent_selector.parse::() { - return Ok(parent_id); - } - if let Some(parent_id) = parent_id_cache.get(parent_selector) { - return Ok(*parent_id); - } - - let parent_id = backend - .resolve_run(parent_selector) - .await - .map_err(|err| ToolError::from_anyhow(&err))? - .id; - parent_id_cache.insert(parent_selector.to_string(), parent_id); - Ok(parent_id) -} - -pub fn create_runs_text(result: &CreateRunsResult) -> String { - let start_requested = result.runs.iter().filter(|run| run.start_requested).count(); - let mut text = format!( - "created {} Fabro run(s), start requested for {start_requested}", - result.runs.len() - ); - for warning in result.runs.iter().flat_map(|run| &run.warnings) { - text.push_str("\nwarning: "); - text.push_str(warning); - } - text -} - #[cfg(test)] mod tests { - use std::sync::Mutex; + use std::collections::HashMap; - use async_trait::async_trait; use chrono::{TimeZone, Utc}; - use fabro_api::types; use fabro_types::{ - EventEnvelope, Run, RunLifecycle, RunLinks, RunOrigin, RunProjection, RunStatus, - RunTimestamps, WorkflowRef, test_support, + GitRunTarget, Run, RunLifecycle, RunLinks, RunOrigin, RunStatus, RunTimestamps, + WorkflowRef, test_support, }; - use schemars::SchemaGenerator; - use serde_json::json; + use httpmock::Method::{GET, POST}; + use httpmock::MockServer; + use serde_json::{Value, json}; use super::*; + use crate::fabro_client::ClientBackend; + + fn version_id() -> WorkflowVersionId { + fabro_types::BlobHash::new(b"workflow").into() + } + fn params(value: Value) -> FabroRunCreateParams { + FabroRunCreateParams { + runs: vec![serde_json::from_value(value).unwrap()], + } + } + fn spec() -> Value { + json!({"workflow_version_id": version_id(), "target": {"kind": "none"}, "start": false}) + } + fn backend(server: &MockServer) -> Arc { + Arc::new(ClientBackend::new(Arc::new( + fabro_client::Client::new_no_proxy(&server.url("")).unwrap(), + ))) + } + fn parent(target: RunTarget) -> RunProjection { + let mut spec = test_support::test_run_spec(); + spec.target = Some(target); + RunProjection::new(String::new(), spec, Utc::now()) + } #[test] - fn run_input_value_schema_allows_only_json_scalars() { - let mut generator = SchemaGenerator::default(); - let schema = RunInputValue::json_schema(&mut generator); - let schema = serde_json::to_value(schema).expect("schema should serialize"); - + fn run_create_accepts_canonical_id_target_and_args() { + let mut value = spec(); + value["args"] = json!({"model":"model", "provider":"provider", "inputs":{"s":"x", "b":true,"i":4,"f":1.25}, "labels":{"x":"y"}, "dry_run":false, "auto_approve":true, "preserve_sandbox":false}); + let p = params(value.clone()); + p.validate(CreateRunOptions::default()).unwrap(); assert_eq!( - schema["anyOf"], - json!([ - { "type": "string" }, - { "type": "boolean" }, - { "type": "integer" }, - { "type": "number" }, - ]) + serde_json::to_value(&p.runs[0].args).unwrap(), + value["args"] ); + let p = params(spec()); + assert_eq!(serde_json::to_value(&p.runs[0].args).unwrap(), json!({})); + assert_eq!(p.runs[0].environment_id, None); } #[test] - fn create_spec_schema_advertises_the_accepted_workflow_and_target_grammar() { - let mut generator = SchemaGenerator::default(); - let schema = CreateRunSpecInput::json_schema(&mut generator); - let schema = serde_json::to_value(schema).expect("schema should serialize"); - let properties = schema["anyOf"][1]["properties"] - .as_object() - .expect("object form should have properties"); - - assert!(!properties.contains_key("run_id")); - assert_eq!(schema["anyOf"][1]["additionalProperties"], false); - let workflow_variants = properties["workflow"]["anyOf"] - .as_array() - .expect("workflow should advertise all source variants"); - assert!( - workflow_variants - .iter() - .any(|variant| variant["type"] == "string") - ); - for kind in ["inline", "stored"] { - assert!(workflow_variants.iter().any(|variant| { - variant.pointer("/properties/kind/const") == Some(&json!(kind)) - })); - } - let target_variants = properties["target"]["anyOf"] - .as_array() - .expect("target should advertise the canonical target variants"); - for kind in ["git", "none", "folder"] { - assert!(target_variants.iter().any(|variant| { - variant.pointer("/properties/kind/const") == Some(&json!(kind)) - })); - } - } - - #[test] - fn create_spec_schema_stays_in_parity_with_run_target_serde() { - let mut generator = SchemaGenerator::default(); - let schema = CreateRunSpecInput::json_schema(&mut generator); - let schema = serde_json::to_value(schema).expect("schema should serialize"); - let validator = jsonschema::validator_for(&schema).expect("advertised schema must compile"); - - // Every serde-produced target shape must satisfy the hand-written - // schema literal; a field added to a target variant without updating - // the literal fails here because the schema denies unknown fields. - let targets = [ - RunTarget::Git(fabro_types::GitRunTarget { - repo: "fabro-sh/fabro".to_string(), - branch: "main".to_string(), - tag: Some("v1.0.0".to_string()), - sha: Some("0123456789abcdef0123456789abcdef01234567".to_string()), - }), - RunTarget::None {}, - RunTarget::Folder { - path: "/srv/workspace".to_string(), - }, - ]; - for target in targets { - let target = serde_json::to_value(&target).expect("target should serialize"); - let spec = json!({ "workflow": "demo", "target": target }); - assert!( - validator.is_valid(&spec), - "advertised schema rejects serde-produced target {target}" - ); - } - - // A spec exercising every field must satisfy the derived schema, so a - // field that deserializes but is missing from the advertised schema - // (or advertised with the wrong shape) fails here. - let full_spec = json!({ - "workflow": { - "kind": "stored", - "workflow_version_id": fabro_types::BlobHash::new(b"stored").to_string() - }, - "cwd": "/srv/project", - "parent_id": "parent", - "target": { "kind": "none" }, - "goal": "goal", - "goal_file": null, - "inputs": { "count": 1, "name": "x", "flag": true, "ratio": 0.5 }, - "labels": { "team": "core" }, - "dry_run": true, - "auto_approve": false, - "model": "model", - "provider": "provider", - "environment": "default", - "preserve_sandbox": true, - "start": false - }); - serde_json::from_value::(full_spec.clone()) - .expect("full spec should deserialize"); - assert!( - validator.is_valid(&full_spec), - "advertised schema rejects a fully populated spec: {:?}", - validator.iter_errors(&full_spec).collect::>() - ); - - // The schema must actually enforce the field lists, so the parity - // assertions above have teeth. - let unknown_field = json!({ - "workflow": "demo", - "target": { - "kind": "git", - "repo": "fabro-sh/fabro", - "branch": "main", - "unknown_field": true - } - }); - assert!(!validator.is_valid(&unknown_field)); - } - - #[test] - fn create_spec_accepts_parent_selector() { - let spec = ValidatedCreateRunSpec::try_from(CreateRunSpec { - workflow: selector("simple.fabro"), - cwd: None, - parent_id: Some(" nightly-parent ".to_string()), - target: None, - goal: None, - goal_file: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: None, - auto_approve: None, - model: None, - provider: None, - environment: None, - preserve_sandbox: None, - start: None, - }) - .expect("parent selectors should validate without requiring exact run ids"); - - assert_eq!(spec.parent_id.as_deref(), Some("nightly-parent")); - } - - #[test] - fn create_params_accept_string_shorthand() { - let params: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": ["simple.fabro"] - })) - .expect("string shorthand should deserialize"); - - let params = ValidatedCreateRuns::try_from(params) - .expect("string shorthand should validate as workflow selector"); - let spec = ¶ms.runs[0]; - assert_eq!(spec.workflow.display(), "simple.fabro"); - assert_eq!(spec.cwd, None); - assert_eq!(spec.parent_id, None); - assert!(spec.inputs.is_empty()); - assert!(spec.labels.is_empty()); - assert_eq!(spec.start, None); - } - - #[test] - fn create_params_accept_inline_and_stored_workflow_sources() { - let stored_id = fabro_types::BlobHash::new(b"stored workflow").to_string(); - let inline: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": { - "kind": "inline", - "entrypoint": "flows/main.fabro", - "files": { - "flows/main.fabro": "digraph Main {}", - "prompts/goal.md": "Ship it" - } - }, - "target": { "kind": "none" }, - "start": false - }] - })) - .expect("inline workflow source should deserialize"); - let inline = - ValidatedCreateRuns::try_from(inline).expect("inline workflow source should validate"); - assert_eq!(inline.runs[0].workflow.display(), "flows/main.fabro"); - - let stored: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": { - "kind": "stored", - "workflow_version_id": stored_id - }, - "target": { "kind": "git", "repo": "fabro-sh/fabro", "branch": "main" } - }] - })) - .expect("stored workflow source should deserialize"); - let stored = - ValidatedCreateRuns::try_from(stored).expect("stored workflow source should validate"); - assert_eq!(stored.runs[0].workflow.display(), stored_id); - } - - #[test] - fn create_params_reject_invalid_workflow_sources_and_inputs_before_backend_work() { - for workflow in [ - json!({ - "kind": "inline", - "entrypoint": "../escape.fabro", - "files": { "../escape.fabro": "digraph W {}" } - }), - json!({ - "kind": "stored", - "workflow_version_id": "not-an-id" - }), - json!({ - "kind": "stored", - "workflow_version_id": fabro_types::BlobHash::new(b"stored").to_string(), - "extra": true - }), + fn run_create_schema_and_serde_reject_old_sources_and_accept_canonical_variants() { + let schema = serde_json::to_value(schemars::schema_for!(FabroRunCreateParams)).unwrap(); + let validator = jsonschema::validator_for(&schema).unwrap(); + for target in [ + json!({"kind":"none"}), + json!({"kind":"folder","path":"/workspace"}), + json!({"kind":"git","repo":"acme/repo","branch":"main","sha":"a".repeat(40),"tag":"v1"}), ] { - serde_json::from_value::(json!({ - "runs": [{ "workflow": workflow }] - })) - .expect_err("invalid workflow source should fail deserialization"); + let value = json!({"runs":[{"workflow_version_id": version_id(), "target":target, "args":{"dry_run":false}}]}); + assert!(validator.is_valid(&value)); + serde_json::from_value::(value) + .unwrap() + .validate(CreateRunOptions::default()) + .unwrap(); } + for old in [ + json!("workflow"), + json!({"workflow":"workflow"}), + json!({"workflow":{"kind":"inline","entrypoint":"x","files":{"x":"content"}}}), + ] { + let value = json!({"runs":[old]}); + assert!(!validator.is_valid(&value)); + let error = serde_json::from_value::(value) + .unwrap_err() + .to_string(); + assert!(error.contains("fabro_workflow_version_create")); + } + for (key, value) in [ + ("cwd", json!("/tmp")), + ("goal_file", json!("goal.md")), + ("inputs", json!({})), + ("environment", json!("local")), + ] { + let mut item = spec(); + item[key] = value; + let value = json!({"runs":[item]}); + assert!(!validator.is_valid(&value)); + assert!(serde_json::from_value::(value).is_err()); + } + for value in [json!(null), json!([]), json!({})] { + let mut item = spec(); + item["args"] = json!({"inputs":{"bad":value}}); + assert!(params(item).validate(CreateRunOptions::default()).is_err()); + } + } - let missing_entrypoint: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": { - "kind": "inline", - "entrypoint": "main.fabro", - "files": { "other.fabro": "digraph Other {}" } - } - }] - })) - .unwrap(); + #[test] + fn run_create_context_validation_requires_standalone_target_and_current_worker_parent() { + let mut item = spec(); + item.as_object_mut().unwrap().remove("target"); + item["parent_id"] = json!(RunId::new()); assert!( - ValidatedCreateRuns::try_from(missing_entrypoint) + params(item.clone()) + .validate(CreateRunOptions::default()) + .unwrap_err() + .as_str() + .contains("explicit target") + ); + let options = CreateRunOptions { + forced_parent_id: Some(RunId::new()), + }; + assert!( + params(item.clone()) + .validate(options) + .unwrap_err() + .as_str() + .contains("current run") + ); + item["parent_id"] = json!(options.forced_parent_id); + params(item).validate(options).unwrap(); + assert!( + FabroRunCreateParams { runs: vec![] } + .validate(options) + .is_err() + ); + } + + #[test] + fn inherited_target_uses_execution_branch_and_requires_execution_state() { + let mut p = parent(RunTarget::Git(GitRunTarget { + repo: "acme/repo".into(), + branch: "main".into(), + sha: Some("a".repeat(40)), + tag: Some("v1".into()), + })); + assert!( + inherit_parent_target(&p) .unwrap_err() .to_string() - .contains("entrypoint") + .contains("no execution branch") ); - - for (files, expected) in [ - (json!({ "a": "x", "a/b.md": "y" }), "paths collide"), - (json!({ "A": "x", "a/b.md": "y" }), "paths collide"), - ( - json!({ "dir/File": "x", "DIR/file/child.md": "y" }), - "paths collide", - ), - ( - json!({ "main.fabro": "digraph W {}", "Prompt.md": "x", "prompt.md": "y" }), - "paths collide", - ), - ] { - let entrypoint = files - .as_object() - .and_then(|files| files.keys().next().cloned()) - .unwrap(); - let colliding: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": { - "kind": "inline", - "entrypoint": entrypoint, - "files": files - } - }] - })) - .unwrap(); - let error = ValidatedCreateRuns::try_from(colliding) - .expect_err("colliding inline paths must fail validation before any staging") - .to_string(); - assert!(error.contains(expected), "unexpected error: {error}"); + p.start = Some(fabro_types::StartRecord { + start_time: Utc::now(), + run_branch: Some("fabro/run/parent".into()), + base_sha: None, + }); + let RunTarget::Git(target) = inherit_parent_target(&p).unwrap() else { + panic!("expected git") + }; + assert_eq!(target.branch, "fabro/run/parent"); + assert_eq!(target.sha, None); + assert_eq!(target.tag, None); + p.spec.settings.run.run_branch.enabled = false; + let RunTarget::Git(target) = inherit_parent_target(&p).unwrap() else { + panic!("expected git") + }; + assert_eq!(target.branch, "main"); + assert_eq!(target.sha, None); + assert_eq!(target.tag, None); + for target in [RunTarget::None {}, RunTarget::Folder { + path: "/workspace".into(), + }] { + p.spec.target = Some(target.clone()); + assert_eq!(inherit_parent_target(&p).unwrap(), target); } - - let too_many = (0..=fabro_types::MAX_WORKFLOW_VERSION_FILES) - .map(|index| (format!("files/{index}.md"), json!("x"))) - .collect::>(); - let too_many: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": { - "kind": "inline", - "entrypoint": "files/0.md", - "files": too_many - } - }] - })) - .unwrap(); - assert!(ValidatedCreateRuns::try_from(too_many).is_err()); - - let oversized: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": { - "kind": "inline", - "entrypoint": "main.fabro", - "files": { "main.fabro": "x".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES + 1) } - } - }] - })) - .unwrap(); - assert!(ValidatedCreateRuns::try_from(oversized).is_err()); - - let aggregate: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": { - "kind": "inline", - "entrypoint": "0.fabro", - "files": { - "0.fabro": "x".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES), - "1.md": "x".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES), - "2.md": "x".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES), - "3.md": "x".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES), - "4.md": "x" - } - } - }] - })) - .unwrap(); - assert!(ValidatedCreateRuns::try_from(aggregate).is_err()); - - let invalid_target: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": "main.fabro", - "target": { "kind": "git", "repo": "not-a-slug", "branch": "HEAD" } - }] - })) - .unwrap(); - assert!(ValidatedCreateRuns::try_from(invalid_target).is_err()); - - let nonscalar: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": "main.fabro", - "inputs": { "nested": { "no": "objects" } } - }] - })) - .unwrap(); - assert!(ValidatedCreateRuns::try_from(nonscalar).is_err()); - } - - #[test] - fn create_params_preserve_object_form_options() { - let params: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": "simple.fabro", - "dry_run": true, - "auto_approve": true, - "labels": { "source": "mcp-test" }, - "start": false - }] - })) - .expect("object form should deserialize"); - - let params = - ValidatedCreateRuns::try_from(params).expect("object form should still validate"); - let spec = ¶ms.runs[0]; - assert_eq!(spec.workflow.display(), "simple.fabro"); - assert_eq!(spec.dry_run, Some(true)); - assert_eq!(spec.auto_approve, Some(true)); - assert_eq!( - spec.labels.get("source").map(String::as_str), - Some("mcp-test") - ); - assert_eq!(spec.start, Some(false)); - } - - #[test] - fn create_params_reject_unknown_object_fields() { - let err = serde_json::from_value::(json!({ - "runs": [{ - "workflow": "simple.fabro", - "run_id": "not-a-valid-run-id", - "start": false - }] - })) - .expect_err("unknown create fields should be rejected"); - - assert!(err.to_string().contains("unknown field `run_id`"), "{err}"); - } - - #[test] - fn create_params_preserve_goal_file_option() { - let params: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": "implement-plan", - "goal_file": "plans/ship-it.md", - "start": false - }] - })) - .expect("object form with goal_file should deserialize"); - - let params = ValidatedCreateRuns::try_from(params).expect("goal_file should validate"); - let spec = ¶ms.runs[0]; - assert_eq!(spec.goal, None); - assert_eq!( - spec.goal_file.as_deref(), - Some(Path::new("plans/ship-it.md")) - ); - } - - #[test] - fn create_params_reject_goal_and_goal_file_together() { - let params: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [{ - "workflow": "implement-plan", - "goal": "inline goal", - "goal_file": "plans/ship-it.md" - }] - })) - .expect("object form with both goal forms should deserialize before validation"); - - let err = ValidatedCreateRuns::try_from(params) - .expect_err("goal and goal_file should be mutually exclusive"); - assert!( - err.to_string() - .contains("goal and goal_file are mutually exclusive"), - "{err}" - ); - } - - #[test] - fn create_params_reject_blank_string_shorthand_workflow() { - let params: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": [" "] - })) - .expect("blank shorthand should deserialize before validation"); - - let err = ValidatedCreateRuns::try_from(params).expect_err("blank workflow should fail"); - assert!(err.to_string().contains("workflow"), "{err}"); - } - - #[test] - fn create_params_missing_object_workflow_keeps_field_error() { - let err = serde_json::from_value::(json!({ - "runs": [{ "dry_run": true }] - })) - .expect_err("object form without workflow should fail deserialization"); - - assert!( - err.to_string().contains("missing field `workflow`"), - "{err}" - ); + p.spec.target = None; + assert!(inherit_parent_target(&p).is_err()); } #[tokio::test] - async fn create_runs_resolves_parent_selector_and_sends_parent_id_to_backend() { - let temp = tempfile::tempdir().expect("tempdir should be created"); - let child_id = run_id("01KRBZW5C00000000000000001"); - let parent_id = run_id("01KRBZW4DW0000000000000002"); - let backend = Arc::new(MockCreateBackend { - child_id, - parent_id, - created_parent_ids: Mutex::new(Vec::new()), - resolved_selectors: Mutex::new(Vec::new()), - started_run_ids: Mutex::new(Vec::new()), - warnings: Vec::new(), - create_error: false, - }); - let params = ValidatedCreateRuns::try_from(FabroRunCreateParams { - runs: vec![ - CreateRunSpec { - workflow: selector("simple.fabro"), - cwd: None, - parent_id: Some("nightly-parent".to_string()), - target: None, - goal: None, - goal_file: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: Some(true), - auto_approve: Some(true), - model: None, - provider: None, - environment: None, - preserve_sandbox: None, - start: Some(false), - } - .into(), - ], - }) - .expect("create params should validate"); - - let result = create_runs(backend.clone(), temp.path(), params) - .await - .expect("run should be created"); - - assert_eq!(result.runs[0].parent_id, Some(parent_id.to_string())); - assert_eq!(result.runs[0].children_count, 0); - assert_eq!(backend.created_parent_ids.lock().unwrap().as_slice(), &[ - Some(parent_id) - ]); - assert_eq!(backend.resolved_selectors.lock().unwrap().as_slice(), &[ - "nightly-parent".to_string() - ]); - } - - #[tokio::test] - async fn create_runs_reuses_parent_selector_resolution_within_batch() { - let temp = tempfile::tempdir().expect("tempdir should be created"); - let child_id = run_id("01KRBZW5C00000000000000001"); - let parent_id = run_id("01KRBZW4DW0000000000000002"); - let backend = Arc::new(MockCreateBackend { - child_id, - parent_id, - created_parent_ids: Mutex::new(Vec::new()), - resolved_selectors: Mutex::new(Vec::new()), - started_run_ids: Mutex::new(Vec::new()), - warnings: Vec::new(), - create_error: false, - }); - let runs: Vec = (0..2) - .map(|_| { - CreateRunSpecInput::from(CreateRunSpec { - workflow: selector("simple.fabro"), - cwd: None, - parent_id: Some("nightly-parent".to_string()), - target: None, - goal: None, - goal_file: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: Some(true), - auto_approve: Some(true), - model: None, - provider: None, - environment: None, - preserve_sandbox: None, - start: Some(false), - }) + async fn run_create_submits_canonical_intent_without_registration_or_parent_lookup() { + let server = MockServer::start_async().await; + let id = RunId::new(); + let parent_id = RunId::new(); + let mut item = spec(); + item["parent_id"] = json!(parent_id); + item["title"] = json!("Title"); + item["goal"] = json!("Literal goal"); + item["environment_id"] = json!("environment"); + item["args"] = json!({"dry_run":false}); + let mut intent = item.clone(); + intent.as_object_mut().unwrap().remove("start"); + let create = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/runs") + .json_body_obj(&intent); + then.status(201).json_body_obj(&run(id, None, 0)); }) - .collect(); - let params = ValidatedCreateRuns::try_from(FabroRunCreateParams { runs }) - .expect("create params should validate"); - - create_runs(backend.clone(), temp.path(), params) - .await - .expect("runs should be created"); - - assert_eq!(backend.created_parent_ids.lock().unwrap().as_slice(), &[ - Some(parent_id), - Some(parent_id), - ]); - assert_eq!(backend.resolved_selectors.lock().unwrap().as_slice(), &[ - "nightly-parent".to_string() - ]); + .await; + server + .mock_async(|when, then| { + when.method(GET).path(format!("/api/v1/runs/{id}")); + then.status(200).json_body_obj(&run(id, Some(parent_id), 0)); + }) + .await; + let registration = server + .mock_async(|when, then| { + when.path("/api/v1/workflow-versions"); + then.status(500); + }) + .await; + let state = server + .mock_async(|when, then| { + when.path(format!("/api/v1/runs/{parent_id}/state")); + then.status(500); + }) + .await; + let result = create_runs(backend(&server), params(item)).await.unwrap(); + assert_eq!(result.runs[0].run_id, id.to_string()); + assert!(!result.runs[0].start_requested); + create.assert_calls_async(1).await; + registration.assert_calls_async(0).await; + state.assert_calls_async(0).await; } #[tokio::test] - async fn create_runs_forced_parent_id_skips_selector_resolution() { - let temp = tempfile::tempdir().expect("tempdir should be created"); - let child_id = run_id("01KRBZW5C00000000000000001"); - let parent_id = run_id("01KRBZW4DW0000000000000002"); - let backend = Arc::new(MockCreateBackend { - child_id, - parent_id, - created_parent_ids: Mutex::new(Vec::new()), - resolved_selectors: Mutex::new(Vec::new()), - started_run_ids: Mutex::new(Vec::new()), - warnings: Vec::new(), - create_error: false, - }); - let params = ValidatedCreateRuns::try_from(FabroRunCreateParams { - runs: vec![ - CreateRunSpec { - workflow: selector("simple.fabro"), - cwd: None, - parent_id: Some(parent_id.to_string()), - target: None, - goal: None, - goal_file: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: Some(true), - auto_approve: Some(true), - model: None, - provider: None, - environment: None, - preserve_sandbox: None, - start: Some(false), - } - .into(), - ], - }) - .expect("create params should validate"); - - create_runs_with_options(backend.clone(), temp.path(), params, CreateRunOptions { + async fn run_create_worker_inherits_fresh_parent_state() { + let server = MockServer::start_async().await; + let id = RunId::new(); + let p = parent(RunTarget::None {}); + let parent_id = p.spec.id(); + let state = server + .mock_async(|when, then| { + when.method(GET) + .path(format!("/api/v1/runs/{parent_id}/state")); + then.status(200).json_body_obj(&p); + }) + .await; + let create = server + .mock_async(|when, then| { + when.method(POST).path("/api/v1/runs").json_body(json!({ + "workflow_version_id": version_id(), + "target": {"kind": "none"}, + "args": {}, + "parent_id": parent_id + })); + then.status(201).json_body_obj(&run(id, Some(parent_id), 0)); + }) + .await; + server + .mock_async(|when, then| { + when.method(GET).path(format!("/api/v1/runs/{id}")); + then.status(200).json_body_obj(&run(id, Some(parent_id), 0)); + }) + .await; + let mut item = spec(); + item.as_object_mut().unwrap().remove("target"); + create_runs_with_options(backend(&server), params(item), CreateRunOptions { forced_parent_id: Some(parent_id), }) .await - .expect("run should be created"); - - assert_eq!(backend.created_parent_ids.lock().unwrap().as_slice(), &[ - Some(parent_id) - ]); - assert!(backend.resolved_selectors.lock().unwrap().is_empty()); - } - - #[tokio::test] - async fn create_runs_defaults_to_start_request_and_reports_pending_child_status() { - let temp = tempfile::tempdir().expect("tempdir should be created"); - let child_id = run_id("01KRBZW5C00000000000000001"); - let parent_id = run_id("01KRBZW4DW0000000000000002"); - let backend = Arc::new(MockCreateBackend { - child_id, - parent_id, - created_parent_ids: Mutex::new(Vec::new()), - resolved_selectors: Mutex::new(Vec::new()), - started_run_ids: Mutex::new(Vec::new()), - warnings: vec!["uncommitted changes are excluded".to_string()], - create_error: false, - }); - let params = ValidatedCreateRuns::try_from(FabroRunCreateParams { - runs: vec![ - CreateRunSpec { - workflow: selector("simple.fabro"), - cwd: None, - parent_id: Some(parent_id.to_string()), - target: None, - goal: None, - goal_file: None, - inputs: HashMap::new(), - labels: HashMap::new(), - dry_run: Some(true), - auto_approve: Some(true), - model: None, - provider: None, - environment: None, - preserve_sandbox: None, - start: None, - } - .into(), - ], - }) - .expect("create params should validate"); - - let result = create_runs(backend.clone(), temp.path(), params) - .await - .expect("run should be created and start requested"); - - assert!(result.runs[0].start_requested); - assert_eq!(result.runs[0].status, "pending"); - assert_eq!(backend.started_run_ids.lock().unwrap().as_slice(), &[ - child_id - ]); - assert_eq!( - create_runs_text(&result), - "created 1 Fabro run(s), start requested for 1\nwarning: uncommitted changes are excluded" - ); - assert_eq!(result.runs[0].warnings, [ - "uncommitted changes are excluded" - ]); - let wire = serde_json::to_value(&result).unwrap(); - assert_eq!( - wire["runs"][0]["warnings"], - json!(["uncommitted changes are excluded"]) - ); - } - - #[tokio::test] - async fn create_runs_create_failure_does_not_request_start() { - let temp = tempfile::tempdir().expect("tempdir should be created"); - let child_id = run_id("01KRBZW5C00000000000000001"); - let parent_id = run_id("01KRBZW4DW0000000000000002"); - let backend = Arc::new(MockCreateBackend { - child_id, - parent_id, - created_parent_ids: Mutex::new(Vec::new()), - resolved_selectors: Mutex::new(Vec::new()), - started_run_ids: Mutex::new(Vec::new()), - warnings: Vec::new(), - create_error: true, - }); - let params: FabroRunCreateParams = serde_json::from_value(json!({ - "runs": ["simple.fabro"] - })) .unwrap(); - - create_runs( - backend.clone(), - temp.path(), - ValidatedCreateRuns::try_from(params).unwrap(), - ) - .await - .expect_err("create failure should be returned"); - - assert!(backend.started_run_ids.lock().unwrap().is_empty()); + state.assert_calls_async(1).await; + create.assert_calls_async(1).await; } - #[test] - fn create_run_result_omits_empty_warnings() { - let result = CreateRunsResult { - runs: vec![CreatedRunResult { - run_id: RunId::new().to_string(), - parent_id: None, - children_count: 0, - workflow: "simple".to_string(), - start_requested: false, - status: "submitted".to_string(), - warnings: Vec::new(), - }], - }; - - assert!( - serde_json::to_value(result).unwrap()["runs"][0] - .get("warnings") - .is_none() - ); + #[tokio::test] + async fn run_create_start_and_later_batch_failures_report_already_created_ids() { + let server = MockServer::start_async().await; + let id = RunId::new(); + server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/runs") + .json_body_includes(json!({"goal":"first"}).to_string()); + then.status(201).json_body_obj(&run(id, None, 0)); + }) + .await; + let start = server + .mock_async(|when, then| { + when.method(POST).path(format!("/api/v1/runs/{id}/start")); + then.status(500); + }) + .await; + let mut item = spec(); + item["goal"] = json!("first"); + item.as_object_mut().unwrap().remove("start"); + let error = create_runs(backend(&server), params(item.clone())) + .await + .unwrap_err(); + assert!(error.as_str().contains(&id.to_string())); + assert!(error.as_str().contains("start request failed")); + start.assert_calls_async(1).await; + item["start"] = json!(false); + server + .mock_async(|when, then| { + when.method(GET).path(format!("/api/v1/runs/{id}")); + then.status(200).json_body_obj(&run(id, None, 0)); + }) + .await; + let failed = server + .mock_async(|when, then| { + when.method(POST) + .path("/api/v1/runs") + .json_body_includes(json!({"goal":"second"}).to_string()); + then.status(400); + }) + .await; + let mut second = item.clone(); + second["goal"] = json!("second"); + let p = serde_json::from_value(json!({"runs":[item,second]})).unwrap(); + let error = create_runs(backend(&server), p).await.unwrap_err(); + assert!(error.as_str().contains(&id.to_string())); + failed.assert_calls_async(1).await; } - fn run_id(raw: &str) -> RunId { - raw.parse().expect("test run id should parse") + #[tokio::test] + async fn run_create_same_run_backend_denies_before_network() { + let client = fabro_client::Client::new_no_proxy("http://127.0.0.1:1").unwrap(); + let backend = Arc::new(ClientBackend::new(Arc::new(client)).with_run_scope(RunId::new())); + let error = create_runs(backend, params(spec())).await.unwrap_err(); + assert!(error.as_str().contains("outside this tool session")); } - - fn selector(value: &str) -> CreateRunWorkflowSource { - CreateRunWorkflowSource::Selector(value.to_string()) - } - fn run(run_id: RunId, parent_id: Option, children_count: u64) -> Run { run_with_status(run_id, parent_id, children_count, RunStatus::Submitted) } @@ -1427,152 +650,4 @@ mod tests { links: RunLinks { web: None }, } } - - struct MockCreateBackend { - child_id: RunId, - parent_id: RunId, - created_parent_ids: Mutex>>, - resolved_selectors: Mutex>, - started_run_ids: Mutex>, - warnings: Vec, - create_error: bool, - } - - #[async_trait] - impl FabroToolBackend for MockCreateBackend { - async fn create_run_from_spec( - &self, - _spec: &ValidatedCreateRunSpec, - _cwd: &Path, - parent_id: Option, - ) -> anyhow::Result { - self.created_parent_ids.lock().unwrap().push(parent_id); - if self.create_error { - anyhow::bail!("create failed"); - } - Ok(crate::CreateRunSubmission { - run_id: self.child_id, - warnings: self.warnings.clone(), - }) - } - - async fn resolve_run(&self, selector: &str) -> anyhow::Result { - assert_eq!(selector, "nightly-parent"); - self.resolved_selectors - .lock() - .unwrap() - .push(selector.to_string()); - Ok(run(self.parent_id, None, 1)) - } - - async fn retrieve_run(&self, run_id: &RunId) -> anyhow::Result { - assert_eq!(*run_id, self.child_id); - Ok(run(self.child_id, Some(self.parent_id), 0)) - } - - async fn start_run(&self, run_id: &RunId, resume: bool) -> anyhow::Result { - assert_eq!(*run_id, self.child_id); - assert!(!resume); - self.started_run_ids.lock().unwrap().push(*run_id); - Ok(run_with_status( - self.child_id, - Some(self.parent_id), - 0, - RunStatus::Pending { - reason: fabro_types::PendingReason::ApprovalRequired, - }, - )) - } - - async fn approve_run(&self, _run_id: &RunId) -> anyhow::Result { - unreachable!() - } - - async fn deny_run(&self, _run_id: &RunId, _reason: Option) -> anyhow::Result { - unreachable!() - } - - async fn cancel_run(&self, _run_id: &RunId) -> anyhow::Result { - unreachable!() - } - - async fn interrupt_run(&self, _run_id: &RunId) -> anyhow::Result<()> { - unreachable!() - } - - async fn steer_run( - &self, - _run_id: &RunId, - _text: String, - _interrupt: bool, - ) -> anyhow::Result<()> { - unreachable!() - } - - async fn archive_run(&self, _run_id: &RunId) -> anyhow::Result { - unreachable!() - } - - async fn unarchive_run(&self, _run_id: &RunId) -> anyhow::Result { - unreachable!() - } - - async fn list_store_runs(&self) -> anyhow::Result> { - unreachable!() - } - - async fn list_store_runs_by_parent(&self, _parent_id: RunId) -> anyhow::Result> { - unreachable!() - } - - async fn link_run_parent( - &self, - _child_id: &RunId, - _parent_id: &RunId, - ) -> anyhow::Result { - unreachable!() - } - - async fn unlink_run_parent(&self, _child_id: &RunId) -> anyhow::Result { - unreachable!() - } - - async fn get_run_state(&self, _run_id: &RunId) -> anyhow::Result { - unreachable!() - } - - async fn list_run_events( - &self, - _run_id: &RunId, - _after: Option, - _limit: Option, - ) -> anyhow::Result> { - unreachable!() - } - - async fn list_run_events_until( - &self, - _run_id: &RunId, - _after: Option, - _limit: usize, - ) -> anyhow::Result> { - unreachable!() - } - - async fn list_run_questions( - &self, - _run_id: &RunId, - ) -> anyhow::Result> { - unreachable!() - } - - async fn submit_run_answer( - &self, - _run_id: &RunId, - _question_id: &str, - _body: types::SubmitAnswerRequest, - ) -> anyhow::Result<()> { - unreachable!() - } - } } diff --git a/lib/components/fabro-tool/src/fabro_client.rs b/lib/components/fabro-tool/src/fabro_client.rs index b5016e703..f21e99bc5 100644 --- a/lib/components/fabro-tool/src/fabro_client.rs +++ b/lib/components/fabro-tool/src/fabro_client.rs @@ -1,23 +1,17 @@ -use std::path::Path; use std::sync::Arc; use async_trait::async_trait; use fabro_api::types; use fabro_types::{ EventEnvelope, PairId, PairMessageRecord, PairMessageRequest, PairRecord, - PairTranscriptResponse, Run, RunId, RunIntent, RunIntentArgs, RunPairStatusResponse, - RunProjection, StageId, + PairTranscriptResponse, Run, RunId, RunIntent, RunPairStatusResponse, RunProjection, StageId, }; -use crate::{ - CreateRunSubmission, FabroToolBackend, PreparedRunCreate, RunCreateAdapter, ToolError, - ValidatedCreateRunSpec, common, -}; +use crate::{FabroToolBackend, common}; #[derive(Clone)] pub struct ClientBackend { client: Arc<::fabro_client::Client>, - run_create_adapter: Option>, run_scope: Option, workflow_version_packager: Option>, } @@ -27,18 +21,11 @@ impl ClientBackend { pub fn new(client: Arc<::fabro_client::Client>) -> Self { Self { client, - run_create_adapter: None, run_scope: None, workflow_version_packager: None, } } - #[must_use] - pub fn with_run_create_adapter(mut self, adapter: Arc) -> Self { - self.run_create_adapter = Some(adapter); - self - } - #[must_use] pub fn with_workflow_version_packager( mut self, @@ -68,41 +55,6 @@ impl ClientBackend { } } -fn run_intent_from_spec( - spec: &ValidatedCreateRunSpec, - prepared: PreparedRunCreate, - parent_id: Option, -) -> (RunIntent, Vec) { - let PreparedRunCreate { - workflow_version_id, - target, - goal, - warnings, - } = prepared; - let intent = RunIntent { - workflow_version_id, - target, - args: RunIntentArgs { - model: spec.model.clone(), - provider: spec.provider.clone(), - inputs: spec - .inputs - .iter() - .map(|(key, value)| (key.clone(), value.json().clone())) - .collect(), - labels: spec.labels.clone(), - dry_run: spec.dry_run, - auto_approve: spec.auto_approve, - preserve_sandbox: spec.preserve_sandbox, - }, - environment_id: spec.environment.clone(), - parent_id, - title: None, - goal, - }; - (intent, warnings) -} - #[async_trait] impl FabroToolBackend for ClientBackend { /// Package the supplied tree, then register dependencies before parents. @@ -129,26 +81,12 @@ impl FabroToolBackend for ClientBackend { Ok(packaged.root_id()) } - async fn create_run_from_spec( - &self, - spec: &crate::ValidatedCreateRunSpec, - cwd: &Path, - parent_id: Option, - ) -> anyhow::Result { - if let Some(parent_id) = parent_id.as_ref() { - self.ensure_run_scope(parent_id)?; - } - let Some(adapter) = self.run_create_adapter.as_ref() else { - return Err(ToolError::message(format!( - "{} is not available", - crate::FABRO_RUN_CREATE_TOOL_NAME - )) - .into()); - }; - let prepared = adapter.prepare(&self.client, spec, cwd).await?; - let (intent, warnings) = run_intent_from_spec(spec, prepared, parent_id); - let run_id = self.client.create_run_from_intent(intent).await?; - Ok(CreateRunSubmission { run_id, warnings }) + async fn create_run_from_intent(&self, intent: RunIntent) -> anyhow::Result { + anyhow::ensure!( + self.run_scope.is_none(), + "run creation is outside this tool session's run scope" + ); + self.client.create_run_from_intent(intent).await } async fn resolve_run(&self, selector: &str) -> anyhow::Result { diff --git a/lib/components/fabro-tool/src/interact.rs b/lib/components/fabro-tool/src/interact.rs index 16ba9edd4..0757cdb51 100644 --- a/lib/components/fabro-tool/src/interact.rs +++ b/lib/components/fabro-tool/src/interact.rs @@ -446,7 +446,6 @@ fn normalize_optional_text(value: Option<&str>) -> Option { #[cfg(test)] mod tests { use std::collections::HashMap; - use std::path::Path; use std::sync::{Arc, Mutex}; use async_trait::async_trait; @@ -742,12 +741,10 @@ mod tests { #[async_trait] impl FabroToolBackend for MockInteractBackend { - async fn create_run_from_spec( + async fn create_run_from_intent( &self, - _spec: &crate::ValidatedCreateRunSpec, - _cwd: &Path, - _parent_id: Option, - ) -> anyhow::Result { + _intent: fabro_types::RunIntent, + ) -> anyhow::Result { unreachable!() } diff --git a/lib/components/fabro-tool/src/lib.rs b/lib/components/fabro-tool/src/lib.rs index 0c661b32a..123c64af3 100644 --- a/lib/components/fabro-tool/src/lib.rs +++ b/lib/components/fabro-tool/src/lib.rs @@ -17,17 +17,14 @@ mod search; mod workflow_version; pub use common::{ - CreateRunSubmission, FABRO_RUN_CREATE_TOOL_NAME, FABRO_RUN_EVENTS_TOOL_NAME, - FABRO_RUN_GATHER_TOOL_NAME, FABRO_RUN_GET_TOOL_NAME, FABRO_RUN_INTERACT_TOOL_NAME, - FABRO_RUN_PAIR_TOOL_NAME, FABRO_RUN_SEARCH_TOOL_NAME, FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME, - FabroToolBackend, PreparedRunCreate, RunCreateAdapter, RunSummaryResult, ToolDefinition, - ToolError, ToolResult, tool_definitions, + FABRO_RUN_CREATE_TOOL_NAME, FABRO_RUN_EVENTS_TOOL_NAME, FABRO_RUN_GATHER_TOOL_NAME, + FABRO_RUN_GET_TOOL_NAME, FABRO_RUN_INTERACT_TOOL_NAME, FABRO_RUN_PAIR_TOOL_NAME, + FABRO_RUN_SEARCH_TOOL_NAME, FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME, FabroToolBackend, + RunSummaryResult, ToolDefinition, ToolError, ToolResult, tool_definitions, }; pub use create::{ - CreateRunOptions, CreateRunSpec, CreateRunSpecInput, CreateRunWorkflowSource, CreateRunsResult, - CreatedRunResult, FabroRunCreateParams, InlineWorkflowSource, RunInputValue, - ValidatedCreateRunSpec, ValidatedCreateRuns, ValidatedRunInputValue, create_runs, - create_runs_text, create_runs_with_options, + CreateRunOptions, CreateRunSpec, CreateRunsResult, CreatedRunResult, FabroRunCreateParams, + create_runs, create_runs_text, create_runs_with_options, }; pub use events::{ FabroRunEventsParams, RunEventResult, RunEventsAction, RunEventsResult, ValidatedRunEvents, diff --git a/lib/components/fabro-workflow/src/handler/llm/fabro_tools.rs b/lib/components/fabro-workflow/src/handler/llm/fabro_tools.rs index c2903a64a..0152b5d8f 100644 --- a/lib/components/fabro-workflow/src/handler/llm/fabro_tools.rs +++ b/lib/components/fabro-workflow/src/handler/llm/fabro_tools.rs @@ -3,7 +3,6 @@ use std::sync::Arc; -use fabro_types::RunId; use pebble_coding_agent::tools::{RegisteredTool, ToolError, ToolSource}; use serde::de::DeserializeOwned; @@ -77,13 +76,9 @@ pub(crate) async fn execute_fabro_run_tool( } fabro_tool::FABRO_RUN_CREATE_TOOL_NAME => { let params = parse_fabro_tool_args::(name, args)?; - ensure_current_run_parent(¶ms, services.current_run_id)?; - let validated = fabro_tool::ValidatedCreateRuns::try_from(params)?; let result = fabro_tool::create_runs_with_options( Arc::clone(&services.backend), - &services.base_cwd, - &services.user_settings_path, - validated, + params, fabro_tool::CreateRunOptions { forced_parent_id: Some(services.current_run_id), }, @@ -168,34 +163,6 @@ where .map_err(|err| fabro_tool::ToolError::message(format!("invalid {name} arguments: {err}"))) } -fn ensure_current_run_parent( - params: &fabro_tool::FabroRunCreateParams, - current_run_id: RunId, -) -> fabro_tool::ToolResult<()> { - let current_parent = current_run_id.to_string(); - for run in ¶ms.runs { - let parent_id = match run { - fabro_tool::CreateRunSpecInput::Workflow(_) => None, - fabro_tool::CreateRunSpecInput::Spec(spec) => spec.parent_id.as_deref().map(str::trim), - }; - match parent_id { - None => {} - Some("") => { - return Err(fabro_tool::ToolError::message( - "parent_id must be omitted or match the current run; blank parent_id is invalid", - )); - } - Some(parent_id) if parent_id == current_parent => {} - Some(parent_id) => { - return Err(fabro_tool::ToolError::message(format!( - "parent_id must be omitted or match the current run {current_parent}; got {parent_id}" - ))); - } - } - } - Ok(()) -} - fn render_fabro_tool_result(summary: &str, result: &T) -> fabro_tool::ToolResult where T: serde::Serialize, @@ -219,6 +186,57 @@ mod tests { use super::*; + #[tokio::test] + async fn native_run_create_submits_intent_and_enforces_current_parent() { + let server = httpmock::MockServer::start_async().await; + let parent_id = fabro_types::RunId::new(); + let version_id: fabro_types::WorkflowVersionId = + fabro_types::BlobHash::new(b"registered workflow").into(); + let create = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/runs") + .json_body(json!({ + "workflow_version_id": version_id, + "target": {"kind":"none"}, + "parent_id": parent_id, + "args": {"auto_approve":false} + })); + // Admission rejection proves the native dispatcher reached the + // canonical API without registering or looking up a workflow. + then.status(422).body("native admission rejection"); + }) + .await; + let state = server + .mock_async(|when, then| { + when.path(format!("/api/v1/runs/{parent_id}/state")); + then.status(500); + }) + .await; + let client = fabro_client::Client::new_no_proxy(&server.url("")).unwrap(); + let services = FabroRunToolServices { + backend: Arc::new(ClientBackend::new(Arc::new(client))), + current_run_id: parent_id, + }; + let name = fabro_tool::FABRO_RUN_CREATE_TOOL_NAME; + let mut args = json!({"runs":[{ + "workflow_version_id":version_id, + "target":{"kind":"none"}, + "args":{"auto_approve":false} + }]}); + let error = execute_fabro_run_tool(name, args.clone(), &services) + .await + .unwrap_err(); + assert!(error.to_string().contains("native admission rejection")); + args["runs"][0]["parent_id"] = json!(fabro_types::RunId::new()); + let error = execute_fabro_run_tool(name, args, &services) + .await + .unwrap_err(); + assert!(error.to_string().contains("match the current run")); + create.assert_calls_async(1).await; + state.assert_calls_async(0).await; + } + struct SingleGraphPackager; #[async_trait] @@ -257,13 +275,11 @@ mod tests { .await; let client = fabro_client::Client::new_no_proxy(&server.url("")).unwrap(); let services = FabroRunToolServices { - backend: Arc::new( + backend: Arc::new( ClientBackend::new(Arc::new(client)) .with_workflow_version_packager(Arc::new(SingleGraphPackager)), ), - current_run_id: "01KRBZW4DW0000000000000002".parse().unwrap(), - base_cwd: "unused".into(), - user_settings_path: "unused".into(), + current_run_id: "01KRBZW4DW0000000000000002".parse().unwrap(), }; let name = fabro_tool::FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME; assert_eq!(register_named_fabro_run_tools(&services, &[name]).len(), 1); diff --git a/lib/components/fabro-workflow/src/services.rs b/lib/components/fabro-workflow/src/services.rs index 949bbf7d4..5f6511b43 100644 --- a/lib/components/fabro-workflow/src/services.rs +++ b/lib/components/fabro-workflow/src/services.rs @@ -79,7 +79,6 @@ impl RunLocations { pub struct FabroRunToolServices { pub backend: Arc, pub current_run_id: RunId, - pub base_cwd: PathBuf, } /// Services shared across workflow phases. diff --git a/lib/foundation/fabro-config/src/user.rs b/lib/foundation/fabro-config/src/user.rs index 00c403699..303f8cc2a 100644 --- a/lib/foundation/fabro-config/src/user.rs +++ b/lib/foundation/fabro-config/src/user.rs @@ -23,10 +23,6 @@ pub fn default_storage_dir() -> PathBuf { Home::from_env().root().join("storage") } -pub fn default_workflows_dir() -> PathBuf { - Home::from_env().workflows_dir() -} - pub fn default_socket_path() -> PathBuf { Home::from_env().root().join("fabro.sock") } @@ -81,8 +77,8 @@ mod tests { use temp_env::with_var; use super::{ - SETTINGS_CONFIG_FILENAME, active_settings_path, active_settings_path_with_lookup, - default_settings_path, default_socket_path, default_storage_dir, default_workflows_dir, + SETTINGS_CONFIG_FILENAME, active_settings_path_with_lookup, default_settings_path, + default_socket_path, default_storage_dir, }; #[test] @@ -95,29 +91,10 @@ mod tests { home.join(".fabro").join(SETTINGS_CONFIG_FILENAME) ); assert_eq!(default_storage_dir(), home.join(".fabro/storage")); - assert_eq!(default_workflows_dir(), home.join(".fabro/workflows")); assert_eq!(default_socket_path(), home.join(".fabro/fabro.sock")); }); } - #[test] - fn workflows_path_uses_fabro_home_when_config_is_elsewhere() { - let dir = tempfile::tempdir().unwrap(); - let fabro_home = dir.path().join("fabro-home"); - let custom_config = dir.path().join("config/settings.toml"); - - with_var(EnvVars::FABRO_HOME, Some(fabro_home.as_os_str()), || { - with_var( - EnvVars::FABRO_CONFIG, - Some(custom_config.as_os_str()), - || { - assert_eq!(active_settings_path(None), custom_config); - assert_eq!(default_workflows_dir(), fabro_home.join("workflows")); - }, - ); - }); - } - #[test] fn active_settings_path_honors_fabro_config_env() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 3783f0bc3..2259c3814 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -198,6 +198,6 @@ pub use workflow_path::{ pub use workflow_version::{ MAX_WORKFLOW_VERSION_BYTES, MAX_WORKFLOW_VERSION_DEPENDENCIES, MAX_WORKFLOW_VERSION_FILE_BYTES, MAX_WORKFLOW_VERSION_FILES, WorkflowVersion, WorkflowVersionShapeError, - validate_workflow_files, validate_workflow_source_paths, validate_workflow_path_collisions, + validate_workflow_files, validate_workflow_source_paths, }; pub use workflow_version_id::{WorkflowVersionId, WorkflowVersionIdParseError}; diff --git a/lib/foundation/fabro-types/src/workflow_version.rs b/lib/foundation/fabro-types/src/workflow_version.rs index b4532c470..58fb8e0ad 100644 --- a/lib/foundation/fabro-types/src/workflow_version.rs +++ b/lib/foundation/fabro-types/src/workflow_version.rs @@ -194,13 +194,6 @@ pub fn validate_workflow_source_paths<'a>( }) } -/// Reject exact duplicate paths and file/directory ancestor collisions. -pub fn validate_workflow_path_collisions<'a>( - paths: impl IntoIterator, -) -> Result<(), WorkflowVersionShapeError> { - validate_path_collisions(paths, Cow::Borrowed) -} - /// Detect colliding paths under a comparison key: identical keys, or a key /// that names an ancestor directory of another. fn validate_path_collisions<'a>(