refactor(run): unify local/remote dispatch via Backend (10 baby steps)#2715
Merged
Conversation
|
❌ PR Review Failed — The review agent encountered an error and could not complete the review. View logs. |
Member
|
This PR has merge conflicts with |
Add a typed sentinel so callers can distinguish 'not supported by this runtime' from other failures via errors.Is. Wrap the existing ad-hoc strings in RemoteRuntime.RestartToolset and ExecuteMCPPrompt with %w so the existing user-facing messages stay unchanged.
--sandbox, --session-db, --session, --record and --fake all silently do nothing when paired with --remote because the remote branch in runOrExec bypasses the code that would honour them. Reject these combinations at parse time so users get a clear error instead of guessing why their flag had no effect.
The remote branch in runOrExec called createRemoteRuntimeAndSession (which POSTs /api/sessions to the server) before launchTUI's dry-run check fired, so --dry-run --remote leaked a server-side session. Move the check ahead of the network call and drop the now-redundant copy from launchTUI.
Pull the []teamloader.Opt and []runtime.Opt construction into named methods on runExecFlags. Both blocks were duplicated between createLocalRuntimeAndSession and createSessionSpawner; the dedup is mechanical and prepares the way for a typed configuration payload that can travel to a remote backend.
Move the team load + local runtime/session construction (and the runtime/toolset cleanup) behind a small backend abstraction. The local code path now goes through it; the remote branch is unchanged. The interface has one implementation today; the next step adds a remoteBackend so runOrExec can collapse to a single linear flow.
remoteBackend wraps the previous createRemoteRuntimeAndSession; the two-branch dispatch in runOrExec collapses to a single linear flow: selectBackend → CreateRuntimeAndSession → handleExecMode|runTUI. The launchTUI helper that existed only because the remote branch couldn't share the local tail goes away.
Replace the scattered "interface{ TogglePause() bool }" and
"rt.(runtime.ModelSwitcher)" assertions with typed accessors
(PauserOf, ModelSwitcherOf, ToolsChangeSubscriberOf) in the runtime
package. The shape is the same but each capability is named, and a
future commit that promotes one of them onto the Runtime interface
only needs to delete the helper rather than chase down call sites.
runRuntimeContract exercises every non-streaming method on the Runtime interface (CurrentAgentName, SetCurrentAgent, CurrentAgentTools, RestartToolset, MCP prompts, Steer/FollowUp, Resume, Close, ...) and runs against any factory. Wired up against LocalRuntime today; future commits that promote optional capabilities onto the interface and add a real RemoteRuntime implementation will plug into the same suite to enforce parity.
TogglePause(ctx) (paused, err) is now part of Runtime. LocalRuntime delegates to the existing implementation; RemoteRuntime returns ErrUnsupported for now. App.TogglePause keys off the typed error instead of a type assertion, and the optional Pauser interface goes away. Pattern for the remaining optional sub-interfaces (model switching, snapshots, ...): each one becomes a copy of this commit.
Replace the scatter of f.modelOverrides / f.promptFiles / f.runConfig and f.agentName / f.autoApprove / f.hideToolResults / f.sessionID / f.snapshotsEnabled / f.globalPermissions arguments with two typed payloads. LocalBackend.CreateRuntimeAndSession builds them from the flags and hands them to loadAgentFrom and createLocalRuntimeAndSession. The field set is the data shape a future RemoteBackend will marshal over the wire so the server runs teamloader.LoadWithConfig and session.New with identical inputs to today's local code path.
dgageot
force-pushed
the
remote-runtime-baby-steps
branch
from
d657f63 to
6c65b31
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
docker agent runhas two parallel code paths — a long-form local one and a remote shortcut that branches early inrunOrExec. The shortcut bypasses team load options,RuntimeConfig,buildSessionOpts, the session spawner, and several optional sub-interfaces, so a long list of features silently break under--remote:--model,--prompt-file,--session-db,--session,--record,--fake, hooks,--working-dir,--hide-tool-results, max iterations, snapshots, model switching, pause,/tools,/sessions,/skills, MCP prompts, compaction, title regeneration, …Rather than rewriting both paths in one go, this PR lays the groundwork: ten small, individually-revertible commits that make the eventual unification a mechanical change. No user-visible behaviour changes except clearer error messages and four flag-combo rejections that previously did nothing.
What
Ten commits, each shippable on its own:
feat(runtime): introduce ErrUnsupported sentinel for remote runtime gaps— typed sentinel so silentnil/empty returns become explicit.feat(run): reject --remote combined with silently-broken flags—MarkFlagsMutuallyExclusivefor--sandbox,--session-db,--session,--record,--fake. Stops the bleeding.fix(run): honour --dry-run before contacting the remote server—--dry-run --remoteno longer creates a server-side session before exiting.refactor(run): extract teamOpts and runtimeOpts helpers— pure tidy-first; preps the typed payload.refactor(run): introduce backend interface with localBackend impl— adds the seam; one implementation today.refactor(run): collapse local/remote branches via shared backend—selectBackend+ linearrunOrExec.launchTUIdisappears.refactor(runtime): centralize optional-capability checks—PauserOf,ModelSwitcherOf,ToolsChangeSubscriberOfreplace scattered type assertions.test(runtime): add backend-agnostic Runtime contract test— pins whatRuntimemeans; runs againstLocalRuntimetoday.refactor(runtime): promote TogglePause onto the Runtime interface— first sub-interface promotion. Template for the others.refactor(run): introduce LoadTeamRequest and CreateSessionRequest— typed config payloads, ready to travel over the wire.After these ten commits,
--remoteis aBackendselector instead of a parallel universe, and adding new wire-protocol endpoints, file uploads, etc. becomes an isolated change against a stable spine.What this PR does not do
ServerPolicy.ModelSwitcher, snapshots,ToolsChangeSubscriber) ontoRuntime— commit 9 establishes the pattern; the rest are mechanical follow-ups.Each of the above is its own PR.
Validation
task build✅task lint✅task test✅