Skip to content

fix: daily ET guardrail step never fails activation job; improve conclusion reporting #36497

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 5 commits into from
Jun 2, 2026
+286 −142
Merged

fix: daily ET guardrail step never fails activation job; improve conclusion reporting #36497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cdccaf7
fix: ensure daily ET guardrail step never fails activation job and im…
Copilot Jun 2, 2026
7d03622
refine: address review feedback on JSDoc and billing note clarity
Copilot Jun 2, 2026
5b87535
Apply remaining changes
Copilot Jun 2, 2026
2884717
Merge branch 'main' into copilot/review-js-check-daily-tokens
pelikhan Jun 2, 2026
f7e508b
fix: format guardrail test for cjs lint
Copilot Jun 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
293 changes: 156 additions & 137 deletions actions/setup/js/check_daily_effective_workflow_guardrail.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,13 @@ async function appendDailyEffectiveWorkflowSummary(workflowName, actorLogin, thr
* @returns {Promise<void>}
*
* Requires github-script globals (`core`, `github`, `context`) provided by setupGlobals().
*
* Error handling: all GitHub API interactions after the initial guard checks are wrapped
* in a top-level try-catch. Any unexpected error (network failure, permission error, etc.)
* is logged as a warning and the function returns cleanly with `daily_effective_workflow_exceeded`
* left at its default value of `"false"`. This design ensures the step never fails the
* activation job — a guardrail error results in a safe bypass (agent allowed to run) rather
* than a confusing workflow failure that blocks the agent entirely.
*/
async function main() {
core.setOutput("daily_effective_workflow_exceeded", "false");
Expand All @@ -296,167 +303,179 @@ async function main() {
return;
}

const githubClient = createRateLimitAwareGithub(github);
const { owner, repo } = context.repo;
const currentRun = await githubClient.rest.actions.getWorkflowRun({
owner,
repo,
run_id: context.runId,
});
const rateLimit = await getCoreRateLimitSnapshot(githubClient);

const workflowID = process.env.GH_AW_WORKFLOW_ID || "";
const workflowName = process.env.GH_AW_WORKFLOW_NAME || workflowID || "workflow";
const runUrl = process.env.GH_AW_RUN_URL || currentRun.data.html_url || "";
const actorLogin = process.env.GITHUB_TRIGGERING_ACTOR || currentRun.data.triggering_actor?.login || currentRun.data.actor?.login || process.env.GITHUB_ACTOR || "";
// Wrap all GitHub API interactions in a top-level try-catch so that transient API
// errors, permission failures, or unexpected exceptions never fail the activation
// job step. A failure here would leave `daily_effective_workflow_exceeded` at its
// default "false" value, which is the safe fallback: the agent is allowed to run
// and the guardrail is effectively bypassed for this invocation rather than causing
// a confusing workflow failure.
try {
const githubClient = createRateLimitAwareGithub(github);
const { owner, repo } = context.repo;
const currentRun = await githubClient.rest.actions.getWorkflowRun({
owner,
repo,
run_id: context.runId,
});
const rateLimit = await getCoreRateLimitSnapshot(githubClient);

if (!currentRun.data.workflow_id || !actorLogin) {
core.warning("Skipping daily workflow ET guardrail because the current workflow or actor could not be resolved.");
return;
}
const workflowID = process.env.GH_AW_WORKFLOW_ID || "";
const workflowName = process.env.GH_AW_WORKFLOW_NAME || workflowID || "workflow";
const actorLogin = process.env.GITHUB_TRIGGERING_ACTOR || currentRun.data.triggering_actor?.login || currentRun.data.actor?.login || process.env.GITHUB_ACTOR || "";
Comment on lines +322 to +324

logDailyGuardrail("Resolved current workflow ET guardrail context", {
owner,
repo,
currentRunId: context.runId,
workflowId: currentRun.data.workflow_id,
workflowName,
actorLogin,
threshold,
rateLimitRemaining: rateLimit.remaining,
rateLimitLimit: rateLimit.limit,
});
const maxInspectableRuns = computeMaxInspectableRuns(rateLimit.remaining);
if (maxInspectableRuns <= 0) {
core.warning(`Skipping daily workflow ET guardrail because the GitHub API rate limit is too low (${rateLimit.remaining} remaining, reserve ${RATE_LIMIT_RESERVE}).`);
return;
}
if (!currentRun.data.workflow_id || !actorLogin) {
core.warning("Skipping daily workflow ET guardrail because the current workflow or actor could not be resolved.");
return;
}

const cutoffMs = Date.now() - DAILY_WORKFLOW_WINDOW_MS;
/** @type {Array<{id:number, html_url:string, created_at:string, conclusion:string}>} */
const candidateRuns = [];
/** @type {Array<any>} */
let runs = [];
let page = 1;
let truncatedByRateLimit = false;
while (page <= MAX_WORKFLOW_RUN_PAGES) {
logDailyGuardrail("Querying completed workflow runs", {
workflowId: currentRun.data.workflow_id,
actorLogin,
page,
perPage: 100,
cutoff: new Date(cutoffMs).toISOString(),
});
const response = await githubClient.rest.actions.listWorkflowRuns({
logDailyGuardrail("Resolved current workflow ET guardrail context", {
owner,
repo,
workflow_id: currentRun.data.workflow_id,
actor: actorLogin,
status: "completed",
per_page: 100,
page,
});
runs = response.data.workflow_runs || [];
logDailyGuardrail("Received workflow runs page", {
page,
runCount: runs.length,
runIds: runs.map(run => run?.id).filter(Boolean),
currentRunId: context.runId,
workflowId: currentRun.data.workflow_id,
workflowName,
actorLogin,
threshold,
rateLimitRemaining: rateLimit.remaining,
rateLimitLimit: rateLimit.limit,
});
if (runs.length === 0) {
break;
const maxInspectableRuns = computeMaxInspectableRuns(rateLimit.remaining);
if (maxInspectableRuns <= 0) {
core.warning(`Skipping daily workflow ET guardrail because the GitHub API rate limit is too low (${rateLimit.remaining} remaining, reserve ${RATE_LIMIT_RESERVE}).`);
return;
}
for (const run of runs) {
if (!run || run.id === context.runId) {
continue;

const cutoffMs = Date.now() - DAILY_WORKFLOW_WINDOW_MS;
/** @type {Array<{id:number, html_url:string, created_at:string, conclusion:string}>} */
const candidateRuns = [];
/** @type {Array<any>} */
let runs = [];
let page = 1;
let truncatedByRateLimit = false;
while (page <= MAX_WORKFLOW_RUN_PAGES) {
logDailyGuardrail("Querying completed workflow runs", {
workflowId: currentRun.data.workflow_id,
actorLogin,
page,
perPage: 100,
cutoff: new Date(cutoffMs).toISOString(),
});
const response = await githubClient.rest.actions.listWorkflowRuns({
owner,
repo,
workflow_id: currentRun.data.workflow_id,
actor: actorLogin,
status: "completed",
per_page: 100,
page,
});
runs = response.data.workflow_runs || [];
logDailyGuardrail("Received workflow runs page", {
page,
runCount: runs.length,
runIds: runs.map(run => run?.id).filter(Boolean),
});
if (runs.length === 0) {
break;
}
const createdAtMs = Date.parse(run.created_at || "");
if (!Number.isFinite(createdAtMs) || createdAtMs < cutoffMs) {
continue;
for (const run of runs) {
if (!run || run.id === context.runId) {
continue;
}
const createdAtMs = Date.parse(run.created_at || "");
if (!Number.isFinite(createdAtMs) || createdAtMs < cutoffMs) {
continue;
}
candidateRuns.push(run);
if (candidateRuns.length >= maxInspectableRuns) {
truncatedByRateLimit = true;
break;
}
}
candidateRuns.push(run);
if (candidateRuns.length >= maxInspectableRuns) {
truncatedByRateLimit = true;
if (candidateRuns.length >= maxInspectableRuns || runs.length < 100) {
break;
}
page += 1;
}
if (candidateRuns.length >= maxInspectableRuns || runs.length < 100) {
break;
}
page += 1;
}
logDailyGuardrail("Prepared candidate workflow runs for artifact inspection", {
candidateRunsCount: candidateRuns.length,
candidateRunIds: candidateRuns.map(run => run.id),
maxInspectableRuns,
truncatedByRateLimit,
});
logDailyGuardrail("Prepared candidate workflow runs for artifact inspection", {
candidateRunsCount: candidateRuns.length,
candidateRunIds: candidateRuns.map(run => run.id),
maxInspectableRuns,
truncatedByRateLimit,
});

const artifactClient = await getArtifactClient();
let totalEffectiveTokens = 0;
/** @type {Array<{id:number, html_url:string, created_at:string, conclusion:string, effective_tokens:number}>} */
const countedRuns = [];
for (const run of candidateRuns) {
if (countedRuns.length >= maxInspectableRuns) {
truncatedByRateLimit = true;
break;
}
try {
const runEffectiveTokens = await getRunEffectiveTokens(artifactClient, run.id, token, owner, repo);
if (runEffectiveTokens <= 0) {
logDailyGuardrail("Skipping run without ET usage artifact data", {
const artifactClient = await getArtifactClient();
let totalEffectiveTokens = 0;
/** @type {Array<{id:number, html_url:string, created_at:string, conclusion:string, effective_tokens:number}>} */
const countedRuns = [];
for (const run of candidateRuns) {
if (countedRuns.length >= maxInspectableRuns) {
truncatedByRateLimit = true;
break;
}
try {
const runEffectiveTokens = await getRunEffectiveTokens(artifactClient, run.id, token, owner, repo);
if (runEffectiveTokens <= 0) {
logDailyGuardrail("Skipping run without ET usage artifact data", {
runId: run.id,
currentEffectiveTokens: totalEffectiveTokens,
threshold,
});
continue;
}
totalEffectiveTokens += runEffectiveTokens;
countedRuns.push({
id: run.id,
html_url: run.html_url || "",
created_at: run.created_at || "",
conclusion: run.conclusion || "",
effective_tokens: runEffectiveTokens,
});
logDailyGuardrail("Updated current ET state", {
runId: run.id,
runEffectiveTokens,
currentEffectiveTokens: totalEffectiveTokens,
threshold,
countedRunIds: countedRuns.map(item => item.id),
});
continue;
} catch (error) {
core.warning(`Failed to inspect token usage for run ${run.id}: ${getErrorMessage(error)}`);
}
totalEffectiveTokens += runEffectiveTokens;
countedRuns.push({
id: run.id,
html_url: run.html_url || "",
created_at: run.created_at || "",
conclusion: run.conclusion || "",
effective_tokens: runEffectiveTokens,
});
logDailyGuardrail("Updated current ET state", {
runId: run.id,
runEffectiveTokens,
currentEffectiveTokens: totalEffectiveTokens,
threshold,
countedRunIds: countedRuns.map(item => item.id),
});
} catch (error) {
core.warning(`Failed to inspect token usage for run ${run.id}: ${getErrorMessage(error)}`);
}
}

core.setOutput("daily_effective_workflow_total_effective_tokens", String(totalEffectiveTokens));
core.setOutput("daily_effective_workflow_threshold", String(threshold));
core.setOutput("daily_effective_workflow_total_effective_tokens", String(totalEffectiveTokens));
core.setOutput("daily_effective_workflow_threshold", String(threshold));

/** @type {{candidateRunsCount:number,inspectedRunsCount:number,truncatedByRateLimit:boolean}} */
const summaryMeta = {
candidateRunsCount: candidateRuns.length,
inspectedRunsCount: countedRuns.length,
truncatedByRateLimit,
};
logDailyGuardrail("Completed ET inspection window", {
candidateRunsCount: summaryMeta.candidateRunsCount,
inspectedRunsCount: summaryMeta.inspectedRunsCount,
countedRunIds: countedRuns.map(run => run.id),
currentEffectiveTokens: totalEffectiveTokens,
threshold,
exceeded: totalEffectiveTokens > threshold,
});

/** @type {{candidateRunsCount:number,inspectedRunsCount:number,truncatedByRateLimit:boolean}} */
const summaryMeta = {
candidateRunsCount: candidateRuns.length,
inspectedRunsCount: countedRuns.length,
truncatedByRateLimit,
};
logDailyGuardrail("Completed ET inspection window", {
candidateRunsCount: summaryMeta.candidateRunsCount,
inspectedRunsCount: summaryMeta.inspectedRunsCount,
countedRunIds: countedRuns.map(run => run.id),
currentEffectiveTokens: totalEffectiveTokens,
threshold,
exceeded: totalEffectiveTokens > threshold,
});
if (totalEffectiveTokens <= threshold) {
await appendDailyEffectiveWorkflowSummary(workflowName, actorLogin, threshold, countedRuns, rateLimit, summaryMeta);
core.info(`Daily workflow ET guardrail not exceeded (${totalEffectiveTokens}/${threshold}).`);
return;
}

if (totalEffectiveTokens <= threshold) {
core.setOutput("daily_effective_workflow_exceeded", "true");
await appendDailyEffectiveWorkflowSummary(workflowName, actorLogin, threshold, countedRuns, rateLimit, summaryMeta);
core.info(`Daily workflow ET guardrail not exceeded (${totalEffectiveTokens}/${threshold}).`);
return;
core.warning(`Daily workflow ET guardrail exceeded for ${workflowName}: ${totalEffectiveTokens}/${threshold}.`);
} catch (error) {
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/diagnose] The outer catch treats all errors — including permanent ones like missing permissions or misconfigured tokens — the same as transient network blips. A persistent configuration error will silently bypass the guardrail on every run, with only a core.warning in the job log as the signal.

💡 Suggestion

The current design is explicitly documented as intentional (safe bypass > broken activation), so this is not a blocker. However, consider adding a core.error() call alongside the warning for non-retryable error classes, or emitting a core.summary line so the bypass is visible in the Actions run summary rather than only in the raw log:

} catch (error) {
  core.warning(`Daily workflow ET guardrail encountered an unexpected error and will be skipped: ${getErrorMessage(error)}`);
  // Make the bypass visible in the run summary so operators notice persistent failures
  await core.summary.addRaw(`> [!WARNING]\n> ET guardrail skipped due to error: ${getErrorMessage(error)}`).write().catch(() => {});
}

This preserves the safe-bypass contract while making a stuck guardrail discoverable without digging into logs.

// Treat any unexpected error as a non-blocking skip so the step never fails the
// activation job. The output stays at the default "false", allowing the agent to
// run. The guardrail is effectively bypassed for this invocation.
core.warning(`Daily workflow ET guardrail encountered an unexpected error and will be skipped: ${getErrorMessage(error)}`);
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

Silent bypass is indistinguishable from a clean pass in workflow outputs. When the catch fires, daily_effective_workflow_exceeded stays at "false" — the same value it has when the guardrail genuinely evaluated to "not exceeded". Any downstream observability (dashboards, audits) that reads this output cannot tell whether the guardrail passed or failed silently. The only signal is a transient ::warning:: in the step log that is easily missed.

💡 Suggested improvement

Consider adding a dedicated output that makes the skip explicit:

} catch (error) {
  core.warning(`Daily workflow ET guardrail encountered an unexpected error and will be skipped: ${getErrorMessage(error)}`);
  core.setOutput("daily_effective_workflow_guardrail_error", "true"); // new
}

Downstream steps and dashboards can then distinguish three states:

  • exceeded=true → guardrail fired, block the agent
  • exceeded=false, guardrail_error=false → evaluated cleanly, under budget
  • exceeded=false, guardrail_error=true → evaluation failed, agent allowed as safe fallback

This is especially important for post-incident cost audits where "did the guardrail skip silently?" is a key question.

}

core.setOutput("daily_effective_workflow_exceeded", "true");
await appendDailyEffectiveWorkflowSummary(workflowName, actorLogin, threshold, countedRuns, rateLimit, summaryMeta);
core.warning(`Daily workflow ET guardrail exceeded for ${workflowName}: ${totalEffectiveTokens}/${threshold}.`);
}

module.exports = {
Expand Down
60 changes: 60 additions & 0 deletions actions/setup/js/check_daily_effective_workflow_guardrail.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,64 @@ describe("check_daily_effective_workflow_guardrail", () => {
expect(markdown).toContain("Stopped early to preserve GitHub API rate limit headroom");
expect(markdown).not.toContain("Guardrail issue:");
});

it("main() does not fail the step when GitHub API calls throw", async () => {
// Simulate a scenario where the GitHub API throws during workflow run lookup.
// The step should catch the error and NOT rethrow it, keeping daily_effective_workflow_exceeded at "false".
const coreOutputs = {};
const coreWarnings = [];
const mockCore = {
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

Incomplete mockCore stub may cause vacuously-passing tests. mockCore is missing error, debug, and notice methods. If any production code path inside the try block calls one of them, the TypeError: core.xxx is not a function will be caught by the outer catch and produce a warning like "Daily workflow ET guardrail encountered an unexpected error and will be skipped: core.error is not a function" — which matches the regex /unexpected error.*skipped/i. The test then passes while hiding the actual regression.

💡 Suggested fix

Add no-op stubs for the missing methods:

const mockCore = {
  setOutput: (key, value) => { coreOutputs[key] = value; },
  info: () => {},
  warning: msg => coreWarnings.push(msg),
  error: () => {},      // prevent silent swallow of TypeError
  debug: () => {},
  notice: () => {},
};

This ensures that any accidental call to these methods causes a type-safe no-op rather than a silently-caught TypeError that fools the regex assertion.

setOutput: (key, value) => {
coreOutputs[key] = value;
},
info: () => {},
warning: msg => coreWarnings.push(msg),
};

const mockGithub = {
rest: {
rateLimit: {
get: async () => {
throw new Error("API rate limit exceeded");
},
},
actions: {
getWorkflowRun: async () => {
throw new Error("Network error");
},
listWorkflowRuns: async () => {
throw new Error("Unexpected error");
},
},
},
};

const mockContext = {
repo: { owner: "test-owner", repo: "test-repo" },
runId: 42,
};

// Inject globals so the module can use them
global.core = mockCore;
global.github = mockGithub;
global.context = mockContext;

process.env.GH_AW_MAX_DAILY_EFFECTIVE_TOKENS = "1000000";
process.env.GH_AW_GITHUB_TOKEN = "fake-token";

try {
// Should resolve without throwing even though the API calls throw
await expect(exports.main()).resolves.toBeUndefined();
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/tdd] Assertions inside the try block mean that if the first expect throws, later assertions are silently skipped, reducing test confidence.

💡 Suggested refactor

Move the await expect(...) call outside the try/finally (or keep the try/finally only for cleanup), so Jest sees all assertion failures independently:

let threw = false;
try {
  await exports.main();
} catch (err) {
  threw = true;
} finally {
  delete global.core;
  delete global.github;
  delete global.context;
  delete process.env.GH_AW_MAX_DAILY_EFFECTIVE_TOKENS;
  delete process.env.GH_AW_GITHUB_TOKEN;
}
expect(threw).toBe(false);
expect(coreOutputs["daily_effective_workflow_exceeded"]).toBe("false");
expect(coreWarnings.some(w => /unexpected error.*skipped/i.test(w))).toBe(true);

This way all three assertions are always evaluated regardless of which one fails.

// The default "false" output must be set
expect(coreOutputs["daily_effective_workflow_exceeded"]).toBe("false");
// A warning must be emitted describing the error
expect(coreWarnings.some(w => /unexpected error.*skipped/i.test(w))).toBe(true);
} finally {
delete global.core;
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

Global state deleted rather than restored — fragile isolation. delete global.core permanently removes the property rather than restoring whatever was there before. If a future beforeEach or test-helper ever pre-populates these globals, this finally block will silently trash them for subsequent tests, causing confusing cross-test contamination.

💡 Suggested fix

Save and restore:

const savedCore = global.core;
const savedGithub = global.github;
const savedContext = global.context;

global.core = mockCore;
global.github = mockGithub;
global.context = mockContext;

try {
  // assertions...
} finally {
  if (savedCore === undefined) delete global.core; else global.core = savedCore;
  if (savedGithub === undefined) delete global.github; else global.github = savedGithub;
  if (savedContext === undefined) delete global.context; else global.context = savedContext;
  delete process.env.GH_AW_MAX_DAILY_EFFECTIVE_TOKENS;
  delete process.env.GH_AW_GITHUB_TOKEN;
}

Alternatively, use afterEach for cleanup so Vitest handles the lifecycle correctly.

delete global.github;
delete global.context;
delete process.env.GH_AW_MAX_DAILY_EFFECTIVE_TOKENS;
delete process.env.GH_AW_GITHUB_TOKEN;
}
});
});
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/tdd] The refactor wraps the entire success path in the outer try/catch too, but there's no regression test verifying that a normal execution where the threshold is exceeded still sets daily_effective_workflow_exceeded to "true". A logic bug inside the try block (e.g., an early return or swallowed error) could silently prevent the output from being set without any test catching it.

💡 What to add

Add a companion test that mocks a successful API response returning runs whose total ET exceeds the threshold, and asserts:

it("main() sets daily_effective_workflow_exceeded=true when threshold is exceeded", async () => {
  // ... mock APIs returning one run with runEffectiveTokens > threshold ...
  await exports.main();
  expect(coreOutputs["daily_effective_workflow_exceeded"]).toBe("true");
});

This guards against the outer catch accidentally consuming an exception that would have set the output.

Loading
Loading