Skip to content

feat(mcp): native OpenClaw MCP via credential-injecting proxy - #298

Open
clemenshelm wants to merge 23 commits into
mainfrom
claude/cranky-aryabhata-563cc1
Open

feat(mcp): native OpenClaw MCP via credential-injecting proxy#298
clemenshelm wants to merge 23 commits into
mainfrom
claude/cranky-aryabhata-563cc1

Conversation

@clemenshelm

@clemenshelm clemenshelm commented May 7, 2026

Copy link
Copy Markdown
Contributor

Connect any MCP server to Pinchy through OpenClaw's native mcp.servers, with third-party tokens kept off disk behind a credential-injecting reverse proxy, per-agent tool gating, and a full audit trail.

How it works

OpenClaw's native mcp.servers.<key> points at a Pinchy proxy (/api/internal/mcp-proxy/<connectionId>), not the third-party server. The proxy authenticates the gateway bootstrap token, decrypts the real bearer token in memory, injects Authorization: Bearer <token>, and streams the request through. So:

  • No third-party token ever lands in openclaw.json — it lives encrypted in Pinchy's DB and only in this process's memory.
  • Token rotation / adding a connection needs no OpenClaw restart.
  • Per-agent gating rides the standard agents.list[].tools.allow (tool name = mcpServerKey(connId)__<tool>).
  • SSRF guard re-validates the upstream URL on every request (opt-out ALLOW_PRIVATE_URLS=1 for self-hosted internal servers).

Best-of-both-worlds re-integration

This branch was rebuilt from the ground up on current main: the proven MCP substance (proxy, native emission, E2E) is kept, while every parallel invention was replaced with main's now-existing generic mechanisms.

  • Permissions use main's generic agent_connection_permissions (model="mcp", operation=<toolName>) — the dedicated agent_mcp_tool_permissions table is gone.
  • Skills: each connection generates a dynamic per-connection skill from the agent's granted tools, with a safety block telling the model that server responses are data, not instructions. Nothing user-authored; it appears and disappears with the grants.
  • Feature flag: one server-side PINCHY_MCP_ENABLED, read at runtime and passed to the client as a prop (no NEXT_PUBLIC_ build-time inline — that would freeze the flag into a prebuilt image).

Presets & templates

Eight presets (GitHub, Linear, Atlassian, Stripe, Cloudflare, Intercom, HighLevel, and a generic "any MCP server" — the load-bearing one). Two agent templates (GitHub PR Reviewer, Linear Triage) with per-preset availability gating and auto-granting of their recommended tools on creation.

Verification

Every gate green, and the MCP E2E ran live against a real Docker stack: 10/10 — connection create + tool discovery, grants, per-tool gating, drift rejection, audit rows, the full round-trip dispatch (trigger → OpenClaw → proxy → mock → audit), and token rotation with no restart.

Docs added under guides/, reference/, and explanation/ (including the security-boundary write-up).


Rebased onto current main (119 commits); migration regenerated against the new baseline. One security fix surfaced by the rebase itself: main's credential-form drift guard (#743) caught two MCP forms that defaulted to a GET submit, which would have leaked the token into the URL — now method="post". Contains one deliberate, label-authorized test removal (dead toolPrefix field).

clemenshelm pushed a commit that referenced this pull request May 7, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
clemenshelm pushed a commit that referenced this pull request May 11, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from f42c279 to ec08025 Compare May 11, 2026 06:50
clemenshelm pushed a commit that referenced this pull request May 11, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from ec08025 to eb2aafb Compare May 11, 2026 07:23
clemenshelm pushed a commit that referenced this pull request May 12, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from eb2aafb to 2ccd808 Compare May 12, 2026 11:31
clemenshelm pushed a commit that referenced this pull request May 12, 2026
PR #298's UI refactor split the integration type picker out of the
modal into a dedicated `/settings/integrations/new` page (commit
82475f1). The "Add Integration" button became a `<Link>`, not a
modal trigger — but the Odoo wizard E2E tests still expected a
dialog to open directly on click. The happy-path test timed out
after 2 minutes waiting for a dialog that won't appear at the old
URL.

Update both Odoo wizard tests to follow the new flow:
1. Click the "Add Integration" link (now navigates)
2. Land on /settings/integrations/new
3. Click the Odoo tile — opens the wizard dialog with
   `initialType=odoo` so the connect step renders straight away
4. Resume the existing wizard flow

Other E2E specs were checked — only odoo-wizard.spec.ts uses the
Add-Integration click path; MCP / Email / Web Search specs all
seed via REST so they're unaffected.
clemenshelm pushed a commit that referenced this pull request May 13, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from 65ee7d2 to f2354aa Compare May 13, 2026 04:24
clemenshelm pushed a commit that referenced this pull request May 13, 2026
PR #298's UI refactor split the integration type picker out of the
modal into a dedicated `/settings/integrations/new` page (commit
82475f1). The "Add Integration" button became a `<Link>`, not a
modal trigger — but the Odoo wizard E2E tests still expected a
dialog to open directly on click. The happy-path test timed out
after 2 minutes waiting for a dialog that won't appear at the old
URL.

Update both Odoo wizard tests to follow the new flow:
1. Click the "Add Integration" link (now navigates)
2. Land on /settings/integrations/new
3. Click the Odoo tile — opens the wizard dialog with
   `initialType=odoo` so the connect step renders straight away
4. Resume the existing wizard flow

Other E2E specs were checked — only odoo-wizard.spec.ts uses the
Add-Integration click path; MCP / Email / Web Search specs all
seed via REST so they're unaffected.
clemenshelm pushed a commit that referenced this pull request May 13, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from f2354aa to 7bca2f4 Compare May 13, 2026 04:45
clemenshelm pushed a commit that referenced this pull request May 13, 2026
PR #298's UI refactor split the integration type picker out of the
modal into a dedicated `/settings/integrations/new` page (commit
82475f1). The "Add Integration" button became a `<Link>`, not a
modal trigger — but the Odoo wizard E2E tests still expected a
dialog to open directly on click. The happy-path test timed out
after 2 minutes waiting for a dialog that won't appear at the old
URL.

Update both Odoo wizard tests to follow the new flow:
1. Click the "Add Integration" link (now navigates)
2. Land on /settings/integrations/new
3. Click the Odoo tile — opens the wizard dialog with
   `initialType=odoo` so the connect step renders straight away
4. Resume the existing wizard flow

Other E2E specs were checked — only odoo-wizard.spec.ts uses the
Add-Integration click path; MCP / Email / Web Search specs all
seed via REST so they're unaffected.
clemenshelm pushed a commit that referenced this pull request May 13, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from 7bca2f4 to 9b78870 Compare May 13, 2026 07:58
clemenshelm pushed a commit that referenced this pull request May 13, 2026
PR #298's UI refactor split the integration type picker out of the
modal into a dedicated `/settings/integrations/new` page (commit
82475f1). The "Add Integration" button became a `<Link>`, not a
modal trigger — but the Odoo wizard E2E tests still expected a
dialog to open directly on click. The happy-path test timed out
after 2 minutes waiting for a dialog that won't appear at the old
URL.

Update both Odoo wizard tests to follow the new flow:
1. Click the "Add Integration" link (now navigates)
2. Land on /settings/integrations/new
3. Click the Odoo tile — opens the wizard dialog with
   `initialType=odoo` so the connect step renders straight away
4. Resume the existing wizard flow

Other E2E specs were checked — only odoo-wizard.spec.ts uses the
Add-Integration click path; MCP / Email / Web Search specs all
seed via REST so they're unaffected.
clemenshelm pushed a commit that referenced this pull request May 13, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
clemenshelm pushed a commit that referenced this pull request May 13, 2026
PR #298's UI refactor split the integration type picker out of the
modal into a dedicated `/settings/integrations/new` page (commit
82475f1). The "Add Integration" button became a `<Link>`, not a
modal trigger — but the Odoo wizard E2E tests still expected a
dialog to open directly on click. The happy-path test timed out
after 2 minutes waiting for a dialog that won't appear at the old
URL.

Update both Odoo wizard tests to follow the new flow:
1. Click the "Add Integration" link (now navigates)
2. Land on /settings/integrations/new
3. Click the Odoo tile — opens the wizard dialog with
   `initialType=odoo` so the connect step renders straight away
4. Resume the existing wizard flow

Other E2E specs were checked — only odoo-wizard.spec.ts uses the
Add-Integration click path; MCP / Email / Web Search specs all
seed via REST so they're unaffected.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from 9b78870 to aff36a5 Compare May 13, 2026 18:01
clemenshelm pushed a commit that referenced this pull request May 18, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
clemenshelm pushed a commit that referenced this pull request May 18, 2026
PR #298's UI refactor split the integration type picker out of the
modal into a dedicated `/settings/integrations/new` page (commit
82475f1). The "Add Integration" button became a `<Link>`, not a
modal trigger — but the Odoo wizard E2E tests still expected a
dialog to open directly on click. The happy-path test timed out
after 2 minutes waiting for a dialog that won't appear at the old
URL.

Update both Odoo wizard tests to follow the new flow:
1. Click the "Add Integration" link (now navigates)
2. Land on /settings/integrations/new
3. Click the Odoo tile — opens the wizard dialog with
   `initialType=odoo` so the connect step renders straight away
4. Resume the existing wizard flow

Other E2E specs were checked — only odoo-wizard.spec.ts uses the
Add-Integration click path; MCP / Email / Web Search specs all
seed via REST so they're unaffected.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from aff36a5 to 0acd70c Compare May 18, 2026 15:12
clemenshelm pushed a commit that referenced this pull request May 18, 2026
Main added a new email-dispatch E2E test (line 265+) during the
merge gap that sends the legacy `{connectionId, permissions}` PUT
body. PR #298 changed the route to require the unified array shape
with `kind: "odoo"|"mcp"` discriminator, so the test was getting a
400 on the permissions grant.

Update the dispatch probe's beforeAll to use the new shape — same
pattern the other email + odoo E2E specs already follow:

    [{ kind: "odoo", connectionId, entries: [...] }]

Email/Google connections share the agentConnectionPermissions table
with Odoo so they ride the same discriminator.
clemenshelm pushed a commit that referenced this pull request May 19, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
@clemenshelm
clemenshelm force-pushed the claude/cranky-aryabhata-563cc1 branch from fb5510c to a41a9cb Compare May 19, 2026 18:51
clemenshelm pushed a commit that referenced this pull request May 19, 2026
PR #298's UI refactor split the integration type picker out of the
modal into a dedicated `/settings/integrations/new` page (commit
82475f1). The "Add Integration" button became a `<Link>`, not a
modal trigger — but the Odoo wizard E2E tests still expected a
dialog to open directly on click. The happy-path test timed out
after 2 minutes waiting for a dialog that won't appear at the old
URL.

Update both Odoo wizard tests to follow the new flow:
1. Click the "Add Integration" link (now navigates)
2. Land on /settings/integrations/new
3. Click the Odoo tile — opens the wizard dialog with
   `initialType=odoo` so the connect step renders straight away
4. Resume the existing wizard flow

Other E2E specs were checked — only odoo-wizard.spec.ts uses the
Add-Integration click path; MCP / Email / Web Search specs all
seed via REST so they're unaffected.
clemenshelm pushed a commit that referenced this pull request May 19, 2026
Main added a new email-dispatch E2E test (line 265+) during the
merge gap that sends the legacy `{connectionId, permissions}` PUT
body. PR #298 changed the route to require the unified array shape
with `kind: "odoo"|"mcp"` discriminator, so the test was getting a
400 on the permissions grant.

Update the dispatch probe's beforeAll to use the new shape — same
pattern the other email + odoo E2E specs already follow:

    [{ kind: "odoo", connectionId, entries: [...] }]

Email/Google connections share the agentConnectionPermissions table
with Odoo so they ride the same discriminator.
clemenshelm pushed a commit that referenced this pull request Jun 8, 2026
Main added a new email-dispatch E2E test (line 265+) during the
merge gap that sends the legacy `{connectionId, permissions}` PUT
body. PR #298 changed the route to require the unified array shape
with `kind: "odoo"|"mcp"` discriminator, so the test was getting a
400 on the permissions grant.

Update the dispatch probe's beforeAll to use the new shape — same
pattern the other email + odoo E2E specs already follow:

    [{ kind: "odoo", connectionId, entries: [...] }]

Email/Google connections share the agentConnectionPermissions table
with Odoo so they ride the same discriminator.
clemenshelm pushed a commit that referenced this pull request Jun 8, 2026
Eight inline findings, all in test code, all in commits this PR
landed first. Two categories:

1. Unanchored host regex (5 findings):
   - `mcp-presets.test.ts` checks `tokenInstructions` markdown
     contains the provider's canonical token URL. Anchored each
     pattern on `https://` so an attacker-controlled prefix like
     `evil.com/github.com/...` couldn't false-match.
   - `add-integration-dialog.test.tsx` matched a link by its
     accessible name with a host regex. The link's visible text
     IS the URL substring (markdown displays the URL as label),
     so prefixing with `https://` would break the match. Switch
     to an exact-string `name` lookup plus an explicit
     `toHaveAttribute("href", "...")` assertion — both stricter
     than the original regex and CodeQL-clean.

2. Dead code in `pinchy-mcp/src/index.test.ts` (3 findings):
   - Drop unused `vi` import.
   - Drop the `mcpResponseOverride` mutable + its dead branch —
     never assigned a non-null value anywhere.
   - Drop the unused `let callNumber = 0;` in the 401-retry test.

5191 unit tests pass + plugin tests pass, prettier clean.
clemenshelm pushed a commit that referenced this pull request Jun 11, 2026
Six CI failures from PR #298, all rooted in three issues:

1. **PUT body shape regression.** The unified discriminated-union endpoint
   `[{kind:"odoo"|"mcp", connectionId, ...}]` replaced the legacy
   `{connectionId, permissions}` shape. The Odoo `setAgentPermissions`
   helper, the email E2E spec, and the odoo-agent-chat response parser
   were still on the old shape. Updated all three to the new shape.

2. **Sync eager-cascaded permissions instead of letting drift surface
   at GET time.** The MCP E2E spec explicitly asserts "permission row
   still exists — drift is detected at read time, not eagerly deleted
   by sync" but the implementation cascade-deleted in the sync
   transaction. Removed the cascade; drift detection in
   `agents/[id]/integrations` GET now finds the stale rows and reports
   them. Added a defensive filter in `openclaw-config/build.ts` so
   drifted permissions aren't emitted into the runtime config. Also
   wired up a failure-audit + 502 path for `listMcpTools` errors
   (review feedback). `toolPrefix` now resolves from the preset
   registry rather than a stale `data.toolPrefix` field.

3. **Test infrastructure misses for the new table + general E2E
   picking up MCP specs.** Added `agent_mcp_tool_permissions` to the
   integration-suite truncate list and excluded `e2e/mcp/**` from the
   general playwright config.

Format-only: `prettier --write` on `mcp-gate.test.ts`.

Verified locally: format check ✓, tsc ✓, lint 0 errors, 3881 vitest
tests pass.
@clemenshelm

Copy link
Copy Markdown
Contributor Author

Architecture pivot: native OpenClaw MCP + credential-injecting proxy (replaces the custom plugin)

After verifying against the running OpenClaw runtime, the original pinchy-mcp plugin turned out to be the wrong layer: OpenClaw's contracts.tools gate silently drops any plugin tool not statically declared in the manifest, which is fundamentally incompatible with dynamically discovered MCP tools — the core of the "generic, connect-any-server" feature. OpenClaw's native mcp.servers materializes tools per-run and bypasses that gate; it's the only blessed path for dynamic MCP tools.

The remaining problem with going native was secret handling (no SecretRef in mcp.servers.headers; env-templates need a gateway restart per connection). This PR solves it with a Pinchy credential-injecting reverse proxy:

  • mcp.servers.<key>.url/api/internal/mcp-proxy/<connectionId> (authed with the gateway bootstrap token). OpenClaw connects to Pinchy, not the third party.
  • The proxy decrypts the connection's bearer token in memory and injects it when forwarding to the upstream. No third-party secret in openclaw.json (Pattern B at the transport layer), and no gateway restart to add/rotate a connection (mcp.servers hot-reloads; rotation is config-free).
  • Governance preserved natively: per-agent tools.allow + pinchy-audit before/after_tool_call hooks fire for the materialized tools. SSRF-guarded via the existing validateExternalUrl.

Why tests were deleted (re: allow-test-deletion label)

The pinchy-mcp plugin and its test files are removed because the functionality they tested no longer exists. The former plugin-emission unit tests were ported to native assertions (no coverage loss), and a gold-standard round-trip E2E was added: a fake-LLM tool_call flows OpenClaw → proxy → mock MCP server → audit. Net removals are limited to the plugin's own test files (testing deleted code).

Built on OpenClaw — this keeps Pinchy's MCP integration on OpenClaw's native rails while adding the governance layer (no secret at rest, per-agent permissions, audit) that the runtime doesn't provide on its own.

Clemens Helm added 23 commits July 15, 2026 16:24
Ports the proven MCP client foundation from the pre-port branch
(mcp-client.ts, mcp-error-messages.ts, and the McpTool/McpIntegrationData
types) and wires MCP into main's generic credential mechanisms instead of
the old branch's route-level MCP special cases:

- mask-credentials.ts: mcp -> { configured: true } (pure secret, like
  web-search)
- integration-edit.ts: mcpEditSchema (token rotation only; extraHeaders
  stays on connection.data)
- [connectionId]/route.ts: credentialSchemas.mcp wiring
- probe.ts: probeIntegrationCredentials gains an optional third `data`
  param (needed because mcp credentials alone don't carry url/transport)
  and an mcp branch that calls listMcpTools() directly rather than a
  duplicate mcp-probe.ts handshake client. Only McpAuthError is
  non-transient; McpServerError, McpSchemaError, and network/timeout
  failures are all fail-safe transient so a healthy connection is never
  flipped to auth_failed on a hiccup.
- Both PATCH and Test-Connection routes now pass the connection's `data`
  column through to the probe registry.
…error

A 403 (authenticated, but the token lacks the scope) fell through both
transports' status checks and ended up classified as a network failure:
the HTTP path tripped over the error body as a bogus JSON-RPC error, the
SSE path returned the SDK's raw SseError. Both surfaced as transient, so
we told the user "Couldn't reach the server right now. Try again in a
moment." while the server was perfectly reachable and their token was the
actual problem — and the connection stayed marked healthy, so the retry
never worked.

401 and 403 are one case: fetch a properly scoped token and reconnect.
main's microsoft probe branch already pairs them (401 || 403); MCP now
matches instead of inventing its own classification.

- mcp-client.ts: fetchJsonRpc and translateSdkError (SSE) both map
  401 || 403 -> McpAuthError; McpAuthError's default message is now
  status-agnostic since it covers both.
- probe.ts: the auth reason names permissions too, not just expiry.
- mcp-mock-server.ts: new "forbidden" (403) mode; both auth modes now
  answer any method, since the SSE transport authenticates on a GET.
- Drops a no-op try/catch in fetchJsonRpc whose two branches both did
  `throw err`; the existing abort/timeout tests stay green and prove the
  removal is behaviour-neutral.

Tests: HTTP 403 and SSE 403 -> McpAuthError (the SSE 401 case is covered
too, pinning the SDK's `.code` shape), plus an end-to-end probe test over
the real listMcpTools and a real 403 server. The mocked-client tests
alone couldn't catch this — they assume an McpAuthError the client never
produced for a 403.
Ports the MCP integration surface (T3 of the mcp-port-to-main plan) onto
main's existing generic mechanisms rather than re-inventing them:

- mcp-presets.ts: the 8 Phase-1 presets (github, linear, atlassian, stripe,
  cloudflare, intercom, highlevel, generic) with setup guidance. Generic is
  the load-bearing preset — any MCP server, not just named providers (D4).
- mcp-tool-diff.ts: diffMcpTools() for sync-time added/removed detection.
- feature-flags.ts: PINCHY_MCP_ENABLED (server-side gate, D3). Off ⇒ every
  mcp code path 404s cleanly instead of 500ing.
- POST /api/integrations: mcp branch does live tool discovery
  (listMcpTools) before persisting, rejects unknown presets and HighLevel
  connections missing their locationId header, and audits
  integration.created. Discovery failures are not audited (nothing was
  persisted — mirrors the PATCH route's pre-persist probe-failure
  convention). The preset enum is derived from mcp-presets.ts so the two
  can't drift.
- POST /api/integrations/test-credentials: mcp branch added alongside the
  existing odoo/web-search variants (the plan referenced a standalone
  /api/integrations/test route that doesn't exist on this main — extending
  the existing shared pre-create test route is the actually-generic path,
  in keeping with the port's "replace parallel inventions" principle).
- POST /api/integrations/[connectionId]/sync: mcp branch now uses main's
  setIntegrationAuthFailed/clearIntegrationAuthError state machine, which
  the pre-port branch predated. Only McpAuthError (401/403) flips a
  connection to auth_failed; McpServerError, McpSchemaError, and
  network/timeout failures leave status untouched so a healthy connection
  survives a server hiccup. Tool diff is audited by name (capped at 20
  entries, falling back to a count) to stay under the 2048-byte detail cap.
- audit.ts: extends the integration.synced detail union with an optional
  `tools` field carrying the added/removed diff, mirroring the existing
  MembershipDetail shape.

Deliberately deferred to later tasks: regenerateOpenClawConfig() on
create/sync (build.ts has no mcp awareness yet — T6) and any UI (T8).

42 new tests (create: 9, test-credentials: 8, sync: 9, presets: 7,
tool-diff: 6, feature-flags: 3); no existing tests weakened or removed.
Both ends of the MCP lifecycle logged that *something* was connected but
not *what*: create emitted { type, name, preset, transport, toolCount }
and delete emitted { id, name, type }. Neither carried the server URL.

That gap is widest exactly where the feature matters most. Generic is the
core preset (any MCP server, D4), and for a generic connection the URL is
the server's only identity — preset:"generic", transport:"http",
toolCount:12 answers nothing about which external endpoint this deployment
could suddenly reach. On delete it's worse: AGENTS.md requires delete
details to carry what the removed row can no longer answer, and once the
row is gone the audit log is the only surviving record of the endpoint and
of when that access was withdrawn. A CISO must be able to ask this log
"which external endpoints could our agents reach, since when, and when was
that revoked" — until now it couldn't answer.

- create: add `url` to the audit detail (flat, matching the existing shape).
- delete: for type === "mcp", snapshot { preset, transport, url } from
  connection.data before the row disappears. Other types keep their exact
  previous detail — the generic path is untouched.

The URL is an admin-entered service address: not PII (no scrubEmails /
redactEmail needed) and not a secret (the token lives in the encrypted
credentials blob and stays out). URLs are short, so the 2048-byte cap is
not a concern.

audit.ts is deliberately unchanged: integration.created and
integration.deleted resolve to the arms shared by every AuditResource
(Record<string, unknown> and DeleteDetail's open index signature), so
typing these MCP fields would mean splitting integration.* into its own
union arm — out of scope here. The tests pin the payloads instead.

4 new tests: create carries the URL (incl. the generic-preset case) and
never leaks the token; MCP delete carries the server identity; non-MCP
delete keeps its exact detail shape (toEqual, so an added key fails it).
Suite 7998 -> 8002, no tests weakened or removed.
Port api/internal/mcp-proxy/[connectionId]/route.ts from the pre-port
branch: OpenClaw's native mcp.servers.<id> will point here (T6) instead
of at the third-party MCP server directly, so this route decrypts the
connection's bearer token in memory, injects it as Authorization, and
transparently streams the JSON-RPC request/response — the third-party
token never lands in openclaw.json.

All prior security properties are preserved and pinned by tests:
gateway-token auth, type/status checks, request-time SSRF revalidation,
leak-free decrypt failures, hop-by-hop header stripping, redirect
rejection, abort passthrough, and unbuffered streaming.

New on top of the ported route:
- Gone-Contract 404: an unknown connectionId now returns the same
  actionable message as internal/integrations/[connectionId]/credentials
  ("no longer connected ... reconnect under Settings -> Integrations")
  instead of an opaque "Connection not found", since plugins surface
  body.error straight into the agent's tool error. The type-mismatch 404
  stays a separate, terser message on purpose (config/programming error,
  not a user problem).
- PINCHY_MCP_ENABLED kill switch, checked before the DB lookup so a
  disabled connectionId's existence isn't observable and no query is
  wasted, consistent with the create/sync routes.
- extraHeaders can no longer clobber Authorization/Content-Type/Accept:
  the route now reuses mcp-client.ts's sanitiseExtraHeaders/
  RESERVED_HEADERS (now exported) instead of applying extraHeaders after
  Authorization unfiltered, which previously let a connection's stored
  extraHeaders silently override the injected token at proxy time even
  though the same headers were already stripped during "Test Connection"
  discovery. mcpCreateSchema also now rejects reserved header names at
  create time (400) instead of silently dropping them later.
- Hardened the abort->499 mapping to check err.name directly instead of
  gating on `instanceof Error` first, since DOMException does not
  reliably satisfy `instanceof Error` across realms (observed directly
  in this route's own jsdom-based test suite).

Unit suite 8002 -> 8021 passing, 13 pre-existing skips unchanged, no
tests weakened or removed.
MCP grants are plain rows in agent_connection_permissions
(model="mcp", operation=<toolName>) per D1 — no dedicated table. Add
tests proving the existing generic PUT/GET/DELETE path already carries
that shape correctly (replace, audit diff, idempotency), plus two new
MCP-specific guards: a 128-char tool-name cap in the schema (mirrors the
MCP tool-naming SEP's own length guidance) and a route-level check that
a granted tool actually exists in the connection's synced data.tools.
The latter is write-time UX hardening only — it does not solve stale
grants after a re-sync removes a tool; T6/T7 must still intersect
against current data.tools at read time.
Ports native-mcp.ts (mcpServerKey/nativeMcpToolName/mcpProxyUrl/buildNativeMcp)
and wires build.ts to emit config.mcp.servers + per-agent tools.allow from
active MCP connections and their agent_connection_permissions grants
(model="mcp", D1) — pointed at the Pinchy credential-injecting proxy, gated by
PINCHY_MCP_ENABLED, and drift-filtered against each connection's currently
synced data.tools so a re-sync that drops a tool can't leave it silently
allowed. MCP tool names are appended after computeAllowedTools()'s fail-closed
base, never replacing it; only the gateway bootstrap token is emitted into
mcp.servers.*.headers, never a third-party credential.

Also adds the config-regeneration triggers MCP's config-baked gating needs
that Odoo/email don't: connection sync (drift can revoke a tool), agent
permission grant/revoke, connection delete, and connection credential-edit
(auth_failed recovery) — each scoped to MCP only so existing Odoo/email/imap
behavior is unchanged. Connection *create* deliberately does NOT regenerate:
a fresh connection has zero grants, so build.ts's own "no server entry
without a grant" rule makes that regenerate a guaranteed no-op.
…T6 review)

Finding 1: the status==="active" filter build.ts applies to mcp.servers /
tools.allow only worked in the recovery direction. The sync route's
McpAuthError branch flipped a connection to auth_failed and returned without
regenerating, so an expired token left a live server entry (and its
tools.allow names) in openclaw.json until something unrelated regenerated —
exactly the dishonest config the filter exists to prevent.

The same gap existed in api/integrations/[connectionId]/test (the "Test
Connection" button), which the review didn't cover: it is generic over
connection type and T2 wired MCP into its probe, so it flips MCP status in
BOTH directions across three call sites, none of which regenerated. Fixed
together — same defect, same reasoning; leaving it would ship a known bug in
a path T6 just built.

The regen is failure-isolated in a shared helper rather than inlined: the test
route's catch-all converts any throw into setIntegrationAuthFailed(reason:
<error>), so an unguarded regen throw would have marked a healthy connection
auth_failed because openclaw.json couldn't be written. Counter-checked by
reverting the guard and watching the new test fail. Non-MCP types are a no-op,
so odoo/email/imap/google/microsoft behavior is unchanged.

Finding 2: extract the internal API base-URL fallback chain
(PINCHY_INTERNAL_URL || http://pinchy:$PORT || 7777) into internalApiBaseUrl().
It was inlined verbatim at 8 sites in build.ts (7 pre-existing + the one T6
added); all 8 were textually identical, so this is a pure mechanical
extraction proven by the existing config tests, plus 4 new tests pinning the
branches.
…e transition

The invariant "MCP status changed ⇒ openclaw.json is stale" now lives at the
single place status actually changes, instead of at each of the four callers
that can flip an MCP connection (sync route, Test Connection ×3). All four were
missed on the first pass — one by the implementer, three by the review — and a
fail-closed path must not depend on every future caller remembering.

Being behind the transition guard also makes the trigger precise. Callers
cannot distinguish a real flip from a no-op: both functions return void and
bail out when the conditional UPDATE matches no rows. A caller-side trigger
therefore regenerated on no-op calls too (repeated "Test Connection" on an
already-failed connection); inside auth-state it fires only on real
transitions. Pinned by new no-op tests, which is the concrete win over the
per-caller form.

The type gate stays (existing.type is already loaded there), so
odoo/imap/google/microsoft/web-search behavior is byte-identical — pinned by an
it.each over all five. Failure isolation moves along with the trigger and keeps
its rationale: the status change has committed, and the Test Connection route's
catch-all would otherwise turn a failed config write into a bogus auth_failed
on a healthy connection.

The data-driven regens stay in their routes untouched (sync success path,
credential PATCH, connection DELETE, permissions PUT/DELETE) — those follow
from changed data, not from status. lib/integrations/mcp-config-regen.ts is
deleted; the route-level assertions it carried are re-pinned in
auth-state.test.ts, where the behavior now lives.
…al PATCH

The call had exactly two outcomes, both useless. On recovery
(auth_failed → active) clearIntegrationAuthError already regenerates at the
status transition, so this was a duplicate. Otherwise it produced a
byte-identical config: mcpEditSchema is token-only, and the token never enters
openclaw.json — the proxy fetches it per request. There is no third case;
name/description live outside this block and never appear in mcp.servers.

Its remaining justification was that regenerate is idempotent, which explains
why the call is harmless, not why it is needed.

The DELETE regen stays: deleting a connection changes real data (the row is
gone, so its mcp.servers entry must go), which is not a status transition.
Sync success path and permissions route likewise unchanged.

The existing MCP PATCH test asserted the regen fired; corrected to assert it
does not, rather than dropped, so the call cannot quietly return.
Adds lib/skills/mcp-skill.ts: builds a SKILL.md body per (agent, MCP
connection) pair from the connection's synced tools and the agent's
drift-filtered grants (T6's mcpAgentToolsByConn). Frontmatter name/
description are forced single-line since the admin-entered connection
name is untrusted input to a parser that is explicitly single-line-only;
tool descriptions from the third-party server are truncated and
collapsed to one line so they can't smuggle a fake markdown heading; the
whole body is hard-capped at 8KB with an honest truncation notice when a
server exposes more tools than fit.

Wires this into build.ts: for every (agent, connection) pair with
surviving grants, materializes the skill file and appends its id to
that agent's emitted skills allowlist — appended AFTER the existing
KNOWN_SKILLS validation of the DB's agent.skills column, since these
dynamic ids are derived from connections (D2) and are never persisted
to the DB nor part of KNOWN_SKILLS.

Also cleans up stale mcp-* skill directories on every regenerate
(scoped strictly to that namespace) so a deleted connection or a
disabled feature flag doesn't leave workspace litter behind, backed by
a new lib/workspace.ts helper (listWorkspaceSkillIds) and the existing
removeWorkspaceSkill. Verified against OpenClaw's own skill loader
(dist/workspace-CD16JXyF.js) that agents.list[].skills is already an
authoritative allowlist for workspace-sourced skills too, not only
bundled ones — this cleanup is defense-in-depth, not the sole
correctness mechanism.
Ports the /settings/integrations/new type-picker page and wires the MCP
connect flow into AddIntegrationDialog: preset cards (GitHub, Linear,
Atlassian, Stripe, Cloudflare, Intercom, HighLevel) plus a Custom MCP
server flow, each with brand icons, structured setup guidance, a
Test-Connection step with a collapsed tool-list preview, and
human-friendly errors via mcp-error-messages.ts. EditCredentialsDialog
and SettingsIntegrations gain matching MCP branches (token rotation,
brand-icon card, Re-sync tools). The whole MCP surface is gated behind
NEXT_PUBLIC_PINCHY_MCP_ENABLED, forwarded from PINCHY_MCP_ENABLED in
next.config.ts so operators only set one env var.

Lifts the create and test-credentials request schemas into
lib/schemas/mcp-integration.ts (shared by the routes and the dialog,
per AGENTS.md "Shared Schemas And Typed Client") and extracts
RESERVED_HEADERS into a new client-safe mcp-shared.ts so the schema
can be imported from a "use client" component without pulling the
MCP SDK into the browser bundle.
… inline

NEXT_PUBLIC_PINCHY_MCP_ENABLED was inlined into the client bundle at image
build time, while isMcpEnabled() read PINCHY_MCP_ENABLED live per request.
Since Pinchy ships as a prebuilt image, an operator setting the flag at
runtime armed the API while the UI stayed dark forever — a silent half-state,
and the next.config.ts comment claimed the opposite.

There is now exactly ONE flag: PINCHY_MCP_ENABLED, read server-side per
request. The server components that own the two routes rendering MCP UI
(settings/integrations/new and settings) resolve it and pass it down as a
plain `mcpEnabled` prop — the idiom app/(app)/usage/page.tsx already uses for
the equally server-side, equally runtime enterprise license flag.

Removes isMcpEnabledClient() and the next.config.ts env entry: one env var
less, one config hook less, one function less.

The prop is required on the components that gate MCP UI (AddIntegrationDialog,
IntegrationTypePicker, NewIntegrationContent) so every render site must decide
— a silent default there is the same bug class in a new place. The two
pass-through containers (SettingsPageContent, SettingsIntegrations) default
fail-closed.

Tests: the "flag off → no MCP in the UI" cases now drive the prop instead of
stubbing env. New server-component tests pin that both pages read the flag per
request and propagate it, including a flip-between-renders case that a
build-time flag could never pass.
Adds McpPermissionSection, following the Odoo/Email composition pattern
in agent-settings-permissions.tsx but generalized to N simultaneous MCP
connections (each with its own searchable, grouped tool-checkbox list via
the ported mcp-tool-groups.ts). A connection's permission entry carries
an explicit empty `permissions` array when all its tools are unchecked,
since PUT /api/agents/[agentId]/integrations replaces grants per
connectionId — without that, revoking every tool on one MCP connection
while others stay untouched would silently leave the old grant in place.

The section is gated by a `mcpEnabled` prop threaded from
isMcpEnabled() in the chat/[agentId]/settings server component down
through AgentSettingsPageContent, never read from NEXT_PUBLIC_*/
process.env in the client tree. MCP connections are additionally
filtered to status "active" (not merely "not pending"), since build.ts
only serves active connections in tools.allow/mcp.servers — unlike
Odoo/Email whose gating is a runtime plugin check.

Uses apiGet + toast.error for the load path instead of the raw fetch
Odoo/Email currently use; the actual save still flows through the
existing generic PUT/DELETE mechanism in agent-settings-page-content.tsx
unchanged, since it already treats {connectionId, permissions} shapes
generically.
…(T9)

Port the GitHub PR Reviewer and Linear Triage MCP templates onto main's
generic template/permission machinery: requiresMcpConnection (a preset
string, not a boolean — MCP has 8 presets vs. Odoo/email's one each) gates
template availability per-preset in api/templates/route.ts, and the entire
MCP surface disappears from the picker when PINCHY_MCP_ENABLED is off (D3).
On agent creation, recommendedTools auto-grants matching tools from the
first active connection of each preset as agent_connection_permissions rows
with model:"mcp", reusing the create route's existing unconditional
regenerateOpenClawConfig() call — no extra regen needed. Missing tools are
silently skipped (never blocks creation) and logged in the audit detail.
notion-knowledge-keeper is deliberately not ported: there's no connectable
"notion" preset, so RecommendedTool.preset is now typed from mcp-presets.ts
instead of a hand-rolled union that could reference it.
…user

Two honesty gaps in the T9 MCP templates, both about telling the user what
an agent actually got.

The template card rendered no access badge for MCP templates: it silently
hid that creating the agent auto-grants tools in an external system, while
the Odoo card beside it advertised "Odoo · Read & Write". The badge is the
permission-transparency surface, so a blank one on the only template that
reaches a third-party system undercuts exactly the promise it should keep.
MCP cards now show "<Provider> · <n> tools" (provider name from the preset
registry, never hardcoded) and the permission preview names the concrete
tools that get granted. Amber, not green: MCP tools carry no read/write
classification we can derive, and their real reach depends on the admin's
token scope, so green would claim a harmlessness we haven't verified. For
the same reason the preview states no "Cannot ..." limit — every other
branch's cross item is a limit we enforce; here it would be a promise we
can't keep.

The skip list from the recommendedTools auto-grant only reached the audit
log. Not failing on a renamed tool is right; not informing the person who
just created the agent is not — they would find out mid-conversation. POST
/api/agents now returns skippedMcpTools and the create flow raises a
non-blocking toast (completed action, not a form error). The copy names no
cause: a skip means either no active connection for the preset or the tool
was absent from the synced list, and the server reports names only, so
guessing would send the user to fix the wrong thing.

Noted in code: AccessBadgeProps.variant is currently never rendered
(template-selector.tsx uses label only), so amber doubling as both Odoo's
"read-write" marker and MCP's "acts externally" is invisible today. Flagged
rather than resolved, since wiring variant to a colour is a product call.
…T10)

Proves the MCP integration feature (T1-T9) end-to-end for the first time:
a mock MCP server, an isolated docker-compose overlay, and a Playwright
suite that dispatches a real fake-LLM tool_call through OpenClaw's native
mcp.servers, Pinchy's credential proxy, and back — asserting per-tool
gating, an audit trail, immediate token-rotation pickup with no OpenClaw
restart, and (live, not just read from OpenClaw's source) that the
dynamically generated per-connection skill reaches the model's prompt.

Ran the full 3-file compose stack locally: 10/10 tests green.
…tecture

Ports and rewrites the MCP integration docs (T11) against main's generic
agent_connection_permissions model, the dynamic per-connection skill
generator, and the credential-injecting proxy — not the pre-port branch,
which described a since-abandoned dedicated permissions table and a
different feature-flag pair. Adds Connect an MCP Server / Connect GitHub
via MCP (how-to), MCP Integrations (reference), and The MCP Security
Boundary (explanation) as the security write-up; cross-links them from
Integrations, Agent Permissions, and Skills & Templates. Also documents
in AGENTS.md that native, non-plugin integrations like MCP owe the same
coverage as plugins but aren't caught by the plugin drift guards.
Both MCP templates told the model to call provider-prefixed tool names —
github_pull_request_read, linear_list_issues, and four more — that exist
under no schema. The raw names the server advertises (and that
recommendedTools grants, by matching connection.data.tools) carry no
prefix, and what the model actually sees is OpenClaw's materialized
<serverKey>__<rawName> from nativeMcpToolName. So the prompt pointed at a
name that appears nowhere in the tool list: at best ignored, at worst a
hallucinated call — in templates that promise "works immediately".

The prompts now use the raw names. The serverKey prefix is deliberately not
mentioned: it's derived per-connection at runtime and is unknowable when the
template is authored, and the dynamic per-connection skill already lists the
fully materialized names. All domain substance (the method parameter,
owner/repo/pullNumber, the review and triage workflows) is unchanged.

The real find is why the guard stayed green: it asserted
defaultAgentsMd.includes(rt.tool), and "github_pull_request_read" contains
"pull_request_read" — a substring match that accepts the very corruption it
should catch. It also only checked one direction (every granted tool is
mentioned); the direction that protects the user — the prompt names no tool
we don't grant — did not exist. The guard is now bidirectional and exact:
direction 1 requires a backtick-delimited exact mention, direction 2
extracts backticked bare snake_case identifiers and requires each to be in
recommendedTools. Verified red against the old prompts before the fix, and
re-verified by mutation: prefixing a single one of four mentions passes
direction 1 and is caught only by direction 2.

The preset registry's toolPrefix field encoded the false assumption and now
has no production consumer at all — left in place, reported separately.
The MCP preset/connect form and the token-rotation form both carry an MCP
bearer token, and both defaulted to a GET submit. If the user hits Enter
before hydration completes, the browser serialises the form into the query
string — putting the token in the URL bar, browser history, the server's
access log, and any Referer header that follows. Same leak the rest of the
credential forms already guard against.

Caught by main's credential-forms-post-method drift guard (PR #743) when
this branch was rebased onto current main — the guard postdates the MCP
work, so the forms were written before the rule existed.
Three of the eight commands documented as runnable "from the repository
root" don't exist there: `pnpm lint`, `pnpm format`, and `pnpm db:generate`
have no root script, and pnpm's implicit recursive fallback dies on the
first plugin package, which has no such script either:

    ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL  Command "lint" not found

AGENTS.md is the canonical instruction file for coding agents, so a command
that fails is worse than no command at all — every agent that follows it
burns a cycle rediscovering the same thing. Documents what CI actually runs
(`pnpm --filter @pinchy/web lint` / `format:check`, and the separate docs +
workflow-YAML prettier check from docs-format.yml) and adds the new
`test:e2e:mcp` to the web command list.
`toolPrefix` (github_/linear_/…) had zero production consumers — its only
references were its own two shape tests. Worse than dead: it encodes a wrong
assumption about how OpenClaw names MCP tools. Tools materialize as
`<serverKey>__<rawName>` where serverKey is per-CONNECTION (mcpServerKey),
not a per-preset string; collision safety lives entirely in serverKey
uniqueness (buildNativeMcp throws on a dupe). Two connections of the same
preset even share a prefix and nothing collides, so the "unique prefix"
test guarded a hazard that cannot occur.

This exact assumption produced the T11 template bug (prompts naming
`github_pull_request_read` while the granted/materialized name is
`pull_request_read` / `m…__pull_request_read`). Leaving the field in the
registry invites the next reader to trust it and rebuild that bug.

Removes the field, its 8 values, and the two tests that only asserted its
shape (net -2 test cases). The check-test-deletions guard counts net across
the PR and does not fire (the branch is strongly net-positive on tests), so
this removal is authorized out of band via the `allow-test-deletion` label
on PR #298 per AGENTS.md "No Untracked Test Removal".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

allow-test-deletion PR deliberately removes tests; overrides the test-removal guard

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants