Uh oh!
There was an error while loading. Please reload this page.
Retry model-catalog 429s and stop forwarding unresolved Copilot model aliases - #53456
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
✅ Ponytail Reviewer completed successfully!
|
✅ PR Code Quality Reviewer completed the code quality review.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53456 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
I don't see a blocking defect in the changed lines.
Notes
I checked the new 429 retry path, the Retry-After plumbing into withRetry, the alias-resolution fail-fast behavior, and the new tests around bounded refresh / abort-before-spawn. The control flow is consistent with the stated goal: transient catalog failures now get one extra recovery path, and known aliases are no longer silently forwarded into a misleading downstream error.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 9.08 AIC · ⌖ 6.18 AIC · ⊞ 6.9K
Comment /review to run again
There was a problem hiding this comment.
Ponytail review (over-engineering only).
L395 (copilot_harness.cjs): dead return after process.exit(1) — see inline comment.
Everything else (retry-status Set, Retry-After extraction, dedicated ModelAliasResolutionError, single bounded refresh-and-retry) is proportionate to the bug being fixed; no speculative abstractions or unused flexibility found.
net: -1 lines possible.
Generated by ✂️ Ponytail Reviewer for #53456 · auto · 26.3 AIC · ⌖ 3.59 AIC · ⊞ 7.2K
Comment /ponytail to run again
| } | ||
| logger(`copilot model alias resolution failed: model-catalog retrieval prevented alias resolution for '${configuredModel}' after a bounded refresh — refusing to start Copilot with an unresolved alias`); | ||
| process.exit(1); | ||
| return configuredModel; // unreachable, keeps TypeScript control-flow analysis happy |
There was a problem hiding this comment.
L395: delete: return configuredModel; after process.exit(1), justified only by a comment about "TypeScript control-flow analysis". Nothing replaces it — the function isn't typed as non-nullable-returning and no lint requires this line.
There was a problem hiding this comment.
Pull request overview
Adds model-catalog throttling recovery and prevents unresolved Copilot aliases from reaching the API proxy.
Changes:
- Retries HTTP 429 responses with bounded backoff and
Retry-Aftersupport. - Refreshes the catalog once before aborting unresolved alias resolution.
- Adds focused retry and alias-resolution tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/awf_reflect.cjs | Adds retry handling for catalog 429 responses. |
actions/setup/js/awf_reflect.test.cjs | Tests retry, exhaustion, delay, and fail-fast behavior. |
actions/setup/js/resolve_model_alias.cjs | Introduces explicit unresolved-alias failure handling. |
actions/setup/js/resolve_model_alias.test.cjs | Tests alias failure and concrete-model behavior. |
actions/setup/js/copilot_harness.cjs | Refreshes the catalog once before terminating. |
actions/setup/js/copilot_harness.test.cjs | Tests refresh success and abort behavior. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| const catalog = buildCatalogFromReflect(options.reflectData); | ||
| if (catalog.length === 0) { | ||
| logger(`copilot model alias resolution skipped (empty catalog from awf-reflect)`); | ||
| return configuredModel; | ||
| // Do not silently forward a known alias unchanged: the API proxy rejects unresolved | ||
| // aliases with a misleading "no AI credits pricing" error instead of surfacing the | ||
| // real cause (catalog retrieval failure). Callers should retry/refresh the catalog | ||
| // and, if it still cannot be built, stop before invoking Copilot. | ||
| logger(`copilot model alias resolution: catalog unavailable from awf-reflect for known alias '${configuredModel}'`); | ||
| throw new ModelAliasResolutionError(configuredModel); |
| function extractRetryAfterHeader(res) { | ||
| try { | ||
| const retryAfter = res?.headers?.get?.("retry-after"); | ||
| return retryAfter != null ? { "retry-after": retryAfter } : undefined; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
The changes are well-structured and correctly address the 429 retry gap described in #52782.
What the PR does:
- Adds
429toAWF_MODELS_URL_RETRYABLE_STATUSESalongside503, with properRetry-Afterheader forwarding to the existingwithRetrybackoff logic. - Replaces the silent alias passthrough on empty catalog with
ModelAliasResolutionError, a safer contract: callers now know to retry/refresh rather than forwarding an unresolved alias. - Adds a bounded single-refresh attempt in
applyCopilotModelAliasResolution, followed by a hardprocess.exit(1)guard if the catalog is still unavailable — preventing the misleading "no AI credits pricing" proxy error.
Quality: comprehensive unit tests cover the 429 retry path, Retry-After capping, permanent-4xx no-retry, alias-resolution error on empty catalog, successful refresh, and exhausted-refresh exit guard. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 31.7 AIC · ⌖ 7.1 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with improvement suggestions; not blocking.
📋 Key Themes & Highlights
Key Themes
- Log message accuracy — Two log messages fire before their described action happens, and one conflates distinct failure modes (null catalog vs. empty-models catalog). Inline comments #1, #3, and #4 cover these.
- Retry-After cap test specificity — The timer-based test for the 2000 ms cap is structurally correct but its description doesn't read as a specification of the capping behaviour. Inline comment #2 covers this.
- Missing-refetch path clarity — When
refetchReflectDatais not supplied (e.g. in tests), the fallback is silentnullbut the exhaustion log says "after a bounded refresh" regardless. Inline comment #1 covers this.
Positive Highlights
- ✅ Clean extraction of
ModelAliasResolutionErroras a dedicated typed error — callers can now distinguish catalog failure from other resolution errors without string-matching. - ✅
Retry-Aftercap wired correctly throughgetRetryAfterMs→maxDelayMs; no separate cap logic needed. - ✅ Exhaustive parametric tests for 400/401/403 non-retry cases — good spec coverage of permanent-failure fast-path.
- ✅ Test cleanup guard for
awf-config.jsonis well-structured; restores pre-existing content rather than blindly deleting. - ✅
extractRetryAfterHeaderis defensively coded (try/catch, optional chaining) and correctly narrowed to theretry-afterkey only — avoids accidental data leakage from headers. - ✅
AWF_MODELS_URL_RETRYABLE_STATUSESas aSetis a clean extension point for future retryable codes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 88.5 AIC · ⌖ 9.09 AIC · ⊞ 7.7K
Comment /matt to run again
| if (!(retryErr instanceof ModelAliasResolutionError)) { | ||
| throw retryErr; | ||
| } | ||
| logger(`copilot model alias resolution failed: model-catalog retrieval prevented alias resolution for '${configuredModel}' after a bounded refresh — refusing to start Copilot with an unresolved alias`); |
There was a problem hiding this comment.
[/diagnosing-bugs] Misleading diagnostic when refetchReflectData is not provided: no refetch is attempted, but the log says "after a bounded refresh", which obscures debugging.
💡 Suggested fix
constdidRefetch=!!options.refetchReflectData;logger(`copilot model alias resolution failed: model-catalog retrieval prevented alias resolution for '${configuredModel}' ${didRefetch ? "after a bounded refresh" : "(no refetch available)"} — refusing to start Copilot with an unresolved alias`);Keeps the message accurate whether or not a refetch was possible.
@copilot please address this.
| expect(fetchMock).toHaveBeenCalledTimes(1); | ||
| await vi.advanceTimersByTimeAsync(1); | ||
| const result = await run; |
There was a problem hiding this comment.
[/tdd] The Retry-After test verifies the second fetch fires after AWF_MODELS_URL_RETRY_MAX_MS but does not assert any delay is actually observed for the cap boundary — only the call count is checked. The cap behaviour (capped from 3000 ms → 2000 ms) is implicit.
💡 Suggested addition
Add a timing assertion to make the spec self-documenting:
// After advancing exactly RETRY_MAX_MS-1, retry must not have fired yetawaitvi.advanceTimersByTimeAsync(AWF_MODELS_URL_RETRY_MAX_MS-1);expect(fetchMock).toHaveBeenCalledTimes(1);// already present// Now one more ms triggers it — confirms the cap is 2000ms, not the raw 3000ms headerawaitvi.advanceTimersByTimeAsync(1);// existing assertions followThe second boundary check is present; consider naming the test description "...caps Retry-After to AWF_MODELS_URL_RETRY_MAX_MS" to make it a specification.
@copilot please address this.
| if (!(err instanceof ModelAliasResolutionError)) { | ||
| throw err; | ||
| } | ||
| logger(`copilot model alias resolution: retrying awf-reflect model-catalog fetch once before failing for '${configuredModel}'`); |
There was a problem hiding this comment.
[/diagnosing-bugs] Log message fires before the refetch is attempted, so if the refetch call itself throws or hangs, the log misleadingly suggests the retry already started. Move the log inside the successful-refetch branch, or rephrase to present tense.
💡 Suggested rephrase
logger(`copilot model alias resolution: initial catalog empty for '${configuredModel}'; attempting one awf-reflect refresh`);Then after the refetch:
logger(`copilot model alias resolution: refresh complete, retrying alias resolution for '${configuredModel}'`);This makes the log sequence reflect what actually happened, not what is about to happen.
@copilot please address this.
| // real cause (catalog retrieval failure). Callers should retry/refresh the catalog | ||
| // and, if it still cannot be built, stop before invoking Copilot. | ||
| logger(`copilot model alias resolution: catalog unavailable from awf-reflect for known alias '${configuredModel}'`); | ||
| throw new ModelAliasResolutionError(configuredModel); |
There was a problem hiding this comment.
[/diagnosing-bugs]ModelAliasResolutionError is thrown even when reflectData is non-null but every endpoint has an empty models array — not just when the catalog fetch failed. The log message says "catalog unavailable from awf-reflect" which is accurate for the null case, but misleading when data was returned but had no models. Consider splitting the log message:
💡 Suggested clarification
constreason=options.reflectData==null ? "catalog fetch returned no data" : "all awf-reflect endpoints had empty model lists";logger(`copilot model alias resolution: ${reason} for known alias '${configuredModel}'`);thrownewModelAliasResolutionError(configuredModel);This helps operators distinguish a transient 429 (null reflectData) from a misconfigured endpoint (empty models).
@copilot please address this.
| const refreshedReflectData = { endpoints: [{ configured: true, provider: "copilot", models: ["gpt-5.6-sol"] }] }; | ||
| const refetch = vi.fn().mockResolvedValue(refreshedReflectData); | ||
| const resolved = await applyCopilotModelAliasResolution({ |
There was a problem hiding this comment.
[/tdd] The "leaves a concrete configured model unchanged" test passes awfReflectData: null but the concrete model copilot/gpt-5.6-sol also appears as a value in the alias map. The test exercises the "not an alias key" branch — but does not explicitly assert that resolveConfiguredCopilotModel was not called a second time via refetch. The intent is clear but the assertion on refetch only says .not.toHaveBeenCalled(), which is sufficient; just worth noting the test name could be "does not refetch when configured model is concrete (not an alias key)" to read as a spec.
@copilot please address this.
pelikhan
commented
Aug 17, 2026
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh-aw-bot
commented
Aug 17, 2026
Uh oh!
There was an error while loading. Please reload this page.
🎉 This pull request is included in a new release. Release: |
A transient HTTP 429 from the AWF model-catalog endpoint caused the Copilot harness to treat the catalog as empty, skip alias resolution, and invoke Copilot with the raw alias (e.g.
sol) instead of the concrete model. The API proxy then rejected the request with a misleading "no AI credits pricing" error, even though the alias and pricing were valid — the real cause was a transient catalog fetch failure.Retry transient 429s in the model-catalog fetch
fetchModelsFromUrl()now treats HTTP 429 as retryable alongside 503, bounded byAWF_MODELS_URL_MAX_ATTEMPTS.Retry-Afterresponse header, capped atAWF_MODELS_URL_RETRY_MAX_MS, falling back to the existing exponential backoff otherwise.503.Stop invoking Copilot with a known, unresolved alias
resolveConfiguredCopilotModel()now throws a dedicatedModelAliasResolutionErrorwhen the configured model is a known alias but the catalog built from awf-reflect data is empty, instead of silently returning the alias unchanged.Bounded catalog refresh before failing
applyCopilotModelAliasResolution()in the Copilot harness is now async. OnModelAliasResolutionError, it performs one bounded awf-reflect refetch and retries resolution once.Tests
awf_reflect.test.cjscover 429 retry-to-success, retry exhaustion,Retry-Aftercapping, and non-retry of 400/401/403.resolve_model_alias.test.cjsandcopilot_harness.test.cjscover the alias-vs-empty-catalog error, the refresh-then-succeed path, and the refresh-then-fail path that aborts before spawning Copilot.Note: the plan also called for a smoke fixture simulating a 429-then-200 models endpoint; no existing smoke/e2e fixture harness for the AWF api-proxy was found in this repo (the api-proxy itself lives outside
gh-aw), so that item was left out of this change.Run: https://github.com/github/gh-aw/actions/runs/32066505163> Generated by 👨🍳 PR Sous Chef · gpt54 · 5.87 AIC · ⌖ 6.48 AIC · ⊞ 8.4K · ◷