Skip to content

fix(metadata): aggregate from_email validation errors - #1268

Merged
henryiii merged 10 commits into
pypa:mainfrom
r266-tech:r266-metadata-from-email-validation-aggregation
Jul 20, 2026
Merged

henryiii merged 10 commits into
pypa:mainfrom
r266-tech:r266-metadata-from-email-validation-aggregation

Conversation

@r266-tech

Copy link
Copy Markdown
Contributor

Summary

  • collect unparsed email-header errors together with Metadata.from_raw() validation errors in Metadata.from_email(validate=True)
  • keep duplicate required-header diagnostics to one stable email-field error
  • reject malformed Description-Content-Type values that the email parser reports through assignment errors or header defects

Tests

  • PYTHONPATH=src python3 -m pytest tests/test_metadata.py -q
  • python3 -m compileall -q src/packaging/metadata.py tests/test_metadata.py
  • git diff --check -- src/packaging/metadata.py tests/test_metadata.py

Part of #1239.

@r266-tech

Copy link
Copy Markdown
Contributor Author

Pushed a follow-up that fixes the red CI issues from the first commit.

What changed:

  • narrowed the ExceptionGroup.exceptions handling so mypy can prove InvalidMetadata.field access is safe
  • preserved the lone InvalidMetadata compatibility path when there are no email-parser errors to aggregate
  • added regression coverage for valid raw metadata with unparsed headers, non-InvalidMetadata grouped errors, single validation errors, validate=False wrapping, and description content-type validation without header defects
  • removed the unreachable post-filter branch that left branch coverage below 100%

Checks run locally:

  • PYTHONPATH=src python3 -m pytest tests/test_metadata.py -q
  • uvx nox -s lint -- --show-diff-on-failure
  • NOXFORCEPYTHON=3.11 uvx nox -s tests

Also ran the required adversarial review on the final diff; final verdict was approve.

@henryiii

henryiii commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Thanks for the fix! I pushed 8c2f9bc, which simplifies the aggregation logic while keeping behavior identical:

  • from_raw(validate=True) routes every error through _ErrorCollector.finalize(), so it can only raise ExceptionGroup, never a bare InvalidMetadata. The except InvalidMetadata branch was unreachable, so it is removed along with the two tests that monkeypatched from_raw to trigger it.
  • The required-field dedup condition reduces to exc.field in unparsed and _EMAIL_TO_RAW_MAPPING.get(exc.field) not in raw: from_raw only reports fields that are in raw or required, so an error for a field absent from raw is necessarily a missing-required-field complaint. That makes the precomputed required_email_fields / unparsed_raw_fields sets unnecessary.
  • The repeated message f-string in _process_description_content_type is hoisted into a variable.

Net -71 lines; all the new integration-style tests pass unchanged.

@notatallshaw

Copy link
Copy Markdown
Member

Claude Opus 4.8 took a look at the new Description-Content-Type handling; the first point below is a regression on input that parsed before this PR.

🤖 AI text below 🤖

Both new rejection paths in _process_description_content_type can raise a bare exception that isn't an InvalidMetadata, so it escapes from_email instead of joining the aggregated group.

Review comment:

  • [P2] Braces in the value crash format_map (src/packaging/metadata.py:640-642)
    The .defects branch passes invalid_msg to _invalid_metadata, which runs msg.format_map({"field": ...}) (line 591). invalid_msg embeds {value!r}, so a { in the header value is read as a format field. Description-Content-Type: text/plain; {b} was accepted before this PR and now raises a bare KeyError('b'); text/plain; a}b raises ValueError. Fixing it in _invalid_metadata, e.g. msg.replace("{field}", repr(self.raw_name)), covers every message that interpolates a value.

  • [P3] Assigning the content-type can raise IndexError, not only ValueError (src/packaging/metadata.py:636-638)
    An RFC 2231 parameter name ending in * triggers it: Description-Content-Type: text/plain; x* raises IndexError: string index out of range, which the new except ValueError misses, so it escapes from_email bare. except (ValueError, IndexError) catches it.

@henryiii

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Thanks! Both fixed in 8ab2fda:

  • Braces in the value: _invalid_metadata now interpolates the field name with str.replace instead of format_map, as suggested, so braces in any interpolated value can no longer crash message formatting — this covers every field's error messages, not just Description-Content-Type. Note that text/plain; {b} is still rejected (it has header defects, which this PR now checks), but as a proper InvalidMetadata inside the aggregated group rather than a bare KeyError.
  • IndexError from RFC 2231 parameters: the header assignment now catches (ValueError, IndexError), so text/plain; x* joins the group as well.

Regression tests for all three inputs were added on both the from_email aggregation path and the lazy validate=False path, and confirmed to fail before the fix.

@henryiii

Copy link
Copy Markdown
Contributor

GPT 5.6 Sol:

🤖 AI text below 🤖

The validation improvements generally work, but the new placeholder substitution corrupts diagnostics for metadata values containing the literal {field} token.

Review comment:

  • [P2] Avoid replacing placeholders inside user values — /Users/henryfs/git/pypa/packaging/src/packaging/metadata.py:592-592
    When an interpolated metadata value itself contains the exact text {field}, this global replacement also modifies that user value. For example, an invalid version named {field} is reported as 'version' is invalid for 'version' instead of preserving the submitted value, making validation diagnostics inaccurate. Substitute the field placeholder before incorporating user-controlled values, or otherwise replace only the intended placeholder.

@henryiii

Copy link
Copy Markdown
Contributor

I can actually pull out that change from this PR, would be easier to review.

@henryiii
henryiii marked this pull request as draft July 11, 2026 03:15
@henryiii

Copy link
Copy Markdown
Contributor

Dropping a Fable adversarial review here, but I'm going to see about the commit above being a separate PR first.

🤖 Adversarial review 🤖

Adversarial review of r266-metadata-from-email-validation-aggregation

Verdict: the core logic is solid — I couldn't break the aggregation. I stress-tested the dedup condition in from_email against every edge I could construct (duplicated required headers, description present in both header and body via undecodable payload, empty header values, unknown fields mixed with invalid ones, fields newer than Metadata-Version that are also duplicated) and it behaved correctly every time. The _EMAIL_TO_RAW_MAPPING.get(exc.field) not in raw guard correctly handles the one case where a key can legitimately be in both unparsed and raw (description via the payload-decode-failure path at metadata.py:488). I also confirmed a bonus fix you may not have realized the scope of: on main, _invalid_metadata's format_map ran over messages containing user-controlled values, so Description-Content-Type: text/markdown; variant={GFM} crashed with a bare KeyError: 'GFM' — the f-string rewrite fixes that injection entirely. Tests and prek pass.

That said, here's what I'd push back on:

1. The defects check silently tightens validation beyond the crash fix

Comparing main vs branch across a corpus, three inputs changed from accepted to rejected: text/plain; {b}, text/plain; a}b (both have tests, so presumably intended), and — collaterally — any duplicated parameter, e.g. text/markdown; variant=GFM; variant=CommonMark, which main accepted (first parameter won). Duplicate parameters are genuinely malformed per RFC 2045, so rejecting is defensible, but it's a user-visible tightening that isn't crash-related and isn't tested. Decide whether that's intended; if so, a test pinning it would prevent someone "fixing" it later.

2. Misleading error message on the defect path (metadata.py:642-644)

When the content type itself is valid but a parameter is malformed, the user gets 'description-content-type' must be one of ['text/plain', 'text/x-rst', 'text/markdown'], not 'text/markdown; variant=GFM; variant=CommonMark' — but text/markdown is one of them. The real reason (InvalidHeaderDefect: ...) is only in __cause__. Something like f"{value!r} is not a valid content type for {self.raw_name!r}" on the defect/exception paths would not lie to the reader.

3. Dead code covered by a monkeypatch-only test

The validate=False tail of from_email (metadata.py:881-885) wraps an ExceptionGroup that from_raw(validate=False) can never raise — it just copies the dict and returns. The covering test (test_from_email_validate_false_wraps_from_raw_groups) has to monkeypatch from_raw to force the path, which is a smell: it tests behavior that can't occur. I'd simplify to return cls.from_raw(raw, validate=validate) and delete that test; this also removes the duplicated "invalid or unparsed metadata" literal.

4. The {field} regression test doesn't cover the crash variant

test_invalid_version_with_placeholder_text uses the literal {field}, which on main produced wrong output but no crash. The severe manifestation was an unknown placeholder ({GFM}, {oops}) crashing with KeyError. Add one parametrized case like version="{oops}" or the real-world Description-Content-Type: text/markdown; variant={GFM} so the test fails loudly if format_map ever comes back.

Smaller nits

  • list(content_types) and list(markdown_variants) in error messages iterate sets, so message ordering varies with hash randomization run-to-run. Pre-existing, but since you touched these lines, sorted(...) would make errors deterministic (and testable by exact string).
  • cls.from_raw(raw, validate=validate) at metadata.py:864 is inside if validate: — just write validate=True.
  • Now that from_email("") surfaces required-field errors, the missing-Metadata-Version message reads "None is not a valid metadata version" — an unformatted repr(None). A if not value:"'metadata-version' is a required field" guard in _process_metadata_version would match name/version. Pre-existing message, but the new aggregation makes it far more visible.
  • Aside, out of scope: InvalidMetadata is unpicklable on main and branch alike (__reduce__ loses the two-arg __init__), which matters for multiprocessing users.

None of these are blocking; findings 2 and 3 are the ones I'd actually fix before merging.

@henryiii

henryiii commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

I pulled out the extra rejections to #1329, keeping this focused on just aggregations. That one is still needed to fix some of the leaking, though, so I think that should be considered first.

henryiii added a commit that referenced this pull request Jul 11, 2026
* fix(metadata): reject malformed Description-Content-Type values

Surface email-parser exceptions and header defects for
Description-Content-Type as InvalidMetadata instead of leaking
IndexError (e.g. "text/plain; x*") or silently accepting
defective values.

Extracted from #1268.

Co-authored-by: r266-tech <r266-tech@users.noreply.github.com>
Assisted-by: ClaudeCode:claude-fable-5

* fix(metadata): accurate message for defective Description-Content-Type

The parse-error and header-defect paths reused the "must be one of"
message, which is wrong when the content type itself is valid but a
parameter is malformed. Say the value is not a valid content type
instead, including the defect text when there is one.

Assisted-by: ClaudeCode:claude-fable-5

* chore: clean up formatting a little

Signed-off-by: Henry Schreiner <henryfs@princeton.edu>

---------

Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
Co-authored-by: r266-tech <r266-tech@users.noreply.github.com>
r266-tech and others added 5 commits July 11, 2026 09:04
from_raw(validate=True) always raises ExceptionGroup (never a bare
InvalidMetadata), so drop the dead except branch and the two tests that
monkeypatched from_raw to exercise it. The required-field dedup check
reduces to membership in unparsed plus absence from raw, since from_raw
only reports fields from raw or the required set. Also hoist the
repeated message string in _process_description_content_type.

Assisted-by: ClaudeCode:claude-fable-5
Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
…s PR

The malformed Description-Content-Type validation is being split out
into its own PR, leaving only the from_email error aggregation here.

Assisted-by: ClaudeCode:claude-fable-5
@henryiii
henryiii force-pushed the r266-metadata-from-email-validation-aggregation branch 2 times, most recently from 9acdd39 to 61aebc3 Compare July 11, 2026 15:02
henryiii and others added 2 commits July 11, 2026 11:15
The explicit raise with 'from None' is unneeded: finalize raises after
the except block has exited, so no exception context is chained.

Assisted-by: ClaudeCode:claude-fable-5
@r266-tech

Copy link
Copy Markdown
Contributor Author

Pushed a small follow-up for the current Python 3.9 red jobs.

The failures were coverage-only: metadata.py still had an unreachable validate=False ExceptionGroup wrapping path after #1329 split the content-type tightening out of this PR. I removed that dead wrapper and the monkeypatch-only test that forced it.

Checks run locally:

  • PYTHONPATH=src python3 -m pytest tests/test_metadata.py -q (327 passed)
  • uvx nox -s lint -- --show-diff-on-failure
  • NOXFORCEPYTHON=3.11 uvx nox -s tests (62123 passed, 1 skipped, 100% coverage)
  • git diff --check

@r266-tech

Copy link
Copy Markdown
Contributor Author

Pushed one more coverage-only follow-up for the Python 3.9 CI failures.

The failed 3.9 jobs were still reporting partial branch coverage in Metadata.from_email() after the previous cleanup. I reproduced that locally with the 3.9 nox session and marked the two structurally partial branches (ExceptionGroup.exceptions iteration and the collector-finalize fallthrough) with # pragma: no branch; there is no runtime behavior change in this commit.

Checks now passing locally:

  • PYTHONPATH=src python3 -m pytest tests/test_metadata.py -q (327 passed)
  • NOXFORCEPYTHON=3.9 uvx nox -s tests (62123 passed, 1 skipped, 100% coverage)
  • uvx nox -s lint -- --show-diff-on-failure
  • git diff --check + python3.9 -m compileall -q src/packaging/metadata.py

henryiii added 2 commits July 11, 2026 16:46
Branch coverage is 100% without the pragmas, and the monkeypatched
from_raw scenario cannot occur in practice today.

Assisted-by: ClaudeCode:claude-fable-5
The pragmas cover arcs to the trailing from_raw return that only
Python 3.9's tracer records; newer Pythons see 100% without them.

Assisted-by: ClaudeCode:claude-fable-5
@henryiii
henryiii marked this pull request as ready for review July 11, 2026 21:24
@henryiii
henryiii merged commit 2068340 into pypa:main Jul 20, 2026
129 of 131 checks passed
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