fix(engine): stop parseAmsPolicySpec aliasing the frozen DEFAULT_AMS_POLICY_SPEC sub-objects - #10069
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-31 06:33:04 UTC
Review summary Nits — 2 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
…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 JSONbored#9995
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10069 +/- ##
==========================================
- Coverage 91.88% 91.88% -0.01%
==========================================
Files 930 930
Lines 113827 113832 +5
Branches 27466 27466
==========================================
+ Hits 104588 104589 +1
Misses 7940 7940
- Partials 1299 1303 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
DEFAULT_AMS_POLICY_SPEC(packages/loopover-engine/src/ams-policy-spec.ts) is a deep-frozen shared singleton, and its own doc says "clone before layering overrides on top." ButnormalizeCapLimitsandnormalizeConvergenceThresholdsreturned theirfallback— whichparseAmsPolicySpecpasses asDEFAULT_AMS_POLICY_SPEC.capLimits/.convergenceThresholds— by reference on theundefined/nulland not-a-mapping paths (only the valid-mapping path built a fresh object).So a parsed spec that carries at least one configured field (and therefore returns the normalized spec at line 389, not the all-defaults
cloneDefaultAmsPolicySpec()at line 387) aliased the frozen singleton's sub-objects into a caller's mutablespec— the exact hazard the singleton's doc warns about.Fix
Return
{ ...fallback }on both fallback paths of each normalizer (a shallow copy fully de-aliases since every field is a primitive), mirroring hownormalizeNetworkAllowlist/normalizeEcosystemListalready copy their fallback on every return path. No production values change;DEFAULT_AMS_POLICY_SPECand its sub-objects stay frozen.Tests
Added to
packages/loopover-engine/test/ams-policy-spec-parser.test.ts:capLimits/convergenceThresholdsaredeepEqualto the defaults butnotStrictEqual(distinct objects), are not frozen, and mutating them cannot corruptDEFAULT_AMS_POLICY_SPEC.{ capLimits: "nope", convergenceThresholds: [] }, and the configured-field variant that actually returns the normalized spec) also yield fresh, non-aliased copies.DEFAULT_AMS_POLICY_SPECand all three of its sub-objects remainObject.isFrozen.The two regression tests fail against the current by-reference code; the existing value assertions pass unchanged. Engine-flag coverage of the four changed lines verified (source-mapped lcov).
Closes #9995