From 30c9e828e6ba5773ac626aeeb824fac0d18bbbd7 Mon Sep 17 00:00:00 2001 From: dchaudhari7177 <111210939+dchaudhari7177@users.noreply.github.com> Date: Sun, 6 Sep 2026 22:17:17 +0530 Subject: [PATCH 1/3] Fix topmatter clobbering validated config values, and enforce global_only Two bugs in `merge_file_level`, both reachable from ordinary front matter. **Validated values were overwritten.** The loop called `validate_field(new, field, value)` and then `setattr(new, name, value)` with the *raw* value. Validators that normalise their input write the result back on the instance, so that write was immediately undone: --- myst: fence_as_directive: ["mermaid"] --- left a `list` in a field the rest of the code treats as a `set`. Fixed by assigning before validating and letting the validator have the last word -- the order `__post_init__` already uses, where the value is on the instance before `validate_fields` runs. An invalid value now has to be rolled back explicitly, so a rejected value is not reported *and* kept. **`global_only` was declared and never checked.** Five fields carry the metadata -- `heading_slug_func`, `update_mathjax`, `mathjax_classes`, `suppress_warnings`, `inventories` -- and topmatter could set all of them. --- myst: heading_slug_func: github --- put the preset *name* in the field instead of the function it names, so every heading raised `'str' object is not callable [myst.heading_slug]` and no slugs were generated. Topmatter now warns and keeps the global value. The documented contract was already right: `_docs.py` filters `global_only` fields out of the `:scope: local` table, so `configuration.md` has never listed these five as front-matter settable. Only the enforcement was missing. Seventeen tests, eight of which fail on master. Beyond the two bugs they pin the surrounding behaviour the reordering runs through: `merge_topmatter` still merges rather than replaces, a rejected field falls back to the *global* value rather than the dataclass default, one bad key does not abandon the rest of the topmatter, and the global config is not mutated. tests/: 1252 passed, 18 skipped. The 3 failures in my run (`test_cmdline[40-linkify]`, `test_extended_syntaxes`, `test_extended_syntaxes_text`) are identical on master with this branch stashed. ruff 0.15.20 clean. Fixes #1167 --- myst_parser/config/main.py | 22 ++++- tests/test_config_topmatter.py | 165 +++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 4 deletions(-) create mode 100644 tests/test_config_topmatter.py diff --git a/myst_parser/config/main.py b/myst_parser/config/main.py index bd19dc7a..41148e9e 100644 --- a/myst_parser/config/main.py +++ b/myst_parser/config/main.py @@ -585,16 +585,30 @@ def merge_file_level( old_value, field = fields[name] - try: - validate_field(new, field, value) - except Exception as exc: - warning(MystWarnings.MD_TOPMATTER, str(exc)) + if field.metadata.get("global_only"): + warning( + MystWarnings.MD_TOPMATTER, + f"'{name}' is a global-only config and cannot be set in topmatter", + ) continue if field.metadata.get("merge_topmatter"): value = {**old_value, **value} + # Assign before validating, and let the validator have the last word. + # A validator may normalise what it is given and write the result back + # on the instance -- check_fence_as_directive turns the sequence into a + # set, check_heading_slug_func resolves a preset name to a callable. + # Validating first and assigning afterwards overwrote that with the raw + # topmatter value. This is also the order __post_init__ uses, where the + # value is already on the instance before validate_fields runs. setattr(new, name, value) + try: + validate_field(new, field, value) + except Exception as exc: + setattr(new, name, old_value) + warning(MystWarnings.MD_TOPMATTER, str(exc)) + continue return new diff --git a/tests/test_config_topmatter.py b/tests/test_config_topmatter.py new file mode 100644 index 00000000..03f8882b --- /dev/null +++ b/tests/test_config_topmatter.py @@ -0,0 +1,165 @@ +"""Tests for merging file-level topmatter into the global config.""" + +import pytest + +from myst_parser.config.main import MdParserConfig, merge_file_level +from myst_parser.warnings_ import MystWarnings + + +def merge(topmatter): + """Merge topmatter into a default config, returning (config, warnings).""" + warnings: list[tuple[MystWarnings, str]] = [] + config = merge_file_level( + MdParserConfig(), topmatter, lambda kind, msg: warnings.append((kind, msg)) + ) + return config, [msg for _kind, msg in warnings] + + +class TestValidatorNormalisation: + """A validator that normalises its value must have the last word. + + ``validate_field`` was called *before* ``setattr``, so a validator that + resolved or converted the value and wrote it back on the instance had that + result immediately overwritten with the raw topmatter value. + """ + + def test_a_sequence_is_stored_as_the_set_the_field_holds(self): + """`check_fence_as_directive` converts to a set; the raw value is a list. + + Downstream code does set operations on this field, so a list here is a + wrong type, not merely a different one. + """ + config, warnings = merge({"myst": {"fence_as_directive": ["mermaid"]}}) + assert warnings == [] + assert isinstance(config.fence_as_directive, set) + assert config.fence_as_directive == {"mermaid"} + + def test_the_global_value_is_a_set_too(self): + """Pins the invariant the test above is protecting.""" + assert isinstance(MdParserConfig().fence_as_directive, set) + + +class TestGlobalOnlyFields: + """``global_only`` metadata must actually be enforced. + + It was declared on five fields and checked nowhere, so topmatter could set + them. For ``heading_slug_func`` that produced + ``'str' object is not callable`` for every heading, because the field then + held the preset *name* rather than the function it names. + """ + + @pytest.mark.parametrize( + "name,value", + [ + ("heading_slug_func", "github"), + ("update_mathjax", False), + ("mathjax_classes", "tex2jax_process"), + ("suppress_warnings", ["myst.xref_missing"]), + ("inventories", {"x": ["https://example.com", None]}), + ], + ) + def test_a_global_only_field_is_rejected(self, name, value): + config, warnings = merge({"myst": {name: value}}) + assert warnings == [ + f"'{name}' is a global-only config and cannot be set in topmatter" + ] + assert getattr(config, name) == getattr(MdParserConfig(), name) + + def test_the_rejected_field_keeps_the_global_value(self): + """Not just unchanged from the default -- unchanged from the global.""" + warnings: list[str] = [] + global_config = MdParserConfig(heading_slug_func=str.upper) + config = merge_file_level( + global_config, + {"myst": {"heading_slug_func": "github"}}, + lambda kind, msg: warnings.append(msg), + ) + assert config.heading_slug_func is str.upper + assert len(warnings) == 1 + + def test_a_non_global_field_alongside_one_is_still_applied(self): + """Rejecting one key must not abandon the rest of the topmatter.""" + config, warnings = merge( + { + "myst": { + "heading_slug_func": "github", + "enable_extensions": ["dollarmath"], + } + } + ) + assert len(warnings) == 1 + assert "dollarmath" in config.enable_extensions + + +class TestInvalidValues: + """An invalid value must be reported and must not be half-applied.""" + + def test_an_invalid_value_is_rolled_back(self): + """The value is now assigned before validation, so it has to be undone. + + Otherwise a rejected value would be reported *and* kept. + """ + config, warnings = merge({"myst": {"fence_as_directive": "not-a-sequence"}}) + assert len(warnings) == 1 + assert config.fence_as_directive == MdParserConfig().fence_as_directive + + def test_an_invalid_value_rolls_back_to_the_global_not_the_default(self): + warnings: list[str] = [] + global_config = MdParserConfig(fence_as_directive={"mermaid"}) + config = merge_file_level( + global_config, + {"myst": {"fence_as_directive": "not-a-sequence"}}, + lambda kind, msg: warnings.append(msg), + ) + assert len(warnings) == 1 + assert config.fence_as_directive == {"mermaid"} + + def test_an_unknown_field_is_reported(self): + config, warnings = merge({"myst": {"not_a_field": 1}}) + assert warnings == ["Unknown field: not_a_field"] + assert not hasattr(config, "not_a_field") + + +class TestUnaffectedBehaviour: + """Guards for the paths the reordering runs through.""" + + def test_a_plain_field_is_applied(self): + config, warnings = merge({"myst": {"enable_extensions": ["dollarmath"]}}) + assert warnings == [] + assert "dollarmath" in config.enable_extensions + + def test_merge_topmatter_still_merges(self): + """`substitutions` merges with the global value rather than replacing.""" + warnings: list[str] = [] + global_config = MdParserConfig(substitutions={"a": "global"}) + config = merge_file_level( + global_config, + {"myst": {"substitutions": {"b": "file"}}}, + lambda kind, msg: warnings.append(msg), + ) + assert warnings == [] + assert config.substitutions == {"a": "global", "b": "file"} + + def test_the_file_level_value_wins_on_a_merged_field(self): + warnings: list[str] = [] + global_config = MdParserConfig(substitutions={"a": "global"}) + config = merge_file_level( + global_config, + {"myst": {"substitutions": {"a": "file"}}}, + lambda kind, msg: warnings.append(msg), + ) + assert config.substitutions == {"a": "file"} + + def test_the_global_config_is_not_mutated(self): + global_config = MdParserConfig() + merge_file_level( + global_config, + {"myst": {"enable_extensions": ["dollarmath"]}}, + lambda k, m: None, + ) + assert "dollarmath" not in global_config.enable_extensions + + def test_a_non_dict_myst_key_is_reported(self): + _config, warnings = merge({"myst": ["not", "a", "dict"]}) + assert len(warnings) == 1 + assert "not a dict" in warnings[0] From 15bf220fe5f0b08cf351a2f3aba00a63c8b49d23 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Mon, 9 Nov 2026 20:30:21 +0100 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=90=9B=20FIX:=20warn=20on=20a=20non-m?= =?UTF-8?q?apping=20`substitutions`/`html=5Fmeta`=20in=20topmatter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging `{**global, **value}` before validating `value` raised a `TypeError` out of `merge_file_level` for front matter such as `substitutions: [a, b]`, `html_meta: x` or `substitutions: null`, aborting the docutils or Sphinx build where master warned. Validate the topmatter value itself first, inside the `try`, so it is reported as a `myst.topmatter` warning and the global value is kept. Also update the comment's example: `heading_slug_func` is now global-only, so `check_url_schemes` is the example of a converting validator. --- myst_parser/config/main.py | 13 +++++++---- tests/test_config_topmatter.py | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/myst_parser/config/main.py b/myst_parser/config/main.py index 41148e9e..ac223368 100644 --- a/myst_parser/config/main.py +++ b/myst_parser/config/main.py @@ -592,18 +592,21 @@ def merge_file_level( ) continue - if field.metadata.get("merge_topmatter"): - value = {**old_value, **value} - # Assign before validating, and let the validator have the last word. # A validator may normalise what it is given and write the result back # on the instance -- check_fence_as_directive turns the sequence into a - # set, check_heading_slug_func resolves a preset name to a callable. + # set, check_url_schemes turns the list form into a dict. # Validating first and assigning afterwards overwrote that with the raw # topmatter value. This is also the order __post_init__ uses, where the # value is already on the instance before validate_fields runs. - setattr(new, name, value) try: + if field.metadata.get("merge_topmatter"): + # Validate the topmatter value itself before merging it into + # the global mapping, so that a non-mapping (a list, a string, + # null) is reported as a warning rather than raised. + validate_field(new, field, value) + value = {**old_value, **value} + setattr(new, name, value) validate_field(new, field, value) except Exception as exc: setattr(new, name, old_value) diff --git a/tests/test_config_topmatter.py b/tests/test_config_topmatter.py index 03f8882b..7f050c1e 100644 --- a/tests/test_config_topmatter.py +++ b/tests/test_config_topmatter.py @@ -1,8 +1,12 @@ """Tests for merging file-level topmatter into the global config.""" +import io + import pytest +from docutils.core import publish_doctree from myst_parser.config.main import MdParserConfig, merge_file_level +from myst_parser.parsers.docutils_ import Parser from myst_parser.warnings_ import MystWarnings @@ -163,3 +167,41 @@ def test_a_non_dict_myst_key_is_reported(self): _config, warnings = merge({"myst": ["not", "a", "dict"]}) assert len(warnings) == 1 assert "not a dict" in warnings[0] + + +class TestMergedFieldsRejectNonMappings: + """A ``merge_topmatter`` field given a non-mapping must warn, not raise. + + Merging ``{**global, **value}`` before validating ``value`` raised + ``TypeError`` out of ``merge_file_level``, which aborted the whole build + (docutils or Sphinx) where a warning is expected. + """ + + @pytest.mark.parametrize("name", ["substitutions", "html_meta"]) + @pytest.mark.parametrize("value", [["a", "b"], "just-a-string", 3, None], ids=repr) + def test_a_non_mapping_is_reported_and_the_global_kept(self, name, value): + warnings: list[tuple[MystWarnings, str]] = [] + global_config = MdParserConfig(**{name: {"a": "global"}}) + config = merge_file_level( + global_config, + {"myst": {name: value}}, + lambda kind, msg: warnings.append((kind, msg)), + ) + assert len(warnings) == 1 + kind, msg = warnings[0] + assert kind is MystWarnings.MD_TOPMATTER + assert f"'{name}' must be of type" in msg + assert getattr(config, name) == {"a": "global"} + + def test_a_non_mapping_does_not_abort_a_docutils_build(self): + """End to end: the document is still built, with one topmatter warning.""" + stream = io.StringIO() + doctree = publish_doctree( + source="---\nmyst:\n substitutions: [a, b]\n---\n\ntext\n", + parser=Parser(), + settings_overrides={"warning_stream": stream}, + ) + assert "'substitutions' must be of type" in stream.getvalue() + assert "[myst.topmatter]" in stream.getvalue() + assert "text" in doctree.astext() + From 65a2e093f950ab7fbd3803b6e0f8b0b778510f03 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Mon, 9 Nov 2026 20:30:38 +0100 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=A7=AA=20TEST:=20pin=20both=20`url=5F?= =?UTF-8?q?schemes`=20forms=20and=20the=20global-only=20warning=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `url_schemes` from topmatter must keep its validated form: the list form becomes `{name: None}` (the raw list crashed the build on master), and the dict form keeps its templates. The global-only rejection is reported as a `myst.topmatter` warning, so it can be suppressed by that code. --- tests/test_config_topmatter.py | 35 ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/test_config_topmatter.py b/tests/test_config_topmatter.py index 7f050c1e..58be4461 100644 --- a/tests/test_config_topmatter.py +++ b/tests/test_config_topmatter.py @@ -205,3 +205,38 @@ def test_a_non_mapping_does_not_abort_a_docutils_build(self): assert "[myst.topmatter]" in stream.getvalue() assert "text" in doctree.astext() + +class TestUrlSchemes: + """``check_url_schemes`` converts both of its accepted forms to a dict. + + The converted form has to survive the merge: the raw list form, left on + the config, crashed the build with ``list indices must be integers``. + """ + + def test_the_list_form_becomes_a_dict(self): + config, warnings = merge({"myst": {"url_schemes": ["wiki"]}}) + assert warnings == [] + assert config.url_schemes == {"wiki": None} + + def test_the_dict_form_keeps_its_templates(self): + config, warnings = merge( + {"myst": {"url_schemes": {"wiki": "https://w.org/{{path}}"}}} + ) + assert warnings == [] + assert config.url_schemes == {"wiki": {"url": "https://w.org/{{path}}"}} + + +class TestGlobalOnlyWarningKind: + """The global-only rejection is a ``myst.topmatter`` warning. + + So it is reported under that code, and can be suppressed by it. + """ + + def test_the_warning_is_a_topmatter_warning(self): + warnings: list[tuple[MystWarnings, str]] = [] + merge_file_level( + MdParserConfig(), + {"myst": {"heading_slug_func": "github"}}, + lambda kind, msg: warnings.append((kind, msg)), + ) + assert [kind for kind, _msg in warnings] == [MystWarnings.MD_TOPMATTER]