Add model inventory validation to compile - #57958
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Imported engine models, inventory refresh failures, and structured JSON warnings are not handled correctly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds opt-in model inventory validation to gh aw compile.
Changes:
- Builds model inventory from catalog, aliases, and observations.
- Warns for unknown model references.
- Adds CLI wiring, tests, and documentation.
File summaries
| File | Description |
|---|---|
pkg/workflow/configured_model_validation.go |
Emits compiler model warnings. |
pkg/workflow/configured_model_validation_test.go |
Tests warning emission. |
pkg/workflow/compiler_types.go |
Stores the validator callback. |
pkg/workflow/compiler_orchestrator_workflow.go |
Runs model validation during parsing. |
pkg/workflow/compiler_mutators.go |
Exposes validator configuration. |
pkg/cli/models_command.go |
Extracts shared inventory reporting logic. |
pkg/cli/compile_model_validation.go |
Implements inventory matching and findings. |
pkg/cli/compile_model_validation_test.go |
Tests inventory validation behavior. |
pkg/cli/compile_config.go |
Adds model-validation configuration. |
pkg/cli/compile_compiler_setup.go |
Connects inventory validation to the compiler. |
docs/src/content/docs/setup/cli.md |
Documents --models. |
cmd/gh-aw/main.go |
Registers and prepares the flag. |
cmd/gh-aw/compile_flags_test.go |
Tests flag propagation. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
| 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"])) | ||
| } |
| report := buildModelsReport(ctx, modelsReportOptions{ | ||
| logsDir: defaultLogsOutputDir, | ||
| refreshObserved: true, | ||
| refreshCount: defaultModelsRefreshCount, | ||
| }) | ||
| config.activeModels = buildActiveModelInventory(report) |
| 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 |
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
@copilot Please review the current PR state, address the latest review feedback, refresh the branch if needed, and run the
|
ADR requiredThis PR meets the ADR gate because it adds implementation work in business-logic code ( I drafted an ADR at Evidence used
Draft ADR contentSee the drafted file content below for review and manual application: # ADR-57958: Add Model Inventory Validation to Compile
**Date**: 2026-09-02
**Status**: Draft
**Deciders**: pelikhan, adr-writer agent
---
### Context
This pull request adds a new `gh aw compile --models` option and threads that option through the CLI, compile configuration, compiler setup, workflow validation, tests, and CLI documentation. The PR description says compilation cannot currently identify model references that are absent from the active inventory, while `gh aw models` already has the catalog, alias, and observed-model collection path needed for that check. The diff shows an explicit requirement to warn without blocking compilation, to skip the check when no observed model data is available, and to validate `models.allowed`, `models.blocked`, and `engine.models`. Because this introduces a new compile-time validation behavior that depends on shared model inventory state and affects how workflow authors receive feedback, the decision should be recorded explicitly.
### Decision
We will add an opt-in `--models` compile flag that reuses the existing `gh aw models` inventory-building path to warn when configured models are absent from the active observed model inventory. The validation will run during workflow compilation, accept built-in aliases, workflow-defined aliases, wildcard patterns, parameterized model references, and runtime expressions, and it will emit deduplicated warnings instead of failing the compile. We will skip this validation entirely when no observed model data is available so compilation behavior remains non-blocking in environments without inventory data.
### Alternatives Considered
#### Alternative 1: Keep Model Inventory Checks Only in `gh aw models`
Continue limiting inventory inspection to the standalone `gh aw models` command and do not add any compile-time validation.
This was considered because it avoids adding another compile flag and keeps compilation focused on syntax and workflow validation only. It was not chosen because the PR evidence shows users need feedback at compile time when workflow configuration references models that are not present in the active inventory, and the existing command already provides reusable data sources for that validation.
#### Alternative 2: Make Unknown Models a Compile Error
Validate the same fields during compilation, but fail the command when an unknown configured model is found.
This was considered because it would enforce stricter correctness and could prevent invalid configurations from shipping. It was not chosen because the PR description and implementation explicitly require warnings without blocking compilation, reflecting that observed-model data may be incomplete or unavailable and that the inventory is advisory rather than an absolute source of truth.
### Consequences
#### Positive
- Workflow authors can detect unknown configured models earlier, at compile time, instead of needing a separate inventory inspection step.
- The implementation reuses the shared model report pipeline, reducing duplicated logic for catalog, alias, and observed-model handling.
- Validation remains practical in real repositories because aliases, wildcard patterns, parameterized model strings, and runtime expressions are treated as valid inputs.
#### Negative
- Compile now has an additional opt-in path that refreshes observed model artifacts, which adds implementation and maintenance complexity across CLI and compiler layers.
- Warning quality depends on the freshness and availability of observed model data, so results may be incomplete when logs are stale or missing.
- The validation logic must track multiple accepted reference forms and inventory aliases, increasing the risk of edge cases if model naming rules evolve.
#### Neutral
- The compiler now supports an external configured-model validator hook and increments warning counts for advisory inventory findings.
- The `models` report-building logic is factored into a reusable helper so both `gh aw models` and `gh aw compile --models` share the same data path.
- CLI documentation and tests now treat model inventory validation as an optional maintenance-style compile check rather than a required correctness gate.
---
*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*Next actionAdd this ADR file to the PR branch and update it during review if the decision scope changes.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No additional merge-blocking issues beyond the three model-validation problems already called out on this PR.
Why I’m not stacking more comments
I re-checked the changed lines around the new --models plumbing, the inventory builder, and the warning emission path. The high-signal risks remain the ones already raised: imported/built-in engine definitions are missed, refresh failures can silently validate against stale observations, and --json loses these warnings from per-workflow results. The extra sub-agent output was discarded because it targeted unrelated executable-bit changes that are not part of this diff.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 57.7 AIC · ⌖ 7.35 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
The new inventory wrapper is more structure than this feature needs; a plain normalized map plus direct checks in findUnknownConfiguredModels would keep the same behavior with less indirection. net: -20 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57958 · codex · mai10 · 6.47 AIC · ⌖ 0.383 AIC · ⊞ 13.5K
Comment /ponytail to run again
| "github.com/github/gh-aw/pkg/workflow" | ||
| ) | ||
|
|
||
| type activeModelInventory struct { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design. The feature is well-tested for its core matching logic (aliases, patterns, runtime expressions), opt-in, and non-blocking — good design fit. Existing bot comments already flag three solid issues in compile_model_validation.go (imported-engine-definitions gap at line 101, swallowed refresh warnings at line 132, and lost structured messages under --json at line 141); I added two more in the same area rather than duplicate those.
📋 Key Themes & Highlights
Key Themes
- Silent no-op risk: When
report.Observedis empty (bad--logs-dir, no artifacts, refresh failure),--modelsproduces zero warnings with zero indication that validation didn't actually run — indistinguishable from "everything is fine." - Divergence from
gh aw models:PrepareCompileModelValidationhardcodes the default logs dir and drops--repooverride, unlike the siblingmodelscommand; untested and undocumented. - These compound with the already-flagged issue that
buildModelsReport's warnings are discarded inPrepareCompileModelValidation— a single fix (surfacing those warnings) would resolve both the "why did I get no warnings" and "refresh failed silently" concerns together.
Positive Highlights
- ✅ Clean reuse of the existing models catalog/alias/observed-model pipeline via
buildModelsReport - ✅ Sensible handling of runtime expressions (
${{ ... }}) and glob patterns inactiveModelInventory.contains - ✅ Solid test coverage for the matching/dedup logic itself (
TestFindUnknownConfiguredModels)
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 67.5 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
|
|
||
| // PrepareCompileModelValidation builds the active model inventory used by compile --models. | ||
| func PrepareCompileModelValidation(ctx context.Context, config *CompileConfig) { | ||
| if !config.Models { |
There was a problem hiding this comment.
[/codebase-design] --models silently no-ops whenever report.Observed is empty (e.g. wrong --logs-dir, no local artifacts, or refresh failure) — activeModels stays nil, so SetConfiguredModelValidator is never wired up and the compiler prints nothing. A user cannot distinguish "no unknown models" from "validation never ran."
💡 Suggested improvement
Surface this state explicitly, e.g. emit a one-line stderr notice ("--models: no observed model data available, skipping validation") from PrepareCompileModelValidation when config.activeModels == nil, and add a test asserting that notice appears. This also ties into the existing review comment on this same function about refresh failures being silently swallowed — both stem from the same root cause: buildModelsReport warnings are discarded here instead of being surfaced to the compile user.
@copilot please address this.
| return | ||
| } | ||
| report := buildModelsReport(ctx, modelsReportOptions{ | ||
| logsDir: defaultLogsOutputDir, |
There was a problem hiding this comment.
[/tdd] PrepareCompileModelValidation hardcodes logsDir: defaultLogsOutputDir and omits repoOverride, unlike runModelsCommand which reads both from CLI flags. There's no test exercising a non-default --logs-dir or --repo through compile --models, so this divergence from gh aw models wasn't caught.
💡 Suggested test
func 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. compile --models is meant to always use the default logs dir), consider documenting why in a comment, since it's a subtle behavioral difference from gh aw models --logs-dir.
@copilot please address this.
There was a problem hiding this comment.
Reviewed the model inventory validation changes. This is a Go/CLI feature (not UI), so I applied a standard correctness review rather than Impeccable UI modes.
Found one high-signal gap: findUnknownConfiguredModels doesn't check the top-level model: frontmatter field or the singular engine.model override, which are the dominant ways workflows configure models in this repo (219+ files use model:), while the only engine-level field it does check (engine.models.default/supported) has no real usages. Left an inline comment with details.
Other aspects (inventory building, alias handling, flag wiring, docs) look solid and well-tested.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 145.3 AIC · ⌖ 14.3 AIC · ⊞ 8.3K
| } | ||
| add("engine.models.supported", stringSlice(models["supported"])) | ||
| } | ||
| } |
There was a problem hiding this comment.
findUnknownConfiguredModels never checks data.Model (the top-level model: frontmatter field) or engine.model (the singular string override consumed by resolveEngineModel in pkg/workflow/engine.go). In this repo, model: is used directly in 219+ workflow files (e.g. .github/workflows/ace-editor.md: model: openai/gpt-5.4), while engine.models.default/supported (the only engine-level field validated here) has zero real usages under .github/workflows/. As written, --models silently skips validating the model configuration for the overwhelming majority of existing workflows, which contradicts the PR's stated goal of warning when models.allowed, models.blocked, or configured engine models are absent from the active inventory. Please also validate data.Model and engine.model (falling back to top-level model:), and add a test case covering the common single-string model: frontmatter form.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
gh aw compilecannot currently identify model references absent from the active inventory. Add opt-in--modelsvalidation that warns without blocking compilation.Shared inventory
gh aw modelscatalog, alias, and observed-model collection path.Compile validation
models.allowed,models.blocked, andengine.models.--modelsto the CLI reference and describe its behavior.Run: https://github.com/github/gh-aw/actions/runs/33641605189