From a3bad1bdd4a25f8e19d4b763487d2bc53433182e Mon Sep 17 00:00:00 2001 From: Codex Automation Date: Tue, 2 Jun 2026 10:22:15 +0200 Subject: [PATCH] Validate workflow rule writes --- .../WorkflowRulesEditorTests.cs | 114 ++++++++++ .../Workflow/MeetingWorkflowEngine.cs | 67 +----- .../Workflow/MeetingWorkflowRulesValidator.cs | 209 ++++++++++++++++++ .../MeetingWorkflowTemplateRenderer.cs | 78 +++++++ .../Workflow/WorkflowRulesEditorTools.cs | 13 +- docs/meeting-workflow-engine.md | 4 +- .../specs/meeting-session/spec.md | 9 +- 7 files changed, 419 insertions(+), 75 deletions(-) create mode 100644 MeetingAssistant/Workflow/MeetingWorkflowRulesValidator.cs create mode 100644 MeetingAssistant/Workflow/MeetingWorkflowTemplateRenderer.cs diff --git a/MeetingAssistant.Tests/WorkflowRulesEditorTests.cs b/MeetingAssistant.Tests/WorkflowRulesEditorTests.cs index e3cf588..5e392e9 100644 --- a/MeetingAssistant.Tests/WorkflowRulesEditorTests.cs +++ b/MeetingAssistant.Tests/WorkflowRulesEditorTests.cs @@ -540,6 +540,120 @@ public sealed class WorkflowRulesEditorTests Assert.Equal("rules: []", await File.ReadAllTextAsync(rulesPath)); } + [Fact] + public async Task RulesEditorToolsRefuseWorkflowRulesThatWouldFailAtRuntime() + { + var root = Path.Combine(Path.GetTempPath(), "meeting-assistant-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + var rulesPath = Path.Combine(root, "rules.yaml"); + await File.WriteAllTextAsync(rulesPath, "rules: []"); + var tools = new WorkflowRulesEditorTools(new MeetingAssistantOptions + { + Automation = new AutomationOptions { RulesPath = rulesPath } + }); + + var result = await tools.WriteRules(""" + rules: + - name: broken-step + on: + - created: {} + steps: + - uses: send_email + value: Manuel + """, replace_file: true); + + Assert.StartsWith("Refused: workflow rules are invalid.", result); + Assert.Contains("broken-step", result); + Assert.Contains("send_email", result); + Assert.Contains("add_attendee", result); + Assert.Equal("rules: []", await File.ReadAllTextAsync(rulesPath)); + } + + [Fact] + public async Task RulesEditorToolsExplainUnsupportedWorkflowProperties() + { + var root = Path.Combine(Path.GetTempPath(), "meeting-assistant-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + var rulesPath = Path.Combine(root, "rules.yaml"); + await File.WriteAllTextAsync(rulesPath, "rules: []"); + var tools = new WorkflowRulesEditorTools(new MeetingAssistantOptions + { + Automation = new AutomationOptions { RulesPath = rulesPath } + }); + + var result = await tools.WriteRules(""" + rules: + - name: broken-property + on: + - created: {} + steps: + - uses: set_property + property: meeting.status + value: active + """, replace_file: true); + + Assert.StartsWith("Refused: workflow rules are invalid.", result); + Assert.Contains("broken-property", result); + Assert.Contains("meeting.status", result); + Assert.Contains("title", result); + Assert.Equal("rules: []", await File.ReadAllTextAsync(rulesPath)); + } + + [Fact] + public async Task RulesEditorToolsRefuseMalformedRazorStepValues() + { + var root = Path.Combine(Path.GetTempPath(), "meeting-assistant-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + var rulesPath = Path.Combine(root, "rules.yaml"); + await File.WriteAllTextAsync(rulesPath, "rules: []"); + var tools = new WorkflowRulesEditorTools(new MeetingAssistantOptions + { + Automation = new AutomationOptions { RulesPath = rulesPath } + }); + + var result = await tools.WriteRules(""" + rules: + - name: broken-razor + on: + - created: {} + steps: + - uses: add_context + value: "Invalid template: @ThisIsNotAModelVar" + """, replace_file: true); + + Assert.StartsWith("Refused: workflow rules are invalid.", result); + Assert.Contains("broken-razor", result); + Assert.Contains("Razor", result); + Assert.Contains("ThisIsNotAModelVar", result); + Assert.Equal("rules: []", await File.ReadAllTextAsync(rulesPath)); + } + + [Fact] + public async Task RulesEditorToolsAllowValidRazorAndEmailStepValues() + { + var root = Path.Combine(Path.GetTempPath(), "meeting-assistant-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + var rulesPath = Path.Combine(root, "rules.yaml"); + var tools = new WorkflowRulesEditorTools(new MeetingAssistantOptions + { + Automation = new AutomationOptions { RulesPath = rulesPath } + }); + + var result = await tools.WriteRules(""" + rules: + - name: valid-razor + on: + - speaker_identified: {} + steps: + - uses: add_context + value: "Speaker @Model.Speaker.Name can contact Support@example.com" + """, replace_file: true); + + Assert.Equal(rulesPath, result); + Assert.Contains("@Model.Speaker.Name", await File.ReadAllTextAsync(rulesPath)); + Assert.Contains("Support@example.com", await File.ReadAllTextAsync(rulesPath)); + } + [Fact] public async Task InstructionBuilderIncludesConfiguredRulesPathAndWorkflowDocs() { diff --git a/MeetingAssistant/Workflow/MeetingWorkflowEngine.cs b/MeetingAssistant/Workflow/MeetingWorkflowEngine.cs index 9e47cc6..7fc85e3 100644 --- a/MeetingAssistant/Workflow/MeetingWorkflowEngine.cs +++ b/MeetingAssistant/Workflow/MeetingWorkflowEngine.cs @@ -3,7 +3,6 @@ using System.Globalization; using System.Text.RegularExpressions; using MeetingAssistant.MeetingNotes; using NCalc; -using RazorLight; namespace MeetingAssistant.Workflow; @@ -30,10 +29,6 @@ public sealed class NoopMeetingWorkflowEngine : IMeetingWorkflowEngine public sealed class MeetingWorkflowEngine : IMeetingWorkflowEngine { - private static readonly Regex EmailAddressPattern = new( - @"(?[A-Za-z0-9.!#$%&'*+/=?^_`{|}~-]+)@(?(?:[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?\.)+[A-Za-z]{2,63})(?![A-Za-z0-9-])", - RegexOptions.Compiled | RegexOptions.CultureInvariant | RegexOptions.IgnoreCase); - private static readonly string[] ParameterNames = [ "meeting.attendees.count", @@ -51,9 +46,7 @@ public sealed class MeetingWorkflowEngine : IMeetingWorkflowEngine private readonly IMeetingNoteStore meetingNoteStore; private readonly IMeetingArtifactStore meetingArtifactStore; private readonly ILogger logger; - private readonly RazorLightEngine razorEngine = new RazorLightEngineBuilder() - .UseNoProject() - .Build(); + private readonly MeetingWorkflowTemplateRenderer templateRenderer = new(); public MeetingWorkflowEngine( IMeetingWorkflowRulesProvider rulesProvider, @@ -256,63 +249,7 @@ public sealed class MeetingWorkflowEngine : IMeetingWorkflowEngine string template, MeetingWorkflowTemplateModel model) { - if (!ContainsRazorTemplateSyntax(template)) - { - return template; - } - - return await razorEngine.CompileRenderStringAsync( - Guid.NewGuid().ToString("N"), - EscapeEmailAddressAtSigns(template), - model); - } - - private static bool ContainsRazorTemplateSyntax(string value) - { - if (!value.Contains('@', StringComparison.Ordinal)) - { - return false; - } - - var emailAtSigns = EmailAddressPattern - .Matches(value) - .Select(match => match.Index + match.Value.IndexOf('@', StringComparison.Ordinal)) - .ToHashSet(); - - for (var index = 0; index < value.Length; index++) - { - if (value[index] != '@') - { - continue; - } - - if (index + 1 < value.Length && value[index + 1] == '@') - { - index++; - continue; - } - - if (emailAtSigns.Contains(index)) - { - continue; - } - - return true; - } - - return false; - } - - private static string EscapeEmailAddressAtSigns(string value) - { - if (!value.Contains('@', StringComparison.Ordinal)) - { - return value; - } - - return EmailAddressPattern.Replace( - value, - match => match.Value.Replace("@", "@@", StringComparison.Ordinal)); + return await templateRenderer.RenderAsync(template, model); } private static bool SetProperty( diff --git a/MeetingAssistant/Workflow/MeetingWorkflowRulesValidator.cs b/MeetingAssistant/Workflow/MeetingWorkflowRulesValidator.cs new file mode 100644 index 0000000..7fda1fe --- /dev/null +++ b/MeetingAssistant/Workflow/MeetingWorkflowRulesValidator.cs @@ -0,0 +1,209 @@ +using MeetingAssistant.MeetingNotes; + +namespace MeetingAssistant.Workflow; + +internal static class MeetingWorkflowRulesValidator +{ + private static readonly string[] SupportedTriggerKeys = ["created", "state_transition", "speaker_identified"]; + private static readonly string[] SupportedConditionKeys = ["condition", "and", "or", "not"]; + private static readonly string[] SupportedStepUses = + ["add_attendee", "remove_attendee", "add_project", "set_property", "add_context"]; + private static readonly string[] SupportedSetPropertyNames = ["title", "meeting.title"]; + + public static async Task> ValidateAsync(MeetingWorkflowRulesFile rulesFile) + { + var errors = new List(); + var templateRenderer = new MeetingWorkflowTemplateRenderer(); + var templateModel = CreateValidationTemplateModel(); + for (var ruleIndex = 0; ruleIndex < rulesFile.Rules.Count; ruleIndex++) + { + var rule = rulesFile.Rules[ruleIndex]; + var ruleName = GetRuleName(rule, ruleIndex); + if (rule.On.Count == 0) + { + errors.Add($"{ruleName} must define at least one trigger in 'on'. Supported triggers: {JoinAllowed(SupportedTriggerKeys)}."); + } + + for (var triggerIndex = 0; triggerIndex < rule.On.Count; triggerIndex++) + { + ValidateTrigger(rule.On[triggerIndex], ruleName, triggerIndex, errors); + } + + for (var conditionIndex = 0; conditionIndex < rule.If.Count; conditionIndex++) + { + ValidateCondition(rule.If[conditionIndex], ruleName, $"if[{conditionIndex + 1}]", errors); + } + + if (rule.Steps.Count == 0) + { + errors.Add($"{ruleName} must define at least one step. Supported steps: {JoinAllowed(SupportedStepUses)}."); + } + + for (var stepIndex = 0; stepIndex < rule.Steps.Count; stepIndex++) + { + await ValidateStepAsync( + rule.Steps[stepIndex], + ruleName, + stepIndex, + errors, + templateRenderer, + templateModel); + } + } + + return errors; + } + + private static void ValidateTrigger( + MeetingWorkflowTrigger trigger, + string ruleName, + int triggerIndex, + List errors) + { + var configuredCount = CountConfigured( + trigger.Created is not null, + trigger.StateTransition is not null, + trigger.SpeakerIdentified is not null); + if (configuredCount == 0) + { + errors.Add($"{ruleName} on[{triggerIndex + 1}] must contain one supported trigger: {JoinAllowed(SupportedTriggerKeys)}."); + } + else if (configuredCount > 1) + { + errors.Add($"{ruleName} on[{triggerIndex + 1}] must contain only one trigger."); + } + } + + private static void ValidateCondition( + MeetingWorkflowCondition condition, + string ruleName, + string path, + List errors) + { + var configuredCount = CountConfigured( + !string.IsNullOrWhiteSpace(condition.Condition), + condition.And is { Count: > 0 }, + condition.Or is { Count: > 0 }, + condition.Not is not null); + if (configuredCount == 0) + { + errors.Add($"{ruleName} {path} must contain one supported condition key: {JoinAllowed(SupportedConditionKeys)}."); + return; + } + + if (configuredCount > 1) + { + errors.Add($"{ruleName} {path} must contain only one condition key."); + } + + if (condition.And is { Count: > 0 } andConditions) + { + for (var index = 0; index < andConditions.Count; index++) + { + ValidateCondition(andConditions[index], ruleName, $"{path}.and[{index + 1}]", errors); + } + } + + if (condition.Or is { Count: > 0 } orConditions) + { + for (var index = 0; index < orConditions.Count; index++) + { + ValidateCondition(orConditions[index], ruleName, $"{path}.or[{index + 1}]", errors); + } + } + + if (condition.Not is not null) + { + ValidateCondition(condition.Not, ruleName, $"{path}.not", errors); + } + } + + private static async Task ValidateStepAsync( + MeetingWorkflowStep step, + string ruleName, + int stepIndex, + List errors, + MeetingWorkflowTemplateRenderer templateRenderer, + MeetingWorkflowTemplateModel templateModel) + { + var stepPath = $"{ruleName} steps[{stepIndex + 1}]"; + var uses = step.Uses.Trim(); + if (string.IsNullOrWhiteSpace(uses)) + { + errors.Add($"{stepPath} must define 'uses'. Supported steps: {JoinAllowed(SupportedStepUses)}."); + return; + } + + if (!SupportedStepUses.Contains(uses, StringComparer.OrdinalIgnoreCase)) + { + errors.Add($"{stepPath} uses unsupported step '{uses}'. Supported steps: {JoinAllowed(SupportedStepUses)}."); + return; + } + + if (string.Equals(uses, "set_property", StringComparison.OrdinalIgnoreCase)) + { + var property = step.Property ?? step.Name; + if (string.IsNullOrWhiteSpace(property)) + { + errors.Add($"{stepPath} must define 'property' or 'name' for set_property. Supported properties: {JoinAllowed(SupportedSetPropertyNames)}."); + } + else if (!SupportedSetPropertyNames.Contains(property.Trim(), StringComparer.OrdinalIgnoreCase)) + { + errors.Add($"{stepPath} uses unsupported property '{property}'. Supported properties: {JoinAllowed(SupportedSetPropertyNames)}."); + } + } + + if (step.Value is { } value && + MeetingWorkflowTemplateRenderer.ContainsRazorTemplateSyntax(value)) + { + try + { + _ = await templateRenderer.RenderAsync(value, templateModel); + } + catch (Exception exception) + { + errors.Add($"{stepPath} has invalid Razor template in 'value' ({value}). {FormatRazorValidationMessage(exception)}"); + } + } + } + + private static string GetRuleName(MeetingWorkflowRule rule, int ruleIndex) + { + return string.IsNullOrWhiteSpace(rule.Name) + ? $"rule[{ruleIndex + 1}]" + : $"rule '{rule.Name}'"; + } + + private static int CountConfigured(params bool[] values) + { + return values.Count(static value => value); + } + + private static string JoinAllowed(IEnumerable values) + { + return string.Join(", ", values); + } + + private static MeetingWorkflowTemplateModel CreateValidationTemplateModel() + { + return new MeetingWorkflowTemplateModel( + new MeetingWorkflowMeetingModel( + "Validation Meeting", + ["Ada Lovelace"], + ["Validation Project"], + MeetingWorkflowStateNames.ToRuleName(AssistantContextState.Transcribing)), + new MeetingWorkflowEventModel( + MeetingWorkflowEventType.StateTransition.ToString(), + MeetingWorkflowStateNames.ToRuleName(AssistantContextState.CollectingMetadata), + MeetingWorkflowStateNames.ToRuleName(AssistantContextState.Transcribing)), + new MeetingWorkflowSpeakerModel("Ada Lovelace")); + } + + private static string FormatRazorValidationMessage(Exception exception) + { + var message = exception.Message.Split(Environment.NewLine)[0].Trim(); + return string.IsNullOrWhiteSpace(message) + ? "The template could not be compiled or rendered with the workflow model." + : message; + } +} diff --git a/MeetingAssistant/Workflow/MeetingWorkflowTemplateRenderer.cs b/MeetingAssistant/Workflow/MeetingWorkflowTemplateRenderer.cs new file mode 100644 index 0000000..98c4c1b --- /dev/null +++ b/MeetingAssistant/Workflow/MeetingWorkflowTemplateRenderer.cs @@ -0,0 +1,78 @@ +using System.Text.RegularExpressions; +using RazorLight; + +namespace MeetingAssistant.Workflow; + +internal sealed class MeetingWorkflowTemplateRenderer +{ + private static readonly Regex EmailAddressPattern = new( + @"(?[A-Za-z0-9.!#$%&'*+/=?^_`{|}~-]+)@(?(?:[A-Za-z0-9](?:[A-Za-z0-9-]{0,61}[A-Za-z0-9])?\.)+[A-Za-z]{2,63})(?![A-Za-z0-9-])", + RegexOptions.Compiled | RegexOptions.CultureInvariant | RegexOptions.IgnoreCase); + + private readonly RazorLightEngine razorEngine = new RazorLightEngineBuilder() + .UseNoProject() + .Build(); + + public async Task RenderAsync( + string template, + MeetingWorkflowTemplateModel model) + { + if (!ContainsRazorTemplateSyntax(template)) + { + return template; + } + + return await razorEngine.CompileRenderStringAsync( + Guid.NewGuid().ToString("N"), + EscapeEmailAddressAtSigns(template), + model); + } + + public static bool ContainsRazorTemplateSyntax(string value) + { + if (!value.Contains('@', StringComparison.Ordinal)) + { + return false; + } + + var emailAtSigns = EmailAddressPattern + .Matches(value) + .Select(match => match.Index + match.Value.IndexOf('@', StringComparison.Ordinal)) + .ToHashSet(); + + for (var index = 0; index < value.Length; index++) + { + if (value[index] != '@') + { + continue; + } + + if (index + 1 < value.Length && value[index + 1] == '@') + { + index++; + continue; + } + + if (emailAtSigns.Contains(index)) + { + continue; + } + + return true; + } + + return false; + } + + private static string EscapeEmailAddressAtSigns(string value) + { + if (!value.Contains('@', StringComparison.Ordinal)) + { + return value; + } + + return EmailAddressPattern.Replace( + value, + match => match.Value.Replace("@", "@@", StringComparison.Ordinal)); + } +} diff --git a/MeetingAssistant/Workflow/WorkflowRulesEditorTools.cs b/MeetingAssistant/Workflow/WorkflowRulesEditorTools.cs index 3b6bf70..f5b3a74 100644 --- a/MeetingAssistant/Workflow/WorkflowRulesEditorTools.cs +++ b/MeetingAssistant/Workflow/WorkflowRulesEditorTools.cs @@ -120,7 +120,7 @@ public sealed class WorkflowRulesEditorTools var updated = replace_file ? yaml : AgentFileToolContent.AppendContent(existing, yaml); - var validation = ValidateYaml(updated); + var validation = await ValidateYamlAsync(updated); if (validation is not null) { return validation; @@ -930,22 +930,27 @@ public sealed class WorkflowRulesEditorTools } } - private static string? ValidateYaml(string yaml) + private static async Task ValidateYamlAsync(string yaml) { if (string.IsNullOrWhiteSpace(yaml)) { return null; } + MeetingWorkflowRulesFile rulesFile; try { - _ = YamlDeserializer.Deserialize(yaml); - return null; + rulesFile = YamlDeserializer.Deserialize(yaml) ?? new MeetingWorkflowRulesFile(); } catch (YamlException exception) { return $"Refused: workflow rules YAML is invalid. {exception.Message}"; } + + var errors = await MeetingWorkflowRulesValidator.ValidateAsync(rulesFile); + return errors.Count == 0 + ? null + : "Refused: workflow rules are invalid. " + string.Join(" ", errors); } private async Task WriteRecommendedProjectSeedAsync(string projectRoot, string projectName) diff --git a/docs/meeting-workflow-engine.md b/docs/meeting-workflow-engine.md index eb334db..8179b68 100644 --- a/docs/meeting-workflow-engine.md +++ b/docs/meeting-workflow-engine.md @@ -48,7 +48,7 @@ Blank editor values inherit from `MeetingAssistant:Agent`. This keeps the editor The editor agent prompt includes this document and is restricted to these tools: - `read_rules`: read the configured rules file, optionally by inclusive 1-based line range. -- `write_rules`: append to the configured rules file by default after validating that the complete YAML document parses as a workflow rules document. Pass `replace_file: true` only when intentionally replacing the whole rules file. +- `write_rules`: append to the configured rules file by default after validating that the complete YAML document parses as a supported workflow rules document. Pass `replace_file: true` only when intentionally replacing the whole rules file. When validation fails, the tool refuses the write and returns the invalid rule/field plus supported values so the agent can fix and retry. - `search`: search only the configured rules file with ripgrep-style syntax. `POST /diagnostics/workflow/reload` reloads application configuration without restarting the process. Use it after changing workflow automation configuration such as `MeetingAssistant:Automation:RulesPath`. The endpoint returns the currently bound rules path: @@ -79,7 +79,7 @@ For every event, the engine: 6. Saves the meeting note once if any meeting-note step changed it. 7. Updates assistant-context frontmatter links/title from the saved meeting note after note changes. -Rules are best-effort automation. Invalid expressions, unknown steps, or unsupported properties currently fail the workflow event and should be covered by tests before the engine is broadened. +Rules are best-effort automation. The settings/logs write tool rejects invalid YAML, unknown steps, unsupported set-property fields, trigger or condition entries without a supported key, and step value Razor templates that fail against the workflow template model before writing the rules file. Invalid NCalc expressions can still fail at workflow runtime and should be covered by tests before the engine is broadened. ## YAML Shape diff --git a/openspec/changes/generalize-settings-and-logs-agent/specs/meeting-session/spec.md b/openspec/changes/generalize-settings-and-logs-agent/specs/meeting-session/spec.md index d89e5e9..2c495fb 100644 --- a/openspec/changes/generalize-settings-and-logs-agent/specs/meeting-session/spec.md +++ b/openspec/changes/generalize-settings-and-logs-agent/specs/meeting-session/spec.md @@ -18,11 +18,11 @@ The settings/logs agent SHALL be configured from the summarizer agent settings b The settings/logs agent SHALL include the workflow engine documentation in its system prompt and SHALL receive read, write, and search tools scoped to the configured workflow rules file. -The rules editor `write_rules` tool SHALL append to the configured workflow rules file by default and SHALL validate the complete resulting YAML document before writing it. +The rules editor `write_rules` tool SHALL append to the configured workflow rules file by default and SHALL validate the complete resulting YAML document as supported workflow rules before writing it. The rules editor `write_rules` tool SHALL replace the whole configured workflow rules file only when `replace_file` is true. -The rules editor `write_rules` tool SHALL refuse invalid YAML without changing the configured workflow rules file. +The rules editor `write_rules` tool SHALL refuse invalid YAML, unsupported workflow rule shape, or workflow step Razor templates that fail against the workflow template model without changing the configured workflow rules file and SHALL return enough detail for the agent to fix and retry. The settings/logs agent SHALL receive speaker identity tools to search/list, read, create, update, delete, and merge identities in the local speaker identity database. @@ -115,11 +115,12 @@ Meeting Assistant SHALL provide a diagnostic endpoint that opens the settings/lo - **WHEN** the rules editor writes valid YAML with `replace_file` set to true - **THEN** Meeting Assistant replaces the complete rules file with the supplied YAML -#### Scenario: Rules editor refuses invalid YAML +#### Scenario: Rules editor refuses invalid workflow rules - **GIVEN** a configured workflow rules file with valid existing rules -- **WHEN** the rules editor writes YAML that would make the file invalid +- **WHEN** the rules editor writes YAML that would make the file invalid, unsupported by the workflow engine, or contain a malformed step value Razor template - **THEN** Meeting Assistant refuses the write - **AND** keeps the existing rules file unchanged +- **AND** returns the invalid rule or field plus supported values when available #### Scenario: Rules editor can manage speaker identities - **GIVEN** the speaker identity database contains speaker identities and samples