Add model inventory validation to compile #57958
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "context" | ||
| "path" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| "github.com/github/gh-aw/pkg/modelsdev" | ||
| "github.com/github/gh-aw/pkg/workflow" | ||
| ) | ||
|
|
||
| type activeModelInventory struct { | ||
| models []string | ||
| aliases map[string]struct{} | ||
| } | ||
|
|
||
| func buildActiveModelInventory(report modelsReport) *activeModelInventory { | ||
| if len(report.Observed) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| models := make(map[string]struct{}) | ||
| for _, row := range report.Observed { | ||
| model := modelsdev.NormalizeComparableModelID(row.Model) | ||
| if model == "" { | ||
| continue | ||
| } | ||
| models[model] = struct{}{} | ||
| if row.Provider != "" { | ||
| provider := modelsdev.NormalizeProvider(row.Provider) | ||
| models[modelsdev.NormalizeComparableModelID(path.Join(provider, row.Model))] = struct{}{} | ||
| if provider == "github-copilot" { | ||
| for _, alias := range []string{"copilot", "github", "github_models"} { | ||
| models[modelsdev.NormalizeComparableModelID(path.Join(alias, row.Model))] = struct{}{} | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| aliases := make(map[string]struct{}, len(report.Aliases)) | ||
| for _, row := range report.Aliases { | ||
| aliases[modelsdev.NormalizeComparableModelID(row.Alias)] = struct{}{} | ||
| } | ||
|
|
||
| activeModels := make([]string, 0, len(models)) | ||
| for model := range models { | ||
| activeModels = append(activeModels, model) | ||
| } | ||
| slices.Sort(activeModels) | ||
| return &activeModelInventory{models: activeModels, aliases: aliases} | ||
| } | ||
|
|
||
| func (i *activeModelInventory) contains(candidate string, workflowAliases map[string][]string) bool { | ||
| base, _, _ := strings.Cut(strings.TrimSpace(candidate), "?") | ||
| if base == "" || strings.Contains(base, "${{") { | ||
| return true | ||
| } | ||
|
|
||
| normalized := modelsdev.NormalizeComparableModelID(base) | ||
| if _, ok := i.aliases[normalized]; ok { | ||
| return true | ||
| } | ||
| for alias := range workflowAliases { | ||
| if modelsdev.NormalizeComparableModelID(alias) == normalized { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| for _, model := range i.models { | ||
| if matched, err := path.Match(normalized, model); err == nil && matched { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func findUnknownConfiguredModels(data *workflow.WorkflowData, inventory *activeModelInventory) []ValidationIssue { | ||
| if data == nil || inventory == nil { | ||
| return nil | ||
| } | ||
|
|
||
| candidates := make(map[string][]string) | ||
| add := func(field string, values []string) { | ||
| for _, value := range values { | ||
| if !inventory.contains(value, data.ModelMappings) { | ||
| candidates[value] = append(candidates[value], field) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| add("models.allowed", data.ModelPolicyAllowed) | ||
| add("models.blocked", data.ModelPolicyBlocked) | ||
|
|
||
| if engine, ok := data.RawFrontmatter["engine"].(map[string]any); ok { | ||
| if models, ok := engine["models"].(map[string]any); ok { | ||
| if value, ok := models["default"].(string); ok { | ||
| add("engine.models.default", []string{value}) | ||
| } | ||
| add("engine.models.supported", stringSlice(models["supported"])) | ||
| } | ||
|
Comment on lines
+95
to
+101
|
||
| } | ||
|
Contributor
There was a problem hiding this comment.
@copilot please address this. |
||
|
|
||
| values := make([]string, 0, len(candidates)) | ||
| for value := range candidates { | ||
| values = append(values, value) | ||
| } | ||
| slices.Sort(values) | ||
|
|
||
| warnings := make([]ValidationIssue, 0, len(values)) | ||
| for _, value := range values { | ||
| fields := candidates[value] | ||
| slices.Sort(fields) | ||
| warnings = append(warnings, ValidationIssue{ | ||
| Type: "unknown_model", | ||
| Message: "Model " + value + " referenced by " + strings.Join(fields, ", ") + " was not found in the active model inventory", | ||
| }) | ||
| } | ||
| return warnings | ||
| } | ||
|
|
||
| // PrepareCompileModelValidation builds the active model inventory used by compile --models. | ||
| func PrepareCompileModelValidation(ctx context.Context, config *CompileConfig) { | ||
| if !config.Models { | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 Suggested improvementSurface this state explicitly, e.g. emit a one-line stderr notice ("--models: no observed model data available, skipping validation") from @copilot please address this. |
||
| return | ||
| } | ||
| report := buildModelsReport(ctx, modelsReportOptions{ | ||
| logsDir: defaultLogsOutputDir, | ||
|
Contributor
There was a problem hiding this comment. [/tdd] 💡 Suggested testfunc TestPrepareCompileModelValidationRespectsLogsDirOverride(t *testing.T) {
// arrange: config with a custom logs dir containing fixture observed-model data
// act: PrepareCompileModelValidation(ctx, &config)
// assert: config.activeModels reflects the fixture data, not the default dir
}If the omission is intentional (e.g. @copilot please address this. |
||
| refreshObserved: true, | ||
| refreshCount: defaultModelsRefreshCount, | ||
| }) | ||
| config.activeModels = buildActiveModelInventory(report) | ||
|
Comment on lines
+127
to
+132
|
||
| } | ||
|
|
||
| func unknownConfiguredModelMessages(data *workflow.WorkflowData, inventory *activeModelInventory) []string { | ||
| issues := findUnknownConfiguredModels(data, inventory) | ||
| messages := make([]string, 0, len(issues)) | ||
| for _, issue := range issues { | ||
| messages = append(messages, issue.Message) | ||
| } | ||
| return messages | ||
|
Comment on lines
+135
to
+141
|
||
| } | ||
|
|
||
| func stringSlice(value any) []string { | ||
| raw, ok := value.([]any) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| result := make([]string, 0, len(raw)) | ||
| for _, entry := range raw { | ||
| if text, ok := entry.(string); ok { | ||
| result = append(result, text) | ||
| } | ||
| } | ||
| return result | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| //go:build !integration | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/github/gh-aw/pkg/workflow" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestBuildActiveModelInventoryDoesNothingWithoutObservedData(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| assert.Nil(t, buildActiveModelInventory(modelsReport{ | ||
| Catalog: []modelCatalogRow{{Provider: "openai", Model: "gpt-5.4"}}, | ||
| Aliases: []modelAliasRow{{Alias: "large"}}, | ||
| })) | ||
| } | ||
|
|
||
| func TestFindUnknownConfiguredModels(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| inventory := buildActiveModelInventory(modelsReport{ | ||
| Aliases: []modelAliasRow{{Alias: "large"}}, | ||
| Observed: []observedModelRow{ | ||
| {Provider: "github-copilot", Model: "gpt-5.4"}, | ||
| {Provider: "anthropic", Model: "claude-sonnet-4.6"}, | ||
| }, | ||
| }) | ||
| require.NotNil(t, inventory) | ||
|
|
||
| data := &workflow.WorkflowData{ | ||
| ModelMappings: map[string][]string{"custom-alias": {"gpt-5.4"}}, | ||
| ModelPolicyAllowed: []string{ | ||
| "gpt-5.4", | ||
| "copilot/gpt-5.*", | ||
| "large", | ||
| "custom-alias", | ||
| "missing-policy-model", | ||
| }, | ||
| ModelPolicyBlocked: []string{"anthropic/claude-sonnet-4.6"}, | ||
| RawFrontmatter: map[string]any{ | ||
| "engine": map[string]any{ | ||
| "models": map[string]any{ | ||
| "default": "large", | ||
| "supported": []any{"gpt-5.4?effort=high", "${{ inputs.model }}", "missing-engine-model"}, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| warnings := findUnknownConfiguredModels(data, inventory) | ||
| require.Len(t, warnings, 2) | ||
| assert.Equal(t, "unknown_model", warnings[0].Type) | ||
| assert.Contains(t, warnings[0].Message, "missing-engine-model") | ||
| assert.Contains(t, warnings[0].Message, "engine.models.supported") | ||
| assert.Contains(t, warnings[1].Message, "missing-policy-model") | ||
| assert.Contains(t, warnings[1].Message, "models.allowed") | ||
| } | ||
|
|
||
| func TestFindUnknownConfiguredModelsSkipsMissingInventory(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| data := &workflow.WorkflowData{ | ||
| ModelPolicyAllowed: []string{"unknown"}, | ||
| } | ||
| assert.Empty(t, findUnknownConfiguredModels(data, nil)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/cli/compile_model_validation.go:13: yagni: activeModelInventory wrapper with one consumer and a bespoke contains method. Replace it with a plain normalized map and inline the matching logic in findUnknownConfiguredModels.