Address PR #1602 review: validate band upstream in _vrt.read_vrt

- Move the ``band`` range/type check into the internal ``_vrt.read_vrt``
  reader where ``len(vrt.bands)`` is already in scope. Reject negative
  indices, non-int values, and out-of-range positive values with a
  clear ``ValueError`` that names the available band count. Previously
  these would either silently return a different band (Python negative
  indexing), raise an opaque ``IndexError`` deep in the read path, or
  raise ``TypeError`` on the list index.
- Drop the now-redundant post-parse guard in the public ``read_vrt``
  and update the comment to reflect that ``band`` is validated upstream.
- Add three regression tests covering the negative, out-of-range, and
  non-int rejection paths.

The original bug from #1598 (band=N reads pairing with band 0's nodata
sentinel) was masked by these silent acceptances on the negative-band
edge: ``read_vrt(path, band=-1)`` previously returned the last band's
data with ``attrs['nodata']`` unset entirely.

Author: brendancol

xrspatial/geotiff/__init__.py

@@ -2738,16 +2738,14 @@ def read_vrt(source: str, *, dtype=None, window=None,
     # advertise band 0's sentinel, the integer-promotion block below
     # would mask against band 0's sentinel, and band N's actual nodata
     # pixels would survive as literal integers. See issue #1598.
+    # ``band`` has already been validated by ``_vrt.read_vrt`` as
+    # 0 <= band < len(vrt.bands), so a simple lookup is safe here.
     nodata = None
     if vrt.bands:
         band_idx_for_nodata = band if band is not None else 0
-        # ``_vrt.read_vrt`` already validates ``band`` is in range; the
-        # extra guard keeps a clearer message if a future refactor
-        # widens the public range without updating the internal reader.
-        if 0 <= band_idx_for_nodata < len(vrt.bands):
-            nodata = vrt.bands[band_idx_for_nodata].nodata
-            if nodata is not None:
-                attrs['nodata'] = nodata
+        nodata = vrt.bands[band_idx_for_nodata].nodata
+        if nodata is not None:
+            attrs['nodata'] = nodata
 
     # Mirror the integer-with-nodata promotion that open_geotiff /
     # read_geotiff_dask / read_geotiff_gpu apply post-decode. The VRT

xrspatial/geotiff/_vrt.py

@@ -242,6 +242,23 @@ def read_vrt(vrt_path: str, *, window=None,
     vrt_dir = os.path.dirname(os.path.abspath(vrt_path))
     vrt = parse_vrt(xml_str, vrt_dir)
 
+    # Validate ``band`` against the parsed band count. Python list
+    # indexing would silently accept negative values (``vrt.bands[-1]``
+    # returns the last band) and raise an opaque ``IndexError`` for
+    # out-of-range positive values, neither of which is the contract the
+    # public API documents (``band`` is a 0-based positive index). Reject
+    # both up front with a clear ``ValueError`` so callers do not get
+    # band-N data paired with band-0's nodata sentinel or a downstream
+    # IndexError from deep in the read path.
+    if band is not None:
+        if not isinstance(band, (int, np.integer)) or isinstance(band, bool):
+            raise ValueError(
+                f"band must be a non-negative int, got {band!r}")
+        if band < 0 or band >= len(vrt.bands):
+            raise ValueError(
+                f"band index {band} out of range for VRT with "
+                f"{len(vrt.bands)} band(s)")
+
     if window is not None:
         r0, c0, r1, c1 = window
         r0 = max(0, r0)

xrspatial/geotiff/tests/test_vrt_band_nodata_1598.py

@@ -15,6 +15,7 @@
 from __future__ import annotations
 
 import numpy as np
+import pytest
 
 from xrspatial.geotiff import read_vrt
 from xrspatial.geotiff._writer import write
@@ -105,3 +106,36 @@ def test_read_vrt_no_band_keeps_band0_nodata_attr(tmp_path):
     vrt_path = _write_two_band_per_band_nodata_vrt(tmp_path)
     r = read_vrt(vrt_path)
     assert r.attrs.get('nodata') == 65535.0
+
+
+def test_read_vrt_negative_band_raises(tmp_path):
+    """Negative band indices used to be silently accepted via Python
+    list indexing (``vrt.bands[-1]`` returned the last band) while the
+    public reader's nodata lookup rejected them, producing band-N data
+    with no nodata sentinel. They are now a clear ValueError up front.
+    """
+    vrt_path = _write_two_band_per_band_nodata_vrt(tmp_path)
+    with pytest.raises(ValueError, match="band"):
+        read_vrt(vrt_path, band=-1)
+
+
+def test_read_vrt_out_of_range_band_raises(tmp_path):
+    """Out-of-range band indices used to raise IndexError from deep in
+    the read path. They are now a ValueError that names the available
+    band count.
+    """
+    vrt_path = _write_two_band_per_band_nodata_vrt(tmp_path)
+    with pytest.raises(ValueError, match="out of range"):
+        read_vrt(vrt_path, band=5)
+
+
+def test_read_vrt_non_integer_band_raises(tmp_path):
+    """A non-int ``band`` would previously have raised TypeError on the
+    list index. ValueError here matches the rest of the input
+    validation surface.
+    """
+    vrt_path = _write_two_band_per_band_nodata_vrt(tmp_path)
+    with pytest.raises(ValueError, match="band"):
+        read_vrt(vrt_path, band="1")
+    with pytest.raises(ValueError, match="band"):
+        read_vrt(vrt_path, band=True)