From 6db1bbe39f2c0dfa85ada47f020097073c45e9cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 3 May 2026 19:49:23 +0000 Subject: [PATCH 1/4] feat: allow GitHub Actions expressions in model alias parser Update validateModelAliasMap and helpers to use containsExpression() instead of isExpression() so partial expressions like \"${{ inputs.model }}?effort=high\" and \"copilot/${{ inputs.model }}\" are also exempt from compile-time syntax validation. Add four new tests covering the new partial-expression cases." Agent-Logs-Url: https://github.com/github/gh-aw/sessions/20ec3dd8-3bdc-4c19-8c95-632689808d51 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/model_alias_validation.go | 11 ++++-- pkg/workflow/model_alias_validation_test.go | 41 +++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/pkg/workflow/model_alias_validation.go b/pkg/workflow/model_alias_validation.go index 15da312457..17f8b070e6 100644 --- a/pkg/workflow/model_alias_validation.go +++ b/pkg/workflow/model_alias_validation.go @@ -53,8 +53,9 @@ func (c *Compiler) validateModelAliasMap( 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) { + // GitHub Actions expressions (${{ … }}) are exempt from syntax checks, + // including partial expressions such as "${{ inputs.model }}?effort=high". + if engineModel != "" && !containsExpression(engineModel) { if err := validateEngineModelNotGlob(engineModel, markdownPath); err != nil { return err } @@ -136,7 +137,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 +162,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..30fe453507 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( From c402d1c4672adbf7dc04f80ce467ff2547757343 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 3 May 2026 19:50:06 +0000 Subject: [PATCH 2/4] style: use ... instead of ellipsis in model_alias_validation comment Agent-Logs-Url: https://github.com/github/gh-aw/sessions/20ec3dd8-3bdc-4c19-8c95-632689808d51 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/model_alias_validation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/workflow/model_alias_validation.go b/pkg/workflow/model_alias_validation.go index 17f8b070e6..bb8187f40b 100644 --- a/pkg/workflow/model_alias_validation.go +++ b/pkg/workflow/model_alias_validation.go @@ -53,7 +53,7 @@ func (c *Compiler) validateModelAliasMap( len(mergedAliasMap), len(frontmatterModels), engineModel) // V-MAF-004: engine.model MUST NOT be a glob pattern. - // GitHub Actions expressions (${{ … }}) are exempt from syntax checks, + // GitHub Actions expressions (${{ ... }}) are exempt from syntax checks, // including partial expressions such as "${{ inputs.model }}?effort=high". if engineModel != "" && !containsExpression(engineModel) { if err := validateEngineModelNotGlob(engineModel, markdownPath); err != nil { From de825aaeb8796b4efd5d6666fefa61d2455b61b1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 3 May 2026 19:57:52 +0000 Subject: [PATCH 3/4] test: add table-driven expression tests for model alias validator Add comprehensive table-driven tests covering all expression forms: - TestValidateModelIdentifierStrings_ExpressionForms: tests validateModelIdentifierStrings directly with whole/partial/embedded expressions, mixed valid/invalid lists, and compact syntax - TestWarnUnrecognizedModelParams_ExpressionSkipped: verifies no V-MAF-011 warnings emitted for expression-containing identifiers - TestValidateModelAliasMap_EngineModelExpressionForms: covers all engine.model expression variants including glob rejection guard still works Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b030920f-e288-4006-a38d-c3da732c1146 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/model_alias_validation_test.go | 171 ++++++++++++++++++++ 1 file changed, 171 insertions(+) diff --git a/pkg/workflow/model_alias_validation_test.go b/pkg/workflow/model_alias_validation_test.go index 30fe453507..0eaf7df461 100644 --- a/pkg/workflow/model_alias_validation_test.go +++ b/pkg/workflow/model_alias_validation_test.go @@ -514,3 +514,174 @@ 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-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}, + // Non-expression values are still validated normally. + {"valid literal is accepted", "copilot/gpt-5", false}, + {"glob in engine.model is still rejected (V-MAF-004)", "copilot/*gpt*", 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 { + assert.Error(t, err, "engine.model=%q should be rejected", tt.engineModel) + } else { + assert.NoError(t, err, "engine.model=%q should be accepted", tt.engineModel) + } + }) + } +} From 1b24e5e377fcf0b2cb1df99ad752375309dbe855 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 3 May 2026 20:14:17 +0000 Subject: [PATCH 4/4] fix: enforce V-MAF-004 glob check on literal parts even when expression is present The engine.model guard now always enforces the glob restriction on the literal text (after stripping ${{ ... }} segments), while still skipping syntax/param validation for runtime-resolved expressions. Previously, containsExpression() in the outer `if` caused the entire validation block to be skipped, allowing values like "${{ inputs.m }}*" or "copilot/*${{ inputs.model }}" to pass despite having a literal glob. Changes: - Inline the V-MAF-004 glob check using ExpressionPattern.ReplaceAllString to strip expressions before checking for "*", so a "*" inside an expression body is not falsely flagged - Move syntax/param validation inside a nested containsExpression() guard - Remove now-unused validateEngineModelNotGlob helper - Rename literalParts -> literalText for clarity - Extend TestValidateModelAliasMap_EngineModelExpressionForms with 3 new subtests covering expression+trailing-glob, literal-glob-before-expression, and expression-between-globs; also assert the original value is quoted in the error message Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2beddf03-18a0-4e6a-a2a8-0c1f01a29c98 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/model_alias_validation.go | 49 ++++++++++----------- pkg/workflow/model_alias_validation_test.go | 11 ++++- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/pkg/workflow/model_alias_validation.go b/pkg/workflow/model_alias_validation.go index bb8187f40b..4b16e71f5c 100644 --- a/pkg/workflow/model_alias_validation.go +++ b/pkg/workflow/model_alias_validation.go @@ -52,19 +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, - // including partial expressions such as "${{ inputs.model }}?effort=high". - if engineModel != "" && !containsExpression(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). @@ -92,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). diff --git a/pkg/workflow/model_alias_validation_test.go b/pkg/workflow/model_alias_validation_test.go index 0eaf7df461..26d83d4e41 100644 --- a/pkg/workflow/model_alias_validation_test.go +++ b/pkg/workflow/model_alias_validation_test.go @@ -657,15 +657,22 @@ func TestValidateModelAliasMap_EngineModelExpressionForms(t *testing.T) { 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 { @@ -678,7 +685,9 @@ func TestValidateModelAliasMap_EngineModelExpressionForms(t *testing.T) { "/fake/path/workflow.md", ) if tt.wantErr { - assert.Error(t, err, "engine.model=%q should be rejected", tt.engineModel) + 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) }