refactor: cut heavy transitive deps (goja, expr, openai-go, go-git, sqlite) from the code-built embedder surface#3771
Merged
Conversation
pkg/runtime wired dmr.ListModels unconditionally, dragging the whole openai-go SDK (via the DMR provider client and oaistream) into every embedder that links the runtime, even Anthropic-only ones. Move endpoint resolution and /models listing - which only use net/http - into pkg/model/provider/dmr/dmrmodels and point the runtime at it. dmr.ListModels and dmr.ErrNotInstalled remain as thin forwarders so existing callers keep compiling and errors.Is keeps matching.
NewVCSMatcher used git.PlainOpen only to locate the worktree root, but importing the top-level go-git package links its whole object/transport stack (ssh, packfile, merkletrie, ...) into anything that touches the permission checker - i.e. every embedder of pkg/runtime. Locate the root by statting basePath/.git directly (PlainOpen did not search parent directories either; a gitfile still counts for linked worktrees) and read patterns via the lightweight plumbing/format/gitignore package over an osfs, exactly as before.
pkg/session mixed the in-memory store with the file-backed constructor, so every embedder of pkg/runtime linked modernc.org/sqlite (large and slow to compile) even when using purely in-memory sessions. session.NewSQLiteSessionStore(ctx, path) becomes sqlitestore.New(ctx, path); behavior (open, migrate, backup+retry recovery) is unchanged. SQLiteSessionStore, NewSQLiteSessionStoreFromDB and the migrations stay in pkg/session - they only use database/sql and don't drag the driver. BREAKING CHANGE: callers of session.NewSQLiteSessionStore must import github.com/docker/docker-agent/pkg/session/sqlitestore and call New.
…t package pkg/runtime and pkg/agent imported pkg/tools/mcp for a handful of small helpers, linking the entire MCP toolset - and through it expr-lang/expr (via toolinstall) and goja (via pkg/js) - into every embedder, even ones with zero MCP toolsets. - Move the interactive-prompt gating helpers, OAuth sentinel errors and PromptInfo/PromptArgument to pkg/tools; pkg/tools/mcp keeps aliases and forwarders so its public API is unchanged. - Move the OAuth authorization-code flow (PKCE/state, callback server, token exchange/refresh, dynamic client registration) to the new leaf pkg/tools/mcp/oauthflow, again with aliases and forwarders; only the upstream-header-scoping transport stays in pkg/tools/mcp. - Runtime prompt discovery now goes through a small mcpPromptToolset interface (satisfied by *mcp.Toolset) instead of a concrete type assertion.
pkg/runtime imported pkg/js solely to expand ${...} expressions in
slash-command instructions, linking the goja JavaScript engine into
every embedder of the runtime.
ResolveCommand now goes through a registered CommandEvaluator factory.
The new pkg/runtime/jscommands package registers the goja-backed
evaluator on import and is blank-imported by teamloader, the CLI and
embeddedchat/defaults, so every YAML/CLI path behaves exactly as
before. Code-built embedders that want JS command expressions opt in
with the same blank import; without it, ${...} is left unexpanded with
a warning pointing at the import.
Assert via go list -deps that the packages a code-built embedder links (runtime, agent, team, session, tools, anthropic provider) stay free of goja, expr, the OpenAI SDK, full go-git, the SQLite driver, pkg/js, pkg/tools/mcp and toolinstall, so the import-graph cuts don't silently regress. Also migrate e2e tests to sqlitestore.New.
golangci-lint's nolintlint flags it as unused; json.Marshal of the token cache no longer trips gosec.
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This is a well-structured dependency-reduction refactor. All seven hypotheses raised during review were dismissed after verification:
pkg/fsx/vcs.gogitfile parsing: The new lightweight.gitstat/parse correctly handles Windows CRLF line endings viastrings.TrimSpace, and the no-upward-search semantics are preserved and tested.pkg/session/sqlitestoredb lifecycle: Theopen()helper correctly closesdbexactly once on failure; no leak or double-close.commandEvaluatorFactorytest: Standard Go test pattern for swapping package-global atomic state; not parallel, so no data race.ListModelsAtcontext use after timeout: Theslog.DebugContextcall runs beforedefer cancel()fires; context is valid. Even a cancelled context doesn't suppressslogoutput.HTTPClientForAllowPrivateIPstype assertion: Pre-existing pattern moved verbatim; not introduced by this PR.- Cache TOCTOU in
NewVCSMatcher: The TOCTOU window between read and write lock yields only harmless idempotent writes. SetHTTPClientForTestingrestore ignored inTestMain: Intentional — the process exits immediately viaos.Exit, so no restore is needed.
The dependency-budget e2e test (e2e/dependencies_test.go) that locks the embedder surface against regression is a solid safeguard for the stated goals.
GetSessionMCPPrompts decodes the server response into map[string]any, so each value is a nested map, never a concrete tools.PromptInfo; the type assertion always failed and RemoteRuntime.CurrentMCPPrompts silently returned an empty map. Round-trip each value through JSON to get typed prompts. Found by review on the PR; the bug predates the PromptInfo move.
os.Rename replaces the destination, so a second migration failure clobbered the .bak from the first one - the last known-good copy of the user's sessions. When .bak already exists, move the broken database aside under a timestamped name instead.
- Cap non-200 response body reads at 512 bytes before embedding them in errors: OAuth error bodies come from the remote endpoint and end up in logs, so a bound keeps a hostile or misconfigured server from stuffing megabytes (or sensitive fields) into them. - Use a comma-ok assertion on http.DefaultTransport so a replaced transport (test helper, proxy shim) degrades to a fresh default instead of panicking.
trungutt
approved these changes
Jul 21, 2026
aheritier
approved these changes
Jul 21, 2026
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
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.
Go developers who build agents and teams in code (rather than YAML) were pulling in 322 → 1,054 linked packages — a +37.6 MB binary — because
pkg/runtime,pkg/agent,pkg/session, and the anthropic provider all hard-imported heavy runtimes (goja, expr, openai-go SDK, full go-git, modernc/sqlite) through their transitive dependency chains. None of that weight is needed just to wire up an agent.This change cuts the code-built embedder surface from 1,054 down to 719 linked packages by surgically breaking each heavyweight import chain. DMR endpoint resolution and
/modelslisting move to a newpkg/model/provider/dmr/dmrmodelsleaf that uses onlynet/http, sopkg/runtimeno longer links the openai-go SDK. The fullgo-gitimport inpkg/fsxis replaced by a lightweight.gitstat/parse that preserves identical no-upward-search semantics, keeping only thego-billyandplumbing/format/gitignoreplumbing. The file-backed SQLite session store moves topkg/session/sqlitestoreso callers that only need the in-memory store don't pay formodernc/sqlite. The goja JS command evaluator becomes pluggable viapkg/runtime/jscommands.Register(); YAML/CLI paths call it automatically throughteamloader,pkg/cli.Run,cmd/root, andembeddedchat/defaults, so there is no behavior change there. Code-built embedders that need${...}expansion opt in with one call. Finally,pkg/tools/mcp's interactive-prompt gating, OAuth sentinel errors, and the authorization-code flow are extracted intopkg/toolsand a newpkg/tools/mcp/oauthflowleaf so neitherpkg/runtimenorpkg/agentneed to import the MCP toolset package (which drags in expr viatoolinstalland goja viapkg/js).The one intentional breaking change is
session.NewSQLiteSessionStore→sqlitestore.New. All other public APIs are preserved via forwarders or aliases. An e2e dependency-budget test (e2e/dependencies_test.go) now asserts viago list -depsthat goja, expr, openai-go, full go-git, sqlite,pkg/js,pkg/tools/mcp, andtoolinstallremain absent from the embedder surface, so the budget cannot silently regress.One item from the original feedback — defining
tools.ToolAnnotationslocally — was not done because the type is also referenced by the runtime's elicitation/sampling handler signatures andpkg/tools' own handler types; removing the MCP go-sdk import there would require breaking those public handler types, which is out of scope here.