Show evals AIC in generated footer metrics - #47260
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (13 additions in default business logic directories). |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds evals AI Credits to generated footer metrics and conclusion-time cost totals.
Changes:
- Exposes and propagates evals AIC.
- Adds
◇evals metrics to standard and failure footers. - Extends no-op cost aggregation and tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/notify_comment_conclusion_helpers.go |
Propagates evals AIC to conclusion handlers. |
pkg/workflow/evals_job.go |
Declares the evals AIC output. |
pkg/workflow/evals_job_test.go |
Tests the output mapping. |
pkg/workflow/compiler_jobs_test.go |
Tests conclusion environment wiring. |
actions/setup/js/messages.test.cjs |
Tests evals footer rendering. |
actions/setup/js/messages_footer.cjs |
Aggregates and renders evals AIC. |
actions/setup/js/handle_noop_message.test.cjs |
Tests no-op evals totals. |
actions/setup/js/handle_noop_message.cjs |
Includes evals in no-op totals. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Medium
| Outputs: map[string]string{ | ||
| "aic": fmt.Sprintf("${{ steps.%s.outputs.aic }}", constants.ParseMCPGatewayStepID), | ||
| }, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are correct and complete. Evals AIC is consistently wired through all footer-generating paths (getFooterMessage, getFooterAgentFailureIssueMessage, getFooterAgentFailureCommentMessage, and buildAICSuffix in the noop handler) with the ◇ icon. The useBreakdown logic, total AIC summation, and job output exposure are all handled correctly. Test coverage is thorough.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 15.7 AIC · ⌖ 5.1 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one observation on test coverage; no blocking issues.
📋 Key Themes & Highlights
Key Theme
- Missing combined test: The new tests each isolate evals AIC on its own, but none tests the scenario where both
GH_AW_THREAT_DETECTION_AICandGH_AW_EVALS_AICare set together. This is the most likely production scenario and would catch any ordering or total-calculation bugs between the two breakdown entries.
Positive Highlights
- ✅ Consistent pattern: evals follows the exact same shape as threat-detection (parse → entry → suffix → return)
- ✅ Good use of the existing
buildAICEntryabstraction — no duplication - ✅ Conditional guard (
data.Evals != nil && data.Evals.HasEvals()) correctly mirrors the detection pattern - ✅ Go-side output wiring and compiler test are clean and complete
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 18.3 AIC · ⌖ 4.69 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| it("should include ambient context in ai_credits_suffix for custom footer templates", async () => { | ||
| process.env.GH_AW_AGENT_AIC = "1.25"; | ||
| process.env.GH_AW_THREAT_DETECTION_AIC = "0.25"; |
There was a problem hiding this comment.
[/tdd] No test covers the combined case where both GH_AW_THREAT_DETECTION_AIC and GH_AW_EVALS_AIC are set simultaneously — the ordering agent → detection → evals in the suffix and the combined total are untested together.
💡 Suggested test
it("should include both detection and evals AIC entries in correct order", async () => {
process.env.GH_AW_AGENT_AIC = "1.00";
process.env.GH_AW_THREAT_DETECTION_AIC = "0.10";
process.env.GH_AW_EVALS_AIC = "0.05";
const { getFooterMessage } = await import("./messages.cjs");
const result = getFooterMessage({ workflowName: "W", runUrl: "(u/redacted)" });
// total = 1.15, detection before evals
expect(result).toContain("⌖ 0.1 AIC · ◇ 0.05 AIC");
});@copilot please address this.
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 49/100 — Needs improvement
📊 Metrics (7 tests)
|
There was a problem hiding this comment.
REQUEST_CHANGES — three blocking issues
This PR cannot achieve its stated goal as written: evals AIC will always be empty at runtime, the noop path diverges from the breakdown rendering, and a test masks the inconsistency through coincidental arithmetic.
🔍 Blocking findings (details in inline comments)
1. Evals job aic output is permanently empty (pkg/workflow/evals_job.go:102)
parse-mcp-gateway is only emitted for the main agent job; the evals job never creates that step. ${{ steps.parse-mcp-gateway.outputs.aic }} will always expand to empty, so GH_AW_EVALS_AIC is never set in conclusion steps. The entire PR is a no-op at runtime.
2. buildAICSuffix in handle_noop_message.cjs discards the ◇ icon (line 94)
messages_footer.cjs shows ◇ 0.025 AIC as a distinct labeled entry; noop collapses everything into one unlabeled total. Inconsistent UX.
3. New noop test passes by coincidental arithmetic (handle_noop_message.test.cjs:56)
0.100 + 0.025 = 0.125 satisfies the assertion, but there is no assertion for ◇ 0.025 AIC appearing separately, giving false coverage for the wrong behavior.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 113.4 AIC · ⌖ 5.23 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/evals_job.go:102
Evals AIC output will always be empty: the parse-mcp-gateway step doesn't exist in the evals job, so GH_AW_EVALS_AIC will never be populated and footers will silently under-report evals cost.
<details>
<summary>💡 Details</summary>
constants.ParseMCPGatewayStepID (parse-mcp-gateway) is emitted only by compiler_yaml_ai_execution.go for the main agent job. The evals job builds all its steps via buildEvalsJobSteps and buildEvalsEngineSteps, neither of which ever emits a ste…
actions/setup/js/handle_noop_message.cjs:94
Noop footer omits evals breakdown icon: buildAICSuffix silently folds evalsAIC into the total with no ◇ marker, diverging from messages_footer.cjs which renders it as a distinct metric.
<details>
<summary>💡 Details</summary>
In messages_footer.cjs, when evalsAIC is present the suffix is rendered as agent + ⌖threat + ◇evals — each component labeled. In handle_noop_message.cjs buildAICSuffix, all three are summed into one totalAIC and shown as a single unlabeled number. A…
actions/setup/js/messages_footer.cjs:123
Potential double-count of evals/threat costs when GH_AW_AIC is the all-up total: if GH_AW_AGENT_AIC is absent, agentAIC falls back to totalAIC (which may already include evals). Adding evalsAIC again in breakdown mode inflates the displayed figure.
<details>
<summary>💡 Details</summary>
const agentAIC = typeof explicitAgentAIC === "number" ? explicitAgentAIC : totalAIC;
// ...
const aiCredits = useBreakdown
? (agentAIC || 0) + (threatDetectionAIC || 0) + (evalsAIC || 0)…
</details>
<details><summary>actions/setup/js/handle_noop_message.test.cjs:56</summary>
**Test asserts wrong AIC value**: expects `0.125` but `GH_AW_AIC=0.100` + `GH_AW_EVALS_AIC=0.025` = `0.125` — the test only passes because both values happen to sum to the expected total, hiding the fact that evals is not shown separately with a `◇` icon.
<details>
<summary>💡 Details</summary>
```js
process.env.GH_AW_AIC = "0.100";
process.env.GH_AW_EVALS_AIC = "0.025";
// ...
expect(commentCall.body).toContain("sonnet46 0.125 AIC");This test verifies that buildAICSuffix produces the…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed a PR-finisher pass and pushed fixes in Addressed the unresolved feedback by adding MCP gateway log parsing to the evals job (so |
|
@copilot please run the Unresolved reviews:
|
|
🎉 This pull request is included in a new release. Release: |
Generated footers previously reported agent and detection AIC but omitted evals cost, so total run cost could be understated. This change adds evals AIC to footer rendering using the same compact metric style and a dedicated Unicode marker.
Footer cost composition
GH_AW_EVALS_AIC.◇, alongside existing entries (e.g.,⌖for detection).Conclusion/no-op wiring
GH_AW_EVALS_AIC.Workflow data flow
aicas a job output.needs.evals.outputs.aicin conclusion helpers to make evals cost available where footers are generated.