From 0864375bcd322edfb22496e5922a0411f8fd7fc7 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 17:06:17 -0400 Subject: [PATCH 01/11] Skip providers with no small model when picking a small default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `small_default_for_provider` fell back to the provider's normal default when no model was marked `small_default`. That turned "give me the small utility model" into "give me the flagship" for any provider without one. Run title generation asks for the small default, then gives it a 64-token budget and a 10s timeout. On a server where kimi is the highest-priority configured provider, that resolved to kimi-k3 — an always-reasoning model that burned 161 reasoning tokens before emitting anything. The structured output never completed, `generate_object` returned NoObjectGenerated, and the caller silently kept the deterministic title. Return `None` instead, and have `small_default_for_configured_ids` move on to the next configured provider. The ordinary default is used only when no configured provider marks a small model, and it now comes from the configured set rather than the global catalog default. Co-Authored-By: Claude Opus 5 (1M context) --- lib/foundation/fabro-model/src/catalog.rs | 94 +++++++++++++++++++---- 1 file changed, 77 insertions(+), 17 deletions(-) diff --git a/lib/foundation/fabro-model/src/catalog.rs b/lib/foundation/fabro-model/src/catalog.rs index f012a62ff..8d89972ed 100644 --- a/lib/foundation/fabro-model/src/catalog.rs +++ b/lib/foundation/fabro-model/src/catalog.rs @@ -1316,15 +1316,17 @@ impl Catalog { } /// Small default model for a provider — the small/cheap utility model used - /// for metadata enrichment. Falls back to the provider's normal default - /// when no explicit small default is configured. + /// for metadata enrichment. `None` when the provider marks no small + /// default. Deliberately does not substitute the provider's normal + /// default, which is typically a large reasoning model: callers asking for + /// a small model give it a small token budget and a short timeout, and a + /// flagship model silently exceeds both. #[must_use] pub fn small_default_for_provider(&self, p: &ProviderId) -> Option<&Model> { let provider_id = self.provider(p).map_or(p, |provider| &provider.id); self.models .iter() .find(|m| &m.provider == provider_id && m.small_default) - .or_else(|| self.default_for_provider(provider_id)) } /// Default model for the best-available provider (based on API keys), @@ -1368,22 +1370,25 @@ impl Catalog { .unwrap_or_else(|| self.default_model()) } - /// Small default model for the best-available built-in provider IDs, - /// falling back to the global catalog default. + /// Small default model for the best-available built-in provider IDs. + /// + /// Configured providers that mark no small default are skipped in favour + /// of a lower-priority provider that has one. Only when none of them does + /// is the ordinary default used. #[must_use] pub fn small_default_for_configured_ids(&self, configured: &[ProviderId]) -> &Model { if configured.is_empty() { return self.default_model(); } - let configured = configured + let resolved = configured .iter() .filter_map(|id| self.provider(id).map(|provider| provider.id.clone())) .collect::>(); self.providers .iter() - .filter(|provider| configured.contains(&provider.id)) + .filter(|provider| resolved.contains(&provider.id)) .find_map(|provider| self.small_default_for_provider(&provider.id)) - .unwrap_or_else(|| self.default_model()) + .unwrap_or_else(|| self.default_for_configured_ids(configured)) } /// Probe model for a provider — the cheapest model suitable for @@ -5064,7 +5069,7 @@ reasoning = false } #[test] - fn small_default_for_provider_falls_back_to_provider_default_when_no_small_default_marked() { + fn small_default_for_provider_returns_none_when_no_small_default_marked() { let layer = minimal_settings( r#" [providers.test] @@ -5102,12 +5107,10 @@ reasoning = false ); let catalog = Catalog::from_settings(&layer).unwrap(); - assert_eq!( + assert!( catalog .small_default_for_provider(&ProviderId::new("test")) - .unwrap() - .id, - "default_model" + .is_none() ); } @@ -5257,6 +5260,66 @@ reasoning = false ); } + #[test] + fn small_default_for_configured_ids_skips_provider_without_a_small_model() { + let layer = minimal_settings( + r#" +[providers.low] +display_name = "Low" +adapter = "openai" +agent_profile = "openai" +priority = 10 + +[providers.high] +display_name = "High" +adapter = "openai" +agent_profile = "openai" +priority = 20 + +[models.low_small] +provider = "low" +display_name = "Low Small" +family = "test" +small_default = true + +[models.low_small.limits] +context_window = 1000 + +[models.low_small.features] +tools = false +vision = false +reasoning = false + +[models.high_default] +provider = "high" +display_name = "High Default" +family = "test" +default = true + +[models.high_default.limits] +context_window = 1000 + +[models.high_default.features] +tools = false +vision = false +reasoning = false +"#, + ); + let catalog = Catalog::from_settings(&layer).unwrap(); + + // `high` outranks `low` but marks no small default, so selection moves + // on rather than substituting `high_default`. + assert_eq!( + catalog + .small_default_for_configured_ids(&[ + ProviderId::new("low"), + ProviderId::new("high") + ]) + .id, + "low_small" + ); + } + #[test] fn small_default_for_configured_ids_falls_back_to_provider_default() { let layer = minimal_settings( @@ -5365,10 +5428,7 @@ small_default = false .expect("sparse built-in model override should build"); let openai = ProviderId::openai(); - assert_eq!( - catalog.small_default_for_provider(&openai).unwrap().id, - catalog.default_for_provider(&openai).unwrap().id - ); + assert!(catalog.small_default_for_provider(&openai).is_none()); } #[test] From 88ca4b1d0310a5b9b3ff25732a914ba5f5d1a48a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 17:11:52 -0400 Subject: [PATCH 02/11] Teach the run title model the title shape we want Move the run title prompt into `src/prompts/run_title.md` and load it with `include_str!`, matching the playground and ask-fabro prompts. Placeholder substitution replaces `format!`, so the literal `{"title":"..."}` in the prompt no longer needs brace escaping. The old instructions only said "concise" and "preserve ticket IDs", so a work order run titled itself with the raw file path. The prompt now asks for a pull-request-shaped title: leading verb, identifier in canonical uppercase, then a description with paths, date prefixes, and extensions stripped and slug hyphens turned back into words. Three worked examples carry the shape. Checked against claude-haiku-4-5 at the existing 64-token budget: Implement Conveyor Work Order docs/planning/orders/2026-07-22-wrk-004-operational-diagnostics.md -> Implement WRK-004: Operational diagnostics Fix flaky checkout test (input branch release-9.2) -> Fix flaky checkout test on release-9.2 Co-Authored-By: Claude Opus 5 (1M context) --- .../fabro-server/src/prompts/run_title.md | 45 +++++++++++ .../fabro-server/src/run_title_generation.rs | 79 ++++++++++++------- 2 files changed, 97 insertions(+), 27 deletions(-) create mode 100644 lib/apps/fabro-server/src/prompts/run_title.md diff --git a/lib/apps/fabro-server/src/prompts/run_title.md b/lib/apps/fabro-server/src/prompts/run_title.md new file mode 100644 index 000000000..01ac24a24 --- /dev/null +++ b/lib/apps/fabro-server/src/prompts/run_title.md @@ -0,0 +1,45 @@ +Generate a concise, human-readable title for this Fabro workflow run. + +Base the title on the workflow identity, workflow goal, and run input values. +Write it the way a person would title a pull request: + +- Start with the verb for the work: Implement, Fix, Migrate, Review, Add. When + the goal opens with Markdown heading marks or a document-type prefix such as + "Plan:", "Goal:", or "Spec:", that prefix names the document rather than the + work — drop it and start from the verb that follows. +- If the run names an identifier — work order, ticket, issue, PR number — put it + next in canonical uppercase, then a colon, then a short description: + "Implement WRK-044: Separate routine and nightly verification". +- Take that description from the most specific human-readable text available. + Drop directory paths, date prefixes, and file extensions; turn slug hyphens + and underscores into spaces; write the result in sentence case. +- Keep a repository, branch, or environment only when it is what distinguishes + this run from an otherwise identical one. +- Do not end with a period. + +Examples: + +- goal "Implement Delivery Work Order docs/planning/orders/2026-03-04-wrk-018-retry-backoff.md" + -> "Implement WRK-018: Retry backoff" +- goal "## Plan: migrate the billing service to Postgres 17" + -> "Migrate billing service to Postgres 17" +- goal "Fix flaky checkout test" with input branch "release-9.2" + -> "Fix flaky checkout test on release-9.2" + +Return only structured JSON with one field: {"title":"..."}. +The title must be a single line, not blank, and no more than {max_chars} characters. + +Workflow identity: +```json +{workflow_identity} +``` + +Run inputs (raw values, not redacted): +```json +{run_inputs} +``` + +Workflow summary: +```json +{workflow_summary} +``` diff --git a/lib/apps/fabro-server/src/run_title_generation.rs b/lib/apps/fabro-server/src/run_title_generation.rs index 7b7f9ad33..b0ad2ab45 100644 --- a/lib/apps/fabro-server/src/run_title_generation.rs +++ b/lib/apps/fabro-server/src/run_title_generation.rs @@ -12,6 +12,12 @@ use toml::Value as TomlValue; const TRUNCATED_MARKER: &str = "...[truncated]"; const MAX_PROMPT_SECTION_CHARS: usize = 4_000; +const TITLE_PROMPT_TEMPLATE: &str = include_str!("prompts/run_title.md"); +const MAX_CHARS_PLACEHOLDER: &str = "{max_chars}"; +const IDENTITY_PLACEHOLDER: &str = "{workflow_identity}"; +const INPUTS_PLACEHOLDER: &str = "{run_inputs}"; +const WORKFLOW_PLACEHOLDER: &str = "{workflow_summary}"; + pub(crate) struct TitlePromptInput<'a> { pub(crate) run_id: &'a RunId, pub(crate) current_title: &'a str, @@ -68,33 +74,20 @@ fn build_title_prompt(input: &TitlePromptInput<'_>) -> String { let inputs = pretty_json(input.run_inputs); let workflow = pretty_json(input.workflow); - format!( - r#"Generate a concise, human-readable title for this Fabro workflow run. - -Base the title on the workflow identity, workflow goal, and run input values. -Preserve meaningful proper nouns, ticket IDs, repositories, branches, environments, and explicit user goals. -Return only structured JSON with one field: {{"title":"..."}}. -The title must be a single line, not blank, and no more than {MAX_RUN_TITLE_CHARS} characters. - -Workflow identity: -```json -{} -``` - -Run inputs (raw values, not redacted): -```json -{} -``` - -Workflow summary: -```json -{} -``` -"#, - truncate_section(&identity, MAX_PROMPT_SECTION_CHARS), - truncate_section(&inputs, MAX_PROMPT_SECTION_CHARS), - truncate_section(&workflow, MAX_PROMPT_SECTION_CHARS), - ) + TITLE_PROMPT_TEMPLATE + .replace(MAX_CHARS_PLACEHOLDER, &MAX_RUN_TITLE_CHARS.to_string()) + .replace( + IDENTITY_PLACEHOLDER, + &truncate_section(&identity, MAX_PROMPT_SECTION_CHARS), + ) + .replace( + INPUTS_PLACEHOLDER, + &truncate_section(&inputs, MAX_PROMPT_SECTION_CHARS), + ) + .replace( + WORKFLOW_PLACEHOLDER, + &truncate_section(&workflow, MAX_PROMPT_SECTION_CHARS), + ) } fn normalize_generated_title(title: &str) -> Option { @@ -203,6 +196,38 @@ mod tests { .unwrap() } + #[test] + fn prompt_template_placeholders_are_present_once_and_substituted_away() { + for placeholder in [ + MAX_CHARS_PLACEHOLDER, + IDENTITY_PLACEHOLDER, + INPUTS_PLACEHOLDER, + WORKFLOW_PLACEHOLDER, + ] { + assert_eq!( + TITLE_PROMPT_TEMPLATE.matches(placeholder).count(), + 1, + "run_title.md must contain {placeholder} exactly once" + ); + } + + let run_id = RunId::new(); + let graph = title_test_graph(); + let summary = workflow_summary(&graph); + let prompt = build_title_prompt(&TitlePromptInput { + run_id: &run_id, + current_title: "Current", + workflow_target: Some("workflow.fabro"), + run_inputs: &HashMap::new(), + workflow: &summary, + }); + + assert!(!prompt.contains(IDENTITY_PLACEHOLDER)); + assert!(!prompt.contains(INPUTS_PLACEHOLDER)); + assert!(!prompt.contains(WORKFLOW_PLACEHOLDER)); + assert!(prompt.contains(&format!("no more than {MAX_RUN_TITLE_CHARS} characters"))); + } + #[test] fn prompt_includes_goal_inputs_and_workflow_summary_without_redaction() { let run_id = RunId::new(); From 713d340db73f0f13e48e5aff9bc1ee8458f6dcdc Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 17:23:16 -0400 Subject: [PATCH 03/11] Render the run title prompt with Jinja instead of string replacement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prompt used hand-rolled `{placeholder}` substitution via `str::replace`. The app already has a MiniJinja layer for exactly this, and every other checked-in prompt uses it, so use it here too. `prompts/run_title.md` becomes `prompts/run_title.md.j2` with `{{ inputs.* }}` variables, rendered through `fabro_template::render_named`. Strict undefined handling now catches a variable the template asks for and the caller does not supply, which the old `.replace()` chain silently left as literal text. `build_title_prompt` returns `Result` accordingly. A checked-in template that will not render is a bug rather than a transient failure, so the caller logs it at `warn` — louder than the `debug` used for a generation miss — and keeps the deterministic title. Re-checked against claude-haiku-4-5: same titles as before the change. Co-Authored-By: Claude Opus 5 (1M context) --- lib/apps/fabro-server/Cargo.toml | 1 + .../prompts/{run_title.md => run_title.md.j2} | 8 +- .../fabro-server/src/run_title_generation.rs | 87 ++++++++++--------- 3 files changed, 51 insertions(+), 45 deletions(-) rename lib/apps/fabro-server/src/prompts/{run_title.md => run_title.md.j2} (90%) diff --git a/lib/apps/fabro-server/Cargo.toml b/lib/apps/fabro-server/Cargo.toml index 1f2c5bb61..12fd294d7 100644 --- a/lib/apps/fabro-server/Cargo.toml +++ b/lib/apps/fabro-server/Cargo.toml @@ -41,6 +41,7 @@ fabro-manifest = { path = "../../components/fabro-manifest" } fabro-mcp-store = { path = "../../components/fabro-mcp-store" } fabro-model = { path = "../../foundation/fabro-model" } fabro-proc = { path = "../../foundation/fabro-proc" } +fabro-template = { path = "../../foundation/fabro-template" } fabro-tool = { path = "../../components/fabro-tool" } fabro-types = { path = "../../foundation/fabro-types" } fabro-util = { path = "../../foundation/fabro-util" } diff --git a/lib/apps/fabro-server/src/prompts/run_title.md b/lib/apps/fabro-server/src/prompts/run_title.md.j2 similarity index 90% rename from lib/apps/fabro-server/src/prompts/run_title.md rename to lib/apps/fabro-server/src/prompts/run_title.md.j2 index 01ac24a24..eb1237bbd 100644 --- a/lib/apps/fabro-server/src/prompts/run_title.md +++ b/lib/apps/fabro-server/src/prompts/run_title.md.j2 @@ -27,19 +27,19 @@ Examples: -> "Fix flaky checkout test on release-9.2" Return only structured JSON with one field: {"title":"..."}. -The title must be a single line, not blank, and no more than {max_chars} characters. +The title must be a single line, not blank, and no more than {{ inputs.max_chars }} characters. Workflow identity: ```json -{workflow_identity} +{{ inputs.workflow_identity }} ``` Run inputs (raw values, not redacted): ```json -{run_inputs} +{{ inputs.run_inputs }} ``` Workflow summary: ```json -{workflow_summary} +{{ inputs.workflow_summary }} ``` diff --git a/lib/apps/fabro-server/src/run_title_generation.rs b/lib/apps/fabro-server/src/run_title_generation.rs index b0ad2ab45..14abce84c 100644 --- a/lib/apps/fabro-server/src/run_title_generation.rs +++ b/lib/apps/fabro-server/src/run_title_generation.rs @@ -5,6 +5,7 @@ use fabro_llm::client::Client; use fabro_llm::generate::{self, GenerateParams}; use fabro_llm::types::TimeoutOptions; use fabro_model::ProviderId; +use fabro_template::{TemplateContext, TemplateError}; use fabro_types::{Graph, MAX_RUN_TITLE_CHARS, RunId}; use serde::Serialize; use toml::Value as TomlValue; @@ -12,11 +13,8 @@ use toml::Value as TomlValue; const TRUNCATED_MARKER: &str = "...[truncated]"; const MAX_PROMPT_SECTION_CHARS: usize = 4_000; -const TITLE_PROMPT_TEMPLATE: &str = include_str!("prompts/run_title.md"); -const MAX_CHARS_PLACEHOLDER: &str = "{max_chars}"; -const IDENTITY_PLACEHOLDER: &str = "{workflow_identity}"; -const INPUTS_PLACEHOLDER: &str = "{run_inputs}"; -const WORKFLOW_PLACEHOLDER: &str = "{workflow_summary}"; +const TITLE_PROMPT_NAME: &str = "run_title.md.j2"; +const TITLE_PROMPT_TEMPLATE: &str = include_str!("prompts/run_title.md.j2"); pub(crate) struct TitlePromptInput<'a> { pub(crate) run_id: &'a RunId, @@ -35,7 +33,15 @@ pub(crate) struct GenerateTitleInput<'a> { pub(crate) async fn generate_title_or_current(input: GenerateTitleInput<'_>) -> String { let current_title = input.prompt.current_title.to_string(); - let prompt = build_title_prompt(&input.prompt); + let prompt = match build_title_prompt(&input.prompt) { + Ok(prompt) => prompt, + Err(err) => { + // A checked-in template that will not render is a bug, not a + // transient failure, so this is louder than a generation miss. + tracing::warn!(error = %err, "Run title prompt template failed to render"); + return current_title; + } + }; let params = GenerateParams::new(input.model_id, input.client) .provider(input.provider_id.to_string()) .prompt(prompt) @@ -62,7 +68,7 @@ pub(crate) async fn generate_title_or_current(input: GenerateTitleInput<'_>) -> .unwrap_or(current_title) } -fn build_title_prompt(input: &TitlePromptInput<'_>) -> String { +fn build_title_prompt(input: &TitlePromptInput<'_>) -> Result { let workflow_identity = serde_json::json!({ "run_id": input.run_id.to_string(), "current_deterministic_title": input.current_title, @@ -74,20 +80,26 @@ fn build_title_prompt(input: &TitlePromptInput<'_>) -> String { let inputs = pretty_json(input.run_inputs); let workflow = pretty_json(input.workflow); - TITLE_PROMPT_TEMPLATE - .replace(MAX_CHARS_PLACEHOLDER, &MAX_RUN_TITLE_CHARS.to_string()) - .replace( - IDENTITY_PLACEHOLDER, - &truncate_section(&identity, MAX_PROMPT_SECTION_CHARS), - ) - .replace( - INPUTS_PLACEHOLDER, - &truncate_section(&inputs, MAX_PROMPT_SECTION_CHARS), - ) - .replace( - WORKFLOW_PLACEHOLDER, - &truncate_section(&workflow, MAX_PROMPT_SECTION_CHARS), - ) + let template_inputs = HashMap::from([ + ( + "max_chars".to_string(), + TomlValue::String(MAX_RUN_TITLE_CHARS.to_string()), + ), + ( + "workflow_identity".to_string(), + TomlValue::String(truncate_section(&identity, MAX_PROMPT_SECTION_CHARS)), + ), + ( + "run_inputs".to_string(), + TomlValue::String(truncate_section(&inputs, MAX_PROMPT_SECTION_CHARS)), + ), + ( + "workflow_summary".to_string(), + TomlValue::String(truncate_section(&workflow, MAX_PROMPT_SECTION_CHARS)), + ), + ]); + let ctx = TemplateContext::new().with_inputs(template_inputs); + fabro_template::render_named(TITLE_PROMPT_NAME, TITLE_PROMPT_TEMPLATE, &ctx) } fn normalize_generated_title(title: &str) -> Option { @@ -196,21 +208,11 @@ mod tests { .unwrap() } + /// Strict rendering already fails on a variable the template asks for and + /// the caller does not supply. This covers the other direction: a variable + /// dropped from the template renders fine but silently starves the model. #[test] - fn prompt_template_placeholders_are_present_once_and_substituted_away() { - for placeholder in [ - MAX_CHARS_PLACEHOLDER, - IDENTITY_PLACEHOLDER, - INPUTS_PLACEHOLDER, - WORKFLOW_PLACEHOLDER, - ] { - assert_eq!( - TITLE_PROMPT_TEMPLATE.matches(placeholder).count(), - 1, - "run_title.md must contain {placeholder} exactly once" - ); - } - + fn prompt_template_renders_every_variable_the_caller_supplies() { let run_id = RunId::new(); let graph = title_test_graph(); let summary = workflow_summary(&graph); @@ -220,12 +222,13 @@ mod tests { workflow_target: Some("workflow.fabro"), run_inputs: &HashMap::new(), workflow: &summary, - }); + }) + .unwrap(); - assert!(!prompt.contains(IDENTITY_PLACEHOLDER)); - assert!(!prompt.contains(INPUTS_PLACEHOLDER)); - assert!(!prompt.contains(WORKFLOW_PLACEHOLDER)); assert!(prompt.contains(&format!("no more than {MAX_RUN_TITLE_CHARS} characters"))); + assert!(prompt.contains(&run_id.to_string())); + assert!(prompt.contains("\"stage_count\": 4")); + assert!(!prompt.contains("{{")); } #[test] @@ -250,7 +253,8 @@ mod tests { workflow_target: Some("workflows/deploy.fabro"), run_inputs: &inputs, workflow: &summary, - }); + }) + .unwrap(); assert!(prompt.contains("Deploy API token SECRET_123 to production")); assert!(prompt.contains("\"api_key\": \"SECRET_123\"")); @@ -276,7 +280,8 @@ mod tests { workflow_target: Some("workflow.fabro"), run_inputs: &inputs, workflow: &summary, - }); + }) + .unwrap(); // Section truncation: per-section budget × 3 + small boilerplate. assert!(prompt.chars().count() < MAX_PROMPT_SECTION_CHARS * 3 + 1_000); From 24b0576ffe2247ba53ce8e57d710ea2e6833f378 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 17:23:26 -0400 Subject: [PATCH 04/11] Record fabro-template as a fabro-server dependency in Cargo.lock Missed in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index e71612fb2..a2acca97e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3028,6 +3028,7 @@ dependencies = [ "fabro-spa", "fabro-static", "fabro-store", + "fabro-template", "fabro-test", "fabro-tool", "fabro-types", From 7950441beeadee1a359ee82170b628647ff05cd2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 18:26:35 -0400 Subject: [PATCH 05/11] Give all_spec_routes_are_routable a 30s timeout that actually applies The test has had a dedicated override since it was first flagged as slow, but it sat below the package-wide `package(fabro-server)` entry. Nextest resolves each setting from the first matching override, so the broader filter won and the narrower one was dead config. The effective timeout was therefore the package default, 5s x 4 = 20s. The test runs 15-19s and tripped that under full-workspace load. Move the override above the package entry and set 10s x 3, so it is both reachable and a 30s kill. Confirmed by the SLOW marker moving from >5s to >10s. Co-Authored-By: Claude Opus 5 (1M context) --- .config/nextest.toml | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/.config/nextest.toml b/.config/nextest.toml index fe7f4f778..7e27d4f8f 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -7,14 +7,17 @@ leak-timeout = "500ms" filter = "package(fabro-cli)" slow-timeout = { period = "6s", terminate-after = 4 } + # Must stay above the package-wide fabro-server override: nextest resolves + # each setting from the first matching override, so a narrower filter + # placed after a broader one never applies. + [[profile.default.overrides]] + filter = "package(fabro-server) & test(all_spec_routes_are_routable)" + slow-timeout = { period = "10s", terminate-after = 3 } + [[profile.default.overrides]] filter = "package(fabro-server)" slow-timeout = { period = "5s", terminate-after = 4 } - [[profile.default.overrides]] - filter = "package(fabro-server) & test(all_spec_routes_are_routable)" - slow-timeout = { period = "15s", terminate-after = 4 } - [[profile.default.overrides]] filter = "package(fabro-workflow)" slow-timeout = { period = "2s", terminate-after = 3 } From 99dd7718c01e5a02c1665924ec01b4d24d0b88d0 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 18:39:25 -0400 Subject: [PATCH 06/11] Keep server tests off the developer's real ~/.fabro/storage Test settings usually omit `[server.storage] root`, so it resolved to the production default. Handlers that walk that tree read whatever the machine happened to have. That is why all_spec_routes_are_routable was slow. Timing every request in it showed 91% of the runtime in two routes: 6304ms GET /api/v1/system/resources 4574ms GET /api/v1/system/df 583ms POST /api/v1/system/prune/runs ... the remaining 134 operations: 8ms combined Both size Fabro-managed storage. On this machine that meant 193MB and 90,795 entries under scratch/, so the test's duration tracked how long the developer had been running Fabro locally. Run-creating tests were writing there too. Redirect settings that still carry the production default to a `storage` directory beside the test vault, alongside the existing `server.env` and `settings.toml` siblings. A test that chose its own root keeps it. all_spec_routes_are_routable drops from ~15s to 0.6s, and the full workspace run from ~33s to ~21s. All 7402 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- lib/apps/fabro-server/src/test_support.rs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/lib/apps/fabro-server/src/test_support.rs b/lib/apps/fabro-server/src/test_support.rs index 8fe368fff..178641298 100644 --- a/lib/apps/fabro-server/src/test_support.rs +++ b/lib/apps/fabro-server/src/test_support.rs @@ -13,6 +13,7 @@ use axum::middleware::Next; use axum::response::Response; use axum::{Router, middleware}; use chrono::Duration as ChronoDuration; +use fabro_config::user::default_storage_dir; use fabro_config::{RunLayer, ServerSettingsBuilder, Storage, envfile}; use fabro_db::DbPool; use fabro_interview::Interviewer; @@ -253,9 +254,10 @@ impl TestAppStateBuilder { self.try_build().expect("test app state should build") } - pub fn try_build(self) -> anyhow::Result> { + pub fn try_build(mut self) -> anyhow::Result> { let (store, artifact_store) = self.store_bundle.unwrap_or_else(test_store_bundle); let vault_path = self.vault_path.unwrap_or_else(test_secret_store_path); + redirect_default_storage_root(&mut self.server_settings, &vault_path); if !self.vault_entries.is_empty() { let mut vault = Vault::load(vault_path.clone()).expect("test vault should load"); for (name, value) in &self.vault_entries { @@ -672,6 +674,25 @@ pub fn test_secret_store_path() -> PathBuf { dir.join("secrets.json") } +/// Keeps tests off the developer's real `~/.fabro/storage`. +/// +/// Settings built for tests usually omit `[server.storage] root`, which +/// resolves to the production default. Handlers that walk that tree — `df`, +/// `system/resources`, `prune` — then read whatever runs and scratch +/// directories the machine happens to have, making tests slow and +/// machine-dependent, and letting run-creating tests write there. +/// +/// Only settings still carrying the production default are redirected; a test +/// that chose its own root keeps it. +fn redirect_default_storage_root(settings: &mut ServerSettings, vault_path: &Path) { + if Path::new(&settings.server.storage.root) != default_storage_dir() { + return; + } + let root = vault_path.with_file_name("storage"); + std::fs::create_dir_all(&root).expect("test storage root should be creatable"); + settings.server.storage.root = root.display().to_string(); +} + #[must_use] pub fn test_auth_mode() -> AuthMode { AuthMode::Enabled(ConfiguredAuth { From a6282d0773ad85d8d4533b1d0378c8dcb8a3186d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 18:58:37 -0400 Subject: [PATCH 07/11] Revert "Give all_spec_routes_are_routable a 30s timeout that actually applies" This reverts commit 7950441beeadee1a359ee82170b628647ff05cd2. --- .config/nextest.toml | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/.config/nextest.toml b/.config/nextest.toml index 7e27d4f8f..fe7f4f778 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -7,17 +7,14 @@ leak-timeout = "500ms" filter = "package(fabro-cli)" slow-timeout = { period = "6s", terminate-after = 4 } - # Must stay above the package-wide fabro-server override: nextest resolves - # each setting from the first matching override, so a narrower filter - # placed after a broader one never applies. - [[profile.default.overrides]] - filter = "package(fabro-server) & test(all_spec_routes_are_routable)" - slow-timeout = { period = "10s", terminate-after = 3 } - [[profile.default.overrides]] filter = "package(fabro-server)" slow-timeout = { period = "5s", terminate-after = 4 } + [[profile.default.overrides]] + filter = "package(fabro-server) & test(all_spec_routes_are_routable)" + slow-timeout = { period = "15s", terminate-after = 4 } + [[profile.default.overrides]] filter = "package(fabro-workflow)" slow-timeout = { period = "2s", terminate-after = 3 } From 6bcd73028434dd9918dedeb2132a219a70a7a08f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 19:56:50 -0400 Subject: [PATCH 08/11] feat(workflow): interpolate goal, inputs, and vars in command scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Command node `script` attributes were literal text: a `{{ inputs.x }}` reached bash verbatim, and the only signal was a `detemplated_attribute` warning. Scripts now substitute `{{ goal }}`, `{{ inputs.NAME }}`, and `{{ vars.NAME }}` at run creation, alongside goals and prompts. Scripts use `InterpString` token substitution rather than the MiniJinja pass that renders prompts. Shell source is full of brace syntax that must survive untouched — jq filters, awk programs, Go templates, brace expansion — and `InterpString` claims only the narrow token forms, leaving everything else literal. `env` and `secrets` are deliberately not wired and now fail loudly instead of passing through as text. A script reads the environment with `$NAME`, which needs no interpolation, and a resolved secret would be baked into the `CommandStarted` event that records the script verbatim. The error points at `[environments..env]` for the secret case. `ResolveCtx` gains opt-in `with_inputs` and `with_goal`. Namespace availability stays scope-determined per call site, so every existing config-layer context leaves both unwired and keeps its current behavior. `goal` names a single value rather than a namespace of them, so it has no dotted form: only the exact body `goal` produces a token and `{{ goal.title }}` stays literal. Values substitute verbatim without shell quoting, matching `[[run.prepare.steps]].script` where the snippet is the author's to quote. Substituted text is never rescanned. Co-Authored-By: Claude Opus 5 (1M context) --- docs/public/workflows/stages-and-nodes.mdx | 2 +- docs/public/workflows/variables.mdx | 57 ++- .../fabro-workflow/src/operations/create.rs | 52 +++ .../src/transforms/variable_expansion.rs | 335 +++++++++++++++++- lib/foundation/fabro-template/src/lib.rs | 68 ++++ .../fabro-types/src/settings/interp.rs | 213 +++++++++-- .../fabro-types/src/settings/mod.rs | 2 +- 7 files changed, 694 insertions(+), 35 deletions(-) diff --git a/docs/public/workflows/stages-and-nodes.mdx b/docs/public/workflows/stages-and-nodes.mdx index 819befe7e..5ab21481a 100644 --- a/docs/public/workflows/stages-and-nodes.mdx +++ b/docs/public/workflows/stages-and-nodes.mdx @@ -104,7 +104,7 @@ test [label="Run Tests", shape=parallelogram, script="cargo test 2>&1 || true"] | Attribute | Description | |---|---| -| `script` | The shell command to execute | +| `script` | The shell command to execute. Substitutes `{{ goal }}`, `{{ inputs.NAME }}`, and `{{ vars.NAME }}` — see [command node scripts](/workflows/variables#command-node-scripts) | | `language` | `"shell"` (default) or `"python"` | ### Human diff --git a/docs/public/workflows/variables.mdx b/docs/public/workflows/variables.mdx index fb42d126b..1b03917a9 100644 --- a/docs/public/workflows/variables.mdx +++ b/docs/public/workflows/variables.mdx @@ -3,7 +3,7 @@ title: "Variables" description: "Using templates in workflows" --- -Fabro renders `{{ ... }}` templates in exactly two workflow attributes: the graph `goal` and node `prompt`s. Every other attribute is literal text. +Fabro renders `{{ ... }}` templates in exactly two workflow attributes: the graph `goal` and node `prompt`s. A command node's `script` gets narrower treatment — [simple value substitution](#command-node-scripts), not templating. Every other attribute is literal text. ## Template context @@ -51,7 +51,7 @@ digraph Check { } ``` -Other attributes — `script`, `label`, `model`, `provider`, `condition`, and all edge attributes — do not render templates. If one of them contains `{{ … }}` or `{% … %}`, the syntax is treated as literal text and Fabro records a `detemplated_attribute` warning suggesting you move the dynamic value into a `prompt` or `goal`. +Other attributes — `label`, `model`, `provider`, `condition`, and all edge attributes — do not render templates. If one of them contains `{{ … }}` or `{% … %}`, the syntax is treated as literal text and Fabro records a `detemplated_attribute` warning suggesting you move the dynamic value into a `prompt` or `goal`. Override individual inputs at run time with repeatable `-I` / `--input` flags: @@ -61,6 +61,54 @@ fabro run .fabro/workflows/check/workflow.toml -I repo_name=fabro-2 --input lang CLI input values use TOML scalar parsing when possible. Quoted strings, booleans, integers, and floats keep their typed values; unquoted bare text falls back to a string. Empty values such as `foo=` are accepted as empty strings. Arrays, inline tables, and datetimes are rejected. +## Command node scripts + +A [command node](/workflows/stages-and-nodes#command) `script` substitutes `{{ goal }}`, `{{ inputs.NAME }}`, and `{{ vars.NAME }}`: + +```dot title="check.fabro" +digraph Check { + test [shape=parallelogram, script="cargo test -p {{ inputs.crate }} --profile {{ vars.PROFILE }}"] + pr [shape=parallelogram, script="gh pr create --title \"{{ goal }}\""] +} +``` + +This is value substitution, not templating. Only those three forms are recognized; every other brace sequence reaches the shell untouched, so `jq` filters, `awk` programs, Go templates, and brace expansion all keep working: + +```dot +report [shape=parallelogram, script="kubectl get pod -o go-template='{{ .status.phase }}' | jq '{phase: .}'"] +``` + +There is no `{% if %}`, no filters, and no loops, and `{{ goal }}` has no dotted form — `{{ goal.title }}` stays literal. Put branching in a [conditional node](/workflows/stages-and-nodes#conditional) or in the shell itself. + +Values are substituted as-is, without shell quoting — the script is yours to quote. If a value can contain spaces or shell metacharacters, quote it at the use site. This matters most for `{{ goal }}`, which is free-form prose: + +```dot +build [shape=parallelogram, script="docker build -t \"{{ inputs.image }}\" ."] +``` + +Substituted text is never scanned again, so a goal or input containing `{{ ... }}` reaches the shell as literal characters rather than being interpolated a second time. + +### `env` and `secrets` are not substituted + +`{{ env.NAME }}` and `{{ secrets.NAME }}` are rejected in a `script` with a validation error. A script runs in a shell, so read environment variables the shell way: + +```dot +deploy [shape=parallelogram, script="deploy --token $DEPLOY_TOKEN"] +``` + +To make a secret available that way, put it in the [environment's env map](/execution/run-configuration#run-environment-and-environments-slug), where `{{ secrets.* }}` does resolve — at run start, into the sandbox environment rather than into the script text: + +```toml title="run.toml" +[environments.ci.env] +DEPLOY_TOKEN = "{{ secrets.DEPLOY_TOKEN }}" +``` + +This keeps secret values out of the `command.started` event, which records the script verbatim. + +### When values resolve + +Inputs and variables are substituted when the run is created, at the same time as goals and prompts — the persisted workflow already contains the final script. An unbound input or variable is a warning from `fabro validate` and an error at run creation, so a run never executes a partially substituted command. + ## Server-managed run config variables Use server-managed variables for non-sensitive values that should be shared across runs, such as deployment environments, default branches, regions, or image tags: @@ -115,8 +163,9 @@ Fabro keeps workflow structure static and renders workflow templates once: 2. Literal `import`, `@file`, graph-goal file, and child-workflow references are resolved. 3. The graph `goal` is rendered with the `{ inputs, vars }` context. 4. Node `prompt` attributes are rendered with the `{ goal, inputs, vars }` context. +5. Node `script` attributes have their `{{ goal }}`, `{{ inputs.* }}`, and `{{ vars.* }}` values substituted. -Templates are not supported in graph syntax, node IDs, edge structure, `import` paths, `@file` paths, child workflow paths, other file references, or any attribute besides `prompt` and `goal`. +Templates are not supported in graph syntax, node IDs, edge structure, `import` paths, `@file` paths, child workflow paths, other file references, or any attribute besides `prompt` and `goal` — and `script`, which takes value substitution rather than templates. Fabro renders the graph `goal` first and stores the rendered value back onto the graph. Prompts that use `{{ goal }}` receive that rendered value. @@ -124,6 +173,8 @@ Fabro renders the graph `goal` first and stores the rendered value back onto the Fabro renders undefined workflow variables as empty text and records a `template_undefined_variable` diagnostic. `fabro validate` reports that diagnostic as a warning so you can validate workflow structure before all inputs are known. Offline validation does not read a server's variable store, so `{{ vars.* }}` references also warn there. Run-style commands such as `fabro run`, `fabro create`, and preflight use the server snapshot and promote any still-undefined reference to an error before proceeding. +In a `script`, an undefined value records the same diagnostic but leaves the token in place rather than emptying it, so validation output shows what is unbound. + ## Template includes Prompt and goal templates support static MiniJinja loader dependencies such as `{% include "partial.md" %}`. Includes are resolved relative to the template file being rendered and can be nested. diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 2ec40d443..61c069ade 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -711,6 +711,58 @@ reasoning = false assert!(validated.has_errors()); } + #[test] + fn vars_resolve_in_command_script_through_create_pipeline() { + let dot = r#"digraph Test { + graph [goal="Ship it"] + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + work [label="Work", shape=parallelogram, script="deploy --stage {{ vars.STAGE }}"] + start -> work -> exit + }"#; + let vars = HashMap::from([("STAGE".to_string(), "staging".to_string())]); + let validated = validate_dot_with_vars(dot, vars); + validated.raise_on_errors().unwrap(); + + assert_eq!( + validated.graph().nodes["work"] + .attrs + .get("script") + .and_then(fabro_graphviz::graph::AttrValue::as_str), + Some("deploy --stage staging"), + ); + } + + /// The script diagnostic must carry the same rule as the prompt one so the + /// existing run-create promotion catches an unbound value before a run + /// executes a half-interpolated command. + #[test] + fn unknown_input_in_script_warns_at_validate_then_errors_at_run_create() { + let dot = r#"digraph Test { + graph [goal="Ship it"] + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + work [label="Work", shape=parallelogram, script="deploy --stage {{ inputs.stage }}"] + start -> work -> exit + }"#; + let mut validated = validate_dot_with_vars(dot, HashMap::new()); + + let diagnostic = validated + .diagnostics() + .iter() + .find(|d| d.rule == TEMPLATE_UNDEFINED_VARIABLE_RULE) + .expect("expected a template_undefined_variable diagnostic"); + assert_eq!(diagnostic.severity, Severity::Warning); + assert!( + diagnostic.message.contains("inputs.stage"), + "message: {}", + diagnostic.message + ); + + validated.promote_template_undefined_variables_to_errors(); + assert!(validated.has_errors()); + } + #[test] fn promote_template_undefined_rule_turns_warning_into_error() { let dot = r#"digraph Test { diff --git a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs index 66434266a..9384a9b8f 100644 --- a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs @@ -7,6 +7,7 @@ use fabro_template::{ TemplateContext, TemplateError, TemplateRenderMode, TemplateSource, TemplateSourceOrigin, TemplateStore, }; +use fabro_types::settings::{InterpString, Namespace, ResolveCtx, ResolveError, ResolveErrorKind}; use fabro_util::error::collect_chain; use fabro_validate::{Diagnostic, Severity}; @@ -234,6 +235,107 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> } } +/// Substitutes `{{ inputs.* }}` and `{{ vars.* }}` in a command node `script`. +/// +/// Substitutes `{{ goal }}`, `{{ inputs.* }}`, and `{{ vars.* }}` in a command +/// node `script`. +/// +/// Scripts interpolate through [`InterpString`] tokens rather than the +/// MiniJinja pass that renders prompts. Shell source is full of brace syntax +/// that must survive untouched — `jq` filters, `awk` programs, Go templates, +/// brace expansion — and `InterpString` claims only the bare `{{ goal }}` and +/// `{{ .NAME }}`, leaving everything else literal. +/// +/// `env` and `secrets` are deliberately not wired, so a token in either +/// namespace fails as [`ResolveErrorKind::Unavailable`]. A script reads the +/// environment with `$NAME`, which needs no interpolation, and a resolved +/// secret would be baked into the `CommandStarted` event that records the +/// script verbatim. +/// +/// Resolved values are substituted verbatim, not shell-quoted — matching +/// `[[run.prepare.steps]].script`, where the shell snippet is the author's to +/// quote. Callers wanting a value treated as a single argument must quote it +/// in the script. This matters most for `{{ goal }}`, which is free-form prose. +fn interpolate_script( + text: &str, + ctx: &TemplateContext, + render_mode: RenderMode, + target: &TemplateRenderTarget, + diagnostics: &mut Vec, +) -> Result { + let mut resolve_ctx = ResolveCtx::new() + .with_inputs(|name| ctx.input(name)) + .with_vars(|name| ctx.var(name)); + // The graph goal is rendered before any node attribute, so by here it is + // the final text. Substituting it does not re-interpolate: whatever the + // goal contains lands in the script as literal characters. + if let Some(goal) = ctx.goal() { + resolve_ctx = resolve_ctx.with_goal(goal); + } + match InterpString::parse(text).resolve_with(&mut resolve_ctx) { + Ok(resolved) => Ok(resolved), + // An unbound input or variable is the same authoring gap the prompt + // pass reports, so it follows the same mode split: a hard error at + // run-create, a diagnostic during `fabro validate`. + Err(err) if err.kind == ResolveErrorKind::Missing => match render_mode { + RenderMode::Strict => Err(script_interpolation_error(target, &err)), + RenderMode::Structural => { + diagnostics.push(script_undefined_variable_diagnostic(&err, target)); + // Leave the script in source form. Validation never executes + // it, and showing the unresolved token beats emptying it. + Ok(text.to_string()) + } + }, + // An unsupported namespace can never resolve here, however the inputs + // are bound, so it fails in both modes — the same treatment + // `render_attrs` gives an invalid static reference. + Err(err) => Err(script_interpolation_error(target, &err)), + } +} + +fn script_interpolation_error(target: &TemplateRenderTarget, err: &ResolveError) -> Error { + Error::Validation(format!( + "script interpolation failed in {}: {err} ({})", + target.owner, + script_interpolation_fix(err) + )) +} + +fn script_undefined_variable_diagnostic( + err: &ResolveError, + target: &TemplateRenderTarget, +) -> Diagnostic { + Diagnostic { + rule: TEMPLATE_UNDEFINED_VARIABLE_RULE.to_owned(), + severity: Severity::Warning, + message: format!("{err} in {}", target.owner), + node_id: target.node_id.clone(), + edge: target.edge.clone(), + fix: Some(script_interpolation_fix(err)), + source_path: target.source_name.clone(), + ..Diagnostic::default() + } +} + +fn script_interpolation_fix(err: &ResolveError) -> String { + let name = &err.name; + match err.namespace { + Namespace::Inputs => format!( + "bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`" + ), + Namespace::Vars => format!("set it with `fabro variable set {name} `"), + Namespace::Env => format!( + "`script` does not interpolate environment variables; read it in the shell as \ + `${name}` instead" + ), + Namespace::Secrets => format!( + "`script` does not interpolate secrets; expose `{name}` to the sandbox through \ + `[environments..env]` and read it in the shell as `${name}`" + ), + Namespace::Goal => "set a graph `goal` on the workflow".to_string(), + } +} + const DETEMPLATED_ATTRIBUTE_RULE: &str = "detemplated_attribute"; /// Warning emitted when an attribute that is no longer a template still @@ -244,7 +346,8 @@ fn detemplated_attribute_diagnostic(attr_name: &str, target: &TemplateRenderTarg severity: Severity::Warning, message: format!( "`{attr_name}` in {} is no longer a template; `{{{{ … }}}}` / `{{% … %}}` is treated \ - as literal text. Only node `prompt` and graph `goal` support templating.", + as literal text. Only node `prompt` and graph `goal` support templating, and node \ + `script` supports `{{{{ inputs.* }}}}` / `{{{{ vars.* }}}}` interpolation.", target.owner ), node_id: target.node_id.clone(), @@ -382,9 +485,14 @@ impl TemplateTransform { .with_source_name(source_name.cloned().unwrap_or_else(|| "workflow".into())) .with_source_origin(source_text, text); if matches!(scope, AttributeScope::Node) && attr_name == "prompt" { - // `prompt` is the only templated node attribute. + // `prompt` is the only node attribute rendered as a full + // MiniJinja template. *text = render_template_for_target(text, ctx, render_mode, &target, diagnostics)?; + } else if matches!(scope, AttributeScope::Node) && attr_name == "script" { + // `script` interpolates a narrow token set instead — see + // `interpolate_script`. + *text = interpolate_script(text, ctx, render_mode, &target, diagnostics)?; } else if fabro_template::contains_template_syntax(text) { // Every other attribute is no longer a template (`label`, // `model`, `provider`, `speed`, `condition`, edge `label`, @@ -556,6 +664,229 @@ mod tests { ); } + /// Build a one-node graph whose `test` node carries `script`. + fn script_graph(script: &str) -> Graph { + let mut graph = Graph::new("test"); + graph + .attrs + .insert("goal".to_string(), AttrValue::String("Ship it".to_string())); + let mut node = Node::new("test"); + node.attrs + .insert("script".to_string(), AttrValue::String(script.to_string())); + graph.nodes.insert("test".to_string(), node); + graph + } + + fn script_transform( + inputs: &[(&str, toml::Value)], + vars: &[(&str, &str)], + render_mode: RenderMode, + ) -> TemplateTransform { + TemplateTransform { + context: TemplateContext::new() + .with_inputs( + inputs + .iter() + .map(|(k, v)| ((*k).to_string(), v.clone())) + .collect(), + ) + .with_vars( + vars.iter() + .map(|(k, v)| ((*k).to_string(), (*v).to_string())) + .collect(), + ), + source_name: None, + source_text: None, + render_mode, + } + } + + fn script_of(graph: &Graph) -> &str { + graph.nodes["test"] + .attrs + .get("script") + .and_then(AttrValue::as_str) + .expect("script attribute should still be a string") + } + + #[test] + fn script_interpolates_inputs_and_vars() { + let graph = script_graph("cargo test -p {{ inputs.crate }} --profile {{ vars.PROFILE }}"); + let transform = script_transform( + &[("crate", toml::Value::String("fabro-workflow".into()))], + &[("PROFILE", "ci")], + RenderMode::Structural, + ); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!( + script_of(&graph), + "cargo test -p fabro-workflow --profile ci" + ); + assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); + } + + #[test] + fn script_substitutes_the_rendered_goal() { + let graph = script_graph("gh pr create --title \"{{ goal }}\""); + let transform = script_transform(&[], &[], RenderMode::Structural); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!(script_of(&graph), "gh pr create --title \"Ship it\""); + assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); + } + + /// The reason `script` uses `InterpString` rather than MiniJinja: shell + /// source is full of brace syntax that must reach the shell untouched. + #[test] + fn script_leaves_non_token_braces_literal() { + let script = "jq '{name: .name}' f.json | awk '{print $1}'; echo {{ .Values.image }}; \ + touch {a,b}.txt; {% raw %}"; + let graph = script_graph(script); + let transform = script_transform(&[], &[], RenderMode::Structural); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!(script_of(&graph), script); + assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); + } + + #[test] + fn script_rejects_secrets_tokens_in_both_render_modes() { + for render_mode in [RenderMode::Structural, RenderMode::Strict] { + let graph = script_graph("curl -H \"Authorization: {{ secrets.API_KEY }}\" $URL"); + let transform = script_transform(&[], &[], render_mode); + + let err = transform + .apply_with_diagnostics(graph) + .expect_err("secrets must never interpolate into a script"); + + let message = err.to_string(); + assert!( + message.contains("does not interpolate secrets"), + "unexpected message: {message}" + ); + assert!( + message.contains("$API_KEY"), + "should point at the shell alternative: {message}" + ); + } + } + + #[test] + fn script_rejects_env_tokens_and_points_at_shell_expansion() { + let graph = script_graph("echo {{ env.HOME }}"); + let transform = script_transform(&[], &[], RenderMode::Structural); + + let err = transform + .apply_with_diagnostics(graph) + .expect_err("env is not interpolated in a script"); + + let message = err.to_string(); + assert!(message.contains("$HOME"), "unexpected message: {message}"); + } + + #[test] + fn script_missing_input_warns_and_preserves_source_in_structural_mode() { + let script = "cargo test -p {{ inputs.crate }}"; + let graph = script_graph(script); + let transform = script_transform(&[], &[], RenderMode::Structural); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!(script_of(&graph), script); + let diagnostic = diagnostics + .iter() + .find(|d| d.rule == TEMPLATE_UNDEFINED_VARIABLE_RULE) + .expect("an unbound input should warn"); + assert_eq!(diagnostic.severity, Severity::Warning); + assert_eq!(diagnostic.node_id.as_deref(), Some("test")); + assert!( + diagnostic + .fix + .as_deref() + .unwrap_or_default() + .contains("--input crate="), + "unexpected fix: {:?}", + diagnostic.fix + ); + } + + #[test] + fn script_missing_input_is_an_error_in_strict_mode() { + let graph = script_graph("cargo test -p {{ inputs.crate }}"); + let transform = script_transform(&[], &[], RenderMode::Strict); + + let err = transform + .apply_with_diagnostics(graph) + .expect_err("strict mode must reject an unbound input"); + + assert!( + err.to_string().contains("inputs.crate"), + "unexpected message: {err}" + ); + } + + /// A typed input must produce the same text in a script as in a prompt, so + /// authors do not have to reason about two stringification rules. + #[test] + fn script_and_prompt_stringify_typed_inputs_identically() { + let mut graph = + script_graph("retry --times {{ inputs.attempts }} --fast {{ inputs.fast }}"); + graph.nodes.get_mut("test").unwrap().attrs.insert( + "prompt".to_string(), + AttrValue::String( + "retry --times {{ inputs.attempts }} --fast {{ inputs.fast }}".to_string(), + ), + ); + let transform = script_transform( + &[ + ("attempts", toml::Value::Integer(3)), + ("fast", toml::Value::Boolean(true)), + ], + &[], + RenderMode::Structural, + ); + + let (graph, _) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!(script_of(&graph), "retry --times 3 --fast true"); + assert_eq!( + graph.nodes["test"] + .attrs + .get("prompt") + .and_then(AttrValue::as_str), + Some(script_of(&graph)), + ); + } + + /// `script` is node-scoped. A graph or edge attribute of the same name is + /// not a command node script and keeps the demoted-attribute behavior. + #[test] + fn script_interpolation_is_node_scoped() { + let mut graph = script_graph("echo ok"); + graph.attrs.insert( + "script".to_string(), + AttrValue::String("echo {{ inputs.crate }}".to_string()), + ); + let transform = script_transform(&[], &[], RenderMode::Structural); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!( + graph.attrs.get("script"), + Some(&AttrValue::String("echo {{ inputs.crate }}".to_string())) + ); + assert!( + diagnostics + .iter() + .any(|d| d.rule == DETEMPLATED_ATTRIBUTE_RULE), + "graph-scope `script` should still warn: {diagnostics:?}" + ); + } + #[test] fn template_transform_leaves_non_string_attrs_unchanged() { let mut graph = Graph::new("test"); diff --git a/lib/foundation/fabro-template/src/lib.rs b/lib/foundation/fabro-template/src/lib.rs index a32a13dd7..bb4950663 100644 --- a/lib/foundation/fabro-template/src/lib.rs +++ b/lib/foundation/fabro-template/src/lib.rs @@ -97,6 +97,35 @@ impl TemplateContext { Self::new().with_goal("{{ goal }}").with_inputs(inputs) } + /// The rendered goal `{{ goal }}` resolves to, or `None` before the goal + /// is known. + #[must_use] + pub fn goal(&self) -> Option<&str> { + self.goal.as_deref() + } + + /// The text `{{ inputs.NAME }}` renders to, or `None` when unbound. + /// + /// Non-template consumers — currently command node `script` interpolation, + /// which uses `InterpString` tokens rather than MiniJinja — read values + /// through here so an input produces the same text in a script as it does + /// in a prompt. + #[must_use] + pub fn input(&self, name: &str) -> Option { + Self::rendered_member(&self.inputs, name) + } + + /// The text `{{ vars.NAME }}` renders to, or `None` when unset. + #[must_use] + pub fn var(&self, name: &str) -> Option { + Self::rendered_member(&self.vars, name) + } + + fn rendered_member(container: &Value, name: &str) -> Option { + let member = container.get_attr(name).ok()?; + (!member.is_undefined()).then(|| member.to_string()) + } + fn into_value(self) -> Value { let goal = self.goal.map(Value::from); let inputs = self.inputs; @@ -786,6 +815,45 @@ mod tests { assert_eq!(rendered, "Goal: Fix bugs"); } + #[test] + fn input_and_var_return_none_when_unbound() { + let ctx = TemplateContext::new(); + + assert_eq!(ctx.input("missing"), None); + assert_eq!(ctx.var("MISSING"), None); + } + + /// `input`/`var` exist so non-template consumers render a value the same + /// way a prompt does. Pin that equivalence across the scalar TOML types. + #[test] + fn input_matches_what_a_template_renders() { + let ctx = TemplateContext::new().with_inputs(HashMap::from([ + ("name".to_string(), toml::Value::String("fabro".into())), + ("attempts".to_string(), toml::Value::Integer(3)), + ("ratio".to_string(), toml::Value::Float(1.5)), + ("fast".to_string(), toml::Value::Boolean(true)), + ])); + + for name in ["name", "attempts", "ratio", "fast"] { + let rendered = render(&format!("{{{{ inputs.{name} }}}}"), &ctx).unwrap(); + assert_eq!(ctx.input(name), Some(rendered), "mismatch for `{name}`"); + } + } + + #[test] + fn var_matches_what_a_template_renders() { + let ctx = TemplateContext::new().with_vars(HashMap::from([( + "STAGE".to_string(), + "staging".to_string(), + )])); + + assert_eq!(ctx.var("STAGE").as_deref(), Some("staging")); + assert_eq!( + ctx.var("STAGE"), + Some(render("{{ vars.STAGE }}", &ctx).unwrap()) + ); + } + #[test] fn references_top_level_variable_detects_goal_self_reference() { assert!(references_top_level_variable("Do {{ goal }} now", "goal")); diff --git a/lib/foundation/fabro-types/src/settings/interp.rs b/lib/foundation/fabro-types/src/settings/interp.rs index e31c3e8db..b91c158ea 100644 --- a/lib/foundation/fabro-types/src/settings/interp.rs +++ b/lib/foundation/fabro-types/src/settings/interp.rs @@ -1,15 +1,18 @@ //! Interpolation for config strings. //! //! An [`InterpString`] field may contain narrow `{{ .NAME }}` -//! tokens — no template logic. Three [`Namespace`]s resolve here: `env`, -//! `vars`, and `secrets`. `inputs` is **template-only**: it is a -//! recognized namespace so an `{{ inputs.* }}` token fails loudly with a clear -//! message instead of passing through as literal text, but it never resolves -//! in an `InterpString` field — it belongs in prompts and goals. Which of the -//! resolvable namespaces actually apply is scope-determined by the caller -//! through [`ResolveCtx`]: server-scope settings provide `env` (and eventually -//! `secrets`), run-scope settings additionally provide `vars`. A token whose -//! namespace is not available in the resolution context fails loudly. +//! tokens — no template logic. Which [`Namespace`]s resolve is +//! scope-determined by the caller through [`ResolveCtx`]: server-scope +//! settings provide `env` (and eventually `secrets`), run-scope settings +//! additionally provide `vars`, and a command node `script` provides `inputs` +//! and `vars` only. A token whose namespace has no lookup in the resolution +//! context fails loudly rather than passing through as literal text. +//! +//! Most call sites do not wire `inputs`: it is bound where a run's typed +//! `[run.inputs]` values are in scope, which is the workflow graph, not +//! general config. Everywhere else an `{{ inputs.* }}` token still parses, so +//! it fails with a clear message instead of reaching a consumer as literal +//! text. //! //! Resolution timing is split: `vars` substitutes early (server-side, at run //! creation) via [`InterpString::substitute_with`], while `env`/`secrets` @@ -43,6 +46,9 @@ enum Segment { }, } +/// The sole token body with no `namespace.name` shape. +const GOAL_TOKEN: &str = "goal"; + /// The interpolation namespaces recognized inside `{{ ... }}` tokens. #[derive( Debug, Clone, Copy, PartialEq, Eq, Hash, strum::Display, strum::EnumString, strum::IntoStaticStr, @@ -57,6 +63,12 @@ pub enum Namespace { Secrets, /// `{{ inputs.NAME }}` — workflow run inputs, substituted early. Inputs, + /// The bare `{{ goal }}` token — the run goal, substituted early. + /// + /// Unlike the others this names a single value rather than a namespace of + /// them, so it has no dotted form: only the exact body `goal` produces it, + /// and `{{ goal.anything }}` stays literal text. + Goal, } impl Namespace { @@ -67,15 +79,27 @@ impl Namespace { Self::Vars => "variable", Self::Secrets => "secret", Self::Inputs => "input", + Self::Goal => "run goal", } } + /// Whether this namespace is written as a bare token rather than + /// `namespace.name`. + fn is_bare(self) -> bool { + matches!(self, Self::Goal) + } + /// Parse a trimmed `{{ ... }}` token body into a namespace + name, or /// `None` when the body is not a recognized token (it then stays literal). fn parse_token(token: &str) -> Option<(Self, String)> { let trimmed = token.trim(); + if trimmed == GOAL_TOKEN { + return Some((Self::Goal, GOAL_TOKEN.to_owned())); + } let (prefix, name) = trimmed.split_once('.')?; - let namespace = prefix.parse::().ok()?; + // A bare namespace has no dotted spelling, so `{{ goal.title }}` is not + // a token and reaches the consumer as literal text. + let namespace = prefix.parse::().ok().filter(|ns| !ns.is_bare())?; namespace .is_valid_name(name) .then(|| (namespace, name.to_owned())) @@ -102,6 +126,7 @@ impl Namespace { } chars.all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '-') } + Self::Goal => name == GOAL_TOKEN, } } } @@ -118,6 +143,8 @@ pub struct ResolveCtx<'a> { env: Option>, vars: Option>, secrets: Option>, + inputs: Option>, + goal: Option>, } type LookupFn<'a> = Box Option + 'a>; @@ -146,16 +173,37 @@ impl<'a> ResolveCtx<'a> { self } + /// Typed `[run.inputs]` values, available only where a run's inputs are in + /// scope. Leaving this unwired — which every config-layer call site does — + /// keeps `{{ inputs.* }}` unavailable, so `resolve_with` fails loudly and + /// `substitute_with` preserves the token for a goal (an `InterpString` + /// that feeds a template) to forward to the template layer. + #[must_use] + pub fn with_inputs(mut self, lookup: impl FnMut(&str) -> Option + 'a) -> Self { + self.inputs = Some(Box::new(lookup)); + self + } + + /// The rendered run goal, available only where a run's goal is in scope. + /// Like [`ResolveCtx::with_inputs`], leaving it unwired keeps + /// `{{ goal }}` unavailable for that call site. + /// + /// Takes the value rather than a lookup: unlike a namespace there is + /// nothing to look up by name, so a wired goal can never be `Missing`. + #[must_use] + pub fn with_goal(mut self, goal: impl Into) -> Self { + let goal = goal.into(); + self.goal = Some(Box::new(move |_| Some(goal.clone()))); + self + } + fn lookup_for(&mut self, namespace: Namespace) -> Option<&mut LookupFn<'a>> { match namespace { Namespace::Env => self.env.as_mut(), Namespace::Vars => self.vars.as_mut(), Namespace::Secrets => self.secrets.as_mut(), - // `inputs` is template-only: an `InterpString` resolve context - // never provides it, so an `{{ inputs.* }}` token is always - // unavailable here. `substitute_with` still preserves the token so a - // goal (an `InterpString` that feeds a template) can forward it. - Namespace::Inputs => None, + Namespace::Inputs => self.inputs.as_mut(), + Namespace::Goal => self.goal.as_mut(), } } } @@ -267,9 +315,11 @@ impl InterpString { Segment::Literal(text) => out.push_str(text), Segment::Token { namespace, name } => { out.push_str("{{ "); - out.push_str(namespace.into()); - out.push('.'); - out.push_str(name); + out.push_str((*namespace).into()); + if !namespace.is_bare() { + out.push('.'); + out.push_str(name); + } out.push_str(" }}"); } } @@ -463,14 +513,19 @@ impl fmt::Display for ResolveError { self.name, self.name ), ResolveErrorKind::Unavailable => match namespace { - // `inputs` is template-only: it never resolves in an - // `InterpString` field. Point the user at where it works. + // `inputs` and `goal` resolve only where a run's values are in + // scope. Point the user at where that is. Namespace::Inputs => write!( f, - "{{{{ inputs.{} }}}} is only available in prompts and goals, not in other \ - config fields", + "{{{{ inputs.{} }}}} is only available in prompts, goals, and command node \ + `script` attributes, not in other config fields", self.name ), + Namespace::Goal => write!( + f, + "{{{{ goal }}}} is only available in prompts and command node `script` \ + attributes, not in other config fields" + ), _ => write!( f, "{noun} {:?} referenced by {{{{ {namespace}.{} }}}} is not supported in \ @@ -754,10 +809,9 @@ mod tests { } #[test] - fn resolve_with_rejects_inputs_as_template_only() { - // `inputs` is template-only. An `{{ inputs.* }}` token never resolves - // in an `InterpString` field — it fails loudly, pointing the - // user at prompts and goals. + fn resolve_with_rejects_inputs_when_not_wired() { + // A context that does not opt into `inputs` fails loudly, pointing the + // user at the scopes where inputs do resolve. let s = InterpString::parse("run-{{ inputs.ticket-id }}"); let err = s.resolve_with(&mut ResolveCtx::new()).unwrap_err(); @@ -765,12 +819,115 @@ mod tests { assert_eq!(err.namespace, Namespace::Inputs); assert_eq!(err.kind, ResolveErrorKind::Unavailable); assert!( - err.to_string() - .contains("only available in prompts and goals"), + err.to_string().contains("only available in prompts, goals"), "unexpected message: {err}" ); } + #[test] + fn resolve_with_resolves_inputs_when_wired() { + let s = InterpString::parse("run-{{ inputs.ticket-id }}-{{ vars.STAGE }}"); + + let resolved = s + .resolve_with( + &mut ResolveCtx::new() + .with_inputs(lookup_from(&[("ticket-id", "4821")])) + .with_vars(lookup_from(&[("STAGE", "staging")])), + ) + .unwrap(); + + assert_eq!(resolved, "run-4821-staging"); + } + + #[test] + fn resolve_with_resolves_the_bare_goal_token_when_wired() { + let s = InterpString::parse("gh pr create --title \"{{ goal }}\""); + + let resolved = s + .resolve_with(&mut ResolveCtx::new().with_goal("Fix the login bug")) + .unwrap(); + + assert_eq!(resolved, "gh pr create --title \"Fix the login bug\""); + } + + #[test] + fn resolve_with_rejects_the_goal_token_when_not_wired() { + let s = InterpString::parse("echo {{ goal }}"); + + let err = s.resolve_with(&mut ResolveCtx::new()).unwrap_err(); + + assert_eq!(err.namespace, Namespace::Goal); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); + assert!( + err.to_string().contains("{{ goal }} is only available"), + "unexpected message: {err}" + ); + } + + /// `goal` names a single value, so it has no dotted spelling. Anything of + /// the form `{{ goal.x }}` must stay literal rather than becoming a token + /// that could never resolve. + #[test] + fn goal_has_no_dotted_form() { + for source in ["{{ goal.title }}", "{{ goal. }}", "{{ goals }}"] { + let s = InterpString::parse(source); + let resolved = s + .resolve_with(&mut ResolveCtx::new().with_goal("Ship it")) + .unwrap_or_else(|err| panic!("`{source}` should stay literal, got: {err}")); + assert_eq!(resolved, source); + } + } + + /// Substituted text is output, not more input. A resolved value that + /// happens to contain token syntax must land verbatim rather than being + /// scanned again — otherwise a goal or input could smuggle in a token the + /// call site never wired. + #[test] + fn resolve_with_does_not_rescan_substituted_values() { + let s = InterpString::parse("{{ goal }} | {{ vars.PAYLOAD }}"); + + let resolved = s + .resolve_with( + &mut ResolveCtx::new() + .with_goal("{{ secrets.API_KEY }}") + .with_vars(lookup_from(&[("PAYLOAD", "{{ env.HOME }}")])), + ) + .unwrap(); + + assert_eq!(resolved, "{{ secrets.API_KEY }} | {{ env.HOME }}"); + } + + #[test] + fn goal_token_round_trips_through_source_form() { + // Unwired, `substitute_with` preserves the token; `as_source` must + // reproduce the bare spelling rather than `{{ goal.goal }}`. + let s = InterpString::parse("deploy # {{ goal }}"); + + let preserved = s.substitute_with(&mut ResolveCtx::new()).unwrap(); + + #[expect(clippy::disallowed_methods, reason = "asserting the source round-trip")] + let source = preserved.as_source(); + assert_eq!(source, "deploy # {{ goal }}"); + } + + /// Wiring `inputs` at one call site must not make it resolvable anywhere + /// else. Every config-layer context leaves it unwired, and this pins that + /// the availability stays per-context rather than global. + #[test] + fn wiring_inputs_does_not_leak_into_other_contexts() { + let s = InterpString::parse("{{ inputs.id }}"); + + let wired = s + .resolve_with(&mut ResolveCtx::new().with_inputs(lookup_from(&[("id", "7")]))) + .unwrap(); + assert_eq!(wired, "7"); + + let err = s + .resolve_with(&mut ResolveCtx::new().with_env(lookup_from(&[("id", "7")]))) + .unwrap_err(); + assert_eq!(err.kind, ResolveErrorKind::Unavailable); + } + #[test] fn substitute_variables_preserves_late_bound_tokens() { let s = diff --git a/lib/foundation/fabro-types/src/settings/mod.rs b/lib/foundation/fabro-types/src/settings/mod.rs index 5bf91e4d9..e7c44be84 100644 --- a/lib/foundation/fabro-types/src/settings/mod.rs +++ b/lib/foundation/fabro-types/src/settings/mod.rs @@ -25,7 +25,7 @@ pub use cli::{ CliLoggingSettings, CliNamespace, CliOutputSettings, CliTargetSettings, CliUpdatesSettings, }; pub use duration::{Duration, ParseDurationError}; -pub use interp::{InterpString, ResolveCtx, ResolveError, ResolveErrorKind}; +pub use interp::{InterpString, Namespace, ResolveCtx, ResolveError, ResolveErrorKind}; pub use model_ref::{ AmbiguousModelRef, ModelRef, ModelRegistry, ParseModelRefError, ResolvedModelRef, }; From e91343bbeb3083430e9684a90342041e96382edb Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 17:14:03 -0400 Subject: [PATCH 09/11] refactor: address run title review findings --- .../fabro-server/src/run_title_generation.rs | 10 ++++- lib/apps/fabro-server/src/test_support.rs | 45 +++++++++++++++++++ lib/foundation/fabro-model/src/catalog.rs | 32 ++++++++----- 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/lib/apps/fabro-server/src/run_title_generation.rs b/lib/apps/fabro-server/src/run_title_generation.rs index 14abce84c..5c64dbc36 100644 --- a/lib/apps/fabro-server/src/run_title_generation.rs +++ b/lib/apps/fabro-server/src/run_title_generation.rs @@ -7,6 +7,7 @@ use fabro_llm::types::TimeoutOptions; use fabro_model::ProviderId; use fabro_template::{TemplateContext, TemplateError}; use fabro_types::{Graph, MAX_RUN_TITLE_CHARS, RunId}; +use fabro_util::error; use serde::Serialize; use toml::Value as TomlValue; @@ -38,7 +39,8 @@ pub(crate) async fn generate_title_or_current(input: GenerateTitleInput<'_>) -> Err(err) => { // A checked-in template that will not render is a bug, not a // transient failure, so this is louder than a generation miss. - tracing::warn!(error = %err, "Run title prompt template failed to render"); + let rendered_error = error::collect_chain(&err).join(": "); + tracing::warn!(error = %rendered_error, "Run title prompt template failed to render"); return current_title; } }; @@ -83,7 +85,11 @@ fn build_title_prompt(input: &TitlePromptInput<'_>) -> Result &Path { + let ObjectStoreSettings::Local { root } = store else { + panic!("test server settings should use a local object store"); + }; + Path::new(root) + } + + #[test] + fn default_storage_redirect_updates_derived_local_store_roots() { + let temp_dir = tempfile::tempdir().expect("test temp dir should be created"); + let vault_path = temp_dir.path().join("secrets.json"); + + let settings = redirect_default_storage_root(default_test_server_settings(), &vault_path); + let storage_root = temp_dir.path().join("storage"); + + assert_eq!(Path::new(&settings.server.storage.root), storage_root); + assert_eq!( + local_store_root(&settings.server.artifacts.store), + storage_root.join("objects/artifacts") + ); + assert_eq!( + local_store_root(&settings.server.slatedb.store), + storage_root.join("objects/slatedb") + ); + } + + #[test] + fn default_storage_redirect_preserves_explicit_storage_root() { + let temp_dir = tempfile::tempdir().expect("test temp dir should be created"); + let vault_path = temp_dir.path().join("secrets.json"); + let expected = + default_test_server_settings().with_storage_override(&temp_dir.path().join("custom")); + + let settings = redirect_default_storage_root(expected.clone(), &vault_path); + + assert_eq!(settings, expected); + } +} diff --git a/lib/foundation/fabro-model/src/catalog.rs b/lib/foundation/fabro-model/src/catalog.rs index 8d89972ed..056f3c912 100644 --- a/lib/foundation/fabro-model/src/catalog.rs +++ b/lib/foundation/fabro-model/src/catalog.rs @@ -1359,10 +1359,7 @@ impl Catalog { if configured.is_empty() { return self.default_model(); } - let configured = configured - .iter() - .filter_map(|id| self.provider(id).map(|provider| provider.id.clone())) - .collect::>(); + let configured = self.canonical_provider_ids(configured); self.providers .iter() .filter(|provider| configured.contains(&provider.id)) @@ -1380,15 +1377,28 @@ impl Catalog { if configured.is_empty() { return self.default_model(); } - let resolved = configured + let configured = self.canonical_provider_ids(configured); + let mut fallback = None; + for model in self + .models + .iter() + .filter(|model| configured.contains(&model.provider)) + { + if model.small_default { + return model; + } + if model.default && fallback.is_none() { + fallback = Some(model); + } + } + fallback.unwrap_or_else(|| self.default_model()) + } + + fn canonical_provider_ids(&self, provider_ids: &[ProviderId]) -> HashSet { + provider_ids .iter() .filter_map(|id| self.provider(id).map(|provider| provider.id.clone())) - .collect::>(); - self.providers - .iter() - .filter(|provider| resolved.contains(&provider.id)) - .find_map(|provider| self.small_default_for_provider(&provider.id)) - .unwrap_or_else(|| self.default_for_configured_ids(configured)) + .collect() } /// Probe model for a provider — the cheapest model suitable for From b04684aec46eeb112b4b05e0b131cbcba79486b6 Mon Sep 17 00:00:00 2001 From: Release Repro Date: Tue, 28 Jul 2026 17:21:35 -0400 Subject: [PATCH 10/11] fix(workflow): harden script value interpolation --- docs/public/workflows/variables.mdx | 18 +- lib/components/fabro-workflow/src/error.rs | 13 +- .../fabro-workflow/src/pipeline/transform.rs | 95 ++++- .../fabro-workflow/src/transforms/mod.rs | 2 +- .../src/transforms/variable_expansion.rs | 327 ++++++++++++++---- lib/foundation/fabro-template/src/lib.rs | 10 +- .../fabro-types/src/settings/interp.rs | 15 +- .../fabro-types/src/settings/mod.rs | 2 +- 8 files changed, 407 insertions(+), 75 deletions(-) diff --git a/docs/public/workflows/variables.mdx b/docs/public/workflows/variables.mdx index 1b03917a9..9e9857571 100644 --- a/docs/public/workflows/variables.mdx +++ b/docs/public/workflows/variables.mdx @@ -68,7 +68,7 @@ A [command node](/workflows/stages-and-nodes#command) `script` substitutes `{{ g ```dot title="check.fabro" digraph Check { test [shape=parallelogram, script="cargo test -p {{ inputs.crate }} --profile {{ vars.PROFILE }}"] - pr [shape=parallelogram, script="gh pr create --title \"{{ goal }}\""] + pr [shape=parallelogram, script="gh pr create --title {{ goal }}"] } ``` @@ -80,22 +80,30 @@ report [shape=parallelogram, script="kubectl get pod -o go-template='{{ .status. There is no `{% if %}`, no filters, and no loops, and `{{ goal }}` has no dotted form — `{{ goal.title }}` stays literal. Put branching in a [conditional node](/workflows/stages-and-nodes#conditional) or in the shell itself. -Values are substituted as-is, without shell quoting — the script is yours to quote. If a value can contain spaces or shell metacharacters, quote it at the use site. This matters most for `{{ goal }}`, which is free-form prose: +In a shell script, Fabro quotes each substituted value as one shell argument. Put the token where one shell word is valid. Do not add quotes around the token, and do not use a token to inject multiple flags or shell syntax: ```dot -build [shape=parallelogram, script="docker build -t \"{{ inputs.image }}\" ."] +build [shape=parallelogram, script="docker build -t {{ inputs.image }} ."] ``` -Substituted text is never scanned again, so a goal or input containing `{{ ... }}` reaches the shell as literal characters rather than being interpolated a second time. +For `language="python"`, Fabro inserts each value as a quoted string literal. Put the token where a Python expression is valid: + +```dot +report [shape=parallelogram, language="python", script="print({{ goal }})"] +``` + +Substituted text is never scanned again, so a goal or input containing `{{ ... }}` reaches the command as literal characters rather than being interpolated a second time. ### `env` and `secrets` are not substituted -`{{ env.NAME }}` and `{{ secrets.NAME }}` are rejected in a `script` with a validation error. A script runs in a shell, so read environment variables the shell way: +`{{ env.NAME }}` and `{{ secrets.NAME }}` are rejected in a `script` with a validation error. Read environment variables with `$NAME` in a shell script: ```dot deploy [shape=parallelogram, script="deploy --token $DEPLOY_TOKEN"] ``` +In a Python script, read them with `os.environ["NAME"]`. + To make a secret available that way, put it in the [environment's env map](/execution/run-configuration#run-environment-and-environments-slug), where `{{ secrets.* }}` does resolve — at run start, into the sandbox environment rather than into the script text: ```toml title="run.toml" diff --git a/lib/components/fabro-workflow/src/error.rs b/lib/components/fabro-workflow/src/error.rs index f52079a29..a16e0b0ab 100644 --- a/lib/components/fabro-workflow/src/error.rs +++ b/lib/components/fabro-workflow/src/error.rs @@ -7,7 +7,7 @@ use fabro_model::ModelSelectionError; use fabro_template::TemplateError; pub use fabro_types::failure_signature::FailureSignature; pub use fabro_types::outcome::FailureCategory; -use fabro_types::settings::AmbiguousModelRef; +use fabro_types::settings::{AmbiguousModelRef, ResolveError}; use fabro_types::{ExecOutputTail, FailureReason, RunFailure}; use fabro_util::error::{SharedError, collect_causes, collect_chain, render_with_causes}; use fabro_validate::Diagnostic; @@ -263,6 +263,14 @@ pub enum Error { #[error("Validation failed")] ValidationFailed { diagnostics: Vec }, + #[error("Validation error: script interpolation failed in {owner}: {source} ({fix})")] + ScriptInterpolation { + owner: String, + fix: String, + #[source] + source: ResolveError, + }, + #[error("Model selection failed: {0}")] ModelSelection(#[from] ModelSelectionError), @@ -427,6 +435,7 @@ impl Error { .as_ref() .map_or_else(Vec::new, |source| collect_chain(source)), Self::Template { source, .. } => collect_chain(source), + Self::ScriptInterpolation { source, .. } => collect_chain(source), Self::Llm(err) => collect_causes(err), _ => Vec::new(), } @@ -452,6 +461,7 @@ impl Error { Self::Parse(_) | Self::Validation(_) | Self::ValidationFailed { .. } + | Self::ScriptInterpolation { .. } | Self::ModelSelection(_) | Self::ModelReference(_) | Self::Template { .. } @@ -475,6 +485,7 @@ impl Error { Self::Parse(_) | Self::Validation(_) | Self::ValidationFailed { .. } + | Self::ScriptInterpolation { .. } | Self::ModelSelection(_) | Self::ModelReference(_) | Self::Template { .. } diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index b399d6637..c7181ef4d 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use super::types::{Parsed, TransformOptions, Transformed}; use crate::error::Error; use crate::transforms::{ - FileInliningTransform, ImportTransform, ModelResolutionTransform, + FileInliningTransform, ImportTransform, ModelResolutionTransform, ScriptInterpolationTransform, StylesheetApplicationTransform, TemplateTransform, Transform, }; @@ -62,6 +62,13 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result run -> exit + }"#, + ); + let parsed = parse( + r#"digraph Test { + graph [goal="Test"] + start [shape=Mdiamond] + child [import="./child.fabro"] + exit [shape=Msquare] + start -> child -> exit + }"#, + ) + .unwrap(); + let transformed = transform(parsed, &TransformOptions { + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from([ + ( + "payload".to_string(), + toml::Value::String("{{ secrets.API_KEY }}".to_string()), + ), + ])), + ..transform_options() + }) + .unwrap(); + + assert_eq!( + transformed.graph.nodes["child.run"] + .attrs + .get("script") + .and_then(AttrValue::as_str), + Some("printf '%s' '{{ secrets.API_KEY }}'") + ); + } + + #[test] + fn imported_script_reports_a_missing_value_once() { + let dir = tempfile::tempdir().unwrap(); + write_file( + &dir.path().join("child.fabro"), + r#"digraph child { + start [shape=Mdiamond] + run [shape=parallelogram, script="echo {{ inputs.missing }}"] + exit [shape=Msquare] + start -> run -> exit + }"#, + ); + let parsed = parse( + r#"digraph Test { + graph [goal="Test"] + start [shape=Mdiamond] + child [import="./child.fabro"] + exit [shape=Msquare] + start -> child -> exit + }"#, + ) + .unwrap(); + let transformed = transform(parsed, &TransformOptions { + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + render_mode: crate::operations::RenderMode::Structural, + ..transform_options() + }) + .unwrap(); + + let missing_value_diagnostics = transformed + .diagnostics + .iter() + .filter(|diagnostic| diagnostic.rule == TEMPLATE_UNDEFINED_VARIABLE_RULE) + .count(); + assert_eq!( + missing_value_diagnostics, 1, + "{:?}", + transformed.diagnostics + ); + } + #[test] fn transform_interpolates_vars_in_node_prompt() { let dot = r#"digraph Test { diff --git a/lib/components/fabro-workflow/src/transforms/mod.rs b/lib/components/fabro-workflow/src/transforms/mod.rs index 1f6879cda..8246ee750 100644 --- a/lib/components/fabro-workflow/src/transforms/mod.rs +++ b/lib/components/fabro-workflow/src/transforms/mod.rs @@ -22,4 +22,4 @@ pub use import::ImportTransform; pub use model_resolution::ModelResolutionTransform; pub use preamble::PreambleTransform; pub use stylesheet_application::StylesheetApplicationTransform; -pub use variable_expansion::{RenderMode, TemplateTransform}; +pub use variable_expansion::{RenderMode, ScriptInterpolationTransform, TemplateTransform}; diff --git a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs index 9384a9b8f..7844c7918 100644 --- a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs @@ -1,14 +1,17 @@ +use std::borrow::Cow; use std::collections::HashMap; use std::fmt::Write as _; use std::sync::Arc; -use fabro_graphviz::graph::{AttrValue, Graph}; +use fabro_graphviz::graph::{AttrValue, Graph, Node}; use fabro_template::{ TemplateContext, TemplateError, TemplateRenderMode, TemplateSource, TemplateSourceOrigin, TemplateStore, }; -use fabro_types::settings::{InterpString, Namespace, ResolveCtx, ResolveError, ResolveErrorKind}; +use fabro_types::settings::interp::Namespace; +use fabro_types::settings::{InterpString, ResolveCtx, ResolveError, ResolveErrorKind}; use fabro_util::error::collect_chain; +use fabro_util::shell; use fabro_validate::{Diagnostic, Severity}; use super::Transform; @@ -223,9 +226,7 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> message, node_id: target.node_id.clone(), edge: target.edge.clone(), - fix: Some(format!( - "bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`" - )), + fix: Some(input_binding_fix(name)), source_path: location.source_name.or_else(|| target.source_name.clone()), line: location.line, column: location.column, @@ -235,10 +236,12 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> } } -/// Substitutes `{{ inputs.* }}` and `{{ vars.* }}` in a command node `script`. -/// -/// Substitutes `{{ goal }}`, `{{ inputs.* }}`, and `{{ vars.* }}` in a command -/// node `script`. +fn input_binding_fix(name: &str) -> String { + format!("bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`") +} + +/// Substitutes `{{ goal }}`, `{{ inputs.* }}`, and `{{ vars.* }}` in one +/// command node `script`. /// /// Scripts interpolate through [`InterpString`] tokens rather than the /// MiniJinja pass that renders prompts. Shell source is full of brace syntax @@ -252,53 +255,81 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> /// secret would be baked into the `CommandStarted` event that records the /// script verbatim. /// -/// Resolved values are substituted verbatim, not shell-quoted — matching -/// `[[run.prepare.steps]].script`, where the shell snippet is the author's to -/// quote. Callers wanting a value treated as a single argument must quote it -/// in the script. This matters most for `{{ goal }}`, which is free-form prose. -fn interpolate_script( - text: &str, +/// Shell values are quoted as one argument. Python values are quoted as string +/// literals. In both languages the token must stand where one value is valid; +/// callers must not wrap it in another string literal. +fn interpolate_script<'a>( + text: &'a str, ctx: &TemplateContext, + language: &str, render_mode: RenderMode, target: &TemplateRenderTarget, diagnostics: &mut Vec, -) -> Result { +) -> Result, Error> { + if !text.contains("{{") { + return Ok(Cow::Borrowed(text)); + } + + let parsed = InterpString::parse(text); + if parsed.is_literal() { + return Ok(Cow::Borrowed(text)); + } + let mut resolve_ctx = ResolveCtx::new() - .with_inputs(|name| ctx.input(name)) - .with_vars(|name| ctx.var(name)); + .with_inputs(|name| { + ctx.input(name) + .map(|value| quote_script_value(&value, language)) + }) + .with_vars(|name| { + ctx.var(name) + .map(|value| quote_script_value(&value, language)) + }); // The graph goal is rendered before any node attribute, so by here it is // the final text. Substituting it does not re-interpolate: whatever the // goal contains lands in the script as literal characters. if let Some(goal) = ctx.goal() { - resolve_ctx = resolve_ctx.with_goal(goal); + resolve_ctx = resolve_ctx.with_goal(quote_script_value(goal, language)); } - match InterpString::parse(text).resolve_with(&mut resolve_ctx) { - Ok(resolved) => Ok(resolved), + match parsed.resolve_with(&mut resolve_ctx) { + Ok(resolved) => Ok(Cow::Owned(resolved)), // An unbound input or variable is the same authoring gap the prompt // pass reports, so it follows the same mode split: a hard error at // run-create, a diagnostic during `fabro validate`. Err(err) if err.kind == ResolveErrorKind::Missing => match render_mode { - RenderMode::Strict => Err(script_interpolation_error(target, &err)), + RenderMode::Strict => Err(script_interpolation_error(target, err, language)), RenderMode::Structural => { diagnostics.push(script_undefined_variable_diagnostic(&err, target)); // Leave the script in source form. Validation never executes // it, and showing the unresolved token beats emptying it. - Ok(text.to_string()) + Ok(Cow::Borrowed(text)) } }, // An unsupported namespace can never resolve here, however the inputs // are bound, so it fails in both modes — the same treatment // `render_attrs` gives an invalid static reference. - Err(err) => Err(script_interpolation_error(target, &err)), + Err(err) => Err(script_interpolation_error(target, err, language)), } } -fn script_interpolation_error(target: &TemplateRenderTarget, err: &ResolveError) -> Error { - Error::Validation(format!( - "script interpolation failed in {}: {err} ({})", - target.owner, - script_interpolation_fix(err) - )) +fn quote_script_value(value: &str, language: &str) -> String { + if language == "python" { + serde_json::to_string(value).expect("serializing a string to JSON should not fail") + } else { + shell::shell_quote(value) + } +} + +fn script_interpolation_error( + target: &TemplateRenderTarget, + source: ResolveError, + language: &str, +) -> Error { + let fix = script_interpolation_fix(&source, Some(language)); + Error::ScriptInterpolation { + owner: target.owner.clone(), + fix, + source, + } } fn script_undefined_variable_diagnostic( @@ -311,23 +342,29 @@ fn script_undefined_variable_diagnostic( message: format!("{err} in {}", target.owner), node_id: target.node_id.clone(), edge: target.edge.clone(), - fix: Some(script_interpolation_fix(err)), + fix: Some(script_interpolation_fix(err, None)), source_path: target.source_name.clone(), ..Diagnostic::default() } } -fn script_interpolation_fix(err: &ResolveError) -> String { +fn script_interpolation_fix(err: &ResolveError, language: Option<&str>) -> String { let name = &err.name; match err.namespace { - Namespace::Inputs => format!( - "bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`" - ), + Namespace::Inputs => input_binding_fix(name), Namespace::Vars => format!("set it with `fabro variable set {name} `"), + Namespace::Env if language == Some("python") => format!( + "`script` does not interpolate environment variables; read it in Python as \ + `os.environ[\"{name}\"]` instead" + ), Namespace::Env => format!( "`script` does not interpolate environment variables; read it in the shell as \ `${name}` instead" ), + Namespace::Secrets if language == Some("python") => format!( + "`script` does not interpolate secrets; expose `{name}` to the sandbox through \ + `[environments..env]` and read it in Python as `os.environ[\"{name}\"]`" + ), Namespace::Secrets => format!( "`script` does not interpolate secrets; expose `{name}` to the sandbox through \ `[environments..env]` and read it in the shell as `${name}`" @@ -347,7 +384,8 @@ fn detemplated_attribute_diagnostic(attr_name: &str, target: &TemplateRenderTarg message: format!( "`{attr_name}` in {} is no longer a template; `{{{{ … }}}}` / `{{% … %}}` is treated \ as literal text. Only node `prompt` and graph `goal` support templating, and node \ - `script` supports `{{{{ inputs.* }}}}` / `{{{{ vars.* }}}}` interpolation.", + command `script` supports `{{{{ goal }}}}`, `{{{{ inputs.* }}}}`, and \ + `{{{{ vars.* }}}}` interpolation.", target.owner ), node_id: target.node_id.clone(), @@ -392,8 +430,8 @@ fn goal_self_reference_diagnostic( } } -/// Expands `{{ goal }}` / `{{ inputs.* }}` / `{{ vars.* }}` across all string -/// attributes. +/// Renders graph goals and node prompts, and diagnoses template syntax in +/// attributes that do not support it. pub struct TemplateTransform { pub context: TemplateContext, pub source_name: Option, @@ -476,6 +514,11 @@ impl TemplateTransform { if attr_name == "stack.child_dot_source" { continue; } + // Command scripts use narrow value interpolation in a separate + // one-shot transform after imports are expanded. + if matches!(scope, AttributeScope::Node) && attr_name == "script" { + continue; + } if let Some(kind) = reference_kind_for_attribute(scope, attr_name, text) { validate_static_reference(text, kind) .map_err(|error| Error::Validation(error.to_string()))?; @@ -489,10 +532,6 @@ impl TemplateTransform { // MiniJinja template. *text = render_template_for_target(text, ctx, render_mode, &target, diagnostics)?; - } else if matches!(scope, AttributeScope::Node) && attr_name == "script" { - // `script` interpolates a narrow token set instead — see - // `interpolate_script`. - *text = interpolate_script(text, ctx, render_mode, &target, diagnostics)?; } else if fabro_template::contains_template_syntax(text) { // Every other attribute is no longer a template (`label`, // `model`, `provider`, `speed`, `condition`, edge `label`, @@ -580,6 +619,83 @@ impl Transform for TemplateTransform { } } +/// Interpolates command node scripts once, after import expansion is complete. +/// +/// Keeping this pass separate from [`TemplateTransform`] prevents imported +/// scripts from being scanned once in their source graph and again after they +/// are merged into the root graph. +pub struct ScriptInterpolationTransform { + pub context: TemplateContext, + pub source_name: Option, + pub render_mode: RenderMode, +} + +impl ScriptInterpolationTransform { + fn command_script_language(node: &Node) -> Option<&'static str> { + let is_command = matches!(node.handler_type(), Some("command" | "tool")); + is_command.then(|| { + if node.attrs.get("language").and_then(AttrValue::as_str) == Some("python") { + "python" + } else { + "shell" + } + }) + } + + pub(crate) fn apply_with_diagnostics( + &self, + graph: Graph, + ) -> Result<(Graph, Vec), Error> { + let mut graph = graph; + let mut diagnostics = Vec::new(); + let ctx = self.context.clone().with_goal(graph.goal().to_string()); + + for (node_id, node) in &mut graph.nodes { + let language = Self::command_script_language(node); + let Some(AttrValue::String(text)) = node.attrs.get_mut("script") else { + continue; + }; + let target = TemplateRenderTarget::node_attr( + self.source_name.clone(), + node_id.clone(), + "script", + ) + .with_source_name( + self.source_name + .clone() + .unwrap_or_else(|| "workflow".to_string()), + ); + + if let Some(language) = language { + if let Cow::Owned(resolved) = interpolate_script( + text, + &ctx, + language, + self.render_mode, + &target, + &mut diagnostics, + )? { + *text = resolved; + } + } else if fabro_template::contains_template_syntax(text) { + diagnostics.push(detemplated_attribute_diagnostic("script", &target)); + } + } + + Ok((graph, diagnostics)) + } +} + +impl Transform for ScriptInterpolationTransform { + fn apply(&self, graph: Graph) -> Result { + let (graph, diagnostics) = self.apply_with_diagnostics(graph)?; + if !diagnostics.is_empty() { + return Err(Error::ValidationFailed { diagnostics }); + } + Ok(graph) + } +} + #[cfg(test)] mod tests { use std::collections::HashMap; @@ -671,6 +787,10 @@ mod tests { .attrs .insert("goal".to_string(), AttrValue::String("Ship it".to_string())); let mut node = Node::new("test"); + node.attrs.insert( + "shape".to_string(), + AttrValue::String("parallelogram".to_string()), + ); node.attrs .insert("script".to_string(), AttrValue::String(script.to_string())); graph.nodes.insert("test".to_string(), node); @@ -681,8 +801,8 @@ mod tests { inputs: &[(&str, toml::Value)], vars: &[(&str, &str)], render_mode: RenderMode, - ) -> TemplateTransform { - TemplateTransform { + ) -> ScriptInterpolationTransform { + ScriptInterpolationTransform { context: TemplateContext::new() .with_inputs( inputs @@ -696,7 +816,6 @@ mod tests { .collect(), ), source_name: None, - source_text: None, render_mode, } } @@ -729,12 +848,58 @@ mod tests { #[test] fn script_substitutes_the_rendered_goal() { - let graph = script_graph("gh pr create --title \"{{ goal }}\""); + let graph = script_graph("gh pr create --title {{ goal }}"); let transform = script_transform(&[], &[], RenderMode::Structural); let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); - assert_eq!(script_of(&graph), "gh pr create --title \"Ship it\""); + assert_eq!(script_of(&graph), "gh pr create --title 'Ship it'"); + assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); + } + + #[test] + fn shell_script_quotes_substituted_values_as_one_argument() { + let graph = script_graph("deploy --release {{ inputs.release }}"); + let transform = script_transform( + &[( + "release", + toml::Value::String("stable; touch /tmp/pwned".to_string()), + )], + &[], + RenderMode::Strict, + ); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!( + script_of(&graph), + "deploy --release 'stable; touch /tmp/pwned'" + ); + assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); + } + + #[test] + fn python_script_quotes_substituted_values_as_string_literals() { + let mut graph = script_graph("print({{ inputs.value }})"); + graph.nodes.get_mut("test").unwrap().attrs.insert( + "language".to_string(), + AttrValue::String("python".to_string()), + ); + let transform = script_transform( + &[( + "value", + toml::Value::String("'); __import__('os').system('id'); #".to_string()), + )], + &[], + RenderMode::Strict, + ); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!( + script_of(&graph), + r#"print("'); __import__('os').system('id'); #")"# + ); assert!(diagnostics.is_empty(), "unexpected: {diagnostics:?}"); } @@ -827,6 +992,12 @@ mod tests { err.to_string().contains("inputs.crate"), "unexpected message: {err}" ); + let source = std::error::Error::source(&err) + .expect("script interpolation errors should preserve ResolveError as their source"); + assert!( + source.to_string().contains("inputs.crate"), + "unexpected source: {source}" + ); } /// A typed input must produce the same text in a script as in a prompt, so @@ -841,16 +1012,25 @@ mod tests { "retry --times {{ inputs.attempts }} --fast {{ inputs.fast }}".to_string(), ), ); - let transform = script_transform( - &[ - ("attempts", toml::Value::Integer(3)), - ("fast", toml::Value::Boolean(true)), - ], - &[], - RenderMode::Structural, - ); - - let (graph, _) = transform.apply_with_diagnostics(graph).unwrap(); + let context = TemplateContext::new().with_inputs(HashMap::from([ + ("attempts".to_string(), toml::Value::Integer(3)), + ("fast".to_string(), toml::Value::Boolean(true)), + ])); + let (graph, _) = TemplateTransform { + context: context.clone(), + source_name: None, + source_text: None, + render_mode: RenderMode::Structural, + } + .apply_with_diagnostics(graph) + .unwrap(); + let (graph, _) = ScriptInterpolationTransform { + context, + source_name: None, + render_mode: RenderMode::Structural, + } + .apply_with_diagnostics(graph) + .unwrap(); assert_eq!(script_of(&graph), "retry --times 3 --fast true"); assert_eq!( @@ -871,9 +1051,12 @@ mod tests { "script".to_string(), AttrValue::String("echo {{ inputs.crate }}".to_string()), ); + let (graph, mut diagnostics) = TemplateTransform::new(HashMap::new()) + .apply_with_diagnostics(graph) + .unwrap(); let transform = script_transform(&[], &[], RenderMode::Structural); - - let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + let (graph, script_diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + diagnostics.extend(script_diagnostics); assert_eq!( graph.attrs.get("script"), @@ -887,6 +1070,32 @@ mod tests { ); } + #[test] + fn non_command_node_script_stays_literal() { + let mut graph = script_graph("echo {{ inputs.value }}"); + graph + .nodes + .get_mut("test") + .unwrap() + .attrs + .insert("shape".to_string(), AttrValue::String("box".to_string())); + let transform = script_transform( + &[("value", toml::Value::String("changed".to_string()))], + &[], + RenderMode::Structural, + ); + + let (graph, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!(script_of(&graph), "echo {{ inputs.value }}"); + assert!( + diagnostics + .iter() + .any(|diagnostic| diagnostic.rule == DETEMPLATED_ATTRIBUTE_RULE), + "non-command scripts should keep the literal-template warning: {diagnostics:?}" + ); + } + #[test] fn template_transform_leaves_non_string_attrs_unchanged() { let mut graph = Graph::new("test"); diff --git a/lib/foundation/fabro-template/src/lib.rs b/lib/foundation/fabro-template/src/lib.rs index bb4950663..f312e9416 100644 --- a/lib/foundation/fabro-template/src/lib.rs +++ b/lib/foundation/fabro-template/src/lib.rs @@ -816,13 +816,21 @@ mod tests { } #[test] - fn input_and_var_return_none_when_unbound() { + fn workflow_values_return_none_when_unbound() { let ctx = TemplateContext::new(); + assert_eq!(ctx.goal(), None); assert_eq!(ctx.input("missing"), None); assert_eq!(ctx.var("MISSING"), None); } + #[test] + fn goal_returns_the_rendered_value() { + let ctx = TemplateContext::new().with_goal("Ship it"); + + assert_eq!(ctx.goal(), Some("Ship it")); + } + /// `input`/`var` exist so non-template consumers render a value the same /// way a prompt does. Pin that equivalence across the scalar TOML types. #[test] diff --git a/lib/foundation/fabro-types/src/settings/interp.rs b/lib/foundation/fabro-types/src/settings/interp.rs index b91c158ea..1152bd7fd 100644 --- a/lib/foundation/fabro-types/src/settings/interp.rs +++ b/lib/foundation/fabro-types/src/settings/interp.rs @@ -5,8 +5,9 @@ //! scope-determined by the caller through [`ResolveCtx`]: server-scope //! settings provide `env` (and eventually `secrets`), run-scope settings //! additionally provide `vars`, and a command node `script` provides `inputs` -//! and `vars` only. A token whose namespace has no lookup in the resolution -//! context fails loudly rather than passing through as literal text. +//! and `vars` plus the bare `goal` value. A token whose namespace has no lookup +//! in the resolution context fails loudly rather than passing through as +//! literal text. //! //! Most call sites do not wire `inputs`: it is bound where a run's typed //! `[run.inputs]` values are in scope, which is the workflow graph, not @@ -31,7 +32,7 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; use crate::variable::is_env_style_name; /// A config string that may contain `{{ env.NAME }}`, `{{ vars.NAME }}`, -/// `{{ secrets.NAME }}`, or `{{ inputs.NAME }}` tokens. +/// `{{ secrets.NAME }}`, `{{ inputs.NAME }}`, or `{{ goal }}` tokens. #[derive(Debug, Clone, PartialEq, Eq)] pub struct InterpString { segments: Vec, @@ -359,7 +360,7 @@ impl InterpString { /// for the namespaces it does not — their resolution happens later, /// possibly in a different process. /// - /// This is the early, server-side pass (`vars`/`inputs`); late-bound + /// This is the early, server-side pass (`vars`/`inputs`/`goal`); late-bound /// namespaces (`env`/`secrets`) survive in token form for their /// consumption-time [`InterpString::resolve_with`]. pub fn substitute_with(&self, ctx: &mut ResolveCtx<'_>) -> Result { @@ -559,7 +560,7 @@ impl<'de> Deserialize<'de> for InterpString { fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str( "a string, optionally containing {{ env.NAME }}, {{ vars.NAME }}, \ - {{ secrets.NAME }}, or {{ inputs.NAME }} interpolation tokens", + {{ secrets.NAME }}, {{ inputs.NAME }}, or {{ goal }} interpolation tokens", ) } @@ -712,7 +713,8 @@ mod tests { s: InterpString, } - let input = r#"{"s":"{{ env.A }}/{{ vars.B }}/{{ secrets.C }}/{{ inputs.d-key }}"}"#; + let input = + r#"{"s":"{{ env.A }}/{{ vars.B }}/{{ secrets.C }}/{{ inputs.d-key }}/{{ goal }}"}"#; let parsed: Wrap = serde_json::from_str(input).unwrap(); let rendered = serde_json::to_string(&parsed).unwrap(); assert_eq!(rendered, input); @@ -982,5 +984,6 @@ mod tests { assert_eq!(Namespace::Vars.to_string(), "vars"); assert_eq!(Namespace::Secrets.to_string(), "secrets"); assert_eq!(Namespace::Inputs.to_string(), "inputs"); + assert_eq!(Namespace::Goal.to_string(), "goal"); } } diff --git a/lib/foundation/fabro-types/src/settings/mod.rs b/lib/foundation/fabro-types/src/settings/mod.rs index e7c44be84..5bf91e4d9 100644 --- a/lib/foundation/fabro-types/src/settings/mod.rs +++ b/lib/foundation/fabro-types/src/settings/mod.rs @@ -25,7 +25,7 @@ pub use cli::{ CliLoggingSettings, CliNamespace, CliOutputSettings, CliTargetSettings, CliUpdatesSettings, }; pub use duration::{Duration, ParseDurationError}; -pub use interp::{InterpString, Namespace, ResolveCtx, ResolveError, ResolveErrorKind}; +pub use interp::{InterpString, ResolveCtx, ResolveError, ResolveErrorKind}; pub use model_ref::{ AmbiguousModelRef, ModelRef, ModelRegistry, ParseModelRefError, ResolvedModelRef, }; From 8f9b36c0b86fe35cfa86be5733a5e2a8342f93ab Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 17:23:31 -0400 Subject: [PATCH 11/11] fix(test): propagate storage setup errors --- lib/apps/fabro-server/src/test_support.rs | 42 +++++++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/lib/apps/fabro-server/src/test_support.rs b/lib/apps/fabro-server/src/test_support.rs index 75dff0a1e..a28e38d56 100644 --- a/lib/apps/fabro-server/src/test_support.rs +++ b/lib/apps/fabro-server/src/test_support.rs @@ -5,6 +5,7 @@ use std::sync::Mutex; use std::sync::{Arc, OnceLock}; use std::time::Duration; +use anyhow::Context as _; use axum::extract::Request; #[cfg(test)] use axum::extract::State as AxumState; @@ -257,7 +258,7 @@ impl TestAppStateBuilder { pub fn try_build(mut self) -> anyhow::Result> { let (store, artifact_store) = self.store_bundle.unwrap_or_else(test_store_bundle); let vault_path = self.vault_path.unwrap_or_else(test_secret_store_path); - self.server_settings = redirect_default_storage_root(self.server_settings, &vault_path); + self.server_settings = redirect_default_storage_root(self.server_settings, &vault_path)?; if !self.vault_entries.is_empty() { let mut vault = Vault::load(vault_path.clone()).expect("test vault should load"); for (name, value) in &self.vault_entries { @@ -686,13 +687,17 @@ pub fn test_secret_store_path() -> PathBuf { /// that chose its own root keeps it. The redirect goes through /// [`ServerSettings::with_storage_override`] so the derived local object-store /// roots move with it instead of pointing back at the real storage tree. -fn redirect_default_storage_root(settings: ServerSettings, vault_path: &Path) -> ServerSettings { +fn redirect_default_storage_root( + settings: ServerSettings, + vault_path: &Path, +) -> anyhow::Result { if Path::new(&settings.server.storage.root) != default_storage_dir() { - return settings; + return Ok(settings); } let root = vault_path.with_file_name("storage"); - std::fs::create_dir_all(&root).expect("test storage root should be creatable"); - settings.with_storage_override(&root) + std::fs::create_dir_all(&root) + .with_context(|| format!("creating test storage root at {}", root.display()))?; + Ok(settings.with_storage_override(&root)) } #[must_use] @@ -791,7 +796,8 @@ mod tests { let temp_dir = tempfile::tempdir().expect("test temp dir should be created"); let vault_path = temp_dir.path().join("secrets.json"); - let settings = redirect_default_storage_root(default_test_server_settings(), &vault_path); + let settings = redirect_default_storage_root(default_test_server_settings(), &vault_path) + .expect("default test storage root should redirect"); let storage_root = temp_dir.path().join("storage"); assert_eq!(Path::new(&settings.server.storage.root), storage_root); @@ -812,8 +818,30 @@ mod tests { let expected = default_test_server_settings().with_storage_override(&temp_dir.path().join("custom")); - let settings = redirect_default_storage_root(expected.clone(), &vault_path); + let settings = redirect_default_storage_root(expected.clone(), &vault_path) + .expect("explicit test storage root should be preserved"); assert_eq!(settings, expected); } + + #[test] + #[expect( + clippy::disallowed_methods, + reason = "synchronous fixture setup creates a blocking file before exercising the helper" + )] + fn default_storage_redirect_preserves_directory_creation_error_chain() { + let temp_dir = tempfile::tempdir().expect("test temp dir should be created"); + let vault_path = temp_dir.path().join("secrets.json"); + std::fs::write(temp_dir.path().join("storage"), "not a directory") + .expect("blocking storage path should be created"); + + let error = redirect_default_storage_root(default_test_server_settings(), &vault_path) + .expect_err("storage redirect should reject a file at the directory path"); + + assert!(error.to_string().contains("creating test storage root at")); + assert!( + error.chain().count() >= 2, + "filesystem error should remain in the source chain" + ); + } }