Add gh aw models for catalog pricing, alias resolution, and observed automation models - #55148
Conversation
…overy Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw models command for catalog pricing and observed modelsgh aw models for catalog pricing, alias resolution, and observed automation models
There was a problem hiding this comment.
Pull request overview
Adds gh aw models for catalog pricing, alias resolution, and automation-observed model discovery.
Changes:
- Adds human-readable and JSON model reports.
- Refreshes and aggregates model observations from workflow artifacts.
- Registers the command in the analysis group.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/models_command.go |
Implements model reporting and artifact discovery. |
pkg/cli/models_command_test.go |
Tests catalog sorting, observation aggregation, and aliases. |
cmd/gh-aw/main.go |
Registers and groups the new command. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
| if refreshCount <= 0 { | ||
| refreshCount = 20 | ||
| } | ||
| return DownloadWorkflowLogs(ctx, LogsDownloadOptions{ |
| func modelExistsInCatalog(index map[string]struct{}, fullID, model string) bool { | ||
| _, hasFullID := index[modelsdev.NormalizeComparableModelID(fullID)] | ||
| _, hasModel := index[modelsdev.NormalizeComparableModelID(model)] | ||
| return hasFullID || hasModel | ||
| } |
| warnings = appendObservedCollectionWarning(warnings, collectObservedFromSummary(logsDir, addObserved), "summary.json") | ||
| warnings = appendObservedCollectionWarning(warnings, collectObservedFromRunDirs(logsDir, addObserved), "run directories") | ||
| warnings = appendObservedCollectionWarning(warnings, collectObservedFromAWFReflect(logsDir, addObserved), "awf-reflect artifacts") |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
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.
Request changes
This adds useful surface area, but the current implementation ships broken output semantics and misleading observed-model data.
Blocking themes
- the default refresh path writes
logsoutput intomodels, which can corrupt both human-readable output and--json - observed request counts are double counted from
summary.jsonplus the samerun-*directories - catalog membership is overstated because provider-qualified observations fall back to providerless model-name matches
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 17.6 AIC · ⌖ 6.97 AIC · ⊞ 7K
Comment /review to run again
| } | ||
|
|
||
| func refreshObservedArtifacts(ctx context.Context, logsDir string, refreshCount int, repoOverride string) error { | ||
| if refreshCount <= 0 { |
There was a problem hiding this comment.
This command refreshes observed artifacts before printing its own report, but DownloadWorkflowLogs emits its compact run summary to stdout by default. As written, gh aw models will prepend unrelated logs output ahead of the catalog/alias sections, and --json mode can no longer guarantee clean JSON when the refresh finds runs.
💡 Keep the refresh side effect from corrupting command output
Either call the downloader in a silent mode (for example by adding an option that skips rendering) or route the refresh through an internal API that only materializes artifacts. Right now the implementation uses the same public downloader path that is designed to print human-readable summaries:
return DownloadWorkflowLogs(ctx, LogsDownloadOptions{
Count: refreshCount,
OutputDir: logsDir,
RepoOverride: repoOverride,
ArtifactSets: []string{string(ArtifactSetFirewall), string(ArtifactSetUsage)},
})That breaks both the plain-text UX and any scripting that expects gh aw models --json to return only a single JSON document.
| } | ||
|
|
||
| warnings = appendObservedCollectionWarning(warnings, collectObservedFromSummary(logsDir, addObserved), "summary.json") | ||
| warnings = appendObservedCollectionWarning(warnings, collectObservedFromRunDirs(logsDir, addObserved), "run directories") |
There was a problem hiding this comment.
The observed-model counter double counts the same requests whenever summary.json and the corresponding run-* directories are both present, which is the normal logs layout produced by the downloader. That makes the Seen column systematically inflated and turns the command's main ranking signal into fiction.
💡 De-duplicate summary-vs-run usage before aggregating counts
collectObservedModelRows currently adds request totals from both sources independently:
warnings = appendObservedCollectionWarning(warnings, collectObservedFromSummary(logsDir, addObserved), "summary.json")
warnings = appendObservedCollectionWarning(warnings, collectObservedFromRunDirs(logsDir, addObserved), "run directories")But summary.json is already derived from those run directories, so a refreshed logs directory usually reports each run twice. Prefer one authoritative source for request counts, or key counts by run ID and merge sources without adding the same run twice. The current test even encodes the bad behavior by asserting >= 8 from a 7-request summary plus one reflect observation.
| } | ||
|
|
||
| func modelExistsInCatalog(index map[string]struct{}, fullID, model string) bool { | ||
| _, hasFullID := index[modelsdev.NormalizeComparableModelID(fullID)] |
There was a problem hiding this comment.
InCatalog falls back to a providerless model-name lookup even when the observed provider is known, so any provider that happens to expose the same slug as a catalog entry is reported as catalog-backed. That quietly mislabels unknown or unsupported providers as valid and undermines the point of this compatibility signal.
💡 Only use bare-model fallback when the provider is actually unknown
The current check indexes both full IDs and naked model names:
index[NormalizeComparableModelID(record.id)] = struct{}{}
index[NormalizeComparableModelID(record.model)] = struct{}{}and then accepts either match:
_, hasFullID := index[NormalizeComparableModelID(fullID)]
_, hasModel := index[NormalizeComparableModelID(model)]
return hasFullID || hasModelThat means other/gpt-5.4 becomes InCatalog=true just because some other provider ships gpt-5.4. Keep the bare-model fallback for missing/empty providers only; once a provider is known, require the provider-qualified ID to match.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /codebase-design, and /tdd — requesting changes on correctness issues.
📋 Key Themes & Highlights
Key Themes
- Occurrence double-counting:
summary.jsonandrun-*directories cover the same data; aggregating both inflates theOccurrencescount. - False-positive
InCatalog: bare-model fallback inmodelExistsInCatalogincorrectly marks models from unknown providers as catalog-present. - Stdout interleaving:
DownloadWorkflowLogswrites a compact status report to stdout before the models tables render (default behaviour since--refresh-observedistrue). - Alias deduplication gap:
inferAliasHintscan append the same alias multiple times if multiple wildcard patterns in its target list match. - Test coupling to a bug: the
gpt-5.4InCatalog: trueassertion in the test validates the false-positive path; fixing the catalog lookup will break the test.
Positive Highlights
- ✅ Clean command structure and well-named flag set.
- ✅ Graceful handling of missing
summary.jsonand non-existentlogsDir. - ✅ Warnings propagated to stderr rather than swallowed.
- ✅ Good use of
slices.SortFuncfor deterministic output.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.6 AIC · ⌖ 11.5 AIC · ⊞ 7.6K
Comment /matt to run again
| for _, modelID := range modelIDs { | ||
| if wildcardMatch(entry, modelID) { | ||
| matches = append(matches, alias) | ||
| break |
There was a problem hiding this comment.
[/diagnosing-bugs] Duplicate alias hints are possible: break exits the inner modelIDs loop but not the outer entries loop, so a single alias whose target list contains two wildcard patterns that both match can appear twice in AliasHints.
💡 Deduplicate with a seen-set
seen := make(map[string]struct{})
for alias, entries := range aliasMap {
for _, entry := range entries {
if !strings.Contains(entry, "/") {
continue
}
for _, modelID := range modelIDs {
if wildcardMatch(entry, modelID) {
if _, already := seen[alias]; !already {
seen[alias] = struct{}{}
matches = append(matches, alias)
}
break
}
}
}
}@copilot please address this.
|
|
||
| warnings = appendObservedCollectionWarning(warnings, collectObservedFromSummary(logsDir, addObserved), "summary.json") | ||
| warnings = appendObservedCollectionWarning(warnings, collectObservedFromRunDirs(logsDir, addObserved), "run directories") | ||
| warnings = appendObservedCollectionWarning(warnings, collectObservedFromAWFReflect(logsDir, addObserved), "awf-reflect artifacts") |
There was a problem hiding this comment.
[/diagnosing-bugs] Double-counting of occurrences when both summary.json and the matching run-* directory are present. collectObservedFromSummary adds per-run request counts from the aggregated file, then collectObservedFromRunDirs re-adds the same per-run counts from the individual directories that built that summary. A model seen in 5 runs of 7 requests each would be credited 70 occurrences instead of 35.
💡 Options
- Skip
collectObservedFromRunDirswhensummary.jsonexists and already covers the same runs. - Use
run-*as the only per-run source and rely onsummary.jsononly for fields unavailable in individual run dirs. - Count each source separately and note in the
Sourcesfield rather than summing them.
@copilot please address this.
| _, hasFullID := index[modelsdev.NormalizeComparableModelID(fullID)] | ||
| _, hasModel := index[modelsdev.NormalizeComparableModelID(model)] | ||
| return hasFullID || hasModel | ||
| } |
There was a problem hiding this comment.
[/codebase-design] modelExistsInCatalog indexes every catalog model by bare model name (without provider), so InCatalog can be a false positive for observed models that share a model name with a completely different provider. For example, if anthropic/claude-3 is in the catalog, an observed my-provider/claude-3 would incorrectly show InCatalog: true.
💡 Suggested fix
Prefer the full-ID check; fall back to bare-model only when the observed provider is empty or unknown:
func modelExistsInCatalog(index map[string]struct{}, fullID, model string, provider string) bool {
if _, ok := index[modelsdev.NormalizeComparableModelID(fullID)]; ok {
return true
}
// Only allow bare-model fallback when provider is unknown
if provider == "" {
_, ok := index[modelsdev.NormalizeComparableModelID(model)]
return ok
}
return false
}@copilot please address this.
| if refreshCount <= 0 { | ||
| refreshCount = 20 | ||
| } | ||
| return DownloadWorkflowLogs(ctx, LogsDownloadOptions{ |
There was a problem hiding this comment.
[/codebase-design] refreshObservedArtifacts calls DownloadWorkflowLogs, which renders a compact status report to stdout (see logs_orchestrator_render.go). This output is interleaved with — and appears before — the models table output, producing confusing UX when --refresh-observed is true (the default).
💡 Options
- Pass a no-op
io.Writerfor the render output when called from the models command. - Add a
Quiet boolfield toLogsDownloadOptionsthat suppresses the compact report. - Move the refresh call after the tables are rendered, or gate it behind an explicit
--refresh-observedflag default offalse.
@copilot please address this.
| } | ||
|
|
||
| func TestInferAliasHints(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
[/tdd] The test asserts gpt.InCatalog == true, but gpt-5.4 from provider copilot is only in the catalog as a bare model name match. This test is actually validating the false-positive path identified in the modelExistsInCatalog comment — if that bug is fixed, this assertion will break. The test should either explicitly use a model+provider pair that is genuinely in the catalog, or assert InCatalog == false for an unknown model to verify the negative path.
@copilot please address this.
There was a problem hiding this comment.
Ponytail review: over-engineering pass only.
net: -25 lines possible.
Generated by ✂️ Ponytail Reviewer for #55148 · auto · 25.3 AIC · ⌖ 6.94 AIC · ⊞ 7.3K
Comment /ponytail to run again
| } | ||
|
|
||
| func wildcardMatch(pattern, value string) bool { | ||
| matched, err := filepath.Match(pattern, value) |
There was a problem hiding this comment.
L405: native: wildcardMatch wraps filepath.Match and swallows its error. Call filepath.Match directly at the two call sites, 0 lines.
|
|
||
| func collectObservedFromRunDirs(logsDir string, addObserved func(provider, model, source string, occurrences int)) error { | ||
| entries, err := os.ReadDir(logsDir) | ||
| if err != nil { |
There was a problem hiding this comment.
L303-308: yagni: appendObservedCollectionWarning is a 6-line wrapper around a 2-line if err != nil { append(...) }, used only 3x. Inline it at each call site.
| return nil | ||
| } | ||
| var payload struct { | ||
| Endpoints []struct { |
There was a problem hiding this comment.
L310-381: shrink: collectObservedFromSummary and collectObservedFromRunDirs duplicate the same provider/requests-default-1 extraction from usage. Extract one helper, ~10 fewer lines.
There was a problem hiding this comment.
Review: gh aw models command
The implementation is well-structured and covers catalog pricing, alias resolution, and observed model discovery. The three existing inline comments flag the most important issues:
- stdout pollution (line 143) —
DownloadWorkflowLogsemits a compact report to stdout before themodelsoutput, breaking--jsonpiping and polluting structured output. - double-counting (line 223) —
summary.jsonand therun-*token-usage dirs overlap, inflatingOccurrencesfor models already aggregated in the summary. - catalog lookup false negatives (line 426) — the bare-model fallback in
modelExistsInCatalogcan produce false positives when a short model name matches unrelated catalog entries from a different provider.
No additional blocking issues found beyond those already commented. Once those three are resolved, this is ready to merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.6 AIC · ⌖ 9.02 AIC · ⊞ 6.2K
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. Test Quality Sentinel 🧪Overall Score: ✅ 90/100 — Excellent Summary3 new test functions added in Test BreakdownTest Classifications (3 new tests analyzed)
Test Details1.
|
|
@copilot Please take another pass on this PR. The failed check still needs attention: Design Decision Gate 🏗️ — https://github.com/github/gh-aw/actions/runs/32662863381/job/97254401123 Please refresh the branch, address the failing workflow, and run the
|
|
@copilot add integration test |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in the latest commit.
Local validation: |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added
They run under the |
This adds a dedicated
gh aw modelscommand to make model selection fast and reliable: it surfaces catalog pricing, alias resolution targets, and models actually seen in automation artifacts. It also attempts a lightweight artifact refresh so observed-model output includes recent AWF reflect data when available.Command surface
gh aw modelsto the CLI and registers it in the analysis command group.--json.--logs-dir--refresh-observed--refresh-count--repoCatalog + pricing view
input,output,cache_read,cache_write,reasoning).Alias intelligence
Observed-model intelligence from automation
summary.jsontoken usage (token_usage_summary.by_model)awf-reflect.jsonendpoint model listssummary,token-usage,awf-reflect), tracks occurrence counts, and marks catalog presence.Run: https://github.com/github/gh-aw/actions/runs/32664768328> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.75 AIC · ⌖ 9.32 AIC · ⊞ 9.5K · ◷