fix: file_api/ast/metadata bug fixes and simplifications#1319
Merged
Conversation
The argument-accumulation loop appended COMMENT and BRACKET_COMMENT token values into the argument string, so a trailing comment inside `cmake_minimum_required(VERSION 3.15 # why not)` produced the value `"3.15 # why not"`, which raised an uncaught InvalidVersion during settings parsing. Comment tokens are now skipped (a bracket comment's regex also swallows leading whitespace, so it is replaced with a single space to keep tokens separated). Malformed input (a missing open paren or an unterminated block) now raises a dedicated `ParseError` instead of a bare AssertionError/IndexError. The auto-cmake-version call site in skbuild_read_settings.py catches it and falls through to the existing graceful warning path with a 3.15 fall-back. Also remove the unreachable second alternative of the LEGACY tokenizer regex, which was a strict subset of the preceding QUOTED alternative. Assisted-by: ClaudeCode:claude-opus-4-8
…gments The toolchains-v1 reply is requested and modeled, but neither converter followed its `jsonFile` indirection, so the populated toolchains-v1-*.json was never read and the index silently structured into an empty `Toolchains` stub. `Toolchains` is now added to the jsonFile-following set in both the built-in and cattrs converters, and the test asserts the actual toolchain content loads. `Link.commandFragments` is optional per the CMake File API spec (its sibling `Archive.commandFragments` already defaults), but lacked a default, so a target whose link object omitted it failed the whole codemodel conversion. It now defaults to an empty list. Also simplify `if origin is not None and origin is list` to `if origin is list`. Assisted-by: ClaudeCode:claude-opus-4-8
The regex and template plugins used a proper-superset check (`settings.keys() > KEYS`), which only fired when the user supplied every valid key plus an extra; a typo'd key passed silently and was ignored. Switched to `settings.keys() - KEYS` so any unrecognized key raises. The `authors`/`maintainers` validation called `.items()` on each list element before checking it was a dict, so `["string"]` raised AttributeError instead of the intended RuntimeError; the dict check is now performed first and the message corrected to "list of dictionaries of strings". The optional-dependencies check now also verifies list elements are strings. Assisted-by: ClaudeCode:claude-opus-4-8
CMake < 3.20 does not support the toolchains-v1 File API object, so it returns an error reply (kind/version/error) with no jsonFile to follow. The cattrs converter indexed with_path["jsonFile"] directly, raising KeyError on those versions; structure the inline dict instead, matching the built-in converter's "jsonFile" in data guard. Assisted-by: ClaudeCode:claude-opus-4.8
90ba5b7 to
04118f1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Autogenerated from #1317.
🤖 AI text below 🤖
This fixes a reviewed batch of bugs and simplifications in
file_api/,ast/,and
metadata/, each with a regression test. All findings were verified againstthe code before fixing; none were skipped.
Fixes
metadata/regex.py+metadata/template.py— wrong superset operator.if settings.keys() > KEYS:is a proper-superset check, so it only firedwhen the user supplied all valid keys plus an extra; a typo'd key (e.g.
removes) passed silently and was ignored. Changed toif settings.keys() - KEYS:in both. Added regression tests that a bogus keyraises.
file_api/reply.py+file_api/_cattrs_converter.py— toolchainsjsonFilenever followed. The toolchains-v1 reply is requested andmodeled, but
make_class/the cattrs converter only followedjsonFilefor{CodeModel, Target, Cache, CMakeFiles}.
Toolchainswas missing, so thepopulated
toolchains-v1-*.jsonwas never read and silently structured intoan empty
Toolchainsstub. AddedToolchainsto the jsonFile-following setin both converters; strengthened
test_included_dirto assert the actualtoolchain content loads (and that both converters agree).
file_api/model/codemodel.py—Link.commandFragmentshad no default.Per the CMake File API spec it's optional (sibling
Archive.commandFragmentsalready defaults), so a target whose link object omitted it failed the whole
codemodel conversion. Gave it the same
default_factory=list. Added a teststructuring a minimal link dict without
commandFragments.ast/ast.py— comments leaked into argument values. Theargument-accumulation loop appended COMMENT / BRACKET_COMMENT token values,
so
cmake_minimum_required(VERSION 3.15 # why not\n)yielded value"3.15 # why not", which flowed intoVersion(...)/SpecifierSet(...)andraised uncaught
InvalidVersionduring settings parsing. Comment tokens arenow skipped; since a bracket comment's regex also swallows leading
whitespace, it is replaced with a single space to keep tokens separated.
Added
find_min_cmake_versionregression tests with in-parens comments(including a leading bracket comment).
ast/ast.py+settings/skbuild_read_settings.py— opaque traceback onmalformed CMakeLists. An unterminated block made
contents[-1]raise abare
IndexError, and the missing-open-paren case raisedAssertionError;the call site only caught
FileNotFoundError, so a CMakeLists.txt the parserchokes on produced an opaque traceback instead of the graceful fallback the
surrounding code anticipates. Added a dedicated
ParseErrorraised by theast module and caught at the
find_min_cmake_versioncall site, which nowfalls through to the existing warning path. Added a settings test.
metadata/__init__.py—authors/maintainersvalidation crashed on badinput. The
_LIST_DICT_FIELDScheck called.items()on each list elementbefore checking it was a dict, so
["string"]raisedAttributeErrorinstead of the intended
RuntimeError; the message also said "dictionary ofstrings" but should be "list of dictionaries of strings". Both fixed. Also
tightened the optional-dependencies check to verify list elements are
strings. Added regression tests.
ast/tokenizer.py— dead LEGACY regex alternative removed. The secondalternative (
|"(?:[^"\\]*(?:\\.[^"\\]*)*)*") was unreachable: the precedingQUOTED alternative matches a strict superset at every position. Removed it;
ast tests confirm no behavior change.
file_api/reply.py— simplifiedif origin is not None and origin is list:toif origin is list:.Tests
uv run pytest tests/test_cmake_ast.py tests/test_fileapi.py tests/test_dynamic_metadata_unit.py tests/test_skbuild_settings.py tests/test_dynamic_metadata.py tests/test_auto.py -q -n auto→ 131 passed.prek -a --quiet: ruff, ruff-format, and mypy pass. (Thecheck-sdisthookflags only pre-existing untracked
uv.lock/.claude/settings.local.jsonworktree artifacts, unrelated to this change.)
🤖 Generated with Claude Code