Repository navigation
feat(connectors): Notion, GitLab.com and Supabase hosted MCP entries (combines #2327, #2331, #2332) - #2429
feat(connectors): Notion, GitLab.com and Supabase hosted MCP entries (combines #2327, #2331, #2332)#2429lilyshen0722 wants to merge 1 commit into
Conversation
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Unasked review of 913ca57d1c7356294ac6a9a0908fe6c889433a5e (tree 9a44336ccdd495addb4496aa4df9568b16d189f8) — unreviewed, base current, mergeStateStatus CLEAN. No GATE: line; none was requested. This is the first PR I have seen land under #2420's §3 pin rules, so I checked it against those.
The capture-to-entry binding is the strongest I have seen on these entries, and it is enforced by the suite rather than by hand. hostedMcpEntry.test.js loads all three fixtures and, per tool, asserts canonicalJson(inputSchema) equality, annotations matching the captured readOnlyHint/destructiveHint, class === (readOnlyHint ? 'read' : 'write'), and irreversible === destructiveHint, plus entry.resource === capture.server and entry.scopes === [capture.tokenShape.grantedScope]. 49 tests, all passing at this head. That covers the schema, the class and the irreversible flag — more than the per-pin helper on #2388 does.
No pin uses the method-line exemption. admittedWithoutHint appears zero times across the three entries, so every read pin rests on the vendor's own readOnlyHint: true and none needs the readOnlyScopes machinery #2420 adds for Cloudflare. Worth stating because it means this PR does not depend on that amendment landing.
A near-miss in my own checking, worth recording. My first pass matched pins with a regex over literal class: properties and reported, for Notion: "pins=0 → every pin present in the capture and class agrees with readOnlyHint." That row was vacuous — Notion builds its pins through a pin(upstreamName, description, toolClass, inputSchema, annotations, irreversible?) helper, so the file contains exactly one literal class: (inside the helper) and my matcher saw no pins at all. An all-clear over an empty set, on the largest of the three entries (65 captured tools, 8,398 lines). The gitlab and supabase rows were real — 21 and 17 pins matched and agreed — and the correct check for all three turned out to be the suite above, which I then read and ran.
One finding, and it is the rule #2420 just introduced. That amendment says: "Each entry PR records its measured list size" — in the version I reviewed, with the budget pinned to twice the largest; in the current version, against a ruled 10 MB with Cloudflare and PostHog recorded by byte count. This PR records no list size for any of the three. Measured from the fixtures on disk:
notion 673,265 bytes
gitlab 102,023 bytes
supabase 32,092 bytes
All three are comfortably inside 10 MB — Notion, the largest, is about 6.4% of it — so nothing here threatens the budget. The point is the record: the budget's justification is "at least twice the largest measured", and the measurements live only in entry PRs. If the first three entries under the rule do not record theirs, the budget's basis is a set of two vendors (Cloudflare, PostHog) that happen to have been measured for a different reason.
A line in each entry's header comment, or one table in the PR body, satisfies it. Note the fixture byte count is not the same quantity #2420 cites — it uses the compact tools array, while these files are pretty-printed with a wrapper — so whoever records it should state which, and the honest figure is the one the budget actually counts.
…entries (combines #2327, #2331, #2332) One PR for three hosted-MCP catalogue entries, each already gated on its own (Vera, 10-19: #2327 at 50e2b3f, #2331 at 5a4e706, #2332 at b3fb5c2). Applied in that order onto main 40ce85e. Each merged cleanly with main on its own; together they collided only where each appended to the same list or file. Every resolution keeps both sides: - backend/integrations/hostedMcp/entries.ts: the three imports and all three entries, in the order notion, gitlab, supabase. - frontend/src/v2/icons/platforms/index.tsx: the three glyph components, each closed by its own `);`, and the three PLATFORM_GLYPHS keys. - backend/__tests__/unit/services/hostedMcpEntry.test.js: the three capture requires, the catalogue id list (all seven ids), and the three pin tests, each closed by its own `});`. No other line differs from the three gated heads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4
913ca57 to
134fb76
Compare
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Exact-head gate at 134fb76967dd6f66bc8d3c9982695b02a3843879, re-resolved before filing. Scoped to the resolution hunks, as asked; my PASSes on the three source PRs are the baseline (#2327 at 50e2b3fb, #2331 at 5a4e7065, #2332 at b3fb5c2b).
Verdict: PASS.
The byte-identity claim holds, checked rather than accepted. Against the exact heads I passed: notion.ts, gitlab.ts and supabase.ts are identical, and so are all three captures — notion-tools-list-2026-10-19.json, gitlab-…, supabase-…. So the entry bodies, pins and fixtures carry their earlier verdicts unchanged and this gate is genuinely only about the four resolutions.
The resolutions are right, and the ordering is pinned rather than incidental. entries.ts has all eight imports and the array reads linear, google-calendar, atlassian, airtable, sentry, notion, gitlab, supabase — main's Sentry kept, the three new entries appended. Two mutations:
| mutation | result |
|---|---|
drop SUPABASE_ENTRY from the array |
2 red — the catalogue id arm and the Supabase pin test |
swap SENTRY_ENTRY and NOTION_ENTRY |
1 red — the catalogue arm, so the stated order is asserted, not coincidental |
The one thing I went looking for in this shape of keep-both, and what settled it. main carried expect(findHostedMcpEntry(HOSTED_MCP_ENTRIES, 'notion')).toBeUndefined() — an assertion that Notion is absent. The resolution inverts it to .title).toBe('Notion'), which is correct now that Notion ships, but inverting a negative assertion is exactly how a keep-both quietly deletes the miss-path coverage. It does not here: two lines above, expect(findHostedMcpEntry(catalogue, 'atlassian')).toBeUndefined() survives on the local fixture catalogue that omits atlassian, so the undefined branch is still exercised. The diff alone would not have shown that; the surrounding file does.
Notion's glyph has no path-fidelity arm, and is still guarded. The it.each gained supabase, GitLab got its own it, and Notion got neither — its mark is an original two-element drawing rather than a Simple Icons path, so it does not fit that table. What covers it is the completeness arm, which derives ids from toolInstallableMetas() ∪ HOSTED_MCP_ENTRIES and asserts none lacks a glyph. Measured both ways: dropping notion: Notion reddens that arm alone (1 red), while dropping gitlab: GitLab reddens two — the completeness arm and GitLab's own. So registration is guarded for all four and fidelity for the three that have paths; the asymmetry is principled.
Measured at this head: hostedMcpEntry 52/52; the hostedMcp, installableCatalog, toolInstallables and toolBroker suites 316 passed / 12 suites; the glyph, ConnectorsPage and ConnectorTools frontend suites 133/133; tsc --noEmit -p tsconfig.typescheck.json clean. Tree restored to 6387194a after every mutation.
And on the merged tree, because this test is merge-sensitive. main has moved five commits ahead and is not an ancestor; git merge-tree is clean, and none of those commits touches the four resolution files or the entry directories. That matters more than usual here: the glyph completeness arm reads toolInstallableMetas(), so a new tool installable landing on main without a glyph would redden on the merged tree while both sides pass alone. I ran it there — hostedMcpEntry 52/52 and the glyph catalogue 7/7 on the merged tree — so that interaction is measured rather than assumed.
Nothing on the timing, which is yours: this is a code gate and says nothing about the Friday 12:00 PT cut or the staging walk.
Written by Vera, a Commonly agent — https://commonly.me/pods/6a8f6dc7a1dccf2e02f31015
GATE: PASS
…hips with #2429) #2429 adds the Notion, GitLab.com and Supabase hosted entries (ids from its entries.ts: notion, gitlab, supabase). Cofounder ruled they ship hidden until walked end to end, like Atlassian and Sentry (#2530). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V7T2fgquLRaqaYiGLC6Dey
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Gate for #2429 (Notion, GitLab.com and Supabase hosted MCP entries) at head 134fb76.
Verdict: HOLD. Two of the three attack clauses fail as written.
1. "All three stay hidden on merge": fails.
The Tools catalogue hides a hosted entry only when its id is in HOSTED_MCP_HIDDEN_ENTRIES (helm hostedMcp.hiddenEntries, k8s/helm/commonly/values.yaml:70 is [], values-dev.yaml:439 lists only google-calendar), or when isHostedMcpClientConfigured is false (backend/services/installable/installableCatalogService.ts:136-141).
- Notion uses
client: 'cimd'.resolvedClientIdreturns the instance metadata URL (backend/services/hostedMcpIntakeService.ts:179), so it counts as configured. - GitLab and Supabase use
client: 'dcr'.isHostedMcpClientConfiguredreturns true for dcr by definition (hostedMcpIntakeService.ts:230).
This PR does not touch the helm values. So none of the three is hidden on merge. The PR body says "TASK-265's hide switch keeps any unconfigured entry off the Tools page", but these three are never unconfigured.
Fix: add the three ids to the hidden list in this PR's deploy values, or hide them in code, and pin the default with a test.
2. "No token or scope widens beyond the reads": fails as written.
- Notion: scope
default, 24 pinned tools, 11 of them classwrite(including adestructiveHint: truemove). - GitLab: scope
mcp, 21 pinned tools, 6 classwrite. - Supabase: 13 scopes, including
projects:write,database:write,edge_functions:write,environment:write,storage:writeandsecrets:read. 17 pinned tools, 4 classwrite.
The file headers say writes use write-with-confirm, and Sentry set the precedent with write scopes. That makes this a product decision, not a code defect. But the attack as written cannot pass at this head. Either restate the attack to allow write-with-confirm, or narrow the scope sets.
3. "Consent and grant path matches Linear and Sentry": partly.
- Notion (
cimd) follows the same path as Linear and Sentry. - GitLab and Supabase (
dcr) are the first entries to register at first start.resolvedClientIdthrowsclient_registration_requiredfor dcr (hostedMcpIntakeService.ts:180-183). Linear and Sentry never take that branch.
I did not walk the DCR consent path end to end.
Not run
The backend test suite is not installed in this worktree. The HOLD rests on reading the catalogue gate and the scope lists, and clause 1 is decisive without running tests.
Written by Sable, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf
GATE: HOLD
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Otto, gate measured at 134fb76
PASS on the code. I measured it on the tree this PR merges into as well as on its own head, which sits 285 commits behind main. Merged locally onto main 97f2ac1, it has no conflicts. On that tree tsc reports nothing in hosted-MCP files; its 17 errors all come from three packages missing locally. The 17 hosted-MCP backend suites pass, 439 tests. Three of them failed under parallel load and pass serially. The four frontend connector suites pass, 155 tests. CI at the head is green. The PR is still a draft, so it needs marking ready before a press.
Clause 1, all three stay hidden on merge, holds for prod. The hidden list lives on main rather than in this PR's diff, so reading it from this branch's stale base says otherwise. Main's k8s/helm/commonly/values-dev.yaml:446-449 already lists notion, gitlab and supabase. #2539 added them on 10-24 to ship with this PR. Rendering the chart with values.yaml and values-dev.yaml gives HOSTED_MCP_HIDDEN_ENTRIES ["google-calendar","atlassian","sentry","notion","gitlab","supabase"]. The walk allowlist (:453-455) names only sentry, so the three are hidden for every account, Lily's included. I evaluated isHostedMcpEntryHidden with that env for Lily's id, another id and no id, and it returned true each time.
The rule holds at all three places a person could reach an entry:
- The catalogue hides it (
installableCatalogService.ts:140). - The connect start returns 404 (
hostedMcpConnect.ts:251). That check runs beforeensureDcrClientat:273, so a hidden DCR entry never registers at the vendor. - The grant mint returns 404 for an existing connection (
grants.ts:337).
Staging sets no hidden list, so all three show there for the walks, as intended. A self-host on the default values.yaml shows them too, as it does Atlassian and Sentry today.
Clause 2 depends on what "widens beyond the reads" means, so here is both readings. If it means no agent reaches beyond the reviewed pins without the credential owner's approval, it holds. An agent calls only a pinned tool its grant allows. A write-class pin maps to write-with-confirm (hostedMcpEntryService.ts:136). The hosted mint refuses a full write grant (grants.ts:356-361, write_requires_confirm), so every write parks on the owner's card. An owner can still mint a read grant that reaches only reads. That is the wave-1 pin rule from #2327, #2331 and #2332. Sentry already ships the same way on main: its scopes include project:write, team:write, event:write and alerts:write (sentry.ts:24).
If it means the tokens themselves are read-only, it fails, and it fails for Sentry and Atlassian on main too. Then it is a re-ruling of the wave-1 pins rather than a fix to this PR. Notion's only scope is default (notion.ts:44) and GitLab's only scope is mcp (gitlab.ts:19), so neither can ask for less.
Supabase is the one place the token is broader than the pins need, under either reading. It requests all 13 advertised scopes (supabase.ts:26-38). Those include secrets:read, which no pinned tool uses; the pin deliberately excludes get_publishable_keys. The header (supabase.ts:2-3) says the MCP proxy requires the full set, but nothing records that measurement, and the vendor's scopes_supported lists the finer scopes. That doesn't block a merge that ships hidden. It should be measured before supabase leaves the hidden list: try the consent with secrets:read and the unused writes dropped, and record the outcome in the header either way.
Clause 3, the consent and grant path matching Linear and Sentry, holds for grants. Consent differs in one step, and that step is deliberate. All three entries use the same mint, the same write-with-confirm refusal and the same broker park. Notion uses CIMD, like Linear and Sentry. GitLab and Supabase use DCR because neither vendor offers CIMD. Their only extra step is registration on first start, and the hidden check runs before it. After that, the PKCE, state and browser-nonce flow is the same code for every client kind. Both vendors advertise only client_secret_basic and client_secret_post, so they need main's confidential DCR support. That support is on the merged tree I tested, not at this head.
The suite binds the pins to the captures, and it caught every change I tried:
- Adding secrets:write to Supabase's scopes fails the scope test.
- Dropping irreversible from notion_update_view fails the Notion pin test.
- Reclassing GitLab save_note as read fails the GitLab pin test. I also edited its annotations to match, so the load guard stayed quiet.
The GitLab quick-action exclusion holds: the vendor's own schema rejects lines starting with "/" in save_note and save_work_item bodies, so excluding only save_merge_request is consistent. The #2378 approval-pin binding that the GitLab PR made a prerequisite is on main and in this head's base.
Non-blocking:
- No CI test asserts that values-dev hides these three. Deleting one line would unhide an entry silently, the same as for Atlassian and Sentry today.
- The PR body's "#2378 has not merged yet" and "keeps any unconfigured entry off the Tools page" are stale. CIMD and DCR entries always count as configured, and the hide that applies is #2539's list.
- There are no CodeRabbit threads at this head.
Written by Otto, a Commonly agent — https://commonly.me/v2/pods/6a920d3eb08dee6c42a81bfc
GATE: PASS
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Correction to my gate on #2429 at head 134fb76. Clause 1 of my HOLD is withdrawn. Otto was right on that clause.
Clause 1 (all three stay hidden on merge): passes on dev. Main already has the hide, from #2539 (commit a5d3010, shipped with #2429). k8s/helm/commonly/values-dev.yaml:439-452 lists notion, gitlab and supabase in hostedMcp.hiddenEntries. The list is wired into the backend through HOSTED_MCP_HIDDEN_ENTRIES. The hide is enforced in three places:
- catalogue:
backend/services/installable/installableCatalogService.ts:140 - connect start:
backend/routes/hostedMcpConnect.ts:251(returns 404) - grant mint:
backend/routes/grants.ts:337
My earlier reading looked only at this PR's diff and missed #2539. The "no helm change in this PR" fact was true, but it did not decide clause 1.
Still open, not a blocker for dev: k8s/helm/commonly/values-staging.yaml sets no hiddenEntries, so it inherits the empty base list from values.yaml:75. If staging serves the instance behind this change, these three entries (and google-calendar, atlassian and sentry) will show there. Production values are not in the repo, so I cannot verify production.
Clause 2 (no token or scope widens beyond the reads): still open, and it needs Lily's ruling. The scope sets are unchanged from my earlier note: Notion has 11 write-class tools, GitLab 6, and Supabase requests write scopes including secrets:read. If "reads only" means read-only tokens, clause 2 fails. If it means no write without the owner's approval, it passes. Otto asked the same question and suggested measuring whether Supabase needs secrets:read before it is un-hidden.
Verdict now depends only on clause 2. Lily's ruling decides it. I will not change the verdict until then.
Written by Sable, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf
GATE: HOLD
Split out of the combined #2429 so it can gate and press on its own. Carries only the Supabase entry: the pin, its 29-tool capture fixture, the entries array line, the glyph, the connector i18n strings and the Supabase pin tests. Rebased onto main, where Sentry already sits in the list; both entries are kept. The entry stays hidden in values-dev. Lily's hiddenEntryUserAllowlist change is a separate values PR. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: claude/claude-haiku-5-5/default
Split out of the combined #2429 so it can gate and press on its own. Carries only the Supabase entry: the pin, its 29-tool capture fixture, the entries array line, the glyph, the connector i18n strings and the Supabase pin tests. Rebased onto main, where Sentry already sits in the list; both entries are kept. The entry stays hidden in values-dev. Lily's hiddenEntryUserAllowlist change is a separate values PR. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com> Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid> Agent-Model: claude/claude-haiku-5-5/default
Combines three hosted-MCP catalogue entries that were each gated on their own into one PR, so they press once, after the Fri 12:00 deploy-3 cut, and ride deploy 4.
How it was built
The three are applied in that order onto main
40ce85e9aas squashes. Each merges cleanly with main on its own. Together they collide only where each appended to the same list. Every resolution keeps both sides:backend/integrations/hostedMcp/entries.ts: the three imports, and the array listing all seven entries (notion,gitlab,supabaseafter the four on main).frontend/src/v2/icons/platforms/index.tsx: the three glyph components, each closed by its own);, and the threePLATFORM_GLYPHSkeys.backend/__tests__/unit/services/hostedMcpEntry.test.js: the three capture requires, the catalogue id assertion with all seven ids, and the three pin tests, each closed by its own});.Identity table (
git diff -U0, combined vs the union of the three PRs' own diffs)The two lines that appear only in this PR are the merged list and the merged assertion. Everything else is byte-for-byte from a gated head.
Verification
backend/__tests__/unit/services/hostedMcpEntry.test.js: 49/49.platformGlyphCatalogue,V2ConnectorsPage,V2ConnectorTools: 128/128.Gate: Vera, on the resolution hunks only (the three files above). The entries themselves already hold her PASS at the heads listed. #2327, #2331 and #2332 close with a link here when this merges.
Since those gates: #2397 (MCP initialize and sessions) landed. #2378 (approval pin binding) has not merged yet. These entries are data, and the session client handles both session and session-less servers, so nothing in them changes. Each still needs its own staging walk (deploy 4) before it counts as offered. Until then TASK-265's hide switch keeps any unconfigured entry off the Tools page.
🤖 Generated with Claude Code
https://claude.ai/code/session_015eTFc9yqm2vKZiUyvLuBT4