From 2ba04be181ed984971922598a2e151ba55bf0b94 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Sat, 16 May 2026 15:47:37 -0700 Subject: [PATCH] feat(template): add source-aware diagnostics (#292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Template failures from `fabro run` and structural warnings from `fabro validate` now preserve source provenance through rendering, workflow transforms, API serialization, and CLI display. Diagnostics can point at the actual workflow, import, or prompt file with node/attribute context instead of surfacing MiniJinja's generic `` source. ## What Changed - Added named MiniJinja render APIs plus miette-aware `TemplateError` metadata for source names, source text, spans, and labels. - Reworked workflow template expansion so inline attributes, imported workflows, and `@prompt` files render with file and owner context. - Split strict run behavior from structural validate behavior: run-start still hard-fails on missing inputs, while validate emits source-aware warnings and continues linting. - Extended validation diagnostics through Rust structs, OpenAPI, server DTO mapping, and CLI rendering with optional source path, line, column, span, and related metadata. - Added regression coverage across template rendering, workflow transforms, CLI output, and the server validate endpoint. ## Verification - `cargo nextest run -p fabro-template` - `ulimit -n 4096 && cargo nextest run -p fabro-workflow --no-fail-fast` - `cargo nextest run -p fabro-cli bare_fabro_with_unbound_inputs_validates_structurally_with_warning run_rejects_unbound_template_inputs_before_creating_remote_run` - `cargo nextest run -p fabro-server validate_endpoint_returns_template_source_coordinates` - `cargo build -p fabro-api` - `cargo +nightly-2026-04-14 fmt --check --all` - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` --- [![Compound Engineering](https://img.shields.io/badge/Compound_Engineering-6366f1)](https://github.com/EveryInc/compound-engineering-plugin) 🤖 Generated with GPT-5 via [Codex](https://openai.com/codex) --------- Co-authored-by: Aleksi Asikainen <1086393+salieri@users.noreply.github.com> --- Cargo.lock | 2 + docs/public/api-reference/fabro-api.yaml | 35 + ...05-16-source-aware-template-diagnostics.md | 647 ++++++++++++++++++ .../fabro-cli/src/commands/run/output.rs | 33 +- lib/crates/fabro-cli/src/main.rs | 30 +- lib/crates/fabro-cli/src/shared/utilities.rs | 56 +- .../fabro-cli/tests/it/cmd/preflight.rs | 4 +- lib/crates/fabro-cli/tests/it/cmd/run.rs | 8 + lib/crates/fabro-cli/tests/it/cmd/validate.rs | 6 +- lib/crates/fabro-server/src/run_manifest.rs | 33 +- lib/crates/fabro-server/src/server/tests.rs | 62 ++ lib/crates/fabro-template/Cargo.toml | 1 + lib/crates/fabro-template/src/lib.rs | 265 ++++++- lib/crates/fabro-validate/src/lib.rs | 58 +- .../src/rules/all_conditional_edges.rs | 3 +- .../fabro-validate/src/rules/backend_valid.rs | 24 +- .../src/rules/condition_syntax.rs | 12 +- .../src/rules/direction_valid.rs | 12 +- .../src/rules/edge_target_exists.rs | 24 +- .../src/rules/exit_no_outgoing.rs | 12 +- .../src/rules/fidelity_valid.rs | 36 +- .../src/rules/freeform_edge_count.rs | 3 +- .../src/rules/goal_gate_has_retry.rs | 3 +- .../fabro-validate/src/rules/import_error.rs | 24 +- .../fabro-validate/src/rules/model_support.rs | 4 + .../src/rules/orphan_custom_outcome.rs | 3 +- .../src/rules/prompt_on_llm_nodes.rs | 15 +- .../rules/random_selection_no_conditions.rs | 3 +- .../fabro-validate/src/rules/reachability.rs | 12 +- .../src/rules/reserved_keyword_node_id.rs | 12 +- .../src/rules/retry_target_exists.rs | 48 +- .../src/rules/script_absolute_cd.rs | 3 +- .../src/rules/selection_valid.rs | 12 +- .../src/rules/start_no_incoming.rs | 12 +- .../fabro-validate/src/rules/start_node.rs | 22 +- .../src/rules/stylesheet_syntax.rs | 12 +- .../fabro-validate/src/rules/terminal_node.rs | 22 +- .../rules/thread_id_requires_fidelity_full.rs | 36 +- .../fabro-validate/src/rules/type_known.rs | 12 +- .../src/rules/unresolved_file_ref.rs | 6 +- lib/crates/fabro-workflow/Cargo.toml | 1 + lib/crates/fabro-workflow/src/error.rs | 158 ++++- .../fabro-workflow/src/operations/create.rs | 83 ++- .../fabro-workflow/src/operations/mod.rs | 1 + .../fabro-workflow/src/operations/validate.rs | 6 + .../fabro-workflow/src/pipeline/transform.rs | 43 +- .../fabro-workflow/src/pipeline/types.rs | 39 +- .../fabro-workflow/src/pipeline/validate.rs | 13 +- .../src/transforms/file_inlining.rs | 176 ++++- .../fabro-workflow/src/transforms/import.rs | 266 ++++++- .../fabro-workflow/src/transforms/mod.rs | 2 +- .../src/transforms/variable_expansion.rs | 398 ++++++++--- .../fabro-workflow/tests/it/integration.rs | 2 + 53 files changed, 2413 insertions(+), 402 deletions(-) create mode 100644 docs/superpowers/plans/2026-05-16-source-aware-template-diagnostics.md diff --git a/Cargo.lock b/Cargo.lock index b02d35ae0..67c45868b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2413,6 +2413,7 @@ version = "0.235.0-nightly.0" dependencies = [ "anyhow", "fabro-util", + "miette", "minijinja", "serde", "thiserror 2.0.18", @@ -2564,6 +2565,7 @@ dependencies = [ "hex", "httpmock", "md5", + "miette", "mime_guess", "object_store", "predicates", diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 21f23b6db..fff684c23 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -6021,6 +6021,41 @@ components: type: string fix: type: ["string", "null"] + source_path: + type: ["string", "null"] + line: + type: ["integer", "null"] + format: int32 + column: + type: ["integer", "null"] + format: int32 + span_start: + type: ["integer", "null"] + format: int64 + span_len: + type: ["integer", "null"] + format: int64 + related: + type: array + items: + $ref: "#/components/schemas/RelatedWorkflowDiagnostic" + default: [] + + RelatedWorkflowDiagnostic: + type: object + required: + - message + properties: + message: + type: string + source_path: + type: ["string", "null"] + line: + type: ["integer", "null"] + format: int32 + column: + type: ["integer", "null"] + format: int32 PreflightCheckReport: type: object diff --git a/docs/superpowers/plans/2026-05-16-source-aware-template-diagnostics.md b/docs/superpowers/plans/2026-05-16-source-aware-template-diagnostics.md new file mode 100644 index 000000000..a377c2ac4 --- /dev/null +++ b/docs/superpowers/plans/2026-05-16-source-aware-template-diagnostics.md @@ -0,0 +1,647 @@ +# Source-Aware Template Diagnostics Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `fabro run` and `fabro validate` template errors point to the real source file, line/column, and node context instead of MiniJinja's generic ``. + +**Architecture:** Preserve source provenance through template rendering instead of flattening errors to strings. `fabro-template` owns named MiniJinja rendering and typed span metadata; `fabro-workflow` owns workflow/node/attribute context; CLI and API layers render or serialize the structured diagnostics at their boundaries. + +**Tech Stack:** Rust, MiniJinja, miette, thiserror, serde, OpenAPI/progenitor, cargo-nextest, insta snapshots. + +--- + +## File Structure + +- Modify `lib/crates/fabro-template/src/lib.rs` to add named render APIs and miette-aware `TemplateError` metadata. +- Modify `lib/crates/fabro-workflow/src/operations/create.rs`, `lib/crates/fabro-workflow/src/transforms/variable_expansion.rs`, `lib/crates/fabro-workflow/src/transforms/file_inlining.rs`, and `lib/crates/fabro-workflow/src/transforms/import.rs` to render templates with source and owner context. +- Modify `lib/crates/fabro-workflow/src/error.rs` to preserve source-aware template errors instead of converting them to `String`. +- Modify `lib/crates/fabro-validate/src/lib.rs`, `docs/public/api-reference/fabro-api.yaml`, and `lib/crates/fabro-server/src/run_manifest.rs` to expose optional source coordinates in validation diagnostics. +- Modify `lib/crates/fabro-cli/src/main.rs`, `lib/crates/fabro-cli/src/shared/utilities.rs`, and `lib/crates/fabro-cli/src/commands/run/output.rs` to render structured source metadata. +- Update or add tests in `lib/crates/fabro-template/src/lib.rs`, `lib/crates/fabro-workflow/src/operations/create.rs`, `lib/crates/fabro-cli/tests/it/cmd/validate.rs`, `lib/crates/fabro-cli/tests/it/cmd/run.rs`, and `lib/crates/fabro-server/src/server/tests.rs`. + +## Task 1: Add Named Template Rendering + +**Files:** +- Modify: `lib/crates/fabro-template/src/lib.rs` + +- [x] **Step 1: Add failing unit tests for named template diagnostics** + +Add tests that call the new API names below: + +```rust +#[test] +fn render_named_reports_source_name_expression_and_span() { + let ctx = TemplateContext::new(); + let err = render_named("prompts/test.md", "Hello {{ inputs.foo }}", &ctx).unwrap_err(); + + let TemplateError::UndefinedVariable { + expression, + line, + source_name, + span, + .. + } = err + else { + panic!("expected undefined variable error"); + }; + + assert_eq!(expression.as_deref(), Some("inputs.foo")); + assert_eq!(line, Some(1)); + assert_eq!(source_name.as_deref(), Some("prompts/test.md")); + assert!(span.is_some()); +} + +#[test] +fn render_lenient_named_preserves_source_name_for_syntax_errors() { + let ctx = TemplateContext::new(); + let err = render_lenient_named("workflow.fabro", "{{ unterminated", &ctx).unwrap_err(); + + let TemplateError::Syntax { + source_name, .. + } = err + else { + panic!("expected syntax error"); + }; + + assert_eq!(source_name.as_deref(), Some("workflow.fabro")); +} +``` + +- [x] **Step 2: Run the tests and verify they fail** + +Run: + +```bash +cargo nextest run -p fabro-template render_named_reports_source_name_expression_and_span render_lenient_named_preserves_source_name_for_syntax_errors +``` + +Expected: tests fail because `render_named` and `render_lenient_named` do not exist. + +- [x] **Step 3: Implement named rendering APIs** + +Add these public functions and route existing `render` functions through an internal helper: + +```rust +pub fn render(template: &str, ctx: &TemplateContext) -> Result { + render_with(None, template, ctx, UndefinedBehavior::Strict) +} + +pub fn render_named( + name: impl Into, + template: &str, + ctx: &TemplateContext, +) -> Result { + render_with(Some(name.into()), template, ctx, UndefinedBehavior::Strict) +} + +pub fn render_lenient(template: &str, ctx: &TemplateContext) -> Result { + render_with(None, template, ctx, UndefinedBehavior::Chainable) +} + +pub fn render_lenient_named( + name: impl Into, + template: &str, + ctx: &TemplateContext, +) -> Result { + render_with(Some(name.into()), template, ctx, UndefinedBehavior::Chainable) +} +``` + +In `render_with`, use `env.render_named_str(&name, template, ctx.clone().into_value())` when a name is present, and `env.render_str(...)` otherwise. Keep the current plain-text fast path. + +- [x] **Step 4: Store source metadata on `TemplateError`** + +Extend each `TemplateError` variant with: + +```rust +source_name: Option, +source_text: Option, +span: Option, +``` + +In `From for TemplateError`, populate: + +```rust +let source_name = error.name().map(str::to_owned); +let source_text = error.template_source().map(str::to_owned); +let span = error + .range() + .and_then(|range| { + let start = range.start; + let len = range.end.checked_sub(range.start)?; + Some((start, len).into()) + }); +``` + +Preserve the existing `expression` and `line` behavior. + +- [x] **Step 5: Derive or implement `miette::Diagnostic` for `TemplateError`** + +Use `#[derive(miette::Diagnostic)]` if it stays readable. If derive becomes awkward because of enum variants, implement `miette::Diagnostic` manually so `source_code()` returns a `NamedSource` when both `source_name` and `source_text` exist, and `labels()` returns a label for `span`. + +- [x] **Step 6: Verify template tests** + +Run: + +```bash +cargo nextest run -p fabro-template +``` + +Expected: all `fabro-template` tests pass. + +## Task 2: Preserve Template Errors in Workflow Errors + +**Files:** +- Modify: `lib/crates/fabro-workflow/src/error.rs` +- Modify: `lib/crates/fabro-workflow/Cargo.toml` + +- [x] **Step 1: Add a failing error-chain regression test** + +Add a test showing that a workflow template error remains visible as a source cause: + +```rust +#[test] +fn template_error_variant_preserves_source_chain() { + let template_err = fabro_template::render_named( + "workflow.fabro", + "{{ inputs.missing }}", + &fabro_template::TemplateContext::new(), + ) + .unwrap_err(); + + let err = Error::template("template expansion failed", template_err); + let chain = collect_chain(&err); + + assert!(chain.iter().any(|part| part.contains("template expansion failed"))); + assert!(chain.iter().any(|part| part.contains("undefined template variable"))); +} +``` + +- [x] **Step 2: Run the test and verify it fails** + +Run: + +```bash +cargo nextest run -p fabro-workflow template_error_variant_preserves_source_chain +``` + +Expected: test fails because `Error::template` does not exist. + +- [x] **Step 3: Add a structured workflow error variant** + +Add a new `Error` variant: + +```rust +#[error("{message}")] +Template { + message: String, + #[source] + source: fabro_template::TemplateError, +} +``` + +Add: + +```rust +pub fn template(message: impl Into, source: fabro_template::TemplateError) -> Self { + Self::Template { + message: message.into(), + source, + } +} +``` + +Update failure classification to treat `Template` as deterministic validation/parse failure. + +- [x] **Step 4: Delegate miette metadata from workflow template errors** + +Implement `miette::Diagnostic` for `Error` or add a wrapper diagnostic used by the CLI so `Error::Template { source, .. }` delegates `source_code`, `labels`, `code`, and `help` to the underlying `TemplateError`. + +- [x] **Step 5: Replace string conversion helper** + +Remove the current `template_parse_error(&TemplateError) -> Error` stringification path in `operations/create.rs` and convert call sites to: + +```rust +.map_err(|err| Error::template("template expansion failed", err))? +``` + +- [x] **Step 6: Verify workflow error tests** + +Run: + +```bash +cargo nextest run -p fabro-workflow template_error_variant_preserves_source_chain +``` + +Expected: test passes. + +## Task 3: Render DOT Attributes Source-Aware After Parse + +**Files:** +- Modify: `lib/crates/fabro-workflow/src/operations/create.rs` +- Modify: `lib/crates/fabro-workflow/src/transforms/variable_expansion.rs` +- Modify: `lib/crates/fabro-workflow/src/pipeline/transform.rs` + +- [x] **Step 1: Add a failing workflow test for inline prompt source context** + +Add a test in `operations/create.rs`: + +```rust +#[test] +fn strict_template_error_for_inline_prompt_names_workflow_file_and_node() { + let dot = r#"digraph ValidatePlan { + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + test_inline_prompt [label="moo" prompt="{{ inputs.foo }}"] + start -> test_inline_prompt -> exit + }"#; + + let err = preprocess_and_validate( + dot, + Some(PathBuf::from(".")), + None, + Vec::new(), + None, + None, + RenderMode::Strict, + &test_catalog(), + ) + .unwrap_err(); + + let rendered = collect_chain(&err).join(": "); + assert!(rendered.contains("test_inline_prompt")); + assert!(rendered.contains("prompt")); + assert!(!rendered.contains("")); +} +``` + +- [x] **Step 2: Run the test and verify it fails** + +Run: + +```bash +cargo nextest run -p fabro-workflow strict_template_error_for_inline_prompt_names_workflow_file_and_node +``` + +Expected: test fails because the pre-parse render path still reports a generic template source. + +- [x] **Step 3: Remove strict pre-parse DOT rendering** + +In `preprocess_and_validate`, delete the initial strict whole-file `render_template(dot_source, ...)` phase. Always parse `dot_source` first, then apply transforms. For `RenderMode::Structural`, keep undefined input warnings by letting the template transform collect diagnostics instead of rendering the whole DOT file. + +- [x] **Step 4: Add source options to the template transform** + +Change `TemplateTransform` to carry: + +```rust +pub struct TemplateTransform { + pub inputs: HashMap, + pub source_name: Option, + pub render_mode: RenderMode, +} +``` + +Render graph attrs, node attrs, and edge attrs one value at a time. Use names like: + +```rust +workflow.fabro graph attribute `goal` +workflow.fabro node `test_inline_prompt` attribute `prompt` +workflow.fabro edge `start -> test_inline_prompt` attribute `label` +``` + +When source path is unknown, use `workflow`. + +- [x] **Step 5: Attach owner context to errors and diagnostics** + +For strict mode, wrap template errors with context: + +```text +template expansion failed in node `test_inline_prompt` attribute `prompt` +``` + +For structural mode undefined variables, produce a warning diagnostic with `node_id` set to the owning node id and `message` including the source path and line when available. + +- [x] **Step 6: Verify workflow transform tests** + +Run: + +```bash +cargo nextest run -p fabro-workflow strict_template_error_for_inline_prompt_names_workflow_file_and_node +cargo nextest run -p fabro-workflow template_transform +``` + +Expected: source-aware test and existing template transform tests pass. + +## Task 4: Make Prompt File Rendering First-Class + +**Files:** +- Modify: `lib/crates/fabro-workflow/src/transforms/file_inlining.rs` +- Modify: `lib/crates/fabro-workflow/src/transforms/variable_expansion.rs` + +- [x] **Step 1: Add a failing test for imported prompt source attribution** + +Add a test that builds a graph with `prompt="@test.md"` and a resolver returning `{{ inputs.foo }}` for `test.md`. Assert strict mode errors mention `test.md`, `test_imported_prompt`, and `prompt`. + +- [x] **Step 2: Run the test and verify it fails** + +Run: + +```bash +cargo nextest run -p fabro-workflow imported_prompt_template_error_names_prompt_file_and_node +``` + +Expected: test fails because file inlining currently loses the prompt file source. + +- [x] **Step 3: Render prompt attribute paths before resolving file references** + +For node `prompt` and graph `goal`, first render the attribute value with the workflow source name so templated paths like `@{{ inputs.prompt_file }}` continue to work. + +- [x] **Step 4: Resolve `@file` references and render file contents with resolved path** + +When a prompt or goal value resolves to a file, render the file contents using: + +```rust +render_named(resolved.path.display().to_string(), &resolved.content, &ctx) +``` + +Wrap failures with node/attr context from the referencing graph value. + +- [x] **Step 5: Keep non-file string attributes on workflow source** + +Only prompt and goal `@file` references switch to the resolved file source. Other string attributes render against their owning workflow/import file source. + +- [x] **Step 6: Verify prompt source tests** + +Run: + +```bash +cargo nextest run -p fabro-workflow imported_prompt_template_error_names_prompt_file_and_node +``` + +Expected: test passes. + +## Task 5: Make Import Rendering Source-Aware + +**Files:** +- Modify: `lib/crates/fabro-workflow/src/transforms/import.rs` + +- [x] **Step 1: Add failing tests for import source attribution** + +Add tests for: + +```rust +#[test] +fn templated_import_path_error_names_importing_node() { /* import="{{ inputs.path }}" */ } + +#[test] +fn imported_workflow_template_error_names_imported_file() { /* imported file contains "{{ inputs.foo }}" */ } +``` + +- [x] **Step 2: Run the tests and verify they fail** + +Run: + +```bash +cargo nextest run -p fabro-workflow templated_import_path_error_names_importing_node imported_workflow_template_error_names_imported_file +``` + +Expected: tests fail with stringified or generic template errors. + +- [x] **Step 3: Render import attributes with node context** + +Before resolving an import path, render the placeholder node's `import` attr with a named template source and wrap failures with the placeholder node id. + +- [x] **Step 4: Parse imported workflows from their own source name** + +When rendering imported workflow content for parse preparation, call `render_named(resolved_file.path.display().to_string(), ...)`. Preserve errors as `Error::Template`. + +- [x] **Step 5: Verify import tests** + +Run: + +```bash +cargo nextest run -p fabro-workflow templated_import_path_error_names_importing_node imported_workflow_template_error_names_imported_file +``` + +Expected: tests pass. + +## Task 6: Serialize Source Coordinates in Validation Diagnostics + +**Files:** +- Modify: `lib/crates/fabro-validate/src/lib.rs` +- Modify: `docs/public/api-reference/fabro-api.yaml` +- Modify: `lib/crates/fabro-server/src/run_manifest.rs` +- Modify: `lib/crates/fabro-cli/src/commands/run/output.rs` + +- [x] **Step 1: Add optional fields to `fabro_validate::Diagnostic`** + +Add: + +```rust +pub source_path: Option, +pub line: Option, +pub column: Option, +pub span_start: Option, +pub span_len: Option, +pub related: Vec, +``` + +Add: + +```rust +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct RelatedDiagnostic { + pub message: String, + pub source_path: Option, + pub line: Option, + pub column: Option, +} +``` + +Update all existing diagnostic constructors with `..Diagnostic::default()` by deriving or implementing `Default` for `Diagnostic`. + +- [x] **Step 2: Update OpenAPI schema** + +Add the same optional fields to `WorkflowDiagnostic` in `docs/public/api-reference/fabro-api.yaml`, and add a `RelatedWorkflowDiagnostic` schema. + +- [x] **Step 3: Regenerate Rust API types** + +Run: + +```bash +cargo build -p fabro-api +``` + +Expected: build succeeds and generated types include the new optional fields. + +- [x] **Step 4: Map diagnostics to and from API DTOs** + +Update `diagnostics_to_api` and CLI `api_diagnostic_to_local` to preserve all new optional fields and related diagnostics. + +- [x] **Step 5: Verify API build** + +Run: + +```bash +cargo build -p fabro-api +``` + +Expected: build succeeds. + +## Task 7: Render CLI Output with Source Context + +**Files:** +- Modify: `lib/crates/fabro-cli/src/main.rs` +- Modify: `lib/crates/fabro-cli/src/shared/utilities.rs` +- Modify: `lib/crates/fabro-cli/tests/it/cmd/validate.rs` +- Modify: `lib/crates/fabro-cli/tests/it/cmd/run.rs` + +- [x] **Step 1: Add CLI snapshot coverage for issue #287** + +Add fixtures or temp workflows for: + +```dot +digraph ValidatePlan { + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + test_inline_prompt [label="moo" prompt="{{ inputs.foo }}"] + start -> test_inline_prompt -> exit +} +``` + +and: + +```dot +digraph ValidatePlan { + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + test_imported_prompt [label="moo" prompt="@test.md"] + start -> test_imported_prompt -> exit +} +``` + +with `test.md` containing: + +```markdown +{{ inputs.foo }} +``` + +- [x] **Step 2: Run snapshots and verify they fail** + +Run: + +```bash +cargo nextest run -p fabro-cli validate_template_source_attribution run_rejects_unbound_template_inputs_before_creating_remote_run +``` + +Expected: snapshots fail because output still contains `` or lacks source context. + +- [x] **Step 3: Delegate fatal miette diagnostics through CLI wrapper** + +Update `CliDiagnostic` so `impl miette::Diagnostic for CliDiagnostic` delegates `source_code`, `labels`, `code`, and `help` to the wrapped error when the wrapped error chain contains a `miette::Diagnostic`. + +- [x] **Step 4: Print validation diagnostics with source coordinates** + +Update `print_diagnostics` so diagnostics with `source_path`, `line`, and `column` print a compact prefix: + +```text +warning: test.md:1:1: undefined template variable `inputs.foo` (template_undefined_variable) +``` + +Keep the existing message shape for diagnostics without source coordinates. + +- [x] **Step 5: Verify CLI snapshots** + +Run: + +```bash +cargo nextest run -p fabro-cli validate_template_source_attribution run_rejects_unbound_template_inputs_before_creating_remote_run +cargo insta pending-snapshots +``` + +Expected: only the intended snapshots are pending. Inspect them, then accept only the intended updates: + +```bash +cargo insta accept +``` + +## Task 8: Add API Validation Coverage + +**Files:** +- Modify: `lib/crates/fabro-server/src/server/tests.rs` + +- [x] **Step 1: Add a validation endpoint test for source fields** + +Add a test that posts a run manifest containing an imported prompt with an undefined template variable and asserts the first diagnostic includes: + +```rust +assert_eq!(diagnostic["source_path"], "test.md"); +assert_eq!(diagnostic["line"], 1); +assert!(diagnostic["node_id"].as_str().unwrap().contains("test_imported_prompt")); +``` + +- [x] **Step 2: Run the server test** + +Run: + +```bash +cargo nextest run -p fabro-server validate_endpoint_returns_template_source_coordinates +``` + +Expected: test passes. + +## Task 9: Final Verification + +**Files:** +- No additional edits. + +- [x] **Step 1: Run focused test suite** + +Run: + +```bash +cargo nextest run -p fabro-template +cargo nextest run -p fabro-workflow +cargo nextest run -p fabro-cli validate_template_source_attribution run_rejects_unbound_template_inputs_before_creating_remote_run +cargo nextest run -p fabro-server validate_endpoint_returns_template_source_coordinates +``` + +Expected: all tests pass. + +- [x] **Step 2: Run API build** + +Run: + +```bash +cargo build -p fabro-api +``` + +Expected: build succeeds. + +- [x] **Step 3: Run formatting check** + +Run: + +```bash +cargo +nightly-2026-04-14 fmt --check --all +``` + +Expected: formatting check passes. + +- [x] **Step 4: Run clippy if the touched crates compile cleanly** + +Run: + +```bash +cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings +``` + +Expected: clippy passes. + +## Assumptions + +- This is a greenfield app, so internal workflow/template APIs and OpenAPI diagnostic DTOs may change. +- Template support after this change is scoped to parsed string attributes and referenced workflow/prompt files, not arbitrary structural DOT templating. +- API responses expose coordinates and context, not full source text. +- Rich source snippets are rendered in local CLI output through `miette`; server/API clients receive structured metadata. +- Existing diagnostics without source metadata keep their current behavior. diff --git a/lib/crates/fabro-cli/src/commands/run/output.rs b/lib/crates/fabro-cli/src/commands/run/output.rs index ced8f83db..33574c330 100644 --- a/lib/crates/fabro-cli/src/commands/run/output.rs +++ b/lib/crates/fabro-cli/src/commands/run/output.rs @@ -70,19 +70,40 @@ pub(crate) fn print_workflow_summary( fn api_diagnostic_to_local(diagnostic: &types::WorkflowDiagnostic) -> fabro_validate::Diagnostic { fabro_validate::Diagnostic { - rule: diagnostic.rule.clone(), - severity: match diagnostic.severity { + rule: diagnostic.rule.clone(), + severity: match diagnostic.severity { types::WorkflowDiagnosticSeverity::Error => fabro_validate::Severity::Error, types::WorkflowDiagnosticSeverity::Warning => fabro_validate::Severity::Warning, types::WorkflowDiagnosticSeverity::Info => fabro_validate::Severity::Info, }, - message: diagnostic.message.clone(), - node_id: diagnostic.node_id.clone(), - edge: diagnostic + message: diagnostic.message.clone(), + node_id: diagnostic.node_id.clone(), + edge: diagnostic .edge .as_ref() .map(|edge| (edge[0].clone(), edge[1].clone())), - fix: diagnostic.fix.clone(), + fix: diagnostic.fix.clone(), + source_path: diagnostic.source_path.clone(), + line: diagnostic.line.and_then(|value| u32::try_from(value).ok()), + column: diagnostic + .column + .and_then(|value| u32::try_from(value).ok()), + span_start: diagnostic + .span_start + .and_then(|value| usize::try_from(value).ok()), + span_len: diagnostic + .span_len + .and_then(|value| usize::try_from(value).ok()), + related: diagnostic + .related + .iter() + .map(|related| fabro_validate::RelatedDiagnostic { + message: related.message.clone(), + source_path: related.source_path.clone(), + line: related.line.and_then(|value| u32::try_from(value).ok()), + column: related.column.and_then(|value| u32::try_from(value).ok()), + }) + .collect(), } } diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 9bc8471f1..f5b3c4f27 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -148,6 +148,13 @@ impl CliDiagnostic { show_auth_hint, } } + + fn delegated_diagnostic(&self) -> Option<&dyn miette::Diagnostic> { + self.err.chain().find_map(|err| { + err.downcast_ref::() + .map(|err| err as &dyn miette::Diagnostic) + }) + } } impl Display for CliDiagnostic { @@ -169,13 +176,34 @@ impl std::error::Error for CliDiagnostic { } impl miette::Diagnostic for CliDiagnostic { + fn code<'a>(&'a self) -> Option> { + self.delegated_diagnostic() + .and_then(miette::Diagnostic::code) + } + fn help<'a>(&'a self) -> Option> { if self.show_auth_hint && exit::exit_class_for(&self.err) == Some(ExitClass::AuthRequired) { Some(Box::new("Run `fabro auth login` to authenticate.")) } else { - None + self.delegated_diagnostic() + .and_then(miette::Diagnostic::help) } } + + fn source_code(&self) -> Option<&dyn miette::SourceCode> { + self.delegated_diagnostic() + .and_then(miette::Diagnostic::source_code) + } + + fn labels(&self) -> Option + '_>> { + self.delegated_diagnostic() + .and_then(miette::Diagnostic::labels) + } + + fn diagnostic_source(&self) -> Option<&dyn miette::Diagnostic> { + self.delegated_diagnostic() + .and_then(miette::Diagnostic::diagnostic_source) + } } #[expect( diff --git a/lib/crates/fabro-cli/src/shared/utilities.rs b/lib/crates/fabro-cli/src/shared/utilities.rs index 7f3c9f20f..8e3d06448 100644 --- a/lib/crates/fabro-cli/src/shared/utilities.rs +++ b/lib/crates/fabro-cli/src/shared/utilities.rs @@ -54,32 +54,78 @@ pub(crate) fn print_diagnostics(diagnostics: &[Diagnostic], styles: &Styles, pri (_, Some((from, to))) => format!(" [edge: {from} -> {to}]"), _ => String::new(), }; + let source_prefix = source_prefix(d); match d.severity { - Severity::Error => fabro_util::printerr!( + Severity::Error if source_prefix.is_empty() => fabro_util::printerr!( printer, "{}{location}: {} ({})", styles.red.apply_to("error"), d.message, styles.dim.apply_to(&d.rule), ), - Severity::Warning => fabro_util::printerr!( + Severity::Error => fabro_util::printerr!( + printer, + "{}: {source_prefix}{}{location} ({})", + styles.red.apply_to("error"), + d.message, + styles.dim.apply_to(&d.rule), + ), + Severity::Warning if source_prefix.is_empty() => fabro_util::printerr!( printer, "{}{location}: {} ({})", styles.yellow.apply_to("warning"), d.message, styles.dim.apply_to(&d.rule), ), + Severity::Warning => fabro_util::printerr!( + printer, + "{}: {source_prefix}{}{location} ({})", + styles.yellow.apply_to("warning"), + d.message, + styles.dim.apply_to(&d.rule), + ), Severity::Info => fabro_util::printerr!( printer, "{}", - styles - .dim - .apply_to(format!("info{location}: {} ({})", d.message, d.rule)), + styles.dim.apply_to(if source_prefix.is_empty() { + format!("info{location}: {} ({})", d.message, d.rule) + } else { + format!("info: {source_prefix}{}{location} ({})", d.message, d.rule) + }), ), } } } +fn source_prefix(diagnostic: &Diagnostic) -> String { + match ( + diagnostic.source_path.as_deref(), + diagnostic.line, + diagnostic.column, + ) { + (Some(path), Some(line), Some(column)) => { + format!("{}:{line}:{column}: ", display_diagnostic_path(path)) + } + (Some(path), Some(line), None) => { + format!("{}:{line}: ", display_diagnostic_path(path)) + } + (Some(path), None, _) => format!("{}: ", display_diagnostic_path(path)), + _ => String::new(), + } +} + +fn display_diagnostic_path(path: &str) -> String { + let path = Path::new(path); + if let Ok(canonical) = path.canonicalize() { + return relative_path(&canonical); + } + if path.is_absolute() { + relative_path(path) + } else { + path.display().to_string() + } +} + pub(crate) fn relative_path(path: &Path) -> String { if let Ok(cwd) = std::env::current_dir() { if let Ok(rel) = path.strip_prefix(&cwd) { diff --git a/lib/crates/fabro-cli/tests/it/cmd/preflight.rs b/lib/crates/fabro-cli/tests/it/cmd/preflight.rs index 6080c8cad..e90c1a9d6 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/preflight.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/preflight.rs @@ -73,8 +73,8 @@ fn preflight_rejects_unbound_template_inputs() { Graph: [FIXTURES]/templated_unbound.fabro Goal: Demo - error: undefined template variable `inputs.app_dir` at line 1 (template_undefined_variable) - error [node: work]: undefined template variable `inputs.app_dir` in node `work` (template_undefined_variable) + error: [FIXTURES]/templated_unbound.fabro:2:26: undefined template variable `inputs.app_dir` in graph attribute `goal` (template_undefined_variable) + error: [FIXTURES]/templated_unbound.fabro:7:44: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) × Validation failed "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index 29e76ef01..6044a3af0 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -693,6 +693,14 @@ fn run_rejects_unbound_template_inputs_before_creating_remote_run() { stderr.contains("inputs.app_dir"), "stderr should name the unbound variable: {stderr}" ); + assert!( + stderr.contains("templated_unbound.fabro"), + "stderr should name the workflow source: {stderr}" + ); + assert!( + !stderr.contains(""), + "stderr should not expose MiniJinja's generic source name: {stderr}" + ); } #[test] diff --git a/lib/crates/fabro-cli/tests/it/cmd/validate.rs b/lib/crates/fabro-cli/tests/it/cmd/validate.rs index a76c675c9..21bebd5fa 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/validate.rs @@ -162,8 +162,8 @@ fn bare_fabro_with_unbound_inputs_validates_structurally_with_warning() { ----- stderr ----- Workflow: TemplatedUnbound (3 nodes, 2 edges) Graph: [FIXTURES]/templated_unbound.fabro - warning: undefined template variable `inputs.app_dir` at line 1 (template_undefined_variable) - warning [node: work]: undefined template variable `inputs.app_dir` in node `work` (template_undefined_variable) + warning: [FIXTURES]/templated_unbound.fabro:2:26: undefined template variable `inputs.app_dir` in graph attribute `goal` (template_undefined_variable) + warning: [FIXTURES]/templated_unbound.fabro:7:44: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) Validation: OK "); } @@ -185,7 +185,7 @@ fn bare_fabro_with_unbound_inputs_in_imported_prompt_validates_structurally_with ----- stderr ----- Workflow: TemplatedUnboundImported (3 nodes, 2 edges) Graph: [FIXTURES]/templated_unbound_imported/workflow.fabro - warning [node: work]: undefined template variable `inputs.app_dir` in node `work` (template_undefined_variable) + warning: [FIXTURES]/templated_unbound_imported/work.md:1:12: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) Validation: OK "); } diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index dbc073632..04eb0bac1 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -1263,19 +1263,40 @@ fn diagnostics_to_api( diagnostics .iter() .map(|diagnostic| types::WorkflowDiagnostic { - edge: diagnostic + column: diagnostic + .column + .and_then(|value| i32::try_from(value).ok()), + edge: diagnostic .edge .as_ref() .map(|edge: &(String, String)| [edge.0.clone(), edge.1.clone()]), - fix: diagnostic.fix.clone(), - message: diagnostic.message.clone(), - node_id: diagnostic.node_id.clone(), - rule: diagnostic.rule.clone(), - severity: match diagnostic.severity { + fix: diagnostic.fix.clone(), + line: diagnostic.line.and_then(|value| i32::try_from(value).ok()), + message: diagnostic.message.clone(), + node_id: diagnostic.node_id.clone(), + related: diagnostic + .related + .iter() + .map(|related| types::RelatedWorkflowDiagnostic { + column: related.column.and_then(|value| i32::try_from(value).ok()), + line: related.line.and_then(|value| i32::try_from(value).ok()), + message: related.message.clone(), + source_path: related.source_path.clone(), + }) + .collect(), + rule: diagnostic.rule.clone(), + severity: match diagnostic.severity { Severity::Error => types::WorkflowDiagnosticSeverity::Error, Severity::Warning => types::WorkflowDiagnosticSeverity::Warning, Severity::Info => types::WorkflowDiagnosticSeverity::Info, }, + source_path: diagnostic.source_path.clone(), + span_len: diagnostic + .span_len + .and_then(|value| i64::try_from(value).ok()), + span_start: diagnostic + .span_start + .and_then(|value| i64::try_from(value).ok()), }) .collect() } diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 953031292..e4ab4d210 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -3050,6 +3050,68 @@ reasoning = false ); } +#[tokio::test] +async fn validate_endpoint_returns_template_source_coordinates() { + let app = test_app_with(); + let dot = r#"digraph ValidatePlan { + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + test_imported_prompt [label="moo" prompt="@test.md"] + start -> test_imported_prompt -> exit + }"#; + let manifest = serde_json::json!({ + "version": 1, + "cwd": "/tmp", + "target": { + "identifier": "workflow.fabro", + "path": "workflow.fabro", + }, + "workflows": { + "workflow.fabro": { + "source": dot, + "files": { + "test.md": { + "content": "{{ inputs.foo }}", + "ref": { + "type": "file_inline", + "original": "test.md", + "from": "workflow.fabro", + }, + }, + }, + }, + }, + }); + + let response = app + .oneshot( + Request::builder() + .method("POST") + .uri(api("/validate")) + .header("content-type", "application/json") + .body(Body::from(serde_json::to_string(&manifest).unwrap())) + .unwrap(), + ) + .await + .unwrap(); + let body = response_json!(response, StatusCode::OK).await; + let diagnostics = body["workflow"]["diagnostics"].as_array().unwrap(); + let diagnostic = diagnostics + .iter() + .find(|diagnostic| diagnostic["rule"] == "template_undefined_variable") + .expect("expected template diagnostic"); + + assert_eq!(diagnostic["source_path"], "test.md"); + assert_eq!(diagnostic["line"], 1); + assert_eq!(diagnostic["column"], 4); + assert!( + diagnostic["node_id"] + .as_str() + .unwrap() + .contains("test_imported_prompt") + ); +} + async fn create_run_for_target(app: &Router, target_path: &str, dot_source: &str) -> String { let req = Request::builder() .method("POST") diff --git a/lib/crates/fabro-template/Cargo.toml b/lib/crates/fabro-template/Cargo.toml index ce31208e0..93dcf1272 100644 --- a/lib/crates/fabro-template/Cargo.toml +++ b/lib/crates/fabro-template/Cargo.toml @@ -15,6 +15,7 @@ workspace = true [dependencies] anyhow.workspace = true fabro-util = { path = "../fabro-util" } +miette.workspace = true minijinja = { workspace = true, features = ["debug"] } serde.workspace = true thiserror.workspace = true diff --git a/lib/crates/fabro-template/src/lib.rs b/lib/crates/fabro-template/src/lib.rs index 6c3b5b09c..3dff5e0fe 100644 --- a/lib/crates/fabro-template/src/lib.rs +++ b/lib/crates/fabro-template/src/lib.rs @@ -3,9 +3,9 @@ use std::fmt; use std::sync::Arc; use fabro_util::env::Env; +use miette::{LabeledSpan, NamedSource, SourceCode, SourceSpan}; use minijinja::value::{Object, Value}; use minijinja::{AutoEscape, Environment, ErrorKind, UndefinedBehavior}; -use thiserror::Error; #[derive(Debug, Default, Clone)] pub struct TemplateContext { @@ -115,33 +115,66 @@ where /// MiniJinja knows about (offending expression, line) plus the original /// `minijinja::Error` as `#[source]`, so the cause chain is preserved across /// boundaries that walk `Error::source()` (anyhow, miette, `collect_chain`). -#[derive(Debug, Error)] +#[derive(Debug)] pub enum TemplateError { - #[error("template syntax error{location}", location = fmt_location(*line))] Syntax { - line: Option, - #[source] - source: minijinja::Error, + line: Option, + source_name: Option, + source_text: Option, + span: Option, + source_code: Option>>, + source: Box, }, - #[error( - "undefined template variable{expr}{location}", - expr = fmt_expr(expression.as_deref()), - location = fmt_location(*line), - )] UndefinedVariable { - expression: Option, - line: Option, - #[source] - source: minijinja::Error, + expression: Option, + line: Option, + source_name: Option, + source_text: Option, + span: Option, + source_code: Option>>, + source: Box, }, - #[error("template render error{location}", location = fmt_location(*line))] Render { - line: Option, - #[source] - source: minijinja::Error, + line: Option, + source_name: Option, + source_text: Option, + span: Option, + source_code: Option>>, + source: Box, }, } +impl fmt::Display for TemplateError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Syntax { line, .. } => { + write!(f, "template syntax error{}", fmt_location(*line)) + } + Self::UndefinedVariable { + expression, line, .. + } => write!( + f, + "undefined template variable{}{}", + fmt_expr(expression.as_deref()), + fmt_location(*line) + ), + Self::Render { line, .. } => { + write!(f, "template render error{}", fmt_location(*line)) + } + } + } +} + +impl std::error::Error for TemplateError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::Syntax { source, .. } + | Self::UndefinedVariable { source, .. } + | Self::Render { source, .. } => Some(source.as_ref()), + } + } +} + fn fmt_expr(expression: Option<&str>) -> String { expression.map(|e| format!(" `{e}`")).unwrap_or_default() } @@ -161,27 +194,148 @@ fn extract_expression(error: &minijinja::Error) -> Option { impl From for TemplateError { fn from(error: minijinja::Error) -> Self { let line = error.line().and_then(|n| u32::try_from(n).ok()); + let source_name = error.name().map(str::to_owned); + let source_text = error.template_source().map(str::to_owned); + let span = error.range().and_then(|range| { + let start = range.start; + let len = range.end.checked_sub(range.start)?; + Some((start, len).into()) + }); + let source_code = source_name + .as_ref() + .zip(source_text.as_ref()) + .map(|(name, source)| Box::new(NamedSource::new(name.clone(), source.clone()))); match error.kind() { ErrorKind::SyntaxError => Self::Syntax { line, - source: error, + source_name, + source_text, + span, + source_code, + source: Box::new(error), }, ErrorKind::UndefinedError => { let expression = extract_expression(&error); Self::UndefinedVariable { expression, line, - source: error, + source_name, + source_text, + span, + source_code, + source: Box::new(error), } } _ => Self::Render { line, - source: error, + source_name, + source_text, + span, + source_code, + source: Box::new(error), }, } } } +impl TemplateError { + #[must_use] + pub fn expression(&self) -> Option<&str> { + match self { + Self::UndefinedVariable { expression, .. } => expression.as_deref(), + Self::Syntax { .. } | Self::Render { .. } => None, + } + } + + #[must_use] + pub fn line(&self) -> Option { + match self { + Self::Syntax { line, .. } + | Self::UndefinedVariable { line, .. } + | Self::Render { line, .. } => *line, + } + } + + #[must_use] + pub fn source_name(&self) -> Option<&str> { + match self { + Self::Syntax { source_name, .. } + | Self::UndefinedVariable { source_name, .. } + | Self::Render { source_name, .. } => source_name.as_deref(), + } + } + + #[must_use] + pub fn source_text(&self) -> Option<&str> { + match self { + Self::Syntax { source_text, .. } + | Self::UndefinedVariable { source_text, .. } + | Self::Render { source_text, .. } => source_text.as_deref(), + } + } + + #[must_use] + pub fn span(&self) -> Option { + match self { + Self::Syntax { span, .. } + | Self::UndefinedVariable { span, .. } + | Self::Render { span, .. } => *span, + } + } + + #[must_use] + pub fn column(&self) -> Option { + let source_text = self.source_text()?; + let offset = self.span()?.offset(); + if offset > source_text.len() || !source_text.is_char_boundary(offset) { + return None; + } + let line_start = source_text[..offset] + .rfind('\n') + .map_or(0, |index| index + 1); + u32::try_from(source_text[line_start..offset].chars().count() + 1).ok() + } + + fn source_code_ref(&self) -> Option<&NamedSource> { + match self { + Self::Syntax { source_code, .. } + | Self::UndefinedVariable { source_code, .. } + | Self::Render { source_code, .. } => source_code.as_deref(), + } + } +} + +impl miette::Diagnostic for TemplateError { + fn code<'a>(&'a self) -> Option> { + let code = match self { + Self::Syntax { .. } => "fabro::template::syntax", + Self::UndefinedVariable { .. } => "fabro::template::undefined_variable", + Self::Render { .. } => "fabro::template::render", + }; + Some(Box::new(code)) + } + + fn source_code(&self) -> Option<&dyn SourceCode> { + self.source_code_ref() + .map(|source| source as &dyn SourceCode) + } + + fn labels(&self) -> Option + '_>> { + let span = self.span()?; + let label = match self { + Self::UndefinedVariable { expression, .. } => expression.as_ref().map_or_else( + || "undefined variable".to_string(), + |expr| format!("`{expr}`"), + ), + Self::Syntax { .. } => "syntax error".to_string(), + Self::Render { .. } => "render error".to_string(), + }; + Some(Box::new( + vec![LabeledSpan::new_primary_with_span(Some(label), span)].into_iter(), + )) + } +} + /// Returns `true` when the string contains MiniJinja delimiter syntax. #[must_use] pub fn contains_template_syntax(template: &str) -> bool { @@ -195,7 +349,15 @@ fn is_plain_text(template: &str) -> bool { } pub fn render(template: &str, ctx: &TemplateContext) -> Result { - render_with(template, ctx, UndefinedBehavior::Strict) + render_with(None, template, ctx, UndefinedBehavior::Strict) +} + +pub fn render_named( + name: impl Into, + template: &str, + ctx: &TemplateContext, +) -> Result { + render_with(Some(name.into()), template, ctx, UndefinedBehavior::Strict) } /// Render with chainable undefined handling: undefined variables and attribute @@ -203,10 +365,24 @@ pub fn render(template: &str, ctx: &TemplateContext) -> Result Result { - render_with(template, ctx, UndefinedBehavior::Chainable) + render_with(None, template, ctx, UndefinedBehavior::Chainable) +} + +pub fn render_lenient_named( + name: impl Into, + template: &str, + ctx: &TemplateContext, +) -> Result { + render_with( + Some(name.into()), + template, + ctx, + UndefinedBehavior::Chainable, + ) } fn render_with( + name: Option, template: &str, ctx: &TemplateContext, undefined: UndefinedBehavior, @@ -218,8 +394,11 @@ fn render_with( env.set_undefined_behavior(undefined); env.set_auto_escape_callback(|_| AutoEscape::None); env.set_debug(true); - env.render_str(template, ctx.clone().into_value()) - .map_err(TemplateError::from) + match name { + Some(name) => env.render_named_str(&name, template, ctx.clone().into_value()), + None => env.render_str(template, ctx.clone().into_value()), + } + .map_err(TemplateError::from) } #[cfg(test)] @@ -322,6 +501,40 @@ mod tests { assert!(matches!(err, TemplateError::Syntax { .. })); } + #[test] + fn render_named_reports_source_name_expression_and_span() { + let ctx = TemplateContext::new(); + let err = render_named("prompts/test.md", "Hello {{ inputs.foo }}", &ctx).unwrap_err(); + + let TemplateError::UndefinedVariable { + expression, + line, + source_name, + span, + .. + } = err + else { + panic!("expected undefined variable error"); + }; + + assert_eq!(expression.as_deref(), Some("inputs.foo")); + assert_eq!(line, Some(1)); + assert_eq!(source_name.as_deref(), Some("prompts/test.md")); + assert!(span.is_some()); + } + + #[test] + fn render_lenient_named_preserves_source_name_for_syntax_errors() { + let ctx = TemplateContext::new(); + let err = render_lenient_named("workflow.fabro", "{{ unterminated", &ctx).unwrap_err(); + + let TemplateError::Syntax { source_name, .. } = err else { + panic!("expected syntax error"); + }; + + assert_eq!(source_name.as_deref(), Some("workflow.fabro")); + } + #[test] fn rejects_undefined_variables_in_strict_mode() { let ctx = TemplateContext::new(); diff --git a/lib/crates/fabro-validate/src/lib.rs b/lib/crates/fabro-validate/src/lib.rs index 55294bc1d..c3d97c7f5 100644 --- a/lib/crates/fabro-validate/src/lib.rs +++ b/lib/crates/fabro-validate/src/lib.rs @@ -15,12 +15,46 @@ pub enum Severity { /// A validation diagnostic produced by a lint rule. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Diagnostic { - pub rule: String, - pub severity: Severity, - pub message: String, - pub node_id: Option, - pub edge: Option<(String, String)>, - pub fix: Option, + pub rule: String, + pub severity: Severity, + pub message: String, + pub node_id: Option, + pub edge: Option<(String, String)>, + pub fix: Option, + pub source_path: Option, + pub line: Option, + pub column: Option, + pub span_start: Option, + pub span_len: Option, + #[serde(default)] + pub related: Vec, +} + +impl Default for Diagnostic { + fn default() -> Self { + Self { + rule: String::new(), + severity: Severity::Info, + message: String::new(), + node_id: None, + edge: None, + fix: None, + source_path: None, + line: None, + column: None, + span_start: None, + span_len: None, + related: Vec::new(), + } + } +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct RelatedDiagnostic { + pub message: String, + pub source_path: Option, + pub line: Option, + pub column: Option, } /// A lint rule that validates a graph. @@ -254,12 +288,14 @@ reasoning = false } fn apply(&self, _graph: &Graph) -> Vec { vec![Diagnostic { - rule: "always_warn".to_string(), + rule: "always_warn".to_string(), severity: Severity::Warning, - message: "custom warning".to_string(), - node_id: None, - edge: None, - fix: None, + message: "custom warning".to_string(), + node_id: None, + edge: None, + fix: None, + + ..Diagnostic::default() }] } } diff --git a/lib/crates/fabro-validate/src/rules/all_conditional_edges.rs b/lib/crates/fabro-validate/src/rules/all_conditional_edges.rs index f2ce2819b..81028c99f 100644 --- a/lib/crates/fabro-validate/src/rules/all_conditional_edges.rs +++ b/lib/crates/fabro-validate/src/rules/all_conditional_edges.rs @@ -36,7 +36,8 @@ impl LintRule for Rule { fix: Some( "Add at least one unconditional edge as a fallback".to_string(), ), - }); + + ..Diagnostic::default()}); } } diagnostics diff --git a/lib/crates/fabro-validate/src/rules/backend_valid.rs b/lib/crates/fabro-validate/src/rules/backend_valid.rs index 9a04af737..19014be56 100644 --- a/lib/crates/fabro-validate/src/rules/backend_valid.rs +++ b/lib/crates/fabro-validate/src/rules/backend_valid.rs @@ -22,29 +22,33 @@ impl LintRule for Rule { Some(Err(_)) => { let expected = LlmBackend::expected_values(); diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!( + message: format!( "unsupported LLM backend \"{backend}\"; expected one of: {expected}" ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(format!("Use one of: {expected}")), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!("Use one of: {expected}")), + + ..Diagnostic::default() }); } Some(Ok(LlmBackend::Acp)) if acp_command_missing(node) => { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: "backend=\"acp\" requires acp_command because Fabro does \ + message: "backend=\"acp\" requires acp_command because Fabro does \ not install ACP agents" .to_string(), - node_id: Some(node.id.clone()), - edge: None, - fix: Some( + node_id: Some(node.id.clone()), + edge: None, + fix: Some( "Set acp_command to a stdio ACP command available in the sandbox" .to_string(), ), + + ..Diagnostic::default() }); } Some(Ok(_)) | None => {} diff --git a/lib/crates/fabro-validate/src/rules/condition_syntax.rs b/lib/crates/fabro-validate/src/rules/condition_syntax.rs index e0db57600..986422e7a 100644 --- a/lib/crates/fabro-validate/src/rules/condition_syntax.rs +++ b/lib/crates/fabro-validate/src/rules/condition_syntax.rs @@ -25,19 +25,21 @@ impl LintRule for Rule { } if let Err(e) = parse_condition(condition) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!( + message: format!( "Condition '{condition}' on edge {} -> {} failed parse: {e}", edge.from, edge.to ), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some( + node_id: None, + edge: Some((edge.from.clone(), edge.to.clone())), + fix: Some( "Use key=value, key!=value, key>value, key contains value, \ key matches pattern, or bare key syntax" .to_string(), ), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/direction_valid.rs b/lib/crates/fabro-validate/src/rules/direction_valid.rs index c29f5f990..1e50b94a8 100644 --- a/lib/crates/fabro-validate/src/rules/direction_valid.rs +++ b/lib/crates/fabro-validate/src/rules/direction_valid.rs @@ -23,12 +23,14 @@ impl LintRule for Rule { return Vec::new(); } vec![Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!("Graph has invalid rankdir '{rankdir}'"), - node_id: None, - edge: None, - fix: Some(format!("Use one of: {}", VALID_DIRECTIONS.join(", "))), + message: format!("Graph has invalid rankdir '{rankdir}'"), + node_id: None, + edge: None, + fix: Some(format!("Use one of: {}", VALID_DIRECTIONS.join(", "))), + + ..Diagnostic::default() }] } } diff --git a/lib/crates/fabro-validate/src/rules/edge_target_exists.rs b/lib/crates/fabro-validate/src/rules/edge_target_exists.rs index 8b4fa5930..8cfe67282 100644 --- a/lib/crates/fabro-validate/src/rules/edge_target_exists.rs +++ b/lib/crates/fabro-validate/src/rules/edge_target_exists.rs @@ -18,28 +18,32 @@ impl LintRule for Rule { for edge in &graph.edges { if !graph.nodes.contains_key(&edge.to) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!( + message: format!( "Edge from '{}' targets non-existent node '{}'", edge.from, edge.to ), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(format!("Define node '{}' or fix the edge target", edge.to)), + node_id: None, + edge: Some((edge.from.clone(), edge.to.clone())), + fix: Some(format!("Define node '{}' or fix the edge target", edge.to)), + + ..Diagnostic::default() }); } if !graph.nodes.contains_key(&edge.from) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!("Edge source '{}' references non-existent node", edge.from), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(format!( + message: format!("Edge source '{}' references non-existent node", edge.from), + node_id: None, + edge: Some((edge.from.clone(), edge.to.clone())), + fix: Some(format!( "Define node '{}' or fix the edge source", edge.from )), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/exit_no_outgoing.rs b/lib/crates/fabro-validate/src/rules/exit_no_outgoing.rs index 56aaa6539..5ea797d34 100644 --- a/lib/crates/fabro-validate/src/rules/exit_no_outgoing.rs +++ b/lib/crates/fabro-validate/src/rules/exit_no_outgoing.rs @@ -25,16 +25,18 @@ impl LintRule for Rule { let outgoing = graph.outgoing_edges(&node.id); if !outgoing.is_empty() { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!( + message: format!( "Exit node '{}' has {} outgoing edge(s) but must have none", node.id, outgoing.len() ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some("Remove outgoing edges from the exit node".to_string()), + node_id: Some(node.id.clone()), + edge: None, + fix: Some("Remove outgoing edges from the exit node".to_string()), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/fidelity_valid.rs b/lib/crates/fabro-validate/src/rules/fidelity_valid.rs index 38cd957b5..a3449b3ce 100644 --- a/lib/crates/fabro-validate/src/rules/fidelity_valid.rs +++ b/lib/crates/fabro-validate/src/rules/fidelity_valid.rs @@ -32,15 +32,17 @@ impl LintRule for Rule { if let Some(fidelity) = node.fidelity() { if fidelity.parse::().is_err() { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( + message: format!( "Node '{}' has invalid fidelity mode '{fidelity}'", node.id ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(Self::fix_message()), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(Self::fix_message()), + + ..Diagnostic::default() }); } } @@ -49,15 +51,17 @@ impl LintRule for Rule { if let Some(fidelity) = edge.fidelity() { if fidelity.parse::().is_err() { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( + message: format!( "Edge {} -> {} has invalid fidelity mode '{fidelity}'", edge.from, edge.to ), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(Self::fix_message()), + node_id: None, + edge: Some((edge.from.clone(), edge.to.clone())), + fix: Some(Self::fix_message()), + + ..Diagnostic::default() }); } } @@ -65,12 +69,14 @@ impl LintRule for Rule { if let Some(fidelity) = graph.default_fidelity() { if fidelity.parse::().is_err() { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!("Graph has invalid default_fidelity '{fidelity}'"), - node_id: None, - edge: None, - fix: Some(Self::fix_message()), + message: format!("Graph has invalid default_fidelity '{fidelity}'"), + node_id: None, + edge: None, + fix: Some(Self::fix_message()), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/freeform_edge_count.rs b/lib/crates/fabro-validate/src/rules/freeform_edge_count.rs index 103157029..d16ae4fc4 100644 --- a/lib/crates/fabro-validate/src/rules/freeform_edge_count.rs +++ b/lib/crates/fabro-validate/src/rules/freeform_edge_count.rs @@ -35,7 +35,8 @@ impl LintRule for Rule { fix: Some( "Remove extra freeform=true edges so at most one remains".to_string(), ), - }); + + ..Diagnostic::default()}); } } } diff --git a/lib/crates/fabro-validate/src/rules/goal_gate_has_retry.rs b/lib/crates/fabro-validate/src/rules/goal_gate_has_retry.rs index 5cfdc9108..3e4c52f0b 100644 --- a/lib/crates/fabro-validate/src/rules/goal_gate_has_retry.rs +++ b/lib/crates/fabro-validate/src/rules/goal_gate_has_retry.rs @@ -34,7 +34,8 @@ impl LintRule for Rule { fix: Some( "Add retry_target or fallback_retry_target attribute".to_string(), ), - }); + + ..Diagnostic::default()}); } } } diff --git a/lib/crates/fabro-validate/src/rules/import_error.rs b/lib/crates/fabro-validate/src/rules/import_error.rs index a2377c103..e615a09b0 100644 --- a/lib/crates/fabro-validate/src/rules/import_error.rs +++ b/lib/crates/fabro-validate/src/rules/import_error.rs @@ -19,26 +19,30 @@ impl LintRule for Rule { for node in graph.nodes.values() { if let Some(AttrValue::String(message)) = node.attrs.get("import_error") { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: message.clone(), - node_id: Some(node.id.clone()), - edge: None, - fix: Some("Fix the imported workflow or import path".to_string()), + message: message.clone(), + node_id: Some(node.id.clone()), + edge: None, + fix: Some("Fix the imported workflow or import path".to_string()), + + ..Diagnostic::default() }); } if node.attrs.contains_key("import") { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: "unresolved import (no base directory available)".to_string(), - node_id: Some(node.id.clone()), - edge: None, - fix: Some( + message: "unresolved import (no base directory available)".to_string(), + node_id: Some(node.id.clone()), + edge: None, + fix: Some( "Load the workflow from a file so imports can resolve relative to it" .to_string(), ), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/model_support.rs b/lib/crates/fabro-validate/src/rules/model_support.rs index f6749e899..91df02991 100644 --- a/lib/crates/fabro-validate/src/rules/model_support.rs +++ b/lib/crates/fabro-validate/src/rules/model_support.rs @@ -19,6 +19,8 @@ pub(super) fn check_model_known( node_id, edge: None, fix: Some("Use a model ID from `fabro model list`".to_string()), + + ..Diagnostic::default() }) } @@ -48,5 +50,7 @@ pub(super) fn check_provider_known( node_id, edge: None, fix: Some(format!("Use one of: {valid_str}")), + + ..Diagnostic::default() }) } diff --git a/lib/crates/fabro-validate/src/rules/orphan_custom_outcome.rs b/lib/crates/fabro-validate/src/rules/orphan_custom_outcome.rs index 21b3c5dd2..8f64ea533 100644 --- a/lib/crates/fabro-validate/src/rules/orphan_custom_outcome.rs +++ b/lib/crates/fabro-validate/src/rules/orphan_custom_outcome.rs @@ -48,7 +48,8 @@ impl LintRule for Rule { "Add an unconditional edge as a safety net for unmatched outcomes" .to_string(), ), - }); + + ..Diagnostic::default()}); } } diagnostics diff --git a/lib/crates/fabro-validate/src/rules/prompt_on_llm_nodes.rs b/lib/crates/fabro-validate/src/rules/prompt_on_llm_nodes.rs index cc90b3a9a..03cd130b2 100644 --- a/lib/crates/fabro-validate/src/rules/prompt_on_llm_nodes.rs +++ b/lib/crates/fabro-validate/src/rules/prompt_on_llm_nodes.rs @@ -25,15 +25,14 @@ impl LintRule for Rule { .is_some_and(|l| !l.is_empty()); if !has_prompt && !has_label { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( - "LLM node '{}' has no prompt or label attribute", - node.id - ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some("Add a prompt or label attribute".to_string()), + message: format!("LLM node '{}' has no prompt or label attribute", node.id), + node_id: Some(node.id.clone()), + edge: None, + fix: Some("Add a prompt or label attribute".to_string()), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/random_selection_no_conditions.rs b/lib/crates/fabro-validate/src/rules/random_selection_no_conditions.rs index f12ec9520..b305443d2 100644 --- a/lib/crates/fabro-validate/src/rules/random_selection_no_conditions.rs +++ b/lib/crates/fabro-validate/src/rules/random_selection_no_conditions.rs @@ -36,7 +36,8 @@ impl LintRule for Rule { fix: Some( "Remove the condition attributes from outgoing edges, or remove selection=\"random\" from the node".to_string(), ), - }); + + ..Diagnostic::default()}); } } diagnostics diff --git a/lib/crates/fabro-validate/src/rules/reachability.rs b/lib/crates/fabro-validate/src/rules/reachability.rs index b30d73012..ca26a1f9d 100644 --- a/lib/crates/fabro-validate/src/rules/reachability.rs +++ b/lib/crates/fabro-validate/src/rules/reachability.rs @@ -44,14 +44,16 @@ impl LintRule for Rule { unreachable .into_iter() .map(|node_id| Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!("Node '{node_id}' is not reachable from the start node"), - node_id: Some(node_id.to_string()), - edge: None, - fix: Some(format!( + message: format!("Node '{node_id}' is not reachable from the start node"), + node_id: Some(node_id.to_string()), + edge: None, + fix: Some(format!( "Add an edge path from the start node to '{node_id}'" )), + + ..Diagnostic::default() }) .collect() } diff --git a/lib/crates/fabro-validate/src/rules/reserved_keyword_node_id.rs b/lib/crates/fabro-validate/src/rules/reserved_keyword_node_id.rs index 414dea9fc..a60a2edd2 100644 --- a/lib/crates/fabro-validate/src/rules/reserved_keyword_node_id.rs +++ b/lib/crates/fabro-validate/src/rules/reserved_keyword_node_id.rs @@ -23,19 +23,21 @@ impl LintRule for Rule { .values() .filter(|node| DOT_RESERVED_KEYWORDS.contains(&node.id.to_lowercase().as_str())) .map(|node| Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( + message: format!( "Node ID '{}' is a DOT reserved keyword and may cause parsing failures", node.id ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(format!( + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!( "Rename '{}' to '{}_step' or another non-reserved ID", node.id, node.id.to_lowercase() )), + + ..Diagnostic::default() }) .collect() } diff --git a/lib/crates/fabro-validate/src/rules/retry_target_exists.rs b/lib/crates/fabro-validate/src/rules/retry_target_exists.rs index 0bb1e1d8b..1c66ca824 100644 --- a/lib/crates/fabro-validate/src/rules/retry_target_exists.rs +++ b/lib/crates/fabro-validate/src/rules/retry_target_exists.rs @@ -19,32 +19,36 @@ impl LintRule for Rule { if let Some(target) = node.retry_target() { if !graph.nodes.contains_key(target) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( + message: format!( "Node '{}' has retry_target '{}' that does not exist", node.id, target ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(format!("Define node '{target}' or fix retry_target")), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!("Define node '{target}' or fix retry_target")), + + ..Diagnostic::default() }); } } if let Some(target) = node.fallback_retry_target() { if !graph.nodes.contains_key(target) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( + message: format!( "Node '{}' has fallback_retry_target '{}' that does not exist", node.id, target ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(format!( + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!( "Define node '{target}' or fix fallback_retry_target" )), + + ..Diagnostic::default() }); } } @@ -52,28 +56,32 @@ impl LintRule for Rule { if let Some(target) = graph.retry_target() { if !graph.nodes.contains_key(target) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!("Graph has retry_target '{target}' that does not exist"), - node_id: None, - edge: None, - fix: Some(format!("Define node '{target}' or fix graph retry_target")), + message: format!("Graph has retry_target '{target}' that does not exist"), + node_id: None, + edge: None, + fix: Some(format!("Define node '{target}' or fix graph retry_target")), + + ..Diagnostic::default() }); } } if let Some(target) = graph.fallback_retry_target() { if !graph.nodes.contains_key(target) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( + message: format!( "Graph has fallback_retry_target '{target}' that does not exist" ), - node_id: None, - edge: None, - fix: Some(format!( + node_id: None, + edge: None, + fix: Some(format!( "Define node '{target}' or fix graph fallback_retry_target" )), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/script_absolute_cd.rs b/lib/crates/fabro-validate/src/rules/script_absolute_cd.rs index 33ccefa50..561f5960d 100644 --- a/lib/crates/fabro-validate/src/rules/script_absolute_cd.rs +++ b/lib/crates/fabro-validate/src/rules/script_absolute_cd.rs @@ -60,7 +60,8 @@ impl LintRule for Rule { "Use a relative path; the engine sets the working directory to the worktree automatically" .to_string(), ), - }); + + ..Diagnostic::default()}); } } diagnostics diff --git a/lib/crates/fabro-validate/src/rules/selection_valid.rs b/lib/crates/fabro-validate/src/rules/selection_valid.rs index 4d4b24ddd..caeed4374 100644 --- a/lib/crates/fabro-validate/src/rules/selection_valid.rs +++ b/lib/crates/fabro-validate/src/rules/selection_valid.rs @@ -21,12 +21,14 @@ impl LintRule for Rule { if let Some(sel) = node.attrs.get("selection").and_then(AttrValue::as_str) { if !VALID_SELECTIONS.contains(&sel) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!("Node '{}' has invalid selection mode '{sel}'", node.id), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(format!("Use one of: {}", VALID_SELECTIONS.join(", "))), + message: format!("Node '{}' has invalid selection mode '{sel}'", node.id), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!("Use one of: {}", VALID_SELECTIONS.join(", "))), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/start_no_incoming.rs b/lib/crates/fabro-validate/src/rules/start_no_incoming.rs index 31032a45b..989ea100a 100644 --- a/lib/crates/fabro-validate/src/rules/start_no_incoming.rs +++ b/lib/crates/fabro-validate/src/rules/start_no_incoming.rs @@ -20,16 +20,18 @@ impl LintRule for Rule { let incoming = graph.incoming_edges(&start.id); if !incoming.is_empty() { return vec![Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!( + message: format!( "Start node '{}' has {} incoming edge(s) but must have none", start.id, incoming.len() ), - node_id: Some(start.id.clone()), - edge: None, - fix: Some("Remove incoming edges to the start node".to_string()), + node_id: Some(start.id.clone()), + edge: None, + fix: Some("Remove incoming edges to the start node".to_string()), + + ..Diagnostic::default() }]; } Vec::new() diff --git a/lib/crates/fabro-validate/src/rules/start_node.rs b/lib/crates/fabro-validate/src/rules/start_node.rs index d18c72325..ba44959b6 100644 --- a/lib/crates/fabro-validate/src/rules/start_node.rs +++ b/lib/crates/fabro-validate/src/rules/start_node.rs @@ -21,26 +21,30 @@ impl LintRule for Rule { .count(); if start_count == 0 { return vec![Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, message: "Pipeline must have exactly one start node (shape=Mdiamond or id start/Start)" .to_string(), - node_id: None, - edge: None, - fix: Some("Add a node with shape=Mdiamond or id 'start'".to_string()), + node_id: None, + edge: None, + fix: Some("Add a node with shape=Mdiamond or id 'start'".to_string()), + + ..Diagnostic::default() }]; } if start_count > 1 { return vec![Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!( + message: format!( "Pipeline has {start_count} start nodes but must have exactly one" ), - node_id: None, - edge: None, - fix: Some("Remove extra start nodes".to_string()), + node_id: None, + edge: None, + fix: Some("Remove extra start nodes".to_string()), + + ..Diagnostic::default() }]; } Vec::new() diff --git a/lib/crates/fabro-validate/src/rules/stylesheet_syntax.rs b/lib/crates/fabro-validate/src/rules/stylesheet_syntax.rs index 40b8d09a5..3964b7619 100644 --- a/lib/crates/fabro-validate/src/rules/stylesheet_syntax.rs +++ b/lib/crates/fabro-validate/src/rules/stylesheet_syntax.rs @@ -22,12 +22,14 @@ impl LintRule for Rule { match parse_stylesheet(stylesheet) { Ok(_) => Vec::new(), Err(e) => vec![Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!("Model stylesheet parse error: {e}"), - node_id: None, - edge: None, - fix: Some("Fix the model_stylesheet syntax".to_string()), + message: format!("Model stylesheet parse error: {e}"), + node_id: None, + edge: None, + fix: Some("Fix the model_stylesheet syntax".to_string()), + + ..Diagnostic::default() }], } } diff --git a/lib/crates/fabro-validate/src/rules/terminal_node.rs b/lib/crates/fabro-validate/src/rules/terminal_node.rs index 8a31d4092..b24d042f5 100644 --- a/lib/crates/fabro-validate/src/rules/terminal_node.rs +++ b/lib/crates/fabro-validate/src/rules/terminal_node.rs @@ -27,26 +27,30 @@ impl LintRule for Rule { .count(); if terminal_count == 0 { return vec![Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, message: "Pipeline must have exactly one terminal node (shape=Msquare or id exit/end)" .to_string(), - node_id: None, - edge: None, - fix: Some("Add a node with shape=Msquare or id 'exit'/'end'".to_string()), + node_id: None, + edge: None, + fix: Some("Add a node with shape=Msquare or id 'exit'/'end'".to_string()), + + ..Diagnostic::default() }]; } if terminal_count > 1 { return vec![Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Error, - message: format!( + message: format!( "Pipeline must have exactly one terminal node, found {terminal_count}" ), - node_id: None, - edge: None, - fix: Some("Remove extra terminal nodes so exactly one remains".to_string()), + node_id: None, + edge: None, + fix: Some("Remove extra terminal nodes so exactly one remains".to_string()), + + ..Diagnostic::default() }]; } Vec::new() diff --git a/lib/crates/fabro-validate/src/rules/thread_id_requires_fidelity_full.rs b/lib/crates/fabro-validate/src/rules/thread_id_requires_fidelity_full.rs index 4eff0788b..457553788 100644 --- a/lib/crates/fabro-validate/src/rules/thread_id_requires_fidelity_full.rs +++ b/lib/crates/fabro-validate/src/rules/thread_id_requires_fidelity_full.rs @@ -25,15 +25,17 @@ impl LintRule for Rule { if node.thread_id().is_some() && node.fidelity() != Some("full") && !graph_default_full { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( + message: format!( "Node '{}' has thread_id but fidelity is not 'full'", node.id ), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(Self::FIX.to_string()), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(Self::FIX.to_string()), + + ..Diagnostic::default() }); } } @@ -45,15 +47,17 @@ impl LintRule for Rule { graph.nodes.get(&edge.to).and_then(|n| n.fidelity()) == Some("full"); if !edge_full && !target_full && !graph_default_full { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!( + message: format!( "Edge {} -> {} has thread_id but fidelity is not 'full'", edge.from, edge.to ), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(Self::FIX.to_string()), + node_id: None, + edge: Some((edge.from.clone(), edge.to.clone())), + fix: Some(Self::FIX.to_string()), + + ..Diagnostic::default() }); } } @@ -61,12 +65,14 @@ impl LintRule for Rule { if graph.default_thread().is_some() && !graph_default_full { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: "Graph has default_thread but default_fidelity is not 'full'".to_string(), - node_id: None, - edge: None, - fix: Some(Self::FIX.to_string()), + message: "Graph has default_thread but default_fidelity is not 'full'".to_string(), + node_id: None, + edge: None, + fix: Some(Self::FIX.to_string()), + + ..Diagnostic::default() }); } diff --git a/lib/crates/fabro-validate/src/rules/type_known.rs b/lib/crates/fabro-validate/src/rules/type_known.rs index 40e2fbd0d..36402e585 100644 --- a/lib/crates/fabro-validate/src/rules/type_known.rs +++ b/lib/crates/fabro-validate/src/rules/type_known.rs @@ -19,12 +19,14 @@ impl LintRule for Rule { if let Some(node_type) = node.node_type() { if !is_known_handler_type(node_type) { diagnostics.push(Diagnostic { - rule: self.name().to_string(), + rule: self.name().to_string(), severity: Severity::Warning, - message: format!("Node '{}' has unrecognized type '{node_type}'", node.id), - node_id: Some(node.id.clone()), - edge: None, - fix: Some(format!("Use one of: {}", KNOWN_HANDLER_TYPES.join(", "))), + message: format!("Node '{}' has unrecognized type '{node_type}'", node.id), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!("Use one of: {}", KNOWN_HANDLER_TYPES.join(", "))), + + ..Diagnostic::default() }); } } diff --git a/lib/crates/fabro-validate/src/rules/unresolved_file_ref.rs b/lib/crates/fabro-validate/src/rules/unresolved_file_ref.rs index 493e29977..c7891fd12 100644 --- a/lib/crates/fabro-validate/src/rules/unresolved_file_ref.rs +++ b/lib/crates/fabro-validate/src/rules/unresolved_file_ref.rs @@ -29,7 +29,8 @@ impl LintRule for Rule { node_id: Some(node.id.clone()), edge: None, fix: Some("Check that the path is relative to the workflow file's directory and the file exists".to_string()), - }); + + ..Diagnostic::default()}); } } } @@ -43,7 +44,8 @@ impl LintRule for Rule { node_id: None, edge: None, fix: Some("Check that the path is relative to the workflow file's directory and the file exists".to_string()), - }); + + ..Diagnostic::default()}); } } diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index d535328b0..716bf6660 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -61,6 +61,7 @@ md5.workspace = true hex.workspace = true sha2 = { workspace = true } mime_guess.workspace = true +miette.workspace = true shlex = "1" git2.workspace = true tokio-util.workspace = true diff --git a/lib/crates/fabro-workflow/src/error.rs b/lib/crates/fabro-workflow/src/error.rs index 5d5b89349..89a2a8952 100644 --- a/lib/crates/fabro-workflow/src/error.rs +++ b/lib/crates/fabro-workflow/src/error.rs @@ -1,5 +1,9 @@ +use std::fmt; +use std::sync::Arc; + use fabro_graphviz::Error as GraphvizError; use fabro_llm::{Error as LlmError, ProviderErrorKind}; +use fabro_template::TemplateError; pub use fabro_types::failure_signature::FailureSignature; pub use fabro_types::outcome::FailureCategory; use fabro_types::{ExecOutputTail, FailureReason, RunFailure}; @@ -99,6 +103,55 @@ const STRUCTURAL_HINTS: &[&str] = &[ "scope violation", ]; +#[derive(Debug, Clone)] +pub struct SharedTemplateError(Arc); + +impl SharedTemplateError { + #[must_use] + pub fn new(error: TemplateError) -> Self { + Self(Arc::new(error)) + } + + #[must_use] + pub fn inner(&self) -> &TemplateError { + &self.0 + } +} + +impl fmt::Display for SharedTemplateError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(&self.0, formatter) + } +} + +impl std::error::Error for SharedTemplateError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.0.source() + } +} + +impl miette::Diagnostic for SharedTemplateError { + fn code<'a>(&'a self) -> Option> { + miette::Diagnostic::code(self.inner()) + } + + fn help<'a>(&'a self) -> Option> { + miette::Diagnostic::help(self.inner()) + } + + fn source_code(&self) -> Option<&dyn miette::SourceCode> { + miette::Diagnostic::source_code(self.inner()) + } + + fn labels(&self) -> Option + '_>> { + miette::Diagnostic::labels(self.inner()) + } + + fn diagnostic_source(&self) -> Option<&dyn miette::Diagnostic> { + miette::Diagnostic::diagnostic_source(self.inner()) + } +} + /// Classify a failure reason string using heuristics. /// /// This is the fallback when structured error information is not available @@ -205,6 +258,13 @@ pub enum Error { #[error("Validation failed")] ValidationFailed { diagnostics: Vec }, + #[error("{message}")] + Template { + message: String, + #[source] + source: SharedTemplateError, + }, + #[error("Engine error: {message}")] Engine { message: String, @@ -262,6 +322,13 @@ impl Error { } } + pub fn template(message: impl Into, source: TemplateError) -> Self { + Self::Template { + message: message.into(), + source: SharedTemplateError::new(source), + } + } + pub fn handler_with_exec_output_tail( message: impl Into, exec_output_tail: Option, @@ -345,6 +412,7 @@ impl Error { Self::Engine { source, .. } | Self::Handler { source, .. } => source .as_ref() .map_or_else(Vec::new, |source| collect_chain(source)), + Self::Template { source, .. } => collect_chain(source), Self::Llm(err) => collect_causes(err), _ => Vec::new(), } @@ -370,6 +438,7 @@ impl Error { Self::Parse(_) | Self::Validation(_) | Self::ValidationFailed { .. } + | Self::Template { .. } | Self::Stylesheet(_) | Self::Checkpoint(_) | Self::Precondition(_) @@ -389,6 +458,7 @@ impl Error { Self::Parse(_) | Self::Validation(_) | Self::ValidationFailed { .. } + | Self::Template { .. } | Self::Stylesheet(_) | Self::Checkpoint(_) | Self::Unsupported(_) => FailureCategory::Deterministic, @@ -448,6 +518,43 @@ impl Error { } } +impl miette::Diagnostic for Error { + fn code<'a>(&'a self) -> Option> { + match self { + Self::Template { source, .. } => miette::Diagnostic::code(source), + _ => None, + } + } + + fn help<'a>(&'a self) -> Option> { + match self { + Self::Template { source, .. } => miette::Diagnostic::help(source), + _ => None, + } + } + + fn source_code(&self) -> Option<&dyn miette::SourceCode> { + match self { + Self::Template { source, .. } => miette::Diagnostic::source_code(source), + _ => None, + } + } + + fn labels(&self) -> Option + '_>> { + match self { + Self::Template { source, .. } => miette::Diagnostic::labels(source), + _ => None, + } + } + + fn diagnostic_source(&self) -> Option<&dyn miette::Diagnostic> { + match self { + Self::Template { source, .. } => Some(source), + _ => None, + } + } +} + #[must_use] pub fn run_failure_from_error(error: &Error, reason: FailureReason) -> RunFailure { RunFailure { @@ -490,7 +597,8 @@ impl From for Error { impl From for Error { fn from(err: fabro_template::TemplateError) -> Self { - Self::Validation(collect_chain(&err).join(": ")) + let rendered = collect_chain(&err).join(": "); + Self::template(format!("template expansion failed: {rendered}"), err) } } @@ -570,17 +678,43 @@ mod tests { fn validation_failed_display() { let err = Error::ValidationFailed { diagnostics: vec![Diagnostic { - rule: "test".to_string(), + rule: "test".to_string(), severity: fabro_validate::Severity::Error, - message: "missing start node".to_string(), - node_id: None, - edge: None, - fix: None, + message: "missing start node".to_string(), + node_id: None, + edge: None, + fix: None, + + ..Diagnostic::default() }], }; assert_eq!(err.to_string(), "Validation failed"); } + #[test] + fn template_error_variant_preserves_source_chain() { + let template_err = fabro_template::render_named( + "workflow.fabro", + "{{ inputs.missing }}", + &fabro_template::TemplateContext::new(), + ) + .unwrap_err(); + + let err = Error::template("template expansion failed", template_err); + let chain = collect_chain(&err); + + assert!( + chain + .iter() + .any(|part| part.contains("template expansion failed")) + ); + assert!( + chain + .iter() + .any(|part| part.contains("undefined template variable")) + ); + } + #[test] fn engine_error_display() { let err = Error::engine("no outgoing edge"); @@ -1807,12 +1941,14 @@ mod tests { Error::Validation("bad".into()), Error::ValidationFailed { diagnostics: vec![Diagnostic { - rule: "test".into(), + rule: "test".into(), severity: fabro_validate::Severity::Error, - message: "bad".into(), - node_id: None, - edge: None, - fix: None, + message: "bad".into(), + node_id: None, + edge: None, + fix: None, + + ..Diagnostic::default() }], }, Error::engine("engine err"), diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index be28511f4..a086389d9 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -28,7 +28,7 @@ use crate::pipeline::{self, Persisted, TransformOptions, Validated}; use crate::records::RunSpec; use crate::run_lookup::default_scratch_base; use crate::run_materialization::materialize_run; -use crate::transforms::Transform; +use crate::transforms::{RenderMode, Transform}; use crate::workflow_bundle::{RunDefinition, WorkflowBundle}; #[derive(Clone, Debug)] @@ -66,6 +66,7 @@ struct PersistCreateOptions { run_id: Option, run_dir: Option, workflow_slug: Option, + source_name: Option, labels: HashMap, source_directory: Option, git: Option, @@ -132,6 +133,10 @@ pub async fn create( }; let raw_source = resolved.raw_source.clone(); + let source_name = resolved + .dot_path + .as_ref() + .map(|path| path.display().to_string()); let persisted = spawn_blocking(move || { create_from_source( &raw_source, @@ -140,6 +145,7 @@ pub async fn create( run_id: Some(run_id), run_dir: Some(persisted_run_dir), workflow_slug: workflow_slug.or(resolved_workflow_slug), + source_name, labels, source_directory, git, @@ -288,11 +294,13 @@ fn create_from_source( ) -> Result { let mut validated = preprocess_and_validate( dot_source, + options.source_name.clone(), current_dir, file_resolver, Vec::new(), Some(&options.settings), goal_override, + RenderMode::Structural, &options.catalog, )?; @@ -308,11 +316,13 @@ fn create_from_source( pub(super) fn preprocess_and_validate( dot_source: &str, + source_name: Option, current_dir: Option, file_resolver: Option>, custom_transforms: Vec>, settings: Option<&WorkflowSettings>, goal_override: Option<&str>, + render_mode: RenderMode, catalog: &Arc, ) -> Result { let inputs = run_inputs(settings); @@ -323,6 +333,8 @@ pub(super) fn preprocess_and_validate( current_dir, file_resolver, inputs, + source_name, + render_mode, custom_transforms, catalog: Arc::clone(catalog), })?; @@ -353,6 +365,7 @@ fn persist_validated( run_id, run_dir, workflow_slug, + source_name: _, labels, source_directory, git, @@ -415,6 +428,7 @@ mod tests { use fabro_types::settings::InterpString; use fabro_types::settings::run::RunMode; use fabro_types::{WorkflowSettings, fixtures}; + use fabro_util::error::collect_chain; use fabro_validate::Severity; use object_store::local::LocalFileSystem; use object_store::memory::InMemory; @@ -527,6 +541,73 @@ mod tests { assert_eq!(diagnostic.severity, Severity::Error); } + #[test] + fn strict_template_error_for_inline_prompt_names_workflow_file_and_node() { + let dot = r#"digraph ValidatePlan { + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + test_inline_prompt [label="moo" prompt="{{ inputs.foo }}"] + start -> test_inline_prompt -> exit + }"#; + + let result = preprocess_and_validate( + dot, + Some("workflow.fabro".to_string()), + Some(PathBuf::from(".")), + None, + Vec::new(), + Some(&WorkflowSettings::default()), + None, + RenderMode::Strict, + &test_catalog(), + ); + let Err(err) = result else { + panic!("expected strict mode to hard-fail on unbound inline prompt"); + }; + + let rendered = collect_chain(&err).join(": "); + assert!(rendered.contains("workflow.fabro"), "{rendered}"); + assert!(rendered.contains("test_inline_prompt"), "{rendered}"); + assert!(rendered.contains("prompt"), "{rendered}"); + assert!(!rendered.contains(""), "{rendered}"); + } + + #[test] + fn imported_prompt_template_error_names_prompt_file_and_node() { + let dir = tempfile::tempdir().unwrap(); + let prompt_path = dir.path().join("test.md"); + std::fs::write(&prompt_path, "{{ inputs.foo }}").unwrap(); + let dot = r#"digraph ValidatePlan { + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + test_imported_prompt [label="moo" prompt="@test.md"] + start -> test_imported_prompt -> exit + }"#; + + let result = preprocess_and_validate( + dot, + Some("workflow.fabro".to_string()), + Some(dir.path().to_path_buf()), + Some(Arc::new(crate::file_resolver::FilesystemFileResolver::new( + None, + ))), + Vec::new(), + Some(&WorkflowSettings::default()), + None, + RenderMode::Strict, + &test_catalog(), + ); + let Err(err) = result else { + panic!("expected strict mode to hard-fail on unbound imported prompt"); + }; + + let rendered = collect_chain(&err).join(": "); + assert!(rendered.contains("test.md"), "{rendered}"); + assert!(rendered.contains("test_imported_prompt"), "{rendered}"); + assert!(rendered.contains("prompt"), "{rendered}"); + assert!(!rendered.contains(""), "{rendered}"); + } + #[test] fn validate_applies_variable_expansion() { let dot = r#"digraph Test { diff --git a/lib/crates/fabro-workflow/src/operations/mod.rs b/lib/crates/fabro-workflow/src/operations/mod.rs index abf9e5bbd..462377b91 100644 --- a/lib/crates/fabro-workflow/src/operations/mod.rs +++ b/lib/crates/fabro-workflow/src/operations/mod.rs @@ -23,3 +23,4 @@ pub use timeline::{ForkTarget, RunTimeline, TimelineEntry, build_timeline, timel pub use validate::{ValidateInput, validate}; pub use crate::pipeline::{DevcontainerSpec, LlmSpec, SandboxEnvSpec}; +pub use crate::transforms::RenderMode; diff --git a/lib/crates/fabro-workflow/src/operations/validate.rs b/lib/crates/fabro-workflow/src/operations/validate.rs index 141f32291..1a4769fc9 100644 --- a/lib/crates/fabro-workflow/src/operations/validate.rs +++ b/lib/crates/fabro-workflow/src/operations/validate.rs @@ -7,6 +7,7 @@ use fabro_types::WorkflowSettings; use super::create::preprocess_and_validate; use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; use crate::error::Error; +use crate::operations::RenderMode; use crate::pipeline::Validated; use crate::transforms::Transform; @@ -32,11 +33,16 @@ pub fn validate(input: ValidateInput) -> Result { preprocess_and_validate( &resolved.raw_source, + resolved + .dot_path + .as_ref() + .map(|path| path.display().to_string()), resolved.current_dir, resolved.file_resolver, input.custom_transforms, Some(&resolved.settings), resolved.goal_override.as_deref(), + RenderMode::Structural, &input.catalog, ) } diff --git a/lib/crates/fabro-workflow/src/pipeline/transform.rs b/lib/crates/fabro-workflow/src/pipeline/transform.rs index 14c0bbffa..5d9945916 100644 --- a/lib/crates/fabro-workflow/src/pipeline/transform.rs +++ b/lib/crates/fabro-workflow/src/pipeline/transform.rs @@ -13,12 +13,25 @@ use crate::transforms::{ /// (e.g. goal override) before validation. pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result { let Parsed { graph, source } = parsed; + let mut diagnostics = Vec::new(); // 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)? + let (graph, transform_diagnostics) = ImportTransform::new( + current_dir.clone(), + Arc::clone(file_resolver), + options.inputs.clone(), + ) + .with_template_options( + options.source_name.clone(), + Some(source.clone()), + options.render_mode, + ) + .apply_with_diagnostics(graph)?; + diagnostics.extend(transform_diagnostics); + graph } else { graph }; @@ -26,13 +39,29 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result, - line: Option, - node_id: Option<&str>, -) -> Diagnostic { - let location = match (node_id, line) { - (Some(id), _) => format!(" in node `{id}`"), - (None, Some(l)) => format!(" at line {l}"), - (None, None) => String::new(), - }; - let (name, message) = match expression { - Some(expr) => ( - expr, - format!("undefined template variable `{expr}`{location}"), - ), - None => ( - "", - format!("undefined template variable{location}"), - ), - }; - Diagnostic { - rule: TEMPLATE_UNDEFINED_VARIABLE_RULE.to_owned(), - severity: Severity::Warning, - message, - node_id: node_id.map(str::to_owned), - edge: None, - fix: Some(format!( - "bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`" - )), - } -} - /// Output of the VALIDATE phase. Always produced (even with errors). /// Caller inspects diagnostics and decides whether to proceed. /// Graph is read-only — use accessors, not direct field access. @@ -365,6 +330,8 @@ pub struct TransformOptions { pub current_dir: Option, pub file_resolver: Option>, pub inputs: HashMap, + pub source_name: Option, + pub render_mode: RenderMode, pub custom_transforms: Vec>, pub catalog: Arc, } diff --git a/lib/crates/fabro-workflow/src/pipeline/validate.rs b/lib/crates/fabro-workflow/src/pipeline/validate.rs index 6a46c584e..166a48078 100644 --- a/lib/crates/fabro-workflow/src/pipeline/validate.rs +++ b/lib/crates/fabro-workflow/src/pipeline/validate.rs @@ -15,11 +15,14 @@ pub fn validate( let Transformed { graph, source, - diagnostics: mut transform_diagnostics, + mut diagnostics, } = transformed; - let lint_diagnostics = fabro_validate::validate_with_catalog(&graph, catalog, extra_rules); - transform_diagnostics.extend(lint_diagnostics); - Validated::new(graph, source, transform_diagnostics) + diagnostics.extend(fabro_validate::validate_with_catalog( + &graph, + catalog, + extra_rules, + )); + Validated::new(graph, source, diagnostics) } #[cfg(test)] @@ -42,6 +45,8 @@ mod tests { current_dir: None, file_resolver: None, inputs: std::collections::HashMap::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, custom_transforms: vec![], catalog: std::sync::Arc::clone(&catalog), }) diff --git a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs index 2911cb216..7fb8a69e7 100644 --- a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs +++ b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs @@ -1,12 +1,18 @@ +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Graph}; +use fabro_template::TemplateContext; +use fabro_validate::Diagnostic; use super::Transform; use crate::error::Error; -use crate::file_resolver::FileResolver; +use crate::file_resolver::{FileResolver, ResolvedFile}; use crate::static_reference::{ReferenceKind, validate_static_reference}; +use crate::transforms::variable_expansion::{ + RenderMode, TemplateRenderTarget, TemplateTransform, render_template_for_target, +}; /// Resolve a potential `@path` file reference. /// @@ -30,8 +36,13 @@ pub fn resolve_file_ref( /// Inlines `@file` references in node prompts and the graph-level goal. pub struct FileInliningTransform { - current_dir: PathBuf, - resolver: Arc, + current_dir: PathBuf, + resolver: Arc, + inputs: HashMap, + source_name: Option, + source_text: Option, + goal_override: Option, + render_mode: RenderMode, } impl FileInliningTransform { @@ -40,39 +51,154 @@ impl FileInliningTransform { Self { current_dir, resolver, + inputs: HashMap::new(), + source_name: None, + source_text: None, + goal_override: None, + render_mode: RenderMode::Strict, } } + + #[must_use] + pub fn with_template_options( + mut self, + inputs: HashMap, + source_name: Option, + source_text: Option, + render_mode: RenderMode, + ) -> Self { + self.inputs = inputs; + self.source_name = source_name; + self.source_text = source_text; + self.render_mode = render_mode; + self + } + + #[must_use] + pub fn with_goal_override(mut self, goal: Option) -> Self { + self.goal_override = goal; + self + } + + pub(crate) fn apply_with_diagnostics( + &self, + graph: Graph, + ) -> Result<(Graph, Vec), Error> { + let mut graph = graph; + let mut diagnostics = Vec::new(); + self.inline_graph_goal(&mut graph, &mut diagnostics)?; + + let resolved_goal = match &self.goal_override { + Some(goal) => goal.clone(), + None => TemplateTransform { + inputs: self.inputs.clone(), + source_name: self.source_name.clone(), + source_text: self.source_text.clone(), + render_mode: self.render_mode, + } + .resolved_goal(&graph, &mut diagnostics)?, + }; + let ctx = TemplateContext::new() + .with_goal(resolved_goal) + .with_inputs(self.inputs.clone()); + + for (node_id, node) in &mut graph.nodes { + let Some(AttrValue::String(prompt)) = node.attrs.get("prompt") else { + continue; + }; + let target = TemplateRenderTarget::node_attr( + self.source_name.clone(), + node_id.clone(), + "prompt", + ) + .with_source_text(self.source_text.as_deref(), prompt); + let rendered = render_template_for_target( + prompt, + &ctx, + self.render_mode, + &target, + &mut diagnostics, + )?; + let value = self + .render_resolved_file_ref(&rendered, &ctx, target, &mut diagnostics)? + .unwrap_or(rendered); + node.attrs + .insert("prompt".to_string(), AttrValue::String(value)); + } + + Ok((graph, diagnostics)) + } + + fn inline_graph_goal( + &self, + graph: &mut Graph, + diagnostics: &mut Vec, + ) -> Result<(), Error> { + let Some(AttrValue::String(goal)) = graph.attrs.get("goal") else { + return Ok(()); + }; + let ctx = TemplateContext::for_input_scan(self.inputs.clone()); + let target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") + .with_source_text(self.source_text.as_deref(), goal); + let rendered = + render_template_for_target(goal, &ctx, self.render_mode, &target, diagnostics)?; + let value = self + .render_resolved_file_ref(&rendered, &ctx, target, diagnostics)? + .unwrap_or(rendered); + graph + .attrs + .insert("goal".to_string(), AttrValue::String(value)); + Ok(()) + } + + fn render_resolved_file_ref( + &self, + value: &str, + ctx: &TemplateContext, + owner_target: TemplateRenderTarget, + diagnostics: &mut Vec, + ) -> Result, Error> { + let Some(path_str) = value.strip_prefix('@') else { + return Ok(None); + }; + validate_static_reference(path_str, ReferenceKind::FileInline) + .map_err(|error| Error::Validation(error.to_string()))?; + let Some(resolved) = self.resolver.resolve(&self.current_dir, path_str) else { + return Ok(None); + }; + let target = owner_target + .with_source_name(resolved.path.display().to_string()) + .with_source_text(Some(&resolved.content), &resolved.content); + Ok(Some(render_file_contents( + &resolved, + ctx, + self.render_mode, + &target, + diagnostics, + )?)) + } } impl Transform for FileInliningTransform { fn apply(&self, graph: Graph) -> Result { - let mut graph = graph; - - // Inline @file refs in node prompts - for node in graph.nodes.values_mut() { - if let Some(AttrValue::String(prompt)) = node.attrs.get("prompt") { - let resolved = resolve_file_ref(prompt, &self.current_dir, self.resolver.as_ref())?; - if resolved != *prompt { - node.attrs - .insert("prompt".to_string(), AttrValue::String(resolved)); - } - } + let (graph, diagnostics) = self.apply_with_diagnostics(graph)?; + if !diagnostics.is_empty() { + return Err(Error::ValidationFailed { diagnostics }); } - - // Inline @file refs in graph-level goal - if let Some(AttrValue::String(goal)) = graph.attrs.get("goal") { - let resolved = resolve_file_ref(goal, &self.current_dir, self.resolver.as_ref())?; - if resolved != *goal { - graph - .attrs - .insert("goal".to_string(), AttrValue::String(resolved)); - } - } - Ok(graph) } } +pub(crate) fn render_file_contents( + resolved: &ResolvedFile, + ctx: &TemplateContext, + render_mode: RenderMode, + target: &TemplateRenderTarget, + diagnostics: &mut Vec, +) -> Result { + render_template_for_target(&resolved.content, ctx, render_mode, target, diagnostics) +} + #[cfg(test)] mod tests { #![expect( diff --git a/lib/crates/fabro-workflow/src/transforms/import.rs b/lib/crates/fabro-workflow/src/transforms/import.rs index 3f424f469..d60e5423b 100644 --- a/lib/crates/fabro-workflow/src/transforms/import.rs +++ b/lib/crates/fabro-workflow/src/transforms/import.rs @@ -4,15 +4,24 @@ use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_graphviz::parser; +use fabro_template::TemplateContext; +use fabro_validate::Diagnostic; use super::{FileInliningTransform, Transform}; use crate::error::Error; use crate::file_resolver::{FileResolver, ResolvedFile}; use crate::static_reference::{ReferenceKind, validate_static_reference}; +use crate::transforms::variable_expansion::{ + RenderMode, TemplateRenderTarget, TemplateTransform, render_template_for_target, +}; pub struct ImportTransform { current_dir: PathBuf, resolver: Arc, + inputs: HashMap, + source_name: Option, + source_text: Option, + render_mode: RenderMode, } struct PlaceholderOptions { @@ -27,6 +36,7 @@ struct PreparedImport { exit_id: String, entry_id: String, exit_predecessor_id: String, + diagnostics: Vec, } enum ImportPrepareError { @@ -42,13 +52,34 @@ impl From for ImportPrepareError { 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, + source_name: None, + source_text: None, + render_mode: RenderMode::Structural, } } + #[must_use] + pub fn with_template_options( + mut self, + source_name: Option, + source_text: Option, + render_mode: RenderMode, + ) -> Self { + self.source_name = source_name; + self.source_text = source_text; + self.render_mode = render_mode; + self + } + fn collect_import_nodes(graph: &Graph) -> Vec<(String, String)> { graph .nodes @@ -67,11 +98,12 @@ impl ImportTransform { graph: &mut Graph, placeholder_id: &str, import_path: &str, + parent_goal: &str, current_base_dir: &Path, import_stack: &mut Vec, - ) -> Result<(), Error> { + ) -> Result, Error> { if !graph.nodes.contains_key(placeholder_id) { - return Ok(()); + return Ok(Vec::new()); } if graph @@ -84,20 +116,20 @@ impl ImportTransform { placeholder_id, &format!("import placeholder '{placeholder_id}' cannot have a self-loop"), ); - return Ok(()); + return Ok(Vec::new()); } let placeholder = match Self::placeholder_config(graph, placeholder_id) { Ok(placeholder) => placeholder, Err(message) => { Self::poison_placeholder(graph, placeholder_id, &message); - return Ok(()); + return Ok(Vec::new()); } }; if let Err(error) = validate_static_reference(import_path, ReferenceKind::Import) { Self::poison_placeholder(graph, placeholder_id, &error.to_string()); - return Ok(()); + return Ok(Vec::new()); } let Some(resolved_file) = self.resolver.resolve(current_base_dir, import_path) else { @@ -106,7 +138,7 @@ impl ImportTransform { placeholder_id, &format!("file not found: {import_path}"), ); - return Ok(()); + return Ok(Vec::new()); }; if import_stack.contains(&resolved_file.path) { @@ -121,17 +153,18 @@ impl ImportTransform { placeholder_id, &format!("circular import detected: {cycle}"), ); - return Ok(()); + return Ok(Vec::new()); } - let prepared = match self.prepare_import(&resolved_file, import_stack) { + let prepared = match self.prepare_import(&resolved_file, parent_goal, import_stack) { Ok(prepared) => prepared, Err(ImportPrepareError::Hard(error)) => return Err(error), Err(ImportPrepareError::Soft(message)) => { Self::poison_placeholder(graph, placeholder_id, &message); - return Ok(()); + return Ok(Vec::new()); } }; + let diagnostics = prepared.diagnostics.clone(); if let Err(message) = Self::splice_import( graph, @@ -143,15 +176,20 @@ impl ImportTransform { Self::poison_placeholder(graph, placeholder_id, &message); } - Ok(()) + Ok(diagnostics) } fn prepare_import( &self, resolved_file: &ResolvedFile, + parent_goal: &str, import_stack: &mut Vec, ) -> Result { Self::with_import_stack(import_stack, resolved_file.path.clone(), |import_stack| { + let mut diagnostics = Vec::new(); + let source_name = resolved_file.path.display().to_string(); + let source_text = resolved_file.content.clone(); + let mut graph = parser::parse(&resolved_file.content).map_err(|error| { ImportPrepareError::Soft(format!( "failed to parse {}: {error}", @@ -163,9 +201,34 @@ impl ImportTransform { .path .parent() .map_or_else(|| PathBuf::from("."), Path::to_path_buf); - graph = FileInliningTransform::new(import_base_dir.clone(), Arc::clone(&self.resolver)) - .apply(graph) - .map_err(ImportPrepareError::Hard)?; + let (inlined_graph, file_diagnostics) = + FileInliningTransform::new(import_base_dir.clone(), Arc::clone(&self.resolver)) + .with_template_options( + self.inputs.clone(), + Some(source_name.clone()), + Some(source_text.clone()), + self.render_mode, + ) + .with_goal_override(Some(parent_goal.to_string())) + .apply_with_diagnostics(graph) + .map_err(ImportPrepareError::Hard)?; + graph = inlined_graph; + diagnostics.extend(file_diagnostics); + + graph.attrs.insert( + "goal".to_string(), + AttrValue::String(parent_goal.to_string()), + ); + let (templated_graph, template_diagnostics) = TemplateTransform { + inputs: self.inputs.clone(), + source_name: Some(source_name), + source_text: Some(source_text), + render_mode: self.render_mode, + } + .apply_with_diagnostics(graph) + .map_err(ImportPrepareError::Hard)?; + graph = templated_graph; + diagnostics.extend(template_diagnostics); if let Some(message) = Self::unresolved_imported_prompt_error(&graph) { return Err(ImportPrepareError::Soft(message)); @@ -173,16 +236,21 @@ impl ImportTransform { let nested_imports = Self::collect_import_nodes(&graph); for (placeholder_id, import_path) in nested_imports { - self.expand_import( + let nested_diagnostics = self.expand_import( &mut graph, &placeholder_id, &import_path, + parent_goal, &import_base_dir, import_stack, )?; + diagnostics.extend(nested_diagnostics); } - Self::validate_imported_graph(graph).map_err(ImportPrepareError::Soft) + let mut prepared = + Self::validate_imported_graph(graph).map_err(ImportPrepareError::Soft)?; + prepared.diagnostics = diagnostics; + Ok(prepared) }) } @@ -221,6 +289,7 @@ impl ImportTransform { exit_id, entry_id, exit_predecessor_id, + diagnostics: _, } = prepared; for node_id in imported_graph.nodes.keys() { @@ -469,6 +538,7 @@ impl ImportTransform { exit_id, entry_id, exit_predecessor_id, + diagnostics: Vec::new(), }) } @@ -583,21 +653,65 @@ impl PreparedImport { impl Transform for ImportTransform { fn apply(&self, graph: Graph) -> Result { + let (graph, diagnostics) = self.apply_with_diagnostics(graph)?; + if !diagnostics.is_empty() { + return Err(Error::ValidationFailed { diagnostics }); + } + Ok(graph) + } +} + +impl ImportTransform { + pub(crate) fn apply_with_diagnostics( + &self, + graph: Graph, + ) -> Result<(Graph, Vec), Error> { let mut graph = graph; let imports = Self::collect_import_nodes(&graph); let mut import_stack = Vec::new(); + let mut diagnostics = Vec::new(); + let path_ctx = TemplateContext::for_input_scan(self.inputs.clone()); + let mut ignored_goal_diagnostics = Vec::new(); + let goal_target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") + .with_source_text(self.source_text.as_deref(), graph.goal()); + let parent_goal = render_template_for_target( + graph.goal(), + &path_ctx, + self.render_mode, + &goal_target, + &mut ignored_goal_diagnostics, + )?; for (placeholder_id, import_path) in imports { - self.expand_import( + if let Err(error) = validate_static_reference(&import_path, ReferenceKind::Import) { + Self::poison_placeholder(&mut graph, &placeholder_id, &error.to_string()); + continue; + } + let target = TemplateRenderTarget::node_attr( + self.source_name.clone(), + placeholder_id.clone(), + "import", + ) + .with_source_text(self.source_text.as_deref(), &import_path); + let rendered_import_path = render_template_for_target( + &import_path, + &path_ctx, + self.render_mode, + &target, + &mut diagnostics, + )?; + let import_diagnostics = self.expand_import( &mut graph, &placeholder_id, - &import_path, + &rendered_import_path, + &parent_goal, &self.current_dir, &mut import_stack, )?; + diagnostics.extend(import_diagnostics); } - Ok(graph) + Ok((graph, diagnostics)) } } @@ -609,6 +723,7 @@ mod tests { use fabro_graphviz::graph::AttrValue; use fabro_graphviz::parser; + use fabro_util::error::collect_chain; use super::*; use crate::file_resolver::FilesystemFileResolver; @@ -631,11 +746,89 @@ mod tests { Arc::new(FilesystemFileResolver::new( fallback_dir.map(Path::to_path_buf), )), + HashMap::new(), ) .apply(graph) .unwrap() } + #[test] + fn templated_import_path_poison_placeholder_before_rendering() { + let dir = tempfile::tempdir().unwrap(); + let graph = parse_graph( + r#"digraph Test { + start [shape=Mdiamond] + validate [import="{{ inputs.path }}"] + exit [shape=Msquare] + start -> validate -> exit + }"#, + ); + + let graph = ImportTransform::new( + dir.path().to_path_buf(), + Arc::new(FilesystemFileResolver::new(None)), + HashMap::new(), + ) + .with_template_options( + Some("workflow.fabro".to_string()), + Some("workflow source {{ inputs.path }}".to_string()), + RenderMode::Strict, + ) + .apply(graph) + .unwrap(); + + let error = graph.nodes["validate"] + .attrs + .get("import_error") + .and_then(AttrValue::as_str) + .expect("templated import path should poison placeholder"); + assert!( + error.contains("templates are not supported in import references"), + "unexpected error: {error}" + ); + } + + #[test] + fn imported_workflow_template_error_names_imported_file() { + let dir = tempfile::tempdir().unwrap(); + write_file( + &dir.path().join("child.fabro"), + r#"digraph Child { + graph [goal="{{ inputs.foo }}"] + start [shape=Mdiamond] + work [prompt="Do it"] + exit [shape=Msquare] + start -> work -> exit + }"#, + ); + let graph = parse_graph( + r#"digraph Test { + start [shape=Mdiamond] + validate [import="./child.fabro"] + exit [shape=Msquare] + start -> validate -> exit + }"#, + ); + + let err = ImportTransform::new( + dir.path().to_path_buf(), + Arc::new(FilesystemFileResolver::new(None)), + HashMap::new(), + ) + .with_template_options( + Some("workflow.fabro".to_string()), + Some("workflow source".to_string()), + RenderMode::Strict, + ) + .apply(graph) + .unwrap_err(); + let rendered = collect_chain(&err).join(": "); + + assert!(rendered.contains("child.fabro"), "{rendered}"); + assert!(rendered.contains("inputs.foo"), "{rendered}"); + assert!(!rendered.contains(""), "{rendered}"); + } + fn basic_import_source() -> &'static str { r#"digraph validate { start [shape=Mdiamond] @@ -709,7 +902,7 @@ mod tests { } #[test] - fn import_preserves_prompt_templates_for_template_transform() { + fn import_reports_structural_diagnostic_for_imported_prompt_templates() { let dir = tempfile::tempdir().unwrap(); write_file( &dir.path().join("validate.fabro"), @@ -721,23 +914,47 @@ mod tests { }"#, ); - let graph = apply_import( + let graph = parse_graph( r#"digraph Deploy { start [shape=Mdiamond] validate [import="./validate.fabro"] exit [shape=Msquare] start -> validate -> exit }"#, - dir.path(), - None, ); + let (graph, diagnostics) = ImportTransform::new( + dir.path().to_path_buf(), + Arc::new(FilesystemFileResolver::new(None)), + HashMap::new(), + ) + .apply_with_diagnostics(graph) + .unwrap(); + assert_eq!( graph.nodes["validate.lint"] .attrs .get("prompt") .and_then(AttrValue::as_str), - Some("Run {{ inputs.task }}") + Some("Run ") + ); + let diagnostic = diagnostics + .iter() + .find(|diagnostic| diagnostic.rule == "template_undefined_variable") + .expect("expected imported prompt template diagnostic"); + assert_eq!(diagnostic.node_id.as_deref(), Some("lint")); + assert!( + diagnostic + .source_path + .as_deref() + .is_some_and(|path| path.ends_with("validate.fabro")), + "{diagnostic:?}" + ); + assert!( + diagnostic + .message + .contains("node `lint` attribute `prompt`"), + "{diagnostic:?}" ); } @@ -1410,6 +1627,7 @@ mod tests { let graph = ImportTransform::new( dir.path().to_path_buf(), Arc::new(FilesystemFileResolver::new(None)), + HashMap::new(), ) .apply(graph) .unwrap(); diff --git a/lib/crates/fabro-workflow/src/transforms/mod.rs b/lib/crates/fabro-workflow/src/transforms/mod.rs index 466d7b342..f812853d3 100644 --- a/lib/crates/fabro-workflow/src/transforms/mod.rs +++ b/lib/crates/fabro-workflow/src/transforms/mod.rs @@ -21,4 +21,4 @@ pub use import::ImportTransform; pub use model_resolution::ModelResolutionTransform; pub use preamble::PreambleTransform; pub use stylesheet_application::StylesheetApplicationTransform; -pub use variable_expansion::TemplateTransform; +pub use variable_expansion::{RenderMode, TemplateTransform}; diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index f755723c9..a8232cca7 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -1,73 +1,233 @@ use std::collections::HashMap; +use std::fmt::Write as _; use fabro_graphviz::graph::{AttrValue, Graph}; -use fabro_template::{TemplateContext, TemplateError, render as render_template, render_lenient}; -use fabro_validate::Diagnostic; +use fabro_template::{TemplateContext, TemplateError, render_lenient_named, render_named}; +use fabro_util::error::collect_chain; +use fabro_validate::{Diagnostic, Severity}; use super::Transform; use crate::error::Error; -use crate::pipeline::types::template_undefined_variable_diagnostic; +use crate::pipeline::types::TEMPLATE_UNDEFINED_VARIABLE_RULE; use crate::static_reference::{ AttributeScope, ReferenceKind, reference_kind_for_attribute, validate_static_reference, }; +/// How the template-expansion pass should treat undefined input variables. +/// +/// Validate is structural — it should not fail just because the user has not +/// bound `{{ inputs.* }}` yet. Run-start is strict — missing inputs are real +/// errors. Splitting the two lets validate work on a bare `.fabro` while +/// run-start preserves its current hard-fail behavior. +#[derive(Clone, Copy, Debug)] +pub enum RenderMode { + /// Undefined inputs are hard errors. Used by run-create. + Strict, + /// Undefined inputs render as empty and become warning diagnostics on the + /// returned `Validated`, so structural lints still run. Used by + /// `fabro validate`. + Structural, +} + +#[derive(Clone, Debug)] +pub(crate) struct TemplateRenderTarget { + pub source_name: Option, + pub source_text: Option, + pub source_offset: Option, + pub node_id: Option, + pub edge: Option<(String, String)>, + pub owner: String, +} + +impl TemplateRenderTarget { + #[must_use] + pub(crate) fn graph_attr(source_name: Option, attr_name: impl Into) -> Self { + let attr_name = attr_name.into(); + Self { + source_name, + source_text: None, + source_offset: None, + node_id: None, + edge: None, + owner: format!("graph attribute `{attr_name}`"), + } + } + + #[must_use] + pub(crate) fn node_attr( + source_name: Option, + node_id: impl Into, + attr_name: impl Into, + ) -> Self { + let node_id = node_id.into(); + let attr_name = attr_name.into(); + Self { + source_name, + source_text: None, + source_offset: None, + node_id: Some(node_id.clone()), + edge: None, + owner: format!("node `{node_id}` attribute `{attr_name}`"), + } + } + + #[must_use] + pub(crate) fn edge_attr( + source_name: Option, + from: impl Into, + to: impl Into, + attr_name: impl Into, + ) -> Self { + let from = from.into(); + let to = to.into(); + let attr_name = attr_name.into(); + Self { + source_name, + source_text: None, + source_offset: None, + node_id: None, + edge: Some((from.clone(), to.clone())), + owner: format!("edge `{from} -> {to}` attribute `{attr_name}`"), + } + } + + #[must_use] + pub(crate) fn with_source_name(mut self, source_name: impl Into) -> Self { + self.source_name = Some(source_name.into()); + self + } + + #[must_use] + pub(crate) fn with_source_text(mut self, source_text: Option<&str>, value: &str) -> Self { + self.source_text = source_text.map(ToOwned::to_owned); + self.source_offset = source_text.and_then(|source_text| source_text.find(value)); + self + } + + #[must_use] + fn template_source_name(&self) -> String { + self.source_name + .clone() + .unwrap_or_else(|| "workflow".to_string()) + } +} + +pub(crate) fn render_template_for_target( + text: &str, + ctx: &TemplateContext, + render_mode: RenderMode, + target: &TemplateRenderTarget, + diagnostics: &mut Vec, +) -> Result { + let source_name = target.template_source_name(); + match render_mode { + RenderMode::Strict => render_named(source_name, text, ctx) + .map_err(|err| template_error_for_target(target, err)), + RenderMode::Structural => match render_named(source_name.clone(), text, ctx) { + Ok(rendered) => Ok(rendered), + Err(err @ TemplateError::UndefinedVariable { .. }) => { + diagnostics.push(template_diagnostic(&err, target)); + render_lenient_named(source_name, text, ctx) + .map_err(|err| template_error_for_target(target, err)) + } + Err(err) => Err(template_error_for_target(target, err)), + }, + } +} + +fn template_error_for_target(target: &TemplateRenderTarget, err: TemplateError) -> Error { + let rendered = collect_chain(&err).join(": "); + Error::template( + format!("template expansion failed in {}: {rendered}", target.owner), + err, + ) +} + +fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> Diagnostic { + let expression = error.expression(); + let name = expression.unwrap_or(""); + let mut message = match expression { + Some(expr) => format!("undefined template variable `{expr}`"), + None => "undefined template variable".to_string(), + }; + let _ = write!(message, " in {}", target.owner); + + let source_location = target + .source_text + .as_deref() + .zip(target.source_offset) + .zip(error.span()) + .and_then(|((source_text, source_offset), span)| { + let absolute_offset = source_offset.checked_add(span.offset())?; + let (line, column) = source_position(source_text, absolute_offset)?; + Some((line, column, absolute_offset, span.len())) + }); + + Diagnostic { + rule: TEMPLATE_UNDEFINED_VARIABLE_RULE.to_owned(), + severity: Severity::Warning, + message, + node_id: target.node_id.clone(), + edge: target.edge.clone(), + fix: Some(format!( + "bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`" + )), + source_path: error + .source_name() + .map(ToOwned::to_owned) + .or_else(|| target.source_name.clone()), + line: source_location + .map(|(line, _, _, _)| line) + .or_else(|| error.line()), + column: source_location + .map(|(_, column, _, _)| column) + .or_else(|| error.column()), + span_start: source_location + .map(|(_, _, span_start, _)| span_start) + .or_else(|| error.span().map(|span| span.offset())), + span_len: source_location + .map(|(_, _, _, span_len)| span_len) + .or_else(|| error.span().map(|span| span.len())), + related: Vec::new(), + } +} + +fn source_position(source_text: &str, offset: usize) -> Option<(u32, u32)> { + if offset > source_text.len() || !source_text.is_char_boundary(offset) { + return None; + } + let line = source_text[..offset] + .bytes() + .filter(|byte| *byte == b'\n') + .count() + + 1; + let line_start = source_text[..offset] + .rfind('\n') + .map_or(0, |index| index + 1); + let column = source_text[line_start..offset].chars().count() + 1; + Some((u32::try_from(line).ok()?, u32::try_from(column).ok()?)) +} + /// Expands `{{ goal }}` / `{{ inputs.* }}` across all string attributes. pub struct TemplateTransform { - pub inputs: HashMap, + pub inputs: HashMap, + pub source_name: Option, + pub source_text: Option, + pub render_mode: RenderMode, } impl TemplateTransform { #[must_use] pub fn new(inputs: HashMap) -> Self { - Self { inputs } + Self { + inputs, + source_name: None, + source_text: None, + render_mode: RenderMode::Structural, + } } - /// Run the transform, returning the rendered graph together with any - /// diagnostics collected during lenient undefined-variable rendering. - pub fn apply_with_diagnostics( - &self, - mut graph: Graph, - ) -> Result<(Graph, Vec), Error> { - let mut diagnostics = Vec::new(); - - let resolved_goal = self.resolve_goal(&graph, &mut diagnostics)?; - graph - .attrs - .insert("goal".to_string(), AttrValue::String(resolved_goal.clone())); - let ctx = TemplateContext::new() - .with_goal(resolved_goal) - .with_inputs(self.inputs.clone()); - - Self::render_attrs( - &mut graph.attrs, - &ctx, - AttributeScope::Graph, - None, - &mut diagnostics, - )?; - for (node_id, node) in &mut graph.nodes { - Self::render_attrs( - &mut node.attrs, - &ctx, - AttributeScope::Node, - Some(node_id), - &mut diagnostics, - )?; - } - for edge in &mut graph.edges { - Self::render_attrs( - &mut edge.attrs, - &ctx, - AttributeScope::Edge, - None, - &mut diagnostics, - )?; - } - - Ok((graph, diagnostics)) - } - - fn resolve_goal( + pub(crate) fn resolved_goal( &self, graph: &Graph, diagnostics: &mut Vec, @@ -79,55 +239,108 @@ impl TemplateTransform { return Ok(goal.to_string()); } let ctx = TemplateContext::for_input_scan(self.inputs.clone()); - Self::render_text(goal, &ctx, None, diagnostics) + let target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") + .with_source_text(self.source_text.as_deref(), goal); + render_template_for_target(goal, &ctx, self.render_mode, &target, diagnostics) } fn render_attrs( attrs: &mut HashMap, ctx: &TemplateContext, + source_name: Option<&String>, + source_text: Option<&str>, + render_mode: RenderMode, scope: AttributeScope, - node_id: Option<&str>, + owner_for_attr: impl Fn(&str) -> TemplateRenderTarget, diagnostics: &mut Vec, ) -> Result<(), Error> { - for (key, value) in attrs { + for (attr_name, value) in attrs { if let AttrValue::String(text) = value { - if matches!(scope, AttributeScope::Graph) && key == "goal" { + if matches!(scope, AttributeScope::Graph) && attr_name == "goal" { continue; } - if key == "stack.child_dot_source" { + if attr_name == "stack.child_dot_source" { continue; } - if let Some(kind) = reference_kind_for_attribute(scope, key, text) { + if let Some(kind) = reference_kind_for_attribute(scope, attr_name, text) { validate_static_reference(text, kind) .map_err(|error| Error::Validation(error.to_string()))?; continue; } - *text = Self::render_text(text, ctx, node_id, diagnostics)?; + let target = owner_for_attr(attr_name) + .with_source_name(source_name.cloned().unwrap_or_else(|| "workflow".into())) + .with_source_text(source_text, text); + *text = render_template_for_target(text, ctx, render_mode, &target, diagnostics)?; } } Ok(()) } - fn render_text( - text: &str, - ctx: &TemplateContext, - node_id: Option<&str>, - diagnostics: &mut Vec, - ) -> Result { - match render_template(text, ctx) { - Ok(rendered) => Ok(rendered), - Err(TemplateError::UndefinedVariable { - expression, line, .. - }) => { - diagnostics.push(template_undefined_variable_diagnostic( - expression.as_deref(), - line, - node_id, - )); - Ok(render_lenient(text, ctx)?) - } - Err(error) => Err(error.into()), + pub(crate) fn apply_with_diagnostics( + &self, + graph: Graph, + ) -> Result<(Graph, Vec), Error> { + let mut diagnostics = Vec::new(); + let mut graph = graph; + let resolved_goal = self.resolved_goal(&graph, &mut diagnostics)?; + graph + .attrs + .insert("goal".to_string(), AttrValue::String(resolved_goal.clone())); + let ctx = TemplateContext::new() + .with_goal(resolved_goal) + .with_inputs(self.inputs.clone()); + + Self::render_attrs( + &mut graph.attrs, + &ctx, + self.source_name.as_ref(), + self.source_text.as_deref(), + self.render_mode, + AttributeScope::Graph, + |attr_name| TemplateRenderTarget::graph_attr(self.source_name.clone(), attr_name), + &mut diagnostics, + )?; + for (node_id, node) in &mut graph.nodes { + Self::render_attrs( + &mut node.attrs, + &ctx, + self.source_name.as_ref(), + self.source_text.as_deref(), + self.render_mode, + AttributeScope::Node, + |attr_name| { + TemplateRenderTarget::node_attr( + self.source_name.clone(), + node_id.clone(), + attr_name, + ) + }, + &mut diagnostics, + )?; } + for edge in &mut graph.edges { + let from = edge.from.clone(); + let to = edge.to.clone(); + Self::render_attrs( + &mut edge.attrs, + &ctx, + self.source_name.as_ref(), + self.source_text.as_deref(), + self.render_mode, + AttributeScope::Edge, + |attr_name| { + TemplateRenderTarget::edge_attr( + self.source_name.clone(), + from.clone(), + to.clone(), + attr_name, + ) + }, + &mut diagnostics, + )?; + } + + Ok((graph, diagnostics)) } } @@ -391,4 +604,35 @@ mod tests { "unexpected error: {err}" ); } + + #[test] + fn template_transform_reports_structural_diagnostics_with_owner_context() { + let mut graph = Graph::new("test"); + let mut node = Node::new("plan"); + node.attrs.insert( + "prompt".to_string(), + AttrValue::String("{{ inputs.missing }}".to_string()), + ); + graph.nodes.insert("plan".to_string(), node); + + let transform = TemplateTransform { + inputs: HashMap::new(), + source_name: Some("workflow.fabro".to_string()), + source_text: None, + render_mode: RenderMode::Structural, + }; + let (_, diagnostics) = transform.apply_with_diagnostics(graph).unwrap(); + + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].node_id.as_deref(), Some("plan")); + assert_eq!( + diagnostics[0].source_path.as_deref(), + Some("workflow.fabro") + ); + assert!( + diagnostics[0] + .message + .contains("node `plan` attribute `prompt`") + ); + } } diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 3a38aaa55..140ba2ba1 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -4307,6 +4307,8 @@ async fn import_e2e_through_engine() { fabro_workflow::file_resolver::FilesystemFileResolver::new(None), )), inputs: std::collections::HashMap::new(), + source_name: None, + render_mode: fabro_workflow::operations::RenderMode::Strict, custom_transforms: vec![], catalog: std::sync::Arc::clone(&catalog), })