feat: allow GitHub Actions expressions in models alias parser#30000
feat: allow GitHub Actions expressions in models alias parser#30000
Conversation
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>
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>
|
@copilot add tests |
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>
Added three new table-driven test functions in commit
|
There was a problem hiding this comment.
Pull request overview
Updates model alias compile-time validation to treat strings containing GitHub Actions expressions (${{ ... }}) as runtime-resolved values, so partial/embedded expressions no longer fail model identifier parsing.
Changes:
- Replace whole-string expression detection (
isExpression) with substring detection (containsExpression) in model alias validation/warning paths. - Expand unit tests to cover partial and embedded expression forms in
engine.model, alias entries, identifier validation, and unknown-param warnings.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/model_alias_validation.go | Skips static parsing/validation when an identifier contains a complete ${{ ... }} expression. |
| pkg/workflow/model_alias_validation_test.go | Adds test coverage for expression-containing identifiers across validation and warning logic. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| // 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 { |
|
| Location | ADR-described behavior | PR behavior |
|---|---|---|
| Normative req 4 | Exempt strings matching the GHA expression pattern | Exempt strings containing a GHA expression |
| Neutral note | Uses isExpression helper |
Uses containsExpression helper |
3. Alignment Assessment: Partially Aligned
The change is consistent with the ADR's core rationale ("resolved at runtime"), and the ADR uses SHOULD rather than MUST for this requirement, giving implementations latitude. However, the normative text and neutral note describe the narrower whole-string match, so the ADR's written specification no longer accurately reflects the code after this PR.
What to Do Next
- Link the ADR in the PR body (see template above).
- Update ADR-29995 to reflect the extended behavior:
- Change normative requirement 4 to say "strings containing a GitHub Actions expression pattern" (or equivalent).
- Update the Neutral note from
isExpressiontocontainsExpressionand explain the distinction between whole-string and substring matching.
- Commit the updated ADR to
docs/adr/29995-compile-time-model-alias-format-validation.mdon this branch.
Once the ADR is linked in the PR body and the divergences are resolved, this gate will re-run.
📋 Michael Nygard ADR Format Reference
An ADR must contain these four sections to be considered complete:
- Context — What is the problem? What forces are at play?
- Decision — What did you decide? Why?
- Alternatives Considered — What else could have been done?
- Consequences — What are the trade-offs (positive and negative)?
All ADRs are stored in docs/adr/ as Markdown files.
🔒 This PR should not merge until the ADR is linked and the divergences are resolved.
References: §25289292363
🏗️ ADR gate enforced by Design Decision Gate 🏗️ · ● 117.5K · ◷
|
@copilot review all comments |
|
test
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…on 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>
Addressed in commit Fix in New test cases in
|
Summary
Updates the model alias parser to allow GitHub Actions expressions (
${{ ... }}) that are resolved at runtime, not just whole-string expressions, while preserving all compile-time static checks on literal portions.Problem
Previously, only whole-string expressions (where the entire value is a
${{ ... }}expression) were exempted from compile-time syntax validation. Partial expressions like:${{ inputs.model }}?effort=high— expression with query paramscopilot/${{ inputs.model }}— expression embedded in a provider-scoped identifierwould fail validation because
isExpression()returnsfalsefor them, causingParseModelIdentifierto error on the$character.Additionally, the original fix using
containsExpression()as the outer guard for theengine.modelblock inadvertently bypassed the V-MAF-004 glob check entirely — allowing values like"${{ inputs.model }}*"or"copilot/*${{ inputs.model }}"to pass despite having a literal*outside the expression.Changes
pkg/workflow/model_alias_validation.goisExpression()withcontainsExpression()in thevalidateModelIdentifierStringsandwarnUnrecognizedModelParamsexpression-skip guards (alias entry loops).engine.model(V-MAF-004): decouple the glob check from the expression skip. The glob restriction is now always enforced on the literal text (usingExpressionPattern.ReplaceAllStringto strip${{ ... }}segments before checking for*), so a*inside an expression body is not falsely flagged, but a*in the literal portions is always caught. Only syntax/param validation is skipped for expression-containing values.validateEngineModelNotGlobhelper (logic inlined with the new strip-then-check approach).pkg/workflow/model_alias_validation_test.goAdded comprehensive table-driven tests covering all expression forms:
TestValidateModelIdentifierStrings_ExpressionForms— testsvalidateModelIdentifierStringsdirectly with whole-string expressions, partial expressions (with query params), provider-scoped expressions, multiple expression entries, mixed expression+valid/invalid lists, and compact${{...}}syntax.TestWarnUnrecognizedModelParams_ExpressionSkipped— verifies that V-MAF-011 warnings are suppressed for all expression-containing identifiers while still firing for plain identifiers with unknown params.TestValidateModelAliasMap_EngineModelExpressionForms— covers allengine.modelexpression variants end-to-end, including V-MAF-004 rejection for literal globs even when an expression is also present (e.g."${{ inputs.model }}*","copilot/*${{ inputs.model }}"), and confirms valid expression forms are accepted.TestValidateModelAliasMap_PartialExpressionEngineModelSkipped,TestValidateModelAliasMap_ExpressionInAliasEntriesSkipped,TestValidateModelAliasMap_PartialExpressionInAliasEntriesSkipped.Testing
All existing tests pass. The new tests confirm the new behavior.
make fmt lint build recompile test-unitall pass.