Fix evals AIC rendering in generated no-op footers - #49802
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ 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. |
There was a problem hiding this comment.
Pull request overview
Splits no-op footer AIC usage into agent, threat-detection, and evals entries.
Changes:
- Adds ordered per-source AIC rendering.
- Updates regression coverage for split and combined entries.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/handle_noop_message.cjs |
Builds separate AIC footer entries. |
actions/setup/js/handle_noop_message.test.cjs |
Verifies formatting and ordering. |
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: Balanced
| * Returns a string like " · 0.001 AIC · ⌖ 0.002 AIC" or "" when not available. | ||
| * @returns {string} | ||
| */ | ||
| function buildAICSuffix() { | ||
| const agentRaw = process.env.GH_AW_AIC; | ||
| const detectionRaw = process.env.GH_AW_THREAT_DETECTION_AIC; | ||
| const evalsRaw = process.env.GH_AW_EVALS_AIC; | ||
| const agentAIC = agentRaw ? Number.parseFloat(agentRaw) : NaN; | ||
| const detectionAIC = detectionRaw ? Number.parseFloat(detectionRaw) : NaN; | ||
| const evalsAIC = evalsRaw ? Number.parseFloat(evalsRaw) : NaN; | ||
| const compressedModelName = reduceModelNameToIdentifier(process.env.GH_AW_PRIMARY_MODEL || process.env.GH_AW_ENGINE_MODEL); | ||
| const totalAIC = (Number.isFinite(agentAIC) && agentAIC > 0 ? agentAIC : 0) + (Number.isFinite(detectionAIC) && detectionAIC > 0 ? detectionAIC : 0) + (Number.isFinite(evalsAIC) && evalsAIC > 0 ? evalsAIC : 0); | ||
| if (totalAIC <= 0) { | ||
| function parsePositiveAIC(raw) { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactor is correct and clean. parsePositiveAIC cleanly extracts parse+guard logic. buildAICEntry handles label/model-alias prefix in all combinations. The combined output matches the documented example. Test coverage locks ordering of all three AIC segments. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.4 AIC · ⌖ 9.18 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Review: no blocking issues
The refactor from a single rolled-up AIC total to per-source breakdown entries (agent / ⌖ detection / ◇ evals) is logically sound and consistent with the new test coverage (26/26 passing, including the new ordering test).
💡 Notes
buildAICEntrycorrectly filters falsy label/modelAlias via.filter(Boolean)and only adds the·separator when a modelAlias is present, matching prior formatting for the agent entry.parsePositiveAICcleanly replaces the inline NaN/threshold checks previously duplicated three times.reduceModelNameToIdentifierreturning""(not undefined) for absent models is handled safely byfilter(Boolean).- Minor maintainability note (non-blocking):
buildAICEntryconflates two different roles (a static marker label like⌘/◇vs. a dynamic model alias) into one function signature, which is slightly confusing to read but is tightly locked down by tests, so not flagging as a comment.
No correctness, concurrency, or security-adjacent issues found in the changed lines.
🔎 Code quality review by PR Code Quality Reviewer · auto · 24.7 AIC · ⌖ 4.86 AIC · ⊞ 7.9K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with minor issues, no blockers.
📋 Key Themes & Highlights
Key Themes
- Separator logic fragility (
buildAICEntryline 103): themodelAlias ? " · " : " "ternary encodes two unrelated concerns in a single boolean, making it brittle for future callers that supply bothlabelandmodelAlias. - Guard type mismatch (line 98):
typeof value === "number"is redundant givenparsePositiveAICalready guaranteesundefined-or-positive; avalue !== undefinedcheck more clearly communicates intent. - Incomplete test isolation (test file): the evals-only test case does not assert absence of
⌖, so a detection-leak regression would pass silently.
Positive Highlights
- ✅ Clean extraction of
parsePositiveAICremoves repeated inline coercion. - ✅
buildAICEntryis a genuinely useful deep helper — composable per-source entries are much easier to extend than a monolithic total accumulator. - ✅ New ordering test (
should place evals AIC after detection AIC) locks the output format precisely — good spec-style naming. - ✅ PR description is detailed and the behavioral impact section is clear.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42.4 AIC · ⌖ 7.61 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
| return ` · ${compressedModelName ? `${compressedModelName} · ` : ""}${formatAIC(totalAIC)} AIC`; | ||
| const prefix = [label, modelAlias].filter(Boolean).join(" "); | ||
| return ` · ${prefix ? `${prefix}${modelAlias ? " · " : " "}` : ""}${formatted} AIC`; |
There was a problem hiding this comment.
[/diagnosing-bugs] The separator logic modelAlias ? " · " : " " is fragile: it conflates "is there a model alias?" with "should I use · as separator?", producing subtle spacing bugs if a future caller passes both label and modelAlias together.
💡 Suggested simplification
Replace the ternary with an unconditional " · " between prefix and value:
function buildAICEntry(label, value, modelAlias) {
const formatted = typeof value === "number" ? formatAIC(value) : "";
if (!formatted) return "";
const parts = [label, modelAlias].filter(Boolean);
const prefix = parts.length ? `${parts.join(" ")} · ` : "";
return ` · ${prefix}${formatted} AIC`;
}This makes the contract explicit: any prefix parts are space-joined, then separated from the AIC value by ·.
@copilot please address this.
| * @returns {string} | ||
| */ | ||
| function buildAICEntry(label, value, modelAlias) { | ||
| const formatted = typeof value === "number" ? formatAIC(value) : ""; |
There was a problem hiding this comment.
[/diagnosing-bugs] formatAIC can return a falsy empty string if given 0, but parsePositiveAIC already guarantees the value is > 0 before returning it. The typeof value === "number" guard is therefore redundant. However, if formatAIC ever returns a non-empty string for 0 (e.g. "0.000"), this check would silently hide the entry rather than raise an error. A direct value !== undefined guard is both safer and more readable.
💡 Suggested change
const formatted = value !== undefined ? formatAIC(value) : "";This matches the intent: skip when the caller explicitly passes undefined (no value), not when formatAIC returns an unexpected falsy output.
@copilot please address this.
| @@ -807,7 +807,32 @@ safe-outputs: | |||
| await main(); | |||
There was a problem hiding this comment.
[/tdd] The existing test that became "evals AIC in the footer breakdown" now only asserts sonnet46 · 0.1 AIC · ◇ 0.025 AIC — it does not assert that detection AIC is absent. This means a regression where detection leaks into an evals-only footer would not be caught.
💡 Suggestion
Add a negative assertion alongside the positive one:
expect(commentCall.body).toContain("sonnet46 · 0.1 AIC · ◇ 0.025 AIC");
expect(commentCall.body).not.toContain("⌖"); // detection AIC should be absent when not set@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (11 tests)
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The JSDoc block was left on |
|
@copilot Please address the remaining maintainer-visible follow-up on this PR, then run the Current signals:
If nothing else is required, leave a short handoff that this is ready for human review on the current head.
|
|
🎉 This pull request is included in a new release. Release: |
No-op footers were collapsing evals AIC into the overall AIC total, so workflows using evals did not show evals consumption explicitly. This updates the generated footer to render evals AIC as its own entry, ordered after threat-detection AIC.
What changed
◇marker after detection AIC (⌖) when both are present.Behavioral impact
Regression coverage
> Generated from [Workflow](<run-url>) · sonnet46 · 0.1 AIC · ⌖ 0.025 AIC · ◇ 0.01 AIC