diff --git a/.changeset/artifact-bracket-binding-guards.md b/.changeset/artifact-bracket-binding-guards.md new file mode 100644 index 0000000000..89d6b489d7 --- /dev/null +++ b/.changeset/artifact-bracket-binding-guards.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Recognize single-quoted artifact integration roots and reject hardcoded connection addresses consistently in dot and bracket notation. diff --git a/e2e/scenarios/artifacts.test.ts b/e2e/scenarios/artifacts.test.ts index fa9a82d03b..1771c16672 100644 --- a/e2e/scenarios/artifacts.test.ts +++ b/e2e/scenarios/artifacts.test.ts @@ -214,6 +214,31 @@ scenario( "infiniteQueryOptions", ); + // Bracket notation must follow the same binding contract as dotted + // references: missing integrations are not silently ignored, and a + // literal connection path is rejected before persistence. The slug is + // unique and deliberately unregistered; no upstream service is needed. + const missingIntegration = `bracket-check-${suffix}`; + const beforeRejected = yield* client.artifacts.list(); + const missingBinding = yield* session.call("create-artifact", { + code: `function App(){ useQuery(tools['${missingIntegration}'].query.queryOptions({})); return
; }`, + title: `Missing bracket binding ${suffix}`, + }); + expect(missingBinding.ok, "a single-quoted root still requires a real connection").toBe( + false, + ); + expect(missingBinding.text).toContain(`No ${missingIntegration} connection`); + + const pinnedBinding = yield* session.call("create-artifact", { + code: `function App(){ useQuery(tools['${missingIntegration}']["org"]['shared'].query.queryOptions({})); return
; }`, + title: `Pinned bracket binding ${suffix}`, + }); + expect(pinnedBinding.ok, "bracket syntax cannot pin a connection in saved code").toBe(false); + expect(pinnedBinding.text).toContain("Artifact code must not name a connection"); + expect((yield* client.artifacts.list()).length, "neither rejection saved an artifact").toBe( + beforeRejected.length, + ); + const rendered = yield* session.call("create-artifact", { code: artifactSource(marker), title, @@ -233,6 +258,22 @@ scenario( artifactId = structured.artifactId as ArtifactId; expect(artifactId, "the artifact was persisted and its id returned").toBeTruthy(); + const beforePinnedEdit = yield* client.artifacts.get({ params: { artifactId } }); + const pinnedEdit = yield* session.call("edit-artifact", { + artifactId, + edits: [ + { + oldText: beforePinnedEdit.code, + newText: `function App(){ useQuery(tools["${missingIntegration}"].user['personal'].query.queryOptions({})); return
; }`, + }, + ], + }); + expect(pinnedEdit.ok, "edit-artifact also rejects bracketed pinned connections").toBe(false); + expect(pinnedEdit.text).toContain("Artifact code must not name a connection"); + expect((yield* client.artifacts.get({ params: { artifactId } })).code).toBe( + beforePinnedEdit.code, + ); + const url = String(structured.url); expect(rendered.text, "the model is handed the URL to relay to the user").toContain(url); diff --git a/packages/core/execution/src/skills.ts b/packages/core/execution/src/skills.ts index deeb31cb3b..89e6f283f0 100644 --- a/packages/core/execution/src/skills.ts +++ b/packages/core/execution/src/skills.ts @@ -146,6 +146,13 @@ const CREATE_ARTIFACT_SKILL_BODY = [ "", "## Addressing: Integrations, Not Connections", "", + "Integration slugs containing hyphens use static bracket notation:", + "`tools[\"cloudflare-bindings\"].` or `tools['cloudflare-bindings'].`.", + "Both quote styles work, including named roles such as", + "`tools['cloudflare-bindings'](\"prod\").`. Bracket notation does not permit", + "hardcoding `.user.` or `.org.` into artifact source;", + "supply the connection separately as described below.", + "", "This is the one place artifact code differs from `execute` code, and getting it", "wrong is rejected outright.", "", diff --git a/packages/hosts/mcp/src/artifact-bindings.test.ts b/packages/hosts/mcp/src/artifact-bindings.test.ts index 8cd527c715..2f4844c922 100644 --- a/packages/hosts/mcp/src/artifact-bindings.test.ts +++ b/packages/hosts/mcp/src/artifact-bindings.test.ts @@ -36,6 +36,56 @@ describe("extractArtifactRoles", () => { expect(roles).toEqual([{ role: "cloudflare-bindings", integration: "cloudflare-bindings" }]); }); + it.each([ + `tools['cloudflare-bindings']`, + `tools [ 'cloudflare-bindings' ]`, + `tools [ "cloudflare-bindings" ]`, + ])("reads a static bracket root: %s", (root) => { + expect(extractArtifactRoles(`${root}.query.queryOptions({});`)).toEqual([ + { role: "cloudflare-bindings", integration: "cloudflare-bindings" }, + ]); + }); + + it("reads both role quote flavours after either bracket quote flavour", () => { + expect( + extractArtifactRoles(` + tools['cloudflare-bindings']("prod").query.queryOptions({}); + tools["cloudflare-bindings"]('staging').query.queryOptions({}); + `), + ).toEqual([ + { role: "prod", integration: "cloudflare-bindings" }, + { role: "staging", integration: "cloudflare-bindings" }, + ]); + }); + + it("deduplicates mixed bracket and dotted references", () => { + expect( + extractArtifactRoles( + `tools.linear.issues.list(); tools['linear'].issues.list(); tools["linear"].issues.list();`, + ), + ).toEqual([{ role: "linear", integration: "linear" }]); + }); + + it("ignores bracketed system roots, comments and another object's tools", () => { + expect( + extractArtifactRoles(` + tools['search']({}); tools["describe"].tool({}); tools['executor'].coreTools.connections.list({}); + // tools['ignored-comment'].query({}); + /* tools["ignored-block"].query({}); */ + other.tools['ignored-property'].query({}); + mytools['ignored-name'].query({}); + `), + ).toEqual([]); + }); + + it.each([ + `tools[integration].query({});`, + `tools['cloudflare-' + suffix].query({});`, + "tools[`cloudflare-bindings`].query({});", + ])("does not infer a static binding from computed syntax: %s", (code) => { + expect(extractArtifactRoles(code)).toEqual([]); + }); + it("collapses repeated references to one role", () => { const roles = extractArtifactRoles( `useQuery(tools.linear.issues.list.queryOptions({})); @@ -113,6 +163,42 @@ describe("oldStyleAddressRejection", () => { ).toContain("tools.inventory.org."); }); + it.each([ + `tools['cloudflare-bindings'].user.personal.query`, + `tools["cloudflare-bindings"].org.shared.query`, + `tools['cloudflare-bindings']['user']['personal'].query`, + `tools["cloudflare-bindings"]["org"]["shared"].query`, + `tools ['cloudflare-bindings'] [ "org" ] ['shared'].query`, + `tools.inventory['user'].personal.query`, + `tools.inventory["org"].shared.query`, + `tools.inventory.org['shared'].query`, + ])("rejects bracketed connection paths: %s", (path) => { + expect(oldStyleAddressRejection(`useQuery(${path}.queryOptions({}));`)).toContain( + "Artifact code must not name a connection", + ); + }); + + it("uses valid bracket notation in a hyphenated root's rejection guidance", () => { + const message = oldStyleAddressRejection(`tools['cloudflare-bindings'].org.shared.query({});`); + expect(message).toContain('tools["cloudflare-bindings"].org.'); + expect(message).toContain('tools["cloudflare-bindings"].(args)'); + expect(message).toContain('tools["cloudflare-bindings"]("prod")'); + expect(message).not.toContain("tools.cloudflare-bindings"); + }); + + it.each([ + `tools['cloudflare-bindings'].query.queryOptions({});`, + `tools["cloudflare-bindings"]('prod').query.queryOptions({});`, + `tools['cloudflare-bindings'].admin['user'].query({});`, + `tools.inventory['organization'].query({});`, + `tools.inventory.userProfile.query({});`, + `// tools['cloudflare-bindings'].org.shared.query({});`, + `/* tools["cloudflare-bindings"]['user'].personal.query({}); */`, + `other.tools['cloudflare-bindings'].org.shared.query({});`, + ])("keeps non-pinned and ignored bracket references legal: %s", (code) => { + expect(oldStyleAddressRejection(code)).toBeNull(); + }); + it("accepts the short form", () => { expect( oldStyleAddressRejection(`useQuery(tools.vercel.domains.getDomains.queryOptions({}));`), @@ -139,6 +225,44 @@ describe("oldStyleAddressRejection", () => { }); describe("resolveArtifactBindings", () => { + it("binds a single-quoted root through the existing connection inventory", () => { + const roles = extractArtifactRoles(`tools['cloudflare-bindings']('prod').query({});`); + expect( + resolveArtifactBindings({ + roles, + connections: { prod: "cloudflare-bindings.org.shared" }, + available: [connection("cloudflare-bindings", "org", "shared")], + }), + ).toEqual({ + ok: true, + bindings: { + prod: { integration: "cloudflare-bindings", owner: "org", connection: "shared" }, + }, + }); + }); + + it("still refuses a mismatched integration for an extracted bracket role", () => { + const result = resolveArtifactBindings({ + roles: extractArtifactRoles(`tools['cloudflare-bindings']('prod').query({});`), + connections: { prod: "linear.org.shared" }, + available: [connection("linear", "org", "shared")], + }); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.message).toContain("A role's connection must belong to the integration"); + }); + + it("still refuses unavailable connections for an extracted bracket role", () => { + const result = resolveArtifactBindings({ + roles: extractArtifactRoles(`tools['cloudflare-bindings'].query({});`), + connections: { "cloudflare-bindings": "cloudflare-bindings.org.missing" }, + available: [connection("cloudflare-bindings", "user", "personal")], + }); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.message).toContain('No connection "cloudflare-bindings.org.missing"'); + }); + it("binds a role silently when the author has exactly one connection", () => { const result = resolveArtifactBindings({ roles: [{ role: "vercel", integration: "vercel" }], diff --git a/packages/hosts/mcp/src/artifact-bindings.ts b/packages/hosts/mcp/src/artifact-bindings.ts index 92104e901b..367438e319 100644 --- a/packages/hosts/mcp/src/artifact-bindings.ts +++ b/packages/hosts/mcp/src/artifact-bindings.ts @@ -94,11 +94,12 @@ const withCommentsBlanked = (code: string): string => /** * A tools root reference, with the optional role call that follows it. * - * The role is captured from either quote flavour. Anything else after the root - * — property access, a call with an object — is left to the caller's own path - * handling; extraction only cares which integration slot is being reached. + * Static bracket roots and roles accept either quote flavour. Anything else + * after the root — property access, a call with an object — is left to the + * caller's own path handling; extraction only cares which integration slot is + * being reached. */ -const TOOL_ROOT = String.raw`(?:\.\s*([A-Za-z_$][\w$]*)|\[\s*"([A-Za-z_$][\w$-]*)"\s*\])`; +const TOOL_ROOT = String.raw`(?:\.\s*([A-Za-z_$][\w$]*)|\[\s*(?:"([A-Za-z_$][\w$-]*)"|'([A-Za-z_$][\w$-]*)')\s*\])`; const TOOLS_REFERENCE = new RegExp( String.raw`(?.user.` / `tools..org.`. + * integration, in either dot or static bracket notation. The root grammar is + * shared with role extraction so a supported root cannot escape this guard. * * Scoped to exactly those two literals in exactly that position. A tool whose * own first path segment is literally `user` or `org` would be caught by this @@ -114,18 +116,24 @@ const TOOLS_REFERENCE = new RegExp( * surface: the two words are reserved by the address grammar itself, the shape * is vanishingly rare, and the error says precisely what to write instead. */ -const OLD_STYLE_TIER_SEGMENT = /(? { const match = OLD_STYLE_TIER_SEGMENT.exec(withCommentsBlanked(code)); if (!match) return null; - const [, integration = "", tier = ""] = match; + const integration = match[1] ?? match[2] ?? match[3] ?? ""; + const tier = match[4] ?? match[5] ?? match[6] ?? ""; + const root = /^[A-Za-z_$][\w$]*$/.test(integration) + ? `tools.${integration}` + : `tools[${JSON.stringify(integration)}]`; return [ - `Artifact code must not name a connection: \`tools.${integration}.${tier}.…\` pins this artifact to one account.`, - `Address the integration only — \`tools.${integration}.(args)\` — and the server binds it to your connection when the artifact runs.`, - `Discovery through \`execute\` still uses the full \`tools.${integration}.${tier}..\` address; only saved artifact code drops the middle segments.`, - `If this artifact needs two accounts of the same integration, tag each one with a role — \`tools.${integration}("prod").(args)\` — and pass \`connections: { "prod": "${integration}.${tier}." }\` to create-artifact.`, + `Artifact code must not name a connection: \`${root}.${tier}.…\` pins this artifact to one account.`, + `Address the integration only — \`${root}.(args)\` — and the server binds it to your connection when the artifact runs.`, + `Discovery through \`execute\` still uses the full \`${root}.${tier}..\` address; only saved artifact code drops the middle segments.`, + `If this artifact needs two accounts of the same integration, tag each one with a role — \`${root}("prod").(args)\` — and pass \`connections: { "prod": "${integration}.${tier}." }\` to create-artifact.`, ].join(" "); }; @@ -141,9 +149,9 @@ export const extractArtifactRoles = (code: string): readonly ArtifactRole[] => { const scannable = withCommentsBlanked(code); const found = new Map(); for (const match of scannable.matchAll(TOOLS_REFERENCE)) { - const integration = match[1] ?? match[2]; + const integration = match[1] ?? match[2] ?? match[3]; if (integration === undefined || RESERVED_TOOL_ROOTS.has(integration)) continue; - const role = match[3] ?? match[4] ?? integration; + const role = match[4] ?? match[5] ?? integration; if (role.length === 0) continue; if (!found.has(role)) found.set(role, { role, integration }); } diff --git a/packages/hosts/mcp/src/artifacts-tools.test.ts b/packages/hosts/mcp/src/artifacts-tools.test.ts index bc0a958b20..5ac2b37d50 100644 --- a/packages/hosts/mcp/src/artifacts-tools.test.ts +++ b/packages/hosts/mcp/src/artifacts-tools.test.ts @@ -1949,6 +1949,56 @@ describe("MCP host — edit-artifact", () => { ); }); + it("binds a single-quoted root on edit and refuses a later pinned-path edit atomically", async () => { + const store = makeArtifactStore(); + const code = + "function App(){ useQuery(tools['cloudflare-bindings'].query.queryOptions({})); return
; }"; + await withClient( + makeStubEngine({}), + APPS_CAPS, + async (client) => { + await seed(client); + const edited = await client.callTool({ + name: "edit-artifact", + arguments: { + artifactId: "art_1", + edits: [{ oldText: COUNTER_CODE, newText: code }], + connections: { "cloudflare-bindings": "cloudflare-bindings.org.shared" }, + }, + }); + expect(edited.isError, textOf(edited)).toBeFalsy(); + expect(store.rows.get("art_1")?.bindings).toEqual({ + "cloudflare-bindings": { + integration: "cloudflare-bindings", + owner: "org", + connection: "shared", + }, + }); + + const refused = await client.callTool({ + name: "edit-artifact", + arguments: { + artifactId: "art_1", + edits: [ + { + oldText: "tools['cloudflare-bindings'].query", + newText: "tools['cloudflare-bindings']['org']['shared'].query", + }, + ], + }, + }); + expect(refused.isError).toBe(true); + expect(textOf(refused)).toContain("Artifact code must not name a connection"); + expect(store.calls).toHaveLength(2); + expect(store.rows.get("art_1")?.code).toBe(code); + }, + { + artifacts: store.port, + connections: connectionsPort([conn("cloudflare-bindings", "org", "shared")]), + }, + ); + }); + it("updates the title and description when asked, keeping the patched code", async () => { const store = makeArtifactStore(); await withClient( @@ -2171,6 +2221,30 @@ describe("MCP host — create-artifact binding", () => { }, ); }); + + it("refuses bracketed pinned paths before create-artifact can persist them", async () => { + const store = makeArtifactStore(); + await withClient( + makeStubEngine({}), + APPS_CAPS, + async (client) => { + const result = await client.callTool({ + name: "create-artifact", + arguments: { + title: "Pinned bracket path", + code: "function App(){ useQuery(tools['cloudflare-bindings']['org']['shared'].query.queryOptions({})); return
; }", + }, + }); + expect(result.isError).toBe(true); + expect(textOf(result)).toContain('tools["cloudflare-bindings"].(args)'); + expect(store.calls).toEqual([]); + }, + { + artifacts: store.port, + connections: connectionsPort([conn("cloudflare-bindings", "org", "shared")]), + }, + ); + }); }); // --------------------------------------------------------------------------- @@ -2236,6 +2310,17 @@ describe("MCP host — execute-action binding resolution", () => { ]); }); + it("creates a single-quoted root and resolves the shell's JSON-quoted action through its binding", async () => { + const { result, executed } = await runBoundAction({ + code: "function App(){ useQuery(tools['cloudflare-bindings']('prod').query.queryOptions({})); return
; }", + available: [conn("cloudflare-bindings", "org", "shared")], + connections: { prod: "cloudflare-bindings.org.shared" }, + action: { code: 'return await tools["cloudflare-bindings"]("prod").query({})' }, + }); + expect(result.isError).toBeFalsy(); + expect(executed).toEqual(['return await tools["cloudflare-bindings"].org.shared.query({})']); + }); + it("routes each role to its own connection", async () => { const roleCode = `function App(){ useQuery(tools.linear("prod").issues.list.queryOptions({}));