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"); |
There was a problem hiding this comment.
Addressed in commit c2a8e33 — daily_ai_credits_guardrail_status is now wired as a 4th activation-job output in compiler_activation_outputs.go alongside the three existing guardrail outputs, golden test snapshots updated, and TestDailyAICWorkflowGuardrailInCompiledWorkflow asserts the new output.
![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.
There was a problem hiding this comment.
Added JSDoc to isStructuralGuardrailError in commit 4159469 explaining that 404 is the only status treated as structural, and that other 4xx errors (403, 422, etc.) are intentionally treated as transient because they may resolve on retry or credential refresh.
| */ | ||
| 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.
There was a problem hiding this comment.
Added @typedef {'workflow_id' | 'repo_workflow_name_fallback'} WorkflowRunLookupMode in commit 4159469 and updated the @param and @returns annotations on listCompletedWorkflowRunsPage to use the named type.
| ...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.
There was a problem hiding this comment.
Fixed in commit 4159469. The fallback path now returns oldestUnfilteredCreatedAt (the created_at of the last unfiltered run on the page). After the per-run loop in the outer pagination, a new check tests this value against cutoffMs and sets reachedCutoff = true when the oldest unfiltered run predates the window — stopping pagination early even when no runs on the page match the workflow name.
| 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.
There was a problem hiding this comment.
Separated the two arms in commit 4159469: !hasHttpStatus(error, 404) rethrows non-404 errors, then !workflowName rethrows with a comment explaining that a 404 with no explicit workflow name means no meaningful name is available for the fallback filter, so we rethrow to let the outer catch classify it as a structural error.
|
|
||
| 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.
There was a problem hiding this comment.
Added the test in commit 4159469: page 1 returns 100 unrelated runs (sourceRunCount = 100, loop continues), page 2 returns workflow_runs: [] (sourceRunCount === 0, fast-exit fires). Asserts listWorkflowRunsForRepoCalls === 2 and that daily_ai_credits_guardrail_status === 'under_budget'.
| 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.
There was a problem hiding this comment.
Fixed and tested in commit 4159469. Introduced workflowFilterName = process.env.GH_AW_WORKFLOW_NAME || "" — this is passed to listCompletedWorkflowRunsPage instead of the derived workflowName, so when GH_AW_WORKFLOW_NAME is absent, !workflowName is true in the catch block and the fallback is skipped. The new test asserts daily_ai_credits_guardrail_status === 'structural_error' and listWorkflowRunsForRepoCalls === 0 when GH_AW_WORKFLOW_NAME is not set.
|
@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.
|
- Add JSDoc to isStructuralGuardrailError explaining why only 404 is structural - Add @typedef WorkflowRunLookupMode for the two lookup mode string literals - Return oldestUnfilteredCreatedAt from fallback path; use it in outer loop to terminate pagination early when all remaining unfiltered runs predate the cutoff - Separate the !hasHttpStatus(404) and !workflowName catch arms with an explanatory comment for the no-name rethrow case - Introduce workflowFilterName = GH_AW_WORKFLOW_NAME || '' so the fallback is only attempted when an explicit name is available to filter by; workflowID and 'workflow' defaults are display-only and would not match run.name - Add test: fallback stops on sourceRunCount === 0 empty-page fast-exit - Add test: no GH_AW_WORKFLOW_NAME + 404 → structural_error, fallback never called Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all 6
|
|
@copilot run pr-finisher skill |
…LookupMode type Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit — TypeScript was widening |
The CI failure (TS2322 on CI is now running against the fixed HEAD — PR finisher summary:
Actions taken this run: Confirmed no uncommitted changes; ran full local validation suite. Hand-off: Wait for the in-progress CI jobs to complete. If all pass, resolve the 7 review threads in the GitHub UI and merge. |
|
@copilot sous-chef triage: The latest fixes are on the branch and checks are green; please run the
|
PR finisher complete. All three merge-ready conditions checked:
Still needed (human): Resolve the 7 review threads in the GitHub UI and merge. |
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
branch-refresh requested by PR Sous Chef run: https://github.com/github/gh-aw/actions/runs/30830929419