Skip to content

Commit f116abf

Browse files
author
RealDiligent
committed
fix(engine): stop parseAmsPolicySpec aliasing the frozen DEFAULT_AMS_POLICY_SPEC sub-objects
DEFAULT_AMS_POLICY_SPEC is a deep-frozen shared singleton, but normalizeCapLimits and normalizeConvergenceThresholds returned their fallback (DEFAULT_AMS_POLICY_SPEC.capLimits / .convergenceThresholds) BY REFERENCE on the undefined/null and not-a-mapping paths. So a parsed spec that carries at least one configured field — and therefore returns the normalized spec rather than the all-defaults clone — aliased the frozen singleton into a caller's mutable spec, exactly the hazard the singleton's own doc comment ("clone before layering overrides") warns about. Return { ...fallback } on both fallback paths of each normalizer, mirroring how normalizeNetworkAllowlist / normalizeEcosystemList already copy their fallback. All parsed VALUES are byte-identical; DEFAULT_AMS_POLICY_SPEC and its sub-objects stay frozen. Closes #9995
1 parent a7673e2 commit f116abf

2 files changed

Lines changed: 51 additions & 4 deletions

File tree

packages/loopover-engine/src/ams-policy-spec.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -208,10 +208,13 @@ function normalizeNonNegativeInteger(value: unknown, field: string, fallback: nu
208208
}
209209

210210
function normalizeCapLimits(value: unknown, fallback: AmsCapLimits, warnings: string[]): AmsCapLimits {
211-
if (value === undefined || value === null) return fallback;
211+
// Fresh copy on the fallback paths too (same reasoning as normalizeNetworkAllowlist): `fallback` is the
212+
// deep-frozen DEFAULT_AMS_POLICY_SPEC.capLimits, so returning it by reference would alias the shared
213+
// singleton into a caller's parsed spec.
214+
if (value === undefined || value === null) return { ...fallback };
212215
if (typeof value !== "object" || Array.isArray(value)) {
213216
warnings.push('AmsPolicySpec field "capLimits" must be a mapping; falling back to defaults.');
214-
return fallback;
217+
return { ...fallback };
215218
}
216219
const record = value as Record<string, unknown>;
217220
return {
@@ -226,10 +229,12 @@ function normalizeConvergenceThresholds(
226229
fallback: PortfolioConvergenceThresholds,
227230
warnings: string[],
228231
): PortfolioConvergenceThresholds {
229-
if (value === undefined || value === null) return fallback;
232+
// Fresh copy on the fallback paths too: `fallback` is the deep-frozen
233+
// DEFAULT_AMS_POLICY_SPEC.convergenceThresholds, so returning it by reference would alias the shared singleton.
234+
if (value === undefined || value === null) return { ...fallback };
230235
if (typeof value !== "object" || Array.isArray(value)) {
231236
warnings.push('AmsPolicySpec field "convergenceThresholds" must be a mapping; falling back to defaults.');
232-
return fallback;
237+
return { ...fallback };
233238
}
234239
const record = value as Record<string, unknown>;
235240
return {

packages/loopover-engine/test/ams-policy-spec-parser.test.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,3 +189,45 @@ test("parseAmsPolicySpecContent: JSON and YAML both parse, malformed content deg
189189
assert.equal(oversized.present, false);
190190
assert.match(oversized.warnings.join(" "), /exceeded/i);
191191
});
192+
193+
test("REGRESSION (#9995): parsed capLimits/convergenceThresholds are fresh copies, never aliased to the frozen DEFAULT singleton", () => {
194+
const parsed = parseAmsPolicySpec({ submissionMode: "enforce" }).spec;
195+
// VALUES are unchanged...
196+
assert.deepEqual(parsed.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits);
197+
assert.deepEqual(parsed.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds);
198+
// ...but the objects are distinct, not the shared frozen singleton returned by reference.
199+
assert.notStrictEqual(parsed.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits);
200+
assert.notStrictEqual(parsed.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds);
201+
// The returned sub-objects are writable, and mutating them cannot corrupt the shared DEFAULT.
202+
assert.equal(Object.isFrozen(parsed.capLimits), false);
203+
assert.equal(Object.isFrozen(parsed.convergenceThresholds), false);
204+
parsed.capLimits.budget = 999;
205+
parsed.convergenceThresholds.maxReenqueues = 999;
206+
assert.notEqual(DEFAULT_AMS_POLICY_SPEC.capLimits.budget, 999);
207+
assert.notEqual(DEFAULT_AMS_POLICY_SPEC.convergenceThresholds.maxReenqueues, 999);
208+
});
209+
210+
test("REGRESSION (#9995): the not-a-mapping fallback paths also return fresh, non-aliased copies", () => {
211+
const parsed = parseAmsPolicySpec({ capLimits: "nope", convergenceThresholds: [] }).spec;
212+
assert.deepEqual(parsed.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits);
213+
assert.deepEqual(parsed.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds);
214+
assert.notStrictEqual(parsed.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits);
215+
assert.notStrictEqual(parsed.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds);
216+
217+
// With ANOTHER field configured (so the parser returns the normalized spec rather than the all-defaults
218+
// clone), the not-a-mapping fallback is the path that actually reaches the caller — it must be fresh too.
219+
const configured = parseAmsPolicySpec({ submissionMode: "enforce", capLimits: "nope", convergenceThresholds: [] }).spec;
220+
assert.deepEqual(configured.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits);
221+
assert.deepEqual(configured.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds);
222+
assert.notStrictEqual(configured.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits);
223+
assert.notStrictEqual(configured.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds);
224+
assert.equal(Object.isFrozen(configured.capLimits), false);
225+
});
226+
227+
test("DEFAULT_AMS_POLICY_SPEC and its three sub-objects remain deep-frozen after parsing (#9995)", () => {
228+
parseAmsPolicySpec({ submissionMode: "enforce" });
229+
assert.equal(Object.isFrozen(DEFAULT_AMS_POLICY_SPEC), true);
230+
assert.equal(Object.isFrozen(DEFAULT_AMS_POLICY_SPEC.capLimits), true);
231+
assert.equal(Object.isFrozen(DEFAULT_AMS_POLICY_SPEC.convergenceThresholds), true);
232+
assert.equal(Object.isFrozen(DEFAULT_AMS_POLICY_SPEC.networkAllowlist), true);
233+
});

0 commit comments

Comments
 (0)