From 24dad48732664ed9ab30d24b6941c01476f0762b Mon Sep 17 00:00:00 2001 From: kai392 Date: Wed, 29 Jul 2026 08:52:01 +0800 Subject: [PATCH] fix(engine): isolate getTenantConfig reads and normalize tenant config (#9614) tenant-config.ts promised full per-tenant isolation but leaked the stored object and skipped normalizing maxConcurrentLoops: - getTenantConfig returned the very object held in the store on a hit (only the miss path was a fresh copy), and setTenantConfig's Object.freeze is shallow, so a caller could mutate preferences/allowedActionClasses and silently rewrite a tenant's stored config. Now getTenantConfig deep-copies on BOTH paths, and setTenantConfig deep-freezes the written entry (config, preferences, and the action-class array). - resolveTenantConfig now normalizes an explicitly-supplied maxConcurrentLoops override with the exact finiteNonNegativeInt body used in tenant-quota.ts / loop-consumption.ts / worktree-pool.ts (#5828) -- -5/0.5/NaN/Infinity no longer pass through; an absent override still inherits DEFAULT_TENANT_CONFIG's 1. It also drops non-string allowedActionClasses entries rather than copying them through, mirroring the existing autonomyLevel guard. - Corrected getTenantConfig's JSDoc; DEFAULT_TENANT_CONFIG, TENANT_AUTONOMY_LEVELS, EMPTY_TENANT_CONFIG_STORE, and every exported type are unchanged. Tests: 5 new cases (100% branch on the changed code, verified non-vacuous). Closes #9614 Co-Authored-By: Claude Opus 4.8 --- packages/loopover-engine/src/tenant-config.ts | 204 +++++++++++------- test/unit/tenant-config.test.ts | 183 ++++++++++------ 2 files changed, 234 insertions(+), 153 deletions(-) diff --git a/packages/loopover-engine/src/tenant-config.ts b/packages/loopover-engine/src/tenant-config.ts index 9334f17818..4130503da2 100644 --- a/packages/loopover-engine/src/tenant-config.ts +++ b/packages/loopover-engine/src/tenant-config.ts @@ -1,83 +1,121 @@ -// Per-tenant configuration layer (pure) — #4787, part of the Rent-a-Loop path #4778. -// -// A customer's own autonomy/config, scoped strictly to their rented repo and independent of loopover's own -// configuration. Deterministic and side-effect-free: it resolves a tenant's effective config from the defaults -// plus their overrides, and holds per-tenant configs in an IMMUTABLE store. Isolation is guaranteed by -// construction — every resolve returns a NEW config with freshly-copied collections, and every store update -// returns a NEW store, so setting or mutating one tenant's config can never affect another tenant's config or -// the shared defaults (the no-cross-contamination requirement). The autonomy level mirrors #4782's graduated -// dial (taken as a value here, not depending on its wiring). This resolves and holds config only — persisting -// it to a datastore is a separate, maintainer-owned concern. - -export type TenantAutonomyLevel = "off" | "suggest" | "assist" | "auto"; - -export const TENANT_AUTONOMY_LEVELS: readonly TenantAutonomyLevel[] = ["off", "suggest", "assist", "auto"]; - -/** Repo-specific execution preferences a tenant can tune for their own loop. */ -export type TenantExecutionPreferences = { - maxConcurrentLoops: number; - pauseOnFailure: boolean; - allowedActionClasses: readonly string[]; -}; - -export type TenantConfig = { - autonomyLevel: TenantAutonomyLevel; - preferences: TenantExecutionPreferences; -}; - -export type TenantConfigOverrides = { - autonomyLevel?: TenantAutonomyLevel | undefined; - preferences?: Partial | undefined; -}; - -/** The conservative baseline a tenant inherits until they override it. */ -export const DEFAULT_TENANT_CONFIG: TenantConfig = { - autonomyLevel: "suggest", - preferences: { maxConcurrentLoops: 1, pauseOnFailure: true, allowedActionClasses: ["open_pr", "comment"] }, -}; - -/** - * Resolve a tenant's effective config from the defaults plus their overrides. Pure and fully isolated: the - * returned config shares no mutable reference with the defaults or any other resolution — the action-class list - * 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. - */ -export function resolveTenantConfig(overrides: TenantConfigOverrides = {}): TenantConfig { - const base = DEFAULT_TENANT_CONFIG; - const autonomyLevel = - overrides.autonomyLevel !== undefined && TENANT_AUTONOMY_LEVELS.includes(overrides.autonomyLevel) - ? overrides.autonomyLevel - : base.autonomyLevel; - const prefs = overrides.preferences ?? {}; - return { - autonomyLevel, - preferences: { - maxConcurrentLoops: prefs.maxConcurrentLoops ?? base.preferences.maxConcurrentLoops, - pauseOnFailure: prefs.pauseOnFailure ?? base.preferences.pauseOnFailure, - allowedActionClasses: [...(prefs.allowedActionClasses ?? base.preferences.allowedActionClasses)], - }, - }; -} - -/** An immutable map of tenant id → resolved config. Setting a tenant returns a new store (see below). */ -export type TenantConfigStore = Readonly>; - -export const EMPTY_TENANT_CONFIG_STORE: TenantConfigStore = Object.freeze({}); - -/** - * Set a tenant's config from their overrides, returning a NEW store. The updated tenant's entry is a freshly - * resolved config; every other tenant's entry is carried over untouched, so one customer setting their config - * can never mutate or observe another customer's. Immutable update — the input store is never modified. - */ -export function setTenantConfig( - store: TenantConfigStore, - tenantId: string, - overrides: TenantConfigOverrides = {}, -): TenantConfigStore { - return Object.freeze({ ...store, [tenantId]: resolveTenantConfig(overrides) }); -} - -/** Read a tenant's effective config, falling back to a fresh copy of the defaults when they've set none. */ -export function getTenantConfig(store: TenantConfigStore, tenantId: string): TenantConfig { - return store[tenantId] ?? resolveTenantConfig(); -} +// Per-tenant configuration layer (pure) — #4787, part of the Rent-a-Loop path #4778. +// +// A customer's own autonomy/config, scoped strictly to their rented repo and independent of loopover's own +// configuration. Deterministic and side-effect-free: it resolves a tenant's effective config from the defaults +// plus their overrides, and holds per-tenant configs in an IMMUTABLE store. Isolation is guaranteed by +// construction — every resolve returns a NEW config with freshly-copied collections, and every store update +// returns a NEW store, so setting or mutating one tenant's config can never affect another tenant's config or +// the shared defaults (the no-cross-contamination requirement). The autonomy level mirrors #4782's graduated +// dial (taken as a value here, not depending on its wiring). This resolves and holds config only — persisting +// it to a datastore is a separate, maintainer-owned concern. + +export type TenantAutonomyLevel = "off" | "suggest" | "assist" | "auto"; + +export const TENANT_AUTONOMY_LEVELS: readonly TenantAutonomyLevel[] = ["off", "suggest", "assist", "auto"]; + +/** Repo-specific execution preferences a tenant can tune for their own loop. */ +export type TenantExecutionPreferences = { + maxConcurrentLoops: number; + pauseOnFailure: boolean; + allowedActionClasses: readonly string[]; +}; + +export type TenantConfig = { + autonomyLevel: TenantAutonomyLevel; + preferences: TenantExecutionPreferences; +}; + +export type TenantConfigOverrides = { + autonomyLevel?: TenantAutonomyLevel | undefined; + preferences?: Partial | undefined; +}; + +/** The conservative baseline a tenant inherits until they override it. */ +export const DEFAULT_TENANT_CONFIG: TenantConfig = { + autonomyLevel: "suggest", + preferences: { maxConcurrentLoops: 1, pauseOnFailure: true, allowedActionClasses: ["open_pr", "comment"] }, +}; + +/** + * Resolve a tenant's effective config from the defaults plus their overrides. Pure and fully isolated: the + * returned config shares no mutable reference with the defaults or any other resolution — the action-class list + * 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 = + overrides.autonomyLevel !== undefined && TENANT_AUTONOMY_LEVELS.includes(overrides.autonomyLevel) + ? overrides.autonomyLevel + : base.autonomyLevel; + const prefs = overrides.preferences ?? {}; + return { + autonomyLevel, + preferences: { + // #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, + // #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], + }, + }; +} + +/** An immutable map of tenant id → resolved config. Setting a tenant returns a new store (see below). */ +export type TenantConfigStore = Readonly>; + +export const EMPTY_TENANT_CONFIG_STORE: TenantConfigStore = Object.freeze({}); + +/** + * Set a tenant's config from their overrides, returning a NEW store. The updated tenant's entry is a freshly + * resolved config; every other tenant's entry is carried over untouched, so one customer setting their config + * can never mutate or observe another customer's. Immutable update — the input store is never modified. + */ +export function setTenantConfig( + store: TenantConfigStore, + tenantId: string, + overrides: TenantConfigOverrides = {}, +): TenantConfigStore { + // #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 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 { + const stored = store[tenantId]; + return stored !== undefined ? cloneTenantConfig(stored) : resolveTenantConfig(); +} diff --git a/test/unit/tenant-config.test.ts b/test/unit/tenant-config.test.ts index d81eed161b..b914f91f18 100644 --- a/test/unit/tenant-config.test.ts +++ b/test/unit/tenant-config.test.ts @@ -1,70 +1,113 @@ -import { describe, expect, it } from "vitest"; - -import { - DEFAULT_TENANT_CONFIG, - EMPTY_TENANT_CONFIG_STORE, - getTenantConfig, - resolveTenantConfig, - setTenantConfig, -} from "../../packages/loopover-engine/src/tenant-config"; - -describe("resolveTenantConfig (#4787)", () => { - it("returns the defaults when given no overrides", () => { - expect(resolveTenantConfig()).toEqual(DEFAULT_TENANT_CONFIG); - }); - - it("does not share a mutable reference with the defaults (fresh action-class list)", () => { - const cfg = resolveTenantConfig(); - (cfg.preferences.allowedActionClasses as string[]).push("merge"); - expect(DEFAULT_TENANT_CONFIG.preferences.allowedActionClasses).not.toContain("merge"); - }); - - it("applies a recognized autonomy-level override", () => { - expect(resolveTenantConfig({ autonomyLevel: "auto" }).autonomyLevel).toBe("auto"); - }); - - it("falls back to the default autonomy level when the override is unrecognized", () => { - expect(resolveTenantConfig({ autonomyLevel: "banana" as never }).autonomyLevel).toBe(DEFAULT_TENANT_CONFIG.autonomyLevel); - }); - - it("merges a partial preferences override onto the defaults", () => { - const cfg = resolveTenantConfig({ preferences: { maxConcurrentLoops: 5 } }); - expect(cfg.preferences.maxConcurrentLoops).toBe(5); - expect(cfg.preferences.pauseOnFailure).toBe(DEFAULT_TENANT_CONFIG.preferences.pauseOnFailure); - expect(cfg.preferences.allowedActionClasses).toEqual(DEFAULT_TENANT_CONFIG.preferences.allowedActionClasses); - }); - - it("honors an explicit false pauseOnFailure (not treated as absent) and a custom action-class list", () => { - const cfg = resolveTenantConfig({ preferences: { pauseOnFailure: false, allowedActionClasses: ["comment"] } }); - expect(cfg.preferences.pauseOnFailure).toBe(false); - expect(cfg.preferences.allowedActionClasses).toEqual(["comment"]); - }); -}); - -describe("tenant config store (#4787)", () => { - it("setTenantConfig returns a NEW store and never mutates the input (immutable update)", () => { - const s0 = EMPTY_TENANT_CONFIG_STORE; - const s1 = setTenantConfig(s0, "acme", { autonomyLevel: "auto" }); - expect(s1).not.toBe(s0); - expect(s0).toEqual({}); // input untouched - expect(getTenantConfig(s1, "acme").autonomyLevel).toBe("auto"); - }); - - it("getTenantConfig returns the defaults for a tenant that has set nothing", () => { - expect(getTenantConfig(EMPTY_TENANT_CONFIG_STORE, "unknown")).toEqual(DEFAULT_TENANT_CONFIG); - }); - - it("two tenants hold independent configs with no cross-contamination (acceptance)", () => { - let store = EMPTY_TENANT_CONFIG_STORE; - store = setTenantConfig(store, "tenant-a", { autonomyLevel: "auto", preferences: { allowedActionClasses: ["open_pr"] } }); - store = setTenantConfig(store, "tenant-b", { autonomyLevel: "off" }); - const a = getTenantConfig(store, "tenant-a"); - const b = getTenantConfig(store, "tenant-b"); - expect(a.autonomyLevel).toBe("auto"); - expect(b.autonomyLevel).toBe("off"); - // Mutating tenant A's resolved list must not affect tenant B or the defaults. - (a.preferences.allowedActionClasses as string[]).push("delete_repo"); - expect(getTenantConfig(store, "tenant-b").preferences.allowedActionClasses).not.toContain("delete_repo"); - expect(DEFAULT_TENANT_CONFIG.preferences.allowedActionClasses).not.toContain("delete_repo"); - }); -}); +import { describe, expect, it } from "vitest"; + +import { + DEFAULT_TENANT_CONFIG, + EMPTY_TENANT_CONFIG_STORE, + getTenantConfig, + resolveTenantConfig, + setTenantConfig, +} from "../../packages/loopover-engine/src/tenant-config"; + +describe("resolveTenantConfig (#4787)", () => { + it("returns the defaults when given no overrides", () => { + expect(resolveTenantConfig()).toEqual(DEFAULT_TENANT_CONFIG); + }); + + it("does not share a mutable reference with the defaults (fresh action-class list)", () => { + const cfg = resolveTenantConfig(); + (cfg.preferences.allowedActionClasses as string[]).push("merge"); + expect(DEFAULT_TENANT_CONFIG.preferences.allowedActionClasses).not.toContain("merge"); + }); + + it("applies a recognized autonomy-level override", () => { + expect(resolveTenantConfig({ autonomyLevel: "auto" }).autonomyLevel).toBe("auto"); + }); + + it("falls back to the default autonomy level when the override is unrecognized", () => { + expect(resolveTenantConfig({ autonomyLevel: "banana" as never }).autonomyLevel).toBe(DEFAULT_TENANT_CONFIG.autonomyLevel); + }); + + it("merges a partial preferences override onto the defaults", () => { + const cfg = resolveTenantConfig({ preferences: { maxConcurrentLoops: 5 } }); + expect(cfg.preferences.maxConcurrentLoops).toBe(5); + expect(cfg.preferences.pauseOnFailure).toBe(DEFAULT_TENANT_CONFIG.preferences.pauseOnFailure); + expect(cfg.preferences.allowedActionClasses).toEqual(DEFAULT_TENANT_CONFIG.preferences.allowedActionClasses); + }); + + it("honors an explicit false pauseOnFailure (not treated as absent) and a custom action-class list", () => { + const cfg = resolveTenantConfig({ preferences: { pauseOnFailure: false, allowedActionClasses: ["comment"] } }); + expect(cfg.preferences.pauseOnFailure).toBe(false); + expect(cfg.preferences.allowedActionClasses).toEqual(["comment"]); + }); +}); + +describe("tenant config store (#4787)", () => { + it("setTenantConfig returns a NEW store and never mutates the input (immutable update)", () => { + const s0 = EMPTY_TENANT_CONFIG_STORE; + const s1 = setTenantConfig(s0, "acme", { autonomyLevel: "auto" }); + expect(s1).not.toBe(s0); + expect(s0).toEqual({}); // input untouched + expect(getTenantConfig(s1, "acme").autonomyLevel).toBe("auto"); + }); + + it("getTenantConfig returns the defaults for a tenant that has set nothing", () => { + expect(getTenantConfig(EMPTY_TENANT_CONFIG_STORE, "unknown")).toEqual(DEFAULT_TENANT_CONFIG); + }); + + it("two tenants hold independent configs with no cross-contamination (acceptance)", () => { + let store = EMPTY_TENANT_CONFIG_STORE; + store = setTenantConfig(store, "tenant-a", { autonomyLevel: "auto", preferences: { allowedActionClasses: ["open_pr"] } }); + store = setTenantConfig(store, "tenant-b", { autonomyLevel: "off" }); + const a = getTenantConfig(store, "tenant-a"); + const b = getTenantConfig(store, "tenant-b"); + expect(a.autonomyLevel).toBe("auto"); + expect(b.autonomyLevel).toBe("off"); + // Mutating tenant A's resolved list must not affect tenant B or the defaults. + (a.preferences.allowedActionClasses as string[]).push("delete_repo"); + expect(getTenantConfig(store, "tenant-b").preferences.allowedActionClasses).not.toContain("delete_repo"); + 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); + }); +});