Claude Fable review of packaging #1239

I ran a review of packaging with Fable; I've had great success with these so far, the highest signal/noise ratio I've seen for an automated review.

🤖 AI text below 🤖

The review is done — six parallel agents covered the codebase and docs, and I independently reproduced every high-severity claim. Here's the full report, most important first.

Release-blocking bug (unreleased regression)

_ranges.py:106-122 — _BoundaryVersion ordering is wrong across post-release families. When two boundaries have different base versions, comparison falls back to plain Version ordering, but AFTER_POSTS(1.0) actually sits above 1.0.post1's entire family. The wrong ordering reaches max() in intersect_ranges, widening intersections. Verified on main (26.2 returns the correct result, so this came in with #1120, unreleased):

• SpecifierSet(">1.0,!=1.0.post1").contains("1.0.post2") → True (must be False; per-spec conjunction and 26.2 both say False)
• SpecifierSet(">1.0,<=1.0.post1").is_unsatisfiable() → False (it's empty)

Fuzzing found 28 divergences vs the all(spec.contains(...)) ground truth, all of the form >V ∧ !=V.postN. Relatedly, _ranges.py:213-219's AFTER_POSTS else-branch in _UpperBound (pragma: no cover) would be semantically wrong if ever reached — worth an assert instead of a silent fallback.

Other bugs (all verified by execution)

markers.py

• markers.py:172-177 — _normalize_extra_values only normalizes top-level atoms, so parenthesized extra == clauses never get PEP 685 normalization: Marker('(extra == "Foo_Bar" or extra == "Baz") and python_version >= "3"').evaluate({"extra": "foo-bar"}) → False (should be True). The eval-time _normalize skips work on the false premise that "both sides are normalized already". Reachable via Requirement markers too. -> PR fix: normalize nested extra marker values #1246
• markers.py:156-169 — if the other side of an extra comparison is a Variable, it's destructively rewritten into a literal: Marker('os_name == extra') becomes '"os-name" == extra' and evaluates against the literal string.
• markers.py:278 — set-valued lockfile variables on the LHS hit a bare AssertionError: Marker("dependency_groups == 'foo'").evaluate(context="lock_file"). -> PR fix(markers): raise UndefinedComparison for set-valued variables used outside the membership form #1265
• markers.py:208-217 — side effect of #939: === on non-version keys now raises UndefinedComparison (Marker("os_name === 'posix'").evaluate()); ≤25.0 evaluated it as string equality. Untested either way — decide and pin it. -> PR test(markers): pin === (arbitrary equality) evaluation on non-version keys (#1239) #1279
• _parser.py:372-374 — ast.literal_eval failures escape as raw SyntaxError instead of InvalidMarker/InvalidRequirement: Marker('os_name == "C:\\"'). Invalid escape sequences also emit SyntaxWarning, which is an error under pytest's filterwarnings = ["error"]. -> PR fix(markers): wrap malformed quoted strings as public parse errors #1249

requirements.py

• requirements.py:139-152 — __eq__ compares canonicalized SpecifierSets but __hash__ hashes raw strings: Requirement("pkg==1.0") == Requirement("pkg==1.0.0") is True with different hashes, breaking set/dict usage. -> PR fix(requirements): make Requirement.__hash__ consistent with __eq__ for trailing-zero-equivalent specifiers #1232

metadata.py

• metadata.py:631-639 — a legally folded Description-Content-Type: header escapes from_email() as a bare ValueError ("Header values may not contain linefeed...") from the EmailMessage assignment used for validation, bypassing the error collector entirely. -> PR fix(metadata): aggregate from_email validation errors #1268
• metadata.py:225-241 — a multipart/mixed METADATA payload raises bare AssertionError (and under -O returns a list of Message objects as the description). -> PR fix(metadata): route multipart payloads to unparsed instead of asserting #1247
• metadata.py:846-860 — from_email raises the "unparsed" group before running validation, so invalid-field errors are silently dropped when both kinds exist, despite the docstring's "all exceptions will be gathered". -> PR fix(metadata): aggregate from_email validation errors #1268

dependency_groups.py

• dependency_groups.py:260 — _parse_group caches partial results even when errors were collected, so the second lookup() of a bad group silently returns () where the first raised (verified). Contradicts the tested re-raise invariant for cycles. -> PR Fix dependency group malformed parse caching #1248
• dependency_groups.py:255-256 — {include-group = 5} (valid TOML) escapes as raw TypeError from _normalize_name instead of a collected validation error. -> PR Fix dependency group malformed parse caching #1248

licenses/init.py

• LicenseRef-custom+ raises bare KeyError (the dict is keyed with the +, lookup strips it). Per PEP 639/SPDX grammar, + isn't allowed on a LicenseRef at all, so the fix is to reject with InvalidLicenseExpression. Leaks through Metadata.license_expression too. -> PR fix: raise correct exception on invalid license #1219
• WITH chaining and non-simple left operands are accepted (mit WITH X WITH X, (mit or apache-2.0) WITH X) — all invalid SPDX. -> PR fix: reject more invalid license expression forms #1266
• LicenseRef- (empty idstring) is accepted; the validation regex uses * instead of +. -> PR fix: reject more invalid license expression forms #1266

tags.py

• tags.py:385 — the explicit-ABI dedup tuple ("abi3", "none") was never updated for abi3t: passing abis=['cp315t','abi3t'] yields cp315-abi3t-plat twice (verified), and abis=['abi3t'] alone emits an inconsistent non-threaded abi3 chain afterward. -> PR fix: duplicate explicit abi3t tags #1245
• tags.py:898 — sys_tags(warn=True) doesn't forward warn to generic_tags(), silently dropping warnings on PyPy/GraalPy. -> PR fix(tags): forward warn to generic_tags() in sys_tags #1264
• tags.py:248 — parse_tag("py3-none") raises bare unpacking ValueError instead of the new 26.3 InvalidTag. -> PR fix(tags): raise InvalidTag for malformed tag field counts #1238
• tags.py:437 — empty-string EXT_SUFFIX raises IndexError instead of the intended SystemError. -> PR fix(tags): raise SystemError (not IndexError) for empty EXT_SUFFIX in _generic_abi #1271
• _musllinux.py:87 — the __main__ debug block passes a str where Sequence[str] is expected, iterating characters and printing garbage tags. -> PR fix(_musllinux): pass archs as a list in the __main__ debug block #1280

version.py / pylock.py / direct_url.py / tooling

• version.py:1072-1081 — _TrimmedRelease copies _key_cache but not _hash_cache; hash() raises AttributeError (latent, internal-only). -> PR fix(version): set _hash_cache when copying a Version in _TrimmedRelease #1242
• version.py:265-313 — Version.from_parts accepts bool for int fields (post=True → unparseable '1.postTrue'), and a non-str pre letter escapes as AttributeError instead of InvalidVersion. -> PR fix(version): raise InvalidVersion for non-str pre letters in from_parts #1241
• pylock.py:104 — _get accepts bool for int fields (size = true validates); pylock.py:593-605 — empty wheels = [] defeats the mutual-exclusivity check against vcs/directory/archive; pylock.py:796 — select() KeyErrors on environments missing python_full_version despite adjacent code anticipating partial environments. -> PR fix(pylock): reject bools where TOML integers are expected #1244
• direct_url.py:286-290 — file:// scheme check is case-sensitive and rejects RFC 8089 file:/path. -> PR fix(direct_url): parse URL scheme case-insensitively when checking for file URLs #1240
• noxfile.py:324 — nox -s release_build -- 26.2 has never worked: git switch <tag> without --detach fails. Also release_build isn't default=False, so bare nox runs it and errors on non-tagged commits. And asv.conf.json:4 has https//github.com (missing colon). -> PR chore(noxfile): detach on tag checkout and mark release_build non-default; fix asv URL typo #1243

Performance

• markers.py:462 — Marker.evaluate rebuilds default_environment() every call (~25% of evaluate time measured); the values are process-constant. -> PR perf(markers): cache the default environment #1250
• markers.py:222 — _eval_op re-parses Specifier(f"{op}{rhs}") on every evaluation; an lru_cache would eliminate it on the resolver hot path. -> PR perf(markers): cache Specifier parsing in _eval_op #1251 (Henry: I didn't like adding a cache here)
• specifiers.py:556 — Specifier.__hash__ recomputes _canonical_spec each call (~13× slower than Version's cached hash); Marker.__hash__/Requirement.__hash__ similarly re-serialize the whole tree. All three are immutable — cache like Version does (PR #1118). -> PR perf: cache __hash__ on Specifier, Marker, and Requirement #1252
• specifiers.py:843 — _canonical_specs resets the _is_unsatisfiable/_ranges caches even though canonicalization is semantics-preserving, so str()/hash()/== after contains() throws away the expensive range intersection. -> PR perf(specifiers): keep range caches across canonicalization #1253
• _manylinux.py:191 — import _manylinux runs inside the per-arch × per-glibc loop; failed imports aren't cached by sys.modules, costing ~1-2 ms per full sys_tags() on typical systems. Resolve once before the loop. -> PR perf(manylinux): cache _manylinux module lookup process-wide #1254
• utils.py:222 — one inline re.match(r"^[\w\d._]*$", ...) where every sibling pattern is precompiled (\d is also redundant inside \w). -> PR perf(utils): precompile the wheel project-name pattern #1256
• tags.py:619 — mac_platforms calls platform.mac_ver() (re-reads SystemVersion.plist) even when both version and arch were passed, the typical cross-target case. -> PR perf(tags): skip platform.mac_ver() when version and arch are given #1255

Simplifications

• pylock.py + direct_url.py — ~150 lines duplicated nearly verbatim (_get, _get_required, _get_object, and character-for-character identical error classes). A shared private module would keep them from drifting — the bool-rejection fix above currently needs making twice.
• tasks/check.py is dead: it calls PyPI XML-RPC methods removed years ago and depends on pkg_resources/invoke. Removing it also drops tasks/__init__.py, tasks/requirements.txt, the types-invoke mypy dep, and a ruff per-file-ignore. -> PR chore(tasks): remove dead tasks/check.py and its invoke wiring (closes #827) #1275
• Smaller: duplicate if version >= (11, 0): blocks in mac_platforms (tags.py:652/662); dead py2 guard in _manylinux.py:144; _GLibCVersion(*...) re-wrap at _manylinux.py:240; set(parsed.extras or []) dead or [] (requirements.py:70); double str.lower pass in _process_dynamic (metadata.py:664); wrong struct-format comments in _elffile.py:60-85 (parsing itself verified correct); unused misspelled EMachine.AArc64; expect() docstring says the token is not read but it returns self.read() (_tokenizer.py:138). -> PR refactor: small cleanups from the #1239 review #1281 (the two _manylinux.py items are false positives — exercised by tests; see PR)

Modernizations

Little to do — slots, cached hashes, and prebound predicates are already in good shape. Two notes:

• Token (_tokenizer.py:14) is a slotless dataclass allocated per token; NamedTuple works on 3.9. -> PR perf: add __slots__ to token classes #1258
• The os.PathLike comment in _manylinux.py:32 is stale now that the floor is 3.9. -> PR fix(_musllinux): pass archs as a list in the __main__ debug block #1280

Docs

• Marker.evaluate documents :raises UndefinedEnvironmentName: but the code raises KeyError (tests pin KeyError); the exception class hasn't been raised since 22.0 and docs/markers.rst:16 imports it pointlessly. Decide: either raise it or remove it from docs/__all__. -> PR fix(markers): correct Marker.evaluate :raises: from docs (and subclass KeyError for backcompat) #1276
• Marker.__and__ is missing entirely from the rendered HTML and Marker.__or__ renders with type.__or__'s "Return self|value." docstring — autodoc drops docstring-less special members. Add real docstrings (with .. versionadded:: 26.1). -> PR docs(markers): add docstrings to Marker.__and__/__or__ so they render in the API reference #1274
• __all__ exports invisible in rendered docs for lack of docstrings: specifiers.BaseSpecifier, utils.BuildTag, tags.AppleVersion, tags.PythonVersion. -> PR docs: render __all__ exports BuildTag/BaseSpecifier/AppleVersion/PythonVersion in the API reference #1277
• docs/requirements.rst:71 documents Requirement(requirement) but the parameter is requirement_string — the documented keyword call raises TypeError. -> PR docs(requirements): fix Requirement constructor parameter name in Reference #1272
• Missing annotations: parse_wheel_filename lacks the 26.3 versionchanged that parse_tag got for the same PR (#1234); parse_sdist_filename's :raises: list omits the invalid-version case; METADATA 2.6 (PEP 808) support has no version note. -> PR docs(utils): add 26.3 versionchanged to parse_wheel_filename + complete parse_sdist_filename :raises #1273
• Otherwise verified clean: all 274 doctests pass, docs build clean under -W -n, all spot-checked versionadded annotations match git history.

What's verified clean: the version.py parse/compare core (differentially fuzzed against 26.2 across ~7k combinations — zero divergences outside the _ranges bug), the specifier fast paths, ELF parsing, manylinux/musllinux semantics, macOS/iOS/Android tag ordering, URL/extras parsing in _parser.py, and _structures.py/errors.py.

The one thing I'd fix before anything else is the _ranges.py boundary-ordering bug — it's an unreleased correctness regression in SpecifierSet.contains/filter, exactly the hot path resolvers depend on.