Allow thread execution overrides for all providers - #1610
Conversation
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
| ); | ||
| } | ||
|
|
||
| const models = await loadThreadProviderModels(deps, thread); |
There was a problem hiding this comment.
🚨 slopcop/review — Keep a restore gate for ACP execution overrides
This now accepts model changes for every dynamic ACP provider. The runtime classifies ACP model changes as session changes and sends thread/resume.
When an agent lacks loadSession, the bridge creates session/new. The next turn then loses prior provider context.
Please retain a capability gate for these agents. Alternatively, add a live ACP model change that does not replace the session.
Please add a custom ACP test without loadSession. The current route matrix only checks built-in catalog persistence.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary
This change lets each provider change a thread model and reasoning level for later turns. The server limits each model choice to the current provider.
Review
I found one high-impact correctness problem.
- Custom ACP agents without
loadSessioncan lose provider conversation history after a valid override. - ACP treats the model change as a session change. The runtime sends
thread/resume. - The bridge creates a new session when the agent cannot restore the old session.
- Keep a capability gate for these agents, or add a live option change path.
- Add a test with a custom ACP agent that lacks
loadSession.
The security review found no issue. The performance review found no hot-path or scale problem.
The capability removal reduces a duplicate provider list. A runtime support check would better match each active provider session.
Validation
- The focused server suite passed 15 tests.
- The provider suite passed 11 tests.
- The template suite passed 44 tests.
- Type checks passed for the server, provider, and template packages.
- All current GitHub CI checks passed.
- The exact pull request SHA ran in a local development server.
- A browser PATCH returned 200 for a Codex model and reasoning update.
- The next Codex turn used
gpt-5.6-lunawith high reasoning.
Summary
Context
The original implementation explicitly gated Codex out for v1. That policy became stale: follow-up requests already send changed model and reasoning settings through the same next-turn/runtime reconfiguration paths, and the Codex, Pi, and ACP resume adapters all apply those settings today.
Deleting the now-always-supported capability avoids rejecting future and dynamically configured ACP providers through a second provider allowlist.
Testing
pnpm exec turbo run test --filter=@bb/server -- test/public/public-threads.execution-override.test.ts test/services/threads/thread-execution-override.test.tspnpm exec turbo run test --filter=@bb/agent-providers --filter=@bb/templatespnpm exec turbo run typecheck --filter=@bb/server --filter=@bb/agent-providers --filter=@bb/templates