feat: add runtime model fallback policy#52
feat: add runtime model fallback policy#52StartupBros wants to merge 4 commits intonicobailon:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c00ebec772
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
runtime-model-fallback.ts
Outdated
| const expiresAt = now + cooldownMinutes * 60 * 1000; | ||
| if (classification.cooldownScope === "provider") { | ||
| const provider = getProviderFromModel(candidate.normalizedModel ?? candidate.model); | ||
| if (!provider) return; |
There was a problem hiding this comment.
Return the current store when provider parsing fails
When a retryable provider-scoped failure occurs on a bare model ID (for example gpt-4.1 without a provider prefix), updateCooldownStore hits if (!provider) return; and returns undefined. executeWithRuntimeModelFallback then stores that into cooldownStore and calls getCooldownSkipReason(next, cooldownStore) for remaining candidates, which crashes with Cannot read properties of undefined (reading 'models'). This converts a recoverable fallback path into a runtime exception whenever additional candidates are present.
Useful? React with 👍 / 👎.
|
Addressed the Codex review note about provider-scoped cooldowns on bare model ids. It was a valid edge case: could return when provider parsing failed, which could poison the in-memory cooldown store during fallback iteration. Fixed in by returning the current store instead, and added a regression test covering that path. |
|
Correction to my previous note: the valid edge case was in updateCooldownStore() returning undefined when provider parsing failed for a provider-scoped cooldown on a bare model id. Fixed in 6ca81da by returning the current store instead, and added a regression test for that path. |
Summary
runtime-model-fallback.tspolicy module for candidate ordering, normalization, conservative failure classification, and session-scoped cooldownsWhy
Today delegated runs resolve a model once and either succeed or fail. That created drift across execution paths and previously caused parity regressions around model overrides and thinking level handling in async mode.
This PR makes model choice an execution policy instead of a one-shot resolution, while staying intentionally narrow for v1:
fallbackModels--modelsImplementation Notes
runtime-model-fallback.tsfor:requestedModelfinalModelmodelAttemptsfallbackSummary/lastFallbackReasonexecution.ts,chain-execution.ts,async-execution.ts, andsubagent-runner.tsthrough the same policy contextTesting
pnpm test:allstatus.jsonPost-Deploy Monitoring & Validation
/tmp/pi-async-subagent-runs/<id>/fallback:messages in run logssubagent_status({ id: "<id>" })/tmp/pi-async-subagent-runs/<id>/status.jsonrequestedModelfinalModelmodelAttemptslastFallbackReason