feat: allow GitHub Actions expressions in models alias parser

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)

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)
+			}
+		})
+	}
+}