Merge branch 'main' into datatable-roles-redesign

frontend/src/lib/components/copilot/chat/AssistantMcpSection.svelte

@@ -14,6 +14,11 @@ switch that decides whether this chat carries its tools.
 	import DropdownV2 from '$lib/components/DropdownV2.svelte'
 	import Toggle from '$lib/components/Toggle.svelte'
 	import { isMcpEnabled, setMcpEnabled } from '$lib/components/mcp/enabledServers'
+	import {
+		isOwnOrSharedMcpPath,
+		MCP_LIST_PER_PAGE,
+		mcpViewer
+	} from '$lib/components/mcp/ownServers'
 	import { loadProviderIcon } from '$lib/components/mcp/providerIcon'
 	import {
 		cachedProviderKey,
@@ -275,11 +280,15 @@ switch that decides whether this chat carries its tools.
 		loading = true
 		loadError = undefined
 		try {
-			const resources = await ResourceService.listResource({
-				workspace: target,
-				resourceType: 'mcp',
-				perPage: 100
-			})
+			const [listed, viewer] = await Promise.all([
+				ResourceService.listResource({
+					workspace: target,
+					resourceType: 'mcp',
+					perPage: MCP_LIST_PER_PAGE
+				}),
+				mcpViewer(target)
+			])
+			const resources = listed.filter((r) => isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer))
 			if (seq !== loadSeq) return
 			servers = resources.map((r) => ({
 				path: r.path,

frontend/src/lib/components/copilot/chat/global/core.test.ts

@@ -321,8 +321,9 @@ vi.mock('./rawAppBundlerBridge', () => ({
 
 vi.mock('$lib/infer', async () => ({
 	...(await vi.importActual<any>('$lib/infer')),
-	// Avoid the wasm parser in unit tests: the script deploy path infers the arg
-	// schema but tolerates failure, and these tests don't assert on the schema.
+	// Avoid the wasm parser in unit tests. A no-op passes the seeded schema through
+	// untouched, which is what lets the password-marking test pin how a schema is
+	// seeded in and carried out without pinning inference's own merge rules.
 	inferArgs: vi.fn(async () => {})
 }))
 
@@ -4358,6 +4359,43 @@ describe('global AI tools', () => {
 		expect(result).toContain('test logs')
 	})
 
+	// No parser emits `password`, so the stored schema is the only thing carrying it: an edit
+	// that rewrites the draft's schema from scratch, or a draft read that drops it, unmarks
+	// the field — and the form then takes the secret as a plain literal into the job's args.
+	it('test_run_script keeps the password marking of the script it previews', async () => {
+		vi.mocked(ScriptService.existsScriptByPath).mockResolvedValueOnce(true)
+		vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({
+			path: 'f/scripts/secretful',
+			language: 'bun',
+			schema: {
+				type: 'object',
+				properties: { token: { type: 'string', password: true } },
+				required: ['token']
+			}
+		} as any)
+
+		await callGlobalTool('write_script', {
+			path: 'f/scripts/secretful',
+			language: 'bun',
+			content: 'export async function main(token: string) { return 1 }'
+		})
+
+		let form: any
+		await callGlobalTool(
+			'test_run_script',
+			{ path: 'f/scripts/secretful' },
+			{
+				...toolCallbacks,
+				requestRunArgs: async (_toolId, opened) => {
+					form = opened
+					return undefined
+				}
+			}
+		)
+
+		expect(form?.schema?.properties).toMatchObject({ token: { password: true } })
+	})
+
 	it('test_run_script previews deployed script content when no draft exists', async () => {
 		vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({
 			path: 'f/scripts/deployed-test',

frontend/src/lib/components/copilot/chat/global/core.ts

@@ -148,6 +148,7 @@ import type { SessionArtifactsStore } from '../artifacts/artifactsState.svelte'
 import type { Runnable } from '$lib/components/apps/inputType'
 import { UserDraft } from '$lib/userDraft.svelte'
 import { emptySchema } from '$lib/utils'
+import type { Schema } from '$lib/common'
 import { inferArgs } from '$lib/infer'
 import {
 	resourceRequestSchema,
@@ -3424,7 +3425,8 @@ export const globalTools: Tool<{}>[] = [
 					draftCountByType.set(draft.type, count + 1)
 					byKey.set(getWorkspaceItemKey(draft.type, draft.path, draft.triggerKind), {
 						...draft,
-						value: undefined
+						value: undefined,
+						schema: undefined
 					})
 				}
 			}
@@ -5033,11 +5035,14 @@ const SCRIPT_SPEC: WriteSpec<NewScript, ScriptDraftArgs> = {
 					language: args.language,
 					kind: 'script'
 				}
-		// Infer the arg schema from the content at save time, like the editor does,
-		// so the persisted draft is the single source of truth at deploy. Keep the
-		// previous schema (or empty) on failure rather than blanking it.
+		// Into the schema the base carries, as the editor does at save: `inferArgs` re-seeds
+		// each arg from the properties it is handed, and those are the only copy of
+		// `password`, enums, formats and titles — no parser emits them. A clone, so a parse
+		// failure leaves the previous schema rather than half of one.
 		try {
-			const schema = emptySchema()
+			const schema = structuredClone(
+				draft.schema?.properties ? draft.schema : emptySchema()
+			) as Schema
 			await inferArgs(draft.language, draft.content, schema)
 			draft.schema = schema
 		} catch (e) {
@@ -5224,10 +5229,9 @@ async function loadScriptForEdit(
 	}
 }
 
-/** The fields a test form offers, for code that may never have been deployed. A draft the
- * chat wrote carries the schema it inferred at write time; anything else — a draft written
- * elsewhere, a deployed script whose schema predates an edit — is inferred here from the
- * content that is about to run, so the form cannot offer a field the code no longer takes. */
+/** The fields a test form offers, for code that may never have been deployed. The stored
+ * schema wins wherever it declares fields — a draft's or the deployed script's; only one
+ * declaring nothing is inferred here, from the content about to run. */
 async function schemaForTestRun(script: {
 	content: string
 	language: ScriptLang

frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts

@@ -4,7 +4,7 @@ const { getMcpToolsMock, callMcpToolMock, listResourceMock, session } = vi.hoist
 	getMcpToolsMock: vi.fn(),
 	callMcpToolMock: vi.fn(),
 	listResourceMock: vi.fn(),
-	session: { email: 'first@windmill.dev' }
+	session: { email: 'first@windmill.dev', workspace_id: 'test-ws', username: 'hugo', pgroups: [] }
 }))
 
 vi.mock('../shared', () => ({

frontend/src/lib/components/copilot/chat/global/mcpTools.ts

@@ -2,6 +2,7 @@ import { z } from 'zod'
 import { ResourceService, type GetMcpToolsResponse } from '$lib/gen'
 import { createToolDef, type Tool } from '../shared'
 import { enabledMcpPaths } from '$lib/components/mcp/enabledServers'
+import { isOwnOrSharedMcpPath, MCP_LIST_PER_PAGE, mcpViewer } from '$lib/components/mcp/ownServers'
 
 /**
  * Access to the MCP servers the user has connected (resources of type `mcp`)
@@ -91,13 +92,18 @@ export async function loadMcpServers(workspace: string): Promise<McpServer[]> {
 	const enabled = enabledMcpPaths(workspace)
 	if (enabled.length === 0) return []
 	try {
-		const resources = await ResourceService.listResource({
-			workspace,
-			resourceType: 'mcp',
-			perPage: 100
-		})
+		const [resources, viewer] = await Promise.all([
+			ResourceService.listResource({
+				workspace,
+				resourceType: 'mcp',
+				perPage: MCP_LIST_PER_PAGE
+			}),
+			mcpViewer(workspace)
+		])
 		return resources
-			.filter((r) => enabled.includes(r.path))
+			.filter(
+				(r) => enabled.includes(r.path) && isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer)
+			)
 			.map((r) => ({ path: r.path, editedAt: r.edited_at }))
 	} catch (e) {
 		console.error('Failed to load MCP servers', e)

frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts

@@ -141,6 +141,7 @@ function scriptDraftToWorkspaceItem(path: string, draft: NewScript): WorkspaceIt
 		summary: draft.summary,
 		language: draft.language,
 		value: draft.content,
+		schema: draft.schema,
 		parentHash: draft.parent_hash,
 		isDraft: true
 	}

frontend/src/lib/components/mcp/mcpMenu.svelte.ts

@@ -8,6 +8,7 @@ import type { Item } from '$lib/utils'
 import type { AIChatManager } from '../copilot/chat/AIChatManager.svelte'
 import { isMcpEnabled, setMcpEnabled } from './enabledServers'
 import { cachedProviderKey, rememberProviderKey } from './iconCache'
+import { isOwnOrSharedMcpPath, MCP_LIST_PER_PAGE, mcpViewer } from './ownServers'
 import { loadProviderIcon } from './providerIcon'
 import McpServerIcon from './McpServerIcon.svelte'
 
@@ -56,17 +57,22 @@ export class McpMenu {
 	async #load(ws: string) {
 		const seq = ++this.#seq
 		try {
-			const resources = await ResourceService.listResource({
-				workspace: ws,
-				resourceType: 'mcp',
-				perPage: 100
-			})
+			const [resources, viewer] = await Promise.all([
+				ResourceService.listResource({
+					workspace: ws,
+					resourceType: 'mcp',
+					perPage: MCP_LIST_PER_PAGE
+				}),
+				mcpViewer(ws)
+			])
 			if (seq !== this.#seq) return
-			this.#rows = resources.map((r) => ({
-				path: r.path,
-				editedAt: r.edited_at,
-				enabled: isMcpEnabled(ws, r.path)
-			}))
+			this.#rows = resources
+				.filter((r) => isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer))
+				.map((r) => ({
+					path: r.path,
+					editedAt: r.edited_at,
+					enabled: isMcpEnabled(ws, r.path)
+				}))
 			this.#rowsWorkspace = ws
 			void this.#loadIcons(ws, seq)
 		} catch {

frontend/src/lib/components/mcp/ownServers.test.ts

@@ -0,0 +1,53 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { session, roleMock } = vi.hoisted(() => ({
+	session: { workspace_id: 'browsed', username: 'alice', pgroups: ['g/browsed-only'] },
+	roleMock: vi.fn()
+}))
+
+vi.mock('$lib/stores', () => ({
+	userStore: { subscribe: (run: (v: unknown) => void) => (run({ ...session }), () => {}) }
+}))
+vi.mock('$lib/user', () => ({ getWorkspaceRole: roleMock }))
+
+import { isOwnOrSharedMcpPath, mcpViewer } from './ownServers'
+
+const alice = { username: 'alice', pgroups: ['g/eng'] }
+
+describe('isOwnOrSharedMcpPath', () => {
+	it('keeps own, folder, and explicitly shared servers; drops the rest of u/', () => {
+		expect(isOwnOrSharedMcpPath('u/alice/notion', {}, alice)).toBe(true)
+		expect(isOwnOrSharedMcpPath('f/team/notion', {}, alice)).toBe(true)
+		expect(isOwnOrSharedMcpPath('u/bob/notion', {}, alice)).toBe(false)
+		expect(isOwnOrSharedMcpPath('u/alicex/notion', {}, alice)).toBe(false)
+		expect(isOwnOrSharedMcpPath('u/bob/notion', { 'u/alice': false }, alice)).toBe(true)
+		expect(isOwnOrSharedMcpPath('u/bob/notion', { 'g/eng': false }, alice)).toBe(true)
+		expect(isOwnOrSharedMcpPath('u/bob/notion', { 'g/ops': true }, alice)).toBe(false)
+		expect(isOwnOrSharedMcpPath('u/alice/notion', {}, { username: undefined, pgroups: [] })).toBe(
+			false
+		)
+	})
+})
+
+describe('mcpViewer', () => {
+	beforeEach(() => roleMock.mockReset())
+
+	it('answers from the store only for the browsed workspace', async () => {
+		expect(await mcpViewer('browsed')).toEqual({ username: 'alice', pgroups: ['g/browsed-only'] })
+		expect(roleMock).not.toHaveBeenCalled()
+	})
+
+	it('looks the identity up for another workspace instead of reusing the browsed one', async () => {
+		roleMock.mockResolvedValue({
+			kind: 'resolved',
+			user: { username: 'alice_other', pgroups: ['g/eng'] }
+		})
+		expect(await mcpViewer('other')).toEqual({ username: 'alice_other', pgroups: ['g/eng'] })
+		expect(roleMock).toHaveBeenCalledWith('other')
+	})
+
+	it('yields no identity when the lookup fails', async () => {
+		roleMock.mockResolvedValue({ kind: 'lookup_failed' })
+		expect(await mcpViewer('other')).toEqual({ username: undefined, pgroups: [] })
+	})
+})

frontend/src/lib/components/mcp/ownServers.ts

@@ -0,0 +1,48 @@
+import { get } from 'svelte/store'
+import { userStore } from '$lib/stores'
+import { getWorkspaceRole } from '$lib/user'
+
+/** The principals an `extra_perms` entry can name to grant this user access. */
+export type McpViewer = { username: string | undefined; pgroups: string[] }
+
+/** Wide enough that a workspace's whole MCP catalog arrives at once: the filter
+ * below runs after the server's LIMIT, so a short page could drop the viewer's
+ * own rows while foreign `u/` rows fill it. */
+export const MCP_LIST_PER_PAGE = 1000
+
+/**
+ * Whether the chat lists this MCP server. A server under another user's `u/`
+ * prefix carries that person's credentials, and an admin's database role lets
+ * the resource listing return it. Acting through it would call the provider
+ * as them, so the chat only offers it when its owner shared it explicitly.
+ */
+export function isOwnOrSharedMcpPath(
+	path: string,
+	extraPerms: Record<string, unknown> | undefined,
+	viewer: McpViewer
+): boolean {
+	if (!path.startsWith('u/')) return true
+	if (viewer.username !== undefined && path.startsWith(`u/${viewer.username}/`)) return true
+	if (!extraPerms) return false
+	return (
+		(viewer.username !== undefined && `u/${viewer.username}` in extraPerms) ||
+		viewer.pgroups.some((g) => g in extraPerms)
+	)
+}
+
+/**
+ * The viewer's identity in `workspace`. Username and groups are per workspace,
+ * and a session chat can operate on a workspace other than the one being
+ * browsed, so `userStore` only answers when it describes that same workspace.
+ * A failed lookup yields no username: every `u/` server is then hidden rather
+ * than judged against another workspace's identity.
+ */
+export async function mcpViewer(workspace: string): Promise<McpViewer> {
+	const u = get(userStore)
+	if (u?.workspace_id === workspace) return { username: u.username, pgroups: u.pgroups ?? [] }
+	const role = await getWorkspaceRole(workspace)
+	if (role.kind === 'resolved') {
+		return { username: role.user.username, pgroups: role.user.pgroups ?? [] }
+	}
+	return { username: undefined, pgroups: [] }
+}