fix(subagents): carry entitlement into the encrypted-recovery preview (#2509) - #2629
Conversation
#2515 fixed the primary selection path to preview per candidate quota scope, and #2623 added the entitled-account filter there. The encrypted-recovery path got the scope but not the filter: it re-previewed per candidate and passed no eligible-account set, so a recovered assignment could select an account with no entitlement to the model and fail closed at final auth. Same stale-selection class as the quota scope, one layer over. Pinned structurally, like the route-inventory contract: both preview assignment sites must accept and forward modelEligibleAccountIds. Driving it end to end needs a recovered encrypted assignment AND an account-gated candidate whose entitlement differs per account, and that fixture proved more fragile than the thing it checks - I tried it and dropped it rather than ship a flaky test. What this does catch is the regression that actually threatens the fix: one of the two sites silently losing the argument again, which is how recovery lost it. Falsified: reverting the recovery site to the two-argument form reddens exactly this test.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSubagent fallback recovery now passes model-eligible account IDs to account previews and fallback resolution. A structural regression test verifies the filter is forwarded through both fallback preview paths. ChangesSubagent recovery account eligibility
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0aa976cd
ℹ️ 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".
| false, | ||
| recoverySelectionOptions, | ||
| subagentFallbackAccountPreview, | ||
| subagentFallbackModelEligibleAccountIdsForModel, |
There was a problem hiding this comment.
When native-profile drain state changes during await recoverEncryptedAgentTask, this reuses the eligibility closure created before recovery while combining it with a fresh recoverySelectionAdmission. For example, if the initial admission excludes __main__, the drain completes during recovery, and only main is entitled to the gated fallback model, the stale set still excludes main and the recovered request skips that now-usable fallback, potentially sending the already-blocked routed primary instead. Re-resolve eligibility after recovery using the current main-account exclusion (or otherwise bind the snapshot and admission atomically), and cover the transition behavior directly—the added source-regex test cannot detect this stale-state failure.
AGENTS.md reference: AGENTS.md:L276-L278
Useful? React with 👍 / 👎.
1 participant
Summary
Closes the remaining half of #2509. #2515 fixed the primary selection path to preview the Pool account per candidate quota scope, and #2623 added the entitled-account filter there. The encrypted-recovery path got the scope but not the filter.
So recovery re-previewed per candidate and passed no eligible-account set, which means a recovered assignment could select an account with no entitlement to the model and then fail closed at final auth. That is the same stale-selection class the issue is about, one layer over — selection rejecting or mis-picking before model-aware auth runs.
Verification
Falsified: reverting the recovery site to its two-argument form reddens exactly the new test and nothing else.
On the test shape, since it is a structural assertion
I tried the end-to-end version first and dropped it. Driving this properly needs a recovered encrypted assignment and an account-gated candidate whose entitlement differs per account — the existing recovery case grants the roster to both pool accounts, so it can only prove the scope is re-previewed. The fixture I built for it was more fragile than the behavior it checked, and a flaky test on a credential-selection path is worse than an honest structural one.
What this test does catch is the regression that actually threatens the fix: one of the two preview sites silently losing the eligibility argument again, which is precisely how recovery lost it. It asserts both assignment sites accept
modelEligibleAccountIdsand forward it into the preview call — accepting it without forwarding was a plausible half-fix.What remains open on #2509
Not everything in that issue is closed by this. Selection still only checks probe availability rather than atomically reserving it (
src/codex/subagent-model-fallback.ts:294), which is a separate lifecycle question. Saying so rather than closing the issue on a partial fix.Checklist
devSummary by CodeRabbit
Bug Fixes
Tests