From e8287f1d56ea3eac8ca77fe6e225e4748e1ba9dc Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Mon, 1 Jun 2026 08:40:47 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=A6=20feat:=20Allow=20Scope=20Override?= =?UTF-8?q?s=20To=20Tombstone=20Inherited=20MCP=20Server=20Entries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add tombstone semantics to the config merge layer for record-type sections that can grow scope-specific deletions. A scope override with a null value at \`mcpServers.\` now removes that key from the effective config for the scope instead of either storing null or no-oping. The admin panel side will follow up by writing this null value where it previously sent a DELETE that could only remove an existing override. TOMBSTONE_SUPPORTED_PARENTS gates the behavior to mcpConfig (the remapped name for the mcpServers section). Nested null values beneath an entry key keep their literal meaning, a null at the section root keeps acting as a normal replacement, and a higher priority layer can re-assert an entry that a lower priority layer tombstoned. Tombstoning an entry the base does not define is a silent no-op. Six new resolution.spec tests cover those paths. --- .../data-schemas/src/app/resolution.spec.ts | 75 +++++++++++++++++++ packages/data-schemas/src/app/resolution.ts | 12 +++ 2 files changed, 87 insertions(+) diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index b751e0bbcf..d92ff7baf6 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -428,6 +428,81 @@ describe('mergeConfigOverrides', () => { }); }); +describe('mergeConfigOverrides — mcpServers tombstones', () => { + const baseWithMcp = { + interfaceConfig: { modelSelect: true }, + mcpConfig: { + inherited: { type: 'sse', url: 'https://base.example.com' }, + other: { type: 'sse', url: 'https://other.example.com', title: 'Other' }, + }, + } as unknown as AppConfig; + + it('drops an inherited mcpServers entry when the override sets it to null', () => { + const configs = [fakeConfig({ mcpServers: { inherited: null } }, 10)]; + const result = mergeConfigOverrides(baseWithMcp, configs) as unknown as Record; + const mcpConfig = result.mcpConfig as Record; + expect(mcpConfig.inherited).toBeUndefined(); + expect(mcpConfig.other).toEqual({ + type: 'sse', + url: 'https://other.example.com', + title: 'Other', + }); + }); + + it('preserves other inherited entries and applies partial overrides alongside a tombstone', () => { + const configs = [ + fakeConfig({ mcpServers: { inherited: null, other: { title: 'Edited' } } }, 10), + ]; + const result = mergeConfigOverrides(baseWithMcp, configs) as unknown as Record; + const mcpConfig = result.mcpConfig as Record; + expect(mcpConfig.inherited).toBeUndefined(); + expect(mcpConfig.other).toEqual({ + type: 'sse', + url: 'https://other.example.com', + title: 'Edited', + }); + }); + + it('lets a higher-priority override re-assert an entry tombstoned at lower priority', () => { + const configs = [ + fakeConfig({ mcpServers: { inherited: null } }, 5), + fakeConfig( + { mcpServers: { inherited: { type: 'sse', url: 'https://reassert.example.com' } } }, + 10, + ), + ]; + const result = mergeConfigOverrides(baseWithMcp, configs) as unknown as Record; + const mcpConfig = result.mcpConfig as Record; + expect(mcpConfig.inherited).toEqual({ type: 'sse', url: 'https://reassert.example.com' }); + }); + + it('treats null at the section root as a normal merge replacement, not a tombstone', () => { + const configs = [fakeConfig({ mcpServers: null }, 10)]; + const result = mergeConfigOverrides(baseWithMcp, configs) as unknown as Record; + expect(result.mcpConfig).toBeNull(); + }); + + it('does not tombstone null values nested beneath an entry key', () => { + const configs = [fakeConfig({ mcpServers: { other: { title: null } } }, 10)]; + const result = mergeConfigOverrides(baseWithMcp, configs) as unknown as Record; + const mcpConfig = result.mcpConfig as Record; + expect(mcpConfig.other).toEqual({ + type: 'sse', + url: 'https://other.example.com', + title: null, + }); + }); + + it('is a no-op tombstone for an entry that does not exist in base', () => { + const configs = [fakeConfig({ mcpServers: { neverExisted: null } }, 10)]; + const result = mergeConfigOverrides(baseWithMcp, configs) as unknown as Record; + const mcpConfig = result.mcpConfig as Record; + expect('neverExisted' in mcpConfig).toBe(false); + expect(mcpConfig.inherited).toBeDefined(); + expect(mcpConfig.other).toBeDefined(); + }); +}); + describe('INTERFACE_PERMISSION_FIELDS', () => { it('contains all expected permission fields', () => { const expected = [ diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index cc1e11cb82..c2c77237f8 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -20,6 +20,16 @@ const ARRAY_MERGE_KEYS: Record = { 'endpoints.custom': 'name', }; +/** + * Record-type sections whose override layer interprets a `null` value at a child + * key as a tombstone: the entry inherited from base (or a lower-priority layer) + * is removed from the effective config for this scope. Without this, a scope + * can only add or amend entries, never subtract inherited ones, because the + * per-field PATCH path has no way to express "for this scope, pretend the + * inherited entry does not exist." + */ +const TOMBSTONE_SUPPORTED_PARENTS = new Set(['mcpConfig']); + /** * Maps YAML-level override keys (TCustomConfig) to their AppConfig equivalents. * Overrides are stored with YAML keys but merged into the already-processed AppConfig @@ -122,6 +132,8 @@ function deepMerge(target: T, source: AnyObject, depth = 0, depth, currentPath, ); + } else if (sourceVal === null && TOMBSTONE_SUPPORTED_PARENTS.has(path)) { + delete result[key]; } else { result[key] = sourceVal; }