From 4ca8962a02879b5f3f2c755eb9b5eb2480f2f908 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Apr 2026 14:50:04 -0700 Subject: [PATCH] fix(cli): keep validate off runtime preflight Add a validation-only API response and route while keeping fabro validate local so it does not start or contact the server for structural workflow checks. --- docs/public/api-reference/fabro-api.yaml | 40 ++++++++- lib/crates/fabro-cli/src/args.rs | 3 - lib/crates/fabro-cli/src/commands/validate.rs | 10 +-- lib/crates/fabro-cli/tests/it/cmd/validate.rs | 20 ++++- lib/crates/fabro-client/src/client.rs | 15 ++++ lib/crates/fabro-server/src/lib.rs | 1 + .../fabro-server/src/manifest_validation.rs | 20 +++++ lib/crates/fabro-server/src/run_manifest.rs | 34 +++++--- lib/crates/fabro-server/src/server.rs | 50 +++++++++++ .../src/.openapi-generator/FILES | 1 + .../fabro-api-client/src/api/runs-api.ts | 85 ++++++++++++++++++- .../fabro-api-client/src/models/index.ts | 1 + .../src/models/validate-response.ts | 27 ++++++ 13 files changed, 283 insertions(+), 24 deletions(-) create mode 100644 lib/crates/fabro-server/src/manifest_validation.rs create mode 100644 lib/packages/fabro-api-client/src/models/validate-response.ts diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 0a2f57a73..b3a3fd4b9 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -617,7 +617,7 @@ paths: operationId: runPreflight tags: [Runs] summary: Validate Workflow Manifest - description: Validates a workflow manifest without creating a run. + description: Validates runtime readiness for a workflow manifest without creating a run. requestBody: required: true content: @@ -638,6 +638,32 @@ paths: schema: $ref: "#/components/schemas/ErrorResponse" + /api/v1/validate: + post: + operationId: validateRunManifest + tags: [Runs] + summary: Validate Workflow Manifest + description: Validates workflow structure and diagnostics without runtime readiness checks. + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/RunManifest" + responses: + "200": + description: Validation result + content: + application/json: + schema: + $ref: "#/components/schemas/ValidateResponse" + "400": + description: Invalid manifest or workflow + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + /api/v1/graph/render: post: operationId: renderWorkflowGraph @@ -3865,6 +3891,18 @@ components: checks: $ref: "#/components/schemas/PreflightCheckReport" + ValidateResponse: + type: object + required: + - ok + - workflow + properties: + ok: + type: boolean + description: Whether validation passed with no error diagnostics. + workflow: + $ref: "#/components/schemas/PreflightWorkflowSummary" + RenderWorkflowGraphRequest: type: object required: diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 4ff68efe5..e321d4679 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -388,9 +388,6 @@ pub(crate) struct LogsArgs { #[derive(Args)] pub(crate) struct ValidateArgs { - #[command(flatten)] - pub(crate) target: ServerTargetArgs, - /// Path to the .fabro workflow file pub(crate) workflow: PathBuf, } diff --git a/lib/crates/fabro-cli/src/commands/validate.rs b/lib/crates/fabro-cli/src/commands/validate.rs index 545398987..093bccb12 100644 --- a/lib/crates/fabro-cli/src/commands/validate.rs +++ b/lib/crates/fabro-cli/src/commands/validate.rs @@ -1,5 +1,7 @@ use anyhow::bail; +use fabro_config::RunLayer; use fabro_config::user::active_settings_path; +use fabro_server::manifest_validation; use fabro_util::terminal::Styles; use crate::args::ValidateArgs; @@ -14,21 +16,19 @@ pub(crate) async fn run( base_ctx: &CommandContext, ) -> anyhow::Result<()> { let printer = base_ctx.printer(); - let ctx = base_ctx.with_target(&args.target)?; let built = build_run_manifest(ManifestBuildInput { workflow: args.workflow.clone(), - cwd: ctx.cwd().to_path_buf(), + cwd: base_ctx.cwd().to_path_buf(), run_overrides: None, cli_overrides: None, args: None, run_id: None, user_settings_path: Some(active_settings_path(None)), })?; - let client = ctx.server().await?; - let response = client.run_preflight(built.manifest).await?; + let response = manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; let diagnostics = api_diagnostics_to_local(&response.workflow.diagnostics); - if ctx.json_output() { + if base_ctx.json_output() { print_json_pretty(&serde_json::json!({ "workflow_name": response.workflow.name, "nodes": response.workflow.nodes, diff --git a/lib/crates/fabro-cli/tests/it/cmd/validate.rs b/lib/crates/fabro-cli/tests/it/cmd/validate.rs index 7c43f8e3c..2d91255d5 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/validate.rs @@ -1,5 +1,7 @@ use fabro_test::{fabro_snapshot, test_context}; +use crate::support::LightweightCli; + fn fixture(name: &str) -> std::path::PathBuf { std::path::Path::new(env!("CARGO_MANIFEST_DIR")) .join(format!("../../../test/{name}")) @@ -25,7 +27,6 @@ fn help() { Options: --json Output as JSON [env: FABRO_JSON=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] --quiet Suppress non-essential output [env: FABRO_QUIET=] @@ -51,6 +52,23 @@ fn simple() { "); } +#[test] +fn simple_does_not_connect_to_configured_server() { + let cli = LightweightCli::new(); + let mut cmd = cli.command(); + cmd.env("FABRO_SERVER", "http://127.0.0.1:9") + .arg("validate") + .arg(fixture("simple.fabro")); + + let output = cmd.output().expect("validate should execute"); + assert!( + output.status.success(), + "validate should run locally without connecting to FABRO_SERVER\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); +} + #[test] fn branching() { let context = test_context!(); diff --git a/lib/crates/fabro-client/src/client.rs b/lib/crates/fabro-client/src/client.rs index 4af8d04df..3d76f9c42 100644 --- a/lib/crates/fabro-client/src/client.rs +++ b/lib/crates/fabro-client/src/client.rs @@ -695,6 +695,21 @@ impl Client { .map(progenitor_client::ResponseValue::into_inner) } + pub async fn validate_run_manifest( + &self, + manifest: types::RunManifest, + ) -> Result { + self.send_api(|client| async move { + client + .validate_run_manifest() + .body(manifest.clone()) + .send() + .await + }) + .await + .map(progenitor_client::ResponseValue::into_inner) + } + pub async fn render_workflow_graph( &self, request: types::RenderWorkflowGraphRequest, diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index 6aac6b4b8..ca6cae4dc 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -24,6 +24,7 @@ pub mod github_webhooks; pub mod install; pub mod ip_allowlist; pub mod jwt_auth; +pub mod manifest_validation; mod run_files; mod run_files_security; mod run_manifest; diff --git a/lib/crates/fabro-server/src/manifest_validation.rs b/lib/crates/fabro-server/src/manifest_validation.rs new file mode 100644 index 000000000..80e450e7a --- /dev/null +++ b/lib/crates/fabro-server/src/manifest_validation.rs @@ -0,0 +1,20 @@ +use anyhow::{Result, anyhow}; +use fabro_api::types; +use fabro_config::RunLayer; +use fabro_workflow::Error as WorkflowError; + +pub fn validate_manifest( + manifest_run_defaults: &RunLayer, + manifest: &types::RunManifest, +) -> Result { + let prepared = crate::run_manifest::prepare_manifest(manifest_run_defaults, manifest)?; + let validated = crate::run_manifest::validate_prepared_manifest(&prepared) + .map_err(validation_error_to_anyhow)?; + Ok(crate::run_manifest::validate_response( + &prepared, &validated, + )) +} + +fn validation_error_to_anyhow(err: WorkflowError) -> anyhow::Error { + anyhow!("{err}") +} diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index d6ab35473..d18bf7ab1 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -186,6 +186,16 @@ pub(crate) async fn run_preflight( )) } +pub(crate) fn validate_response( + prepared: &PreparedManifest, + validated: &Validated, +) -> types::ValidateResponse { + types::ValidateResponse { + ok: !validated.has_errors(), + workflow: workflow_summary(validated, &prepared.target_path), + } +} + pub(crate) fn graph_source(prepared: &PreparedManifest, direction: Option<&str>) -> String { direction.map_or_else( || prepared.root_source.clone(), @@ -980,16 +990,20 @@ fn preflight_response( types::PreflightResponse { ok, checks: report_to_api(report), - workflow: types::PreflightWorkflowSummary { - diagnostics: diagnostics_to_api(validated.diagnostics()), - edges: i64::try_from(validated.graph().edges.len()) - .expect("graph edge count should fit in i64"), - goal: validated.graph().goal().to_string(), - graph_path: Some(target_path.display().to_string()), - name: validated.graph().name.clone(), - nodes: i64::try_from(validated.graph().nodes.len()) - .expect("graph node count should fit in i64"), - }, + workflow: workflow_summary(validated, target_path), + } +} + +fn workflow_summary(validated: &Validated, target_path: &Path) -> types::PreflightWorkflowSummary { + types::PreflightWorkflowSummary { + diagnostics: diagnostics_to_api(validated.diagnostics()), + edges: i64::try_from(validated.graph().edges.len()) + .expect("graph edge count should fit in i64"), + goal: validated.graph().goal().to_string(), + graph_path: Some(target_path.display().to_string()), + name: validated.graph().name.clone(), + nodes: i64::try_from(validated.graph().nodes.len()) + .expect("graph node count should fit in i64"), } } diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index e611db324..fcd3e2e3b 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -1118,6 +1118,7 @@ fn demo_routes() -> Router> { .route("/runs/resolve", get(demo::resolve_run)) .route("/boards/runs", get(demo::list_board_runs)) .route("/preflight", post(run_preflight)) + .route("/validate", post(validate_run_manifest)) .route("/graph/render", post(render_graph_from_manifest)) .route("/attach", get(demo::attach_events_stub)) .route("/runs/{id}", get(demo::get_run_status)) @@ -1201,6 +1202,7 @@ fn real_routes() -> Router> { .route("/runs", get(list_runs).post(create_run)) .route("/runs/resolve", get(resolve_run)) .route("/preflight", post(run_preflight)) + .route("/validate", post(validate_run_manifest)) .route("/graph/render", post(render_graph_from_manifest)) .route("/attach", get(attach_events)) .route("/boards/runs", get(list_board_runs)) @@ -4322,6 +4324,30 @@ async fn run_preflight( (StatusCode::OK, Json(response)).into_response() } +async fn validate_run_manifest( + _auth: AuthenticatedService, + State(state): State>, + Json(req): Json, +) -> Response { + let manifest_run_defaults = state.manifest_run_defaults(); + let prepared = match run_manifest::prepare_manifest(manifest_run_defaults.as_ref(), &req) { + Ok(prepared) => prepared, + Err(err) => return ApiError::bad_request(err.to_string()).into_response(), + }; + let validated = match run_manifest::validate_prepared_manifest(&prepared) { + Ok(validated) => validated, + Err(WorkflowError::Parse(_)) => { + return ApiError::bad_request("Validation failed").into_response(); + } + Err(err) => return ApiError::bad_request(err.to_string()).into_response(), + }; + ( + StatusCode::OK, + Json(run_manifest::validate_response(&prepared, &validated)), + ) + .into_response() +} + async fn render_graph_from_manifest( _auth: AuthenticatedService, State(state): State>, @@ -9468,6 +9494,29 @@ allowed_usernames = ["octocat"] body["id"].as_str().unwrap().to_string() } + #[tokio::test] + async fn validate_endpoint_returns_workflow_summary_without_preflight_checks() { + let app = test_app_with(); + let response = app + .oneshot( + Request::builder() + .method("POST") + .uri(api("/validate")) + .header("content-type", "application/json") + .body(manifest_body(MINIMAL_DOT)) + .unwrap(), + ) + .await + .unwrap(); + let body = response_json!(response, StatusCode::OK).await; + + assert_eq!(body["ok"], true); + assert_eq!(body["workflow"]["name"], "Test"); + assert_eq!(body["workflow"]["nodes"], 2); + assert_eq!(body["workflow"]["edges"], 1); + assert!(body.get("checks").is_none()); + } + async fn create_run_for_target(app: &Router, target_path: &str, dot_source: &str) -> String { let req = Request::builder() .method("POST") @@ -11811,6 +11860,7 @@ slug = "fabro" (Method::POST, "/runs".to_string()), (Method::GET, "/runs/resolve".to_string()), (Method::POST, "/preflight".to_string()), + (Method::POST, "/validate".to_string()), (Method::POST, "/graph/render".to_string()), (Method::GET, "/attach".to_string()), (Method::GET, "/boards/runs".to_string()), diff --git a/lib/packages/fabro-api-client/src/.openapi-generator/FILES b/lib/packages/fabro-api-client/src/.openapi-generator/FILES index 8c34eb72c..4f159dd97 100644 --- a/lib/packages/fabro-api-client/src/.openapi-generator/FILES +++ b/lib/packages/fabro-api-client/src/.openapi-generator/FILES @@ -257,6 +257,7 @@ models/timeline-entry-response.ts models/tool-stage-turn.ts models/tool-use.ts models/user-response.ts +models/validate-response.ts models/webhook-strategy.ts models/workflow-diagnostic.ts models/workflow-reference.ts diff --git a/lib/packages/fabro-api-client/src/api/runs-api.ts b/lib/packages/fabro-api-client/src/api/runs-api.ts index f79957b47..bebdf48a6 100644 --- a/lib/packages/fabro-api-client/src/api/runs-api.ts +++ b/lib/packages/fabro-api-client/src/api/runs-api.ts @@ -61,6 +61,8 @@ import type { RunSummary } from '../models'; import type { StartRunRequest } from '../models'; // @ts-ignore import type { TimelineEntryResponse } from '../models'; +// @ts-ignore +import type { ValidateResponse } from '../models'; /** * RunsApi - axios parameter creator */ @@ -870,7 +872,7 @@ export const RunsApiAxiosParamCreator = function (configuration?: Configuration) }; }, /** - * Validates a workflow manifest without creating a run. + * Validates runtime readiness for a workflow manifest without creating a run. * @summary Validate Workflow Manifest * @param {RunManifest} runManifest * @param {*} [options] Override http request option. @@ -1028,6 +1030,47 @@ export const RunsApiAxiosParamCreator = function (configuration?: Configuration) let headersFromBaseOptions = baseOptions && baseOptions.headers ? baseOptions.headers : {}; localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...options.headers}; + return { + url: toPathString(localVarUrlObj), + options: localVarRequestOptions, + }; + }, + /** + * Validates workflow structure and diagnostics without runtime readiness checks. + * @summary Validate Workflow Manifest + * @param {RunManifest} runManifest + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + validateRunManifest: async (runManifest: RunManifest, options: RawAxiosRequestConfig = {}): Promise => { + // verify required parameter 'runManifest' is not null or undefined + assertParamExists('validateRunManifest', 'runManifest', runManifest) + const localVarPath = `/api/v1/validate`; + // use dummy base URL string because the URL constructor only accepts absolute URLs. + const localVarUrlObj = new URL(localVarPath, DUMMY_BASE_URL); + let baseOptions; + if (configuration) { + baseOptions = configuration.baseOptions; + } + + const localVarRequestOptions = { method: 'POST', ...baseOptions, ...options}; + const localVarHeaderParameter = {} as any; + const localVarQueryParameter = {} as any; + + // authentication SessionCookie required + + // authentication BearerAuth required + // http bearer authentication required + await setBearerAuthToObject(localVarHeaderParameter, configuration) + + localVarHeaderParameter['Content-Type'] = 'application/json'; + localVarHeaderParameter['Accept'] = 'application/json'; + + setSearchParams(localVarUrlObj, localVarQueryParameter); + let headersFromBaseOptions = baseOptions && baseOptions.headers ? baseOptions.headers : {}; + localVarRequestOptions.headers = {...localVarHeaderParameter, ...headersFromBaseOptions, ...options.headers}; + localVarRequestOptions.data = serializeDataIfNeeded(runManifest, localVarRequestOptions, configuration) + return { url: toPathString(localVarUrlObj), options: localVarRequestOptions, @@ -1298,7 +1341,7 @@ export const RunsApiFp = function(configuration?: Configuration) { return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); }, /** - * Validates a workflow manifest without creating a run. + * Validates runtime readiness for a workflow manifest without creating a run. * @summary Validate Workflow Manifest * @param {RunManifest} runManifest * @param {*} [options] Override http request option. @@ -1350,6 +1393,19 @@ export const RunsApiFp = function(configuration?: Configuration) { const localVarOperationServerBasePath = operationServerMap['RunsApi.unpauseRun']?.[localVarOperationServerIndex]?.url; return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); }, + /** + * Validates workflow structure and diagnostics without runtime readiness checks. + * @summary Validate Workflow Manifest + * @param {RunManifest} runManifest + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + async validateRunManifest(runManifest: RunManifest, options?: RawAxiosRequestConfig): Promise<(axios?: AxiosInstance, basePath?: string) => AxiosPromise> { + const localVarAxiosArgs = await localVarAxiosParamCreator.validateRunManifest(runManifest, options); + const localVarOperationServerIndex = configuration?.serverIndex ?? 0; + const localVarOperationServerBasePath = operationServerMap['RunsApi.validateRunManifest']?.[localVarOperationServerIndex]?.url; + return (axios, basePath) => createRequestFunction(localVarAxiosArgs, globalAxios, BASE_PATH, configuration)(axios, localVarOperationServerBasePath || basePath); + }, } }; @@ -1558,7 +1614,7 @@ export const RunsApiFactory = function (configuration?: Configuration, basePath? return localVarFp.rewindRun(id, rewindRequest, options).then((request) => request(axios, basePath)); }, /** - * Validates a workflow manifest without creating a run. + * Validates runtime readiness for a workflow manifest without creating a run. * @summary Validate Workflow Manifest * @param {RunManifest} runManifest * @param {*} [options] Override http request option. @@ -1598,6 +1654,16 @@ export const RunsApiFactory = function (configuration?: Configuration, basePath? unpauseRun(id: string, options?: RawAxiosRequestConfig): AxiosPromise { return localVarFp.unpauseRun(id, options).then((request) => request(axios, basePath)); }, + /** + * Validates workflow structure and diagnostics without runtime readiness checks. + * @summary Validate Workflow Manifest + * @param {RunManifest} runManifest + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + validateRunManifest(runManifest: RunManifest, options?: RawAxiosRequestConfig): AxiosPromise { + return localVarFp.validateRunManifest(runManifest, options).then((request) => request(axios, basePath)); + }, }; }; @@ -1823,7 +1889,7 @@ export class RunsApi extends BaseAPI { } /** - * Validates a workflow manifest without creating a run. + * Validates runtime readiness for a workflow manifest without creating a run. * @summary Validate Workflow Manifest * @param {RunManifest} runManifest * @param {*} [options] Override http request option. @@ -1866,5 +1932,16 @@ export class RunsApi extends BaseAPI { public unpauseRun(id: string, options?: RawAxiosRequestConfig) { return RunsApiFp(this.configuration).unpauseRun(id, options).then((request) => request(this.axios, this.basePath)); } + + /** + * Validates workflow structure and diagnostics without runtime readiness checks. + * @summary Validate Workflow Manifest + * @param {RunManifest} runManifest + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + public validateRunManifest(runManifest: RunManifest, options?: RawAxiosRequestConfig) { + return RunsApiFp(this.configuration).validateRunManifest(runManifest, options).then((request) => request(this.axios, this.basePath)); + } } diff --git a/lib/packages/fabro-api-client/src/models/index.ts b/lib/packages/fabro-api-client/src/models/index.ts index 023aac3cf..ea1e2d74f 100644 --- a/lib/packages/fabro-api-client/src/models/index.ts +++ b/lib/packages/fabro-api-client/src/models/index.ts @@ -236,6 +236,7 @@ export * from './timeline-entry-response'; export * from './tool-stage-turn'; export * from './tool-use'; export * from './user-response'; +export * from './validate-response'; export * from './webhook-strategy'; export * from './workflow-diagnostic'; export * from './workflow-reference'; diff --git a/lib/packages/fabro-api-client/src/models/validate-response.ts b/lib/packages/fabro-api-client/src/models/validate-response.ts new file mode 100644 index 000000000..e516a89ef --- /dev/null +++ b/lib/packages/fabro-api-client/src/models/validate-response.ts @@ -0,0 +1,27 @@ +/* tslint:disable */ +/* eslint-disable */ +/** + * Fabro Run API + * HTTP API for managing Fabro workflow run executions. + * + * The version of the OpenAPI document: 0.1.0 + * + * + * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). + * https://openapi-generator.tech + * Do not edit the class manually. + */ + + +// May contain unused imports in some cases +// @ts-ignore +import type { PreflightWorkflowSummary } from './preflight-workflow-summary'; + +export interface ValidateResponse { + /** + * Whether validation passed with no error diagnostics. + */ + 'ok': boolean; + 'workflow': PreflightWorkflowSummary; +} +