diff --git a/pkg/workflow/model_alias_validation.go b/pkg/workflow/model_alias_validation.go index 15da312457..4b16e71f5c 100644 --- a/pkg/workflow/model_alias_validation.go +++ b/pkg/workflow/model_alias_validation.go @@ -52,18 +52,32 @@ func (c *Compiler) validateModelAliasMap( modelAliasValidationLog.Printf("Validating model alias map: %d merged entries, %d frontmatter entries, engine.model=%q", len(mergedAliasMap), len(frontmatterModels), engineModel) - // V-MAF-004: engine.model MUST NOT be a glob pattern. - // GitHub Actions expressions (${{ … }}) are exempt from syntax checks. - if engineModel != "" && !isExpression(engineModel) { - if err := validateEngineModelNotGlob(engineModel, markdownPath); err != nil { - return err + // V-MAF-004: engine.model MUST NOT contain a glob pattern ("*"). + // The check is always performed, but only on the *literal* parts of the + // value — expression segments (${{ ... }}) are stripped first so that a + // "*" inside an expression body (e.g. "${{ contains(inputs.m, '*') }}") + // is not falsely flagged. A "*" that appears *outside* an expression + // (e.g. "${{ inputs.model }}*" or "copilot/*${{ inputs.model }}") is + // still a glob and must be rejected. + if engineModel != "" { + literalText := ExpressionPattern.ReplaceAllString(engineModel, "") + if strings.Contains(literalText, "*") { + return formatCompilerError(markdownPath, "error", + fmt.Sprintf("engine.model: glob patterns are not allowed in engine.model; "+ + "got %q — glob patterns may only appear in models alias list entries (V-MAF-004)", engineModel), + nil) } - // V-MAF-001 + V-MAF-006: validate syntax of engine.model. - if errs := validateModelIdentifierStrings([]string{engineModel}, "engine.model"); len(errs) > 0 { - return formatCompilerError(markdownPath, "error", errs[0], nil) + // Syntax and parameter checks ($-character parsing, known params) are + // skipped for runtime-resolved expressions — they cannot be parsed at + // compile time. + if !containsExpression(engineModel) { + // V-MAF-001 + V-MAF-006: validate syntax of engine.model. + if errs := validateModelIdentifierStrings([]string{engineModel}, "engine.model"); len(errs) > 0 { + return formatCompilerError(markdownPath, "error", errs[0], nil) + } + // V-MAF-011: warn about unrecognised parameter keys in engine.model. + c.warnUnrecognizedModelParams([]string{engineModel}, markdownPath) } - // V-MAF-011: warn about unrecognised parameter keys in engine.model. - c.warnUnrecognizedModelParams([]string{engineModel}, markdownPath) } // Validate user-supplied frontmatter aliases only (builtins are pre-validated). @@ -91,20 +105,6 @@ func (c *Compiler) validateModelAliasMap( return nil } -// ─── V-MAF-004: glob check for engine.model ─────────────────────────────────── - -// validateEngineModelNotGlob returns an error if the engine.model value is a glob -// pattern (contains "*"), which is prohibited in engine.model (V-MAF-004). -func validateEngineModelNotGlob(engineModel, markdownPath string) error { - if strings.Contains(engineModel, "*") { - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("engine.model: glob patterns are not allowed in engine.model; "+ - "got %q — glob patterns may only appear in models alias list entries (V-MAF-004)", engineModel), - nil) - } - return nil -} - // ─── V-MAF-005: alias key validation ───────────────────────────────────────── // validateAliasKey validates a single alias map key (V-MAF-005). @@ -136,7 +136,9 @@ func validateModelIdentifierStrings(identifiers []string, context string) []stri continue } // Skip GitHub Actions expressions — they are resolved at runtime. - if isExpression(id) { + // This includes whole-string expressions ("${{ inputs.model }}") and + // partial expressions ("${{ inputs.model }}?effort=high", "copilot/${{ inputs.model }}"). + if containsExpression(id) { continue } p, err := ParseModelIdentifier(id) @@ -159,7 +161,7 @@ func validateModelIdentifierStrings(identifiers []string, context string) []stri // parameter key found in the given model identifier strings (V-MAF-011). func (c *Compiler) warnUnrecognizedModelParams(identifiers []string, markdownPath string) { for _, id := range identifiers { - if id == "" || isExpression(id) { + if id == "" || containsExpression(id) { continue } p, err := ParseModelIdentifier(id) diff --git a/pkg/workflow/model_alias_validation_test.go b/pkg/workflow/model_alias_validation_test.go index 79c938e15c..26d83d4e41 100644 --- a/pkg/workflow/model_alias_validation_test.go +++ b/pkg/workflow/model_alias_validation_test.go @@ -424,6 +424,47 @@ func TestValidateModelAliasMap_ExpressionModelSkipped(t *testing.T) { assert.NoError(t, err, "GitHub Actions expression in engine.model should be skipped") } +func TestValidateModelAliasMap_PartialExpressionEngineModelSkipped(t *testing.T) { + // Partial expressions (expression + query params) must also be exempt from validation. + // The query params cannot be evaluated until the expression is resolved at runtime. + compiler := NewCompiler() + err := compiler.validateModelAliasMap( + BuiltinModelAliases(), + nil, + "${{ inputs.model }}?effort=high", + "/fake/path/workflow.md", + ) + assert.NoError(t, err, "partial expression in engine.model should be skipped") +} + +func TestValidateModelAliasMap_ExpressionInAliasEntriesSkipped(t *testing.T) { + // Expressions in alias list entries must be exempt from validation. + compiler := NewCompiler() + err := compiler.validateModelAliasMap( + BuiltinModelAliases(), + map[string][]string{ + "dynamic": {"${{ inputs.model }}", "${{ inputs.fallback }}"}, + }, + "", + "/fake/path/workflow.md", + ) + assert.NoError(t, err, "GitHub Actions expressions in alias entries should be skipped") +} + +func TestValidateModelAliasMap_PartialExpressionInAliasEntriesSkipped(t *testing.T) { + // Partial expressions in alias entries (expression with query params) must be exempt. + compiler := NewCompiler() + err := compiler.validateModelAliasMap( + BuiltinModelAliases(), + map[string][]string{ + "dynamic": {"${{ inputs.model }}?effort=high", "copilot/${{ inputs.model_token }}"}, + }, + "", + "/fake/path/workflow.md", + ) + assert.NoError(t, err, "partial expressions in alias entries should be skipped") +} + func TestValidateModelAliasMap_NoEngineModel(t *testing.T) { compiler := NewCompiler() err := compiler.validateModelAliasMap( @@ -473,3 +514,183 @@ func TestIsAliasReference(t *testing.T) { assert.False(t, isAliasReference("copilot/*sonnet*", aliasMap), "glob entry should not be alias reference") assert.False(t, isAliasReference("unknown", aliasMap), "unknown bare name is not an alias reference") } + +// ─── Expression skip tests for validateModelIdentifierStrings ───────────────── + +// TestValidateModelIdentifierStrings_ExpressionForms covers every expression +// form that must be silently skipped by validateModelIdentifierStrings. +func TestValidateModelIdentifierStrings_ExpressionForms(t *testing.T) { + tests := []struct { + name string + identifiers []string + wantErrs bool + }{ + // Whole-string expression (original supported form). + { + name: "whole-string expression", + identifiers: []string{"${{ inputs.model }}"}, + wantErrs: false, + }, + // Expression with trailing query params — new form supported after this change. + { + name: "expression with effort param", + identifiers: []string{"${{ inputs.model }}?effort=high"}, + wantErrs: false, + }, + { + name: "expression with temperature param", + identifiers: []string{"${{ inputs.model }}?temperature=0.7"}, + wantErrs: false, + }, + { + name: "expression with multiple params", + identifiers: []string{"${{ inputs.model }}?effort=high&temperature=0.5"}, + wantErrs: false, + }, + // Expression embedded inside a provider-scoped identifier — new form. + { + name: "expression as model token in provider-scoped identifier", + identifiers: []string{"copilot/${{ inputs.model_token }}"}, + wantErrs: false, + }, + // Multiple expressions in the same list — each entry evaluated independently. + { + name: "multiple expression entries", + identifiers: []string{ + "${{ inputs.primary }}", + "${{ inputs.fallback }}", + }, + wantErrs: false, + }, + // Mix of expression and valid literal — literal must still be validated. + { + name: "mix of expression and valid literal", + identifiers: []string{ + "${{ inputs.model }}", + "opus", + }, + wantErrs: false, + }, + // Mix of expression and invalid literal — literal error must still be reported. + { + name: "mix of expression and invalid literal", + identifiers: []string{ + "${{ inputs.model }}", + "invalid identifier!", + }, + wantErrs: true, + }, + // Expression with compact (no-space) syntax. + { + name: "compact expression syntax", + identifiers: []string{"${{inputs.model}}"}, + wantErrs: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errs := validateModelIdentifierStrings(tt.identifiers, "test context") + if tt.wantErrs { + assert.NotEmpty(t, errs, "expected validation errors but got none") + } else { + assert.Empty(t, errs, "expected no validation errors but got: %v", errs) + } + }) + } +} + +// ─── Expression skip tests for warnUnrecognizedModelParams ──────────────────── + +// TestWarnUnrecognizedModelParams_ExpressionSkipped verifies that +// expression-containing identifiers do not trigger V-MAF-011 warnings. +func TestWarnUnrecognizedModelParams_ExpressionSkipped(t *testing.T) { + tests := []struct { + name string + identifiers []string + wantWarning bool + }{ + { + name: "whole-string expression emits no warning", + identifiers: []string{"${{ inputs.model }}"}, + wantWarning: false, + }, + { + name: "partial expression emits no warning", + identifiers: []string{"${{ inputs.model }}?unknownparam=value"}, + wantWarning: false, + }, + { + name: "provider-scoped expression emits no warning", + identifiers: []string{"copilot/${{ inputs.model }}?unknownparam=value"}, + wantWarning: false, + }, + { + name: "non-expression with unknown param emits warning", + identifiers: []string{"opus?unknownparam=value"}, + wantWarning: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + compiler.warnUnrecognizedModelParams(tt.identifiers, "/fake/path.md") + if tt.wantWarning { + assert.Positive(t, compiler.GetWarningCount(), + "expected V-MAF-011 warning but warning counter is zero") + } else { + assert.Zero(t, compiler.GetWarningCount(), + "expected no V-MAF-011 warning for expression-containing identifier") + } + }) + } +} + +// ─── Engine.model table-driven expression tests ─────────────────────────────── + +// TestValidateModelAliasMap_EngineModelExpressionForms covers every expression +// form allowed in engine.model (resolved at runtime, exempt from compile-time checks). +func TestValidateModelAliasMap_EngineModelExpressionForms(t *testing.T) { + tests := []struct { + name string + engineModel string + wantErr bool + }{ + // Whole/partial expressions — no "*" in literal parts — accepted. + {"whole-string expression", "${{ inputs.model }}", false}, + {"expression with effort param", "${{ inputs.model }}?effort=high", false}, + {"expression with temperature param", "${{ inputs.model }}?temperature=0.5", false}, + {"expression with multiple params", "${{ inputs.model }}?effort=low&temperature=0.3", false}, + {"expression with unknown param — no error (no static parse)", "${{ inputs.model }}?unknownparam=x", false}, + {"compact expression syntax", "${{inputs.model}}", false}, + {"provider-scoped expression", "copilot/${{ inputs.model }}", false}, + // Non-expression values are still validated normally. + {"valid literal is accepted", "copilot/gpt-5", false}, + // V-MAF-004: glob in literal part — always rejected regardless of expressions. + {"glob in engine.model is still rejected (V-MAF-004)", "copilot/*gpt*", true}, + // V-MAF-004 must fire even when "*" appears outside an expression. + {"expression + trailing glob rejected (V-MAF-004)", "${{ inputs.model }}*", true}, + {"literal glob before expression rejected (V-MAF-004)", "copilot/*${{ inputs.model }}", true}, + {"expression between globs rejected (V-MAF-004)", "*${{ inputs.model }}*", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + err := compiler.validateModelAliasMap( + BuiltinModelAliases(), + nil, + tt.engineModel, + "/fake/path/workflow.md", + ) + if tt.wantErr { + require.Error(t, err, "engine.model=%q should be rejected", tt.engineModel) + assert.Contains(t, err.Error(), "V-MAF-004", "engine.model=%q error should reference V-MAF-004", tt.engineModel) + assert.Contains(t, err.Error(), tt.engineModel, "engine.model error should quote the offending value") + } else { + assert.NoError(t, err, "engine.model=%q should be accepted", tt.engineModel) + } + }) + } +}