remote-runtime: close silent gaps, consolidate Runtime, scaffold wire (10 baby steps)#2723
Open
dgageot wants to merge 10 commits intodocker:mainfrom
Open
remote-runtime: close silent gaps, consolidate Runtime, scaffold wire (10 baby steps)#2723dgageot wants to merge 10 commits intodocker:mainfrom
dgageot wants to merge 10 commits intodocker:mainfrom
Conversation
Member
|
Conflicts |
The TUI's createSessionSpawner was calling teamloader.LoadWithConfig directly with only WithModelOverrides, silently dropping --prompt-file on every session spawned with the 'open in working dir' affordance. Switch it to loadAgentFrom(ctx, loadTeamRequest(...)) so spawned sessions inherit the same flag set as the initial load. Sets up later steps that turn LoadTeamRequest into a wire payload: all callers now go through one function.
Plug RemoteRuntime into runRuntimeContract via a stub RemoteClient. The new TestRemoteRuntime_Contract pins the same surface LocalRuntime already passes, so any silent no-op slipping into RemoteRuntime becomes a red CI signal instead of a runtime user complaint. The only contract gap surfaced was SetCurrentAgent silently accepting any name. Fix it inline by validating against the agent list returned by client.GetAgent — same behaviour as LocalRuntime.SetCurrentAgent. Sets up the next step that audits the methods the contract doesn't cover (SessionStore, PermissionsInfo, etc.) and decides which are legitimately server-owned vs. quietly broken.
Two methods on RemoteRuntime were silently returning zero values that the TUI then treated as success: - CurrentAgentTools returned (nil, nil): the /tools dialog opened with an empty list, indistinguishable from 'agent has no tools'. Now returns ErrUnsupported so the dialog handler shows the actual reason (the wire protocol cannot enumerate server-side tools). - Summarize emitted a SessionSummary event with text 'not yet implemented'. The persistence observer writes every SessionSummaryEvent unchanged, so this corrupted the persisted session with a fake summary. Emit an Error event instead. Other zero-returning methods (SessionStore, PermissionsInfo, CurrentAgentSkillsToolset, TitleGenerator, CurrentAgentToolsetStatuses) are kept as-is: their callers already check for nil/empty as a 'server-owned' signal, and exposing them would require new wire endpoints rather than a typed error.
SetAgentModel(ctx, agentName, modelRef), AvailableModels(ctx) and a cheap SupportsModelSwitching() are now part of the Runtime interface. LocalRuntime delegates to its existing implementation; RemoteRuntime returns ErrUnsupported / nil / false. Callers no longer scatter type assertions: - App.SetCurrentAgentModel : runtime.ErrUnsupported -> 'not supported' - App.AvailableModels : SupportsModelSwitching gate - App.applySessionModelOverrides - cmd/root.createLocalRuntimeAndSession (resume path) Drop the ModelSwitcher interface and ModelSwitcherOf helper; the only optional sub-interface left in capabilities.go is ToolsChangeSubscriber, which the next step folds away the same way.
OnToolsChanged is now a regular Runtime method rather than an optional sub-interface. LocalRuntime keeps its existing emit machinery; RemoteRuntime is a no-op (tool-list changes already flow through the run-stream events server-side). Drop the ToolsChangeSubscriber interface and ToolsChangeSubscriberOf helper. capabilities.go is now empty save the package declaration; the next step removes the file entirely.
Both helpers it once held (ModelSwitcherOf, ToolsChangeSubscriberOf) were folded back into the Runtime interface in the previous two commits. The file was reduced to just its package declaration; remove it. The Runtime interface is now the single source of truth for what backends can do — no more 'is this thing also implementing some optional sub-interface' type assertions scattered around the codebase.
…kg/runtime The two structs lived in cmd/root because LocalBackend was the only consumer. They are about to gain JSON tags and a wire-protocol round trip; the natural home for that is pkg/runtime, alongside the Runtime interface they parametrize. cmd/root keeps the factory methods (loadTeamRequest / createSessionRequest) — those still read from the CLI flag struct and stay near it. The struct types themselves move; consumers now import them as runtime.LoadTeamRequest / runtime.CreateSessionRequest. No behavioural change; sets up the next step's JSON tags + round-trip tests.
LoadTeamRequest and CreateSessionRequest now marshal/unmarshal to and
from JSON. Simple flag-driven fields get explicit json:"..." tags
with omitempty; the wire-incompatible fields (Source interface,
RuntimeConfig with its sync.Mutex/Provider, permissions.Checker) get
json:"-" with a doc comment explaining the situation.
payload_test.go pins the contract:
- round trip preserves every tagged field
- the empty struct serializes to '{}' (no surprise zero-values on
the wire)
- the json:"-" fields stay out of the marshal output
This is the minimum data layer a future server handler can decode and
a future remote backend can encode. Three follow-ups make the excluded
fields wire-friendly (a tagged-union SourceSpec, a serializable
RuntimeConfig snapshot, a Patterns wire form for permissions); each is
its own commit.
backend.CreateRuntimeAndSession was doing two distinct things:
resolving the team description, then turning it into a runtime + a
session. Split them so the interface mirrors the two-step protocol a
future RPC server will expose:
LoadTeam(ctx, LoadTeamRequest) -> *teamloader.LoadResult
CreateSession(ctx, *LoadResult, CreateSessionRequest)
-> Runtime, *Session, cleanup
Plus paired LoadTeamRequest() / CreateSessionRequest(wd) factories so
the call site in runOrExec doesn't have to know which backend it
holds.
Side effect (deliberate): --dry-run for the LOCAL backend now exits
AFTER the team has been resolved, so a typo in the YAML is caught at
dry-run time. The remote dry-run path remains harmless — LoadTeam is
a no-op for remote, so no server-side session is created (the
guarantee the previous --dry-run fix introduced).
Sets up the next step's HTTP scaffolding: each handler decodes a
request, calls one of the backend methods, and re-encodes the
response.
Introduce pkg/runtime/wire as the (still WIP) HTTP transport for the
docker-agent server-mode story. Today it contains only the server side
of two endpoints:
POST /v1/team/load -> Backend.LoadTeam
POST /v1/session/create -> Backend.CreateSession
Both decode the JSON payloads added in the previous step
(LoadTeamRequest / CreateSessionRequest) and dispatch to a Backend
interface. The wire-side response shapes (LoadTeamResponse,
CreateSessionResponse) carry only what crosses the wire — the
LoadResult's live toolset handles stay server-side.
Server contract (pinned by tests):
- happy-path round trip preserves every tagged request field and
every tagged response field
- Backend errors map to 500
- errors.Is(err, runtime.ErrUnsupported) maps to 501 so a future
wire.Client driving RemoteRuntime can surface 'feature not
supported by this server' clearly
- unknown JSON fields are rejected with 400
Out of scope for this commit (each is its own follow-up):
- wire.Client driving RemoteRuntime
- /v1/session/run streaming endpoint (Server-Sent Events or chunked
JSON over HTTP/2)
- authentication
- bridging cmd/root.backend onto wire.Backend so the server actually
runs a real LocalBackend rather than a stub
dgageot
force-pushed
the
remote-runtime-phase-b
branch
from
ea5b099 to
8ca35dd
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
2 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.
Goal
Pick up phase A+B+C from the previous "remote-runtime baby steps" PR: close the silent-breakage gaps
--remotestill has compared to local mode, finish consolidating theRuntimeinterface, and stand up the data layer + HTTP scaffold that a real docker-agent server will eventually live behind.Ten Kent-Beck-style baby steps. Each commit:
task lint(golangci-lint + custom checks +go mod tidy),task test(full suite),Commits
fix(run): route session spawner through loadTeamRequestteamloader.LoadWithConfigdirectly with onlyWithModelOverrides, silently dropping--prompt-fileon every spawned session. Funnel it throughloadAgentFrom(ctx, loadTeamRequest(...))so spawned sessions see the same flag set as the initial load.test(runtime): run the contract suite against RemoteRuntimeTestRemoteRuntime_ContractplugsRemoteRuntimeintorunRuntimeContractvia a stub client. All 14 non-streaming subtests now pin the surface for both runtimes. The one gap surfaced wasSetCurrentAgentsilently accepting any name; fix it inline by validating against the agent list returned byclient.GetAgent.feat(runtime): wrap remaining RemoteRuntime no-ops with ErrUnsupportedCurrentAgentToolsreturned(nil, nil)(the /tools dialog opened with an empty list, indistinguishable from "agent has no tools") andSummarizeemitted aSessionSummaryevent with text "not yet implemented" (the persistence observer wrote it to the session DB unchanged, corrupting the session). Both now surfaceErrUnsupported/Errorevents.refactor(runtime): promote ModelSwitcher onto RuntimeSetAgentModel/AvailableModels/SupportsModelSwitchingare now part of theRuntimeinterface.LocalRuntimedelegates to its existing implementation;RemoteRuntimereturnsErrUnsupported. Drop theModelSwitcherinterface andModelSwitcherOfhelper; 5 call sites simplified.refactor(runtime): promote ToolsChangeSubscriber onto RuntimeOnToolsChangedis a regularRuntimemethod now;RemoteRuntimeis a no-op (tool-list changes flow through the run-stream events server-side). Drop the interface and helper.refactor(runtime): delete capabilities.goRuntimeis now the single source of truth for what backends can do — no more "is this thing also implementing some optional sub-interface" type assertions.refactor(runtime): move LoadTeamRequest and CreateSessionRequest to pkg/runtimecmd/rootintopkg/runtime/payload.goso they can grow JSON tags and a server side. Factory methods (loadTeamRequest,createSessionRequest) stay near the CLI flag struct they read from.feat(runtime): add JSON tags and round-trip tests for payloadsLoadTeamRequestandCreateSessionRequestnow marshal/unmarshal cleanly. Wire-incompatible fields (Sourceinterface,RuntimeConfig,permissions.Checker) getjson:"-"with a doc comment; round trip + omitempty + non-serializable-fields-excluded contracts pinned by 6 tests.refactor(run): split backend interface into LoadTeam and CreateSessionbackendinterface. Side-effect: local--dry-runnow validates the team (catches YAML typos at dry-run time). Remote--dry-runis unchanged —LoadTeamis a no-op for remote, so the previous PR's "no leaked server-side session" guarantee still holds.feat(runtime): scaffold pkg/runtime/wire HTTP handlerPOST /v1/team/loadandPOST /v1/session/createdecode the payloads from step 8 and dispatch to aBackendinterface. Round-trip + error mapping pinned by 5 tests: backend errors → 500,errors.Is(err, runtime.ErrUnsupported)→ 501, unknown JSON fields → 400.What's still out of scope (each its own follow-up)
wire.ClientdrivingRemoteRuntime/v1/session/runstreaming endpoint (SSE or chunked HTTP/2)Source/RuntimeConfig/permissions.Checker(today they'rejson:"-")cmd/root.backendontowire.Backendso the server actually runs a realLocalBackendrather than a stubValidation
Plus smoke tests on both
--remote http://nosuchhost:9999 --dry-run(exits without contacting the server) and local--dry-run(now also validates the team config).