Handle required-workflow daily AIC guardrail lookups and expose structural failure state#49961
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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
Adds required-workflow fallback handling and distinct guardrail execution statuses.
Changes:
- Falls back to repository run history on workflow-specific 404s.
- Distinguishes structural, transient, skipped, disabled, and budget states.
- Adds focused regression tests for fallback pagination and structural errors.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/check_daily_aic_workflow_guardrail.cjs |
Implements fallback lookup and status reporting. |
actions/setup/js/check_daily_aic_workflow_guardrail.test.cjs |
Tests fallback and status behavior. |
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
| core.setOutput("daily_ai_credits_exceeded", "false"); | ||
| core.setOutput("daily_ai_credits_total_effective_tokens", ""); | ||
| core.setOutput("daily_ai_credits_threshold", ""); | ||
| core.setOutput("daily_ai_credits_guardrail_status", "not_run"); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is correct and well-structured.
hasHttpStatussafely handles botherror.statusanderror.response.statuserror shapes.sourceRunCounttracks the unfiltered API page size in fallback mode, so pagination correctly continues through pages where name-filtering yields zero matches — exactly the required behavior.daily_ai_credits_guardrail_statusconsistently covers all exit paths:not_run,disabled,skipped,under_budget,exceeded,transient_error,structural_error.isStructuralGuardrailErrorlimits thestructural_errorclassification to 404s only, keeping other faults astransient_error.- The two new tests cover both the required-workflow fallback path and the double-404 structural error path with appropriate assertions.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.6 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 84/100 — Excellent
📊 Metrics (37 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a few correctness and coverage gaps.
📋 Key Themes & Highlights
Key Themes
isStructuralGuardrailErrorscope: Only HTTP 404 is treated as structural; other permanent 4xx errors (403, 410) will be misclassified as transient. The intent should be documented or the function renamed.- Fallback pagination efficiency: In
repo_workflow_name_fallbackmode,sourceRunCounttracks unfiltered page size, which can keep pagination alive long after all matching runs have been scanned past the cutoff window. - Missing test coverage: The
sourceRunCount === 0early-exit path in fallback mode, and the no-workflowName+ 404 path, are not directly tested. - Untyped
lookupModestring: A JSDoc@typedefunion would eliminate the typo risk.
Positive Highlights
- ✅ Excellent use of two-level error detection (
hasHttpStatus) covering both direct and nested Octokit error shapes. - ✅ Fallback is transparent: the caller learns the mode transition via the returned
lookupModeand the log message is clear. - ✅ Seven-state
daily_ai_credits_guardrail_statusoutput is a great observability improvement — callers can now route on structural vs transient failure without re-parsing warning text. - ✅ The new
httpErrortest helper is clean and reusable across the test file.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 50 AIC · ⌖ 12.1 AIC · ⊞ 7.1K
Comment /matt to run again
| * @returns {boolean} | ||
| */ | ||
| function isStructuralGuardrailError(error) { | ||
| return hasHttpStatus(error, 404); |
There was a problem hiding this comment.
[/diagnosing-bugs] isStructuralGuardrailError currently maps exclusively to HTTP 404. Other permanent failures (e.g. 403 on missing scope) will be misclassified as transient_error, causing the guardrail to keep failing open permanently.
💡 Suggestion
Either rename to is404Error for precision, or document explicitly in the JSDoc why only 404 is structural and all other 4xx errors are intentionally treated as transient.
/**
* Returns true when the error is a permanent HTTP 404 — the only status
* treated as structural. Other 4xx errors are treated as transient because
* they may resolve on retry or credential refresh.
*/
function isStructuralGuardrailError(error) {
return hasHttpStatus(error, 404);
}@copilot please address this.
| */ | ||
| async function listCompletedWorkflowRunsPage(githubClient, params) { | ||
| const { owner, repo, workflowId, workflowName, page, perPage, lookupMode } = params; | ||
| if (lookupMode === "repo_workflow_name_fallback") { |
There was a problem hiding this comment.
[/codebase-design] The lookupMode parameter is a free-form string ("workflow_id" / "repo_workflow_name_fallback"). Using an untyped string literal makes callers fragile to typos and means the exhaustiveness of the switch isn't enforced.
💡 Suggestion
Define a JSDoc @typedef for the mode union so tooling catches invalid values at call sites:
/** `@typedef` {'workflow_id' | 'repo_workflow_name_fallback'} WorkflowRunLookupMode */Then annotate the param and internal variable accordingly.
@copilot please address this.
| ...response.data, | ||
| workflow_runs: filteredRuns, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
[/diagnosing-bugs] When in fallback mode, sourceRunCount is set to allRuns.length (total unfiltered runs), but the outer loop uses sourceRunCount < 100 to decide whether to stop paginating. If the repo has exactly 100 runs on a page but only 0 match the workflow name, the loop will correctly continue — but if the last page of unfiltered results is full (100 items) yet all filtered results are exhausted, pagination never terminates early.
💡 Example scenario
- Repo has 500 runs, none named
workflowNameon pages 3-5. - Pages 1-2 each return 100 unfiltered runs, some matching.
- Pages 3-5 return 100 unfiltered runs, 0 matching — loop keeps going until
MAX_WORKFLOW_RUN_PAGES.
This wastes rate-limit budget. Consider stopping when the oldest run on a page predates the cutoff regardless of sourceRunCount.
@copilot please address this.
| page, | ||
| perPage, | ||
| lookupMode: "repo_workflow_name_fallback", | ||
| }); |
There was a problem hiding this comment.
[/diagnosing-bugs] When workflowName is an empty string or undefined and a 404 is thrown, the condition !workflowName is truthy, so the error is re-thrown directly. That means the outer catch will classify it as structural_error (since it's a 404), even though the root cause is a missing workflow name — a configuration issue, not a required-workflow shape.
💡 Suggestion
Distinguish the two 404 cases explicitly:
if (!hasHttpStatus(error, 404)) {
throw error;
}
if (!workflowName) {
// 404 with no name to fall back on — rethrow as a structural error
throw error;
}
// ... proceed with fallbackAdd a comment explaining why a missing workflowName means we cannot fall back, so future maintainers don't collapse these two arms.
@copilot please address this.
|
|
||
| const mockGithub = { | ||
| rest: { | ||
| rateLimit: { |
There was a problem hiding this comment.
[/tdd] The fallback test asserts listWorkflowRunsForRepoCalls === 2, which implies page 1 returns 100 unfiltered runs (all unrelated) and page 2 returns the matching run. But the test never asserts that reachedCutoff or the cutoff-based loop-termination condition fired — meaning the loop only stops because page 2 returns fewer than 100 runs. The empty-page fast-exit (sourceRunCount === 0) path is untested in the fallback scenario.
💡 Suggestion
Add a test where the fallback runs across a page that has zero unfiltered runs (workflow_runs: []) and assert that the loop terminates without querying a further page. This exercises the sourceRunCount === 0 early-exit branch under fallback mode.
@copilot please address this.
| global.github = mockGithub; | ||
| global.context = { repo: { owner: "test-owner", repo: "test-repo" }, runId: 42 }; | ||
|
|
||
| process.env.GH_AW_MAX_DAILY_AI_CREDITS = "10"; |
There was a problem hiding this comment.
[/tdd] The structural-error test sets GH_AW_WORKFLOW_NAME so both listWorkflowRuns and listWorkflowRunsForRepo get called. However there is no test covering the case where GH_AW_WORKFLOW_NAME is absent (empty) and listWorkflowRuns returns 404 — which would take the !workflowName branch in listCompletedWorkflowRunsPage and skip the fallback entirely. That branch currently ends in a rethrow, so the outer catch should still produce structural_error, but it is not explicitly verified.
💡 Suggestion
Add a test: no GH_AW_WORKFLOW_NAME, listWorkflowRuns throws 404, assert daily_ai_credits_guardrail_status === 'structural_error' and that listWorkflowRunsForRepo is never called.
@copilot please address this.
|
@copilot sous-chef triage: Failed checks: Please refresh the branch if needed and then run the
|
🔍 PR TriageCategory: bug · Risk: medium · Total score: 65/100
Fixes a structural failure-detection gap in the daily AI Credits guardrail for required workflows (404 fallback + explicit status states). Includes regression tests. CI pending (1 in-progress check). Recommended action:
|
…tputs - Add daily_ai_credits_guardrail_status to compiler_activation_outputs.go so downstream jobs can observe structural vs transient failure state - Update 13 golden test snapshots with the new output in alphabetical position - Add test assertion in daily_aic_workflow_guardrail_test.go - Recompile 268 workflow lock files to include the new activation output Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
The |
|
@copilot sous-chef triage: Please refresh the branch if needed and then run the skill.
|
Organization-level required workflows can execute in a repository without being members of that repository’s workflow collection. In that case, the daily AI Credits guardrail resolved the current run successfully, then 404ed when querying run history by
workflow_id, permanently failing open and making that structural failure indistinguishable from a transient skip.Run-history lookup fallback
workflow_idquery as the primary path.404, fall back to repository-level run listing and filter by workflow name, so required-workflow injection still produces a usable 24h run window.Structural vs transient failure signaling
daily_ai_credits_guardrail_statusoutput to distinguish:under_budgetexceededtransient_errorstructural_errorskipped/disabled/not_runFocused guardrail regression coverage
getWorkflowRun()succeeds for the current runlistWorkflowRuns({ workflow_id })returns404run: https://github.com/github/gh-aw/actions/runs/30815680828
Run: https://github.com/github/gh-aw/actions/runs/30820387900