Prefix effective-token footer values with deterministic 5-char model IDs#34291
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>
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #34291 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold is 100). |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR adds a deterministic, compact (5-character) model identifier and prefixes effective-token (ET) footer values with it so readers can quickly see which model produced the usage signal (e.g., son46 12.5K).
Changes:
- Added
reduceModelNameToIdentifier(modelName)(with well-known family shortcuts + deterministic fallback) and updatedgetEffectiveTokensSuffix()to include the reduced model prefix whenGH_AW_ENGINE_MODELis set. - Updated footer rendering paths (default and templated
{effective_tokens_suffix}) to prepend the reduced model identifier when available. - Expanded unit tests to cover model reduction, date-like suffix handling, fallback IDs, and footer/template output.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/effective_tokens.cjs | Introduces deterministic 5-char model ID reduction and prefixes ET suffix output with the reduced model ID when available. |
| actions/setup/js/messages_footer.cjs | Wires model ID reduction into footer ET suffix generation for both default and templated footers. |
| actions/setup/js/effective_tokens.test.cjs | Adds tests for model ID reduction and for model-prefixed ET suffix behavior. |
| actions/setup/js/messages.test.cjs | Adds footer tests ensuring model-prefixed ET rendering for default and custom-template suffix paths. |
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: 0
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /grill-with-docs, and /zoom-out — overall a clean, well-structured feature with good baseline test coverage. No blocking issues, but a few areas worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps:
haikuandgeminishortcuts are declared but untested;null/undefinedinput contract is unverified. - Date-like heuristic ambiguity:
^\d{3,}$suppresses any 3+ digit minor segment, not just calendar stamps — the threshold deserves a comment or a narrower regex. - Duplicate suffix computation: In
getFooterMessage,getEffectiveTokensFromEnvreturns aneffectiveTokensSuffixthat is silently discarded and rebuilt moments later — a minor architectural inconsistency.
Positive Highlights
- ✅ Deterministic fallback (
buildFallbackModelIdentifier) handles arbitrary model names without crashing or producing unbounded output. - ✅
normalizeVersionDigitsdate-suppression logic is well-commented and makes the intent clear. - ✅ Integration tests in
messages.test.cjsverify end-to-end footer rendering with the new prefix — good coverage of the happy path. - ✅ Exporting
reduceModelNameToIdentifierfrom the module makes it independently testable.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● son46 1.7M
|
|
||
| test("returns deterministic 5-character fallback for unknown models", () => { | ||
| expect(reduceModelNameToIdentifier("my-custom-engine-v2")).toBe("myc20"); | ||
| }); |
There was a problem hiding this comment.
[/tdd] haiku and gemini shortcuts are declared in the shortcuts array but have no test coverage — leaving two of the five known families unverified.
💡 Suggested tests
test("uses well-known haiku shortcut", () => {
expect(reduceModelNameToIdentifier("claude-haiku-4.5")).toBe("hai45");
});
test("uses well-known gemini shortcut", () => {
expect(reduceModelNameToIdentifier("gemini-2.5-pro")).toBe("gem25");
});If a regex or prefix typo were introduced for either family, it would only be caught at runtime.
| expect(formatET(999)).toBe("999"); | ||
| }); | ||
|
|
||
| describe("reduceModelNameToIdentifier", () => { |
There was a problem hiding this comment.
[/tdd] The reduceModelNameToIdentifier test suite doesn't cover null, undefined, or an empty string — all of which the implementation handles, but the contract isn't pinned by a test.
💡 Suggested tests
test("returns empty string for null input", () => {
expect(reduceModelNameToIdentifier(null)).toBe("");
});
test("returns empty string for undefined input", () => {
expect(reduceModelNameToIdentifier(undefined)).toBe("");
});
test("returns empty string for empty string input", () => {
expect(reduceModelNameToIdentifier("")).toBe("");
});Without these, a refactor that accidentally breaks the null-guard would go undetected.
| const majorDigit = getFirstDigit(major); | ||
| // Date-like suffixes (e.g. 20250514) are build/release stamps, not semantic minor versions. | ||
| // Treat them as missing so identifiers stay stable (gpt-5-2025-08-07 -> gpt50). | ||
| const minorIsDateLike = minor && /^\d{3,}$/.test(minor); |
There was a problem hiding this comment.
[/grill-with-docs] The date-like detection regex ^\d{3,}$ suppresses any minor token with 3+ digits — not just actual date stamps. gpt-4-100 would produce gpt40 (100 treated as date-like), which may be surprising for non-date version numbers.
💡 Consider narrowing the heuristic
If the intent is specifically to suppress 8-digit YYYYMMDD stamps or 4+ digit year prefixes, a more precise pattern like /^\d{8}$|^20\d{2}/ would avoid false positives on smaller numbers.
Alternatively, document the current threshold explicitly:
// Treat any 3+ digit minor segment as a build/date stamp (e.g. "100", "20250514")
// to keep identifiers stable across date-stamped releases.
const minorIsDateLike = minor && /^\d{3,}$/.test(minor);Either way, a test case like reduceModelNameToIdentifier("gpt-4-100") would pin the current behaviour.
| // still get the effective token count in the footer when GH_AW_EFFECTIVE_TOKENS is set. | ||
| const { effectiveTokens: envEffectiveTokens } = getEffectiveTokensFromEnv(); | ||
| const resolvedModelName = ctx.model || process.env.GH_AW_ENGINE_MODEL || ""; | ||
| const { effectiveTokens: envEffectiveTokens } = getEffectiveTokensFromEnv(resolvedModelName); |
There was a problem hiding this comment.
[/zoom-out] getEffectiveTokensFromEnv(resolvedModelName) computes and returns an effectiveTokensSuffix (including model prefix) that is immediately discarded — only effectiveTokens (the raw number) is destructured. The suffix is then rebuilt independently at line 103. This split makes the function's signature misleading for this call-site.
💡 Options to resolve
Option A — use the suffix already built by getEffectiveTokensFromEnv directly:
const { effectiveTokens: envEffectiveTokens, effectiveTokensSuffix: envSuffix } = getEffectiveTokensFromEnv(resolvedModelName);
// then use envSuffix rather than rebuilding it belowOption B — keep getEffectiveTokensFromEnv model-agnostic (just parse the env var number), and move all model-prefix logic to the callers that need it. This would reduce the function's responsibility and make the suffix computation visible at each call-site.
Either choice avoids the current situation where the same suffix is computed twice for the getFooterMessage path.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics & Test Classification (9 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
|
@copilot review all comments and reviews including from GitHub-actions shop |
…uffix reuse Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed all PR comments/reviews (including github-actions) and addressed the actionable items in Updates include:
Targeted lint/tests pass, and final parallel validation is clean. |
…ID prefix Documents two user-facing changes merged 2026-05-23/24: - #34239: compiler warning when prompts reference /tmp/ or /tmp/gh-aw/ directly. Added a "Temporary File Paths in Prompts" section to the Markdown reference describing the convention and the warning text. - #34291: 5-character model ID prefix (sonNN/gptNN/opuNN/haiNN/gemNN) in the effective-token footer suffix. Updated the {effective_tokens_suffix} description in safe-outputs and footers references to show the new shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Effective token values in generated footers now include a compact model identifier prefix so readers can instantly see which model produced the usage signal (e.g.,
son46,gpt55,opu47).This adds a deterministic reducer with well-known shortcuts and wires it into footer suffix rendering paths.
Model identifier reduction
reduceModelNameToIdentifier(modelName)inactions/setup/js/effective_tokens.cjs.sonnet -> sonNNgpt -> gptNNopus -> opuNNhaiku -> haiNNgemini -> gemNNFooter effective-token rendering
GH_AW_ENGINE_MODELis present.· ● <modelId> <formattedET>instead of· ● <formattedET>.Safe-output footer integration
actions/setup/js/messages_footer.cjsto use the same reducer path for:{effective_tokens_suffix}Targeted behavior coverage
actions/setup/js/effective_tokens.test.cjsactions/setup/js/messages.test.cjs