Standardize management payload validation (codex)

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`.
 
 

src/mcp/management/payload-parsers.ts

@@ -121,6 +121,21 @@ export function parseOptionalObject<T>(payload: Record<string, unknown>, key: st
   return undefined;
 }
 
+export function parseRequiredObject<T>(payload: Record<string, unknown>, 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<string, unknown>, key: string, customError?: string): unknown {
+  if (!(key in payload)) {
+    throw managementValidationError(customError || `${key} is required`, key);
+  }
+  return payload[key];
+}
+
 export function parseOptionalEnum<T extends string>(payload: Record<string, unknown>, key: string, validValues: readonly T[]): T | undefined {
   const val = payload[key];
   if (typeof val === "string") {

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<string, unknown>): unknown {
-  if (!("value" in payload)) {
-    throw managementValidationError("value is required", "value");
-  }
-  return payload.value;
-}
-
-function readRequiredSettingsObject<T>(payload: Record<string, unknown>): 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,15 +144,15 @@ export class SettingsActions {
   }
 
   private replaceSystemSettings(args: ManageCodeUxArgs, payload: Record<string, unknown>): ManagementResponseEnvelope {
-    const settings = readRequiredSettingsObject<SystemSettings>(payload);
+    const settings = parseRequiredObject<SystemSettings>(payload, "settings", "settings object is required");
     const approval = this.requireSettingsApproval(args, payload);
     if (approval) return approval;
     return { result: { settings: this.settingsRepository.saveSystemSettings(settings) } };
   }
 
   private patchSystemSetting(args: ManageCodeUxArgs, payload: Record<string, unknown>): 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<string, unknown>): ManagementResponseEnvelope {
     const projectId = readRequiredString(payload, "projectId");
-    const settings = readRequiredSettingsObject<ProjectSettingsOverride>(payload);
+    const settings = parseRequiredObject<ProjectSettingsOverride>(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<string, unknown>): 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<string, unknown>): ManagementResponseEnvelope {
     const projectId = readRequiredString(payload, "projectId");
     const sprintId = readRequiredString(payload, "sprintId");
-    const settings = readRequiredSettingsObject<SprintSettingsOverride>(payload);
+    const settings = parseRequiredObject<SprintSettingsOverride>(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;
 

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" } }),