Render compressed model alias before AIC in generated footers - #47018
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds compressed model aliases to AIC footer attribution.
Changes:
- Prefixes AIC values with compressed model names.
- Applies formatting to standard, failure, and no-op footers.
- Updates footer tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/messages_footer.cjs |
Adds model-prefixed AIC formatting. |
actions/setup/js/handle_noop_message.cjs |
Adds model attribution to no-op footers. |
actions/setup/js/messages.test.cjs |
Updates standard and failure footer expectations. |
actions/setup/js/handle_noop_message.test.cjs |
Tests no-op attribution. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests generated failure issue attribution. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Medium
| aiCredits, | ||
| aiCreditsFormatted, | ||
| aiCreditsSuffix, | ||
| compressedModelName, |
There was a problem hiding this comment.
Fixed in the latest commit. compressedModelName: string|undefined is now included in the getAICFromEnv() JSDoc @returns type, resolving the TS2353/TS2339 errors.
| * }} | ||
| */ | ||
| function getAICFromEnv() { | ||
| const compressedModelName = reduceModelNameToIdentifier(process.env.GH_AW_ENGINE_MODEL); |
There was a problem hiding this comment.
Fixed. parse_token_usage.cjs now exports GH_AW_PRIMARY_MODEL via core.exportVariable (using the actual model observed in token-usage logs). getAICFromEnv() in messages_footer.cjs now reads process.env.GH_AW_PRIMARY_MODEL || process.env.GH_AW_ENGINE_MODEL, so the display reflects the real spending model rather than the configured alias.
| const detectionRaw = process.env.GH_AW_THREAT_DETECTION_AIC; | ||
| const agentAIC = agentRaw ? Number.parseFloat(agentRaw) : NaN; | ||
| const detectionAIC = detectionRaw ? Number.parseFloat(detectionRaw) : NaN; | ||
| const compressedModelName = reduceModelNameToIdentifier(process.env.GH_AW_ENGINE_MODEL); |
There was a problem hiding this comment.
Fixed. buildAICSuffix() in handle_noop_message.cjs now uses process.env.GH_AW_PRIMARY_MODEL || process.env.GH_AW_ENGINE_MODEL for the same accurate attribution, consistent with the regular footer path.
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47018 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Review: Render compressed model alias before AIC in generated footers
The change is clean and consistent across footer paths. Existing inline comments capture the two substantive concerns:
-
Model attribution accuracy —
GH_AW_ENGINE_MODELmay hold a logical alias (e.g.agent) rather than the observed model from token usage, so the displayed prefix could be misleading. -
JSDoc return type gap —
compressedModelNameis now included in thegetAICFromEnvreturn object but is absent from the declared@returnstypedef.
No blocking issues beyond what is already flagged in existing inline comments.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 17.9 AIC · ⌖ 4.37 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs and /codebase-design. The three existing inline comments from Copilot already surface the key concerns. No new blocking issues found.
Positive highlights: clean consistent application across all footer paths, backwards-compatible buildAICEntry extension, tests updated with proper env teardown.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 14.8 AIC · ⌖ 4.35 AIC · ⊞ 6.6K
Comment /matt to run again
There was a problem hiding this comment.
Non-blocking observations
The PR consistently prepends compressedModelName before the AIC value. The logic is correct for the happy path, but three pre-existing review comments (JSDoc return-type gap, GH_AW_ENGINE_MODEL attribution reliability) and one new finding (redundant buildAICEntry call on the non-breakdown path) are worth addressing before merge.
Summary of findings
- JSDoc return type (
messages_footer.cjs~line 125):compressedModelNameis now returned but not declared — callers using//@ts-check`` will see a type error. - Attribution correctness (
messages_footer.cjs~line 109):GH_AW_ENGINE_MODELmay hold the logical alias (agent) rather than the resolved model name; the suffix could display misleading identifiers. - Same gap in noop path (
handle_noop_message.cjs~line 91): identical attribution concern applies there. - Redundant
buildAICEntrycall (messages_footer.cjsline 173): non-breakdown path builds a second entry with the same args instead of reusingagentEntry, which is a silent divergence risk.
None of these are data-loss or crash-level bugs, but the first two affect output correctness.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 26.2 AIC · ⌖ 4.59 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/messages_footer.cjs:173
Redundant buildAICEntry call on the non-breakdown path discards already-computed entry: agentEntry was just built with the same (\"\" , agentAIC, compressedModelName) args; when totalAIC is used as the aiCredits value these two can silently diverge.
<details>
<summary>💡 Suggested fix</summary>
Either reuse agentEntry.suffix directly (valid when aiCredits === agentAIC), or extract the entry into a named variable so it is computed only once:
const aiCreditsEntry = buil…
</details>
🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
Verdict
All 6 modified tests verify user-visible footer output with the new
|
|
@copilot please run the
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
…footer display - Add `compressedModelName: string|undefined` to `getAICFromEnv()` JSDoc return type, fixing TS2353/TS2339 errors that broke js-typecheck and impacted-js-tests - Export `GH_AW_PRIMARY_MODEL` env var from `parse_token_usage.cjs` so the actual observed model (from token-usage logs) is available downstream - Update `messages_footer.cjs` and `handle_noop_message.cjs` to read `GH_AW_PRIMARY_MODEL` first, falling back to `GH_AW_ENGINE_MODEL`, so the displayed model reflects what actually incurred the AIC rather than the configured logical alias (e.g. 'agent') - Add `GH_AW_PRIMARY_MODEL` to messages.test.cjs beforeEach cleanup - Add assertions in parse_token_usage.test.cjs for the new export Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Footer cost attribution was emitting only numeric AIC, which obscured which model generated the spend. This update prepends the compressed model alias (for example
sonnet46) immediately before the AIC value across generated issue/comment/body footer paths.Core footer AIC suffix formatting
messages_footer.cjsto derivecompressedModelNamefromGH_AW_ENGINE_MODELand include it in AIC suffix construction.GH_AW_AIC)GH_AW_AGENT_AIC,GH_AW_THREAT_DETECTION_AIC)No-op comment footer parity
handle_noop_message.cjsfooter suffix generation to use the same compressed model prefix before total AIC.Behavioral coverage updates
Run: https://github.com/github/gh-aw/actions/runs/29816999996
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.