fix(aic-guardrail): widen fallback search window and add in-loop rate-limit backoff

[Copilot]

Under high run frequency with a cold cache, N concurrent activations each calling listArtifacts per prior run produces an O(N×M) fan-out against the shared GITHUB_TOKEN core budget — in production this accounted for 99.5% of all /actions/runs/:run_id/artifacts calls in one hour, starving unrelated critical steps (e.g. attestation persistence) with HTTP 403.

Changes

restore_aic_usage_cache_fallback.cjs — wider cross-branch artifact search (AC2)

• MAX_RUNS_TO_SEARCH: 5 → 30
• pull_request branches never hit a per-branch actions/cache entry from another PR, making the artifact fallback their only warm-cache path. The previous 5-run window missed on active repos; 30 covers the same single listWorkflowRuns page at no additional cost.

check_daily_aic_workflow_guardrail.cjs — in-loop rate-limit re-check (AC3, AC4)

• Adds RATE_LIMIT_RECHECK_INTERVAL = 10: after every 10 API operations consumed by cache misses (ESTIMATED_API_OPERATIONS_PER_RUN = 2 → fires every 5 cache-miss runs), calls getCoreRateLimitSnapshot and breaks early if remaining ≤ RATE_LIMIT_RESERVE.
• The existing upfront budget check (computeMaxInspectableRuns) is computed once from a stale snapshot; concurrent activations each see the same snapshot and collectively overspend it. The in-loop re-check reads the live shared quota and stops as soon as it's actually at risk.

if (apiCallsInLoop > 0 && apiCallsInLoop % RATE_LIMIT_RECHECK_INTERVAL === 0) {
  const midLoopRL = await getCoreRateLimitSnapshot(githubClient);
  if (midLoopRL.remaining <= RATE_LIMIT_RESERVE) {
    truncatedByRateLimit = true;
    break;
  }
}
// Only incremented on cache misses — cache hits cost no API operations
apiCallsInLoop += ESTIMATED_API_OPERATIONS_PER_RUN;

Test coverage

• New test: 15 cache-miss candidate runs, rate-limit mock drops below reserve on the in-loop re-check call; asserts getRunAIC is called exactly 5 times before the loop stops.

[github-actions]

⚠️ PR Code Quality Reviewer failed during code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[Copilot]

Pull request overview

This PR updates the AIC (AI Credits) guardrail and its supporting cache-fallback behavior to reduce GitHub API rate-limit exhaustion under high concurrency by (1) searching further back for cache artifacts and (2) periodically re-checking live core rate-limit headroom inside the inspection loop and stopping early when the shared reserve is at risk.

Changes:

• Widen restore_aic_usage_cache_fallback.cjs artifact fallback search window from 5 → 30 recent runs to improve cache hit rate, especially for pull_request branches.
• Add in-loop rate-limit re-checking to check_daily_aic_workflow_guardrail.cjs (every 10 estimated cache-miss API ops) to stop inspection when remaining <= RATE_LIMIT_RESERVE.
• Add a new unit test covering early loop termination triggered by the in-loop rate-limit re-check.

Show a summary per file

File	Description
pkg/workflow/data/action_pins.json	Large JSON reformatting change (appears non-functional); flagged as out-of-scope vs PR description.
pkg/actionpins/data/action_pins.json	Large JSON reformatting change (appears non-functional); flagged as out-of-scope vs PR description.
actions/setup/js/restore_aic_usage_cache_fallback.cjs	Increases fallback search window for locating the aic-usage-cache artifact.
actions/setup/js/check_daily_aic_workflow_guardrail.cjs	Adds periodic in-loop core rate-limit snapshot checks and early exit when headroom is exhausted.
actions/setup/js/check_daily_aic_workflow_guardrail.test.cjs	Adds test asserting the inspection loop stops after 5 cache-miss runs when mid-loop rate limit drops below reserve.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 3/5 changed files
• Comments generated: 2
• Review effort level: Low

[pelikhan]

@copilot merge main and recompile

[github-actions]

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (>100 new lines in pkg/) but does not have a linked Architecture Decision Record (ADR).

📄 Draft ADR committed: docs/adr/43447-in-loop-rate-limit-recheck-for-aic-guardrail.md — review and complete it before merging.

🔒 This PR cannot merge until an ADR is linked in the PR body.

📋 What to do next

1. Review the draft ADR committed to your branch at docs/adr/43447-in-loop-rate-limit-recheck-for-aic-guardrail.md — it was generated from the PR diff
2. Complete the missing sections — add context the AI could not fully infer (deciders, any team discussion), refine the decision rationale, and confirm the alternatives are accurate
3. Commit the finalized ADR to docs/adr/ on your branch (update Status from Draft to Accepted once approved)
4. Reference the ADR in this PR body by adding a line such as:

   ADR: ADR-43447: In-Loop Rate-Limit Re-Check for AIC Guardrail

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

❓ Why ADRs Matter

ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you.

📋 Michael Nygard ADR Format Reference

An ADR must contain these four sections to be considered complete:

• Context — What is the problem? What forces are at play?
• Decision — What did you decide? Why?
• Alternatives Considered — What else could have been done?
• Consequences — What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number (e.g., 43447-in-loop-rate-limit-recheck-for-aic-guardrail.md for PR #43447).

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · 55.2 AIC · ⌖ 10.2 AIC · ⊞ 8.4K · ◷
_{Comment /review to run again}

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 90/100 — Excellent

Analyzed 1 test(s): 1 design, 0 implementation, 0 violation(s).

📊 Metrics (1 test)

Metric	Value
Analyzed	1 (Go: 0, JS: 1)
✅ Design	1 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	1 (100%)
Duplicate clusters	0
Inflation	YES (2.86:1)
🚨 Violations	0

Test	File	Classification	Issues
main() stops inspection loop early when rate-limit headroom exhausted mid-loop	check_daily_aic_workflow_guardrail.test.cjs:574	design_test / behavioral_contract / high_value	Test inflation (100 added / 35 prod = 2.86:1)

i️ Flagged Tests (1 — inflation only)

main() stops the inspection loop early when in-loop rate-limit re-check finds headroom exhausted (check_daily_aic_workflow_guardrail.test.cjs:574) — This test is a strong design test: it directly verifies the AC3/AC4 behavioral contract that the in-loop rate-limit re-check (triggered after exactly 10 consumed API ops = 5 cache-miss runs) breaks the loop when remaining <= RATE_LIMIT_RESERVE, and validates three observable outputs: exact getRunAIC call count (5), the specific stop-log message, and the daily_ai_credits_exceeded output flag. Not flagged as a quality issue.

The only concern is test inflation (100 new test lines / 35 new production lines = 2.86:1) — understandable given the extensive mock scaffolding required for an integration-style vitest test, but worth noting.

No test was added for the MAX_RUNS_TO_SEARCH 5→30 change in restore_aic_usage_cache_fallback.cjs; that constant change has limited unit-testability and the existing fallback tests provide indirect coverage.

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). The new test covers the critical concurrent rate-limit exhaustion edge case (AC3/AC4) with clear behavioral assertions. Score docked 10 points for test inflation ratio (2.86:1 > 2.0:1).

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 42.9 AIC · ⌖ 10.9 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 90/100. 0% implementation tests (threshold: 30%). The new test covers the critical concurrent rate-limit exhaustion edge case with clear behavioral assertions. Minor: test inflation at 2.86:1 (no guideline violation).

[github-actions]

Review Summary

Good, targeted fix for a real production incident. Applied /diagnosing-bugs and /tdd.

5 inline suggestions — none are blocking, but a few are worth addressing:

1. Increment apiCallsInLoop before the async call (guardrail line 596) — exceptions currently skip the counter
2. Document the sub-interval edge case (guardrail line 573) — runs that exit before the next multiple never trigger the re-check
3. Add the "loop completes normally" counterpart test (test line 596) — only the early-break path is currently specified
4. Derive magic number 5 from constants (test line 658) — guards against silent drift if constants change
5. Expand JSDoc on MAX_RUNS_TO_SEARCH (fallback line 57) — documents the cold-run fan-out trade-off

@copilot please address the review comments above.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 97.3 AIC · ⌖ 7.79 AIC · ⊞ 6.6K · ◷
_{Comment /matt to run again}

[github-actions]

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 97.3 AIC · ⌖ 7.79 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[github-actions]

[/diagnosing-bugs] The in-loop re-check only fires at exact multiples of RATE_LIMIT_RECHECK_INTERVAL (10 ops). If the loop finishes with fewer ops (e.g. only 3–4 cache-miss runs), the re-check never fires for that execution, leaving a concurrent budget-exhaustion window undetected.

💡 Suggestion

Document this known limitation in the constant's JSDoc, or add a post-loop check:

// After the for...of loop:
if (apiCallsInLoop > 0 && apiCallsInLoop % RATE_LIMIT_RECHECK_INTERVAL !== 0) {
  const finalRL = await getCoreRateLimitSnapshot(githubClient);
  if (finalRL.remaining <= RATE_LIMIT_RESERVE) {
    logDailyGuardrail('Post-loop rate limit check: headroom exhausted', { remaining: finalRL.remaining });
  }
}

Alternatively, accepting this as a known trade-off is fine — just note it explicitly in the JSDoc so future maintainers don't wonder why short runs skip the check.

@copilot please address this.

[github-actions]

[/diagnosing-bugs] apiCallsInLoop is incremented after getRunAIC is awaited on line 597. This means if getRunAIC throws an exception, the catch block on line 622 swallows the error and the ops counter is never incremented — so a run that fails mid-artifact-fetch is invisible to the rate-limit accounting.

💡 Suggestion

Increment before the async call to ensure every cache-miss attempt is counted regardless of whether it succeeds or fails:

} else {
  // Cache miss: fetch AIC from the run's usage artifact.
  apiCallsInLoop += ESTIMATED_API_OPERATIONS_PER_RUN; // increment first
  runAIC = await module.exports.getRunAIC(artifactClient, run.id, token, owner, repo);
}

This prevents the re-check threshold from being delayed in error-heavy runs where all artifact fetches throw.

@copilot please address this.

[github-actions]

[/tdd] The mock's rate-limit collapse is binary: call 1 returns 5000, all subsequent calls return 50. This conflates the in-loop re-check and the post-loop end snapshot into the same low-remaining scenario. The test doesn't verify what happens if the in-loop re-check returns above the reserve (e.g. 200) but the final end snapshot is below — the loop should continue but reporting should still capture the final state.

💡 Suggested additional test case

Add a sibling test where the mid-loop re-check returns a "safe" value (e.g. 500) and the loop runs to completion, confirming no early break occurs:

it('main() continues loop when in-loop re-check finds sufficient headroom', async () => {
  // all rate-limit calls return safely above reserve
  // assert getRunAIC called for all 15 runs
});

This completes the specification of the in-loop branch's both paths (break vs. continue).

@copilot please address this.

[github-actions]

[/tdd] The assertion expect(getRunAICSpy).toHaveBeenCalledTimes(5) is tightly coupled to the magic number 5, which is derived from RATE_LIMIT_RECHECK_INTERVAL / ESTIMATED_API_OPERATIONS_PER_RUN. If either constant changes, this test will silently become wrong without a clear signal.

💡 Suggestion

Reference the constants explicitly in the assertion comment or compute the expected call count from the imported constants:

// RATE_LIMIT_RECHECK_INTERVAL=10 ops / ESTIMATED_API_OPERATIONS_PER_RUN=2 = 5 runs before first re-check fires
const expectedRunsBeforeRecheck = RATE_LIMIT_RECHECK_INTERVAL / ESTIMATED_API_OPERATIONS_PER_RUN;
expect(getRunAICSpy).toHaveBeenCalledTimes(expectedRunsBeforeRecheck);

This makes the test self-documenting and resilient to constant changes.

@copilot please address this.

[github-actions]

[/diagnosing-bugs] MAX_RUNS_TO_SEARCH = 30 widens the fallback search window but also widens the window for the listWorkflowRuns API call. The PR notes "30 covers the same single listWorkflowRuns page at no additional cost" — this is only true if the workflow has ≥30 completed runs on a single page (100 per page). On a lightly-used workflow or a brand-new repo, this assumption holds fine. But the 6× wider window also means the listArtifacts calls are spread over up to 30 runs rather than 5, which could increase the rate-limit fan-out when the cache is cold.

💡 Suggestion

The PR description says the artifact fallback is the only warm path for PR branches. If that's the goal, the constant might benefit from a more explicit name and JSDoc that clarifies the trade-off:

/**
 * Maximum number of recent workflow runs to scan for a usable cache artifact.
 * On pull_request branches, actions/cache is branch-scoped and will never hit
 * a run from another PR, so this artifact fallback is the only warm path.
 *
 * Trade-off: a larger value improves cold-cache hit rate but increases
 * listArtifacts fan-out when no artifact is found (e.g. completely cold run).
 * 30 stays within a single listWorkflowRuns page (100 per page) and is the
 * observed sufficient window for active repositories.
 */
const MAX_RUNS_TO_SEARCH = 30;

Also consider whether the fallback should cap its listArtifacts calls using the same rate-limit guard applied in the guardrail file.

@copilot please address this.

[github-actions]

[/tdd] The test name describes the re-check mechanic precisely, but the test doesn't assert that truncatedByRateLimit is reflected in the output (e.g. daily_ai_credits_exceeded or some output/log that signals "inspection was rate-limited"). The truncatedByRateLimit = true branch should have a visible observable effect for downstream consumers.

💡 Suggestion

Check if the code sets any output or log that signals truncation and add an assertion:

// If truncatedByRateLimit affects any output, assert it:
expect(coreOutputs['daily_aic_guardrail_truncated_by_rate_limit']).toBe('true');
// or check the warning log emitted when truncation occurs

If no such output exists, consider adding one to make truncation observable to downstream workflow steps.

@copilot please address this.

[gh-aw-bot]

@copilot please run the pr-finisher skill, address the latest maintainer feedback (merge main and recompile), review the design gate comment about linking the drafted ADR in the PR body, and rerun checks once updated.

Generated by 👨‍🍳 PR Sous Chef · 4.83 AIC · ⌖ 6.02 AIC · ⊞ 3.4K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot merge main and recompile

Done — merged main and recompiled. All pre-PR validation checks passed.

[github-actions]

Review: fix(aic-guardrail) — widen fallback search window and in-loop rate-limit backoff

The core fix in check_daily_aic_workflow_guardrail.cjs is sound: the in-loop getCoreRateLimitSnapshot re-check addresses the concurrent-activation staleness problem cleanly, and the new test accurately exercises the early-exit path. The implementation is correct.

I've surfaced a few non-blocking issues and one reliability concern worth addressing before merge:

🔴 Reliability concern (non-blocking but worth addressing)

restore_aic_usage_cache_fallback.cjs: Widening MAX_RUNS_TO_SEARCH from 5→30 increases the worst-case listWorkflowRunArtifacts fan-out from 5 to 30 calls per activation (one per run until a usable artifact is found). The PR body identifies this exact call class as causing 99.5% of rate-limit saturation. The fix in the guardrail file won't protect against the fallback script's fan-out. See inline comment.

🟡 Non-blocking suggestions

1. apiCallsInLoop incremented before the API call: conservative accounting but worth a clarifying note or a post-call placement (inline comment on line 596).
2. Re-check overhead not budgeted: computeMaxInspectableRuns doesn't account for the extra rateLimit.get() calls from in-loop re-checks (inline comment on line 574).
3. mockCore missing setFailed: the new test mock is incomplete; a future logic change that triggers setFailed will produce a cryptic TypeError rather than a meaningful test failure (inline comment on line 629).

Overall the in-loop backoff (AC3/AC4) and the RATE_LIMIT_RECHECK_INTERVAL constant are well-designed. The fallback search window concern (AC2) needs attention before this ships.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 139.5 AIC · ⌖ 7.03 AIC · ⊞ 4.9K

Comments that could not be inline-anchored

actions/setup/js/restore_aic_usage_cache_fallback.cjs:135

listWorkflowRunArtifacts fan-out increases 6×

The change to MAX_RUNS_TO_SEARCH = 30 means the per_page on listWorkflowRuns fetches 30 runs in one call — that part is free. However, the inner loop at line 138 calls listWorkflowRunArtifacts once per run until it finds a usable artifact. Worst-case, that is 30 separate API calls instead of 5.

The PR body describes the O(N×M) fan-out of listArtifacts in check_daily_aic_workflow_guardrail.cjs as the core rate-limit proble…

actions/setup/js/check_daily_aic_workflow_guardrail.cjs:596

apiCallsInLoop incremented before the API call on error

The counter is incremented at line 596, then getRunAIC is called at line 597. If getRunAIC throws (caught at line 622), the budget counter has already been advanced for a failed call — the next re-check will fire one interval earlier than expected.

This is conservative (safe) accounting, but it means a burst of failing artifact lookups advances the re-check counter without consuming real rate-limit budget. If the intent is to t…

actions/setup/js/check_daily_aic_workflow_guardrail.cjs:574

In-loop re-check itself consumes 1 API call per interval

The getCoreRateLimitSnapshot at this line calls githubClient.rest.rateLimit.get() which itself costs 1 API call. With RATE_LIMIT_RECHECK_INTERVAL = 10 and ESTIMATED_API_OPERATIONS_PER_RUN = 2, a re-check fires every 5 cache-miss runs. On a 100-run inspection window that would add ~10 extra rate-limit calls on top of the 200 artifact calls.

The computeMaxInspectableRuns budget doesn't account for these re-check calls (only …

actions/setup/js/check_daily_aic_workflow_guardrail.cjs:573

Re-check fires at the start of the iteration, using last iteration's count

The guard condition apiCallsInLoop > 0 && apiCallsInLoop % RATE_LIMIT_RECHECK_INTERVAL === 0 is evaluated at the top of the loop. Since apiCallsInLoop is incremented only on cache misses inside the else branch (line 596), a mix of cache hits and misses will cause the re-check to fire less frequently than RATE_LIMIT_RECHECK_INTERVAL / ESTIMATED_API_OPERATIONS_PER_RUN runs (because cache hits advance the ite…

actions/setup/js/check_daily_aic_workflow_guardrail.test.cjs:629

Test mockCore is missing setFailed

The test mock at line 629 defines setOutput, info, warning, and summary, but does not define setFailed. If a future change alters the loop logic so that the guardrail threshold is exceeded before the rate-limit re-check fires, main() will call core.setFailed(...)) and the test will throw TypeError: core.setFailed is not a function, producing a confusing failure message.

Add setFailed: () => {} (or a spy) to the mock to make future b…

[gh-aw-bot]

pr-sous-chef
@copilot please run the pr-finisher skill, address the latest maintainer and reviewer feedback, especially the ADR/body follow-up and the fallback-search rate-limit concern, then rerun checks once the branch is updated.

Generated by 👨‍🍳 PR Sous Chef · 11.4 AIC · ⌖ 5.77 AIC · ⊞ 3.4K · ◷
_{Comment /souschef to run again}

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.3