Code review findings (Claude)

[henryiii]

Tried to use Claude Fable, but I think it might have been just after the cutoff and it used Opus instead - not clear from my history.

🤖 AI text below 🤖

A code review pass (agent-assisted, findings manually verified) turned up one confirmed memory-safety bug, one concurrency issue, and a few smaller items. Filing them together here; draft PRs will reference this issue.

1. bind_vector __delitem__ with negative-step slices: wrong elements deleted / out-of-bounds erase

include/pybind11/stl_bind.h (__delitem__(slice)) uses start += step - 1, which is only correct for positive steps (where erasing shifts subsequent elements down by one). For negative steps, elements below start don't shift, so the -1 correction deletes the wrong elements and eventually erases out of bounds.

Verified with a minimal bind_vector<std::vector<int>> module:

• del v[::-2] on [0, 1, 2, 3] produced [1, 2] instead of [0, 2] — silent data corruption
• del v[::-1] ends up calling v.erase(v.begin() - 1) — UB; observed both a SIGBUS crash and a "correct-looking" result on different runs

There is currently no test coverage for vector slice deletion (only test_map_delitem exists).

Adjacent: the same function contains if (step == 1 && false) — a debugging artifact from the original 2016 stl_bind redesign (25c03ce) that has disabled the O(n) contiguous-erase fast path ever since, making del v[1:1000] perform 999 separate erase calls.

2. gil_safe_call_once_and_store (subinterpreter branch): data race on last_storage_ptr_

include/pybind11/gil_safe_call_once.h (get_stored()):

1. Data race under free-threading. last_storage_ptr_ is a plain T*, written in call_once_and_store_result() / get_stored() and read in get_stored() with no synchronization (under Py_GIL_DISABLED, gil_scoped_acquire provides no mutual exclusion). The pointer is also read before the is_last_storage_valid() check, so even an atomic flag doesn't order it — a thread could pass validation having read a stale/null pointer. Fix: make last_storage_ptr_ std::atomic<T*> and load it after the validity check.
2. Embedded finalize/re-init staleness (medium confidence). is_initialized_by_at_least_one_interpreter_ is never reset, and has_seen_non_main_interpreter() stays false if only the main interpreter is used. After finalize_interpreter() + initialize_interpreter(), the fast path would return the cached pointer into the destroyed interpreter's state-dict capsule. Not yet reproduced; needs discussion on whether/where to hook a reset.

3. CMake: stale cache / variable mismatch

• tools/pybind11NewTools.cmake — when the Python executable changes between runs, PYTHON_IS_DEBUG and PYTHON_MODULE_EXTENSION are unset from cache but PYTHON_MODULE_DEBUG_POSTFIX is not, so a stale postfix from the previous interpreter survives (matters for Windows debug builds switching interpreters in one build dir).
• CMakeLists.txt (~line 307) — the branch checks DEFINED PYTHON_INCLUDE_DIR (singular) but uses ${PYTHON_INCLUDE_DIRS} (plural). Latent today because FindPythonLibsNew.cmake sets both, but breaks if the user pre-seeds only the singular form. Also, USE_PYTHON_INCLUDE_DIR silently no-ops on the FindPython3 path since Python3_INCLUDE_DIRS matches neither branch.

4. eval.h: Python 2 leftover coding cookie

Every py::eval/py::exec prepends "# -*- coding: utf-8 -*-\n" to the source. Python 3's PyRun_String already assumes UTF-8, so this is removable — dropping it removes a string copy and stops shifting line numbers by one in tracebacks/SyntaxErrors from evaluated code.

5. Small cleanups in setup_helpers.py

• cpp_flag_cache = None is dead code (caching is the @lru_cache on auto_cpp_level); the comment above it is stale too.
• def no_recompile(obg: ...) — obg is a typo for obj.

---

Planned draft PRs (each referencing this issue):

• fix(stl_bind): negative-step slice deletion + re-enable contiguous fast path, with tests — fix(stl_bind): correct __delitem__ for negative-step slices and re-enable contiguous erase fast path #6088
• fix: data race on last_storage_ptr_ in gil_safe_call_once_and_store — fix: data race on last_storage_ptr_ cache in gil_safe_call_once_and_store #6087
• fix(cmake): stale PYTHON_MODULE_DEBUG_POSTFIX cache + USE_PYTHON_INCLUDE_DIR variable mismatch — fix(cmake): unset stale PYTHON_MODULE_DEBUG_POSTFIX and correct USE_PYTHON_INCLUDE_DIR variable #6086
• chore: remove Python 2 coding cookie from eval.h — fix: drop Python 2 coding cookie prepended to evaluated source #6089
• chore: minor setup_helpers.py cleanups — chore(setup_helpers): remove dead cpp_flag_cache and fix parameter typo #6085

[henryiii]

🤖 AI text below 🤖

All done — issue filed and five draft PRs opened against pybind/pybind11, all referencing it:

Issue: #6084 — full findings write-up, with the checklist now linking each PR.

Draft PRs:

PR	Change	Model
#6088	fix(stl_bind): negative-step __delitem__ corruption/UB fix + re-enabled contiguous erase fast path, with a new test_vector_delitem_slice comparing against Python list semantics across lengths 0–7	opus
#6087	fix: last_storage_ptr_ made std::atomic<T*> and get_stored() reordered to validate before loading the cached pointer (the finalize/re-init staleness item is explicitly left for discussion on the issue)	opus
#6086	fix(cmake): unset stale PYTHON_MODULE_DEBUG_POSTFIX on interpreter change; USE_PYTHON_INCLUDE_DIR now uses the variable it checks and gained a Python3_INCLUDE_DIRS branch	sonnet
#6089	chore(eval): dropped the Python 2 coding cookie — verified tracebacks from py::exec now report the correct line number (was off by one)	sonnet
#6085	chore(setup_helpers): removed dead cpp_flag_cache, fixed obg → obj typo	sonnet

Every PR was verified before pushing: #6088 and #6089 by compiling scratch modules against the branch headers and exercising the behavior, #6087 by syntax-checking the instantiated template under Python 3.14 (subinterpreter branch active), #6086 by a clean CMake configure, and #6085 via prek + import check. All commits follow conventional commits with the Assisted-by: ClaudeCode:claude-fable-5 trailer, and PR bodies carry the 🤖 prefix. Branches are pushed to your henryiii fork; I removed the agent worktrees since everything is safely on the remote.

One heads-up for review: #6088 switched __delitem__ to the signed ssize_t overload of slice::compute rather than juggling wrapped size_t arithmetic — worth a close look since it's the one behavior-critical change, though it passed an exhaustive slice-case comparison against list semantics.