fix(owletto-backend): re-register list_watchers/get_watcher/read_knowledge for REST#434
Merged
Conversation
…ledge for REST callers The frontend's watchers page broke with `Tool not found: list_watchers` because PR #348 deleted these MCP-tool registrations. The plan was to migrate everything to `execute` + `client.<ns>.<method>`, but the frontend (`apiCall('list_watchers'…)` in `use-watchers.ts`) and owletto-cli still need the named handlers via the REST proxy. The MCP boundary stays small. The REST/CLI boundary regains the names. - Re-register `list_watchers`, `get_watcher`, `read_knowledge` as `internal: true` so they're hidden from MCP `tools/list` but reachable via `POST /api/{slug}/{toolName}` (the REST proxy sets `allowInternalTools=true`). - Rename `LEGACY_ADMIN_TOOLS` → `INTERNAL_REST_TOOLS`. They're not legacy — they're the deliberate REST/CLI surface. Consolidate the 12 near-duplicate `ToolDefinition` literals into a compact `ENTRIES` array, and pass handlers to `defineTool` directly instead of wrapping each in a pass-through `async (args, env, ctx) => handler(args, env, ctx)`. Same simplification applied to the public `TOOLS` array in `registry.ts`. - Add `ToolNotRegisteredError` and throw it from `checkToolAccess` when `getTool()` returns undefined (registry/frontend drift). The REST proxy now captures it to Sentry so the next "Tool not found" outage surfaces as an alert instead of a silent 400. Internal-tool hiding still throws a plain Error to avoid leaking handler existence. - Regression test in `tool-access.test.ts`: scan `packages/owletto-web/src` for every `apiCall(<…>)('toolName', …)` call site and assert each name is registered. `manage_queue` is in a `KNOWN_DEAD_NAMES` allowlist (the only frontend caller is the unused `useDeleteWindow` hook); the test fails the day someone wires it up without first registering a backend handler. Verified: removing `list_watchers` from the registry locally now fails the new test with `Received: ["list_watchers"]`.
github-actions
Bot
added
the
triage:needs-human
Contributor
|
Triage decision: Reasons:
Next: merge manually after final review — this is a hotfix for production watchers page errors |
…ered internal tools Direct assertion that list_watchers, get_watcher, and read_knowledge: - look like 'Tool not found' to external MCP callers, and - are reachable from the REST proxy when allowInternalTools=true. Complements the apiCall-coverage scan with explicit access-control coverage so the visibility split is hard to silently regress.
…P surfaces) Both the frontend and the CLI use the same dispatch (`POST /api/:orgSlug/:toolName` → `restToolProxy` → `executeTool` → `getTool(name)`), but the CLI's browser-auth flow goes through MCP RPC and needs its tools to ALSO stay visible on `tools/list` (i.e. NOT `internal: true`). - Scan packages/owletto-cli/src for `callTool(ctx, 'X')` patterns and assert each name is registered (matches the frontend's REST surface). - Scan browser-auth.ts for `name: 'X'` literals and assert each is registered AND non-internal — flipping `manage_connections` or `manage_auth_profiles` to internal would silently break the CLI's browser-auth flow. Three first-party tool callers are now covered: owletto-web (frontend REST), owletto-cli/seed (CLI REST), owletto-cli/browser-auth (CLI MCP).
- Frontend scanner missed hook-factory's `tool: 'X'` config form (api/entities.ts, api/connections.ts — 30+ sites). Combine apiCall(...) regex with a second tool: 'X' regex so removing any of those tools also fails the test. - CLI MCP test was filtering candidates to registered names before checking missing — meaning an outright deletion silently passed. Replace the regex-derived candidate set with a hardcoded `CLI_PUBLIC_MCP_TOOLS` allowlist, plus a separate drift detector that asserts browser-auth.ts's tools/call literals match the pinned set exactly. Verified the new test fails when manage_connections is flipped to internal:true. - Sentry tag cardinality: tool_name lives in `extra`, not `tags` — the URL segment is attacker-controlled and would blow up tag cardinality.
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
1 participant
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.
Summary
Hotfix the
Tool not found: list_watcherserror that broke the watchers page in prod. Restoreslist_watchers,get_watcher, andread_knowledgeto the REST/CLI tool surface (still hidden from external MCPtools/list), and adds a regression test that catches the same class of drift before it ships again.Why it broke
PR #348 deleted these MCP-tool registrations as part of the move to
execute+client.<ns>.<method>. The frontend'sapiCall('list_watchers', …)inpackages/owletto-web/src/hooks/use-watchers.tswas never migrated — andrestToolProxyswallowed the resultingTool not founderror as a 400 JSON body, so Sentry never fired. The existingauth.test.tsassertion atexpect(toolNames).not.toContain('list_watchers')codifies the MCP-surface removal, which is correct, but no test covers the REST proxy's view of the registry.Changes
list_watchers/get_watcher/read_knowledgeasinternal: trueinINTERNAL_REST_TOOLS(formerlyLEGACY_ADMIN_TOOLS). They stay hidden from MCPtools/list(which callsgetAllTools({ includeInternalTools: false })) butrestToolProxyreaches them because it setsallowInternalTools=true.LEGACY_ADMIN_TOOLS→INTERNAL_REST_TOOLS. Per the user's framing: the manage_/list_/get_* surface isn't legacy, it's the deliberate REST/CLI boundary alongside the smaller MCP boundary.ToolDefinitionliterals collapse into a compactENTRIESarray; handlers are passed directly todefineToolinstead of viaasync (args, env, ctx) => handler(args, env, ctx)pass-throughs. Same treatment applied to the publicTOOLSarray inregistry.ts.ToolNotRegisteredErrorthrown fromcheckToolAccesswhengetTool()returns undefined.restToolProxycaptures it before returning the 400 — internal-tool hiding still throws a plainErrorso we don't leak handler existence over MCP.tool-access.test.tsscans everyapiCall(<…>)('name', …)call site underpackages/owletto-web/srcand asserts each name is registered.manage_queueis inKNOWN_DEAD_NAMES(the only caller is the unuseduseDeleteWindowhook); the test will fail if anyone wires it up without registering the backend handler first.Verification
bun test packages/owletto-backend/src/auth/__tests__/tool-access.test.ts— 40 pass.list_watchersfromadmin/index.tslocally producesReceived: ["list_watchers"]and a red test.bun run typecheck,bun run lint,bun run format:check— all green.bun test packages/owletto-backend/src/__tests__/unit/sandbox) — 24 pass.Test plan
https://app.lobu.ai/{slug}/watchersloads instead of erroring.ToolNotRegisteredErrorSentry alerts in the hour after deploy.