fix(engine): isolate getTenantConfig reads and normalize tenant config

packages/loopover-engine/src/tenant-config.ts

@@ -42,6 +42,13 @@ export const DEFAULT_TENANT_CONFIG: TenantConfig = {
  * is copied on every call — so mutating one tenant's config can never affect another's. An override with an
  * unrecognized autonomy level falls back to the default level rather than trusting arbitrary input.
  */
+// #9614: normalize any numeric input to a non-negative integer (a non-finite or negative value becomes 0) --
+// the exact body used in tenant-quota.ts:45-47 / loop-consumption.ts / worktree-pool.ts (the #5828
+// finiteNonNegativeInt discipline), so maxConcurrentLoops can never resolve NaN, fractional, negative, or Infinity.
+function finiteNonNegativeInt(value: number): number {
+  return Number.isFinite(value) ? Math.max(0, Math.floor(value)) : 0;
+}
+
 export function resolveTenantConfig(overrides: TenantConfigOverrides = {}): TenantConfig {
   const base = DEFAULT_TENANT_CONFIG;
   const autonomyLevel =
@@ -52,9 +59,17 @@ export function resolveTenantConfig(overrides: TenantConfigOverrides = {}): Tena
   return {
     autonomyLevel,
     preferences: {
-      maxConcurrentLoops: prefs.maxConcurrentLoops ?? base.preferences.maxConcurrentLoops,
+      // #9614: normalize an EXPLICITLY-supplied override (finiteNonNegativeInt); an absent override still
+      // inherits DEFAULT_TENANT_CONFIG's 1, since `??` alone let -5/0.5/NaN/Infinity pass through verbatim.
+      maxConcurrentLoops:
+        prefs.maxConcurrentLoops !== undefined ? finiteNonNegativeInt(prefs.maxConcurrentLoops) : base.preferences.maxConcurrentLoops,
       pauseOnFailure: prefs.pauseOnFailure ?? base.preferences.pauseOnFailure,
-      allowedActionClasses: [...(prefs.allowedActionClasses ?? base.preferences.allowedActionClasses)],
+      // #9614: drop non-string entries from an override rather than copying them through, mirroring how an
+      // unrecognized autonomyLevel is refused above -- untrusted input must not smuggle a non-string action class in.
+      allowedActionClasses:
+        prefs.allowedActionClasses !== undefined
+          ? prefs.allowedActionClasses.filter((entry): entry is string => typeof entry === "string")
+          : [...base.preferences.allowedActionClasses],
     },
   };
 }
@@ -74,10 +89,33 @@ export function setTenantConfig(
   tenantId: string,
   overrides: TenantConfigOverrides = {},
 ): TenantConfigStore {
-  return Object.freeze({ ...store, [tenantId]: resolveTenantConfig(overrides) });
+  // #9614: deep-freeze the written entry. Object.freeze is shallow, so freeze each level (the config, its
+  // preferences, and the allowedActionClasses array) -- otherwise a caller holding a reference to a nested
+  // value could still mutate the stored config.
+  const config = resolveTenantConfig(overrides);
+  Object.freeze(config.preferences.allowedActionClasses);
+  Object.freeze(config.preferences);
+  Object.freeze(config);
+  return Object.freeze({ ...store, [tenantId]: config });
+}
+
+// #9614: a fresh deep copy sharing no mutable reference with the stored (frozen-but-shallow) config, so a
+// caller that mutates the result can never rewrite the tenant's stored preferences/allowedActionClasses.
+function cloneTenantConfig(config: TenantConfig): TenantConfig {
+  return {
+    autonomyLevel: config.autonomyLevel,
+    preferences: {
+      maxConcurrentLoops: config.preferences.maxConcurrentLoops,
+      pauseOnFailure: config.preferences.pauseOnFailure,
+      allowedActionClasses: [...config.preferences.allowedActionClasses],
+    },
+  };
 }
 
-/** Read a tenant's effective config, falling back to a fresh copy of the defaults when they've set none. */
+/** Read a tenant's effective config as a fresh, caller-owned copy. The returned config -- and its nested
+ *  `preferences` and `allowedActionClasses` -- shares no reference with the store on EITHER path, so mutating
+ *  it never affects the stored config; a tenant with none set gets a fresh copy of the defaults. */
 export function getTenantConfig(store: TenantConfigStore, tenantId: string): TenantConfig {
-  return store[tenantId] ?? resolveTenantConfig();
+  const stored = store[tenantId];
+  return stored !== undefined ? cloneTenantConfig(stored) : resolveTenantConfig();
 }

packages/loopover-engine/test/tenant-config.test.ts

@@ -0,0 +1,63 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+
+import {
+  DEFAULT_TENANT_CONFIG,
+  EMPTY_TENANT_CONFIG_STORE,
+  getTenantConfig,
+  resolveTenantConfig,
+  setTenantConfig,
+} from "../dist/index.js";
+import type { TenantConfig, TenantConfigStore } from "../dist/index.js";
+
+test("resolveTenantConfig: defaults, autonomy override (recognized/unrecognized), explicit pauseOnFailure", () => {
+  assert.deepEqual(resolveTenantConfig(), DEFAULT_TENANT_CONFIG);
+  assert.equal(resolveTenantConfig({ autonomyLevel: "auto" }).autonomyLevel, "auto");
+  assert.equal(
+    resolveTenantConfig({ autonomyLevel: "banana" as never }).autonomyLevel,
+    DEFAULT_TENANT_CONFIG.autonomyLevel,
+  );
+  assert.equal(resolveTenantConfig({ preferences: { pauseOnFailure: false } }).preferences.pauseOnFailure, false);
+});
+
+test("#9614: maxConcurrentLoops normalizes an explicit override (finiteNonNegativeInt); absent inherits the default 1", () => {
+  const norm = (v: number) => resolveTenantConfig({ preferences: { maxConcurrentLoops: v } }).preferences.maxConcurrentLoops;
+  assert.equal(norm(-5), 0);
+  assert.equal(norm(0.5), 0);
+  assert.equal(norm(Number.NaN), 0);
+  assert.equal(norm(Number.POSITIVE_INFINITY), 0);
+  assert.equal(norm(3), 3);
+  assert.equal(resolveTenantConfig({ preferences: { pauseOnFailure: true } }).preferences.maxConcurrentLoops, 1);
+});
+
+test("#9614: allowedActionClasses drops non-string override entries and copies the default when absent", () => {
+  const cfg = resolveTenantConfig({
+    preferences: { allowedActionClasses: ["open_pr", 42, null, "comment"] as unknown as string[] },
+  });
+  assert.deepEqual(cfg.preferences.allowedActionClasses, ["open_pr", "comment"]);
+  assert.deepEqual(resolveTenantConfig().preferences.allowedActionClasses, DEFAULT_TENANT_CONFIG.preferences.allowedActionClasses);
+});
+
+test("#9614: getTenantConfig returns a caller-owned deep copy on the HIT path — mutating it never reaches the store", () => {
+  const store = setTenantConfig(EMPTY_TENANT_CONFIG_STORE, "acme", { preferences: { allowedActionClasses: ["open_pr"] } });
+  const first = getTenantConfig(store, "acme");
+  (first.preferences.allowedActionClasses as string[]).push("merge_pr");
+  first.preferences.maxConcurrentLoops = 99;
+  first.autonomyLevel = "auto";
+  const second = getTenantConfig(store, "acme");
+  assert.deepEqual(second.preferences.allowedActionClasses, ["open_pr"]);
+  assert.equal(second.preferences.maxConcurrentLoops, 1);
+  assert.equal(second.autonomyLevel, "suggest");
+});
+
+test("#9614: getTenantConfig returns fresh defaults on the MISS path", () => {
+  assert.deepEqual(getTenantConfig(EMPTY_TENANT_CONFIG_STORE, "unknown"), DEFAULT_TENANT_CONFIG);
+});
+
+test("#9614: setTenantConfig deep-freezes the stored entry (config, preferences, action-class array)", () => {
+  const store: TenantConfigStore = setTenantConfig(EMPTY_TENANT_CONFIG_STORE, "acme", {});
+  const stored = (store as Record<string, TenantConfig>)["acme"]!;
+  assert.equal(Object.isFrozen(stored), true);
+  assert.equal(Object.isFrozen(stored.preferences), true);
+  assert.equal(Object.isFrozen(stored.preferences.allowedActionClasses), true);
+});

test/unit/tenant-config.test.ts

@@ -68,3 +68,46 @@ describe("tenant config store (#4787)", () => {
     expect(DEFAULT_TENANT_CONFIG.preferences.allowedActionClasses).not.toContain("delete_repo");
   });
 });
+
+describe("tenant-config isolation + normalization (#9614)", () => {
+  it("normalizes an EXPLICIT maxConcurrentLoops override with finiteNonNegativeInt", () => {
+    const norm = (v: number) => resolveTenantConfig({ preferences: { maxConcurrentLoops: v } }).preferences.maxConcurrentLoops;
+    expect(norm(-5)).toBe(0); // negative -> 0
+    expect(norm(0.5)).toBe(0); // fractional -> floor -> 0
+    expect(norm(Number.NaN)).toBe(0); // non-finite -> 0
+    expect(norm(Number.POSITIVE_INFINITY)).toBe(0); // non-finite -> 0
+    expect(norm(3)).toBe(3); // finite positive int passes through
+  });
+
+  it("an ABSENT maxConcurrentLoops override still inherits DEFAULT_TENANT_CONFIG's 1", () => {
+    // pauseOnFailure supplied so the preferences override object is present but maxConcurrentLoops is not.
+    expect(resolveTenantConfig({ preferences: { pauseOnFailure: false } }).preferences.maxConcurrentLoops).toBe(1);
+  });
+
+  it("drops non-string entries from an allowedActionClasses override rather than copying them through", () => {
+    const cfg = resolveTenantConfig({
+      preferences: { allowedActionClasses: ["open_pr", 42, null, "comment"] as unknown as string[] },
+    });
+    expect(cfg.preferences.allowedActionClasses).toEqual(["open_pr", "comment"]);
+  });
+
+  it("getTenantConfig returns a caller-owned deep copy on the HIT path — mutating it never reaches the store", () => {
+    const store = setTenantConfig(EMPTY_TENANT_CONFIG_STORE, "acme", { preferences: { allowedActionClasses: ["open_pr"] } });
+    const first = getTenantConfig(store, "acme");
+    (first.preferences.allowedActionClasses as string[]).push("merge_pr");
+    first.preferences.maxConcurrentLoops = 99;
+    first.autonomyLevel = "auto";
+    const second = getTenantConfig(store, "acme");
+    expect(second.preferences.allowedActionClasses).toEqual(["open_pr"]);
+    expect(second.preferences.maxConcurrentLoops).toBe(1);
+    expect(second.autonomyLevel).toBe("suggest");
+  });
+
+  it("setTenantConfig deep-freezes the stored entry (config, its preferences, and the action-class array)", () => {
+    const store = setTenantConfig(EMPTY_TENANT_CONFIG_STORE, "acme", {});
+    const stored = store["acme"]!;
+    expect(Object.isFrozen(stored)).toBe(true);
+    expect(Object.isFrozen(stored.preferences)).toBe(true);
+    expect(Object.isFrozen(stored.preferences.allowedActionClasses)).toBe(true);
+  });
+});