From f116abfae4bc08cdf94d5ecbb2446e0ea5ba1e06 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 31 Jul 2026 14:14:24 +0800 Subject: [PATCH] fix(engine): stop parseAmsPolicySpec aliasing the frozen DEFAULT_AMS_POLICY_SPEC sub-objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../loopover-engine/src/ams-policy-spec.ts | 13 ++++-- .../test/ams-policy-spec-parser.test.ts | 42 +++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/packages/loopover-engine/src/ams-policy-spec.ts b/packages/loopover-engine/src/ams-policy-spec.ts index 0bd5b27062..59218f2a78 100644 --- a/packages/loopover-engine/src/ams-policy-spec.ts +++ b/packages/loopover-engine/src/ams-policy-spec.ts @@ -208,10 +208,13 @@ function normalizeNonNegativeInteger(value: unknown, field: string, fallback: nu } function normalizeCapLimits(value: unknown, fallback: AmsCapLimits, warnings: string[]): AmsCapLimits { - if (value === undefined || value === null) return fallback; + // Fresh copy on the fallback paths too (same reasoning as normalizeNetworkAllowlist): `fallback` is the + // deep-frozen DEFAULT_AMS_POLICY_SPEC.capLimits, so returning it by reference would alias the shared + // singleton into a caller's parsed spec. + if (value === undefined || value === null) return { ...fallback }; if (typeof value !== "object" || Array.isArray(value)) { warnings.push('AmsPolicySpec field "capLimits" must be a mapping; falling back to defaults.'); - return fallback; + return { ...fallback }; } const record = value as Record; return { @@ -226,10 +229,12 @@ function normalizeConvergenceThresholds( fallback: PortfolioConvergenceThresholds, warnings: string[], ): PortfolioConvergenceThresholds { - if (value === undefined || value === null) return fallback; + // Fresh copy on the fallback paths too: `fallback` is the deep-frozen + // DEFAULT_AMS_POLICY_SPEC.convergenceThresholds, so returning it by reference would alias the shared singleton. + if (value === undefined || value === null) return { ...fallback }; if (typeof value !== "object" || Array.isArray(value)) { warnings.push('AmsPolicySpec field "convergenceThresholds" must be a mapping; falling back to defaults.'); - return fallback; + return { ...fallback }; } const record = value as Record; return { diff --git a/packages/loopover-engine/test/ams-policy-spec-parser.test.ts b/packages/loopover-engine/test/ams-policy-spec-parser.test.ts index 4de08f1c98..2f3337dd85 100644 --- a/packages/loopover-engine/test/ams-policy-spec-parser.test.ts +++ b/packages/loopover-engine/test/ams-policy-spec-parser.test.ts @@ -189,3 +189,45 @@ test("parseAmsPolicySpecContent: JSON and YAML both parse, malformed content deg assert.equal(oversized.present, false); assert.match(oversized.warnings.join(" "), /exceeded/i); }); + +test("REGRESSION (#9995): parsed capLimits/convergenceThresholds are fresh copies, never aliased to the frozen DEFAULT singleton", () => { + const parsed = parseAmsPolicySpec({ submissionMode: "enforce" }).spec; + // VALUES are unchanged... + assert.deepEqual(parsed.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits); + assert.deepEqual(parsed.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds); + // ...but the objects are distinct, not the shared frozen singleton returned by reference. + assert.notStrictEqual(parsed.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits); + assert.notStrictEqual(parsed.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds); + // The returned sub-objects are writable, and mutating them cannot corrupt the shared DEFAULT. + assert.equal(Object.isFrozen(parsed.capLimits), false); + assert.equal(Object.isFrozen(parsed.convergenceThresholds), false); + parsed.capLimits.budget = 999; + parsed.convergenceThresholds.maxReenqueues = 999; + assert.notEqual(DEFAULT_AMS_POLICY_SPEC.capLimits.budget, 999); + assert.notEqual(DEFAULT_AMS_POLICY_SPEC.convergenceThresholds.maxReenqueues, 999); +}); + +test("REGRESSION (#9995): the not-a-mapping fallback paths also return fresh, non-aliased copies", () => { + const parsed = parseAmsPolicySpec({ capLimits: "nope", convergenceThresholds: [] }).spec; + assert.deepEqual(parsed.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits); + assert.deepEqual(parsed.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds); + assert.notStrictEqual(parsed.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits); + assert.notStrictEqual(parsed.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds); + + // With ANOTHER field configured (so the parser returns the normalized spec rather than the all-defaults + // clone), the not-a-mapping fallback is the path that actually reaches the caller — it must be fresh too. + const configured = parseAmsPolicySpec({ submissionMode: "enforce", capLimits: "nope", convergenceThresholds: [] }).spec; + assert.deepEqual(configured.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits); + assert.deepEqual(configured.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds); + assert.notStrictEqual(configured.capLimits, DEFAULT_AMS_POLICY_SPEC.capLimits); + assert.notStrictEqual(configured.convergenceThresholds, DEFAULT_AMS_POLICY_SPEC.convergenceThresholds); + assert.equal(Object.isFrozen(configured.capLimits), false); +}); + +test("DEFAULT_AMS_POLICY_SPEC and its three sub-objects remain deep-frozen after parsing (#9995)", () => { + parseAmsPolicySpec({ submissionMode: "enforce" }); + assert.equal(Object.isFrozen(DEFAULT_AMS_POLICY_SPEC), true); + assert.equal(Object.isFrozen(DEFAULT_AMS_POLICY_SPEC.capLimits), true); + assert.equal(Object.isFrozen(DEFAULT_AMS_POLICY_SPEC.convergenceThresholds), true); + assert.equal(Object.isFrozen(DEFAULT_AMS_POLICY_SPEC.networkAllowlist), true); +});