BSP: add safety features and API improvements (closes #202, #203, #204, #205, #206)

Safety improvements:
- Generation counter detects stale BSPNode references after clear()/split_recursive()
- GRID_MAX validation prevents oversized BSP trees
- Depth parameter capped at 16 to prevent resource exhaustion
- Iterator checks generation to detect invalidation during mutation

API improvements:
- Changed constructor from bounds=((x,y),(w,h)) to pos=(x,y), size=(w,h)
- Added pos and size properties alongside bounds
- BSPNode __eq__ compares underlying pointers for identity
- BSP __iter__ as shorthand for leaves()
- BSP __len__ returns leaf count

Tests:
- Added tests for stale node detection, GRID_MAX validation, depth cap
- Added tests for __len__, __iter__, and BSPNode equality

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

src/PyBSP.cpp

@@ -85,11 +85,10 @@ PyObject* PyTraversal::create_enum_class(PyObject* module) {
         return NULL;
     }
 
-    // Cache reference
+    // Cache reference (borrowed by module after AddObject)
     traversal_enum_class = traversal_class;
-    Py_INCREF(traversal_enum_class);
 
-    // Add to module
+    // Add to module (steals reference)
     if (PyModule_AddObject(module, "Traversal", traversal_class) < 0) {
         Py_DECREF(traversal_class);
         traversal_enum_class = nullptr;
@@ -99,6 +98,12 @@ PyObject* PyTraversal::create_enum_class(PyObject* module) {
     return traversal_class;
 }
 
+void PyTraversal::cleanup() {
+    // The enum class is owned by the module after PyModule_AddObject
+    // We just clear our pointer; the module handles cleanup
+    traversal_enum_class = nullptr;
+}
+
 int PyTraversal::from_arg(PyObject* arg, int* out_order) {
     // Accept None -> default to LEVEL_ORDER
     if (arg == NULL || arg == Py_None) {
@@ -182,29 +187,41 @@ int PyTraversal::from_arg(PyObject* arg, int* out_order) {
 PyGetSetDef PyBSP::getsetters[] = {
     {"bounds", (getter)PyBSP::get_bounds, NULL,
      MCRF_PROPERTY(bounds, "Root node bounds as ((x, y), (w, h)). Read-only."), NULL},
+    {"pos", (getter)PyBSP::get_pos, NULL,
+     MCRF_PROPERTY(pos, "Top-left position (x, y). Read-only."), NULL},
+    {"size", (getter)PyBSP::get_size, NULL,
+     MCRF_PROPERTY(size, "Dimensions (width, height). Read-only."), NULL},
     {"root", (getter)PyBSP::get_root, NULL,
      MCRF_PROPERTY(root, "Reference to the root BSPNode. Read-only."), NULL},
     {NULL}
 };
 
+// Sequence methods for len() and iteration
+PySequenceMethods PyBSP::sequence_methods = {
+    .sq_length = (lenfunc)PyBSP::len,
+};
+
 // ==================== BSP Method Definitions ====================
 
 PyMethodDef PyBSP::methods[] = {
     {"split_once", (PyCFunction)PyBSP::split_once, METH_VARARGS | METH_KEYWORDS,
      MCRF_METHOD(BSP, split_once,
          MCRF_SIG("(horizontal: bool, position: int)", "BSP"),
-         MCRF_DESC("Split the root node once at the specified position."),
+         MCRF_DESC("Split the root node once at the specified position. "
+                   "horizontal=True creates a horizontal divider, producing top/bottom rooms. "
+                   "horizontal=False creates a vertical divider, producing left/right rooms."),
          MCRF_ARGS_START
-         MCRF_ARG("horizontal", "True for horizontal split, False for vertical")
+         MCRF_ARG("horizontal", "True for horizontal divider (top/bottom), False for vertical (left/right)")
          MCRF_ARG("position", "Split coordinate (y for horizontal, x for vertical)")
          MCRF_RETURNS("BSP: self, for method chaining")
      )},
     {"split_recursive", (PyCFunction)PyBSP::split_recursive, METH_VARARGS | METH_KEYWORDS,
      MCRF_METHOD(BSP, split_recursive,
          MCRF_SIG("(depth: int, min_size: tuple[int, int], max_ratio: float = 1.5, seed: int = None)", "BSP"),
-         MCRF_DESC("Recursively split to the specified depth."),
+         MCRF_DESC("Recursively split to the specified depth. "
+                   "WARNING: Invalidates all existing BSPNode references from this tree."),
          MCRF_ARGS_START
-         MCRF_ARG("depth", "Maximum recursion depth. Creates up to 2^depth leaves.")
+         MCRF_ARG("depth", "Maximum recursion depth (1-16). Creates up to 2^depth leaves.")
          MCRF_ARG("min_size", "Minimum (width, height) for a node to be split.")
          MCRF_ARG("max_ratio", "Maximum aspect ratio before forcing split direction. Default: 1.5.")
          MCRF_ARG("seed", "Random seed. None for random.")
@@ -213,13 +230,14 @@ PyMethodDef PyBSP::methods[] = {
     {"clear", (PyCFunction)PyBSP::clear, METH_NOARGS,
      MCRF_METHOD(BSP, clear,
          MCRF_SIG("()", "BSP"),
-         MCRF_DESC("Remove all children, keeping only the root node with original bounds."),
+         MCRF_DESC("Remove all children, keeping only the root node with original bounds. "
+                   "WARNING: Invalidates all existing BSPNode references from this tree."),
          MCRF_RETURNS("BSP: self, for method chaining")
      )},
     {"leaves", (PyCFunction)PyBSP::leaves, METH_NOARGS,
      MCRF_METHOD(BSP, leaves,
          MCRF_SIG("()", "Iterator[BSPNode]"),
-         MCRF_DESC("Iterate all leaf nodes (the actual rooms)."),
+         MCRF_DESC("Iterate all leaf nodes (the actual rooms). Same as iterating the BSP directly."),
          MCRF_RETURNS("Iterator yielding BSPNode objects")
      )},
     {"traverse", (PyCFunction)PyBSP::traverse, METH_VARARGS | METH_KEYWORDS,
@@ -264,41 +282,33 @@ PyObject* PyBSP::pynew(PyTypeObject* type, PyObject* args, PyObject* kwds)
         self->orig_y = 0;
         self->orig_w = 0;
         self->orig_h = 0;
+        self->generation = 0;
     }
     return (PyObject*)self;
 }
 
 int PyBSP::init(PyBSPObject* self, PyObject* args, PyObject* kwds)
 {
-    static const char* keywords[] = {"bounds", nullptr};
+    static const char* keywords[] = {"pos", "size", nullptr};
-    PyObject* bounds_obj = nullptr;
+    PyObject* pos_obj = nullptr;
+    PyObject* size_obj = nullptr;
 
-    if (!PyArg_ParseTupleAndKeywords(args, kwds, "O", const_cast<char**>(keywords),
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "OO", const_cast<char**>(keywords),
-                                     &bounds_obj)) {
+                                     &pos_obj, &size_obj)) {
         return -1;
     }
 
-    // Parse bounds: ((x, y), (w, h))
+    // Parse position using PyPositionHelper pattern
-    if (!PyTuple_Check(bounds_obj) || PyTuple_Size(bounds_obj) != 2) {
+    int x, y;
-        PyErr_SetString(PyExc_TypeError, "bounds must be ((x, y), (w, h)) tuple");
+    if (!PyPosition_FromObjectInt(pos_obj, &x, &y)) {
+        PyErr_SetString(PyExc_TypeError, "pos must be a tuple (x, y), list, or Vector");
         return -1;
     }
 
-    PyObject* pos_obj = PyTuple_GetItem(bounds_obj, 0);
+    // Parse size using PyPositionHelper pattern
-    PyObject* size_obj = PyTuple_GetItem(bounds_obj, 1);
+    int w, h;
-
+    if (!PyPosition_FromObjectInt(size_obj, &w, &h)) {
-    if (!PyTuple_Check(pos_obj) || PyTuple_Size(pos_obj) != 2 ||
+        PyErr_SetString(PyExc_TypeError, "size must be a tuple (w, h), list, or Vector");
-        !PyTuple_Check(size_obj) || PyTuple_Size(size_obj) != 2) {
-        PyErr_SetString(PyExc_TypeError, "bounds must be ((x, y), (w, h)) tuple");
-        return -1;
-    }
-
-    int x = (int)PyLong_AsLong(PyTuple_GetItem(pos_obj, 0));
-    int y = (int)PyLong_AsLong(PyTuple_GetItem(pos_obj, 1));
-    int w = (int)PyLong_AsLong(PyTuple_GetItem(size_obj, 0));
-    int h = (int)PyLong_AsLong(PyTuple_GetItem(size_obj, 1));
-
-    if (PyErr_Occurred()) {
         return -1;
     }
 
@@ -307,6 +317,14 @@ int PyBSP::init(PyBSPObject* self, PyObject* args, PyObject* kwds)
         return -1;
     }
 
+    // Validate against GRID_MAX like HeightMap does
+    if (w > GRID_MAX || h > GRID_MAX) {
+        PyErr_Format(PyExc_ValueError,
+            "BSP dimensions cannot exceed %d (got %dx%d)",
+            GRID_MAX, w, h);
+        return -1;
+    }
+
     // Clean up any existing BSP
     if (self->root) {
         TCOD_bsp_delete(self->root);
@@ -324,6 +342,7 @@ int PyBSP::init(PyBSPObject* self, PyObject* args, PyObject* kwds)
     self->orig_y = y;
     self->orig_w = w;
     self->orig_h = h;
+    self->generation = 0;
 
     return 0;
 }
@@ -373,6 +392,26 @@ PyObject* PyBSP::get_bounds(PyBSPObject* self, void* closure)
                          self->root->w, self->root->h);
 }
 
+// Property: pos
+PyObject* PyBSP::get_pos(PyBSPObject* self, void* closure)
+{
+    if (!self->root) {
+        PyErr_SetString(PyExc_RuntimeError, "BSP not initialized");
+        return nullptr;
+    }
+    return Py_BuildValue("(ii)", self->root->x, self->root->y);
+}
+
+// Property: size
+PyObject* PyBSP::get_size(PyBSPObject* self, void* closure)
+{
+    if (!self->root) {
+        PyErr_SetString(PyExc_RuntimeError, "BSP not initialized");
+        return nullptr;
+    }
+    return Py_BuildValue("(ii)", self->root->w, self->root->h);
+}
+
 // Property: root
 PyObject* PyBSP::get_root(PyBSPObject* self, void* closure)
 {
@@ -400,6 +439,8 @@ PyObject* PyBSP::split_once(PyBSPObject* self, PyObject* args, PyObject* kwds)
         return nullptr;
     }
 
+    // Note: split_once only adds children, doesn't remove any nodes
+    // Root node pointer remains valid, so we don't increment generation
     TCOD_bsp_split_once(self->root, horizontal ? true : false, position);
 
     Py_INCREF(self);
@@ -425,21 +466,22 @@ PyObject* PyBSP::split_recursive(PyBSPObject* self, PyObject* args, PyObject* kw
         return nullptr;
     }
 
-    // Parse min_size tuple
+    // Parse min_size using PyPositionHelper pattern
-    if (!PyTuple_Check(min_size_obj) || PyTuple_Size(min_size_obj) != 2) {
+    int min_w, min_h;
-        PyErr_SetString(PyExc_TypeError, "min_size must be (width, height) tuple");
+    if (!PyPosition_FromObjectInt(min_size_obj, &min_w, &min_h)) {
+        PyErr_SetString(PyExc_TypeError, "min_size must be (width, height) tuple, list, or Vector");
         return nullptr;
     }
 
-    int min_w = (int)PyLong_AsLong(PyTuple_GetItem(min_size_obj, 0));
+    if (depth < 1) {
-    int min_h = (int)PyLong_AsLong(PyTuple_GetItem(min_size_obj, 1));
+        PyErr_SetString(PyExc_ValueError, "depth must be at least 1");
-
-    if (PyErr_Occurred()) {
         return nullptr;
     }
 
-    if (depth < 0) {
+    if (depth > BSP_MAX_DEPTH) {
-        PyErr_SetString(PyExc_ValueError, "depth must be non-negative");
+        PyErr_Format(PyExc_ValueError,
+            "depth cannot exceed %d (got %d) to prevent memory exhaustion",
+            BSP_MAX_DEPTH, depth);
         return nullptr;
     }
 
@@ -462,6 +504,9 @@ PyObject* PyBSP::split_recursive(PyBSPObject* self, PyObject* args, PyObject* kw
         rnd = TCOD_random_new_from_seed(TCOD_RNG_MT, seed);
     }
 
+    // Increment generation BEFORE splitting - invalidates existing nodes
+    self->generation++;
+
     TCOD_bsp_split_recursive(self->root, rnd, depth, min_w, min_h, max_ratio, max_ratio);
 
     if (rnd) {
@@ -480,6 +525,9 @@ PyObject* PyBSP::clear(PyBSPObject* self, PyObject* Py_UNUSED(args))
         return nullptr;
     }
 
+    // Increment generation BEFORE clearing - invalidates existing nodes
+    self->generation++;
+
     TCOD_bsp_remove_sons(self->root);
 
     // Restore original bounds
@@ -489,6 +537,31 @@ PyObject* PyBSP::clear(PyBSPObject* self, PyObject* Py_UNUSED(args))
     return (PyObject*)self;
 }
 
+// Sequence protocol: len(bsp) returns leaf count
+Py_ssize_t PyBSP::len(PyBSPObject* self)
+{
+    if (!self->root) {
+        PyErr_SetString(PyExc_RuntimeError, "BSP not initialized");
+        return -1;
+    }
+
+    int leaf_count = 0;
+    TCOD_bsp_traverse_pre_order(self->root, [](TCOD_bsp_t* node, void* data) -> bool {
+        if (TCOD_bsp_is_leaf(node)) {
+            (*(int*)data)++;
+        }
+        return true;
+    }, &leaf_count);
+
+    return (Py_ssize_t)leaf_count;
+}
+
+// __iter__ is shorthand for leaves()
+PyObject* PyBSP::iter(PyBSPObject* self)
+{
+    return PyBSP::leaves(self, nullptr);
+}
+
 // ==================== BSP Iteration ====================
 
 // Traversal callback to collect nodes
@@ -527,6 +600,7 @@ PyObject* PyBSP::leaves(PyBSPObject* self, PyObject* Py_UNUSED(args))
 
     iter->index = 0;
     iter->bsp_owner = (PyObject*)self;
+    iter->generation = self->generation;  // Capture generation for validity check
     Py_INCREF(self);
 
     return (PyObject*)iter;
@@ -584,6 +658,7 @@ PyObject* PyBSP::traverse(PyBSPObject* self, PyObject* args, PyObject* kwds)
 
     iter->index = 0;
     iter->bsp_owner = (PyObject*)self;
+    iter->generation = self->generation;  // Capture generation for validity check
     Py_INCREF(self);
 
     return (PyObject*)iter;
@@ -633,13 +708,8 @@ PyObject* PyBSP::to_heightmap(PyBSPObject* self, PyObject* args, PyObject* kwds)
     int width = self->root->w;
     int height = self->root->h;
     if (size_obj != nullptr && size_obj != Py_None) {
-        if (!PyTuple_Check(size_obj) || PyTuple_Size(size_obj) != 2) {
+        if (!PyPosition_FromObjectInt(size_obj, &width, &height)) {
-            PyErr_SetString(PyExc_TypeError, "size must be (width, height) tuple");
+            PyErr_SetString(PyExc_TypeError, "size must be (width, height) tuple, list, or Vector");
-            return nullptr;
-        }
-        width = (int)PyLong_AsLong(PyTuple_GetItem(size_obj, 0));
-        height = (int)PyLong_AsLong(PyTuple_GetItem(size_obj, 1));
-        if (PyErr_Occurred()) {
             return nullptr;
         }
     }
@@ -751,6 +821,10 @@ PyObject* PyBSP::to_heightmap(PyBSPObject* self, PyObject* args, PyObject* kwds)
 PyGetSetDef PyBSPNode::getsetters[] = {
     {"bounds", (getter)PyBSPNode::get_bounds, NULL,
      MCRF_PROPERTY(bounds, "Node bounds as ((x, y), (w, h)). Read-only."), NULL},
+    {"pos", (getter)PyBSPNode::get_pos, NULL,
+     MCRF_PROPERTY(pos, "Top-left position (x, y). Read-only."), NULL},
+    {"size", (getter)PyBSPNode::get_size, NULL,
+     MCRF_PROPERTY(size, "Dimensions (width, height). Read-only."), NULL},
     {"level", (getter)PyBSPNode::get_level, NULL,
      MCRF_PROPERTY(level, "Depth in tree (0 for root). Read-only."), NULL},
     {"is_leaf", (getter)PyBSPNode::get_is_leaf, NULL,
@@ -792,6 +866,30 @@ PyMethodDef PyBSPNode::methods[] = {
 
 // ==================== BSPNode Implementation ====================
 
+// Validity check - returns false and sets error if node is stale
+bool PyBSPNode::checkValid(PyBSPNodeObject* self)
+{
+    if (!self->node) {
+        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid (null pointer)");
+        return false;
+    }
+
+    if (!self->bsp_owner) {
+        PyErr_SetString(PyExc_RuntimeError, "BSPNode has no parent BSP");
+        return false;
+    }
+
+    PyBSPObject* bsp = (PyBSPObject*)self->bsp_owner;
+    if (self->generation != bsp->generation) {
+        PyErr_SetString(PyExc_RuntimeError,
+            "BSPNode is stale: parent BSP was modified (clear() or split_recursive() called). "
+            "Re-fetch nodes from the BSP object.");
+        return false;
+    }
+
+    return true;
+}
+
 PyObject* PyBSPNode::pynew(PyTypeObject* type, PyObject* args, PyObject* kwds)
 {
     // BSPNode cannot be directly instantiated
@@ -817,20 +915,52 @@ void PyBSPNode::dealloc(PyBSPNodeObject* self)
 PyObject* PyBSPNode::repr(PyObject* obj)
 {
     PyBSPNodeObject* self = (PyBSPNodeObject*)obj;
+
+    // Check validity without raising error for repr
+    PyBSPObject* bsp = self->bsp_owner ? (PyBSPObject*)self->bsp_owner : nullptr;
+    bool is_valid = self->node && bsp && self->generation == bsp->generation;
+
     std::ostringstream ss;
 
-    if (self->node) {
+    if (is_valid) {
         const char* type = TCOD_bsp_is_leaf(self->node) ? "leaf" : "split";
         ss << "<BSPNode " << type << " at (" << self->node->x << ", " << self->node->y
            << ") size (" << self->node->w << " x " << self->node->h << ") level "
            << (int)self->node->level << ">";
     } else {
-        ss << "<BSPNode (invalid)>";
+        ss << "<BSPNode (invalid/stale)>";
     }
 
     return PyUnicode_FromString(ss.str().c_str());
 }
 
+// Rich comparison for BSPNode - compares underlying pointers
+PyObject* PyBSPNode::richcompare(PyObject* self, PyObject* other, int op)
+{
+    // Only support == and !=
+    if (op != Py_EQ && op != Py_NE) {
+        Py_RETURN_NOTIMPLEMENTED;
+    }
+
+    // Check if other is also a BSPNode
+    if (!PyObject_TypeCheck(other, &mcrfpydef::PyBSPNodeType)) {
+        if (op == Py_EQ) Py_RETURN_FALSE;
+        else Py_RETURN_TRUE;
+    }
+
+    PyBSPNodeObject* self_node = (PyBSPNodeObject*)self;
+    PyBSPNodeObject* other_node = (PyBSPNodeObject*)other;
+
+    // Compare wrapped pointers
+    bool equal = (self_node->node == other_node->node);
+
+    if (op == Py_EQ) {
+        return PyBool_FromLong(equal);
+    } else {
+        return PyBool_FromLong(!equal);
+    }
+}
+
 // Helper to create a BSPNode wrapper
 PyObject* PyBSPNode::create(TCOD_bsp_t* node, PyObject* bsp_owner)
 {
@@ -846,6 +976,7 @@ PyObject* PyBSPNode::create(TCOD_bsp_t* node, PyObject* bsp_owner)
 
     py_node->node = node;
     py_node->bsp_owner = bsp_owner;
+    py_node->generation = ((PyBSPObject*)bsp_owner)->generation;
     Py_INCREF(bsp_owner);
 
     return (PyObject*)py_node;
@@ -854,42 +985,44 @@ PyObject* PyBSPNode::create(TCOD_bsp_t* node, PyObject* bsp_owner)
 // Property: bounds
 PyObject* PyBSPNode::get_bounds(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
     return Py_BuildValue("((ii)(ii))",
                          self->node->x, self->node->y,
                          self->node->w, self->node->h);
 }
 
+// Property: pos
+PyObject* PyBSPNode::get_pos(PyBSPNodeObject* self, void* closure)
+{
+    if (!checkValid(self)) return nullptr;
+    return Py_BuildValue("(ii)", self->node->x, self->node->y);
+}
+
+// Property: size
+PyObject* PyBSPNode::get_size(PyBSPNodeObject* self, void* closure)
+{
+    if (!checkValid(self)) return nullptr;
+    return Py_BuildValue("(ii)", self->node->w, self->node->h);
+}
+
 // Property: level
 PyObject* PyBSPNode::get_level(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
     return PyLong_FromLong(self->node->level);
 }
 
 // Property: is_leaf
 PyObject* PyBSPNode::get_is_leaf(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
     return PyBool_FromLong(TCOD_bsp_is_leaf(self->node));
 }
 
 // Property: split_horizontal
 PyObject* PyBSPNode::get_split_horizontal(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
     if (TCOD_bsp_is_leaf(self->node)) {
         Py_RETURN_NONE;
     }
@@ -899,10 +1032,7 @@ PyObject* PyBSPNode::get_split_horizontal(PyBSPNodeObject* self, void* closure)
 // Property: split_position
 PyObject* PyBSPNode::get_split_position(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
     if (TCOD_bsp_is_leaf(self->node)) {
         Py_RETURN_NONE;
     }
@@ -912,40 +1042,28 @@ PyObject* PyBSPNode::get_split_position(PyBSPNodeObject* self, void* closure)
 // Property: left
 PyObject* PyBSPNode::get_left(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
     return PyBSPNode::create(TCOD_bsp_left(self->node), self->bsp_owner);
 }
 
 // Property: right
 PyObject* PyBSPNode::get_right(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
     return PyBSPNode::create(TCOD_bsp_right(self->node), self->bsp_owner);
 }
 
 // Property: parent
 PyObject* PyBSPNode::get_parent(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
     return PyBSPNode::create(TCOD_bsp_father(self->node), self->bsp_owner);
 }
 
 // Property: sibling
 PyObject* PyBSPNode::get_sibling(PyBSPNodeObject* self, void* closure)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
 
     TCOD_bsp_t* parent = TCOD_bsp_father(self->node);
     if (!parent) {
@@ -965,10 +1083,7 @@ PyObject* PyBSPNode::get_sibling(PyBSPNodeObject* self, void* closure)
 // Method: contains(pos) -> bool
 PyObject* PyBSPNode::contains(PyBSPNodeObject* self, PyObject* args, PyObject* kwds)
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
 
     int x, y;
     if (!PyPosition_ParseInt(args, kwds, &x, &y)) {
@@ -981,10 +1096,7 @@ PyObject* PyBSPNode::contains(PyBSPNodeObject* self, PyObject* args, PyObject* k
 // Method: center() -> (x, y)
 PyObject* PyBSPNode::center(PyBSPNodeObject* self, PyObject* Py_UNUSED(args))
 {
-    if (!self->node) {
+    if (!checkValid(self)) return nullptr;
-        PyErr_SetString(PyExc_RuntimeError, "BSPNode is invalid");
-        return nullptr;
-    }
 
     int cx = self->node->x + self->node->w / 2;
     int cy = self->node->y + self->node->h / 2;
@@ -1017,6 +1129,16 @@ PyObject* PyBSPIter::next(PyBSPIterObject* self)
         return NULL;
     }
 
+    // Check for tree modification during iteration
+    if (self->bsp_owner) {
+        PyBSPObject* bsp = (PyBSPObject*)self->bsp_owner;
+        if (self->generation != bsp->generation) {
+            PyErr_SetString(PyExc_RuntimeError,
+                "BSP tree was modified during iteration (clear() or split_recursive() called)");
+            return NULL;
+        }
+    }
+
     if (self->index >= self->nodes->size()) {
         PyErr_SetNone(PyExc_StopIteration);
         return NULL;

src/PyBSP.h

@@ -3,17 +3,23 @@
 #include "Python.h"
 #include <libtcod.h>
 #include <vector>
+#include <cstdint>
 
 // Forward declarations
 class PyBSP;
 class PyBSPNode;
 
+// Maximum recursion depth to prevent memory exhaustion
+// 2^16 = 65536 potential leaf nodes, which is already excessive
+constexpr int BSP_MAX_DEPTH = 16;
+
 // Python object structure for BSP tree (root owner)
 typedef struct {
     PyObject_HEAD
     TCOD_bsp_t* root;           // libtcod BSP root (owned, will be deleted)
     int orig_x, orig_y;         // Original bounds for clear()
     int orig_w, orig_h;
+    uint64_t generation;        // Incremented on structural changes (clear, split)
 } PyBSPObject;
 
 // Python object structure for BSPNode (lightweight reference)
@@ -21,6 +27,7 @@ typedef struct {
     PyObject_HEAD
     TCOD_bsp_t* node;           // libtcod BSP node (NOT owned)
     PyObject* bsp_owner;        // Reference to PyBSPObject to prevent dangling
+    uint64_t generation;        // Generation at time of creation (for validity check)
 } PyBSPNodeObject;
 
 // BSP iterator for traverse()
@@ -29,6 +36,7 @@ typedef struct {
     std::vector<TCOD_bsp_t*>* nodes;  // Pre-collected nodes
     size_t index;
     PyObject* bsp_owner;              // Reference to PyBSPObject
+    uint64_t generation;              // Generation at iterator creation
 } PyBSPIterObject;
 
 class PyBSP
@@ -42,6 +50,8 @@ public:
 
     // Properties
     static PyObject* get_bounds(PyBSPObject* self, void* closure);
+    static PyObject* get_pos(PyBSPObject* self, void* closure);
+    static PyObject* get_size(PyBSPObject* self, void* closure);
     static PyObject* get_root(PyBSPObject* self, void* closure);
 
     // Splitting methods (#202)
@@ -59,9 +69,14 @@ public:
     // HeightMap conversion (#206)
     static PyObject* to_heightmap(PyBSPObject* self, PyObject* args, PyObject* kwds);
 
+    // Sequence protocol
+    static Py_ssize_t len(PyBSPObject* self);
+    static PyObject* iter(PyBSPObject* self);
+
     // Method and property definitions
     static PyMethodDef methods[];
     static PyGetSetDef getsetters[];
+    static PySequenceMethods sequence_methods;
 };
 
 class PyBSPNode
@@ -73,8 +88,13 @@ public:
     static void dealloc(PyBSPNodeObject* self);
     static PyObject* repr(PyObject* obj);
 
+    // Comparison
+    static PyObject* richcompare(PyObject* self, PyObject* other, int op);
+
     // Properties (#203)
     static PyObject* get_bounds(PyBSPNodeObject* self, void* closure);
+    static PyObject* get_pos(PyBSPNodeObject* self, void* closure);
+    static PyObject* get_size(PyBSPNodeObject* self, void* closure);
     static PyObject* get_level(PyBSPNodeObject* self, void* closure);
     static PyObject* get_is_leaf(PyBSPNodeObject* self, void* closure);
     static PyObject* get_split_horizontal(PyBSPNodeObject* self, void* closure);
@@ -93,6 +113,9 @@ public:
     // Helper to create a BSPNode from a TCOD_bsp_t*
     static PyObject* create(TCOD_bsp_t* node, PyObject* bsp_owner);
 
+    // Validity check - returns false and sets error if node is stale
+    static bool checkValid(PyBSPNodeObject* self);
+
     // Method and property definitions
     static PyMethodDef methods[];
     static PyGetSetDef getsetters[];
@@ -116,6 +139,8 @@ public:
     static PyObject* traversal_enum_class;
     static PyObject* create_enum_class(PyObject* module);
     static int from_arg(PyObject* arg, int* out_order);
+    // Cleanup for module finalization (optional)
+    static void cleanup();
 };
 
 namespace mcrfpydef {
@@ -127,23 +152,31 @@ namespace mcrfpydef {
         .tp_itemsize = 0,
         .tp_dealloc = (destructor)PyBSP::dealloc,
         .tp_repr = PyBSP::repr,
+        .tp_as_sequence = &PyBSP::sequence_methods,
         .tp_flags = Py_TPFLAGS_DEFAULT,
         .tp_doc = PyDoc_STR(
-            "BSP(bounds: tuple[tuple[int, int], tuple[int, int]])\n\n"
+            "BSP(pos: tuple[int, int], size: tuple[int, int])\n\n"
             "Binary Space Partitioning tree for procedural dungeon generation.\n\n"
             "BSP recursively divides a rectangular region into smaller sub-regions, "
             "creating a tree structure perfect for generating dungeon rooms and corridors.\n\n"
             "Args:\n"
-            "    bounds: ((x, y), (w, h)) - Position and size of the root region.\n\n"
+            "    pos: (x, y) - Top-left position of the root region.\n"
+            "    size: (w, h) - Width and height of the root region.\n\n"
             "Properties:\n"
-            "    bounds ((x, y), (w, h)): Read-only. Root node bounds.\n"
+            "    pos (tuple[int, int]): Read-only. Top-left position (x, y).\n"
+            "    size (tuple[int, int]): Read-only. Dimensions (width, height).\n"
+            "    bounds ((pos), (size)): Read-only. Combined position and size.\n"
             "    root (BSPNode): Read-only. Reference to the root node.\n\n"
+            "Iteration:\n"
+            "    for leaf in bsp:  # Iterates over leaf nodes (rooms)\n"
+            "    len(bsp)          # Returns number of leaf nodes\n\n"
             "Example:\n"
-            "    bsp = mcrfpy.BSP(bounds=((0, 0), (80, 50)))\n"
+            "    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 50))\n"
             "    bsp.split_recursive(depth=4, min_size=(8, 8))\n"
-            "    for leaf in bsp.leaves():\n"
+            "    for leaf in bsp:\n"
-            "        print(f'Room at {leaf.bounds}')\n"
+            "        print(f'Room at {leaf.pos}, size {leaf.size}')\n"
         ),
+        .tp_iter = (getiterfunc)PyBSP::iter,
         .tp_methods = nullptr,  // Set in McRFPy_API.cpp
         .tp_getset = nullptr,   // Set in McRFPy_API.cpp
         .tp_init = (initproc)PyBSP::init,
@@ -163,8 +196,13 @@ namespace mcrfpydef {
             "BSPNode - Lightweight reference to a node in a BSP tree.\n\n"
             "BSPNode provides read-only access to node properties and navigation.\n"
             "Nodes are created by BSP methods, not directly instantiated.\n\n"
+            "WARNING: BSPNode references become invalid after BSP.clear() or\n"
+            "BSP.split_recursive(). Accessing properties of an invalid node\n"
+            "raises RuntimeError.\n\n"
             "Properties:\n"
-            "    bounds ((x, y), (w, h)): Position and size of this node.\n"
+            "    pos (tuple[int, int]): Top-left position (x, y).\n"
+            "    size (tuple[int, int]): Dimensions (width, height).\n"
+            "    bounds ((pos), (size)): Combined position and size.\n"
             "    level (int): Depth in tree (0 for root).\n"
             "    is_leaf (bool): True if this node has no children.\n"
             "    split_horizontal (bool | None): Split orientation, None if leaf.\n"
@@ -174,6 +212,7 @@ namespace mcrfpydef {
             "    parent (BSPNode | None): Parent node, or None if root.\n"
             "    sibling (BSPNode | None): Other child of parent, or None.\n"
         ),
+        .tp_richcompare = PyBSPNode::richcompare,
         .tp_methods = nullptr,  // Set in McRFPy_API.cpp
         .tp_getset = nullptr,   // Set in McRFPy_API.cpp
         .tp_init = (initproc)PyBSPNode::init,

tests/unit/bsp_simple_test.py

@@ -5,7 +5,7 @@ import mcrfpy
 print("Step 1: Import complete")
 
 print("Step 2: Creating BSP...")
-bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80)))
+bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80))
 print("Step 2: BSP created:", bsp)
 
 print("Step 3: Getting bounds...")

tests/unit/bsp_test.py

@@ -11,14 +11,18 @@ import sys
 import mcrfpy
 
 def test_bsp_construction():
-    """Test BSP construction with bounds."""
+    """Test BSP construction with pos/size."""
     print("Testing BSP construction...")
 
-    # Basic construction
+    # Basic construction with pos/size kwargs
-    bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80))
     assert bsp is not None, "BSP should be created"
 
-    # Check bounds property
+    # Check pos and size properties
+    assert bsp.pos == (0, 0), f"pos should be (0, 0), got {bsp.pos}"
+    assert bsp.size == (100, 80), f"size should be (100, 80), got {bsp.size}"
+
+    # Check bounds property (combines pos and size)
     bounds = bsp.bounds
     assert bounds == ((0, 0), (100, 80)), f"Bounds should be ((0, 0), (100, 80)), got {bounds}"
 
@@ -28,7 +32,7 @@ def test_bsp_construction():
     assert root.bounds == ((0, 0), (100, 80)), f"Root bounds mismatch"
 
     # Construction with offset
-    bsp2 = mcrfpy.BSP(bounds=((10, 20), (50, 40)))
+    bsp2 = mcrfpy.BSP(pos=(10, 20), size=(50, 40))
     assert bsp2.bounds == ((10, 20), (50, 40)), "Offset bounds not preserved"
 
     print("  BSP construction: PASS")
@@ -37,7 +41,7 @@ def test_bsp_split_once():
     """Test single split operation (#202)."""
     print("Testing BSP split_once...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80))
 
     # Before split, root should be a leaf
     assert bsp.root.is_leaf, "Root should be leaf before split"
@@ -66,7 +70,7 @@ def test_bsp_split_recursive():
     """Test recursive splitting (#202)."""
     print("Testing BSP split_recursive...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
 
     # Recursive split with seed for reproducibility
     result = bsp.split_recursive(depth=3, min_size=(8, 8), max_ratio=1.5, seed=42)
@@ -91,7 +95,7 @@ def test_bsp_clear():
     """Test clear operation (#202)."""
     print("Testing BSP clear...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80))
     bsp.split_recursive(depth=4, min_size=(8, 8), seed=42)
 
     # Should have multiple leaves
@@ -116,7 +120,7 @@ def test_bspnode_properties():
     """Test BSPNode properties (#203)."""
     print("Testing BSPNode properties...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80))
     bsp.split_recursive(depth=3, min_size=(8, 8), seed=42)
 
     root = bsp.root
@@ -155,7 +159,7 @@ def test_bspnode_navigation():
     """Test BSPNode navigation (#203)."""
     print("Testing BSPNode navigation...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (100, 80)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(100, 80))
     bsp.split_once(horizontal=True, position=40)
 
     root = bsp.root
@@ -182,7 +186,7 @@ def test_bspnode_contains():
     """Test BSPNode contains method (#203)."""
     print("Testing BSPNode contains...")
 
-    bsp = mcrfpy.BSP(bounds=((10, 20), (50, 40)))  # x: 10-60, y: 20-60
+    bsp = mcrfpy.BSP(pos=(10, 20), size=(50, 40))  # x: 10-60, y: 20-60
 
     root = bsp.root
 
@@ -203,7 +207,7 @@ def test_bsp_leaves_iteration():
     """Test leaves iteration (#204)."""
     print("Testing BSP leaves iteration...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
     bsp.split_recursive(depth=3, min_size=(8, 8), seed=42)
 
     # Iterate leaves
@@ -224,7 +228,7 @@ def test_bsp_traverse():
     """Test traverse with different orders (#204)."""
     print("Testing BSP traverse...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
     bsp.split_recursive(depth=2, min_size=(8, 8), seed=42)
 
     # Test all traversal orders
@@ -261,7 +265,7 @@ def test_bsp_find():
     """Test find method (#205)."""
     print("Testing BSP find...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
     bsp.split_recursive(depth=3, min_size=(8, 8), seed=42)
 
     # Find a point inside bounds
@@ -285,7 +289,7 @@ def test_bsp_to_heightmap():
     """Test to_heightmap conversion (#206)."""
     print("Testing BSP to_heightmap...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (50, 40)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(50, 40))
     bsp.split_recursive(depth=2, min_size=(8, 8), seed=42)
 
     # Basic conversion
@@ -346,7 +350,7 @@ def test_bsp_chaining():
     """Test method chaining."""
     print("Testing BSP method chaining...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
 
     # Chain multiple operations
     result = bsp.split_recursive(depth=2, min_size=(8, 8), seed=42).clear().split_once(True, 30)
@@ -358,7 +362,7 @@ def test_bsp_repr():
     """Test repr output."""
     print("Testing BSP repr...")
 
-    bsp = mcrfpy.BSP(bounds=((0, 0), (80, 60)))
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
     repr_str = repr(bsp)
     assert "BSP" in repr_str, f"repr should contain BSP: {repr_str}"
     assert "80" in repr_str and "60" in repr_str, f"repr should contain size: {repr_str}"
@@ -373,6 +377,159 @@ def test_bsp_repr():
 
     print("  BSP repr: PASS")
 
+def test_bsp_stale_node_detection():
+    """Test that stale nodes are detected after clear()/split_recursive() (#review)."""
+    print("Testing BSP stale node detection...")
+
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
+    bsp.split_recursive(depth=2, min_size=(8, 8), seed=42)
+
+    # Save reference to a node
+    old_root = bsp.root
+    old_leaf = list(bsp.leaves())[0]
+
+    # Clear invalidates all nodes
+    bsp.clear()
+
+    # Accessing stale node should raise RuntimeError
+    try:
+        _ = old_root.bounds
+        assert False, "Accessing stale root bounds should raise RuntimeError"
+    except RuntimeError as e:
+        assert "stale" in str(e).lower() or "invalid" in str(e).lower(), \
+            f"Error should mention staleness: {e}"
+
+    try:
+        _ = old_leaf.is_leaf
+        assert False, "Accessing stale leaf should raise RuntimeError"
+    except RuntimeError as e:
+        pass  # Expected
+
+    # split_recursive also invalidates (rebuilds tree)
+    bsp2 = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
+    bsp2.split_recursive(depth=2, min_size=(8, 8), seed=42)
+    saved_node = bsp2.root
+    bsp2.split_recursive(depth=3, min_size=(8, 8), seed=99)
+
+    try:
+        _ = saved_node.bounds
+        assert False, "Accessing node after split_recursive should raise RuntimeError"
+    except RuntimeError:
+        pass  # Expected
+
+    print("  BSP stale node detection: PASS")
+
+def test_bsp_grid_max_validation():
+    """Test GRID_MAX validation (#review)."""
+    print("Testing BSP GRID_MAX validation...")
+
+    # Should succeed with valid size
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(1000, 1000))
+    assert bsp is not None
+
+    # Should fail with size exceeding GRID_MAX (8192)
+    try:
+        bsp_too_big = mcrfpy.BSP(pos=(0, 0), size=(10000, 100))
+        assert False, "Should raise ValueError for size > GRID_MAX"
+    except ValueError as e:
+        assert "8192" in str(e) or "exceed" in str(e).lower(), f"Error should mention limit: {e}"
+
+    try:
+        bsp_too_big = mcrfpy.BSP(pos=(0, 0), size=(100, 10000))
+        assert False, "Should raise ValueError for height > GRID_MAX"
+    except ValueError as e:
+        pass  # Expected
+
+    print("  BSP GRID_MAX validation: PASS")
+
+def test_bsp_depth_cap():
+    """Test depth is capped at 16 (#review)."""
+    print("Testing BSP depth cap...")
+
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(1000, 1000))
+
+    # Should cap depth at 16 or raise ValueError
+    try:
+        bsp.split_recursive(depth=20, min_size=(1, 1), seed=42)
+        # If it succeeds, verify reasonable number of leaves (not 2^20)
+        leaves = list(bsp.leaves())
+        assert len(leaves) <= 2**16, f"Too many leaves: {len(leaves)}"
+    except ValueError as e:
+        # It's also acceptable to reject excessive depth
+        assert "16" in str(e) or "depth" in str(e).lower(), f"Error should mention depth limit: {e}"
+
+    print("  BSP depth cap: PASS")
+
+def test_bsp_len():
+    """Test __len__ returns leaf count (#review)."""
+    print("Testing BSP __len__...")
+
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
+    assert len(bsp) == 1, f"Unsplit BSP should have 1 leaf, got {len(bsp)}"
+
+    bsp.split_once(horizontal=True, position=30)
+    assert len(bsp) == 2, f"After one split should have 2 leaves, got {len(bsp)}"
+
+    bsp.clear()
+    bsp.split_recursive(depth=3, min_size=(8, 8), seed=42)
+    expected = len(list(bsp.leaves()))
+    assert len(bsp) == expected, f"len() mismatch: {len(bsp)} != {expected}"
+
+    print("  BSP __len__: PASS")
+
+def test_bsp_iter():
+    """Test __iter__ as shorthand for leaves() (#review)."""
+    print("Testing BSP __iter__...")
+
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
+    bsp.split_recursive(depth=2, min_size=(8, 8), seed=42)
+
+    # Direct iteration should yield same results as leaves()
+    iter_list = list(bsp)
+    leaves_list = list(bsp.leaves())
+
+    assert len(iter_list) == len(leaves_list), \
+        f"Iterator count mismatch: {len(iter_list)} != {len(leaves_list)}"
+
+    # All items should be leaves
+    for node in bsp:
+        assert node.is_leaf, f"Iterator should yield leaves: {node}"
+
+    # Can iterate multiple times
+    count1 = sum(1 for _ in bsp)
+    count2 = sum(1 for _ in bsp)
+    assert count1 == count2, "Should be able to iterate multiple times"
+
+    print("  BSP __iter__: PASS")
+
+def test_bspnode_equality():
+    """Test BSPNode __eq__ comparison (#review)."""
+    print("Testing BSPNode equality...")
+
+    bsp = mcrfpy.BSP(pos=(0, 0), size=(80, 60))
+    bsp.split_recursive(depth=2, min_size=(8, 8), seed=42)
+
+    # Same node should be equal
+    root1 = bsp.root
+    root2 = bsp.root
+    assert root1 == root2, "Same node should be equal"
+
+    # Different nodes should not be equal
+    leaves = list(bsp.leaves())
+    assert len(leaves) >= 2, "Need at least 2 leaves for comparison"
+    assert leaves[0] != leaves[1], "Different nodes should not be equal"
+
+    # Parent vs child should not be equal
+    root = bsp.root
+    left = root.left
+    assert root != left, "Parent and child should not be equal"
+
+    # Not equal to non-BSPNode
+    assert not (root == "not a node"), "BSPNode should not equal string"
+    assert not (root == 42), "BSPNode should not equal int"
+
+    print("  BSPNode equality: PASS")
+
 def run_all_tests():
     """Run all BSP tests."""
     print("\n=== BSP Unit Tests ===\n")
@@ -392,6 +549,12 @@ def run_all_tests():
         test_traversal_enum()
         test_bsp_chaining()
         test_bsp_repr()
+        test_bsp_stale_node_detection()
+        test_bsp_grid_max_validation()
+        test_bsp_depth_cap()
+        test_bsp_len()
+        test_bsp_iter()
+        test_bspnode_equality()
 
         print("\n=== ALL BSP TESTS PASSED ===\n")
         sys.exit(0)