Skip to content

Fix evals AIC rendering in generated no-op footers #49802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Aug 2, 2026
+58 −17
Merged

Fix evals AIC rendering in generated no-op footers #49802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db6de3d
fix noop footer evals AIC breakdown
Copilot Aug 2, 2026
58ef148
Merge branch 'main' into copilot/fix-footer-evals-aic-rendering
pelikhan Aug 2, 2026
124685a
Fix JSDoc on parsePositiveAIC: correct description and return type
Copilot Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions actions/setup/js/handle_noop_message.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,24 +78,40 @@ async function ensureAgentRunsIssue() {
}

/**
* Build the AIC suffix string for use in comment footers.
* Includes agent, threat-detection, and evals AIC when available.
* Returns a string like " · 0.001 AIC" or "" when not available.
* Parse a raw AIC environment variable value and return it as a positive number.
* Returns undefined when the value is absent, non-numeric, or non-positive.
* @param {string|undefined} raw
* @returns {number|undefined}
*/
function parsePositiveAIC(raw) {
const parsed = raw ? Number.parseFloat(raw) : NaN;
return Number.isFinite(parsed) && parsed > 0 ? parsed : undefined;
}

/**
* @param {string} label
* @param {number|undefined} value
* @param {string|undefined} [modelAlias]
* @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 buildAICEntry(label, value, modelAlias) {
const formatted = typeof value === "number" ? formatAIC(value) : "";

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

if (!formatted) {
return "";
}
return ` · ${compressedModelName ? `${compressedModelName} · ` : ""}${formatAIC(totalAIC)} AIC`;
const prefix = [label, modelAlias].filter(Boolean).join(" ");
return ` · ${prefix ? `${prefix}${modelAlias ? " · " : " "}` : ""}${formatted} AIC`;

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

}

function buildAICSuffix() {
const agentAIC = parsePositiveAIC(process.env.GH_AW_AIC);
const detectionAIC = parsePositiveAIC(process.env.GH_AW_THREAT_DETECTION_AIC);
const evalsAIC = parsePositiveAIC(process.env.GH_AW_EVALS_AIC);
const compressedModelName = reduceModelNameToIdentifier(process.env.GH_AW_PRIMARY_MODEL || process.env.GH_AW_ENGINE_MODEL);
const agentSuffix = buildAICEntry("", agentAIC, compressedModelName);
const detectionSuffix = buildAICEntry("⌖", detectionAIC);
const evalsSuffix = buildAICEntry("◇", evalsAIC);
return `${agentSuffix}${detectionSuffix}${evalsSuffix}`;
}

/**
Expand Down
31 changes: 28 additions & 3 deletions actions/setup/js/handle_noop_message.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,10 +783,10 @@ safe-outputs:
await main();

const commentCall = mockGithub.rest.issues.createComment.mock.calls[0][0];
expect(commentCall.body).toContain("sonnet46 · 0.125 AIC");
expect(commentCall.body).toContain("sonnet46 · 0.1 AIC · ⌖ 0.025 AIC");
});

it("should include evals AIC in the footer total when GH_AW_EVALS_AIC is set", async () => {
it("should include evals AIC in the footer breakdown when GH_AW_EVALS_AIC is set", async () => {
process.env.GH_AW_WORKFLOW_NAME = "Evals AIC Workflow";
process.env.GH_AW_RUN_URL = "https://github.com/test/test/actions/runs/123";
process.env.GH_AW_AGENT_CONCLUSION = "success";
Expand All @@ -807,7 +807,32 @@ safe-outputs:
await main();

@github-actions github-actions Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.


const commentCall = mockGithub.rest.issues.createComment.mock.calls[0][0];
expect(commentCall.body).toContain("sonnet46 · 0.125 AIC");
expect(commentCall.body).toContain("sonnet46 · 0.1 AIC · ◇ 0.025 AIC");
});

it("should place evals AIC after detection AIC in the footer breakdown", async () => {
process.env.GH_AW_WORKFLOW_NAME = "Ordered AIC Workflow";
process.env.GH_AW_RUN_URL = "https://github.com/test/test/actions/runs/123";
process.env.GH_AW_AGENT_CONCLUSION = "success";
process.env.GH_AW_AIC = "0.100";
process.env.GH_AW_THREAT_DETECTION_AIC = "0.025";
process.env.GH_AW_EVALS_AIC = "0.010";
process.env.GH_AW_ENGINE_MODEL = "claude-sonnet-4.6";

const outputFile = path.join(tempDir, "agent_output.json");
fs.writeFileSync(outputFile, JSON.stringify({ items: [{ type: "noop", message: "No action needed" }] }));
process.env.GH_AW_AGENT_OUTPUT = outputFile;

mockGithub.rest.search.issuesAndPullRequests.mockResolvedValue({
data: { total_count: 1, items: [{ number: 1, node_id: "ID", html_url: "url" }] },
});
mockGithub.rest.issues.createComment.mockResolvedValue({ data: {} });

const { main } = await import("./handle_noop_message.cjs?t=" + Date.now());
await main();

const commentCall = mockGithub.rest.issues.createComment.mock.calls[0][0];
expect(commentCall.body).toContain("sonnet46 · 0.1 AIC · ⌖ 0.025 AIC · ◇ 0.01 AIC");
});

it("should not include AIC suffix in comment footer when GH_AW_AIC is not set", async () => {
Expand Down
Loading