diff --git a/Cargo.lock b/Cargo.lock index 384eac09b..802408479 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2818,6 +2818,7 @@ dependencies = [ "fabro-types", "fabro-workflow", "git2", + "serde_json", "temp-env", "tempfile", "toml 0.8.23", diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 56b062382..d21a439a0 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -9197,7 +9197,7 @@ components: type: string ManifestGoal: - description: Resolved goal with provenance. + description: Resolved goal kind and content. type: object required: - type @@ -9212,9 +9212,6 @@ components: text: type: string description: Resolved goal content. - path: - type: ["string", "null"] - description: Original goal file path when the goal came from a file. ManifestArgs: description: Sparse command-local args that affect run settings. @@ -9248,13 +9245,8 @@ components: ManifestTarget: type: object required: - - identifier - path properties: - identifier: - type: string - description: What the user typed. - example: smoke path: type: string description: Resolved path that keys into the workflows map. diff --git a/lib/apps/fabro-cli/tests/it/cmd/support.rs b/lib/apps/fabro-cli/tests/it/cmd/support.rs index 362de669d..6470375a3 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/support.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/support.rs @@ -955,7 +955,6 @@ async fn create_seeded_run( "version": 1, "cwd": context.temp_dir.display().to_string(), "target": { - "identifier": target_path, "path": target_path, }, "args": args, diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index be968159f..01b0e200e 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -1400,8 +1400,7 @@ mod tests { parent_id: None, title: None, target: types::ManifestTarget { - identifier: "workflow.fabro".to_string(), - path: "workflow.fabro".to_string(), + path: "workflow.fabro".to_string(), }, version: 1, workflows: HashMap::from([("workflow.fabro".to_string(), types::ManifestWorkflow { @@ -2229,7 +2228,6 @@ id = "local" #[test] fn prepare_manifest_keeps_missing_metadata_names_absent() { let mut manifest = minimal_manifest(); - manifest.target.identifier = "release-flow".to_string(); manifest.workflows.get_mut("workflow.fabro").unwrap().source = r" digraph GraphName { start [shape=Mdiamond] diff --git a/lib/apps/fabro-server/src/server/automation_scheduler.rs b/lib/apps/fabro-server/src/server/automation_scheduler.rs index 0abd3e988..bfaea745c 100644 --- a/lib/apps/fabro-server/src/server/automation_scheduler.rs +++ b/lib/apps/fabro-server/src/server/automation_scheduler.rs @@ -401,7 +401,6 @@ mod tests { "version": 1, "cwd": "/tmp", "target": { - "identifier": "workflow.fabro", "path": "workflow.fabro", }, "workflows": { diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index ec44728d8..c0488e037 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -3190,7 +3190,6 @@ fn manifest_json(target_path: &str, dot_source: &str) -> serde_json::Value { "version": 1, "cwd": "/tmp", "target": { - "identifier": target_path, "path": target_path, }, "workflows": { @@ -3562,6 +3561,60 @@ async fn post_runs_ignores_removed_run_id_input() { assert_ne!(allocated_run_id, submitted_run_id); } +/// Old clients may still send the removed `target.identifier` and +/// `goal.path` manifest properties. The server must keep accepting such +/// bodies as unknown fields: the workflow is selected by `target.path` +/// and the goal comes from the resolved `goal.text`. +#[tokio::test] +async fn post_runs_accepts_legacy_manifest_metadata_properties() { + let state = test_app_state(); + let app = crate::test_support::build_test_router(Arc::clone(&state)); + let picked_dot = r"digraph PickedFlow { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> exit + }"; + let decoy_dot = r"digraph DecoyFlow { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> exit + }"; + let manifest = serde_json::json!({ + "version": 1, + "cwd": "/tmp", + "target": { + // Legacy display metadata: deliberately names the decoy entry + // to prove selection never reads it. + "identifier": "decoy.fabro", + "path": "picked.fabro", + }, + "goal": { + "type": "file", + "text": "Goal text from the legacy body", + "path": "/tmp/original/goal.md", + }, + "workflows": { + "picked.fabro": { "source": picked_dot, "files": {} }, + "decoy.fabro": { "source": decoy_dot, "files": {} }, + }, + }); + + let created = post_run_manifest(&app, manifest).await; + let run_id = created["id"] + .as_str() + .expect("create response should contain an id") + .parse::() + .expect("create response id should be valid"); + + let run_store = state.stores.runs.open_run_reader(&run_id).await.unwrap(); + let run_state = run_store.state().await.unwrap(); + assert_eq!(run_state.spec.graph.name, "PickedFlow"); + assert_eq!( + run_state.spec.graph.goal(), + "Goal text from the legacy body" + ); +} + #[tokio::test] async fn post_runs_create_regression_keeps_api_behavior_without_automation_metadata() { let state = TestAppStateBuilder::new() @@ -4225,7 +4278,6 @@ async fn validate_endpoint_returns_template_source_coordinates() { "version": 1, "cwd": "/tmp", "target": { - "identifier": "workflow.fabro", "path": "workflow.fabro", }, "workflows": { @@ -11169,7 +11221,6 @@ async fn create_run_keeps_missing_project_and_workflow_names_absent() { "version": 1, "cwd": "/tmp/project", "target": { - "identifier": "workflow.fabro", "path": "workflow.fabro", }, "configs": [ @@ -13366,7 +13417,6 @@ async fn get_graph_returns_svg() { "version": 1, "cwd": "/tmp", "target": { - "identifier": "workflow.fabro", "path": "workflow.fabro", }, "workflows": { @@ -13426,7 +13476,6 @@ async fn get_graph_source_returns_dot() { "version": 1, "cwd": "/tmp", "target": { - "identifier": "workflow.fabro", "path": "workflow.fabro", }, "workflows": { @@ -13480,7 +13529,6 @@ async fn render_graph_from_manifest_returns_svg() { "version": 1, "cwd": "/tmp", "target": { - "identifier": "workflow.fabro", "path": "workflow.fabro", }, "workflows": { @@ -13538,7 +13586,6 @@ async fn render_graph_from_manifest_accepts_fabro_dotted_attributes() { "version": 1, "cwd": "/tmp", "target": { - "identifier": "workflow.fabro", "path": "workflow.fabro", }, "workflows": { diff --git a/lib/apps/fabro-server/tests/it/helpers.rs b/lib/apps/fabro-server/tests/it/helpers.rs index 9ecf30bb8..ba093b2f2 100644 --- a/lib/apps/fabro-server/tests/it/helpers.rs +++ b/lib/apps/fabro-server/tests/it/helpers.rs @@ -250,7 +250,6 @@ pub(crate) fn minimal_manifest_json(dot_source: &str) -> serde_json::Value { "version": 1, "cwd": "/tmp", "target": { - "identifier": "workflow.fabro", "path": "workflow.fabro" }, "workflows": { diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index 0cc8a695a..29b32aadb 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -25,5 +25,6 @@ git2.workspace = true toml.workspace = true [dev-dependencies] +serde_json.workspace = true tempfile = "3" temp-env = "0.3" diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index 656de96eb..3bfb92c5c 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -252,10 +252,7 @@ pub fn build_run_manifest(input: ManifestBuildInput) -> Result { goal, parent_id: None, title: None, - target: types::ManifestTarget { - identifier: input.workflow.display().to_string(), - path: target_key, - }, + target: types::ManifestTarget { path: target_key }, version: 1, workflows: context.workflows, }, @@ -709,7 +706,6 @@ fn resolve_manifest_goal( ) .ok_or_else(|| anyhow!("unsupported manifest goal reference: {reference}"))?; return Ok(Some(types::ManifestGoal { - path: Some(reference.to_string()), text: std::fs::read_to_string(&goal_path) .with_context(|| format!("Failed to read {}", goal_path.display()))?, type_: types::ManifestGoalType::Graph, @@ -717,24 +713,21 @@ fn resolve_manifest_goal( } Ok(Some(types::ManifestGoal { - path: None, text: goal.to_string(), type_: types::ManifestGoalType::Graph, })) } /// Translate a [`ResolvedRunGoal`] into the wire-level `ManifestGoal` -/// shape. Inline goals get `type = Value`; file-sourced goals keep their -/// absolute path as the `path` field and use `type = File`. +/// shape. Inline goals get `type = Value`; file-sourced goals carry their +/// already-resolved contents with `type = File`. fn resolved_goal_to_manifest(resolved: ResolvedRunGoal) -> types::ManifestGoal { match resolved.source { ResolvedGoalSource::Inline => types::ManifestGoal { - path: None, text: resolved.text, type_: types::ManifestGoalType::Value, }, - ResolvedGoalSource::File { path } => types::ManifestGoal { - path: Some(path.to_string_lossy().into_owned()), + ResolvedGoalSource::File { .. } => types::ManifestGoal { text: resolved.text, type_: types::ManifestGoalType::File, }, @@ -1118,8 +1111,8 @@ mod tests { .unwrap(); assert_eq!( - built.manifest.target.path, - ".fabro/workflows/demo/workflow.fabro" + serde_json::to_value(&built.manifest.target).unwrap(), + serde_json::json!({ "path": ".fabro/workflows/demo/workflow.fabro" }) ); assert_eq!(built.manifest.workflows.len(), 2); let root = &built.manifest.workflows[".fabro/workflows/demo/workflow.fabro"]; @@ -1139,7 +1132,10 @@ mod tests { root.files .contains_key(".fabro/workflows/demo/prompts/lint.md") ); - assert_eq!(built.manifest.goal.unwrap().text, "ship it"); + assert_eq!( + serde_json::to_value(built.manifest.goal.as_ref().unwrap()).unwrap(), + serde_json::json!({ "type": "graph", "text": "ship it" }) + ); assert!( built .manifest @@ -1573,11 +1569,10 @@ file = "prompts/goal.md" .unwrap(); let goal = built.manifest.goal.expect("manifest goal should be set"); - assert_eq!(goal.text, "ship from project root"); - assert_eq!(goal.type_, types::ManifestGoalType::File); - let resolved = goal.path.expect("file goal must carry a path"); - let expected = project.join(".fabro").join("prompts").join("goal.md"); - assert_eq!(PathBuf::from(resolved), expected); + assert_eq!( + serde_json::to_value(&goal).unwrap(), + serde_json::json!({ "type": "file", "text": "ship from project root" }) + ); } /// A relative `[run.goal] file = "..."` declared in `workflow.toml` @@ -1623,11 +1618,35 @@ file = "prompts/goal.md" .unwrap(); let goal = built.manifest.goal.expect("manifest goal should be set"); - assert_eq!(goal.text, "ship from workflow dir"); - assert_eq!(goal.type_, types::ManifestGoalType::File); - let resolved = goal.path.expect("file goal must carry a path"); - let expected = workflow_dir.join("prompts").join("goal.md"); - assert_eq!(PathBuf::from(resolved), expected); + assert_eq!( + serde_json::to_value(&goal).unwrap(), + serde_json::json!({ "type": "file", "text": "ship from workflow dir" }) + ); + } + + /// The wire-level goal carries only the resolved kind and content: + /// inline and file-sourced goals serialize to exactly `type` + `text`. + #[test] + fn resolved_goals_serialize_type_and_text_only() { + let inline = resolved_goal_to_manifest(ResolvedRunGoal { + text: "inline goal".to_string(), + source: ResolvedGoalSource::Inline, + }); + assert_eq!( + serde_json::to_value(&inline).unwrap(), + serde_json::json!({ "type": "value", "text": "inline goal" }) + ); + + let file = resolved_goal_to_manifest(ResolvedRunGoal { + text: "goal from file".to_string(), + source: ResolvedGoalSource::File { + path: PathBuf::from("/tmp/project/goal.md"), + }, + }); + assert_eq!( + serde_json::to_value(&file).unwrap(), + serde_json::json!({ "type": "file", "text": "goal from file" }) + ); } /// When `[run] working_dir` points to a nested git repo, the manifest's diff --git a/lib/packages/fabro-api-client/src/models/manifest-goal.ts b/lib/packages/fabro-api-client/src/models/manifest-goal.ts index 71d0f5cfb..377b6e3ac 100644 --- a/lib/packages/fabro-api-client/src/models/manifest-goal.ts +++ b/lib/packages/fabro-api-client/src/models/manifest-goal.ts @@ -15,7 +15,7 @@ /** - * Resolved goal with provenance. + * Resolved goal kind and content. */ export interface ManifestGoal { 'type': ManifestGoalTypeEnum; @@ -23,10 +23,6 @@ export interface ManifestGoal { * Resolved goal content. */ 'text': string; - /** - * Original goal file path when the goal came from a file. - */ - 'path'?: string | null; } export const ManifestGoalTypeEnum = { diff --git a/lib/packages/fabro-api-client/src/models/manifest-target.ts b/lib/packages/fabro-api-client/src/models/manifest-target.ts index 44a463a09..c78249f32 100644 --- a/lib/packages/fabro-api-client/src/models/manifest-target.ts +++ b/lib/packages/fabro-api-client/src/models/manifest-target.ts @@ -15,10 +15,6 @@ export interface ManifestTarget { - /** - * What the user typed. - */ - 'identifier': string; /** * Resolved path that keys into the workflows map. */