fix(server): select an available provider for delegated tasks - #11870
juliusmarminge merged 8 commits into
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped provider-resolution bug fix: unavailable inherited instances now fall back to healthy peers, with explicit targets remaining strict. The production change is small and covered by focused regression tests, while the documentation is updated consistently. You can add or adjust custom eligibility rules. Learn more. |
a5ebe9a to
a62d7ad
Compare
b7e28ab to
792db8c
Compare
94be5b7 to
84c9498
Compare
792db8c to
9658bc7
Compare
9a98db3 to
69bcbee
Compare
7cedd7b to
c2ae308
Compare
82d344d to
341b1ac
Compare
c2ae308 to
ad59c2a
Compare
341b1ac to
f63b335
Compare
ad59c2a to
6ae1588
Compare
f63b335 to
08a1b86
Compare
6ae1588 to
cacc469
Compare
0f32464 to
4ab2efc
Compare
cacc469 to
5966041
Compare
43db598 to
6f6817f
Compare
5966041 to
802b2fe
Compare
d61550b to
e6cb2b5
Compare
c2d7665 to
2822470
Compare
Dismissing prior approval to re-evaluate bcf5ec8
bcf5ec8 to
8355a7f
Compare
a6b73de to
3d45b30
Compare
b3f5ee6 to
02a525d
Compare
b6933f8 to
642168a
Compare
5b1f6a2 to
9b4fd10
Compare
642168a to
bbfc07d
Compare
5ff0a93 to
fd8ea2f
Compare
bbfc07d to
c4dbf89
Compare
This comment has been minimized.
This comment has been minimized.
68475ad to
eb1d74d
Compare
Dismissing prior approval to re-evaluate f13b647
| if (instanceId === undefined) { | ||
| const reasons = candidates.flatMap((candidate) => | ||
| providerConstraints(candidate, true).map( | ||
| (constraint) => `${candidate.instanceId}: ${constraint}`, | ||
| ), | ||
| ); | ||
| return yield* failure( | ||
| "provider_unavailable", | ||
| `No available V2 provider instance for driver ${requestedDriver}: ${reasons.join(" ")}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This returned MCP failure now concatenates every provider constraint, including raw provider.unavailableReason / provider.message, without any length bound. Those strings may contain arbitrary provider diagnostics or sensitive data. Keep the caller-visible error normalized and bounded; the provider details can remain available through a separately sanitized diagnostic path.
| if (instanceId === undefined) { | |
| const reasons = candidates.flatMap((candidate) => | |
| providerConstraints(candidate, true).map( | |
| (constraint) => `${candidate.instanceId}: ${constraint}`, | |
| ), | |
| ); | |
| return yield* failure( | |
| "provider_unavailable", | |
| `No available V2 provider instance for driver ${requestedDriver}: ${reasons.join(" ")}`, | |
| ); | |
| } | |
| if (instanceId === undefined) { | |
| return yield* failure( | |
| "provider_unavailable", | |
| `No available V2 provider instance for driver ${requestedDriver}.`, | |
| ); | |
| } |
Posted via Macroscope — Effect Service Conventions
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
Effect Service Conventions found 1 issue: the new provider-unavailable failure exposes unbounded, potentially sensitive provider diagnostics. See the inline review comment for the suggested fix. Posted via Macroscope — Effect Service Conventions |
10d3c83 to
ab79e2f
Compare
|
All clear Posted via Macroscope — Effect Service Conventions |
a1f8051 to
0337dd6
Compare
A driver-only delegation target could fail on an unavailable inherited provider instance even when another instance of that driver was healthy. Prefer the inherited instance only when it passes the availability checks, then fall back to a healthy peer. Explicit instance selection remains strict. Adapted from the proposal in pingdotgg#11154 onto the adapter-registry capability lookup. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Upstream 1ee1d04 rejects v2 file-restore rollback in shared workspaces; fixture threads had worktreePath: null so every thread_rollback replay was treated as shared and rejected. Each fixture workspace is a private temp checkout, so declare it as the thread's worktreePath — an accurate model that keeps the file-restore path covered. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Two consecutive CI runs failed queued_cancelled_while_active/codex on release_replay_gate:turn/completed (budget exhausted in <200ms) while the base stayed green. Declaring every fixture workspace as a private worktree enables checkpoint capture git work per turn, adding latency unrelated fixtures never need. Only thread_rollback exercises the file-restore path that requires a non-shared worktree, so gate the declaration on rollback steps. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
A driver-only delegation request whose registered candidates were all unavailable fell through to the parent's instance, which belongs to a different driver, and failed with invalid_request. The request is valid; the instances are not. Return provider_unavailable listing each candidate's constraints instead. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Keep the parent snapshot on the codex driver and make only the peer candidate claudeAgent, so the pre-fix fallback would hit the real driver-mismatch invalid_request rather than passing on the shared provider_unavailable code. Also assert provider_unavailable through the shared create_threads path. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The provider_unavailable failure for an exhausted driver candidate set concatenated raw provider diagnostics (unavailableReason, status message) without a length bound. Return the normalized driver-scoped message instead; provider detail stays visible through provider state. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…Records v2 already avoids the shared-workspace rollback rejection with restoreFiles: false, so the fixture worktreePath option and its integration-test branch are dead weight. Also point the new delegation test's ThreadManagementService mock at getThreadRecords, which is what OrchestratorMcpService calls on v2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ab79e2f to
46353a3
Compare
|
Rebased onto One follow-up commit on top (
Verified in Rebased and touched up by a maintainer's agent; a human will re-review. |
fc66b9f
into
pingdotgg:t3code/codex-turn-mapping
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
A driver-only delegation target (
target.driverKindwithoutproviderInstanceId) could fail on an unavailable inherited provider instance even when another instance of that driver was healthy:resolveTargetpreferred the parent's instance whenever it was registered and orchestration-capable, and the constraint check then rejected it.The fix requires the inherited candidate to pass the same
providerConstraintsavailability check as any other candidate before it can win; otherwise the first constraint-free instance of the requested driver is selected. When no healthy candidate exists, resolution falls through to the parent instance id so the constraint check reports why the inherited instance cannot serve. ExplicitproviderInstanceIdselection is unchanged and stays strict — an unavailable explicit target fails instead of silently rerouting.Adapted from @kiiwee's earlier proposal in #11154 (closed unmerged) onto the live adapter-registry capability lookup from #11578. Credit to kiiwee for identifying the defect and the original fix shape.
Test plan
vp test run apps/server/src/mcp/OrchestratorMcpService.test.ts— the new table-driven test proves healthy inheritance (parent model selection including options preserved verbatim), unavailable parent + healthy peer fallback, no-available-peer rejection, explicit-unavailable rejection without rerouting, and explicit healthy selection. The fallback case failed before the fix (Provider codex cannot run a child task: Provider instance is disabled.).vp test run apps/server/src/mcp/— all 170 MCP tests pass, including the cross-provider delegation integration test.vp lint,vp fmt --check, andtsc --noEmitclean for the changed scope.Independent cross-provider review: attempted
codex exec -m gpt-6-astra -c model_reasoning_effort="xhigh" --sandbox read-onlyon the frozen diff; the reviewer account was at its usage limit (reported retry Sep 19), so the review was skipped — disclosed, not substituted.Targets Orchestrator V2 in #2829.
Engineering: SWE-2 Max through the Devin/T3 harness.
Coordination: T3 thread 3aa91588-f393-4df8-9d47-c8c769d27899, campaign saphid/t3code-personal#298.