Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kai392 wants to merge 1 commit into from
+234 −153
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
204 changes: 121 additions & 83 deletions packages/loopover-engine/src/tenant-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<TenantExecutionPreferences> | 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<Record<string, TenantConfig>>;

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<TenantExecutionPreferences> | 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<Record<string, TenantConfig>>;

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();
}
183 changes: 113 additions & 70 deletions test/unit/tenant-config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
});
});
Loading