diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index 80152f2d9..e8b2307fc 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -12302,11 +12302,25 @@ impl Agent { workflow_builder = workflow_builder.parameters(parameters); } - let workflow = workflow_builder.build().map_err(|e| { + let mut workflow = workflow_builder.build().map_err(|e| { tracing::error!("Failed to build workflow: {}", e); anyhow!("Workflow build failed: {}", e) })?; + // A parameter the document refers to nowhere makes the whole workflow + // unsavable: `workflow::service::validate` refuses it either way round + // — with no default as "Optional parameters missing default values", with + // one as "Unnecessary parameter definitions" — so a capture that emitted + // one could not be saved from any surface. Models produce them routinely, + // because `workflow.md` asks for parameters and for their `{{ key }}` + // references as two separate instructions. + // + // Pruned HERE rather than in each of the three capture surfaces: the + // route, the CLI's `/workflow` and the model's own `generate` all come + // through this function, which is the whole reason + // `tests/workflow_capture_parity.rs` exists. + crate::workflow::service::drop_unreferenced_parameters(&mut workflow); + tracing::info!("Workflow creation completed successfully"); Ok(workflow) } diff --git a/crates/biorouter/src/workflow/service.rs b/crates/biorouter/src/workflow/service.rs index 46a84cd23..d52b099ad 100644 --- a/crates/biorouter/src/workflow/service.rs +++ b/crates/biorouter/src/workflow/service.rs @@ -333,6 +333,86 @@ pub fn validate(workflow: &Workflow) -> Result<()> { .map_err(|err| anyhow::anyhow!("{err}")) } +/// Drop the parameters no part of the document refers to, and return their keys. +/// +/// [`validate`] refuses a parameter that appears in no `{{ key }}` anywhere in +/// the document ("Unnecessary parameter definitions"), and separately refuses an +/// `optional` parameter with no `default` ("Optional parameters missing default +/// values"). Both rules are right on their own, and together they leave an +/// unreferenced `optional` parameter with no accepted form at all: removing its +/// default trades one refusal for the other. A generator that emits one +/// therefore produces a workflow the user cannot save by any route — which is +/// what "Create workflow from this chat" did, because `workflow.md` asks for +/// parameters and for `{{ key }}` references as two separate instructions and a +/// model routinely obeys only the first. +/// +/// So the generated document is normalised instead of the rules being relaxed. A +/// parameter nothing refers to is a value the run would collect and discard, and +/// dropping it is the same stance `Agent::create_workflow` already takes towards +/// a parameter list that does not parse: keep the expensive part, lose the part +/// that cannot work, say so in the log. +/// +/// ⚠ **The reference set is read the way [`validate`] reads it** — every +/// `{{ … }}` in the serialized document, not just the ones in `prompt` and +/// `instructions` — so the two can never disagree about what "referenced" +/// means. A serialization failure drops nothing: an unpruned document still has +/// a chance of being valid, and a silently emptied `parameters` list does not. +pub fn drop_unreferenced_parameters(workflow: &mut Workflow) -> Vec { + let Some(parameters) = workflow.parameters.as_ref() else { + return Vec::new(); + }; + if parameters.is_empty() { + return Vec::new(); + } + + let yaml = match workflow.to_yaml() { + Ok(yaml) => yaml, + Err(err) => { + tracing::warn!( + "Keeping the generated parameters: the workflow would not serialize \ + for a reference check: {err}" + ); + return Vec::new(); + } + }; + let referenced = match crate::workflow::template_workflow::parse_workflow_content(&yaml, None) { + Ok((_, variables)) => variables, + Err(err) => { + tracing::warn!( + "Keeping the generated parameters: the workflow would not parse \ + for a reference check: {err}" + ); + return Vec::new(); + } + }; + + let mut dropped = Vec::new(); + let kept: Vec<_> = parameters + .iter() + .filter(|parameter| { + if referenced.contains(¶meter.key) { + return true; + } + dropped.push(parameter.key.clone()); + false + }) + .cloned() + .collect(); + + if dropped.is_empty() { + return dropped; + } + tracing::warn!( + "Dropping generated workflow parameters the document never refers to: {}", + dropped.join(", ") + ); + // `None`, not an empty list: `skip_serializing_if` keeps an empty `Vec` out + // of the YAML anyway, and `None` is what every other "there is nothing to + // say" path in this module means by it. + workflow.parameters = if kept.is_empty() { None } else { Some(kept) }; + dropped +} + /// Substitute parameter values into a workflow template. /// /// `Ok(None)` means required parameters are still missing — the caller is diff --git a/crates/biorouter/src/workflow/validate_workflow.rs b/crates/biorouter/src/workflow/validate_workflow.rs index 1dff4308a..191422c88 100644 --- a/crates/biorouter/src/workflow/validate_workflow.rs +++ b/crates/biorouter/src/workflow/validate_workflow.rs @@ -103,24 +103,34 @@ fn validate_parameters_in_template( let mut message = String::new(); if !missing_keys.is_empty() { + // Sorted, like the arm below: both sides come out of a `HashSet` + // difference, so an unsorted join gives the same fault a different + // message on each run. + let mut names: Vec = missing_keys.iter().map(|s| s.to_string()).collect(); + names.sort(); message.push_str(&format!( "Missing definitions for parameters in the workflow file: {}.", - missing_keys - .iter() - .map(|s| s.to_string()) - .collect::>() - .join(", ") + names.join(", ") )); } if !extra_keys.is_empty() { + // The prefix is load-bearing — it is what the messages users and tests + // have already matched on say. What follows it is new: the old message + // named the parameter and stopped, leaving "unnecessary" to be guessed + // at, and the obvious guess (remove its `default`) trades this refusal + // for `validate_optional_parameters`' one. Naming the fix rather than + // only the fault is the difference between a rule and a dead end. + let mut names: Vec = extra_keys.iter().map(|s| s.to_string()).collect(); + names.sort(); message.push_str(&format!( - "\nUnnecessary parameter definitions: {}.", - extra_keys - .iter() - .map(|s| s.to_string()) - .collect::>() - .join(", ") + "\nUnnecessary parameter definitions: {}. Nothing in the workflow \ + refers to {}, so a value for it would be collected and then \ + discarded — write {{{{ {} }}}} into `prompt` or `instructions`, or \ + remove the definition. Giving it a `default` does not help.", + names.join(", "), + if names.len() == 1 { "it" } else { "them" }, + names[0] )); } Err(anyhow::anyhow!("{}", message.trim_end())) diff --git a/crates/biorouter/tests/workflow_capture_parity.rs b/crates/biorouter/tests/workflow_capture_parity.rs index 9f3175ad0..15f6a6e2d 100644 --- a/crates/biorouter/tests/workflow_capture_parity.rs +++ b/crates/biorouter/tests/workflow_capture_parity.rs @@ -61,7 +61,9 @@ fn sandbox_config_root_for_this_test_binary() { /// the two documents below therefore came from the capture path, which is /// exactly the fault being tested for. #[derive(Clone)] -struct FixedWorkflowProvider; +struct FixedWorkflowProvider { + json: &'static str, +} const GENERATED_JSON: &str = r#"{ "title": "Gene association summary", @@ -98,7 +100,7 @@ impl Provider for FixedWorkflowProvider { _tools: &[Tool], ) -> Result<(Message, ProviderUsage), ProviderError> { Ok(( - Message::assistant().with_text(GENERATED_JSON), + Message::assistant().with_text(self.json), ProviderUsage::new("fixed-model".to_string(), Usage::default()), )) } @@ -115,6 +117,10 @@ struct Harness { } async fn harness() -> Harness { + harness_generating(GENERATED_JSON).await +} + +async fn harness_generating(json: &'static str) -> Harness { let dir = TempDir::new().unwrap(); let data_dir = dir.path().to_path_buf(); let session_manager = Arc::new(SessionManager::new(data_dir.clone())); @@ -136,7 +142,7 @@ async fn harness() -> Harness { .unwrap(); agent - .update_provider(Arc::new(FixedWorkflowProvider), &session.id) + .update_provider(Arc::new(FixedWorkflowProvider { json }), &session.id) .await .expect("bind the fixed provider"); @@ -353,3 +359,163 @@ async fn the_settings_pin_names_the_bound_provider_and_never_panics() { ); assert_eq!(settings.biorouter_model.as_deref(), Some("fixed-model")); } + +/// A generation whose optional parameter the document never refers to. +/// +/// Models emit these constantly — `workflow.md` asks for parameters and for +/// `{{ key }}` references separately, and a model that obeys the first half and +/// forgets the second produces exactly this. `output_format` below is declared +/// and then never used. +const GENERATED_JSON_WITH_AN_UNREFERENCED_PARAMETER: &str = r#"{ + "title": "Gene association summary", + "description": "Looks a gene up and summarises its disease associations.", + "instructions": "Query the graph and report the strongest associations first.", + "activities": ["Summarise APOE"], + "prompt": "Summarise the disease associations for {{ gene_symbol }}.", + "parameters": [ + { + "key": "gene_symbol", + "input_type": "string", + "requirement": "user_prompt", + "description": "HGNC gene symbol" + }, + { + "key": "output_format", + "input_type": "select", + "requirement": "optional", + "description": "How to format the summary", + "default": "markdown", + "options": ["markdown", "table"] + } + ], + "skills": [] +}"#; + +/// A workflow captured from a chat must be SAVABLE. +/// +/// The two parameter validators are individually reasonable and jointly closed +/// over a parameter the document never refers to: +/// +/// * `optional` with no `default` → "Optional parameters missing default +/// values in the workflow: output_format." +/// * any requirement, referenced nowhere → "Unnecessary parameter +/// definitions: output_format." +/// +/// So there was no `default` — present or absent — for which `POST +/// /workflows/save` accepted the generated document, and "Create workflow from +/// this chat" could not complete at all. +/// +/// Measured 2026-09-12. The refusal below, run against `origin/main`'s +/// validator, read exactly `\nUnnecessary parameter definitions: +/// output_format.` and nothing else; posting the same two documents to a +/// running daemon answered **400** both times. +/// +/// The generator is the half that is wrong. A parameter nothing refers to is a +/// question the run would ask and then discard, so it is dropped at capture +/// time — next to the existing rule that a parameter list which does not even +/// parse is dropped rather than losing the whole document. +#[tokio::test] +async fn a_captured_workflow_never_declares_a_parameter_the_document_does_not_use() { + let h = harness_generating(GENERATED_JSON_WITH_AN_UNREFERENCED_PARAMETER).await; + let knowledge = biorouter_mcp::knowledge::service::KnowledgeService::new_default() + .expect("a knowledge service in the sandboxed root"); + let session = h + .agent + .config + .session_manager + .get_session(&h.session_id, true) + .await + .unwrap(); + + let mut workflow = h + .agent + .create_workflow(session.conversation.clone().unwrap()) + .await + .expect("the capture"); + // The save path validates the ENRICHED document — the same one the modal + // posts — so the enrichment runs here too. + let enrichment = service::session_enrichment(&h.agent, &knowledge, &h.session_id, None) + .await + .expect("the route's enrichment"); + service::apply_session_enrichment(&mut workflow, enrichment); + + let keys: Vec = workflow + .parameters + .iter() + .flatten() + .map(|parameter| parameter.key.clone()) + .collect(); + assert_eq!( + keys, + vec!["gene_symbol".to_string()], + "the referenced parameter is kept and the unreferenced one dropped" + ); + + // The whole point: what the capture produced is what `POST /workflows/save` + // will accept. `service::validate` is the function that route calls. + service::validate(&workflow).unwrap_or_else(|err| { + panic!( + "a workflow captured from a chat must save: {err}\n\n{}", + workflow.to_yaml().unwrap_or_default() + ) + }); +} + +/// The two messages that closed the gap, pinned as a pair. +/// +/// Read together they are the specification the generator now satisfies: a +/// parameter must be referenced, and an `optional` one must also carry a +/// default. Neither is relaxed — the "unnecessary" arm is the only thing that +/// catches a key typo (`{{ gene }}` in the prompt beside a parameter keyed +/// `gene_symbol` reports both halves of the mismatch), and a parameter nothing +/// reads is dead weight in a document meant to be shared. What changed is that +/// the message now says what to do about it. +#[test] +fn an_unreferenced_parameter_is_still_refused_and_the_refusal_says_why() { + let unreferenced_with_a_default = r#" +version: 1.0.0 +title: Test +description: Test +instructions: Nothing refers to the parameter below. +parameters: + - key: output_format + input_type: string + requirement: optional + description: How to format the summary + default: markdown +"#; + let err = service::validate( + &biorouter::workflow::Workflow::from_content(unreferenced_with_a_default).unwrap(), + ) + .expect_err("a parameter the document never refers to is refused"); + let message = err.to_string(); + assert!( + message.contains("Unnecessary parameter definitions: output_format."), + "the refusal still names the parameter: {message}" + ); + assert!( + message.contains("{{ output_format }}"), + "and now says how to fix it, naming the key: {message}" + ); + + let unreferenced_without_a_default = r#" +version: 1.0.0 +title: Test +description: Test +instructions: Nothing refers to the parameter below. +parameters: + - key: output_format + input_type: string + requirement: optional + description: How to format the summary +"#; + let message = service::validate( + &biorouter::workflow::Workflow::from_content(unreferenced_without_a_default).unwrap(), + ) + .expect_err("dropping the default does not help") + .to_string(); + assert!( + message.contains("Optional parameters missing default values"), + "the other half of the pair, unchanged: {message}" + ); +} diff --git a/ui/desktop/src/components/workflows/shared/WorkflowFormFields.tsx b/ui/desktop/src/components/workflows/shared/WorkflowFormFields.tsx index 32bbc5d24..d2e7c0c0b 100644 --- a/ui/desktop/src/components/workflows/shared/WorkflowFormFields.tsx +++ b/ui/desktop/src/components/workflows/shared/WorkflowFormFields.tsx @@ -689,6 +689,18 @@ export function WorkflowFormFields({ onSelectedIdsChange={onKnowledgeBaseIdsChange} defaultId={defaultKnowledgeBaseId} onDefaultIdChange={onDefaultKnowledgeBaseIdChange} + // A chat with no pinned primary is captured as `default: null`, + // and the daemon never infers one from `visible` + // (`plan_knowledge_selection`) — so the workflow really will + // have no default, and every chat it starts will have no write + // target. That is the intent, not a bug, and the card said + // nothing about it while its own description promised "which + // one is focused by default": the reader had no way to tell + // which of the two they were looking at. + noDefaultText={ + 'No default — this workflow will not focus one. Chats it starts search ' + + 'every base above, and a write that names no base asks which one to use.' + } notice={knowledgeBaseNotice} emptyText="No knowledge bases found" searchPlaceholder="Search knowledge bases..." diff --git a/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx b/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx index a6e609aca..e137024cc 100644 --- a/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx +++ b/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx @@ -20,6 +20,19 @@ interface WorkflowResourcePickerProps { onSelectedIdsChange: (ids: string[]) => void; defaultId?: string | null; onDefaultIdChange?: (id: string | null) => void; + /** + * What "no default" MEANS here, shown on the card whenever a default could be + * named and none is. + * + * The rows — and with them the only mark of which item is the default — live + * inside a closed popover, so a card with a default and a card with none read + * identically from outside it. That is fine for a control whose unset state is + * obvious and wrong for one whose description promises a default: the reader + * cannot tell a captured "this chat has no primary base" from a bug. The + * wording belongs to the caller because what the absence costs is + * domain-specific. + */ + noDefaultText?: string; /** A standing condition the selection is subject to, shown under the label. */ notice?: string; emptyText: string; @@ -41,6 +54,7 @@ export function WorkflowResourcePicker({ onSelectedIdsChange, defaultId, onDefaultIdChange, + noDefaultText, notice, emptyText, searchPlaceholder, @@ -50,6 +64,20 @@ export function WorkflowResourcePicker({ const [query, setQuery] = useState(''); const selected = useMemo(() => new Set(selectedIds), [selectedIds]); + // Named by id, labelled by label: the id is what is saved, and a picker whose + // items have not loaded yet must still be able to say a default is set. + const defaultLabel = defaultId + ? (items.find((item) => item.id === defaultId)?.label ?? defaultId) + : null; + // Nothing selected already says its own thing on the trigger ("No KBs + // selected"), and "no default" on top of it is noise about a set that is empty. + const defaultSummary = + !onDefaultIdChange || selectedIds.length === 0 + ? null + : defaultLabel + ? `Default: ${defaultLabel}` + : (noDefaultText ?? null); + const filteredItems = useMemo(() => { const q = query.trim().toLowerCase(); const filtered = q @@ -91,6 +119,14 @@ export function WorkflowResourcePicker({
{description &&

{description}

} + {defaultSummary && ( +

+ {defaultSummary} +

+ )}
diff --git a/ui/desktop/src/components/workflows/shared/__tests__/WorkflowFormFields.test.tsx b/ui/desktop/src/components/workflows/shared/__tests__/WorkflowFormFields.test.tsx index 59043e319..885fb4e66 100644 --- a/ui/desktop/src/components/workflows/shared/__tests__/WorkflowFormFields.test.tsx +++ b/ui/desktop/src/components/workflows/shared/__tests__/WorkflowFormFields.test.tsx @@ -847,4 +847,48 @@ describe('WorkflowFormFields', () => { expect(result).toEqual(['user_name', 'user_id', 'email_address', 'app_name']); }); }); + + /** + * A chat with no pinned primary base is captured as `default: null`, and the + * daemon never infers a primary from `visible` + * (`plan_knowledge_selection`) — so the workflow really will have no default. + * The card promised "which one is focused by default" and then said nothing at + * all about not having one, leaving the correct capture indistinguishable from + * a bug. + */ + describe('Knowledge bases card', () => { + const renderKnowledgeCard = (defaultKnowledgeBaseId: string | null) => + render( + + ); + + it('says the workflow will have no default when none is named', async () => { + const user = userEvent.setup(); + renderKnowledgeCard(null); + await expandAdvancedSection(user); + + const summary = screen.getByTestId('resource-picker-default-summary'); + expect(summary).toHaveTextContent(/no default/i); + expect(summary).toHaveTextContent(/this workflow will not focus one/i); + }); + + it('names the default when one is named', async () => { + const user = userEvent.setup(); + renderKnowledgeCard('lab-notes'); + await expandAdvancedSection(user); + + expect(screen.getByTestId('resource-picker-default-summary')).toHaveTextContent( + 'Default: lab-notes' + ); + }); + }); }); diff --git a/ui/desktop/src/components/workflows/shared/__tests__/WorkflowResourcePicker.test.tsx b/ui/desktop/src/components/workflows/shared/__tests__/WorkflowResourcePicker.test.tsx index beb0fd7a6..37476fdc7 100644 --- a/ui/desktop/src/components/workflows/shared/__tests__/WorkflowResourcePicker.test.tsx +++ b/ui/desktop/src/components/workflows/shared/__tests__/WorkflowResourcePicker.test.tsx @@ -6,12 +6,15 @@ import { WorkflowResourcePicker } from '../WorkflowResourcePicker'; function renderPicker({ selectedIds, defaultId, + noDefaultText, + onDefaultIdChange = vi.fn(), }: { selectedIds: string[]; defaultId: string | null; + noDefaultText?: string; + onDefaultIdChange?: (id: string | null) => void; }) { const onSelectedIdsChange = vi.fn(); - const onDefaultIdChange = vi.fn(); render( { expect(onDefaultIdChange).toHaveBeenLastCalledWith(null); }); }); + +/** + * Whether a default is set is only ever *marked* on a row, and the rows live + * inside a closed popover — so a captured "this chat has no primary base", which + * is the correct capture for a chat that pinned none, looked exactly like a card + * whose default had gone missing. The card has to say it. + */ +describe('WorkflowResourcePicker default summary', () => { + it('says there is no default, on the card, without opening the popover', () => { + renderPicker({ + selectedIds: ['lab-notes', 'soul'], + defaultId: null, + noDefaultText: NO_DEFAULT, + }); + + expect(screen.getByTestId('resource-picker-default-summary')).toHaveTextContent(NO_DEFAULT); + // Still closed: the rows, and the Default control that marks one, are not + // rendered at all. + expect(screen.queryByRole('button', { name: 'Default KB: lab-notes' })).toBeNull(); + }); + + it('names the default on the card when one is set', () => { + renderPicker({ + selectedIds: ['lab-notes', 'soul'], + defaultId: 'lab-notes', + noDefaultText: NO_DEFAULT, + }); + + expect(screen.getByTestId('resource-picker-default-summary')).toHaveTextContent( + 'Default: lab-notes' + ); + }); + + // An empty selection already says so on the trigger, and "no default" on top + // of it is a statement about a set with nothing in it. + it('says nothing about a default when nothing is selected', () => { + renderPicker({ selectedIds: [], defaultId: null, noDefaultText: NO_DEFAULT }); + + expect(screen.queryByTestId('resource-picker-default-summary')).toBeNull(); + }); + + // Skills and extensions have no default at all; the line must not appear for + // a picker that cannot name one. + it('says nothing for a picker with no default control', () => { + render( + + ); + + expect(screen.queryByTestId('resource-picker-default-summary')).toBeNull(); + }); +});