AI review (take with grain of salt!)

[henryiii]

I'm busy at PyCon, dropping this here. From Kimi-K2.6:

🤖

Here's my comprehensive review of the repository, organized by category:

---

🔴 Bugs / Logic Errors

File	Issue	Details
src/scikit_build_core/build/wheel.py:216	.format() on user-controlled string	settings_reader.settings.messages.after_failure.format() — if the message contains { or }, it raises KeyError/IndexError, masking the original error. Fix: remove .format()
src/scikit_build_core/builder/builder.py:234,241	Missing format argument in logger	logger.info("py-api {} requires CPython, ignoring") — {} has no argument; raises IndexError on PyPy. Fix: pass py_api
src/scikit_build_core/cmake.py:90	Mutable class-level default	single_config: bool = not sysconfig.get_platform().startswith("win") evaluated once at import time; stale after mocks. Fix: use field(init=False) + __post_init__
src/scikit_build_core/settings/skbuild_read_settings.py:220-222	Unsafe dict access	pyproject["build-system"]["requires"] crashes with KeyError if [build-system] is missing. Fix: use .get("build-system") with helpful error
src/scikit_build_core/settings/sources.py:772,785	Overly broad except Exception	Catches AttributeError etc. from programming bugs. Fix: narrow to TypeError, ValueError

---

🟠 Performance Issues

File	Issue	Recommendation
src/scikit_build_core/build/wheel.py:410-468	Dead build_args variable	Always empty list; remove it
src/scikit_build_core/builder/builder.py:132-145	Entry points loaded 3× per configure	Cache _get_entry_point_search_path with @lru_cache
src/scikit_build_core/builder/builder.py:326-328	get_archs recomputed	Already computed in wheel.py; pass the result instead
src/scikit_build_core/program_search.py:41-59	_macos_binary_is_x86 subprocess overhead	Cache with canonicalized Path
src/scikit_build_core/program_search.py:129-173	Duplicate subprocess for CMake version	Consider defaulting to --version if JSON path often fails
src/scikit_build_core/program_search.py:186-212	Ninja version probe uncached	Add @lru_cache(None)
src/scikit_build_core/settings/skbuild_read_settings.py:262-264	Settings conversion runs twice	Reuse intermediate TOML-derived object

---

🟡 Simplifications & Best Practices

File	Issue	Recommendation
pyproject.toml:48-50	Empty [project.optional-dependencies] pyproject	Remove the empty group
pyproject.toml:143-148	dev group redundantly includes test	Since cov → test → test-core, remove { include-group = "test" } from dev
src/scikit_build_core/cmake.py	8× repeated .replace("\\", "/")	Extract _cmake_path_str() helper
src/scikit_build_core/builder/builder.py:66,289-292	Inconsistent platform detection	Standardize on sys.platform.startswith("win")
src/scikit_build_core/program_search.py:215-224	get_make_programs not in __all__	Add it or prefix with _
src/scikit_build_core/build/sdist.py:115	copy.deepcopy(metadata) workaround	Check if upstream bug (python-pyproject-metadata#49) is fixed; remove if so

---

🟢 CI / Packaging Best Practices (from sp-repo-review)

Passed all checks — sp-repo-review reports zero failures. The repo is well-configured for Scientific Python standards. Minor observations:

Observation	Note
FORCE_COLOR	Already set in workflows
uv backend	Nox correctly uses uv|virtualenv
Dependency groups	Properly structured (just redundant test in dev)
Pre-commit	prek is used; good

[henryiii]

🤖 AI text below 🤖

Done — I checked all five items in the 🔴 Bugs/Logic Errors table against current main.

Verdict: 2 of 5 were real bugs, and both are already fixed. The other 3 don't need a fix.

#	Item	Valid?	Status
1	wheel.py .format() on after_failure	✅ Real bug	Already fixed in #1311 (3ecfcbd). The retry path called .format() on the message even though rich_print/err.msg already handles it, so a {/} in the message would raise only on retry. The redundant .format() was dropped.
2	builder.py py-api {} log missing arg	✅ Real bug	Already fixed in #1305 (f7231e0). The line was genuinely introduced without its argument in #1269 (logger.info("py-api {} requires CPython, ignoring")), and since the logger calls .format(), it would have raised IndexError on non-CPython. Now reads ..., py_api).
3	cmake.py:90 "mutable class-level default"	❌ Not a bug	The claim is wrong: single_config is an immutable bool, not a mutable default, and it's recomputed from the real generator in configure() (cmake.py:274). The import-time value is just a fallback. The "stale after mocks" point is a test-only nuance, not a runtime bug. No fix warranted.
4	skbuild_read_settings.py:263 unsafe pyproject["build-system"]["requires"]	⚠️ Borderline	Only reachable when a user sets minimum-version = "build-system.requires". In that situation [build-system].requires is practically always present (it's how this backend got invoked), and the very next lines already emit a helpful error if no version pin is found. A missing-table KeyError is a negligible edge case — a defensive .get() would be a minor polish, not a needed fix.
5	sources.py:769,782 broad except Exception	❌ Intentional	Both carry # noqa: BLE001. The broad catch is deliberate error aggregation — it attaches a Field: <name> note to each conversion failure and collects them for a combined report. Narrowing to TypeError, ValueError would silently drop other converter errors. Working as designed.

Bottom line: The only two genuinely valid findings (items 1 and 2) were both caught and fixed in the recent repo-sweep PRs (#1305, #1311). Nothing in this table still needs a fix.

Worth noting the model also got a fact wrong on item 3 ("mutable class-level default" — it's an immutable bool), which is a good reminder of the "grain of salt" in the issue title.