From a64f4d0cd83286dca83b243675a09495d2bb7b4d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 11 Apr 2026 10:58:50 -0400 Subject: [PATCH] feat(template): unify workflow and config template syntax Add a shared MiniJinja-based template crate and migrate workflow prompts, imports, hooks, and InterpString env references to the new {{ ... }} syntax. This also threads typed run inputs through workflow rendering and updates docs and tests to match the new templating model. --- Cargo.lock | 30 ++ Cargo.toml | 1 + docs/agents/prompts.mdx | 13 +- docs/api-reference/fabro-api.yaml | 2 +- docs/examples/clone-substack.mdx | 8 +- docs/execution/run-configuration.mdx | 28 +- docs/integrations/brave-search.mdx | 2 +- docs/reference/cli.mdx | 4 +- docs/reference/dot-language.mdx | 2 +- docs/workflows/variables.mdx | 77 ++-- lib/crates/fabro-cli/src/args.rs | 4 +- lib/crates/fabro-cli/tests/it/cmd/create.rs | 2 +- .../fabro-cli/tests/it/cmd/preflight.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/run.rs | 2 +- lib/crates/fabro-config/tests/resolve_run.rs | 8 +- .../fabro-config/tests/resolve_server.rs | 14 +- lib/crates/fabro-hooks/Cargo.toml | 1 + lib/crates/fabro-hooks/src/executor.rs | 352 ++++++++++++------ lib/crates/fabro-server/tests/it/api/runs.rs | 8 +- lib/crates/fabro-template/Cargo.toml | 21 ++ lib/crates/fabro-template/src/lib.rs | 278 ++++++++++++++ lib/crates/fabro-types/src/settings/interp.rs | 86 +++-- .../fabro-types/tests/run_event_serde.rs | 14 +- .../fabro-types/tests/run_record_serde.rs | 12 +- lib/crates/fabro-util/src/env.rs | 2 + lib/crates/fabro-workflow/Cargo.toml | 3 +- .../fabro-workflow/src/handler/agent.rs | 35 +- .../src/handler/manager_loop.rs | 2 + lib/crates/fabro-workflow/src/handler/mod.rs | 3 + .../fabro-workflow/src/handler/parallel.rs | 2 + .../fabro-workflow/src/handler/prompt.rs | 2 +- .../fabro-workflow/src/operations/create.rs | 53 ++- .../fabro-workflow/src/pipeline/execute.rs | 4 + .../fabro-workflow/src/pipeline/initialize.rs | 7 + .../fabro-workflow/src/pipeline/transform.rs | 62 ++- .../fabro-workflow/src/pipeline/types.rs | 2 + .../fabro-workflow/src/pipeline/validate.rs | 4 +- lib/crates/fabro-workflow/src/test_support.rs | 6 + .../src/transforms/file_inlining.rs | 8 +- .../fabro-workflow/src/transforms/import.rs | 79 ++-- .../fabro-workflow/src/transforms/mod.rs | 8 +- .../src/transforms/model_resolution.rs | 16 +- .../fabro-workflow/src/transforms/preamble.rs | 12 +- .../src/transforms/stylesheet_application.rs | 10 +- .../src/transforms/variable_expansion.rs | 264 ++++++------- .../fabro-workflow/tests/it/integration.rs | 44 ++- 46 files changed, 1087 insertions(+), 512 deletions(-) create mode 100644 lib/crates/fabro-template/Cargo.toml create mode 100644 lib/crates/fabro-template/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index c3595a3e4..c2f38f4c0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1706,6 +1706,7 @@ dependencies = [ "fabro-config", "fabro-llm", "fabro-model", + "fabro-template", "fabro-types", "fabro-util", "httpmock", @@ -2014,6 +2015,18 @@ dependencies = [ "uuid", ] +[[package]] +name = "fabro-template" +version = "0.176.2" +dependencies = [ + "anyhow", + "fabro-util", + "minijinja", + "serde", + "thiserror 2.0.18", + "toml 0.8.23", +] + [[package]] name = "fabro-test" version = "0.176.2" @@ -2125,6 +2138,7 @@ dependencies = [ "fabro-retro", "fabro-sandbox", "fabro-store", + "fabro-template", "fabro-test", "fabro-types", "fabro-util", @@ -3700,6 +3714,12 @@ version = "2.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" +[[package]] +name = "memo-map" +version = "0.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "38d1115007560874e373613744c6fba374c17688327a71c1476d1a5954cc857b" + [[package]] name = "memoffset" version = "0.9.1" @@ -3725,6 +3745,16 @@ dependencies = [ "unicase", ] +[[package]] +name = "minijinja" +version = "2.19.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "805bfd7352166bae857ee569628b52bcd85a1cecf7810861ebceb1686b72b75d" +dependencies = [ + "memo-map", + "serde", +] + [[package]] name = "minimad" version = "0.14.0" diff --git a/Cargo.toml b/Cargo.toml index 1d4a37b24..f39bac0ba 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,6 +73,7 @@ slatedb = "0.11.2" object_store = { version = "0.12.5", features = ["aws"] } rust-embed = "8" percent-encoding = "2" +minijinja = "2" [workspace.lints.rust] unsafe_code = "deny" diff --git a/docs/agents/prompts.mdx b/docs/agents/prompts.mdx index 3901c9162..4a7df5d93 100644 --- a/docs/agents/prompts.mdx +++ b/docs/agents/prompts.mdx @@ -47,23 +47,24 @@ File references are resolved relative to the Graphviz file's directory first, th ### Variable expansion -Prompts support `$variable` placeholders that expand at runtime. Currently the only built-in variable is `$goal`, which resolves to the graph-level `goal` attribute: +Prompts support MiniJinja-style templates. Prompt rendering has access to the workflow goal and typed run inputs: ```dot title="pipeline.fabro" digraph Pipeline { graph [goal="Add a /health endpoint to the API server"] - implement [prompt="Implement the following: $goal"] + implement [prompt="Implement the following: {{ goal }}"] } ``` -At runtime, `$goal` becomes `Add a /health endpoint to the API server`. +At runtime, `{{ goal }}` becomes `Add a /health endpoint to the API server`. -| Variable | Resolves to | +| Expression | Resolves to | |---|---| -| `$goal` | The graph-level `goal` attribute | +| `{{ goal }}` | The graph-level `goal` attribute | +| `{{ inputs.name }}` | A value from `[run.inputs]` | -A `$` not followed by an identifier character (e.g. `$5`) is left as-is. An undefined variable like `$foo` produces a runtime error, catching typos early. +Prompt templates use strict undefined handling, so an expression like `{{ inputs.foo }}` fails fast if `foo` is not defined. Environment variables are not available in prompt templates. ### Fallback to label diff --git a/docs/api-reference/fabro-api.yaml b/docs/api-reference/fabro-api.yaml index 1992a517b..0c719b47f 100644 --- a/docs/api-reference/fabro-api.yaml +++ b/docs/api-reference/fabro-api.yaml @@ -4062,7 +4062,7 @@ components: - `server.auth.api.{jwt,mtls}` internals - `server.artifacts.s3` / `server.slatedb.s3` credentials - `server.integrations.github.webhooks`, Slack/Discord/Teams tokens - - Every `${env.NAME}` InterpString is serialized in its unresolved + - Every `{{ env.NAME }}` InterpString is serialized in its unresolved template form, never the resolved secret value. The top-level object keys follow the v2 schema: `_version`, `project`, diff --git a/docs/examples/clone-substack.mdx b/docs/examples/clone-substack.mdx index fd9ad1799..0c1656a81 100644 --- a/docs/examples/clone-substack.mdx +++ b/docs/examples/clone-substack.mdx @@ -75,7 +75,7 @@ GitHub to Railway validated by code review only — no live deployment execution expand_spec [ label="Expand Spec", - prompt="Goal: $goal\n\n\ + prompt="Goal: {{ goal }}\n\n\ The project specification is at substack-spec-v01.md and the Definition of Done \ is at substack-dod-v01.md. The UI flow diagram is at substack-spec-v01-ui.gv.\n\n\ Read all three files. Scratch artifacts go under .workflow/.\n\n\ @@ -100,7 +100,7 @@ adequate, skip." plan_a [ label="Plan A", class="branch-a", - prompt="Goal: $goal\n\n\ + prompt="Goal: {{ goal }}\n\n\ Read .workflow/spec.md and .workflow/definition_of_done.md. If those files do not \ exist, fall back to reading substack-spec-v01.md and substack-dod-v01.md directly. \ If .workflow/postmortem_latest.md exists, incorporate its lessons.\n\n\ @@ -126,7 +126,7 @@ Write to .workflow/plan_a.md." plan_b [ label="Plan B", class="branch-b", - prompt="Goal: $goal\n\n\ + prompt="Goal: {{ goal }}\n\n\ Read .workflow/spec.md and .workflow/definition_of_done.md. If those files do not \ exist, fall back to reading substack-spec-v01.md and substack-dod-v01.md directly. \ If .workflow/postmortem_latest.md exists, incorporate its lessons.\n\n\ @@ -184,7 +184,7 @@ Write the final plan to .workflow/plan_final.md." class="hard", max_tokens=32768, label="Implement", - prompt="Goal: $goal\n\n\ + prompt="Goal: {{ goal }}\n\n\ Read .workflow/plan_final.md, .workflow/spec.md, and \ .workflow/definition_of_done.md. If the spec or DoD files do not exist at those \ paths, fall back to reading substack-spec-v01.md and substack-dod-v01.md directly.\n\n\ diff --git a/docs/execution/run-configuration.mdx b/docs/execution/run-configuration.mdx index 3f855f8fb..e35b83bd3 100644 --- a/docs/execution/run-configuration.mdx +++ b/docs/execution/run-configuration.mdx @@ -40,7 +40,7 @@ _version = 1 graph = "fabro/workflows/ci.fabro" [run] -goal = "Run the CI pipeline for $repo_name" +goal = "Run the CI pipeline" working_dir = "/tmp/workdir" [run.model] @@ -48,7 +48,7 @@ name = "claude-sonnet-4-5" fallbacks = ["openai", "gemini"] [[run.prepare.steps]] -script = "git clone $repo_url repo" +script = "git clone https://github.com/fabro-sh/fabro repo" [[run.prepare.steps]] script = "cd repo && npm install" @@ -72,7 +72,7 @@ disk = "20GB" dockerfile = "FROM node:20-slim\nRUN apt-get update && apt-get install -y git" [run.sandbox.env] -API_KEY = "${env.MY_API_KEY}" +API_KEY = "{{ env.MY_API_KEY }}" NODE_ENV = "production" [run.checkpoint] @@ -217,20 +217,20 @@ worktree_mode = "always" #### `[run.sandbox.env]` -Pass environment variables into sandbox command and agent execution. Values can be literal strings or host environment references using `${env.VARNAME}` syntax: +Pass environment variables into sandbox command and agent execution. Values can be literal strings or host environment references using `{{ env.VARNAME }}` syntax: ```toml title="run.toml" [run.sandbox.env] -API_KEY = "${env.MY_API_KEY}" +API_KEY = "{{ env.MY_API_KEY }}" NODE_ENV = "production" -SERVICE_URL = "https://api.${env.REGION}.example.com" +SERVICE_URL = "https://api.{{ env.REGION }}.example.com" ``` | Syntax | Description | |---|---| | `"literal"` | Static value passed as-is | -| `"${env.VARNAME}"` | Whole-value reference resolved from the host environment at consumption time | -| `"prefix-${env.X}-suffix"` | Substring interpolation; multiple tokens per string are supported | +| `"{{ env.VARNAME }}"` | Whole-value reference resolved from the host environment at consumption time | +| `"prefix-{{ env.X }}-suffix"` | Substring interpolation; multiple tokens per string are supported | Missing host variables produce a hard error pointing at the specific field and unresolved token. `run.sandbox.env` is a sticky merge-by-key map: entries from all layers combine, with higher-precedence layers overriding individual keys. @@ -260,17 +260,17 @@ repo_url = "https://github.com/fabro-sh/fabro" language = "rust" ``` -Inputs can be used anywhere in the Graphviz file with `$name` syntax: +Inputs can be used anywhere in the Graphviz file with `{{ inputs.name }}` syntax: ```dot title="c-i.fabro" digraph CI { - graph [goal="Run tests for $repo_name"] - clone [shape=parallelogram, script="git clone $repo_url repo"] - test [label="Test", prompt="Run the $language test suite."] + graph [goal="Run tests for {{ inputs.repo_name }}"] + clone [shape=parallelogram, script="git clone {{ inputs.repo_url }} repo"] + test [label="Test", prompt="Run the {{ inputs.language }} test suite."] } ``` -If a `$variable` in the Graphviz file has no matching entry in `[run.inputs]`, Fabro raises an error immediately. A bare `$` not followed by an identifier (e.g. `costs $5`) is left as-is. +If a workflow template references an undefined input like `{{ inputs.langauge }}`, Fabro raises an error immediately. `[run.inputs]` replaces wholesale across layers. Unlike labels, inputs do not merge by key — the highest-precedence layer that sets `inputs` wins its entire map. @@ -430,7 +430,7 @@ Fabro validates the run config when it loads: - **`_version` check** — Only `_version = 1` (or missing, which defaults to `1`) is accepted. The legacy top-level `version` key is rejected with a rename hint. - **Unknown keys** — Any top-level key not in `[project]`, `[workflow]`, `[run]`, `[cli]`, `[server]`, `[features]`, or `_version` is rejected with a targeted rename hint pointing at the v2 replacement path. -- **Variable check** — Any `$variable` in the Graphviz file without a matching `[run.inputs]` entry produces an error. +- **Variable check** — Any undefined workflow template variable in the Graphviz file produces an error. Use `fabro preflight` to validate a run config without executing it: diff --git a/docs/integrations/brave-search.mdx b/docs/integrations/brave-search.mdx index 6b9e16b2c..7d7463875 100644 --- a/docs/integrations/brave-search.mdx +++ b/docs/integrations/brave-search.mdx @@ -64,7 +64,7 @@ digraph Research { start [shape=Mdiamond, label="Start"] exit [shape=Msquare, label="Exit"] - research [label="Research", prompt="Use web_search to find recent information about $goal. Save your findings to research.md."] + research [label="Research", prompt="Use web_search to find recent information about {{ goal }}. Save your findings to research.md."] summarize [label="Summarize", shape=tab, prompt="Read research.md and write a concise summary of the key findings."] start -> research -> summarize -> exit diff --git a/docs/reference/cli.mdx b/docs/reference/cli.mdx index b9bcb993d..05cadb9c7 100644 --- a/docs/reference/cli.mdx +++ b/docs/reference/cli.mdx @@ -94,7 +94,7 @@ fabro run run.toml | `-v, --verbose` | Enable verbose output | | `--sandbox ` | Sandbox for agent tools: `local`, `docker`, or `daytona` | | `--label ` | Attach a label to this run (repeatable) | -| `--goal ` | Override the workflow goal (exposed as `$goal` in prompts) | +| `--goal ` | Override the workflow goal (available as `{{ goal }}` in prompts) | | `--goal-file ` | Read the goal from a file instead of inline text | | `--no-retro` | Skip retro generation after the run | | `--preserve-sandbox` | Keep the sandbox alive after the run finishes (for debugging) | @@ -112,7 +112,7 @@ fabro preflight run.toml | Argument / Flag | Description | |---|---| | `` | Path to a `.fabro` workflow file, `.toml` task config, or workflow name. | -| `--goal ` | Override the workflow goal (exposed as `$goal` in prompts) | +| `--goal ` | Override the workflow goal (available as `{{ goal }}` in prompts) | | `--goal-file ` | Read the goal from a file instead of inline text | | `--model ` | Override default LLM model | | `--provider ` | Override default LLM provider | diff --git a/docs/reference/dot-language.mdx b/docs/reference/dot-language.mdx index bfdcfdfdf..62d329ca6 100644 --- a/docs/reference/dot-language.mdx +++ b/docs/reference/dot-language.mdx @@ -377,7 +377,7 @@ digraph ImplementFeature { exit [shape=Msquare, label="Exit"] // Planning phase - plan [label="Plan", shape=tab, prompt="Create a detailed implementation plan for: $goal"] + plan [label="Plan", shape=tab, prompt="Create a detailed implementation plan for: {{ goal }}"] // Human approval approve [shape=hexagon, label="Approve Plan"] diff --git a/docs/workflows/variables.mdx b/docs/workflows/variables.mdx index c0a0d72bb..ad4be2a38 100644 --- a/docs/workflows/variables.mdx +++ b/docs/workflows/variables.mdx @@ -1,13 +1,24 @@ --- title: "Variables" -description: "Using variables in workflows" +description: "Using templates in workflows" --- -Fabro supports `$variable` placeholders that let you parameterize workflows without editing the Graphviz file. +Fabro uses `{{ ... }}` templates for workflow strings and prompts. + +## Template context + +Workflow and prompt templates can reference: + +| Expression | Resolves to | +|---|---| +| `{{ goal }}` | The workflow goal | +| `{{ inputs.name }}` | A value from `[run.inputs]` | + +Environment variables are **not** available in workflow or prompt templates. Use `{{ env.NAME }}` only in config strings and HTTP hook headers. ## Run config inputs -Define inputs in the `[run.inputs]` section of a run config TOML file: +Define typed inputs in `[run.inputs]`: ```toml title="run.toml" _version = 1 @@ -16,7 +27,7 @@ _version = 1 graph = "check.fabro" [run] -goal = "Run tests for $repo_name" +goal = "Run repository checks" [run.inputs] repo_name = "fabro" @@ -24,55 +35,63 @@ repo_url = "https://github.com/fabro-sh/fabro" language = "rust" ``` -These inputs are expanded into the Graphviz source **before** the graph is parsed. You can use `$variable` anywhere in the Graphviz file — goals, prompts, labels, scripts, or any other attribute: +These values are available throughout the workflow as `{{ inputs.* }}`: ```dot title="check.fabro" digraph Check { - graph [goal="Run tests for $repo_name"] + graph [goal="Run tests for {{ inputs.repo_name }}"] start [shape=Mdiamond, label="Start"] exit [shape=Msquare, label="Exit"] - clone [label="Clone", shape=parallelogram, script="git clone $repo_url repo"] - test [label="Test", prompt="Run the $language test suite in the repo/ directory."] + clone [label="Clone", shape=parallelogram, script="git clone {{ inputs.repo_url }} repo"] + test [label="Test", prompt="Run the {{ inputs.language }} test suite in the repo/ directory."] start -> clone -> test -> exit } ``` -When launched with `fabro run run.toml`, Fabro replaces `$repo_name`, `$repo_url`, and `$language` with their values before parsing the graph. +## `goal` -### Undefined variables - -If a `$variable` in the Graphviz file has no matching entry in `[run.inputs]`, Fabro raises an error. This catches typos early — a misspelled `$langauge` fails immediately rather than passing a literal `$langauge` to the LLM. - -### Escaping `$` - -To include a literal `$` in the output, write `$$`: - -```dot -test [prompt="The env var is $$HOME"] -``` - -This produces `The env var is $HOME` without treating `$HOME` as a variable reference. A bare `$` not followed by an identifier character (e.g. `costs $5`) does not need escaping. - -## The `$goal` variable - -Inside agent and prompt node prompts, Fabro automatically expands `$goal` to the workflow's `goal` attribute. This happens at runtime, after graph parsing: +Agent and prompt nodes also receive the workflow goal at runtime: ```dot title="example.fabro" digraph Example { graph [goal="Implement the login feature"] - plan [label="Plan", prompt="Create a plan for: $goal"] + plan [label="Plan", prompt="Create a plan for: {{ goal }}"] } ``` -The plan node's prompt becomes `"Create a plan for: Implement the login feature"`. +That prompt becomes `Create a plan for: Implement the login feature`. + +## Expansion timing + +Fabro expands templates in multiple passes: + +1. Before DOT parsing, `{{ inputs.* }}` can parameterize structural parts of the graph, including imported `.fabro` files. +2. After parsing, all string graph, node, and edge attributes are rendered again with the real `{ goal, inputs }` context. +3. Agent and prompt handlers do a final runtime render pass as a safety net. + +`{{ goal }}` is preserved through the pre-parse step so it can be resolved later. That means goal-dependent MiniJinja control flow such as `{% if goal %}` is not useful in structural pre-parse templates. + +## Undefined variables + +Fabro uses strict undefined-variable handling. If a workflow template references an unknown value such as `{{ inputs.langauge }}`, validation fails instead of passing the literal text through to the model. + +## Escaping + +To emit literal template syntax, use MiniJinja escaping: + +```dot +test [prompt="{% raw %}{{ goal }}{% endraw %}"] +``` + +You can also emit literal braces with expressions such as `{{ '{{' }}` when needed. ## Input merging -`[run.inputs]` intentionally replaces the inherited map wholesale rather than merging by key. Whichever layer has the highest precedence and sets `[run.inputs]` wins its entire map — lower-precedence inputs do not show through. +`[run.inputs]` intentionally replaces the inherited map wholesale rather than merging by key. Whichever layer has the highest precedence and sets `[run.inputs]` wins its entire map. | Source | Priority | |---|---| diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 63db4ad58..d228c5aa6 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -127,7 +127,7 @@ pub(crate) struct RunArgs { #[arg(long)] pub(crate) auto_approve: bool, - /// Override the workflow goal (exposed as $goal in prompts) + /// Override the workflow goal (available as {{ goal }} in prompts) #[arg(long)] pub(crate) goal: Option, @@ -180,7 +180,7 @@ pub(crate) struct PreflightArgs { /// Path to a .fabro workflow file or .toml task config pub(crate) workflow: PathBuf, - /// Override the workflow goal (exposed as $goal in prompts) + /// Override the workflow goal (available as {{ goal }} in prompts) #[arg(long)] pub(crate) goal: Option, diff --git a/lib/crates/fabro-cli/tests/it/cmd/create.rs b/lib/crates/fabro-cli/tests/it/cmd/create.rs index df1209272..ff7cb4c54 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/create.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/create.rs @@ -45,7 +45,7 @@ fn help() { --dry-run Execute with simulated LLM backend --auto-approve Auto-approve all human gates --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --goal Override the workflow goal (exposed as $goal in prompts) + --goal Override the workflow goal (available as {{ goal }} in prompts) --quiet Suppress non-essential output [env: FABRO_QUIET=] --goal-file Read the workflow goal from a file --model Override default LLM model diff --git a/lib/crates/fabro-cli/tests/it/cmd/preflight.rs b/lib/crates/fabro-cli/tests/it/cmd/preflight.rs index 7f4f2a4e9..b03623511 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/preflight.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/preflight.rs @@ -23,7 +23,7 @@ fn help() { --json Output as JSON [env: FABRO_JSON=] --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --goal Override the workflow goal (exposed as $goal in prompts) + --goal Override the workflow goal (available as {{ goal }} in prompts) --goal-file Read the workflow goal from a file --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] --model Override default LLM model diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index 96929af26..e4e20a973 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -120,7 +120,7 @@ fn help() { --dry-run Execute with simulated LLM backend --auto-approve Auto-approve all human gates --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --goal Override the workflow goal (exposed as $goal in prompts) + --goal Override the workflow goal (available as {{ goal }} in prompts) --quiet Suppress non-essential output [env: FABRO_QUIET=] --goal-file Read the workflow goal from a file --model Override default LLM model diff --git a/lib/crates/fabro-config/tests/resolve_run.rs b/lib/crates/fabro-config/tests/resolve_run.rs index a329dd923..281916ea1 100644 --- a/lib/crates/fabro-config/tests/resolve_run.rs +++ b/lib/crates/fabro-config/tests/resolve_run.rs @@ -27,10 +27,10 @@ fn preserves_goal_variants_and_model_sources() { _version = 1 [run] -working_dir = "${env.FABRO_WORKDIR}" +working_dir = "{{ env.FABRO_WORKDIR }}" [run.goal] -file = "${env.GOAL_FILE}" +file = "{{ env.GOAL_FILE }}" [run.model] provider = "anthropic" @@ -42,13 +42,13 @@ name = "sonnet" match settings.goal { Some(RunGoal::File(path)) => { - assert_eq!(path, InterpString::parse("${env.GOAL_FILE}")); + assert_eq!(path, InterpString::parse("{{ env.GOAL_FILE }}")); } other => panic!("expected file goal, got {other:?}"), } assert_eq!( settings.working_dir, - Some(InterpString::parse("${env.FABRO_WORKDIR}")) + Some(InterpString::parse("{{ env.FABRO_WORKDIR }}")) ); assert_eq!( settings.model.provider, diff --git a/lib/crates/fabro-config/tests/resolve_server.rs b/lib/crates/fabro-config/tests/resolve_server.rs index 211c8a3ff..8a16b349c 100644 --- a/lib/crates/fabro-config/tests/resolve_server.rs +++ b/lib/crates/fabro-config/tests/resolve_server.rs @@ -83,7 +83,7 @@ _version = 1 provider = "s3" [server.artifacts.s3] -endpoint = "${env.S3_ENDPOINT}" +endpoint = "{{ env.S3_ENDPOINT }}" "#, ); @@ -107,11 +107,11 @@ _version = 1 [server.listen] type = "unix" -path = "${env.FABRO_SOCKET}" +path = "{{ env.FABRO_SOCKET }}" [server.integrations.github] -app_id = "${env.GITHUB_APP_ID}" -client_id = "${env.GITHUB_CLIENT_ID}" +app_id = "{{ env.GITHUB_APP_ID }}" +client_id = "{{ env.GITHUB_CLIENT_ID }}" slug = "fabro-app" "#, ); @@ -121,18 +121,18 @@ slug = "fabro-app" match settings.listen { ServerListenSettings::Unix { path } => { - assert_eq!(path, InterpString::parse("${env.FABRO_SOCKET}")); + assert_eq!(path, InterpString::parse("{{ env.FABRO_SOCKET }}")); } ServerListenSettings::Tcp { .. } => panic!("expected unix listen transport"), } assert_eq!( settings.integrations.github.app_id, - Some(InterpString::parse("${env.GITHUB_APP_ID}")) + Some(InterpString::parse("{{ env.GITHUB_APP_ID }}")) ); assert_eq!( settings.integrations.github.client_id, - Some(InterpString::parse("${env.GITHUB_CLIENT_ID}")) + Some(InterpString::parse("{{ env.GITHUB_CLIENT_ID }}")) ); assert_eq!( settings.integrations.github.slug, diff --git a/lib/crates/fabro-hooks/Cargo.toml b/lib/crates/fabro-hooks/Cargo.toml index 11df7a484..f77c65eda 100644 --- a/lib/crates/fabro-hooks/Cargo.toml +++ b/lib/crates/fabro-hooks/Cargo.toml @@ -17,6 +17,7 @@ fabro-agent = { path = "../fabro-agent" } fabro-config = { path = "../fabro-config" } fabro-llm = { path = "../fabro-llm" } fabro-model = { path = "../fabro-model" } +fabro-template = { path = "../fabro-template" } fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } serde.workspace = true diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index c6b240982..53d43e263 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -1,5 +1,6 @@ use std::borrow::Cow; use std::collections::HashMap; +use std::fmt; use std::path::Path; use std::sync::{Arc, LazyLock}; use std::time::Instant; @@ -10,6 +11,8 @@ use fabro_agent::tool_registry::ToolContext; use fabro_llm::client::Client as LlmClient; use fabro_llm::generate::{GenerateParams, generate_object}; use fabro_llm::types::{Message, Request, ToolResult}; +use fabro_template::{TemplateContext, render as render_template}; +use fabro_types::settings::InterpString; use fabro_util::env::{Env, SystemEnv}; use tokio::process::Command as TokioCommand; use tokio::time::timeout as tokio_timeout; @@ -44,45 +47,26 @@ pub trait HookExecutor: Send + Sync { ) -> HookResult; } -/// Interpolate `$VAR` and `${VAR}` references in `value` using environment -/// variables, but only when the variable name appears in `allowed_vars`. -/// Unlisted or missing vars are replaced with the empty string. -pub fn interpolate_env_vars(value: &str, allowed_vars: &[String], env: &dyn Env) -> String { - let mut result = String::with_capacity(value.len()); - let mut chars = value.chars().peekable(); +fn resolve_interp_string(value: &str, env: &E) -> Result +where + E: Env + ?Sized, +{ + InterpString::parse(value) + .resolve(|name| env.var(name).ok()) + .map(|resolved| resolved.value) + .map_err(|error| error.to_string()) +} - while let Some(ch) = chars.next() { - if ch == '$' { - let braced = chars.peek() == Some(&'{'); - if braced { - chars.next(); // consume '{' - } - - let mut var_name = String::new(); - while let Some(&c) = chars.peek() { - if braced { - if c == '}' { - chars.next(); - break; - } - } else if !c.is_ascii_alphanumeric() && c != '_' { - break; - } - var_name.push(c); - chars.next(); - } - - if !var_name.is_empty() && allowed_vars.iter().any(|v| v == &var_name) { - if let Ok(val) = env.var(&var_name) { - result.push_str(&val); - } - } - } else { - result.push(ch); - } - } - - result +fn render_header_template( + value: &str, + allowed_vars: &[String], + env: &E, +) -> Result +where + E: Env + Clone + Send + Sync + fmt::Debug + 'static, +{ + let ctx = TemplateContext::new().with_env_lookup_allowed(env, allowed_vars); + render_template(value, &ctx).map_err(|error| error.to_string()) } /// Executes hooks via shell commands or HTTP POST. @@ -113,13 +97,25 @@ impl HookExecutorImpl { } /// Execute a command hook (sandbox or host). - async fn execute_command( + async fn execute_command( definition: &HookDefinition, command: &str, context: &HookContext, sandbox: &Arc, work_dir: Option<&Path>, - ) -> HookDecision { + env: &E, + ) -> HookDecision + where + E: Env + Clone + Send + Sync + fmt::Debug + 'static, + { + let command = match resolve_interp_string(command, env) { + Ok(command) => command, + Err(error) => { + return HookDecision::Block { + reason: Some(error), + }; + } + }; let context_json = serde_json::to_string(context).unwrap_or_default(); let timeout_ms = u64::try_from(definition.timeout().as_millis()).unwrap(); @@ -143,7 +139,7 @@ impl HookExecutorImpl { env_vars.insert("FABRO_HOOK_CONTEXT".to_string(), ctx_path.clone()); } match sandbox - .exec_command(command, timeout_ms, None, Some(&env_vars), None) + .exec_command(&command, timeout_ms, None, Some(&env_vars), None) .await { Ok(result) => Self::parse_decision(result.exit_code, &result.stdout), @@ -153,7 +149,7 @@ impl HookExecutorImpl { } } else { let mut cmd = TokioCommand::new("sh"); - cmd.arg("-c").arg(command); + cmd.arg("-c").arg(&command); if let Some(wd) = work_dir { cmd.current_dir(wd); } @@ -219,8 +215,8 @@ impl HookExecutorImpl { } /// Resolve a model alias (e.g. "haiku") to a concrete model ID. - fn resolve_model(model: Option<&String>) -> String { - let model_id = model.map_or("haiku", String::as_str); + fn resolve_model(model: Option<&str>) -> String { + let model_id = model.unwrap_or("haiku"); let model_info = fabro_model::Catalog::builtin().get(model_id); model_info.map_or(model_id, |m| m.id.as_str()).to_string() } @@ -250,16 +246,38 @@ impl HookExecutorImpl { } /// Execute a prompt hook: single-turn LLM call returning ok/block. - async fn execute_prompt( + async fn execute_prompt( + definition: &HookDefinition, prompt: &str, - model: Option<&String>, + model: Option<&str>, context: &HookContext, - timeout: std::time::Duration, - ) -> HookDecision { - let resolved_model = Self::resolve_model(model); - let user_msg = Self::build_hook_user_message(prompt, context); + env: &E, + ) -> HookDecision + where + E: Env + Clone + Send + Sync + fmt::Debug + 'static, + { + let prompt = match resolve_interp_string(prompt, env) { + Ok(prompt) => prompt, + Err(error) => { + tracing::warn!(error = %error, "prompt hook prompt env resolution failed, proceeding"); + return HookDecision::Proceed; + } + }; + let model = match model + .map(|model| resolve_interp_string(model, env)) + .transpose() + { + Ok(model) => model, + Err(error) => { + tracing::warn!(error = %error, "prompt hook model env resolution failed, proceeding"); + return HookDecision::Proceed; + } + }; - Self::execute_llm_with_timeout(timeout, "prompt", || async move { + let resolved_model = Self::resolve_model(model.as_deref()); + let user_msg = Self::build_hook_user_message(&prompt, context); + + Self::execute_llm_with_timeout(definition.timeout(), "prompt", || async move { let params = GenerateParams::new(&resolved_model) .system(HOOK_EVALUATOR_SYSTEM_PROMPT) .prompt(user_msg) @@ -293,18 +311,40 @@ impl HookExecutorImpl { /// Reuses the core `ToolRegistry` from `fabro_agent` so the agent hook has /// the same tools (read_file, write_file, shell, grep, glob, etc.) as /// a normal agent session. - async fn execute_agent( + async fn execute_agent( + definition: &HookDefinition, prompt: &str, - model: Option<&String>, + model: Option<&str>, max_tool_rounds: Option, context: &HookContext, sandbox: Arc, - timeout: std::time::Duration, - ) -> HookDecision { - let resolved_model = Self::resolve_model(model); - let user_msg = Self::build_hook_user_message(prompt, context); + env: &E, + ) -> HookDecision + where + E: Env + Clone + Send + Sync + fmt::Debug + 'static, + { + let prompt = match resolve_interp_string(prompt, env) { + Ok(prompt) => prompt, + Err(error) => { + tracing::warn!(error = %error, "agent hook prompt env resolution failed, proceeding"); + return HookDecision::Proceed; + } + }; + let model = match model + .map(|model| resolve_interp_string(model, env)) + .transpose() + { + Ok(model) => model, + Err(error) => { + tracing::warn!(error = %error, "agent hook model env resolution failed, proceeding"); + return HookDecision::Proceed; + } + }; - Self::execute_llm_with_timeout(timeout, "agent", || async move { + let resolved_model = Self::resolve_model(model.as_deref()); + let user_msg = Self::build_hook_user_message(&prompt, context); + + Self::execute_llm_with_timeout(definition.timeout(), "agent", || async move { let client = match LlmClient::from_env().await { Ok(c) => c, Err(e) => { @@ -414,7 +454,7 @@ impl HookExecutorImpl { /// Execute an HTTP hook: POST context JSON and parse the response. /// Fail-open: non-2xx and connection errors return `Proceed`. #[allow(clippy::too_many_arguments)] - async fn execute_http( + async fn execute_http( client: &reqwest::Client, url: &str, headers: Option<&HashMap>, @@ -422,12 +462,27 @@ impl HookExecutorImpl { tls: &TlsMode, context: &HookContext, timeout: std::time::Duration, - env: &dyn Env, - ) -> HookDecision { + env: &E, + ) -> HookDecision + where + E: Env + Clone + Send + Sync + fmt::Debug + 'static, + { + let resolved_url = match resolve_interp_string(url, env) { + Ok(url) => url, + Err(error) => { + tracing::warn!( + url = %url, + error = %error, + "HTTP hook URL env resolution failed, proceeding" + ); + return HookDecision::Proceed; + } + }; + // Enforce URL scheme based on TLS mode match tls { TlsMode::Verify | TlsMode::NoVerify => { - if !url.starts_with("https://") { + if !resolved_url.starts_with("https://") { return HookDecision::Block { reason: Some(format!( "HTTP hook URL must use https:// (tls mode is {tls:?})" @@ -438,11 +493,22 @@ impl HookExecutorImpl { TlsMode::Off => {} } - let mut request = client.post(url).timeout(timeout).json(context); + let mut request = client.post(&resolved_url).timeout(timeout).json(context); if let Some(hdrs) = headers { for (key, value) in hdrs { - let interpolated = interpolate_env_vars(value, allowed_env_vars, env); + let interpolated = match render_header_template(value, allowed_env_vars, env) { + Ok(rendered) => rendered, + Err(error) => { + tracing::warn!( + url = %resolved_url, + header = %key, + error = %error, + "HTTP hook header template render failed, proceeding" + ); + return HookDecision::Proceed; + } + }; request = request.header(key, interpolated); } } @@ -450,14 +516,14 @@ impl HookExecutorImpl { let response = match request.send().await { Ok(resp) => resp, Err(e) => { - tracing::warn!(url, error = %e, "HTTP hook request failed, proceeding"); + tracing::warn!(url = %resolved_url, error = %e, "HTTP hook request failed, proceeding"); return HookDecision::Proceed; } }; if !response.status().is_success() { tracing::warn!( - url, + url = %resolved_url, status = response.status().as_u16(), "HTTP hook returned non-2xx, proceeding" ); @@ -467,7 +533,7 @@ impl HookExecutorImpl { let body = match response.text().await { Ok(text) => text, Err(e) => { - tracing::warn!(url, error = %e, "HTTP hook body read failed, proceeding"); + tracing::warn!(url = %resolved_url, error = %e, "HTTP hook body read failed, proceeding"); return HookDecision::Proceed; } }; @@ -479,7 +545,7 @@ impl HookExecutorImpl { match serde_json::from_str::(body.trim()) { Ok(decision) => decision, Err(e) => { - tracing::warn!(url, error = %e, "HTTP hook response parse failed, proceeding"); + tracing::warn!(url = %resolved_url, error = %e, "HTTP hook response parse failed, proceeding"); HookDecision::Proceed } } @@ -530,12 +596,15 @@ impl HookExecutor for HookExecutorImpl { static HTTP_CLIENTS: OnceLock = OnceLock::new(); let start = Instant::now(); + let env = SystemEnv; let decision = match definition.resolved_hook_type() { Some( Cow::Borrowed(HookType::Command { ref command }) | Cow::Owned(HookType::Command { ref command }), - ) => Self::execute_command(definition, command, context, &sandbox, work_dir).await, + ) => { + Self::execute_command(definition, command, context, &sandbox, work_dir, &env).await + } Some( Cow::Borrowed(HookType::Http { ref url, @@ -559,7 +628,7 @@ impl HookExecutor for HookExecutorImpl { tls, context, definition.timeout(), - &SystemEnv, + &env, ) .await } @@ -572,7 +641,7 @@ impl HookExecutor for HookExecutorImpl { ref prompt, ref model, }), - ) => Self::execute_prompt(prompt, model.as_ref(), context, definition.timeout()).await, + ) => Self::execute_prompt(definition, prompt, model.as_deref(), context, &env).await, Some( Cow::Borrowed(HookType::Agent { ref prompt, @@ -586,12 +655,13 @@ impl HookExecutor for HookExecutorImpl { }), ) => { Self::execute_agent( + definition, prompt, - model.as_ref(), + model.as_deref(), *max_tool_rounds, context, sandbox, - definition.timeout(), + &env, ) .await } @@ -850,7 +920,7 @@ mod tests { ); } - // --- interpolate_env_vars tests --- + // --- hook template helpers --- fn test_env(vars: &[(&str, &str)]) -> TestEnv { TestEnv( @@ -861,60 +931,46 @@ mod tests { } #[test] - fn interpolate_resolves_allowed_var() { + fn render_header_template_resolves_allowlisted_var() { let env = test_env(&[("FABRO_TEST_KEY_1", "secret123")]); - let result = interpolate_env_vars( - "Bearer $FABRO_TEST_KEY_1", + let result = render_header_template( + "Bearer {{ env.FABRO_TEST_KEY_1 }}", &["FABRO_TEST_KEY_1".to_string()], &env, - ); + ) + .unwrap(); assert_eq!(result, "Bearer secret123"); } #[test] - fn interpolate_resolves_braced_var() { + fn render_header_template_rejects_unlisted_var() { + let env = test_env(&[("FABRO_TEST_KEY_3", "should_not_appear")]); + let err = render_header_template("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix", &[], &env) + .unwrap_err(); + assert!(err.contains("undefined")); + } + + #[test] + fn resolve_interp_string_resolves_embedded_var() { let env = test_env(&[("FABRO_TEST_KEY_2", "val")]); - let result = interpolate_env_vars( - "x${FABRO_TEST_KEY_2}y", - &["FABRO_TEST_KEY_2".to_string()], - &env, - ); + let result = resolve_interp_string("x{{ env.FABRO_TEST_KEY_2 }}y", &env).unwrap(); assert_eq!(result, "xvaly"); } #[test] - fn interpolate_unlisted_var_becomes_empty() { - let env = test_env(&[("FABRO_TEST_KEY_3", "should_not_appear")]); - let result = interpolate_env_vars("prefix-$FABRO_TEST_KEY_3-suffix", &[], &env); - assert_eq!(result, "prefix--suffix"); - } - - #[test] - fn interpolate_missing_var_becomes_empty() { + fn resolve_interp_string_errors_on_missing_var() { let env = test_env(&[]); - let result = interpolate_env_vars( - "a$FABRO_TEST_NOEXIST-b", - &["FABRO_TEST_NOEXIST".to_string()], - &env, - ); - assert_eq!(result, "a-b"); + let err = resolve_interp_string("a{{ env.FABRO_TEST_NOEXIST }}-b", &env).unwrap_err(); + assert!(err.contains("FABRO_TEST_NOEXIST")); } #[test] - fn interpolate_no_vars_passes_through() { + fn resolve_interp_string_without_vars_passes_through() { let env = test_env(&[]); - assert_eq!(interpolate_env_vars("plain text", &[], &env), "plain text"); - } - - #[test] - fn interpolate_mixed_text() { - let env = test_env(&[("FABRO_TEST_A", "hello"), ("FABRO_TEST_B", "world")]); - let result = interpolate_env_vars( - "$FABRO_TEST_A ${FABRO_TEST_B}!", - &["FABRO_TEST_A".to_string(), "FABRO_TEST_B".to_string()], - &env, + assert_eq!( + resolve_interp_string("plain text", &env).unwrap(), + "plain text" ); - assert_eq!(result, "hello world!"); } // --- HTTP hook execution tests --- @@ -1042,7 +1098,7 @@ mod tests { let headers = HashMap::from([( "Authorization".to_string(), - "Bearer $FABRO_TEST_TOKEN".to_string(), + "Bearer {{ env.FABRO_TEST_TOKEN }}".to_string(), )]); let client = test_http_client(); @@ -1062,6 +1118,34 @@ mod tests { assert_eq!(decision, HookDecision::Proceed); } + #[tokio::test] + async fn http_hook_resolves_url_before_dispatch() { + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method("POST").path("/hook"); + then.status(200).body(""); + }) + .await; + + let client = test_http_client(); + let env = test_env(&[("FABRO_TEST_URL", &server.url("/hook"))]); + let decision = HookExecutorImpl::execute_http( + &client, + "{{ env.FABRO_TEST_URL }}", + None, + &[], + &TlsMode::Off, + &make_context(), + std::time::Duration::from_secs(5), + &env, + ) + .await; + + mock.assert_async().await; + assert_eq!(decision, HookDecision::Proceed); + } + // --- TLS mode enforcement tests --- #[tokio::test] @@ -1161,4 +1245,50 @@ mod tests { assert_eq!(result.decision, HookDecision::Proceed); assert_eq!(result.hook_name.as_deref(), Some("http-test")); } + + #[tokio::test] + async fn command_hook_missing_env_blocks() { + let sandbox = make_sandbox(); + let decision = HookExecutorImpl::execute_command( + &make_definition("echo {{ env.MISSING_HOOK_VALUE }}"), + "echo {{ env.MISSING_HOOK_VALUE }}", + &make_context(), + &sandbox, + None, + &test_env(&[]), + ) + .await; + + assert!(matches!(decision, HookDecision::Block { .. })); + } + + #[tokio::test] + async fn prompt_hook_missing_env_proceeds() { + let decision = HookExecutorImpl::execute_prompt( + &make_definition("unused"), + "{{ env.MISSING_HOOK_VALUE }}", + None, + &make_context(), + &test_env(&[]), + ) + .await; + + assert_eq!(decision, HookDecision::Proceed); + } + + #[tokio::test] + async fn agent_hook_missing_env_proceeds() { + let decision = HookExecutorImpl::execute_agent( + &make_definition("unused"), + "{{ env.MISSING_HOOK_VALUE }}", + None, + Some(1), + &make_context(), + make_sandbox(), + &test_env(&[]), + ) + .await; + + assert_eq!(decision, HookDecision::Proceed); + } } diff --git a/lib/crates/fabro-server/tests/it/api/runs.rs b/lib/crates/fabro-server/tests/it/api/runs.rs index 0778f296e..a79a6d86b 100644 --- a/lib/crates/fabro-server/tests/it/api/runs.rs +++ b/lib/crates/fabro-server/tests/it/api/runs.rs @@ -28,7 +28,7 @@ ca = "/etc/fabro/tls/ca.pem" [server.auth.api.jwt] enabled = true issuer = "https://auth.example.com" -audience = "${{env.JWT_AUDIENCE}}" +audience = "{{{{ env.JWT_AUDIENCE }}}}" [server.auth.api.mtls] enabled = true @@ -37,7 +37,7 @@ ca = "/etc/fabro/tls/ca.pem" [server.auth.web.providers.github] enabled = true client_id = "Iv1.abcdef" -client_secret = "${{env.GITHUB_OAUTH_SECRET}}" +client_secret = "{{{{ env.GITHUB_OAUTH_SECRET }}}}" [server.storage] root = "{}" @@ -46,7 +46,7 @@ root = "{}" max_concurrent_runs = 9 [server.integrations.github] -app_id = "${{env.GITHUB_APP_ID}}" +app_id = "{{{{ env.GITHUB_APP_ID }}}}" client_id = "Iv1.github" slug = "fabro-app" "#, @@ -107,7 +107,7 @@ session_sandboxes = true assert_eq!(body["server"]["scheduler"]["max_concurrent_runs"], 9); assert_eq!( body["server"]["integrations"]["github"]["app_id"], - "${env.GITHUB_APP_ID}" + "{{ env.GITHUB_APP_ID }}" ); assert_eq!( body["server"]["auth"]["api"]["jwt"]["enabled"], diff --git a/lib/crates/fabro-template/Cargo.toml b/lib/crates/fabro-template/Cargo.toml new file mode 100644 index 000000000..61e1dc75a --- /dev/null +++ b/lib/crates/fabro-template/Cargo.toml @@ -0,0 +1,21 @@ +[package] +name = "fabro-template" +edition.workspace = true +version.workspace = true +publish = false +license.workspace = true +description = "Shared MiniJinja-based template rendering for Fabro" + +[lib] +doctest = false + +[lints] +workspace = true + +[dependencies] +anyhow.workspace = true +fabro-util = { path = "../fabro-util" } +minijinja.workspace = true +serde.workspace = true +thiserror.workspace = true +toml.workspace = true diff --git a/lib/crates/fabro-template/src/lib.rs b/lib/crates/fabro-template/src/lib.rs new file mode 100644 index 000000000..40f68fb6b --- /dev/null +++ b/lib/crates/fabro-template/src/lib.rs @@ -0,0 +1,278 @@ +use std::collections::HashMap; +use std::fmt; +use std::sync::Arc; + +use fabro_util::env::Env; +use minijinja::value::{Object, Value}; +use minijinja::{AutoEscape, Environment, ErrorKind, UndefinedBehavior}; +use thiserror::Error; + +#[derive(Debug, Default, Clone)] +pub struct TemplateContext { + goal: Option, + inputs: HashMap, + env: Option, +} + +impl TemplateContext { + #[must_use] + pub fn new() -> Self { + Self::default() + } + + #[must_use] + pub fn with_goal(mut self, goal: impl Into) -> Self { + self.goal = Some(goal.into()); + self + } + + #[must_use] + pub fn with_inputs(mut self, inputs: HashMap) -> Self { + self.inputs = inputs; + self + } + + #[must_use] + pub fn with_env_lookup(mut self, env: &E) -> Self + where + E: Env + Clone + Send + Sync + fmt::Debug + 'static, + { + self.env = Some(Value::from_object(EnvLookup { + env: env.clone(), + allowlist: None, + })); + self + } + + #[must_use] + pub fn with_env_lookup_allowed(mut self, env: &E, allowlist: &[String]) -> Self + where + E: Env + Clone + Send + Sync + fmt::Debug + 'static, + { + self.env = Some(Value::from_object(EnvLookup { + env: env.clone(), + allowlist: Some(allowlist.to_vec()), + })); + self + } + + fn into_value(self) -> Value { + let goal = self.goal.map(Value::from); + let inputs = Value::from_serialize(self.inputs); + let env = self.env; + Value::from_object(RenderContext { goal, inputs, env }) + } +} + +#[derive(Debug, Clone)] +struct RenderContext { + goal: Option, + inputs: Value, + env: Option, +} + +impl Object for RenderContext { + fn get_value_by_str(self: &Arc, key: &str) -> Option { + match key { + "goal" => self.goal.clone(), + "inputs" => Some(self.inputs.clone()), + "env" => self.env.clone(), + _ => None, + } + } +} + +#[derive(Debug, Clone)] +pub struct EnvLookup { + env: E, + allowlist: Option>, +} + +impl Object for EnvLookup +where + E: Env + Send + Sync + fmt::Debug + 'static, +{ + fn get_value_by_str(self: &Arc, key: &str) -> Option { + if let Some(allowlist) = &self.allowlist { + if !allowlist.iter().any(|allowed| allowed == key) { + return None; + } + } + + self.env.var(key).ok().map(Value::from) + } +} + +#[derive(Debug, Error, Clone, PartialEq, Eq)] +pub enum TemplateError { + #[error("template syntax error: {message}")] + Syntax { message: String }, + #[error("template referenced an undefined variable: {message}")] + UndefinedVariable { message: String }, + #[error("template render error: {message}")] + Render { message: String }, +} + +impl From for TemplateError { + fn from(error: minijinja::Error) -> Self { + let message = error.to_string(); + match error.kind() { + ErrorKind::SyntaxError => Self::Syntax { message }, + ErrorKind::UndefinedError => Self::UndefinedVariable { message }, + _ => Self::Render { message }, + } + } +} + +pub fn render(template: &str, ctx: &TemplateContext) -> Result { + let mut env = Environment::new(); + env.set_undefined_behavior(UndefinedBehavior::Strict); + env.set_auto_escape_callback(|_| AutoEscape::None); + env.render_str(template, ctx.clone().into_value()) + .map_err(TemplateError::from) +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use fabro_util::env::TestEnv; + + use super::*; + + #[test] + fn renders_simple_goal_variable() { + let ctx = TemplateContext::new().with_goal("Fix bugs"); + + let rendered = render("Goal: {{ goal }}", &ctx).unwrap(); + + assert_eq!(rendered, "Goal: Fix bugs"); + } + + #[test] + fn renders_typed_input_values() { + let ctx = TemplateContext::new().with_inputs(HashMap::from([ + ("enabled".to_string(), toml::Value::Boolean(true)), + ("count".to_string(), toml::Value::Integer(3)), + ])); + + let rendered = render( + "{% if inputs.enabled %}count={{ inputs.count }}{% endif %}", + &ctx, + ) + .unwrap(); + + assert_eq!(rendered, "count=3"); + } + + #[test] + fn renders_nested_input_variable() { + let ctx = TemplateContext::new().with_inputs(HashMap::from([( + "repo".to_string(), + toml::Value::Table(toml::map::Map::from_iter([( + "name".to_string(), + toml::Value::String("fabro".to_string()), + )])), + )])); + + let rendered = render("Repo {{ inputs.repo.name }}", &ctx).unwrap(); + + assert_eq!(rendered, "Repo fabro"); + } + + #[test] + fn renders_env_variable() { + let env = TestEnv(HashMap::from([( + "API_KEY".to_string(), + "secret".to_string(), + )])); + let ctx = TemplateContext::new().with_env_lookup(&env); + + let rendered = render("{{ env.API_KEY }}", &ctx).unwrap(); + + assert_eq!(rendered, "secret"); + } + + #[test] + fn renders_allowlisted_env_variable() { + let env = TestEnv(HashMap::from([("TOKEN".to_string(), "abc123".to_string())])); + let ctx = TemplateContext::new().with_env_lookup_allowed(&env, &["TOKEN".to_string()]); + + let rendered = render("Bearer {{ env.TOKEN }}", &ctx).unwrap(); + + assert_eq!(rendered, "Bearer abc123"); + } + + #[test] + fn rejects_non_allowlisted_env_variable() { + let env = TestEnv(HashMap::from([("SECRET".to_string(), "shh".to_string())])); + let ctx = TemplateContext::new().with_env_lookup_allowed(&env, &[]); + + let err = render("{{ env.SECRET }}", &ctx).unwrap_err(); + + assert!(matches!(err, TemplateError::UndefinedVariable { .. })); + } + + #[test] + fn rejects_undefined_variables_in_strict_mode() { + let ctx = TemplateContext::new(); + + let err = render("{{ missing }}", &ctx).unwrap_err(); + + assert!(matches!(err, TemplateError::UndefinedVariable { .. })); + } + + #[test] + fn supports_partial_interpolation() { + let ctx = TemplateContext::new().with_goal("ship it"); + + let rendered = render("Please {{ goal }} today", &ctx).unwrap(); + + assert_eq!(rendered, "Please ship it today"); + } + + #[test] + fn preserves_passthrough_goal_literal() { + let ctx = TemplateContext::new().with_goal("{{ goal }}"); + + let rendered = render("{{ goal }}", &ctx).unwrap(); + + assert_eq!(rendered, "{{ goal }}"); + } + + #[test] + fn renders_empty_goal() { + let ctx = TemplateContext::new().with_goal(""); + + let rendered = render("Goal={{ goal }}", &ctx).unwrap(); + + assert_eq!(rendered, "Goal="); + } + + #[test] + fn leaves_dollar_signs_untouched() { + let ctx = TemplateContext::new().with_goal("ignored"); + + let rendered = render("price is $5", &ctx).unwrap(); + + assert_eq!(rendered, "price is $5"); + } + + #[test] + fn passes_through_plain_text() { + let ctx = TemplateContext::new(); + + let rendered = render("just text", &ctx).unwrap(); + + assert_eq!(rendered, "just text"); + } + + #[test] + fn supports_raw_block_escape() { + let ctx = TemplateContext::new(); + + let rendered = render("{% raw %}{{ goal }}{% endraw %}", &ctx).unwrap(); + + assert_eq!(rendered, "{{ goal }}"); + } +} diff --git a/lib/crates/fabro-types/src/settings/interp.rs b/lib/crates/fabro-types/src/settings/interp.rs index ef8e43b3a..5a3a8d258 100644 --- a/lib/crates/fabro-types/src/settings/interp.rs +++ b/lib/crates/fabro-types/src/settings/interp.rs @@ -1,6 +1,6 @@ //! Env var interpolation for config strings. //! -//! Any string field may use `${env.NAME}` tokens, either as a whole value or +//! Any string field may use `{{ env.NAME }}` tokens, either as a whole value or //! as one or more substrings inside a larger string. Resolution happens only //! when the field is consumed, and provenance tracking lets outward-facing //! renderers redact env-sourced values uniformly. @@ -10,7 +10,7 @@ use std::fmt; use serde::de::{self, Visitor}; use serde::{Deserialize, Deserializer, Serialize, Serializer}; -/// A config string that may contain `${env.NAME}` tokens. +/// A config string that may contain `{{ env.NAME }}` tokens. #[derive(Debug, Clone, PartialEq, Eq)] pub struct InterpString { segments: Vec, @@ -23,6 +23,30 @@ enum Segment { } impl InterpString { + fn push_literal(segments: &mut Vec, text: &str) { + if text.is_empty() { + return; + } + + match segments.last_mut() { + Some(Segment::Literal(existing)) => existing.push_str(text), + Some(Segment::EnvVar(_)) | None => segments.push(Segment::Literal(text.to_owned())), + } + } + + fn parse_env_token(token: &str) -> Option { + let trimmed = token.trim(); + let name = trimmed.strip_prefix("env.")?; + if name.is_empty() + || !name + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_') + { + return None; + } + Some(name.to_owned()) + } + /// Parse a raw string into its literal/env-var segments. /// /// Parsing is infallible: the token grammar is intentionally permissive so @@ -32,26 +56,28 @@ impl InterpString { let mut segments: Vec = Vec::new(); let mut rest = input; - while let Some(start) = rest.find("${env.") { - if start > 0 { - segments.push(Segment::Literal(rest[..start].to_owned())); - } - // rest[start..] begins with "${env." - let after_prefix = &rest[start + "${env.".len()..]; - if let Some(close) = after_prefix.find('}') { - let name = after_prefix[..close].to_owned(); - segments.push(Segment::EnvVar(name)); - rest = &after_prefix[close + 1..]; + while let Some(start) = rest.find("{{") { + Self::push_literal(&mut segments, &rest[..start]); + + let after_open = &rest[start + 2..]; + if let Some(close) = after_open.find("}}") { + let token = &after_open[..close]; + if let Some(name) = Self::parse_env_token(token) { + segments.push(Segment::EnvVar(name)); + } else { + Self::push_literal(&mut segments, &rest[start..start + 2 + close + 2]); + } + rest = &after_open[close + 2..]; } else { // Unterminated token — treat the remainder as literal text. - segments.push(Segment::Literal(rest[start..].to_owned())); + Self::push_literal(&mut segments, &rest[start..]); rest = ""; break; } } if !rest.is_empty() { - segments.push(Segment::Literal(rest.to_owned())); + Self::push_literal(&mut segments, rest); } if segments.is_empty() { @@ -97,9 +123,9 @@ impl InterpString { match seg { Segment::Literal(text) => out.push_str(text), Segment::EnvVar(name) => { - out.push_str("${env."); + out.push_str("{{ env."); out.push_str(name); - out.push('}'); + out.push_str(" }}"); } } } @@ -179,7 +205,7 @@ impl fmt::Display for ResolveEnvError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( f, - "environment variable {:?} referenced by ${{env.{}}} is not set", + "environment variable {:?} referenced by {{{{ env.{} }}}} is not set", self.name, self.name ) } @@ -201,7 +227,7 @@ impl<'de> Deserialize<'de> for InterpString { type Value = InterpString; fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str("a string, optionally containing ${env.NAME} interpolation tokens") + f.write_str("a string, optionally containing {{ env.NAME }} interpolation tokens") } fn visit_str(self, value: &str) -> Result { @@ -241,21 +267,21 @@ mod tests { #[test] fn whole_value_env_reference() { - let s = InterpString::parse("${env.API_KEY}"); + let s = InterpString::parse("{{ env.API_KEY }}"); assert!(!s.is_literal()); assert_eq!(s.env_var_names(), vec!["API_KEY"]); - assert_eq!(s.as_source(), "${env.API_KEY}"); + assert_eq!(s.as_source(), "{{ env.API_KEY }}"); } #[test] fn substring_env_reference() { - let s = InterpString::parse("Bearer ${env.TOKEN}"); + let s = InterpString::parse("Bearer {{ env.TOKEN }}"); assert_eq!(s.env_var_names(), vec!["TOKEN"]); } #[test] fn multi_token_env_reference() { - let s = InterpString::parse("${env.USER}@${env.HOST}:${env.PORT}"); + let s = InterpString::parse("{{ env.USER }}@{{ env.HOST }}:{{env.PORT}}"); assert_eq!(s.env_var_names(), vec!["USER", "HOST", "PORT"]); } @@ -269,7 +295,7 @@ mod tests { #[test] fn resolve_whole_value() { - let s = InterpString::parse("${env.API_KEY}"); + let s = InterpString::parse("{{ env.API_KEY }}"); let resolved = s .resolve(lookup_from(&[("API_KEY", "secret-123")])) .unwrap(); @@ -284,14 +310,14 @@ mod tests { #[test] fn resolve_substring() { - let s = InterpString::parse("Bearer ${env.TOKEN}"); + let s = InterpString::parse("Bearer {{ env.TOKEN }}"); let resolved = s.resolve(lookup_from(&[("TOKEN", "abc")])).unwrap(); assert_eq!(resolved.value, "Bearer abc"); } #[test] fn resolve_multiple_tokens() { - let s = InterpString::parse("${env.USER}@${env.HOST}"); + let s = InterpString::parse("{{ env.USER }}@{{ env.HOST }}"); let resolved = s .resolve(lookup_from(&[("USER", "root"), ("HOST", "example.com")])) .unwrap(); @@ -306,16 +332,16 @@ mod tests { #[test] fn resolve_missing_env_fails_with_name() { - let s = InterpString::parse("${env.MISSING}"); + let s = InterpString::parse("{{ env.MISSING }}"); let err = s.resolve(lookup_from(&[])).unwrap_err(); assert_eq!(err.name, "MISSING"); } #[test] fn unterminated_token_treated_as_literal() { - let s = InterpString::parse("${env.OPEN"); + let s = InterpString::parse("{{ env.OPEN"); let resolved = s.resolve(lookup_from(&[])).unwrap(); - assert_eq!(resolved.value, "${env.OPEN"); + assert_eq!(resolved.value, "{{ env.OPEN"); assert_eq!(resolved.provenance, Provenance::Literal); } @@ -326,9 +352,9 @@ mod tests { s: InterpString, } - let input = r#"{"s":"Bearer ${env.TOKEN}"}"#; + let input = r#"{"s":"Bearer {{ env.TOKEN }}"}"#; let parsed: Wrap = serde_json::from_str(input).unwrap(); - assert_eq!(parsed.s.as_source(), "Bearer ${env.TOKEN}"); + assert_eq!(parsed.s.as_source(), "Bearer {{ env.TOKEN }}"); let rendered = serde_json::to_string(&parsed).unwrap(); assert_eq!(rendered, input); } diff --git a/lib/crates/fabro-types/tests/run_event_serde.rs b/lib/crates/fabro-types/tests/run_event_serde.rs index f081d6da9..15947155a 100644 --- a/lib/crates/fabro-types/tests/run_event_serde.rs +++ b/lib/crates/fabro-types/tests/run_event_serde.rs @@ -14,17 +14,17 @@ fn templated_settings() -> SettingsLayer { version: Some(1), run: Some(RunLayer { goal: Some(RunGoalLayer::Inline(InterpString::parse( - "Ship ${env.TASK}", + "Ship {{ env.TASK }}", ))), ..RunLayer::default() }), server: Some(ServerLayer { storage: Some(ServerStorageLayer { - root: Some(InterpString::parse("${env.FABRO_STORAGE}")), + root: Some(InterpString::parse("{{ env.FABRO_STORAGE }}")), }), integrations: Some(ServerIntegrationsLayer { github: Some(GithubIntegrationLayer { - app_id: Some(InterpString::parse("${env.GITHUB_APP_ID}")), + app_id: Some(InterpString::parse("{{ env.GITHUB_APP_ID }}")), ..GithubIntegrationLayer::default() }), ..ServerIntegrationsLayer::default() @@ -41,7 +41,7 @@ fn run_created_props_round_trip_templated_settings() { settings: templated_settings(), graph: Graph::new("ship"), workflow_source: Some("digraph Ship { start -> exit }".to_string()), - workflow_config: Some("[run]\ngoal = \"Ship ${env.TASK}\"".to_string()), + workflow_config: Some("[run]\ngoal = \"Ship {{ env.TASK }}\"".to_string()), labels: BTreeMap::from([("team".to_string(), "platform".to_string())]), run_dir: "/tmp/run".to_string(), working_directory: "/tmp/project".to_string(), @@ -69,7 +69,7 @@ fn run_created_props_round_trip_templated_settings() { .as_ref() .and_then(|run| run.goal.as_ref()), Some(&RunGoalLayer::Inline(InterpString::parse( - "Ship ${env.TASK}" + "Ship {{ env.TASK }}" ))) ); assert_eq!( @@ -80,7 +80,7 @@ fn run_created_props_round_trip_templated_settings() { .and_then(|server| server.storage.as_ref()) .and_then(|storage| storage.root.as_ref()) .map(InterpString::as_source), - Some("${env.FABRO_STORAGE}".to_string()) + Some("{{ env.FABRO_STORAGE }}".to_string()) ); assert_eq!( round_trip @@ -91,6 +91,6 @@ fn run_created_props_round_trip_templated_settings() { .and_then(|integrations| integrations.github.as_ref()) .and_then(|github| github.app_id.as_ref()) .map(InterpString::as_source), - Some("${env.GITHUB_APP_ID}".to_string()) + Some("{{ env.GITHUB_APP_ID }}".to_string()) ); } diff --git a/lib/crates/fabro-types/tests/run_record_serde.rs b/lib/crates/fabro-types/tests/run_record_serde.rs index 5e02ed304..98e536836 100644 --- a/lib/crates/fabro-types/tests/run_record_serde.rs +++ b/lib/crates/fabro-types/tests/run_record_serde.rs @@ -16,17 +16,17 @@ fn templated_settings() -> SettingsLayer { version: Some(1), run: Some(RunLayer { goal: Some(RunGoalLayer::Inline(InterpString::parse( - "Ship ${env.TASK}", + "Ship {{ env.TASK }}", ))), ..RunLayer::default() }), server: Some(ServerLayer { storage: Some(ServerStorageLayer { - root: Some(InterpString::parse("${env.FABRO_STORAGE}")), + root: Some(InterpString::parse("{{ env.FABRO_STORAGE }}")), }), integrations: Some(ServerIntegrationsLayer { github: Some(GithubIntegrationLayer { - app_id: Some(InterpString::parse("${env.GITHUB_APP_ID}")), + app_id: Some(InterpString::parse("{{ env.GITHUB_APP_ID }}")), ..GithubIntegrationLayer::default() }), ..ServerIntegrationsLayer::default() @@ -69,7 +69,7 @@ fn run_record_round_trips_templated_settings() { .as_ref() .and_then(|run| run.goal.as_ref()), Some(&RunGoalLayer::Inline(InterpString::parse( - "Ship ${env.TASK}" + "Ship {{ env.TASK }}" ))) ); assert_eq!( @@ -80,7 +80,7 @@ fn run_record_round_trips_templated_settings() { .and_then(|server| server.storage.as_ref()) .and_then(|storage| storage.root.as_ref()) .map(InterpString::as_source), - Some("${env.FABRO_STORAGE}".to_string()) + Some("{{ env.FABRO_STORAGE }}".to_string()) ); assert_eq!( round_trip @@ -91,6 +91,6 @@ fn run_record_round_trips_templated_settings() { .and_then(|integrations| integrations.github.as_ref()) .and_then(|github| github.app_id.as_ref()) .map(InterpString::as_source), - Some("${env.GITHUB_APP_ID}".to_string()) + Some("{{ env.GITHUB_APP_ID }}".to_string()) ); } diff --git a/lib/crates/fabro-util/src/env.rs b/lib/crates/fabro-util/src/env.rs index 3c2dcfebc..e3109fed3 100644 --- a/lib/crates/fabro-util/src/env.rs +++ b/lib/crates/fabro-util/src/env.rs @@ -8,6 +8,7 @@ pub trait Env: Send + Sync { } /// Reads real process environment variables. +#[derive(Clone, Debug)] pub struct SystemEnv; impl Env for SystemEnv { @@ -20,6 +21,7 @@ impl Env for SystemEnv { /// /// Intended for use in tests across the workspace. Unconditionally compiled /// because it is trivial and has no external dependencies. +#[derive(Clone, Debug)] pub struct TestEnv(pub std::collections::HashMap); impl Env for TestEnv { diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index 729001040..f4cc27c9c 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -28,6 +28,7 @@ fabro-sandbox = { path = "../fabro-sandbox", features = ["daytona"] } fabro-mcp = { path = "../fabro-mcp" } fabro-github = { path = "../fabro-github" } fabro-interview = { path = "../fabro-interview" } +fabro-template = { path = "../fabro-template" } fabro-util = { path = "../fabro-util" } fabro-checkpoint = { path = "../fabro-checkpoint" } fabro-llm = { path = "../fabro-llm" } @@ -62,9 +63,9 @@ tracing.workspace = true walkdir.workspace = true reqwest.workspace = true tempfile = "3" +toml.workspace = true [dev-dependencies] base64.workspace = true -toml.workspace = true fabro-mcp = { path = "../fabro-mcp" } tokio = { workspace = true, features = ["test-util", "macros"] } object_store.workspace = true diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 484f65b5e..7deaad358 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use async_trait::async_trait; use fabro_agent::Sandbox; use fabro_model::Provider; +use fabro_template::{TemplateContext, render as render_template}; use fabro_types::RunId; use crate::context::keys; @@ -14,7 +15,6 @@ use crate::event::{Emitter, Event, StageScope}; use crate::outcome::{ BilledModelUsage, FailureCategory, FailureDetail, Outcome, OutcomeExt, StageStatus, }; -use crate::transforms::variable_expansion::expand_vars; use fabro_graphviz::graph::{Graph, Node}; use super::{EngineServices, Handler}; @@ -71,14 +71,16 @@ impl AgentHandler { } } -/// Expand `$variable` placeholders in text using graph attributes. -/// -/// Known variables are built from graph attributes (e.g. `$goal`). Any -/// `$identifier` not in the map produces an error, catching typos like -/// `$gaol` at runtime. -pub(crate) fn expand_variables(text: &str, graph: &Graph) -> Result { - let vars = HashMap::from([("goal".to_string(), graph.goal().to_string())]); - expand_vars(text, &vars).map_err(|e| FabroError::Validation(e.to_string())) +/// Expand `{{ goal }}` / `{{ inputs.* }}` placeholders in handler prompts. +pub(crate) fn expand_variables( + text: &str, + graph: &Graph, + inputs: &HashMap, +) -> Result { + let ctx = TemplateContext::new() + .with_goal(graph.goal()) + .with_inputs(inputs.clone()); + render_template(text, &ctx).map_err(|error| FabroError::Validation(error.to_string())) } /// Status fields that indicate a JSON object contains routing directives. @@ -241,7 +243,7 @@ impl Handler for AgentHandler { .prompt() .filter(|p| !p.is_empty()) .unwrap_or_else(|| node.label()); - let expanded = expand_variables(raw_prompt, graph)?; + let expanded = expand_variables(raw_prompt, graph, &services.inputs)?; let preamble = context.preamble(); let prompt = if preamble.is_empty() { expanded @@ -476,7 +478,7 @@ mod tests { let mut node = Node::new("plan"); node.attrs.insert( "prompt".to_string(), - AttrValue::String("Achieve: $goal".to_string()), + AttrValue::String("Achieve: {{ goal }}".to_string()), ); let context = test_context(); let mut graph = Graph::new("test"); @@ -765,16 +767,17 @@ mod tests { "goal".to_string(), AttrValue::String("Fix bugs".to_string()), ); - let result = expand_variables("Goal is: $goal, do it", &graph).unwrap(); + let result = + expand_variables("Goal is: {{ goal }}, do it", &graph, &HashMap::new()).unwrap(); assert_eq!(result, "Goal is: Fix bugs, do it"); } #[test] fn expand_variables_errors_on_unknown_variable() { let graph = Graph::new("test"); - let err = expand_variables("Do $foo now", &graph).unwrap_err(); + let err = expand_variables("Do {{ inputs.foo }} now", &graph, &HashMap::new()).unwrap_err(); assert!( - err.to_string().contains("Undefined variable: $foo"), + err.to_string().contains("undefined"), "unexpected error: {err}" ); } @@ -782,14 +785,14 @@ mod tests { #[test] fn expand_variables_allows_bare_dollar() { let graph = Graph::new("test"); - let result = expand_variables("costs $5", &graph).unwrap(); + let result = expand_variables("costs $5", &graph, &HashMap::new()).unwrap(); assert_eq!(result, "costs $5"); } #[test] fn expand_variables_allows_dollar_alone() { let graph = Graph::new("test"); - let result = expand_variables("just a $ sign", &graph).unwrap(); + let result = expand_variables("just a $ sign", &graph, &HashMap::new()).unwrap(); assert_eq!(result, "just a $ sign"); } diff --git a/lib/crates/fabro-workflow/src/handler/manager_loop.rs b/lib/crates/fabro-workflow/src/handler/manager_loop.rs index e89a800e5..fb41d0b76 100644 --- a/lib/crates/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflow/src/handler/manager_loop.rs @@ -232,6 +232,7 @@ impl Handler for SubWorkflowHandler { let registry = Arc::clone(&services.registry); let hook_runner = services.hook_runner.clone(); let env = services.env.clone(); + let inputs = services.inputs.clone(); let dry_run = services.dry_run; let workflow_bundle = services.workflow_bundle.clone(); let object_store = Arc::new(InMemory::new()); @@ -251,6 +252,7 @@ impl Handler for SubWorkflowHandler { let initialized = Initialized { graph: child_graph, source: String::new(), + inputs, run_options: child_run_options, workflow_path: child_workflow_path, workflow_bundle, diff --git a/lib/crates/fabro-workflow/src/handler/mod.rs b/lib/crates/fabro-workflow/src/handler/mod.rs index 59ec4e801..4a317de84 100644 --- a/lib/crates/fabro-workflow/src/handler/mod.rs +++ b/lib/crates/fabro-workflow/src/handler/mod.rs @@ -51,6 +51,8 @@ pub struct EngineServices { pub hook_runner: Option>, /// Environment variables from `[sandbox.env]` config, injected into command nodes. pub env: HashMap, + /// Typed values from `[run.inputs]`, available to prompt templates. + pub inputs: HashMap, /// When true, handlers should skip real execution and return simulated results. pub dry_run: bool, /// Optional run-scoped cancellation flag from the core executor. @@ -120,6 +122,7 @@ impl EngineServices { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + inputs: HashMap::new(), dry_run: false, cancel_requested: None, workflow_path: None, diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index de674fe9a..761e66c5a 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -290,6 +290,7 @@ impl Handler for ParallelHandler { let hook_runner = services.hook_runner.clone(); let run_store = services.run_store.clone(); let env = services.env.clone(); + let inputs = services.inputs.clone(); let dry_run = services.dry_run; let cancel_requested = services.cancel_requested.clone(); let workflow_path = services.workflow_path.clone(); @@ -361,6 +362,7 @@ impl Handler for ParallelHandler { git_state: std::sync::RwLock::new(None), hook_runner: hook_runner.clone(), env: env.clone(), + inputs: inputs.clone(), dry_run, cancel_requested, workflow_path, diff --git a/lib/crates/fabro-workflow/src/handler/prompt.rs b/lib/crates/fabro-workflow/src/handler/prompt.rs index 67fc8227c..078d37536 100644 --- a/lib/crates/fabro-workflow/src/handler/prompt.rs +++ b/lib/crates/fabro-workflow/src/handler/prompt.rs @@ -52,7 +52,7 @@ impl Handler for PromptHandler { .prompt() .filter(|p| !p.is_empty()) .unwrap_or_else(|| node.label()); - let expanded = expand_variables(raw_prompt, graph)?; + let expanded = expand_variables(raw_prompt, graph, &services.inputs)?; let preamble = context.preamble(); let prompt = if preamble.is_empty() { expanded diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index 2af4bd843..1faacfa07 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -3,6 +3,7 @@ use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_model::Catalog; use fabro_sandbox::SandboxProvider; use fabro_store::Database; +use fabro_template::{TemplateContext, render as render_template}; use fabro_types::settings::run::RunMode; use fabro_types::settings::{Settings, SettingsLayer}; use fabro_types::{RunId, RunProvenance}; @@ -18,7 +19,7 @@ use crate::pipeline::{self, Persisted, TransformOptions, Validated}; use crate::records::RunRecord; use crate::run_lookup::default_scratch_base; use crate::run_materialization::materialize_run; -use crate::transforms::{Transform, expand_vars}; +use crate::transforms::Transform; use crate::workflow_bundle::{RunDefinition, WorkflowBundle}; use fabro_sandbox::daytona::detect_repo_info; use fabro_util::json::normalize_json_value; @@ -330,14 +331,14 @@ pub(super) fn preprocess_and_validate( settings: Option<&SettingsLayer>, goal_override: Option<&str>, ) -> Result { - let source = match run_inputs_as_strings(settings) { - Some(mut vars) => { - vars.insert("goal".to_string(), "$goal".to_string()); - expand_vars(dot_source, &vars) - .map_err(|e| FabroError::Parse(format!("var expansion failed: {e}")))? - } - None => dot_source.to_string(), - }; + let inputs = run_inputs(settings); + let source = render_template( + dot_source, + &TemplateContext::new() + .with_goal("{{ goal }}") + .with_inputs(inputs.clone()), + ) + .map_err(|error| FabroError::Parse(format!("template expansion failed: {error}")))?; let mut parsed = pipeline::parse(&source)?; apply_goal_override(&mut parsed.graph, goal_override); @@ -347,27 +348,19 @@ pub(super) fn preprocess_and_validate( &TransformOptions { current_dir, file_resolver, + inputs, custom_transforms, }, - ); + )?; Ok(pipeline::validate(transformed, &[])) } -fn run_inputs_as_strings(settings: Option<&SettingsLayer>) -> Option> { +fn run_inputs(settings: Option<&SettingsLayer>) -> HashMap { settings .and_then(|settings| settings.run.as_ref()) .and_then(|run| run.inputs.as_ref()) - .map(|inputs| { - inputs - .iter() - .map(|(key, value)| { - let stringified = value - .as_str() - .map_or_else(|| value.to_string(), ToString::to_string); - (key.clone(), stringified) - }) - .collect() - }) + .cloned() + .unwrap_or_default() } fn apply_goal_override(graph: &mut Graph, goal_override: Option<&str>) { @@ -493,7 +486,7 @@ mod tests { let dot = r#"digraph Test { graph [goal="Fix bugs"] start [shape=Mdiamond] - work [prompt="Goal: $goal"] + work [prompt="Goal: {{ goal }}"] exit [shape=Msquare] start -> work -> exit }"#; @@ -549,7 +542,7 @@ mod tests { let dot = r#"digraph Test { graph [goal="original"] start [shape=Mdiamond] - work [prompt="$who: $goal"] + work [prompt="{{ inputs.who }}: {{ goal }}"] exit [shape=Msquare] start -> work -> exit }"#; @@ -608,14 +601,17 @@ mod tests { struct TagTransform; impl Transform for TagTransform { - fn apply(&self, graph: fabro_graphviz::graph::Graph) -> fabro_graphviz::graph::Graph { + fn apply( + &self, + graph: fabro_graphviz::graph::Graph, + ) -> Result { let mut graph = graph; for node in graph.nodes.values_mut() { node.attrs .insert("tagged".to_string(), AttrValue::Boolean(true)); } - graph + Ok(graph) } } @@ -690,7 +686,10 @@ mod tests { }"# .to_string(), ), - (PathBuf::from("prompts/lint.md"), "Lint $goal".to_string()), + ( + PathBuf::from("prompts/lint.md"), + "Lint {{ goal }}".to_string(), + ), ]), }), settings: SettingsLayer::default(), diff --git a/lib/crates/fabro-workflow/src/pipeline/execute.rs b/lib/crates/fabro-workflow/src/pipeline/execute.rs index d375a829c..e86d021f4 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute.rs @@ -38,6 +38,7 @@ pub async fn execute(init: Initialized) -> Executed { let Initialized { graph, source: _, + inputs, run_options, workflow_path, workflow_bundle, @@ -58,6 +59,8 @@ pub async fn execute(init: Initialized) -> Executed { provider, } = init; + let service_inputs = inputs; + let mut checkpoint = checkpoint; if let Some(cp) = checkpoint.as_mut() { artifact::normalize_checkpoint_for_resume(cp); @@ -87,6 +90,7 @@ pub async fn execute(init: Initialized) -> Executed { git_state: std::sync::RwLock::new(git_state), hook_runner: hook_runner.clone(), env, + inputs: service_inputs, dry_run, cancel_requested: run_options.cancel_token.clone(), workflow_path, diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index 4cd866d6f..cd6942fa0 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -656,6 +656,13 @@ pub async fn initialize( Ok(Initialized { graph, source, + inputs: options + .run_options + .settings + .run + .as_ref() + .and_then(|run| run.inputs.clone()) + .unwrap_or_default(), run_options: options.run_options, workflow_path: options.workflow_path, workflow_bundle: options.workflow_bundle, diff --git a/lib/crates/fabro-workflow/src/pipeline/transform.rs b/lib/crates/fabro-workflow/src/pipeline/transform.rs index ffe8e4add..d1dfd7157 100644 --- a/lib/crates/fabro-workflow/src/pipeline/transform.rs +++ b/lib/crates/fabro-workflow/src/pipeline/transform.rs @@ -2,23 +2,31 @@ use std::sync::Arc; use crate::transforms::{ FileInliningTransform, ImportTransform, ModelResolutionTransform, - StylesheetApplicationTransform, Transform, VariableExpansionTransform, + StylesheetApplicationTransform, TemplateTransform, Transform, }; use super::types::{Parsed, TransformOptions, Transformed}; /// TRANSFORM phase: apply built-in and custom transforms to a parsed graph. /// -/// Infallible. Returns `Transformed` with a graph for post-transform -/// adjustments (e.g. goal override) before validation. -pub fn transform(parsed: Parsed, options: &TransformOptions) -> Transformed { +/// Returns `Transformed` with a graph for post-transform adjustments +/// (e.g. goal override) before validation. +pub fn transform( + parsed: Parsed, + options: &TransformOptions, +) -> Result { let Parsed { graph, source } = parsed; // Built-in transforms (PreambleTransform moved to engine execution time) let graph = if let (Some(current_dir), Some(file_resolver)) = (&options.current_dir, &options.file_resolver) { - ImportTransform::new(current_dir.clone(), Arc::clone(file_resolver)).apply(graph) + ImportTransform::new( + current_dir.clone(), + Arc::clone(file_resolver), + options.inputs.clone(), + ) + .apply(graph)? } else { graph }; @@ -26,26 +34,30 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Transformed { let graph = if let (Some(current_dir), Some(file_resolver)) = (&options.current_dir, &options.file_resolver) { - FileInliningTransform::new(current_dir.clone(), Arc::clone(file_resolver)).apply(graph) + FileInliningTransform::new(current_dir.clone(), Arc::clone(file_resolver)).apply(graph)? } else { graph }; - let graph = VariableExpansionTransform.apply(graph); - let graph = StylesheetApplicationTransform.apply(graph); - let graph = ModelResolutionTransform.apply(graph); + let graph = TemplateTransform { + inputs: options.inputs.clone(), + } + .apply(graph)?; + let graph = StylesheetApplicationTransform.apply(graph)?; + let graph = ModelResolutionTransform.apply(graph)?; // Custom transforms let graph = options .custom_transforms .iter() - .fold(graph, |graph, transform| transform.apply(graph)); + .try_fold(graph, |graph, transform| transform.apply(graph))?; - Transformed { graph, source } + Ok(Transformed { graph, source }) } #[cfg(test)] mod tests { + use std::collections::HashMap; use std::path::Path; use std::sync::Arc; @@ -66,7 +78,7 @@ mod tests { let dot = r#"digraph Test { graph [goal="Fix bugs"] start [shape=Mdiamond] - work [prompt="Goal: $goal"] + work [prompt="Goal: {{ goal }}"] exit [shape=Msquare] start -> work -> exit }"#; @@ -76,9 +88,11 @@ mod tests { &TransformOptions { current_dir: None, file_resolver: None, + inputs: HashMap::new(), custom_transforms: vec![], }, - ); + ) + .unwrap(); let prompt = transformed.graph.nodes["work"] .attrs .get("prompt") @@ -102,9 +116,11 @@ mod tests { &TransformOptions { current_dir: None, file_resolver: None, + inputs: HashMap::new(), custom_transforms: vec![], }, - ); + ) + .unwrap(); assert_eq!( transformed.graph.nodes["work"].attrs.get("model"), Some(&AttrValue::String("claude-sonnet-4-6".into())) @@ -114,7 +130,7 @@ mod tests { #[test] fn transform_inlines_files_before_variable_expansion() { let dir = tempfile::tempdir().unwrap(); - write_file(&dir.path().join("goal.md"), "Expand $goal"); + write_file(&dir.path().join("goal.md"), "Expand {{ goal }}"); let parsed = parse( r#"digraph Test { @@ -131,9 +147,11 @@ mod tests { &TransformOptions { current_dir: Some(dir.path().to_path_buf()), file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + inputs: HashMap::new(), custom_transforms: vec![], }, - ); + ) + .unwrap(); assert_eq!( transformed.graph.nodes["work"] @@ -147,7 +165,10 @@ mod tests { #[test] fn transform_imports_before_variable_expansion_and_stylesheet() { let dir = tempfile::tempdir().unwrap(); - write_file(&dir.path().join("prompts/lint.md"), "Run checks for $goal"); + write_file( + &dir.path().join("prompts/lint.md"), + "Run checks for {{ inputs.task }}", + ); write_file( &dir.path().join("validate.fabro"), r#"digraph validate { @@ -173,9 +194,14 @@ mod tests { &TransformOptions { current_dir: Some(dir.path().to_path_buf()), file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + inputs: HashMap::from([( + "task".to_string(), + toml::Value::String("Launch".to_string()), + )]), custom_transforms: vec![], }, - ); + ) + .unwrap(); let lint = &transformed.graph.nodes["validate.lint"]; assert_eq!( diff --git a/lib/crates/fabro-workflow/src/pipeline/types.rs b/lib/crates/fabro-workflow/src/pipeline/types.rs index 61cd9396e..48f15f3cf 100644 --- a/lib/crates/fabro-workflow/src/pipeline/types.rs +++ b/lib/crates/fabro-workflow/src/pipeline/types.rs @@ -259,6 +259,7 @@ pub struct InitOptions { pub struct Initialized { pub graph: Graph, pub source: String, + pub inputs: HashMap, pub run_options: RunOptions, pub workflow_path: Option, pub workflow_bundle: Option>, @@ -336,6 +337,7 @@ pub struct Finalized { pub struct TransformOptions { pub current_dir: Option, pub file_resolver: Option>, + pub inputs: HashMap, pub custom_transforms: Vec>, } diff --git a/lib/crates/fabro-workflow/src/pipeline/validate.rs b/lib/crates/fabro-workflow/src/pipeline/validate.rs index ed084ec0d..e07cf8747 100644 --- a/lib/crates/fabro-workflow/src/pipeline/validate.rs +++ b/lib/crates/fabro-workflow/src/pipeline/validate.rs @@ -26,9 +26,11 @@ mod tests { &TransformOptions { current_dir: None, file_resolver: None, + inputs: std::collections::HashMap::new(), custom_transforms: vec![], }, - ); + ) + .unwrap(); validate(transformed, &[]) } diff --git a/lib/crates/fabro-workflow/src/test_support.rs b/lib/crates/fabro-workflow/src/test_support.rs index b8763c29a..aea9214e8 100644 --- a/lib/crates/fabro-workflow/src/test_support.rs +++ b/lib/crates/fabro-workflow/src/test_support.rs @@ -118,6 +118,12 @@ async fn initialized( initialized: Initialized { graph: graph.clone(), source: String::new(), + inputs: run_options + .settings + .run + .as_ref() + .and_then(|run| run.inputs.clone()) + .unwrap_or_default(), run_options: run_options.clone(), workflow_path: None, workflow_bundle: None, diff --git a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs index 530fe17b2..96a6f1e68 100644 --- a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs +++ b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs @@ -38,7 +38,7 @@ impl FileInliningTransform { } impl Transform for FileInliningTransform { - fn apply(&self, graph: Graph) -> Graph { + fn apply(&self, graph: Graph) -> Result { let mut graph = graph; // Inline @file refs in node prompts @@ -62,7 +62,7 @@ impl Transform for FileInliningTransform { } } - graph + Ok(graph) } } @@ -155,7 +155,7 @@ mod tests { dir.path().to_path_buf(), Arc::new(FilesystemFileResolver::new(None)), ); - let graph = transform.apply(graph); + let graph = transform.apply(graph).unwrap(); assert_eq!( graph.nodes["work"] @@ -288,7 +288,7 @@ mod tests { fallback.path().to_path_buf(), ))), ); - let graph = transform.apply(graph); + let graph = transform.apply(graph).unwrap(); assert_eq!( graph.nodes["work"] diff --git a/lib/crates/fabro-workflow/src/transforms/import.rs b/lib/crates/fabro-workflow/src/transforms/import.rs index 34190f0d8..4e71e61c3 100644 --- a/lib/crates/fabro-workflow/src/transforms/import.rs +++ b/lib/crates/fabro-workflow/src/transforms/import.rs @@ -4,7 +4,9 @@ use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_graphviz::parser; +use fabro_template::{TemplateContext, render as render_template}; +use crate::error::FabroError; use crate::file_resolver::{FileResolver, ResolvedFile}; use super::{FileInliningTransform, Transform}; @@ -12,6 +14,7 @@ use super::{FileInliningTransform, Transform}; pub struct ImportTransform { current_dir: PathBuf, resolver: Arc, + inputs: HashMap, } struct PlaceholderOptions { @@ -28,12 +31,28 @@ struct PreparedImport { exit_predecessor_id: String, } +enum ImportPrepareError { + Hard(FabroError), + Soft(String), +} + +impl From for ImportPrepareError { + fn from(error: FabroError) -> Self { + Self::Hard(error) + } +} + impl ImportTransform { #[must_use] - pub fn new(current_dir: PathBuf, resolver: Arc) -> Self { + pub fn new( + current_dir: PathBuf, + resolver: Arc, + inputs: HashMap, + ) -> Self { Self { current_dir, resolver, + inputs, } } @@ -57,9 +76,9 @@ impl ImportTransform { import_path: &str, current_base_dir: &Path, import_stack: &mut Vec, - ) { + ) -> Result<(), FabroError> { if !graph.nodes.contains_key(placeholder_id) { - return; + return Ok(()); } if graph @@ -72,14 +91,14 @@ impl ImportTransform { placeholder_id, &format!("import placeholder '{placeholder_id}' cannot have a self-loop"), ); - return; + return Ok(()); } let placeholder = match Self::placeholder_config(graph, placeholder_id) { Ok(placeholder) => placeholder, Err(message) => { Self::poison_placeholder(graph, placeholder_id, &message); - return; + return Ok(()); } }; @@ -89,7 +108,7 @@ impl ImportTransform { placeholder_id, &format!("file not found: {import_path}"), ); - return; + return Ok(()); }; if import_stack.contains(&resolved_file.logical_path) { @@ -104,14 +123,15 @@ impl ImportTransform { placeholder_id, &format!("circular import detected: {cycle}"), ); - return; + return Ok(()); } let prepared = match self.prepare_import(&resolved_file, import_stack) { Ok(prepared) => prepared, - Err(message) => { + Err(ImportPrepareError::Hard(error)) => return Err(error), + Err(ImportPrepareError::Soft(message)) => { Self::poison_placeholder(graph, placeholder_id, &message); - return; + return Ok(()); } }; @@ -124,22 +144,34 @@ impl ImportTransform { ) { Self::poison_placeholder(graph, placeholder_id, &message); } + + Ok(()) } fn prepare_import( &self, resolved_file: &ResolvedFile, import_stack: &mut Vec, - ) -> Result { + ) -> Result { Self::with_import_stack( import_stack, resolved_file.logical_path.clone(), |import_stack| { - let mut graph = parser::parse(&resolved_file.content).map_err(|error| { - format!( + let rendered_source = render_template( + &resolved_file.content, + &TemplateContext::new() + .with_goal("{{ goal }}") + .with_inputs(self.inputs.clone()), + ) + .map_err(|error| { + ImportPrepareError::Hard(FabroError::Validation(error.to_string())) + })?; + + let mut graph = parser::parse(&rendered_source).map_err(|error| { + ImportPrepareError::Soft(format!( "failed to parse {}: {error}", resolved_file.logical_path.display() - ) + )) })?; let import_base_dir = resolved_file @@ -148,10 +180,11 @@ impl ImportTransform { .map_or_else(|| PathBuf::from("."), Path::to_path_buf); graph = FileInliningTransform::new(import_base_dir.clone(), Arc::clone(&self.resolver)) - .apply(graph); + .apply(graph) + .map_err(ImportPrepareError::Hard)?; if let Some(message) = Self::unresolved_imported_prompt_error(&graph) { - return Err(message); + return Err(ImportPrepareError::Soft(message)); } let nested_imports = Self::collect_import_nodes(&graph); @@ -162,10 +195,10 @@ impl ImportTransform { &import_path, &import_base_dir, import_stack, - ); + )?; } - Self::validate_imported_graph(graph) + Self::validate_imported_graph(graph).map_err(ImportPrepareError::Soft) }, ) } @@ -565,7 +598,7 @@ impl PreparedImport { } impl Transform for ImportTransform { - fn apply(&self, graph: Graph) -> Graph { + fn apply(&self, graph: Graph) -> Result { let mut graph = graph; let imports = Self::collect_import_nodes(&graph); let mut import_stack = Vec::new(); @@ -577,10 +610,10 @@ impl Transform for ImportTransform { &import_path, &self.current_dir, &mut import_stack, - ); + )?; } - graph + Ok(graph) } } @@ -613,8 +646,10 @@ mod tests { Arc::new(FilesystemFileResolver::new( fallback_dir.map(Path::to_path_buf), )), + HashMap::new(), ) .apply(graph) + .unwrap() } fn basic_import_source() -> &'static str { @@ -1319,8 +1354,10 @@ mod tests { let graph = ImportTransform::new( dir.path().to_path_buf(), Arc::new(FilesystemFileResolver::new(None)), + HashMap::new(), ) - .apply(graph); + .apply(graph) + .unwrap(); assert_eq!( graph.nodes["validate"] diff --git a/lib/crates/fabro-workflow/src/transforms/mod.rs b/lib/crates/fabro-workflow/src/transforms/mod.rs index 46715c7d2..d321254b0 100644 --- a/lib/crates/fabro-workflow/src/transforms/mod.rs +++ b/lib/crates/fabro-workflow/src/transforms/mod.rs @@ -1,9 +1,10 @@ use fabro_graphviz::graph::Graph; +use crate::error::FabroError; + /// A transform that modifies the pipeline graph after parsing and before validation. pub trait Transform { - #[must_use] - fn apply(&self, graph: Graph) -> Graph; + fn apply(&self, graph: Graph) -> Result; } mod file_inlining; @@ -19,4 +20,5 @@ pub use import::ImportTransform; pub use model_resolution::ModelResolutionTransform; pub use preamble::PreambleTransform; pub use stylesheet_application::StylesheetApplicationTransform; -pub use variable_expansion::{VariableExpansionTransform, expand_vars}; +pub use variable_expansion::TemplateTransform; +pub type VariableExpansionTransform = TemplateTransform; diff --git a/lib/crates/fabro-workflow/src/transforms/model_resolution.rs b/lib/crates/fabro-workflow/src/transforms/model_resolution.rs index 495936095..faee22931 100644 --- a/lib/crates/fabro-workflow/src/transforms/model_resolution.rs +++ b/lib/crates/fabro-workflow/src/transforms/model_resolution.rs @@ -6,7 +6,7 @@ use super::Transform; pub struct ModelResolutionTransform; impl Transform for ModelResolutionTransform { - fn apply(&self, graph: Graph) -> Graph { + fn apply(&self, graph: Graph) -> Result { let mut graph = graph; for node in graph.nodes.values_mut() { let model = node @@ -31,7 +31,7 @@ impl Transform for ModelResolutionTransform { } } - graph + Ok(graph) } } @@ -51,7 +51,7 @@ mod tests { ); graph.nodes.insert("a".to_string(), node); - let graph = ModelResolutionTransform.apply(graph); + let graph = ModelResolutionTransform.apply(graph).unwrap(); assert_eq!( graph.nodes["a"] @@ -76,7 +76,7 @@ mod tests { ); graph.nodes.insert("a".to_string(), node); - let graph = ModelResolutionTransform.apply(graph); + let graph = ModelResolutionTransform.apply(graph).unwrap(); assert_eq!( graph.nodes["a"] @@ -97,7 +97,7 @@ mod tests { ); graph.nodes.insert("a".to_string(), node); - let graph = ModelResolutionTransform.apply(graph); + let graph = ModelResolutionTransform.apply(graph).unwrap(); assert_eq!(graph.nodes["a"].attrs.get("provider"), None); } @@ -108,7 +108,7 @@ mod tests { let node = Node::new("a"); graph.nodes.insert("a".to_string(), node); - let graph = ModelResolutionTransform.apply(graph); + let graph = ModelResolutionTransform.apply(graph).unwrap(); assert_eq!(graph.nodes["a"].attrs.get("provider"), None); } @@ -121,7 +121,7 @@ mod tests { .insert("model".to_string(), AttrValue::String("gpt-54".to_string())); graph.nodes.insert("a".to_string(), node); - let graph = ModelResolutionTransform.apply(graph); + let graph = ModelResolutionTransform.apply(graph).unwrap(); assert_eq!( graph.nodes["a"] @@ -149,7 +149,7 @@ mod tests { ); graph.nodes.insert("a".to_string(), node); - let graph = ModelResolutionTransform.apply(graph); + let graph = ModelResolutionTransform.apply(graph).unwrap(); assert_eq!( graph.nodes["a"] diff --git a/lib/crates/fabro-workflow/src/transforms/preamble.rs b/lib/crates/fabro-workflow/src/transforms/preamble.rs index 3ab6f2a63..63be0e38e 100644 --- a/lib/crates/fabro-workflow/src/transforms/preamble.rs +++ b/lib/crates/fabro-workflow/src/transforms/preamble.rs @@ -6,7 +6,7 @@ use super::Transform; pub struct PreambleTransform; impl Transform for PreambleTransform { - fn apply(&self, graph: Graph) -> Graph { + fn apply(&self, graph: Graph) -> Result { use crate::context::keys::Fidelity; let mut graph = graph; @@ -30,7 +30,7 @@ impl Transform for PreambleTransform { } } - graph + Ok(graph) } } @@ -54,7 +54,7 @@ mod tests { ); graph.nodes.insert("work".to_string(), node); - let graph = PreambleTransform.apply(graph); + let graph = PreambleTransform.apply(graph).unwrap(); let prompt = graph.nodes["work"] .attrs @@ -78,7 +78,7 @@ mod tests { ); graph.nodes.insert("work".to_string(), node); - let graph = PreambleTransform.apply(graph); + let graph = PreambleTransform.apply(graph).unwrap(); let prompt = graph.nodes["work"] .attrs @@ -102,7 +102,7 @@ mod tests { ); graph.nodes.insert("work".to_string(), node); - let graph = PreambleTransform.apply(graph); + let graph = PreambleTransform.apply(graph).unwrap(); let prompt = graph.nodes["work"] .attrs @@ -122,7 +122,7 @@ mod tests { ); graph.nodes.insert("work".to_string(), node); - let graph = PreambleTransform.apply(graph); + let graph = PreambleTransform.apply(graph).unwrap(); assert!(!graph.nodes["work"].attrs.contains_key("prompt")); } diff --git a/lib/crates/fabro-workflow/src/transforms/stylesheet_application.rs b/lib/crates/fabro-workflow/src/transforms/stylesheet_application.rs index b7d7d4519..5f1cd0954 100644 --- a/lib/crates/fabro-workflow/src/transforms/stylesheet_application.rs +++ b/lib/crates/fabro-workflow/src/transforms/stylesheet_application.rs @@ -7,17 +7,17 @@ use super::stylesheet::{apply_stylesheet, parse_stylesheet}; pub struct StylesheetApplicationTransform; impl Transform for StylesheetApplicationTransform { - fn apply(&self, graph: Graph) -> Graph { + fn apply(&self, graph: Graph) -> Result { let mut graph = graph; let stylesheet_text = graph.model_stylesheet().to_string(); if stylesheet_text.is_empty() { - return graph; + return Ok(graph); } let Ok(stylesheet) = parse_stylesheet(&stylesheet_text) else { - return graph; + return Ok(graph); }; apply_stylesheet(&stylesheet, &mut graph); - graph + Ok(graph) } } @@ -34,6 +34,6 @@ mod tests { let transform = StylesheetApplicationTransform; // Should not panic with empty stylesheet - let _graph = transform.apply(graph); + let _graph = transform.apply(graph).unwrap(); } } diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index 597321edc..e33c2c8d5 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -1,70 +1,57 @@ use std::collections::HashMap; -use anyhow::bail; use fabro_graphviz::graph::{AttrValue, Graph}; +use fabro_template::{TemplateContext, render as render_template}; use super::Transform; +use crate::error::FabroError; -/// Expand `$name` placeholders in `source` using the given variable map. -/// -/// Identifiers match `[a-zA-Z_][a-zA-Z0-9_]*`. A `$` not followed by an -/// identifier character is left as-is. Undefined variables produce an error. -pub fn expand_vars(source: &str, vars: &HashMap) -> anyhow::Result { - let mut result = String::with_capacity(source.len()); - let bytes = source.as_bytes(); - let len = bytes.len(); - let mut i = 0; - - while i < len { - if bytes[i] == b'$' { - let start = i + 1; - if start < len && bytes[start] == b'$' { - result.push('$'); - i = start + 1; - } else if start < len && (bytes[start].is_ascii_alphabetic() || bytes[start] == b'_') { - let mut end = start + 1; - while end < len && (bytes[end].is_ascii_alphanumeric() || bytes[end] == b'_') { - end += 1; - } - let name = &source[start..end]; - match vars.get(name) { - Some(value) => result.push_str(value), - None => bail!("Undefined variable: ${name}"), - } - i = end; - } else { - result.push('$'); - i = start; - } - } else { - result.push(source[i..].chars().next().unwrap()); - i += source[i..].chars().next().unwrap().len_utf8(); - } - } - - Ok(result) +/// Expands `{{ goal }}` / `{{ inputs.* }}` across all string attributes. +pub struct TemplateTransform { + pub inputs: HashMap, } -/// Expands `$goal` in node `prompt` attributes to the graph-level `goal` value. -pub struct VariableExpansionTransform; - -impl Transform for VariableExpansionTransform { - fn apply(&self, graph: Graph) -> Graph { - let mut graph = graph; - let goal = graph.goal().to_string(); - let vars = HashMap::from([("goal".to_string(), goal)]); - for node in graph.nodes.values_mut() { - if let Some(AttrValue::String(prompt)) = node.attrs.get("prompt") { - if let Ok(expanded) = expand_vars(prompt, &vars) { - if expanded != *prompt { - node.attrs - .insert("prompt".to_string(), AttrValue::String(expanded)); - } - } +impl TemplateTransform { + fn render_attrs( + attrs: &mut HashMap, + ctx: &TemplateContext, + ) -> Result<(), FabroError> { + for value in attrs.values_mut() { + if let AttrValue::String(text) = value { + let rendered = render_template(text, ctx) + .map_err(|error| FabroError::Validation(error.to_string()))?; + *text = rendered; } } + Ok(()) + } - graph + fn resolved_goal(&self, graph: &Graph) -> Result { + let ctx = TemplateContext::new() + .with_goal("{{ goal }}") + .with_inputs(self.inputs.clone()); + render_template(graph.goal(), &ctx) + .map_err(|error| FabroError::Validation(error.to_string())) + } +} + +impl Transform for TemplateTransform { + fn apply(&self, graph: Graph) -> Result { + let mut graph = graph; + let resolved_goal = self.resolved_goal(&graph)?; + let ctx = TemplateContext::new() + .with_goal(resolved_goal) + .with_inputs(self.inputs.clone()); + + Self::render_attrs(&mut graph.attrs, &ctx)?; + for node in graph.nodes.values_mut() { + Self::render_attrs(&mut node.attrs, &ctx)?; + } + for edge in &mut graph.edges { + Self::render_attrs(&mut edge.attrs, &ctx)?; + } + + Ok(graph) } } @@ -72,64 +59,55 @@ impl Transform for VariableExpansionTransform { mod tests { use std::collections::HashMap; - use fabro_graphviz::graph::{AttrValue, Graph, Node}; + use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use super::*; #[test] - fn expand_single_var() { - let vars = HashMap::from([("name".to_string(), "world".to_string())]); - assert_eq!(expand_vars("Hello $name", &vars).unwrap(), "Hello world"); - } - - #[test] - fn expand_multiple_vars() { - let vars = HashMap::from([ - ("greeting".to_string(), "Hello".to_string()), - ("name".to_string(), "world".to_string()), - ]); - assert_eq!( - expand_vars("$greeting $name!", &vars).unwrap(), - "Hello world!" - ); - } - - #[test] - fn expand_undefined_var_errors() { - let vars = HashMap::new(); - let err = expand_vars("Hello $missing", &vars).unwrap_err(); - assert!( - err.to_string().contains("Undefined variable: $missing"), - "unexpected error: {err}" - ); - } - - #[test] - fn expand_escaped_dollar() { - let vars = HashMap::from([("name".to_string(), "world".to_string())]); - assert_eq!( - expand_vars("literal $$name here", &vars).unwrap(), - "literal $name here" - ); - } - - #[test] - fn variable_expansion_replaces_goal() { + fn template_transform_replaces_goal_and_inputs_across_string_attrs() { let mut graph = Graph::new("test"); graph.attrs.insert( "goal".to_string(), AttrValue::String("Fix bugs".to_string()), ); + graph.attrs.insert( + "label".to_string(), + AttrValue::String("Workflow: {{ goal }}".to_string()), + ); let mut node = Node::new("plan"); node.attrs.insert( "prompt".to_string(), - AttrValue::String("Achieve: $goal now".to_string()), + AttrValue::String("Achieve: {{ goal }} now".to_string()), + ); + node.attrs.insert( + "label".to_string(), + AttrValue::String("{{ inputs.name }}".to_string()), ); graph.nodes.insert("plan".to_string(), node); - let transform = VariableExpansionTransform; - let graph = transform.apply(graph); + graph.edges.push(Edge { + from: "start".to_string(), + to: "plan".to_string(), + attrs: HashMap::from([( + "label".to_string(), + AttrValue::String("{{ inputs.greeting }}".to_string()), + )]), + }); + + let transform = TemplateTransform { + inputs: HashMap::from([ + ( + "name".to_string(), + toml::Value::String("Planner".to_string()), + ), + ( + "greeting".to_string(), + toml::Value::String("hello".to_string()), + ), + ]), + }; + let graph = transform.apply(graph).unwrap(); let prompt = graph.nodes["plan"] .attrs @@ -137,46 +115,53 @@ mod tests { .and_then(AttrValue::as_str) .unwrap(); assert_eq!(prompt, "Achieve: Fix bugs now"); + assert_eq!( + graph.nodes["plan"].attrs.get("label"), + Some(&AttrValue::String("Planner".to_string())) + ); + assert_eq!( + graph.attrs.get("label"), + Some(&AttrValue::String("Workflow: Fix bugs".to_string())) + ); + assert_eq!( + graph.edges[0].attrs.get("label"), + Some(&AttrValue::String("hello".to_string())) + ); } #[test] - fn variable_expansion_no_goal_variable() { + fn template_transform_leaves_non_string_attrs_unchanged() { let mut graph = Graph::new("test"); - graph.attrs.insert( - "goal".to_string(), - AttrValue::String("Fix bugs".to_string()), - ); - let mut node = Node::new("plan"); - node.attrs.insert( - "prompt".to_string(), - AttrValue::String("Do something".to_string()), - ); + node.attrs + .insert("max_retries".to_string(), AttrValue::Integer(3)); graph.nodes.insert("plan".to_string(), node); - let transform = VariableExpansionTransform; - let graph = transform.apply(graph); + let transform = TemplateTransform { + inputs: HashMap::new(), + }; + let graph = transform.apply(graph).unwrap(); - let prompt = graph.nodes["plan"] - .attrs - .get("prompt") - .and_then(AttrValue::as_str) - .unwrap(); - assert_eq!(prompt, "Do something"); + assert_eq!( + graph.nodes["plan"].attrs.get("max_retries"), + Some(&AttrValue::Integer(3)) + ); } #[test] - fn variable_expansion_empty_goal() { + fn template_transform_supports_empty_goal() { let mut graph = Graph::new("test"); let mut node = Node::new("plan"); node.attrs.insert( "prompt".to_string(), - AttrValue::String("Goal: $goal".to_string()), + AttrValue::String("Goal: {{ goal }}".to_string()), ); graph.nodes.insert("plan".to_string(), node); - let transform = VariableExpansionTransform; - let graph = transform.apply(graph); + let transform = TemplateTransform { + inputs: HashMap::new(), + }; + let graph = transform.apply(graph).unwrap(); let prompt = graph.nodes["plan"] .attrs @@ -187,44 +172,19 @@ mod tests { } #[test] - fn variable_expansion_no_prompt() { + fn template_transform_errors_on_undefined_variable() { let mut graph = Graph::new("test"); - graph.attrs.insert( - "goal".to_string(), - AttrValue::String("Fix bugs".to_string()), - ); - let node = Node::new("plan"); - graph.nodes.insert("plan".to_string(), node); - - let transform = VariableExpansionTransform; - // Should not panic - let graph = transform.apply(graph); - assert!(!graph.nodes["plan"].attrs.contains_key("prompt")); - } - - #[test] - fn variable_expansion_escaped_dollar_goal() { - let mut graph = Graph::new("test"); - graph.attrs.insert( - "goal".to_string(), - AttrValue::String("Fix bugs".to_string()), - ); - let mut node = Node::new("plan"); node.attrs.insert( "prompt".to_string(), - AttrValue::String("literal $$goal here".to_string()), + AttrValue::String("{{ inputs.missing }}".to_string()), ); graph.nodes.insert("plan".to_string(), node); - let transform = VariableExpansionTransform; - let graph = transform.apply(graph); - - let prompt = graph.nodes["plan"] - .attrs - .get("prompt") - .and_then(AttrValue::as_str) - .unwrap(); - assert_eq!(prompt, "literal $goal here"); + let transform = TemplateTransform { + inputs: HashMap::new(), + }; + let err = transform.apply(graph).unwrap_err(); + assert!(err.to_string().contains("undefined")); } } diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index beff6405f..ea0e8f4cd 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -49,9 +49,7 @@ use fabro_workflow::records::{Checkpoint, CheckpointExt}; use fabro_workflow::run_options::{GitCheckpointOptions, RunOptions}; use fabro_workflow::test_support::{WorkflowRunner, run_graph_with_hooks, test_store_dir}; use fabro_workflow::transforms::stylesheet::{apply_stylesheet, parse_stylesheet}; -use fabro_workflow::transforms::{ - StylesheetApplicationTransform, Transform, VariableExpansionTransform, -}; +use fabro_workflow::transforms::{StylesheetApplicationTransform, TemplateTransform, Transform}; use object_store::local::LocalFileSystem; use ulid::Ulid; @@ -1070,14 +1068,14 @@ fn variable_expansion_replaces_goal_in_prompts() { let mut plan_node = Node::new("plan"); plan_node.attrs.insert( "prompt".to_string(), - AttrValue::String("Plan to achieve: $goal".to_string()), + AttrValue::String("Plan to achieve: {{ goal }}".to_string()), ); graph.nodes.insert("plan".to_string(), plan_node); let mut impl_node = Node::new("implement"); impl_node.attrs.insert( "prompt".to_string(), - AttrValue::String("Implement $goal now".to_string()), + AttrValue::String("Implement {{ goal }} now".to_string()), ); graph.nodes.insert("implement".to_string(), impl_node); @@ -1088,8 +1086,10 @@ fn variable_expansion_replaces_goal_in_prompts() { ); graph.nodes.insert("report".to_string(), no_var_node); - let transform = VariableExpansionTransform; - let graph = transform.apply(graph); + let transform = TemplateTransform { + inputs: std::collections::HashMap::new(), + }; + let graph = transform.apply(graph).unwrap(); let plan_prompt = graph.nodes["plan"] .attrs @@ -1154,7 +1154,7 @@ fn stylesheet_application_by_specificity() { graph.nodes.insert("explicit_node".to_string(), explicit); let transform = StylesheetApplicationTransform; - let graph = transform.apply(graph); + let graph = transform.apply(graph).unwrap(); // plan: universal -> claude-sonnet-4-5 assert_eq!( @@ -1214,7 +1214,7 @@ fn stylesheet_application_via_parsed_graph() { validate_or_raise(&graph, &[]).expect("validation should pass"); let transform = StylesheetApplicationTransform; - let graph = transform.apply(graph); + let graph = transform.apply(graph).unwrap(); // All nodes without explicit model should get "sonnet" assert_eq!( @@ -1691,7 +1691,7 @@ async fn smoke_test_with_mock_codergen_backend() { .insert("shape".to_string(), AttrValue::String("box".to_string())); plan.attrs.insert( "prompt".to_string(), - AttrValue::String("Plan to achieve: $goal".to_string()), + AttrValue::String("Plan to achieve: {{ goal }}".to_string()), ); graph.nodes.insert("plan".to_string(), plan); @@ -2615,7 +2615,7 @@ async fn scenario_ship_a_feature() { rankdir=LR start [shape=Mdiamond] exit [shape=Msquare] - plan [shape=box, prompt="Plan to achieve: $goal"] + plan [shape=box, prompt="Plan to achieve: {{ goal }}"] implement [shape=box, prompt="Implement the plan"] test [shape=parallelogram, script="echo PASS"] review [shape=hexagon, label="Review Changes"] @@ -2625,7 +2625,11 @@ async fn scenario_ship_a_feature() { }"#; let graph = parse(dot).expect("parse"); validate_or_raise(&graph, &[]).expect("validate"); - let graph = VariableExpansionTransform.apply(graph); + let graph = TemplateTransform { + inputs: std::collections::HashMap::new(), + } + .apply(graph) + .unwrap(); assert_eq!( graph.nodes["plan"].prompt().unwrap(), "Plan to achieve: Ship the widget" @@ -3573,7 +3577,7 @@ async fn stylesheet_applies_model_override() { }"#; let graph = parse(input).expect("parse"); validate_or_raise(&graph, &[]).expect("validate"); - let graph = StylesheetApplicationTransform.apply(graph); + let graph = StylesheetApplicationTransform.apply(graph).unwrap(); assert_eq!(graph.nodes["work"].model(), Some("custom-model")); let dir = tempfile::tempdir().unwrap(); @@ -3672,7 +3676,7 @@ async fn integration_smoke_plan_implement_review_done() { rankdir=LR start [shape=Mdiamond] exit [shape=Msquare] - plan [shape=box, prompt="Plan: $goal"] + plan [shape=box, prompt="Plan: {{ goal }}"] implement [shape=box, prompt="Implement"] review [shape=hexagon, label="Review"] start -> plan -> implement -> review @@ -3690,8 +3694,12 @@ async fn integration_smoke_plan_implement_review_done() { assert!(errors.is_empty()); // Apply transforms - let graph = VariableExpansionTransform.apply(graph); - let graph = StylesheetApplicationTransform.apply(graph); + let graph = TemplateTransform { + inputs: std::collections::HashMap::new(), + } + .apply(graph) + .unwrap(); + let graph = StylesheetApplicationTransform.apply(graph).unwrap(); // Verify transforms applied assert_eq!( @@ -4076,9 +4084,11 @@ async fn import_e2e_through_engine() { file_resolver: Some(std::sync::Arc::new( fabro_workflow::file_resolver::FilesystemFileResolver::new(None), )), + inputs: std::collections::HashMap::new(), custom_transforms: vec![], }, - ); + ) + .unwrap(); let validated = validate(transformed, &[]); validated .raise_on_errors()