Better tool registry#2807
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
+ Simpler interface Signed-off-by: David Gageot <david.gageot@docker.com>
Signed-off-by: David Gageot <david.gageot@docker.com>
Signed-off-by: David Gageot <david.gageot@docker.com>
Signed-off-by: David Gageot <david.gageot@docker.com>
…tructors) Signed-off-by: David Gageot <david@gageot.net> Assisted-By: docker-agent
Drop resolveToolsetWorkingDir and checkDirExists helpers in favor of
workingdir.Resolve and workingdir.CheckDirExists, mirroring the MCP
toolset's approach. This ensures consistent behavior (~ and ${VAR}
expansion, error wording) across all toolsets.
Signed-off-by: David Gageot <david@gageot.net>
Assisted-By: docker-agent
Delete resolveToolsetWorkingDir and its tests (TestResolveToolsetWorkingDir, TestResolveToolsetWorkingDir_EnvVarExpansion). The workingdir package is the single source of truth for working directory resolution. Signed-off-by: David Gageot <david@gageot.net> Assisted-By: docker-agent
Create pkg/tools/toolsetpath with a Resolve helper that both memory and tasks toolsets now call. This eliminates the duplicate resolveToolsetPath implementations and ensures consistent path resolution behavior across all toolsets that need to validate file/directory paths. Signed-off-by: David Gageot <david@gageot.net> Assisted-By: docker-agent
Change "should have been resolved from ref" to "requires either a rag_config block or a ref" to avoid leaking internal expectations to YAML authors who might be hand-writing configs. Signed-off-by: David Gageot <david@gageot.net> Assisted-By: docker-agent
Add comment explaining why os.Environ() is appended after expanding
toolset.Env: EnvProvider is used only to expand ${...} references,
while the subprocess still needs access to the full host environment.
Signed-off-by: David Gageot <david@gageot.net>
Assisted-By: docker-agent
|
/review |
dgageot
force-pushed
the
better-tool-registry
branch
from
a802e89 to
e89c09c
Compare
|
❌ PR Review Failed — The review agent encountered an error and could not complete the review. View logs. |
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Large but largely mechanical refactor — and unusually reviewable thanks to the commit-per-toolset cadence. Each step compiles and is well-scoped. CI is green; local build/test/vet/gofmt all pass. Approving.
Highlights:
pkg/tools/workingdir+pkg/tools/toolsetpathare exactly the right size — small, focused, with their own tests, andResolvesemantics documented up-front.- The end-to-end
working_dirregression tests (TestCreateMCPTool_WorkingDir_ReachesSubprocess,TestCreateLSPTool_WorkingDir_ReachesSubprocess) are the kind that would have caught the original bug they defend against. - Moving
lifecyclePolicyFromConfigintopkg/tools/lifecycleremoves a layering inversion — clean win.
Findings below are non-blocking polish.
| } | ||
|
|
||
| // TestCreateMCPTool_WorkingDir_ReachesSubprocess verifies that working_dir is | ||
| } // TestCreateMCPTool_WorkingDir_ReachesSubprocess verifies that working_dir is |
There was a problem hiding this comment.
Nit — Stray missing newline: the closing } of TestCreateMCPTool_GatewayRefMissingBinaryStillBuilds is on the same line as the next function's leading comment (} // TestCreateMCPTool_WorkingDir_…). gofmt accepts it, but it's the only place in the file with that style.
| } | ||
|
|
||
| // Default returns the configured agent working directory or the process cwd. | ||
| func Default(agentWorkingDir string) string { |
There was a problem hiding this comment.
Nit — Default is exported but has zero callers. Meanwhile pkg/tools/builtin/filesystem/filesystem.go (~L130–137) still inlines the same os.Getwd() fallback. Either route filesystem through workingdir.Default or drop the exported helper.
| @@ -43,29 +30,74 @@ import ( | |||
| // configName identifies the agent config file (e.g. "memory_agent" from "memory_agent.yaml"). | |||
| type ToolsetCreator func(ctx context.Context, toolset latest.Toolset, parentDir string, runConfig *config.RuntimeConfig, configName string) (tools.ToolSet, error) | |||
There was a problem hiding this comment.
Question — ToolsetCreator is exported, but the new ToolsetRegistry interface only exposes CreateTool — there's no public Register method anymore. External implementations (e.g. ACP) wrap NewDefaultToolsetRegistry() and forward; they never construct a ToolsetCreator. Consider unexporting it if registration is closed.
| return NewWithPath(db, validatedMemoryPath), nil | ||
| } | ||
|
|
||
| func New(manager DB) *ToolSet { |
There was a problem hiding this comment.
Nit — After the rename, New(manager) (no path) is only used in tests; production code goes through NewWithPath. Worth folding the two into one constructor with an optional path, or marking New test-only / unexporting it.
| // host environment. EnvProvider is used only to expand ${...} references | ||
| // in toolset.Env; the subprocess still needs access to the full environment. | ||
|
|
||
| env = append(env, os.Environ()...) |
There was a problem hiding this comment.
Nit — The new comment explaining the os.Environ() re-append is helpful — but the same logic is repeated verbatim (without the comment) in script_shell.go, lsp.go, mcp.go. Consider hoisting to a small expandedEnv(ctx, toolset.Env, runConfig) helper alongside workingdir.Resolve — same shape every time.
4 participants
No description provided.