feat: add model alias validation enforcing the Model Alias Format spec#29995
Merged
feat: add model alias validation enforcing the Model Alias Format spec#29995
Conversation
Implements compile-time enforcement of the Model Alias Format Specification
(docs/src/content/docs/reference/model-alias-specification.md):
- V-MAF-001/006: Reject model identifiers not conforming to the ABNF grammar;
error message names the offending character and segment type.
- V-MAF-002: Reject effort parameter values outside {low, medium, high}.
- V-MAF-003: Reject temperature parameter values outside [0.0, 2.0].
- V-MAF-004: Reject glob patterns in engine.model.
- V-MAF-005: Reject alias keys containing '/', '?', or '&'.
- V-MAF-010: Detect and report circular alias references (DFS) at compile time;
abort compilation on any detected cycle.
- V-MAF-011: Emit a warning for unrecognised parameter keys.
New files:
- pkg/workflow/model_identifier.go — ABNF grammar parser
- pkg/workflow/model_alias_validation.go — compile-time validation
- pkg/workflow/model_alias_validation_test.go — 35 tests covering all rules
Wired into ParseWorkflowFile in compiler_orchestrator_workflow.go.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2f4eea04-b395-4344-947b-11e22a690c20
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…helpers and struct-based DFS Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2f4eea04-b395-4344-947b-11e22a690c20 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
pelikhan
May 3, 2026 19:14
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Adds compile-time validation for workflow models: frontmatter entries and engine.model values to enforce the Model Alias Format (MAF) spec, including a dedicated model-identifier parser and cycle detection across merged alias maps.
Changes:
- Introduces
ParseModelIdentifierto parse/validate MAF identifiers (bare names, provider-scoped, globs, and query params). - Adds compile-time validation for alias keys, identifier syntax/params, engine.model glob prohibition, and DFS-based cycle detection.
- Adds a focused unit test suite covering parsing and validation helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/model_identifier.go | New parser + helpers for MAF identifier syntax and known-parameter validation. |
| pkg/workflow/model_alias_validation.go | New compile-time validation entrypoint (engine.model checks, alias key checks, cycle detection, warnings). |
| pkg/workflow/model_alias_validation_test.go | New tests for parsing, parameter validation, alias key validation, and cycle detection behavior. |
| pkg/workflow/compiler_orchestrator_workflow.go | Hooks model-alias validation into ParseWorkflowFile after model mappings are built. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
Comment on lines
+362
to
+370
| func ValidateTemperatureParam(value string) error { | ||
| f, err := strconv.ParseFloat(value, 64) | ||
| if err != nil { | ||
| return fmt.Errorf("model parameter 'temperature': value %q cannot be parsed as a decimal float (V-MAF-003)", value) | ||
| } | ||
| if f < 0.0 || f > 2.0 { | ||
| return fmt.Errorf("model parameter 'temperature': value %q is out of range; must be in [0.0, 2.0] (V-MAF-003)", value) | ||
| } | ||
| return nil |
Comment on lines
+295
to
+296
| // Returns an error if any key or value violates the grammar or if known parameters | ||
| // have invalid values. |
| // 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) | ||
| } |
Collaborator
|
@copilot review all comments |
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 95/100✅ Excellent test quality
Test Classification DetailsView all 39 test classifications
Flagged Tests — Minor Issues Only
|
| Component | Weight | Result |
|---|---|---|
| Behavioral coverage | 40 pts | 39/39 = 100% → 40 pts |
| Error/edge case coverage | 30 pts | 35/39 = 90% → 27 pts |
| Low duplication | 20 pts | 0 duplicate clusters → 20 pts |
| Proportional growth | 10 pts | 455 test lines / 702 prod lines = 0.65:1 → 10 pts |
| Minor guideline deduction | — | 3 assertions missing messages → −2 pts |
| Total | 100 | 95/100 |
Language Support
Tests analyzed:
- 🐹 Go (
*_test.go): 39 tests — unit (//go:build !integration) - 🟨 JavaScript (
*.test.cjs,*.test.js): 0 tests changed
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). The test suite is comprehensive and spec-aligned — it references spec rule IDs (V-MAF-001 through V-MAF-010), uses table-driven tests for boundary validation, and covers all major error paths. The three missing assertion messages (lines 108–109, 115) are minor and do not affect correctness.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §25288249499
🧪 Test quality analysis by Test Quality Sentinel · ● 635.6K · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Test Quality Sentinel: 95/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). 39 behavioral tests with strong spec alignment (V-MAF rule IDs), table-driven coverage of error/boundary cases, and no mock libraries. Three assertions are missing descriptive messages (lines 108–109, 115) — minor cleanup recommended but does not block merge.
…lidation Generated by the Design Decision Gate workflow as a draft for PR #29995.
Contributor
|
Commit pushed:
|
Contributor
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,178 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Draft ADR SummaryThe draft captures the following architectural decision:
Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §25288249489
|
…-MAF-011, fuzz tests - ValidateTemperatureParam: reject NaN and infinite values (math.IsNaN/IsInf check) - parseQueryString doc comment: corrects claim about known-param validation (happens in ValidateKnownParams, not here) - validateModelAliasMap: apply V-MAF-011 warning to engine.model unrecognised params - Add NaN/Inf test cases to TestValidateTemperatureParam - Add TestValidateModelAliasMap_EngineModelUnknownParamEmitsWarning - Add model_identifier_fuzz_test.go with FuzzParseModelIdentifier and FuzzValidateTemperatureParam Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c6fe1e8d-74bb-48d3-968f-c0e4c9dd68a9 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Addressed all three review comments and added fuzz tests in commit e5ea993: Review comments fixed:
Tests added:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR reviews the
models:configuration in frontmatter andengine.modelhandling, and adds compile-time enforcement of the Model Alias Format Specification.Changes
New files
pkg/workflow/model_identifier.go— ABNF grammar parser for model identifier strings (Section 4 of spec). ImplementsParseModelIdentifierwhich parses bare names, provider-scoped names, glob patterns, and URL-style query parameters.ValidateTemperatureParamexplicitly rejects NaN and infinite values viamath.IsNaN/math.IsInf.pkg/workflow/model_alias_validation.go— compile-time validation implementing the required validation rules from Section 11 of the spec. V-MAF-011 unrecognised-parameter warnings are applied to bothmodels:alias entries andengine.model.pkg/workflow/model_alias_validation_test.go— 38 tests covering all validation rules including T-MAF-001 through T-MAF-044.pkg/workflow/model_identifier_fuzz_test.go— fuzz tests for the MAF parser (FuzzParseModelIdentifier) and temperature parameter validation (FuzzValidateTemperatureParam).Modified files
pkg/workflow/compiler_orchestrator_workflow.go— wiresvalidateModelAliasMapintoParseWorkflowFile, called immediately afterModelMappingsis populated.Validation Rules Implemented
effortvalues outside{low, medium, high}temperaturevalues outside[0.0, 2.0]; NaN and Inf are explicitly rejectedengine.model/,?, or&models:alias entries andengine.modelNote: V-MAF-012 (effort warning for non-reasoning models) and V-MAF-013 (runtime cycle guard) are runtime/agent-side rules not enforced at compile time.
Testing
validateModelAliasMapfor all enforcement paths, including V-MAF-011 warning onengine.modelValidateTemperatureParam${{ ... }}) are exempted from syntax validationFuzzParseModelIdentifier: no-panic and invariant checks across arbitrary inputsFuzzValidateTemperatureParam: verifies NaN/Inf rejection and range bounds under fuzzing