Skip to content

fix(pylock): two PEP 751 lockfile validation fixes#1244

Open
henryiii wants to merge 3 commits into
pypa:mainfrom
henryiii:fix/pylock-validation
Open

fix(pylock): two PEP 751 lockfile validation fixes#1244
henryiii wants to merge 3 commits into
pypa:mainfrom
henryiii:fix/pylock-validation

Conversation

@henryiii

@henryiii henryiii commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Item 3 has been removed.

🤖 AI text below 🤖

Part of #1239.

This fixes three related PEP 751 lockfile validation bugs surfaced in the code review for #1239. Each fix is a separate commit with a regression test, and src/packaging/pylock.py stays at 100% coverage.

1. Booleans accepted where TOML integers are expected

_get used isinstance(value, expected_type), and because bool is a subclass of int, a boolean passed the integer check. TOML distinguishes the two types, so a lockfile could set an integer field to a boolean:

>>> from packaging.pylock import _get
>>> _get({"size": True}, int, "size")
True   # before: accepted; PackageWheel.size becomes True

_get now rejects a boolean when the expected type is int, raising the usual PylockValidationError with field context (following the existing str-vs-Sequence special case):

Unexpected type bool (expected int) in 'packages[0].wheels[0].size'

2. Empty wheels = [] array defeated the mutual-exclusivity check

The check counted distributions with len(package.wheels or []), so an empty wheels = [] array alongside a direct URL source validated, even though [[packages.wheels]] is mutually exclusive with vcs/directory/archive:

>>> from packaging.pylock import Package
>>> Package._from_dict({"name": "foo", "wheels": [], "directory": {"path": "./foo"}})
# before: validated; after: PylockValidationError

The check now treats the presence of the key (package.wheels is not None) as wheels being specified. An empty wheels = [] on its own is likewise treated as a specified distribution and validates without requiring a direct URL source.

3. select() raised KeyError on partial environments

Pylock.select() indexed environment["python_full_version"] directly, so a partial environment dict missing that key raised a bare KeyError, even though the adjacent code (dict(environment or {})) anticipates partial environments and lets Marker.evaluate fill defaults:

>>> list(lock.select(environment={"sys_platform": "linux"}))
KeyError: 'python_full_version'   # before

The lookup now falls back to markers.default_environment()["python_full_version"] when the key is missing.

🤖 Generated with Claude Code

@henryiii henryiii requested a review from sbidoul June 10, 2026 22:36
@sbidoul

sbidoul commented Jun 11, 2026

Copy link
Copy Markdown
Member
  1. bool vs int. ok, good catch.
  2. you're nitpicking, Claude :). ok, for me. But I'd like confirmation from @brettcannon that we want to reject these, as it could be seen as a matter of spec interpretation.
  3. I'm not sure a change is necessary. This PR adds a comment saying a caller may pass a partial environment, but that goes against the function signature which says it must be an Environment, which can't be partial. And the test actively bypasses type checking with a cast. So, no? If the user does not respect the API contract then they must expect things to fail in a not nice way.

henryiii added 2 commits June 11, 2026 23:16
Because bool is a subclass of int in Python, isinstance(True, int) is true,
so the _get helper accepted a boolean wherever an integer was expected. This
meant a lockfile with, for example, size = true would validate and set
PackageWheel.size to True. TOML distinguishes booleans from integers, so _get
now rejects a boolean when the expected type is int, raising the usual
PylockValidationError with the field context, following the existing
str-versus-Sequence special case.

Assisted-by: ClaudeCode:claude-opus-4-8
…y check

The mutual-exclusivity check counted distributions using the number of wheels,
so an empty wheels = [] array alongside vcs, directory, or archive incorrectly
validated even though [[packages.wheels]] is mutually exclusive with those
direct URL sources. The check now treats the presence of the wheels key as
wheels being specified, regardless of how many entries it contains. As a
result, an empty wheels array on its own is also treated as a specified
distribution and validates without requiring a direct URL source.

Assisted-by: ClaudeCode:claude-opus-4-8
@henryiii henryiii force-pushed the fix/pylock-validation branch from cf170d6 to 1211ec3 Compare June 12, 2026 03:17
@henryiii

Copy link
Copy Markdown
Contributor Author

I dropped commit 3 and shortened a comment. Claude seems convinced that environment or {} means a partial environment is allowed. I don't think that follows.

@henryiii henryiii changed the title fix(pylock): three PEP 751 lockfile validation fixes fix(pylock): two PEP 751 lockfile validation fixes Jun 12, 2026

@brettcannon brettcannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the model is being unnecessarily pedantic for change 2.

Comment thread src/packaging/pylock.py Outdated
tool=_get(d, Mapping, "tool"), # type: ignore[type-abstract]
)
distributions = bool(package.sdist) + len(package.wheels or [])
# The presence of the wheels key (even as an empty array) counts as wheels

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes no sense to me. I don't see why having an empty [[packages.wheels]] array just to exclude other tables makes sense. If you don't want the other tables then don't include them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm walking this back. FWIW, I think this version would have been easier to validate with a schema; it's easier to indicate that you need one key of several than to special case an empty array key as not counting as a key (it can be done via JSON Schema, but it's more steps).

@henryiii henryiii requested a review from brettcannon July 2, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants