Skip to content

Add structured diagnostics to the daily workflow ET guardrail #36164

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 7 commits into from
Jun 1, 2026
Merged

Add structured diagnostics to the daily workflow ET guardrail #36164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a8e35e1
Add daily ET guardrail logging
Copilot Jun 1, 2026
444d873
Format guardrail logging test
Copilot Jun 1, 2026
5aeda1f
Harden guardrail logging test
Copilot Jun 1, 2026
c47ca96
Resolve guardrail logging review nits
Copilot Jun 1, 2026
9f4fccc
Harden daily ET logging helpers
Copilot Jun 1, 2026
7e37cb8
Tighten guardrail artifact checks
Copilot Jun 1, 2026
95f8426
Polish guardrail log fields
Copilot Jun 1, 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
118 changes: 116 additions & 2 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 @@ -26,6 +26,35 @@ async function getArtifactClient() {
return new DefaultArtifactClient();
}

/**
* @param {string} message
* @param {Record<string, unknown>} [details]
* @returns {string}
*/
function formatDailyGuardrailLogMessage(message, details) {
if (!details || Object.keys(details).length === 0) {
return `[daily-workflow-et] ${message}`;
}
let serializedDetails = "";
try {
serializedDetails = JSON.stringify(details);
} catch {
serializedDetails = JSON.stringify({ error: "failed to serialize log details" });
}
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/improve-codebase-architecture] The catch block in formatDailyGuardrailLogMessage swallows the original serialisation error completely, making it hard to know why serialisation failed during debugging.

💡 Suggested improvement 
} catch (err) {
  serializedDetails = JSON.stringify({
    error: "failed to serialize log details",
    reason: err instanceof Error ? err.message : String(err),
  });
}

Adding reason preserves the intent of defensive logging while still surfacing enough context to diagnose circular-reference or BigInt serialisation failures.

return `[daily-workflow-et] ${message}: ${serializedDetails}`;
}

/**
* Emit a consistently prefixed daily workflow ET diagnostic log line.
*
* @param {string} message
* @param {Record<string, unknown>} [details]
* @returns {void}
*/
function logDailyGuardrail(message, details) {
core.info(formatDailyGuardrailLogMessage(message, details));
}

/**
* @returns {boolean}
*/
Expand Down Expand Up @@ -66,12 +95,34 @@ async function getRunEffectiveTokens(artifactClient, runId, token, owner, repo)
repositoryName: repo,
},
});
const artifactSummaries = artifacts.map(item => ({ id: item?.id ?? null, name: item?.name || "" }));
logDailyGuardrail("Listed workflow artifacts", {
runId,
artifactCount: artifacts.length,
artifacts: artifactSummaries,
});
Comment on lines +98 to +103

const artifact = artifacts.find(item => matchesGuardrailArtifactName(item.name));
const artifact = artifacts.find(item => item?.name && matchesGuardrailArtifactName(item.name));
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/tdd] Changing the predicate from matchesGuardrailArtifactName(item.name) to item?.name && matchesGuardrailArtifactName(item.name) is a behaviour change bundled into logging work — artifacts with a falsy name are now silently skipped. No test covers this path.

💡 Suggested test

Add a case where listArtifacts returns [{ id: 1, name: "" }, { id: 2, name: "guardrail-artifact" }] and assert only the second artifact is inspected. This prevents the guard from regressing silently.

if (!artifact) {
logDailyGuardrail("No matching guardrail artifact found", {
runId,
availableArtifacts: artifactSummaries,
});
return 0;
}
if (!artifact.id) {
logDailyGuardrail("Skipping guardrail artifact without an id", {
runId,
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/tdd] The new artifact.id null-guard is a behaviour change with no test coverage — an artifact whose name matches but whose id is falsy will silently yield 0 ET instead of proceeding to downloadArtifact.

💡 Suggested test

Add a unit test that stubs listArtifacts to return [{ id: undefined, name: "guardrail-artifact" }] and asserts that getRunEffectiveTokens returns 0 and emits the "Skipping guardrail artifact without an id" log message.

This pins both the guard and the new structured log emitted on that path.

artifactName: artifact.name,
});
return 0;
}

logDailyGuardrail("Selected guardrail artifact", {
runId,
artifactId: artifact.id,
artifactName: artifact.name,
});
const downloadRoot = fs.mkdtempSync(path.join(os.tmpdir(), `gh-aw-daily-guardrail-${runId}-`));
const download = await artifactClient.downloadArtifact(artifact.id, {
path: downloadRoot,
Expand All @@ -84,7 +135,20 @@ async function getRunEffectiveTokens(artifactClient, runId, token, owner, repo)
});

const tokenUsageFile = findTokenUsageFile(download.downloadPath || downloadRoot);
return sumEffectiveTokensFromTokenUsageFile(tokenUsageFile);
logDailyGuardrail("Downloaded guardrail artifact", {
runId,
artifactId: artifact.id,
artifactName: artifact.name,
downloadPath: download.downloadPath || downloadRoot,
tokenUsageFile,
});
const effectiveTokens = sumEffectiveTokensFromTokenUsageFile(tokenUsageFile);
logDailyGuardrail("Computed run ET from artifact", {
runId,
artifactId: artifact.id,
effectiveTokens,
});
return effectiveTokens;
}

/**
Expand Down Expand Up @@ -251,6 +315,17 @@ async function main() {
return;
}

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}).`);
Expand All @@ -265,6 +340,13 @@ async function main() {
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,
Expand All @@ -275,6 +357,11 @@ async function main() {
page,
});
runs = response.data.workflow_runs || [];
logDailyGuardrail("Received workflow runs page", {
page,
runCount: runs.length,
runIds: runs.map(run => run?.id).filter(Boolean),
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/zoom-out] runIds: runs.map(run => run?.id).filter(Boolean) can produce an array of up to 100 elements per paginated page. With MAX_WORKFLOW_RUN_PAGES pages this could generate megabytes of log output in a busy repo, and the IDs are already captured in the subsequent "Prepared candidate workflow runs" log.

💡 Suggested fix

Replace the full ID array with a count, matching the style used in nearby logs:

logDailyGuardrail("Received workflow runs page", {
  page,
  runCount: runs.length,
  // runIds omitted — captured in candidateRunIds after filtering
});

If per-page IDs are genuinely useful, consider capping the array: runs.slice(0, 20).map(r => r?.id).

});
Comment on lines +360 to +364
if (runs.length === 0) {
break;
}
Expand All @@ -297,6 +384,12 @@ async function main() {
}
page += 1;
}
logDailyGuardrail("Prepared candidate workflow runs for artifact inspection", {
candidateRunsCount: candidateRuns.length,
candidateRunIds: candidateRuns.map(run => run.id),
maxInspectableRuns,
truncatedByRateLimit,
});
Comment on lines +387 to +392

const artifactClient = await getArtifactClient();
let totalEffectiveTokens = 0;
Expand All @@ -310,6 +403,11 @@ async function main() {
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;
Expand All @@ -320,6 +418,13 @@ async function main() {
conclusion: run.conclusion || "",
effective_tokens: runEffectiveTokens,
});
logDailyGuardrail("Updated current ET state", {
runId: run.id,
runEffectiveTokens,
currentEffectiveTokens: totalEffectiveTokens,
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

O(n2) log output inside the per-run loop: countedRunIds: countedRuns.map(item => item.id) remaps the full (and ever-growing) countedRuns array on every iteration, producing quadratic work and log volume as counted runs accumulate.

💡 Suggested fix

Log only the most recently added run ID, not the full list on each iteration:

logDailyGuardrail("Updated current ET state", {
  runId: run.id,
  runEffectiveTokens,
  currentEffectiveTokens: totalEffectiveTokens,
  threshold,
  countedRunsCount: countedRuns.length,  // just the count
});

The full countedRunIds list is already emitted once at the end in the "Completed ET inspection window" log, so repeating it on every iteration adds no diagnostic value and compounds the log-volume concern already flagged in existing review threads.

threshold,
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/zoom-out] countedRunIds is re-serialised on every iteration of the inspection loop. With up to maxInspectableRuns runs, the emitted log volume grows O(n2) — the final log line could list dozens of IDs already reported in earlier iterations.

💡 Suggested fix

Log only the newly-added run ID and the current count rather than the full accumulated array each time:

logDailyGuardrail("Updated current ET state", {
  runId: run.id,
  runEffectiveTokens,
  currentEffectiveTokens: totalEffectiveTokens,
  threshold,
  countedRunCount: countedRuns.length,  // count, not the full array
});

The complete list is already captured in the final "Completed ET inspection window" log, so the per-iteration array is redundant.

countedRunIds: countedRuns.map(item => item.id),
});
} catch (error) {
core.warning(`Failed to inspect token usage for run ${run.id}: ${getErrorMessage(error)}`);
}
Expand All @@ -334,6 +439,14 @@ async function main() {
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);
Expand All @@ -355,4 +468,5 @@ module.exports = {
calculateDailyEffectiveWorkflowStats,
computeMaxInspectableRuns,
renderDailyEffectiveWorkflowSummary,
formatDailyGuardrailLogMessage,
};
16 changes: 16 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 @@ -66,6 +66,22 @@ describe("check_daily_effective_workflow_guardrail", () => {
expect(exports.computeMaxInspectableRuns(120)).toBeGreaterThan(0);
});

it("formats structured daily ET log messages", () => {
const message = exports.formatDailyGuardrailLogMessage("Resolved current workflow ET guardrail context", {
currentRunId: 123,
workflowId: 456,
currentEffectiveTokens: 789,
});
const prefix = "[daily-workflow-et] Resolved current workflow ET guardrail context: ";
expect(message).toContain(prefix);
expect(JSON.parse(message.slice(prefix.length))).toEqual({
currentRunId: 123,
workflowId: 456,
currentEffectiveTokens: 789,
});
expect(exports.formatDailyGuardrailLogMessage("Completed ET inspection window")).toBe("[daily-workflow-et] Completed ET inspection window");
});
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

[/tdd] logDailyGuardrail is not exported and therefore cannot be directly unit-tested. The two new guard branches (item?.name && and !artifact.id) are also not covered. The single new test only verifies the formatter, leaving the runtime paths dark.

💡 Suggestions
  1. Export logDailyGuardrail (or extract its core.info call behind a seam) so tests can assert it was called with the right message.
  2. Add tests for the guard branches in a getRunEffectiveTokens-level test: one where artifact.name is "" and one where artifact.id is undefined, each asserting the function returns 0.
  3. Split the two formatDailyGuardrailLogMessage assertions into separate it() blocks — one for the details path, one for the no-details path — so failure messages are unambiguous.


it("renders a daily ET details summary with stats and prior runs", () => {
const markdown = exports.renderDailyEffectiveWorkflowSummary(
"Nightly triage",
Expand Down
Loading