Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 2 additions & 2 deletions actions/setup/js/daily_aic_cache_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ function readScanCache(content, repository, workflowId, now = Date.now()) {
}
if (
entry?.version !== 2 ||
entry.coverage_version !== 1 ||
entry.coverage_version !== 2 ||
entry.repository !== repository ||
entry.workflow_id !== workflowId ||
!Number.isSafeInteger(entry.run_id) ||
Expand Down Expand Up @@ -72,7 +72,7 @@ function scanCacheEntry(run, aic, repository, workflowId, now = Date.now(), sour
}
return {
version: 2,
coverage_version: 1,
coverage_version: 2,
repository,
workflow_id: workflowId,
run_id: run.id,
Expand Down
7 changes: 4 additions & 3 deletions actions/setup/js/daily_aic_component_coverage.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,17 @@ async function loadBillableJobs({ github, budget }, owner, repo, run) {
const jobs = response.data.jobs;
if (!Array.isArray(jobs)) throw new Error("Incomplete daily AIC job metadata");
for (const job of jobs) {
if (!Object.hasOwn(COMPONENT_FILES, job.name)) continue;
const name = typeof job.name === "string" ? job.name.toLowerCase() : "";

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.

This fix correctly lowercases GitHub's display names Agent/Detection to match COMPONENT_FILES, but the real compiled evals job display name is Evaluations, not Evals (confirmed across all 167 compiled .github/workflows/*.lock.yml — e.g. ab-testing-advisor.lock.yml line 1913: name: Evaluations).

"evaluations".toLowerCase() does not match the evals key in COMPONENT_FILES (actions/setup/js/daily_aic_component_coverage.cjs:9-12), so completed Evaluations jobs will still be silently skipped by Object.hasOwn(COMPONENT_FILES, name) — the exact same class of bug this PR is fixing for Agent/Detection, just left unfixed for evals.

The new regression test at daily_aic_component_coverage.test.cjs:108 uses job("Evals") instead of the real display name job("Evaluations"), which masks this gap (the fabricated name happens to lowercase to evals and pass).

Suggest adding an evaluations → evals alias (or mapping GitHub's actual display names explicitly) and updating the test to use the real compiled name "Evaluations".

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Handled in 86450fe: Evaluations now maps to evals for component and artifact accounting, with regression coverage using the actual display name.

if (!Object.hasOwn(COMPONENT_FILES, name)) continue;
if (!Number.isSafeInteger(job.run_attempt) || job.run_attempt < 1 || job.run_attempt > run.run_attempt || job.status !== "completed" || !job.conclusion) {
throw new Error("Incomplete daily AIC component attempt metadata");
}
const prior = components.get(job.name);
const prior = components.get(name);
if (prior && job.run_attempt === prior.run_attempt && job.id !== prior.id) {
throw new Error("Ambiguous daily AIC component jobs");
}
if (!prior || (prior.conclusion === "skipped" && job.conclusion !== "skipped") || (job.conclusion !== "skipped" && job.run_attempt > prior.run_attempt) || (prior.conclusion === "skipped" && job.run_attempt > prior.run_attempt)) {
components.set(job.name, job);
components.set(name, job);
}
}
if (jobs.length < 100) {
Expand Down
19 changes: 19 additions & 0 deletions actions/setup/js/daily_aic_component_coverage.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,25 @@ it("counts an empty detection accounting file as zero AIC", async () => {
expect(global.core.info).toHaveBeenCalledWith(expect.stringContaining('"source":"detection/token_usage.jsonl"'));
});

it("counts compiler-generated Agent, Detection and Evals jobs using their lowercase accounting paths", async () => {
const f = evaluate(
{
"agent/token_usage.jsonl": '{"aic":18.02604}',
"detection/token_usage.jsonl": '{"aic":30.08748}',
"evals/token_usage.jsonl": '{"aic":1}',
},
[job("Agent"), job("Detection"), job("Evals")]

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 compiler's real generated evals job name is "Evaluations" (see pkg/workflow/compiler_job_annotations.go:17, confirmed as name: Evaluations in every compiled .lock.yml), not "Evals". "Evaluations".toLowerCase() is "evaluations", which does not match the evals key in COMPONENT_FILES, so this fix does not actually resolve evals-job accounting — only agent/detection are genuinely fixed.

💡 Why this matters

The test at line 115 (job("Evals")) exercises a job name that never occurs in a real workflow run, so it gives false confidence that evals coverage is fixed. In production, evals jobs will still be silently skipped (zero-credit) exactly like before this PR, defeating the PR's stated goal of fixing counted jobs across the board.

Suggested fix: either special-case "evaluations" in the lookup (e.g. add an alias map from display name → component key), or derive the lowercase key more robustly, and add a regression test using the actual display name "Evaluations".

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Handled in 86450fe: the test now uses the compiler-generated Evaluations name, which is normalized to the lowercase evals accounting component.

);
await expect(f.result).resolves.toBeCloseTo(49.11352);
expect(f.client.listArtifacts).toHaveBeenCalledOnce();
expect(global.core.info).not.toHaveBeenCalledWith(expect.stringContaining('"reason":"no_billable_jobs"'));
});

it("treats differently cased names for the same component as ambiguous", async () => {
const f = evaluate({}, [job("Agent", { id: 1 }), job("agent", { id: 2 })]);
await expect(f.result).rejects.toThrow("Ambiguous daily AIC component jobs");
});

it("still requires accounting when detection/token_usage.jsonl is missing (not empty)", async () => {
const f = evaluate(
{
Expand Down
35 changes: 35 additions & 0 deletions actions/setup/js/daily_aic_scan.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,41 @@ describe("complete daily AIC scan observations", () => {
expect(global.core.info).toHaveBeenCalledWith(expect.stringContaining('"reason":"scan_cache"'));
});

it("recounts version 1 zero-credit observations from completed Agent and Detection jobs", async () => {
const prior = scanCacheEntry(run(1), 0, repository, 7, now);
prior.coverage_version = 1;
writeEntries([prior]);
const f = fixture([run(1)]);
f.getRunAIC = guardrail.getRunAIC;
f.github.rest.actions.listJobsForWorkflowRun.mockResolvedValue(
response({
jobs: [
{ id: 1, name: "Agent", run_attempt: 1, status: "completed", conclusion: "success", started_at: time, completed_at: time },
{ id: 2, name: "Detection", run_attempt: 1, status: "completed", conclusion: "success", started_at: time, completed_at: time },
],
})
);
f.artifactClient = {
listArtifacts: vi.fn(async () => ({ artifacts: [{ id: 10, name: "usage", createdAt: new Date(time) }] })),
downloadArtifact: vi.fn(async (_id, options) => {
for (const [name, aic] of [
["agent", 18.02604],
["detection", 30.08748],
]) {
const file = path.join(options.path, name, "token_usage.jsonl");
fs.mkdirSync(path.dirname(file), { recursive: true });
fs.writeFileSync(file, JSON.stringify({ aic }));
}
return { downloadPath: options.path };
}),
};
const result = await scanDailyAIC(f);
expect(result.cacheHits).toBe(0);
expect(result.countedRuns[0].aic).toBeCloseTo(48.11352);
expect(readScanCache(fs.readFileSync(cachePath, "utf8"), repository, 7, now).get(1)).toMatchObject({ coverage_version: 2, source: "recorded" });
expect((await scanDailyAIC(f)).cacheHits).toBe(1);
});

it.each(["missing", "metadata-only", "malformed", "unknown-model", "old-attempt", "detection", "evals", "invalid-numeric"])("conservatively assumes max AI Credits when %s usage cannot be resolved", async kind => {
const f = fixture([run(1)]);
f.getRunAIC = guardrail.getRunAIC;
Expand Down
Loading