Fixing model name normalizer logic - #49001
Conversation
…ToIdentifier Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Updates model alias normalization to preserve short model names.
Changes:
- Returns normalized names shorter than six characters unchanged.
- Adds tests for model families, fallbacks, casing, and legends.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/model_aliases.cjs |
Adds short-name handling. |
actions/setup/js/model_aliases.test.cjs |
Adds normalization tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
| return normalized; | ||
| } | ||
|
|
||
| if (normalized.length < 6) { |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-tested. The length < 6 guard cleanly short-circuits short model names before the version-extraction pipeline can mangle them. Test coverage covers the relevant edge cases (short names, known families, fallback). No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.2 AIC · ⌖ 4.74 AIC · ⊞ 5.3K
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · auto · 20.2 AIC · ⌖ 4.58 AIC · ⊞ 7.8K
Comment /review to run again
| return normalized; | ||
| } | ||
|
|
||
| if (normalized.length < 6) { |
There was a problem hiding this comment.
This new short-circuit breaks alias consistency for known model families whose raw name is under 6 chars (e.g. gpt-4 → gpt-4, o1-mini → o10mini, gem-3 → gem-3), causing the same family to produce wildly different alias formats depending on string length alone.
💡 Inconsistent aliasing for short known-family names
Verified via direct execution:
reduceModelNameToIdentifier("gpt-4") -> "gpt-4" (bypasses normalization, raw string kept)
reduceModelNameToIdentifier("gpt-4o") -> "gpt40" (normalized as expected)
reduceModelNameToIdentifier("gem-3") -> "gem-3" (bypasses normalization)
The < 6 length check runs before the family-shortcut loop, so it intercepts recognized families (gpt, o-series, gem, etc.) whenever the raw name happens to be short, defeating the whole purpose of the normalizer for exactly the names most likely to appear in short form (gpt-4, gpt-5, o1-5). This also means the emoji-alias legend can show both gpt-4=gpt-4 and gpt40=gpt-4o for the same family, which is confusing to end users and inconsistent with the stated goal of a "deterministic compact model identifier."
Suggested fix: move the length check after the shortcuts loop (as a fallback-only guard), or make it conditional on none of the familyPatterns matching:
const isKnownFamily = shortcuts.some(({ familyPattern }) => familyPattern.test(normalized));
if (!isKnownFamily && normalized.length < 6) {
return normalized;
}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on one correctness issue and two test-quality gaps.
📋 Key Themes
Issues
- Broken test syntax (critical):
model_aliases.test.cjsuses ESMimportin a.cjsfile. Node treats.cjsas CommonJS;importwill throw aSyntaxErrorunless vitest has a special transform override for this extension. The tests cannot be trusted until this is resolved. - Missing boundary test: the
length < 6guard has no test at exactly 6 characters — the threshold itself is unverified. - Guard ordering undocumented: the interaction between the exact-name guard (
opus/sonnet/haiku) and the new length guard is non-obvious; a short comment would prevent future misreordering.
Positive Highlights
- ✅ Minimal, correctly placed fix
- ✅ Good edge-case coverage (null, undefined, empty string)
- ✅ Known model families (Claude, GPT, Gemini) tested end-to-end
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.9 AIC · ⌖ 5.14 AIC · ⊞ 7K
Comment /matt to run again
| // @ts-check | ||
|
|
||
| import { describe, expect, it } from "vitest"; | ||
| import { createRequire } from "module"; |
There was a problem hiding this comment.
[/tdd] ESM import statement in a .cjs file will fail at runtime.
Line 4 uses import { describe, expect, it } from "vitest" — import is not valid in CommonJS modules. This test file will throw a SyntaxError unless vitest's config applies a transform, but the .cjs extension explicitly opts out of ESM.
💡 Fix
Replace the ESM imports with CommonJS requires:
const { describe, expect, it } = require('vitest');Or rename the file to .test.mjs and remove the createRequire wrapper since native ESM can use import directly.
@copilot please address this.
There was a problem hiding this comment.
No change needed. Every other .test.cjs file in actions/setup/js/ (e.g. generate_workflow_overview.test.cjs, safe_output_processor.test.cjs) uses the same ESM import syntax. The vitest config (include: ["**/*.test.{js,cjs}"]) transforms these files — confirmed by all 12 tests passing. Changing this file alone would create an inconsistency with the rest of the test suite.
| expect(reduceModelNameToIdentifier("auto")).toBe("auto"); | ||
| }); | ||
|
|
||
| it("returns short model names (< 6 chars) unchanged", () => { |
There was a problem hiding this comment.
[/tdd] Missing boundary test: length === 6 (the exact cutoff).
The guard normalized.length < 6 is tested on names shorter than 6 chars, but there's no test for a name of exactly 6 characters, which should fall through to version-extraction logic. Without this, a future change to the threshold (e.g. <= 6 or < 5) would go unnoticed.
💡 Suggested test
it('processes a 6-char name through version extraction (boundary)', () => {
// 'gpt-4o' is exactly 6 chars and matches the gpt family pattern
expect(reduceModelNameToIdentifier('gpt-4o')).toBe('gpt40');
});@copilot please address this.
| return normalized; | ||
| } | ||
|
|
||
| if (normalized.length < 6) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The early-return threshold (< 6) interacts with the earlier exact-string guard for opus, sonnet, haiku (all ≤ 6 chars). Those three names are already handled before this new guard runs, so their cases are fine — but the added guard now silently passes through any other ≤ 5-char string that could match a family pattern (e.g. a future 3-letter model code). A brief inline comment explaining the intentional ordering would prevent future readers from reordering the guards incorrectly.
💡 Suggestion
// NOTE: exact-name shortcuts (opus/sonnet/haiku) are checked above this guard,
// so < 6 chars here means an unrecognised short identifier — return as-is.
if (normalized.length < 6) {
return normalized;
}@copilot please address this.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Test Quality Sentinel Report - Score 68/100 Acceptable - 10 tests analyzed, 9 design, 1 implementation, 0 violations, 10% implementation tests (threshold 30%). Passed.
|
|
@copilot please run the Reviewer feedback to address:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in the latest commit. The short-name early return now requires |
|
@copilot please run the Unresolved review threads (newest first):
Reviewer feedback to address:
Run: https://github.com/github/gh-aw/actions/runs/30499690701
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all unresolved threads in commit
|
|
@copilot please run the
|
Triage: Fixing model name normalizer logic
|
All previous changes are already pushed (commit Summary:
Hand-off: CI must be re-triggered (close/reopen PR or push from a maintainer) before merge. |
|
@copilot please run the Unresolved review threads (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30507870059
|
All five threads are addressed in the current HEAD (
CI run 30507870059 completed successfully with no errors. Local validation passes (12/12 tests, |
Pull request created by AI Agent
Run: https://github.com/github/gh-aw/actions/runs/30503727864