feat(mcp): add connections get tool - #7066
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 16 files
Confidence score: 4/5
- In
packages/server/lib/controllers/mcp/connections/list.ts, very largepageinputs can produce unbounded SQL offsets, which risks expensive table scans or offset errors under user-supplied pagination—cappagesimilarly to the v1 connections endpoint to bound query cost and avoid invalid offsets. - In
packages/server/lib/controllers/mcp/management.integration.test.ts, the expected error substring does not match the actualformatArgumentsErroroutput, so the test can fail for the wrong reason and stop validating the real tool-error contract—update the assertion text (or standardize the emitted message) so the test checks the intended behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server/lib/controllers/mcp/connections/list.ts">
<violation number="1" location="packages/server/lib/controllers/mcp/connections/list.ts:22">
P2: Large valid `page` values turn into unbounded SQL offsets; cap page like the v1 connections endpoint to prevent costly scans and invalid offset errors.
(Based on your team's feedback about avoiding unnecessary heavy reads.)</violation>
</file>
<file name="packages/server/lib/controllers/mcp/management.integration.test.ts">
<violation number="1" location="packages/server/lib/controllers/mcp/management.integration.test.ts:368">
P2: This assertion checks for the substring 'Invalid arguments for tool connections_get', but the tool error path produces 'Invalid connections_get arguments: ...'. formatArgumentsError (managementTool.ts) builds `Invalid ${toolName} arguments: ${details}`, confirmed by managementTool.unit.test.ts expecting 'Invalid test_tool arguments:' and connections/get.unit.test.ts expecting 'Invalid connections_get arguments:'. The checked substring never appears, so this test fails.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| end_user_organization_id: z.string().min(1).max(255).optional(), | ||
| tags: connectionTagsSchema.optional(), | ||
| limit: z.number().int().min(1).max(10_000).optional(), | ||
| page: z.number().int().min(0).optional() |
There was a problem hiding this comment.
P2: Large valid page values turn into unbounded SQL offsets; cap page like the v1 connections endpoint to prevent costly scans and invalid offset errors.
(Based on your team's feedback about avoiding unnecessary heavy reads.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server/lib/controllers/mcp/connections/list.ts, line 22:
<comment>Large valid `page` values turn into unbounded SQL offsets; cap page like the v1 connections endpoint to prevent costly scans and invalid offset errors.
(Based on your team's feedback about avoiding unnecessary heavy reads.) </comment>
<file context>
@@ -0,0 +1,51 @@
+ end_user_organization_id: z.string().min(1).max(255).optional(),
+ tags: connectionTagsSchema.optional(),
+ limit: z.number().int().min(1).max(10_000).optional(),
+ page: z.number().int().min(0).optional()
+ })
+ .strict();
</file context>
| } | ||
| }); | ||
| expect(invalid.json.result).toMatchObject({ isError: true }); | ||
| expect(invalid.json.result.content[0].text).toContain('Invalid arguments for tool connections_get'); |
There was a problem hiding this comment.
P2: This assertion checks for the substring 'Invalid arguments for tool connections_get', but the tool error path produces 'Invalid connections_get arguments: ...'. formatArgumentsError (managementTool.ts) builds Invalid ${toolName} arguments: ${details}, confirmed by managementTool.unit.test.ts expecting 'Invalid test_tool arguments:' and connections/get.unit.test.ts expecting 'Invalid connections_get arguments:'. The checked substring never appears, so this test fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server/lib/controllers/mcp/management.integration.test.ts, line 368:
<comment>This assertion checks for the substring 'Invalid arguments for tool connections_get', but the tool error path produces 'Invalid connections_get arguments: ...'. formatArgumentsError (managementTool.ts) builds `Invalid ${toolName} arguments: ${details}`, confirmed by managementTool.unit.test.ts expecting 'Invalid test_tool arguments:' and connections/get.unit.test.ts expecting 'Invalid connections_get arguments:'. The checked substring never appears, so this test fails.</comment>
<file context>
@@ -256,6 +267,174 @@ describe('POST /mcp management server', () => {
+ }
+ });
+ expect(invalid.json.result).toMatchObject({ isError: true });
+ expect(invalid.json.result.content[0].text).toContain('Invalid arguments for tool connections_get');
+
+ const missing = await mcpPost({
</file context>
marcindobry
changed the title
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
a3b71f4 to
02d79b3
Compare
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 4/5
- In
packages/persist/lib/server.integration.test.ts, removingclearDb()can leave seeded integration data behind in a long-lived shared DB, which risks cross-test contamination and intermittent failures as later suites read unexpected rows — reintroduce cleanup or isolate each run with a fresh/schema-scoped test database.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/persist/lib/server.integration.test.ts">
<violation number="1" location="packages/persist/lib/server.integration.test.ts:74">
P3: Removing `clearDb()` leaves all seeded rows (account, env, plan, provider config, connection, sync, sync_job, and records) in the shared integration DB after the suite. When the integration DB is long-lived rather than recreated per run (e.g. local runs against a persistent DATABASE_URL), this data accumulates on every run since each `seeders.createAccount()` creates a fresh uuid account; consider deleting just the created rows in afterAll instead of dropping the whole schema.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| afterAll(async () => { | ||
| await clearDb(); | ||
| vi.restoreAllMocks(); |
There was a problem hiding this comment.
P3: Removing clearDb() leaves all seeded rows (account, env, plan, provider config, connection, sync, sync_job, and records) in the shared integration DB after the suite. When the integration DB is long-lived rather than recreated per run (e.g. local runs against a persistent DATABASE_URL), this data accumulates on every run since each seeders.createAccount() creates a fresh uuid account; consider deleting just the created rows in afterAll instead of dropping the whole schema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/persist/lib/server.integration.test.ts, line 74:
<comment>Removing `clearDb()` leaves all seeded rows (account, env, plan, provider config, connection, sync, sync_job, and records) in the shared integration DB after the suite. When the integration DB is long-lived rather than recreated per run (e.g. local runs against a persistent DATABASE_URL), this data accumulates on every run since each `seeders.createAccount()` creates a fresh uuid account; consider deleting just the created rows in afterAll instead of dropping the whole schema.</comment>
<file context>
@@ -66,7 +71,10 @@ describe('Persist API', () => {
afterAll(async () => {
- await clearDb();
+ vi.restoreAllMocks();
+ if (httpServer) {
+ await new Promise<void>((resolve) => httpServer?.close(() => resolve()));
</file context>
ade5d6f to
1763785
Compare
There was a problem hiding this comment.
1 issue found across 14 files (changes from recent commits).
Confidence score: 4/5
- In
packages/shared/lib/services/connection.service.unit.test.ts, the test currently enforces collapsing all credential refresh failures intoinvalid_credentials, which can mask distinct error codes fromrefreshOrTestCredentialsand reduce debugging/API handling fidelity; update the expectation to preserve specific upstream error codes (or explicitly map only intended cases).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/shared/lib/services/connection.service.unit.test.ts">
<violation number="1" location="packages/shared/lib/services/connection.service.unit.test.ts:140">
P2: This test codifies the behavior that every credential refresh failure is collapsed to a generic `invalid_credentials` code, discarding the specific error code that `refreshOrTestCredentials` (`connections/credentials/refresh.ts`) produces (e.g. `connection_refresh_exhausted`, `connection_refresh_backoff`, `invalid_crypted_connection`, `refresh_lock_timeout`). The implementation keeps only `status` and `payload` and drops the original code, so callers can no longer distinguish an exhausted/back-off credential or a lock-contention timeout from genuinely invalid credentials. The team expects lock contention to be preserved as `refresh_lock_timeout` rather than mapped to `invalid_credentials`. Add/extend a unit test asserting the specific refresh error code is preserved in the returned `GetConnectionError` (and fix `getConnectionWithCredentials` at connection.service.ts:650 accordingly, since this test locks in the lossy mapping).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| connection | ||
| }); | ||
| if (result.isErr()) { | ||
| expect(result.error).toMatchObject({ code: 'invalid_credentials', status: 424, payload: { reason: 'exhausted' } }); |
There was a problem hiding this comment.
P2: This test codifies the behavior that every credential refresh failure is collapsed to a generic invalid_credentials code, discarding the specific error code that refreshOrTestCredentials (connections/credentials/refresh.ts) produces (e.g. connection_refresh_exhausted, connection_refresh_backoff, invalid_crypted_connection, refresh_lock_timeout). The implementation keeps only status and payload and drops the original code, so callers can no longer distinguish an exhausted/back-off credential or a lock-contention timeout from genuinely invalid credentials. The team expects lock contention to be preserved as refresh_lock_timeout rather than mapped to invalid_credentials. Add/extend a unit test asserting the specific refresh error code is preserved in the returned GetConnectionError (and fix getConnectionWithCredentials at connection.service.ts:650 accordingly, since this test locks in the lossy mapping).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/lib/services/connection.service.unit.test.ts, line 140:
<comment>This test codifies the behavior that every credential refresh failure is collapsed to a generic `invalid_credentials` code, discarding the specific error code that `refreshOrTestCredentials` (`connections/credentials/refresh.ts`) produces (e.g. `connection_refresh_exhausted`, `connection_refresh_backoff`, `invalid_crypted_connection`, `refresh_lock_timeout`). The implementation keeps only `status` and `payload` and drops the original code, so callers can no longer distinguish an exhausted/back-off credential or a lock-contention timeout from genuinely invalid credentials. The team expects lock contention to be preserved as `refresh_lock_timeout` rather than mapped to `invalid_credentials`. Add/extend a unit test asserting the specific refresh error code is preserved in the returned `GetConnectionError` (and fix `getConnectionWithCredentials` at connection.service.ts:650 accordingly, since this test locks in the lossy mapping).</comment>
<file context>
@@ -1,15 +1,163 @@
+ connection
+ });
+ if (result.isErr()) {
+ expect(result.error).toMatchObject({ code: 'invalid_credentials', status: 424, payload: { reason: 'exhausted' } });
+ expect(result.error.connection).not.toHaveProperty('credentials');
+ }
</file context>
There was a problem hiding this comment.
This is how the existing endpoint works
5ad8fc4 to
def6c5d
Compare
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/shared/lib/services/connection.service.ts | Centralizes detailed connection retrieval, credential refresh, refresh-token filtering, and primary-database enrichment. |
| packages/server/lib/controllers/mcp/connections/get.ts | Implements the scoped connections_get MCP handler and prevents credential operations for read-only keys. |
| packages/server/lib/controllers/connection/connectionId/getConnection.ts | Refactors the public endpoint to use the shared connection retrieval service while preserving response and error handling. |
| packages/server/lib/controllers/mcp/connections/formatter.ts | Formats retrieved connections and recursively serializes credential dates for MCP output. |
| packages/server/lib/controllers/mcp/connections/schema.ts | Defines the structured output contract for full MCP connection retrieval. |
Reviews (4): Last reviewed commit: "refactor(api): narrow connection formatt..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| resolvedConnection = connectionResult.response; | ||
| } | ||
|
|
||
| const result = await db.knex |
There was a problem hiding this comment.
This is a part extracted out of listConnections method in this service that is relevant to callers of this method. Old code that can be found in the public API getConnection handler called listConnections, took the first connection, and took out the data that this queried. This is a dedicated query to do that instead.
| }; | ||
|
|
||
| const credentialResponse = await refreshOrTestCredentials({ | ||
| const result = await connectionService.getConnectionWithCredentials({ |
There was a problem hiding this comment.
Personally, I'm not a huge fan of getting the whole connection, decrypting the credentials and dropping them if they are not required or the scopes don't allow it.
| import type { GetConnectionOutput } from './schema.js'; | ||
|
|
||
| const getConnectionArgumentsSchema = z | ||
| .object({ | ||
| connection_id: connectionIdSchema.min(1), | ||
| integration_id: providerConfigKeySchema.min(1), | ||
| refresh_token: z.boolean().optional(), | ||
| force_refresh: z.boolean().optional(), | ||
| refresh_github_app_jwt_token: z.boolean().optional() | ||
| }) | ||
| .strict(); |
There was a problem hiding this comment.
Q: Saw that we are doing this as a convention, (input schema on the tool file and output on the schema file). Any particular reason to it?
|
|
||
| function withoutDirectAndRawRefreshToken(credentials: OAuth2Credentials): OAuth2Credentials; | ||
| function withoutDirectAndRawRefreshToken(credentials: TwoStepCredentials): TwoStepCredentials; | ||
| function withoutDirectAndRawRefreshToken(credentials: OAuth2Credentials | TwoStepCredentials): OAuth2Credentials | TwoStepCredentials { |
There was a problem hiding this comment.
There are provider-specific refresh token shapes that would leak with this. For example, [workday-refresh-token](
nango/packages/providers/providers.yaml
Lines 25673 to 25690 in 8d1d956
credentials.refreshToken. Some TWO_STEP providers also retain it in raw under provider-specific names such as raw.RefreshToken or raw.refreshToken.
2 participants
Adds the
connections_getManagement MCP toolNAN-6306: https://linear.app/nango/issue/NAN-6306/mcp-tool-connections-get