diff --git a/docs/mcp/tools-and-contracts.md b/docs/mcp/tools-and-contracts.md index a1f06a0638..17e23a9699 100644 --- a/docs/mcp/tools-and-contracts.md +++ b/docs/mcp/tools-and-contracts.md @@ -186,6 +186,8 @@ For payload normalization in management tools, Code UX centralizes parsing behav - **Optional Numbers**: Extracted via `parseOptionalNumber`. Validates finiteness and optional min/max constraints. - **Optional Enums**: Extracted via `parseOptionalEnum`. Normalizes case and whitespace to match allowed literal types. - **Strict Optional Integers and Enums**: Extracted via `parseOptionalIntegerStrict` and `parseOptionalEnumStrict` when a supplied invalid value should be rejected instead of silently ignored. Omitted values still allow action-level defaults. +- **Required Objects**: Extracted via `parseRequiredObject`. The value must be a non-null object and not an array. +- **Required Present Values**: Extracted via `parseRequiredPresentValue` for patch-style payloads. The key must be present, but the value may explicitly be `null`; omitted and `undefined` values are distinct from `null` in approval fingerprints and patch application. - **Validation Errors**: Parser failures throw `ManagementValidationError`, which the management tool handler serializes as the standardized `result.status: "error"` envelope with `errorType: "validation"` and `isError: true`. diff --git a/src/mcp/management/payload-parsers.ts b/src/mcp/management/payload-parsers.ts index c095f820d6..e8b1b056ee 100644 --- a/src/mcp/management/payload-parsers.ts +++ b/src/mcp/management/payload-parsers.ts @@ -121,6 +121,21 @@ export function parseOptionalObject(payload: Record, key: st return undefined; } +export function parseRequiredObject(payload: Record, key: string, customError?: string): T { + const val = payload[key]; + if (typeof val === "object" && val !== null && !Array.isArray(val)) { + return val as T; + } + throw managementValidationError(customError || `${key} object is required`, key); +} + +export function parseRequiredPresentValue(payload: Record, key: string, customError?: string): unknown { + if (!(key in payload)) { + throw managementValidationError(customError || `${key} is required`, key); + } + return payload[key]; +} + export function parseOptionalEnum(payload: Record, key: string, validValues: readonly T[]): T | undefined { const val = payload[key]; if (typeof val === "string") { diff --git a/src/mcp/management/settings-actions.ts b/src/mcp/management/settings-actions.ts index 1b9ee0a042..76fe500b6b 100644 --- a/src/mcp/management/settings-actions.ts +++ b/src/mcp/management/settings-actions.ts @@ -2,7 +2,11 @@ import type { ManageCodeUxArgs, ManagementResponseEnvelope } from "../../contrac import type { SettingsRepository } from "../../repositories/settings-repository.js"; import { SettingsPathUpdater } from "../../services/settings-path-updater.js"; import type { SystemSettings, ProjectSettingsOverride, SprintSettingsOverride } from "../../contracts/settings-scope-types.js"; -import { managementValidationError, parseRequiredString as readRequiredString } from "./payload-parsers.js"; +import { + parseRequiredObject, + parseRequiredPresentValue, + parseRequiredString as readRequiredString, +} from "./payload-parsers.js"; const SETTINGS_APPROVAL_TTL_MS = 15 * 60 * 1000; const SETTINGS_APPROVAL_MESSAGE = [ @@ -12,21 +16,6 @@ const SETTINGS_APPROVAL_MESSAGE = [ "This approval is one-use, bound to this exact action and payload, and expires in 15 minutes.", ].join(" "); -function readRequiredValue(payload: Record): unknown { - if (!("value" in payload)) { - throw managementValidationError("value is required", "value"); - } - return payload.value; -} - -function readRequiredSettingsObject(payload: Record): T { - const settings = payload.settings; - if (typeof settings !== "object" || settings === null || Array.isArray(settings)) { - throw managementValidationError("settings object is required", "settings"); - } - return settings as T; -} - function normalizeForApproval(value: unknown): unknown { if (Array.isArray(value)) { return value.map((item) => normalizeForApproval(item)); @@ -155,7 +144,7 @@ export class SettingsActions { } private replaceSystemSettings(args: ManageCodeUxArgs, payload: Record): ManagementResponseEnvelope { - const settings = readRequiredSettingsObject(payload); + const settings = parseRequiredObject(payload, "settings", "settings object is required"); const approval = this.requireSettingsApproval(args, payload); if (approval) return approval; return { result: { settings: this.settingsRepository.saveSystemSettings(settings) } }; @@ -163,7 +152,7 @@ export class SettingsActions { private patchSystemSetting(args: ManageCodeUxArgs, payload: Record): ManagementResponseEnvelope { const path = readRequiredString(payload, "path"); - const value = readRequiredValue(payload); + const value = parseRequiredPresentValue(payload, "value"); const approval = this.requireSettingsApproval(args, payload); if (approval) return approval; @@ -174,7 +163,7 @@ export class SettingsActions { private replaceProjectSettings(args: ManageCodeUxArgs, payload: Record): ManagementResponseEnvelope { const projectId = readRequiredString(payload, "projectId"); - const settings = readRequiredSettingsObject(payload); + const settings = parseRequiredObject(payload, "settings", "settings object is required"); const approval = this.requireSettingsApproval(args, payload); if (approval) return approval; return { result: { settings: this.settingsRepository.saveProjectSettings(projectId, settings) } }; @@ -183,7 +172,7 @@ export class SettingsActions { private patchProjectSetting(args: ManageCodeUxArgs, payload: Record): ManagementResponseEnvelope { const projectId = readRequiredString(payload, "projectId"); const path = readRequiredString(payload, "path"); - const value = readRequiredValue(payload); + const value = parseRequiredPresentValue(payload, "value"); const approval = this.requireSettingsApproval(args, payload); if (approval) return approval; @@ -203,7 +192,7 @@ export class SettingsActions { private replaceSprintSettings(args: ManageCodeUxArgs, payload: Record): ManagementResponseEnvelope { const projectId = readRequiredString(payload, "projectId"); const sprintId = readRequiredString(payload, "sprintId"); - const settings = readRequiredSettingsObject(payload); + const settings = parseRequiredObject(payload, "settings", "settings object is required"); const approval = this.requireSettingsApproval(args, payload); if (approval) return approval; @@ -215,7 +204,7 @@ export class SettingsActions { const projectId = readRequiredString(payload, "projectId"); const sprintId = readRequiredString(payload, "sprintId"); const path = readRequiredString(payload, "path"); - const value = readRequiredValue(payload); + const value = parseRequiredPresentValue(payload, "value"); const approval = this.requireSettingsApproval(args, payload); if (approval) return approval; diff --git a/tests/backend/mcp/management-settings-actions.test.ts b/tests/backend/mcp/management-settings-actions.test.ts index 0017c41ff4..d575d2edc2 100644 --- a/tests/backend/mcp/management-settings-actions.test.ts +++ b/tests/backend/mcp/management-settings-actions.test.ts @@ -124,6 +124,25 @@ describe("SettingsActions", () => { expect(settingsRepository.saveSystemSettings).not.toHaveBeenCalled(); }); + it("preserves explicit null patch values after confirmation", async () => { + const payload = { path: "defaults.automationLevel", value: null }; + + await actions.handleSettingsAction({ + domain: "settings", + action: "patch_system_setting", + payload, + }); + const res = await actions.handleSettingsAction({ + domain: "settings", + action: "patch_system_setting", + payload, + approval: { confirmed: true }, + }); + + expect(res.result).toEqual({ settings: { defaults: { automationLevel: null } } }); + expect(settingsRepository.saveSystemSettings).toHaveBeenCalledWith({ defaults: { automationLevel: null } }); + }); + it("requires approval for replacing system settings", async () => { const res = await actions.handleSettingsAction({ domain: "settings", @@ -249,6 +268,13 @@ describe("SettingsActions", () => { ).rejects.toThrow(/settings object is required/); }); + it("rejects replace_system_settings with an invalid settings object type", async () => { + await expect( + actions.handleSettingsAction({ domain: "settings", action: "replace_system_settings", payload: { settings: [] } }), + ).rejects.toThrow(/settings object is required/); + expect(settingsRepository.saveSystemSettings).not.toHaveBeenCalled(); + }); + it("rejects replace_project_settings without a settings object", async () => { await expect( actions.handleSettingsAction({ domain: "settings", action: "replace_project_settings", payload: { projectId: "p" } }),