feat(tools): add workspace tool overrides via /workspace/tools/<name>/#31949
feat(tools): add workspace tool overrides via /workspace/tools/<name>/#31949vellum-apollo-bot[bot] wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18d7049a1b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // The loader owns its own per-tool isolation contract; a thrown error | ||
| // here is unexpected and we let it surface so startup fails loudly | ||
| // rather than silently mounting half a tools layout. | ||
| await loadWorkspaceTools(); |
There was a problem hiding this comment.
This startup path now dynamic-imports code from <workspaceDir>/tools/*, so writing there becomes a code-injection vector on next restart, but the risk pipeline was not updated to treat that path like plugins/. buildFileContext() still sends only hooksDir/pluginsDir (no tools dir), and gateway/src/risk/file-risk-classifier.ts only escalates writes for hooks/plugins, so file_write/file_edit to workspace/tools remain low/medium instead of high. In practice, an agent can create or modify a workspace override with a weaker approval prompt than intended and gain persistent tool-execution control after restart.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to follow-up PR per @dvargasfuertes's note above on the same write-classifier change. Keeping the override path in this PR scoped to discovery / registry semantics; the classifier bump to "high" will land alongside the ToolDefinition generalization in a separate PR.
| function isValidToolDirName(name: string): boolean { | ||
| if (name.length === 0) return false; | ||
| if (name.startsWith(".")) return false; | ||
| if (name.includes("/") || name.includes("\\")) return false; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The validation here says it avoids silent renames, but it only rejects empty/dot/slash names; names with spaces or punctuation still pass and are later rewritten by withProviderSafeToolName() during registration. That breaks the documented “directory name is registered verbatim” contract and can make tools unexpectedly appear under hashed names (or collide after sanitization), which is hard to diagnose for operators. This should validate against provider-safe naming up front and skip/fail clearly instead of silently remapping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 150a146 — the loader now validates each filename stem via isProviderSafeToolName (/^[a-zA-Z0-9_-]{1,64}$/) and logs at error + skips when the stem doesn't pass. Documented in workspace-tools.md and covered by the filename stems that fail provider-safe validation are skipped test.
| > **Looking to override a single core tool?** Plugins can't — same-name | ||
| > collisions with core tools are warn-and-skipped to keep ownership | ||
| > unambiguous. Use a [workspace tool override](workspace-tools.md) | ||
| > instead: drop a single `tool.ts` under `<workspaceDir>/tools/<name>/` | ||
| > and it authoritatively replaces the core tool. |
There was a problem hiding this comment.
This is too high. just make this distinction in the tools section below. Also this doc is actually the legacy plugin architecture. Mention it at the top and that the modern plugin schema is under active development at /experimental/plugins/README.md
There was a problem hiding this comment.
Done in 150a146: callout moved down to the Tools (plugin.tools) subsection in plugins.md, and the header now flags this document as the legacy plugin architecture with a pointer to experimental/plugins/README.md for the modern schema.
| @@ -0,0 +1,197 @@ | |||
| # Workspace Tool Overrides | |||
|
|
|||
| Workspace tool overrides let the operator replace a core daemon tool (or | |||
There was a problem hiding this comment.
| Workspace tool overrides let the operator replace a core daemon tool (or | |
| Workspace tool overrides let the operator replace a core assistant tool (or |
Replace the word daemon with assistant wherever we see it
There was a problem hiding this comment.
Done in 150a146 — every daemon reference in workspace-tools.md is now assistant.
| This page explains the directory convention, lifecycle position, and the | ||
| "single canonical source per name" invariant the design is built around. | ||
|
|
||
| ## When to use this vs. a plugin |
There was a problem hiding this comment.
Delete this section
There was a problem hiding this comment.
Done in 150a146 — the When to use this vs. a plugin section is deleted.
| ``` | ||
| <workspaceDir>/ | ||
| tools/ | ||
| <toolName>/ |
There was a problem hiding this comment.
we should just support <workspaceDir>/tools/<toolName>.{ts|js|json} to start, no readme or additional files needed
There was a problem hiding this comment.
Done in e5e8f2e (code) + 150a146 (docs). The loader now scans <workspaceDir>/tools/<name>.{ts,js,json} directly — no subdirectory, no README, no helper files at the loader level. .js wins over .ts when both exist for the same stem (compiled-binary semantics); .json loads as a declarative spec with a default error-result executor. Shadowed siblings get a warn log so the operator can clean them up.
| ```ts | ||
| // /workspace/tools/skill_load/tool.ts | ||
| import type { | ||
| PluginTool, |
There was a problem hiding this comment.
In a new PR before we merge this one, let's generalize this to ToolDefinition
There was a problem hiding this comment.
Acknowledged — I'll open a separate PR for the ToolDefinition generalization before this one merges, per the rule of thumb "review feedback that forces a new abstraction is a follow-up, not an expansion." The new PR will rename the example import + carry the matching downstream changes.
| its place. Removing the workspace directory (and restarting the daemon) | ||
| causes the original to resurface — workspace tools are not destructive | ||
| to the core baseline. |
There was a problem hiding this comment.
We should support removing core tools from this API somehow
There was a problem hiding this comment.
Done in e5e8f2e — added a <name>.removed sentinel file convention. Dropping (e.g.) <workspaceDir>/tools/host_bash.removed strips the same-named core tool from the registry without substituting a replacement. The original is stashed in the same map used for override-style stashing, so removing the sentinel restores the core tool on the next reconcile. Registry-side: new removeCoreToolViaWorkspace / restoreStrippedCoreTool / getStrippedCoreToolNames helpers. All non-workspace surfaces (skill / plugin / MCP) refuse to fill a name whose slot is reserved by a .removed stash. Documented in workspace-tools.md.
| - **Collision with a workspace tool already in the registry** → hard error. | ||
| Filesystem constraints make this practically impossible (two | ||
| directories cannot share a name), but the registry double-checks. |
There was a problem hiding this comment.
bc filesystem makes this impossible, remove
There was a problem hiding this comment.
Done in 150a146 — section removed. With the single-file design, two files can't share an extension+stem pair on disk, so the filesystem-uniqueness invariant doesn't need explanatory prose anymore.
| - **Collision with a skill / plugin / MCP tool of the same name** → the | ||
| workspace registration throws. This indicates a lifecycle-order | ||
| regression: workspace tools must register before other extension | ||
| categories, and seeing one of these owners here means the daemon | ||
| startup order was changed without updating this contract. |
There was a problem hiding this comment.
should be noted that this is impossible due to the namespacing that those do
There was a problem hiding this comment.
Done in 150a146 — workspace-tools.md now notes that skill / plugin / MCP collisions with a workspace tool name are impossible at the cross-surface level because each of those surfaces runs in its own owner-scoped registry path, and the workspace registration's stamping is authoritative regardless of pre-existing ownership metadata on the incoming tool.
| Removing the `<workspaceDir>/tools/<name>/` directory and restarting the | ||
| daemon is the supported way to undo a workspace tool. The runtime | ||
| `unregisterWorkspaceTool(name)` API exists for tests and for future | ||
| file-watcher integration — it restores the stashed core tool when one | ||
| exists, or simply deletes the entry when the workspace tool was net-new. |
There was a problem hiding this comment.
we shouldn't need to restart the assistant to register or unregister, file watchers should do this for us without restarting
There was a problem hiding this comment.
Done in da42dd4 (watcher) + 150a146 (docs). New WorkspaceToolsWatcher singleton wraps fs.watch on <workspaceDir>/tools/ non-recursively, debounces per filename stem, and reconciles registry state by re-deriving the world from disk on each event — same eventual-consistency pattern as the plugin source watcher. Started in DaemonServer.start() after the initial scan completes, stopped in DaemonServer.stop(). No assistant restart required to register / unregister / hot-edit a workspace tool.
| There is no in-process hot-reload for workspace tools today. If you | ||
| edit `tool.ts`, restart the daemon to pick up the change. | ||
|
|
||
| ## Diagnostic surface |
There was a problem hiding this comment.
delete this section
The author-facing tool spec that plugin tool files and workspace tool files default-export. Lives at `@vellumai/plugin-api`'s public surface, type-only, structurally identical to the existing internal `PluginTool` shape so plugin authors can migrate by changing only the import name. This is the type the upcoming workspace-tool authoring guide (PR #31949) will reference instead of the daemon-internal `PluginTool` name, which was never publicly exported. Workspace tools and plugin tools share this surface but differ in their default risk floor: plugin tools default to \"medium\" (in-tree-vetted code), workspace tools default to \"high\" (operator-authored on-disk code). The default switch lives in each loader; this PR just adds the shared author-facing type. The name `ToolDefinition` collides with an internal type of the same name in `@vellumai/skill-host-contracts` that represents the JSON-schema bundle sent to providers. The doc comment on the new interface flags this — the imports are disjoint (different packages), so plugin authors always land on the right one. Tests cover full / empty / each risk-level literal and the narrow ToolContext exposure.
vellum-apollo-bot
Bot
force-pushed
the
apollo/workspace-tool-overrides
branch
from
18d7049 to
b25d93c
Compare
Per Vargas's PR #31949 review: workspace-tools.md: - Rewrite for single-file convention (`<workspaceDir>/tools/<name>.{ts,js,json}`) - Document the `<name>.removed` sentinel for stripping core tools without a replacement - Document the filesystem watcher and remove the "restart the daemon" workflow - Replace 'daemon' → 'assistant' throughout - Delete the "When to use this vs. a plugin" comparison section - Delete the "Diagnostic surface" section - Drop the filesystem-impossibility item (single-file design means filesystem-level uniqueness is implicit, no explanatory text needed) - Note that skill / plugin / MCP collisions with a workspace tool name cannot bypass the workspace override by registering earlier plugins.md: - Mark the legacy plugin architecture and point new plugins at `experimental/plugins/README.md` (modern schema under active development) - Relocate the workspace-tool override callout from the document header down to the "Tools (plugin.tools)" subsection where it belongs contextually - Update the callout's example to the new single-file path Generalization to `ToolDefinition` and the `"high"`-risk write-classification switch are tracked for a follow-up PR per Vargas's note in the review thread.
Iteration round 2 — pushed e5e8f2e · da42dd4 · 150a146@dvargasfuertes — addressed every inline comment from your round-1 review. Highlights: Single-file convention (was:
Filesystem watcher — Doc rewrite of Tests: 13 loader tests (single-file, Deferred to follow-up PR (per your inline notes):
Will open that follow-up before this one merges. |
Workspace tools are user-authored files dropped under
`<workspaceDir>/tools/<name>.{ts,js,json}` that authoritatively replace
same-named core tools at registry-load time. The original core tool is
stashed and restored on workspace-tool unregister, so workspace overrides
are non-destructive to the core baseline.
Highlights:
- New `registerWorkspaceTools` / `unregisterWorkspaceTool` /
`removeCoreToolViaWorkspace` / `restoreStrippedCoreTool` registry
entry points. Ownership is tracked in the existing `ownersByName` map
(`kind: "workspace"`, `id: <workspacePath>`) — no extra fields land
on the `Tool` object, keeping the post-#32631 `Required<ToolDefinition>`
shape pristine.
- Workspace ownership wins over plugin / skill / MCP: registrations of
the same name from those surfaces warn-skip (or throw, for skills) so
the workspace override stays canonical for its lifetime.
- `<name>.removed` sentinel strips a core tool from the live registry
while keeping it in the stash. Deleting the sentinel restores it.
Workspace-owned names can't be sentinel-stripped (the user must
remove the workspace tool file first).
- File watcher (`workspace-tools-watcher.ts`) drives hot reload:
re-imports on change, unregister on delete, re-strip / re-restore on
sentinel toggle. Uses `getToolOwner` to detect workspace ownership
(no `origin` field needed).
- Loader (`workspace-tools/loader.ts`) delegates default-filling to the
shared `finalizeTool` from `tool-defaults.ts`, layering only two
workspace-specific overrides on top: `defaultRiskLevel: "high"` (user
code from disk runs with full privilege) and a workspace-flavored
default `execute` error message.
Followup items from PR #31949 round-1 review (now incorporated):
- Generalized the `workspace-tools.md` doc example to use
`ToolDefinition` instead of `PluginTool` (comment 3295343340).
- Default-risk-level table entry shows `"high"` (comment 3295345328).
Followup from PR #32683 review (also incorporated):
- `QuestionPrompter` dropped its `{ broadcastMessage }` constructor arg
and imports `broadcastMessage` directly from `runtime/assistant-event-hub.js`.
Updated `ask-question-tool.ts` caller + `question-prompter.test.ts`
setup (the test now mocks the module export rather than injecting via
constructor; `mock.module` calls in that file also spread the real
module exports so other tests in the same `bun test` run still see
e.g. `assistantEventHub` and `setNestedValue`).
Tests: 88/88 passing across the 4 critical files (registry.test.ts,
workspace-tool-loader.test.ts, question-prompter.test.ts,
ask-question-tool.test.ts).
vellum-apollo-bot
Bot
force-pushed
the
apollo/workspace-tool-overrides
branch
from
150a146 to
56771cf
Compare
|
Rebased onto Conflict resolution (~325 commits behind main):
Round-1 feedback (deferred at the time) now incorporated:
Followup from #32683 also incorporated here:
Verification: 88/88 passing across the 4 critical files ( |
Workspace tools are user-authored files dropped under
`<workspaceDir>/tools/<name>.{ts,js,json}` that authoritatively replace
same-named core tools at registry-load time. The original core tool is
stashed and restored on workspace-tool unregister, so workspace overrides
are non-destructive to the core baseline.
Highlights:
- New `registerWorkspaceTools` / `unregisterWorkspaceTool` /
`removeCoreToolViaWorkspace` / `restoreStrippedCoreTool` registry
entry points. Ownership is tracked in the existing `ownersByName` map
(`kind: "workspace"`, `id: <workspacePath>`) — no extra fields land
on the `Tool` object, keeping the post-#32631 `Required<ToolDefinition>`
shape pristine.
- Workspace ownership wins over plugin / skill / MCP: registrations of
the same name from those surfaces warn-skip (or throw, for skills) so
the workspace override stays canonical for its lifetime.
- `<name>.removed` sentinel strips a core tool from the live registry
while keeping it in the stash. Deleting the sentinel restores it.
Workspace-owned names can't be sentinel-stripped (the user must
remove the workspace tool file first).
- File watcher (`workspace-tools-watcher.ts`) drives hot reload:
re-imports on change, unregister on delete, re-strip / re-restore on
sentinel toggle. Uses `getToolOwner` to detect workspace ownership
(no `origin` field needed).
- Loader (`workspace-tools/loader.ts`) delegates default-filling to the
shared `finalizeTool` from `tool-defaults.ts`, layering only two
workspace-specific overrides on top: `defaultRiskLevel: "high"` (user
code from disk runs with full privilege) and a workspace-flavored
default `execute` error message.
Followup items from PR #31949 round-1 review (now incorporated):
- Generalized the `workspace-tools.md` doc example to use
`ToolDefinition` instead of `PluginTool` (comment 3295343340).
- Default-risk-level table entry shows `"high"` (comment 3295345328).
Followup from PR #32683 review (also incorporated):
- `QuestionPrompter` dropped its `{ broadcastMessage }` constructor arg
and imports `broadcastMessage` directly from `runtime/assistant-event-hub.js`.
Updated `ask-question-tool.ts` caller + `question-prompter.test.ts`
setup (the test now mocks the module export rather than injecting via
constructor; `mock.module` calls in that file also spread the real
module exports so other tests in the same `bun test` run still see
e.g. `assistantEventHub` and `setNestedValue`).
Tests: 88/88 passing across the 4 critical files (registry.test.ts,
workspace-tool-loader.test.ts, question-prompter.test.ts,
ask-question-tool.test.ts).
vellum-apollo-bot
Bot
force-pushed
the
apollo/workspace-tool-overrides
branch
from
56771cf to
f0738d5
Compare
Workspace tools are user-authored files dropped under
`<workspaceDir>/tools/<name>.{ts,js,json}` that authoritatively replace
same-named core tools at registry-load time. The original core tool is
stashed and restored on workspace-tool unregister, so workspace overrides
are non-destructive to the core baseline.
Highlights:
- New `registerWorkspaceTools` / `unregisterWorkspaceTool` /
`removeCoreToolViaWorkspace` / `restoreStrippedCoreTool` registry
entry points. Ownership is tracked in the existing `ownersByName` map
(`kind: "workspace"`, `id: <workspacePath>`) — no extra fields land
on the `Tool` object, keeping the post-#32631 `Required<ToolDefinition>`
shape pristine.
- Workspace ownership wins over plugin / skill / MCP: registrations of
the same name from those surfaces warn-skip (or throw, for skills) so
the workspace override stays canonical for its lifetime.
- `<name>.removed` sentinel strips a core tool from the live registry
while keeping it in the stash. Deleting the sentinel restores it.
Workspace-owned names can't be sentinel-stripped (the user must
remove the workspace tool file first).
- File watcher (`workspace-tools-watcher.ts`) drives hot reload:
re-imports on change, unregister on delete, re-strip / re-restore on
sentinel toggle. Uses `getToolOwner` to detect workspace ownership
(no `origin` field needed).
- Loader (`workspace-tools/loader.ts`) delegates default-filling to the
shared `finalizeTool` from `tool-defaults.ts`, layering only two
workspace-specific overrides on top: `defaultRiskLevel: "high"` (user
code from disk runs with full privilege) and a workspace-flavored
default `execute` error message.
Followup items from PR #31949 round-1 review (now incorporated):
- Generalized the `workspace-tools.md` doc example to use
`ToolDefinition` instead of `PluginTool` (comment 3295343340).
- Default-risk-level table entry shows `"high"` (comment 3295345328).
Followup from PR #32683 review (also incorporated):
- `QuestionPrompter` dropped its `{ broadcastMessage }` constructor arg
and imports `broadcastMessage` directly from `runtime/assistant-event-hub.js`.
Updated `ask-question-tool.ts` caller + `question-prompter.test.ts`
setup (the test now mocks the module export rather than injecting via
constructor; `mock.module` calls in that file also spread the real
module exports so other tests in the same `bun test` run still see
e.g. `assistantEventHub` and `setNestedValue`).
Tests: 88/88 passing across the 4 critical files (registry.test.ts,
workspace-tool-loader.test.ts, question-prompter.test.ts,
ask-question-tool.test.ts).
devin-ai-integration
Bot
force-pushed
the
apollo/workspace-tool-overrides
branch
from
f0738d5 to
0a24f23
Compare
…ngleton watcher - Call loadWorkspaceTools() from inside initializeTools() instead of a separate providers-setup step. - Give WorkspaceToolsWatcher a static getInstance() singleton and drop the constructor options arg. - Start/stop the watcher via WorkspaceToolsWatcher.getInstance() rather than holding it as a DaemonServer field. - Drop the workspace-tools doc additions from plugins.md (moved to a separate PR). Co-Authored-By: vargas@vellum.ai <vargas@vellum.ai>
Keep the functional "workspace" OwnerKind union member; drop the verbose workspace-specific prose from the OwnerKind/OwnerInfo docstrings. Co-Authored-By: vargas@vellum.ai <vargas@vellum.ai>
1 participant
Adds a workspace tool override mechanism: dropping
<name>.{ts,js,json}under<workspaceDir>/tools/registers a new tool or authoritatively replaces a same-named core tool (the original is stashed and restored when the file is removed), while a<name>.removedsentinel strips a core tool outright. Overrides load insideinitializeTools()before MCP / plugin registration so workspace tools own their names, are hot-reloaded by theWorkspaceToolsWatcher.getInstance()singleton, and stay isolated per file so one broken override cannot block startup; a single file per name gives exactly one canonical source, which is what makes the override safe without conflict-resolution UX.