You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.
Context
packages/loopover-engine/src/tenant-config.ts states its whole purpose in its header:
"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"
Two gaps between that claim and the code:
1. The read path leaks the stored object.getTenantConfig is return store[tenantId] ?? resolveTenantConfig();. Its own JSDoc says "falling back to a fresh copy
of the defaults when they've set none" — only the miss path is a fresh copy. On a hit it returns
the very object held in the store. setTenantConfig freezes the store with Object.freeze, which is
shallow: the TenantConfig values and their nested preferences object are not frozen. So:
A caller that reads a config and mutates it silently rewrites that tenant's stored config — including autonomyLevel and allowedActionClasses, the two fields that decide what the tenant's loop is
permitted to do. The asymmetry is sharp: on a tenant with no config set, the identical caller code has
no effect at all, because that path really does return a fresh object.
2. maxConcurrentLoops is the one numeric field in the Rent-a-Loop family with no normalization. resolveTenantConfig does prefs.maxConcurrentLoops ?? base.preferences.maxConcurrentLoops, so -5, 0.5, NaN and Infinity all pass through verbatim (?? does not catch NaN). Every sibling module
in this same #4778 family normalizes exactly this kind of value with the same three-line helper: packages/loopover-engine/src/tenant-quota.ts:45-47, packages/loopover-engine/src/loop-consumption.ts:56-58, and packages/loopover-engine/src/miner/worktree-pool.ts:49-51 — whose comment even names the discipline: "Mirrors the finiteNonNegativeInt discipline already applied in governor/rate-limit.ts,
governor/budget-cap.ts, and tenant-quota.ts (#5828)."
Requirements
getTenantConfig MUST return a config that shares no mutable reference with the store on both
paths. Mutating the returned object (its autonomyLevel, its preferences, or its allowedActionClasses array) MUST leave a subsequent getTenantConfig for the same tenant
unchanged.
setTenantConfig MUST deep-freeze the entry it writes: the TenantConfig, its preferences, and
its allowedActionClasses array. The returned store MUST still be Object.freezed as today.
resolveTenantConfig MUST normalize maxConcurrentLoops with the exact finiteNonNegativeInt body
used in packages/loopover-engine/src/tenant-quota.ts:45-47
(Number.isFinite(value) ? Math.max(0, Math.floor(value)) : 0), applied to an explicitly-supplied
override only — an absent override still inherits DEFAULT_TENANT_CONFIG's 1.
resolveTenantConfig MUST drop non-string entries from an allowedActionClasses override rather
than copying them through, matching how it already refuses an unrecognized autonomyLevel.
DEFAULT_TENANT_CONFIG, TENANT_AUTONOMY_LEVELS, EMPTY_TENANT_CONFIG_STORE, and every exported
type MUST be unchanged.
getTenantConfig's JSDoc MUST be corrected so it no longer implies only the fallback path is fresh.
⚠️ Required pattern: use the exact finiteNonNegativeInt body from packages/loopover-engine/src/tenant-quota.ts:45-47, and keep the copy-on-read shape the module
already uses for allowedActionClasses ([...(...)], line 57). What does NOT satisfy this issue:
switching the store to a Map or a class; introducing a structured-clone/deep-copy utility; making getTenantConfig return readonly/Readonly<...> types instead of an actually-independent object
(a type-level change does not stop a JS caller mutating it); or normalizing maxConcurrentLoops at
the evaluateTenantQuota call site instead of in resolveTenantConfig.
Deliverables
A new named regression test asserting that mutating the result of getTenantConfig(store, "acme")
— pushing onto preferences.allowedActionClasses and reassigning autonomyLevel — leaves a
second getTenantConfig(store, "acme") deep-equal to the original.
A new test asserting the same for two successive getTenantConfig calls on a tenant with no
stored config (the fallback path), so both arms are covered.
Object.isFrozen(getTenantConfig(store, "acme").preferences.allowedActionClasses) is true for
a stored tenant, asserted by a new test.
resolveTenantConfig({ preferences: { maxConcurrentLoops: -5 } }).preferences.maxConcurrentLoops === 0,
and the same for Number.NaN, Number.POSITIVE_INFINITY, and 2.9 (-> 2), asserted by new
test cases.
resolveTenantConfig({}).preferences.maxConcurrentLoops === 1 (the default is still inherited,
not normalized to 0), asserted by a test.
resolveTenantConfig({ preferences: { allowedActionClasses: ["open_pr", 7, null] as never } })
yields ["open_pr"], asserted by a new test.
getTenantConfig's JSDoc describes both paths as returning an independent config.
All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
deep-freezing on write without also making getTenantConfig return an independent object, so a caller
gets a silent TypeError in strict mode instead of correct isolation — does not resolve this issue.
Test Coverage Requirements
packages/loopover-engine/src/**/*.tsis inside coverage.include in vitest.config.ts and
carries its own engine Codecov flag; the 99%+ branch-counted codecov/patch gate applies here in
full. Both arms of every changed conditional need a test: getTenantConfig hit and miss; finiteNonNegativeInt with a finite, non-finite, negative, and fractional input; the maxConcurrentLoops override present and absent; and the allowedActionClasses filter with an
all-string array and a mixed array. The mutation-isolation test is the required named regression test.
Expected Outcome
Reading a tenant's config can no longer rewrite it, the "IMMUTABLE store" claim in the module header
is actually enforced rather than asserted, and maxConcurrentLoops cannot be negative, fractional, or NaN — matching every other numeric input in the same Rent-a-Loop module family.
Links & Resources
packages/loopover-engine/src/tenant-config.ts (whole file; lines 62-83 for the store)
Context
packages/loopover-engine/src/tenant-config.tsstates its whole purpose in its header:Two gaps between that claim and the code:
1. The read path leaks the stored object.
getTenantConfigisreturn store[tenantId] ?? resolveTenantConfig();. Its own JSDoc says "falling back to a fresh copyof the defaults when they've set none" — only the miss path is a fresh copy. On a hit it returns
the very object held in the store.
setTenantConfigfreezes the store withObject.freeze, which isshallow: the
TenantConfigvalues and their nestedpreferencesobject are not frozen. So:A caller that reads a config and mutates it silently rewrites that tenant's stored config — including
autonomyLevelandallowedActionClasses, the two fields that decide what the tenant's loop ispermitted to do. The asymmetry is sharp: on a tenant with no config set, the identical caller code has
no effect at all, because that path really does return a fresh object.
2.
maxConcurrentLoopsis the one numeric field in the Rent-a-Loop family with no normalization.resolveTenantConfigdoesprefs.maxConcurrentLoops ?? base.preferences.maxConcurrentLoops, so-5,0.5,NaNandInfinityall pass through verbatim (??does not catchNaN). Every sibling modulein this same #4778 family normalizes exactly this kind of value with the same three-line helper:
packages/loopover-engine/src/tenant-quota.ts:45-47,packages/loopover-engine/src/loop-consumption.ts:56-58, andpackages/loopover-engine/src/miner/worktree-pool.ts:49-51— whose comment even names the discipline:"Mirrors the
finiteNonNegativeIntdiscipline already applied in governor/rate-limit.ts,governor/budget-cap.ts, and tenant-quota.ts (#5828)."
Requirements
getTenantConfigMUST return a config that shares no mutable reference with the store on bothpaths. Mutating the returned object (its
autonomyLevel, itspreferences, or itsallowedActionClassesarray) MUST leave a subsequentgetTenantConfigfor the same tenantunchanged.
setTenantConfigMUST deep-freeze the entry it writes: theTenantConfig, itspreferences, andits
allowedActionClassesarray. The returned store MUST still beObject.freezed as today.resolveTenantConfigMUST normalizemaxConcurrentLoopswith the exactfiniteNonNegativeIntbodyused in
packages/loopover-engine/src/tenant-quota.ts:45-47(
Number.isFinite(value) ? Math.max(0, Math.floor(value)) : 0), applied to an explicitly-suppliedoverride only — an absent override still inherits
DEFAULT_TENANT_CONFIG's1.resolveTenantConfigMUST drop non-string entries from anallowedActionClassesoverride ratherthan copying them through, matching how it already refuses an unrecognized
autonomyLevel.DEFAULT_TENANT_CONFIG,TENANT_AUTONOMY_LEVELS,EMPTY_TENANT_CONFIG_STORE, and every exportedtype MUST be unchanged.
getTenantConfig's JSDoc MUST be corrected so it no longer implies only the fallback path is fresh.Deliverables
getTenantConfig(store, "acme")— pushing onto
preferences.allowedActionClassesand reassigningautonomyLevel— leaves asecond
getTenantConfig(store, "acme")deep-equal to the original.getTenantConfigcalls on a tenant with nostored config (the fallback path), so both arms are covered.
Object.isFrozen(getTenantConfig(store, "acme").preferences.allowedActionClasses)istruefora stored tenant, asserted by a new test.
resolveTenantConfig({ preferences: { maxConcurrentLoops: -5 } }).preferences.maxConcurrentLoops === 0,and the same for
Number.NaN,Number.POSITIVE_INFINITY, and2.9(->2), asserted by newtest cases.
resolveTenantConfig({}).preferences.maxConcurrentLoops === 1(the default is still inherited,not normalized to 0), asserted by a test.
resolveTenantConfig({ preferences: { allowedActionClasses: ["open_pr", 7, null] as never } })yields
["open_pr"], asserted by a new test.getTenantConfig's JSDoc describes both paths as returning an independent config.All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
deep-freezing on write without also making
getTenantConfigreturn an independent object, so a callergets a silent
TypeErrorin strict mode instead of correct isolation — does not resolve this issue.Test Coverage Requirements
packages/loopover-engine/src/**/*.tsis insidecoverage.includeinvitest.config.tsandcarries its own
engineCodecov flag; the 99%+ branch-countedcodecov/patchgate applies here infull. Both arms of every changed conditional need a test:
getTenantConfighit and miss;finiteNonNegativeIntwith a finite, non-finite, negative, and fractional input; themaxConcurrentLoopsoverride present and absent; and theallowedActionClassesfilter with anall-string array and a mixed array. The mutation-isolation test is the required named regression test.
Expected Outcome
Reading a tenant's config can no longer rewrite it, the "IMMUTABLE store" claim in the module header
is actually enforced rather than asserted, and
maxConcurrentLoopscannot be negative, fractional, orNaN— matching every other numeric input in the same Rent-a-Loop module family.Links & Resources
packages/loopover-engine/src/tenant-config.ts(whole file; lines 62-83 for the store)packages/loopover-engine/src/tenant-quota.ts:43-47(finiteNonNegativeIntprecedent)packages/loopover-engine/src/loop-consumption.ts:54-58,packages/loopover-engine/src/miner/worktree-pool.ts:44-51(the same discipline, fix(engine): worktree-pool.ts doesn't normalize maxConcurrency, NaN disables the cap #5828)