From d3a2ee615ec8918b5684c575beb5bc82625dfa20 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 16:47:52 +0000 Subject: [PATCH 1/3] scripts/repo_shape.py: the YAML reader refuses a key written twice in one mapping, block or flow, by the line of the second, and a block scalar line indented less than its first by its line, where it kept the last key and cut the line's first characters; such a line opening `#` ends the block as a comment (#917) U021. Both mapping builders assigned each key as it came, so `a: 1` then `a: 2` read `{'a': 2}` and `k: {a: 1, a: 2}` read `{'k': {'a': 2}}`. `_refuse_written_twice` refuses the second, before its value is read: "line N: key 'k' is written twice". A flow collection is read on one line, which `_flow` and `_flow_value` now take from `_block_value`, so a flow key is named by that line too. A block scalar's record is on its header's line, where its key is; it was on the body's last. U181. `_block_scalar_body` stripped the block's column from a line indented less than it, so `k: |` over ` a`, ` bcd`, ` e` read `'a\nd\ne\n'`. Such a line is refused by its line. One opening `#` is a trailing comment, which ends the block as YAML reads it. The seeded fuzz in tests/test_yaml_indicator_openers.py counts a key written twice, which PyYAML reads keeping the last, as refused by design. Co-Authored-By: Claude Opus 5.5 --- scripts/repo_shape.py | 74 +++++++- tests/test_yaml_indicator_openers.py | 19 +- tests/test_yaml_reader_spaces.py | 268 +++++++++++++++++++++++++++ 3 files changed, 351 insertions(+), 10 deletions(-) diff --git a/scripts/repo_shape.py b/scripts/repo_shape.py index 66b1b592..032b1261 100644 --- a/scripts/repo_shape.py +++ b/scripts/repo_shape.py @@ -215,6 +215,14 @@ def __init__(self, message: str, remediation: str = ""): # would have to invent, and an invented meaning in a validator is a wrong # answer with a confident tone. # +# A KEY WRITTEN TWICE IN ONE MAPPING IS REFUSED BY ITS LINE (#917), block or +# flow: YAML requires a mapping's keys to be unique, and keeping the last, as +# PyYAML does, read a doubled `commit:` or `files:` as whichever came second. +# So is A BLOCK SCALAR'S LINE INDENTED LESS THAN ITS FIRST, which lost the +# characters left of the block's column; one opening `#` is a comment that +# ends the block, as YAML reads it. See `_refuse_written_twice` and +# `_block_scalar_body`. +# # A QUOTE OPENS A QUOTED SCALAR ONLY WHERE A SCALAR BEGINS, as in YAML; in # the middle of a plain scalar (`it's`) it is a character of it (#249). A # quoted scalar must close on its own line, with nothing after its closing @@ -652,9 +660,13 @@ def _refuse_entry_indicator(text: str, remedy: bool = True) -> None: raise refusal -def _flow_value(text: str, item: bool = False) -> Any: +def _flow_value(text: str, line: int, item: bool = False) -> Any: """A flow entry is itself a flow collection when it OPENS with `[` or `{`. + `line` IS THE LINE THE OUTERMOST COLLECTION IS ON, and so this entry's + (#917): this reader takes a flow collection on one line, and a nested + mapping refuses a key written twice by it, as `_flow` does. + AN EMPTY ENTRY OPENS NOTHING (#252): `{a: }` and `{a:}` give this `''`, and `'' in "[{"` is True for any string, so the test used to run `_flow` on it and refuse `{a: }` as `not a flow collection: ''`, a text nobody @@ -686,7 +698,7 @@ def _flow_value(text: str, item: bool = False) -> Any: keep those words. """ if text.startswith(("[", "{")): - return _flow(text) + return _flow(text, line) _refuse_flow_indicator(text) if item: _refuse_entry_indicator(text) @@ -707,11 +719,14 @@ def _flow_value(text: str, item: bool = False) -> Any: return _scalar(text) -def _flow(text: str) -> Any: +def _flow(text: str, line: int) -> Any: + """A flow collection, on `line`: this reader takes one on a single + line, the block line holding it, and a key its mapping holds twice is + refused by that line (#917).""" text = text.strip(_YAML_WHITE) if text.startswith("[") and text.endswith("]"): body = text[1:-1].strip(_YAML_WHITE) - return [] if not body else [_flow_value(p, item=True) + return [] if not body else [_flow_value(p, line, item=True) for p in _split_flow(body)] if text.startswith("{") and text.endswith("}"): body = text[1:-1].strip(_YAML_WHITE) @@ -721,11 +736,31 @@ def _flow(text: str) -> Any: kv = _split_key_flow(part) if kv is None: raise YamlError(f"flow mapping entry without a key: {part!r}") - out[kv[0]] = _flow_value(kv[1]) + _refuse_written_twice(out, kv[0], line) + out[kv[0]] = _flow_value(kv[1], line) return out raise YamlError(f"not a flow collection: {text!r}") +def _refuse_written_twice(out: dict, key: str, line: int) -> None: + """Refuse `key` where the mapping being built, `out`, holds it already + (#917), naming the line of the second. + + YAML requires a mapping's keys to be unique. Both builders assigned each + key as it came, as PyYAML's constructor does, so the last of two won + without a word: a shape pin holding a `files:` list and then `files: 5` + read as `files: 5`, and a doubled `commit:` or `members:` in any contract + file passed. Each builder asks before it reads the second value, so the + key written twice is what is named, whatever that value holds. + + A KEY IS THE TEXT THIS READER RETURNS FOR IT, so `a` and `"a"` are one + key, as they are to YAML, and so are `1` and `"1"`, which YAML reads as + two: this reader reads every key as text, so its caller would see one. + """ + if key in out: + raise YamlError(f"line {line}: key {key!r} is written twice") + + def _split_key_flow(text: str) -> tuple[str, str] | None: """`_split_key` for a flow-mapping entry, where the value MAY open a flow. @@ -897,6 +932,15 @@ def _block_scalar_body(raw_lines: list[str], i: int, belongs to the body whatever its column, because it has none. A line of Unicode spaces at the block's own indentation or deeper is not blank (#715): it is text, as YAML reads it. + + A LINE INDENTED LESS THAN THE BLOCK'S OWN, BUT MORE THAN THE KEY, IS + REFUSED BY ITS LINE (#917). Stripping the block's column from it took + its first characters with its spaces: `k: |` over ` a`, ` bcd` and + ` e` read `'a\\nd\\ne\\n'`. YAML ends the block at such a line, which + then stands where no key or entry can, and PyYAML refuses the document. + ONE OPENING `#` IS A COMMENT, which ends the block as YAML's trailing + comment does and is read as one: ` # c` under ` a` and over the next + key was the text `c`. """ body: list[str] = [] block_indent = None @@ -911,6 +955,13 @@ def _block_scalar_body(raw_lines: list[str], i: int, break if block_indent is None: block_indent = nxt_indent + elif nxt_indent < block_indent: + if nxt[nxt_indent] == "#": + break + raise YamlError( + f"line {i + 1}: a line of a block scalar is indented " + f"{nxt_indent} spaces, fewer than the {block_indent} of its " + "first line") body.append(nxt[block_indent:]) i += 1 return body, i @@ -984,8 +1035,11 @@ def _tokenise(text: str) -> list[_Rec]: if _opens_block_scalar(kv): # A block scalar: swallow every following line more indented than # the key, and keep the key and the body as the record's pair. + # The record is on the HEADER's line, where its key is, which a + # key written twice is refused by (#917); it was the body's last. + header_line = i body, i = _block_scalar_body(raw_lines, i, indent) - recs.append(_Rec(indent, "", False, i, + recs.append(_Rec(indent, "", False, header_line, (kv[0], _block_scalar_text(kv[1], body)))) continue recs.append(_Rec(indent, content, False, i)) @@ -1043,12 +1097,17 @@ def _parse_mapping(recs: list[_Rec], i: int, indent: int) -> tuple[dict, int]: text `-`, and PyYAML refuses it. `_scalar` refuses the other indicators; this one is refused here because a flow collection reads `[-]` and `{a: -}` as the text `-`, as PyYAML does. + + A KEY WRITTEN TWICE IS REFUSED BY THE LINE OF THE SECOND (#917), before + its value is read, whether that value is on the line, under it, or a + block scalar: `_refuse_written_twice`. """ out: dict[str, Any] = {} while i < len(recs) and not recs[i].dash and recs[i].indent == indent: rec = recs[i] if rec.block is not None: key, body = rec.block + _refuse_written_twice(out, key, rec.line) out[key] = body i += 1 continue @@ -1056,6 +1115,7 @@ def _parse_mapping(recs: list[_Rec], i: int, indent: int) -> tuple[dict, int]: if kv is None: raise YamlError(f"line {rec.line}: expected `key: value`, got {rec.text!r}") key, rest = kv + _refuse_written_twice(out, key, rec.line) i += 1 if rest == "": out[key], i = _nested_value(recs, i, indent) @@ -1084,7 +1144,7 @@ def _block_value(text: str, line: int) -> Any: """ try: if text[0] in "[{": - return _flow(text) + return _flow(text, line) if text == "-": raise _indicator_refusal(text) return _scalar(text) diff --git a/tests/test_yaml_indicator_openers.py b/tests/test_yaml_indicator_openers.py index d6b3307d..9616966c 100644 --- a/tests/test_yaml_indicator_openers.py +++ b/tests/test_yaml_indicator_openers.py @@ -44,6 +44,7 @@ from __future__ import annotations import random +import re import sys import pytest @@ -511,9 +512,21 @@ def fuzz_documents(seed: int) -> list[str]: return sorted(set(docs)) -#: The two refusals this reader makes of a document PyYAML reads, by design: -#: an explicit key (#252) and a single pair in a flow sequence (#392). +#: The refusals this reader makes of a document PyYAML reads, by design: an +#: explicit key (#252), a single pair in a flow sequence (#392), and a key +#: written twice (#917), the last of which PyYAML keeps. The fuzz draws a +#: key written twice wherever a flow mapping's two entries share one, `a` +#: or `"a"`. BY_DESIGN = (EXPLICIT_KEY, PAIR) +WRITTEN_TWICE = re.compile(r"line [0-9]+: key '[^']*' is written twice") + + +def by_design(refusal) -> bool: + """Is `refusal` one this reader makes by design: `BY_DESIGN`'s, or a + key written twice, whose words open with the line?""" + words = str(refusal) + return (words.startswith(BY_DESIGN) + or WRITTEN_TWICE.fullmatch(words) is not None) @NEEDS_PYYAML @@ -537,7 +550,7 @@ def test_the_fuzz_finds_no_document_read_where_pyyaml_refuses_or_misread(seed): misread.append((doc, mine, theirs)) elif refused(theirs): tally["both refuse"] += 1 - elif str(mine).startswith(BY_DESIGN): + elif by_design(mine): tally["by design"] += 1 else: undesigned.append((doc, str(mine), theirs)) diff --git a/tests/test_yaml_reader_spaces.py b/tests/test_yaml_reader_spaces.py index bcdf3a5c..c11daddb 100644 --- a/tests/test_yaml_reader_spaces.py +++ b/tests/test_yaml_reader_spaces.py @@ -22,6 +22,19 @@ - L4-154, `_neutral_pin_entry`'s docstring gave `id: -` as "the text", which the reader refuses since #393; its example is now one it reads. +And two things the reader read without a word, from #459's triage (#917): + +- U021, A KEY WRITTEN TWICE. Both mapping builders kept the last, as + PyYAML does: `a: 1` then `a: 2` read `{'a': 2}`, and `k: {a: 1, a: 2}` + read `{'k': {'a': 2}}`. YAML requires a mapping's keys to be unique, and + Brett Heap ruled on 2026-10-14 to refuse one: each is refused by the + line of the second. +- U181, A BLOCK SCALAR'S LINE INDENTED LESS THAN ITS FIRST lost the + characters left of the block's column: `k: |` over ` a`, ` bcd` and + ` e` read `'a\\nd\\ne\\n'`, where PyYAML refuses the document. It is + refused by its line. One opening `#` is a comment that ends the block, + as PyYAML reads it, where it was text. + PyYAML is the ORACLE ONLY, as in `tests/test_yaml_indicator_openers.py`, whose helpers this file uses: where it is not installed, the comparisons are skipped and this reader's answers are still asserted. @@ -873,3 +886,258 @@ def test_tracking_branch_of_names_a_nul_and_a_surrogate_as_the_validator_does( assert str(refused.value).startswith( f"REFUSED unseen-branch: tracking_branch is {value!r}, which holds " f"{words}\n"), refused.value + + +# --- #917 U021: a key written twice ----------------------------------------- + +#: The words a key written twice is refused with, `{line}` and `{key}` its. +TWICE = "line {line}: key {key!r} is written twice" + +#: `(document, the line of the second key, the key, what PyYAML reads)`: +#: the issue's two rows first, then the same wherever a mapping stands. +#: PyYAML keeps the last, as this reader did. +WRITTEN_TWICE = [ + pytest.param("a: 1\na: 2", 2, "a", {"a": 2}, id="the-issue-block-row"), + pytest.param("k: {a: 1, a: 2}", 1, "a", {"k": {"a": 2}}, + id="the-issue-flow-row"), + pytest.param("k:\n a: 1\n a: 2", 3, "a", {"k": {"a": 2}}, + id="nested-block"), + pytest.param("j: 1\nk: {a: {b: 1, b: 2}}", 2, "b", + {"j": 1, "k": {"a": {"b": 2}}}, id="nested-flow"), + pytest.param("j: 1\nk: [x, {a: 1, a: 2}]", 2, "a", + {"j": 1, "k": ["x", {"a": 2}]}, + id="a-flow-mapping-in-a-flow-sequence"), + pytest.param("- a: 1\n a: 2", 2, "a", [{"a": 2}], id="a-sequence-item"), + pytest.param("a:\n b: 1\na:\n c: 2", 3, "a", {"a": {"c": 2}}, + id="over-nested-values"), + pytest.param("k: 1\nk: |\n a\n b", 2, "k", {"k": "a\nb\n"}, + id="a-block-scalar-second"), + pytest.param("k: >\n a\n b\nk: 1", 4, "k", {"k": 1}, + id="a-block-scalar-first"), + pytest.param('a: 1\n"a": 2', 2, "a", {"a": 2}, + id="quoted-the-second-time"), + pytest.param("k: {a: 1, 'a': 2}", 1, "a", {"k": {"a": 2}}, + id="quoted-in-a-flow"), + pytest.param("files:\n - path: a\nfiles: 5", 3, "files", {"files": 5}, + id="a-files-list-then-a-number"), +] + + +@pytest.mark.parametrize("text, line, key, pyyaml_reads", WRITTEN_TWICE) +def test_a_key_written_twice_is_refused_by_the_line_of_the_second( + text, line, key, pyyaml_reads): + assert refusal_words(text) == TWICE.format(line=line, key=key) + + +@NEEDS_PYYAML +@pytest.mark.parametrize("text, line, key, pyyaml_reads", WRITTEN_TWICE) +def test_pyyaml_keeps_the_last_of_each(text, line, key, pyyaml_reads): + assert stock(text) == pyyaml_reads + + +def test_the_key_is_refused_before_its_second_value_is_read(): + """A second value the reader refuses on its own is not what is named: + the key written twice is.""" + assert refusal_words("a: ]") == ( + f"{OPENS} `]`, which closes a flow sequence in YAML: ']'") + assert refusal_words("a: 1\na: ]") == TWICE.format(line=2, key="a") + + +def test_a_key_read_as_the_same_text_is_one_key(): + """This reader reads every key as text, so `1` and `'1'` are one key to + its caller, which would see one: refused, though YAML reads two.""" + assert refusal_words("1: a\n'1': b") == TWICE.format(line=2, key="1") + + +#: The same key in two mappings, and a value repeated: no key written twice. +NOT_WRITTEN_TWICE = [ + pytest.param("- a: 1\n- a: 2", [{"a": 1}, {"a": 2}], + id="two-sequence-items"), + pytest.param("a:\n a: 1", {"a": {"a": 1}}, id="the-key-of-its-value"), + pytest.param("a: {a: 1}", {"a": {"a": 1}}, id="the-key-of-a-flow-value"), + pytest.param("k: [{a: 1}, {a: 2}]", {"k": [{"a": 1}, {"a": 2}]}, + id="two-flow-mappings"), + pytest.param("a: 1\nb:\n a: 2", {"a": 1, "b": {"a": 2}}, + id="a-nested-mapping"), + pytest.param("a: 1\nA: 2", {"a": 1, "A": 2}, id="another-case"), + pytest.param("k: [a, a]", {"k": ["a", "a"]}, id="a-repeated-item"), + pytest.param("a: 1\nb: 1", {"a": 1, "b": 1}, id="a-repeated-value"), +] + + +@pytest.mark.parametrize("text, expected", NOT_WRITTEN_TWICE) +def test_the_same_key_in_another_mapping_still_reads(text, expected): + assert parse_yaml(text + "\n") == expected + + +@NEEDS_PYYAML +@pytest.mark.parametrize("text, expected", NOT_WRITTEN_TWICE) +def test_pyyaml_reads_each_other_mapping_the_same(text, expected): + assert stock(text) == expected + + +@pytest.mark.parametrize("seed, document", [ + pytest.param(392393, 'k: {"a": "a", a: -a,}', id="seed-392393"), + pytest.param(3, 'k: {"a": "a", "a": ab}', id="seed-3"), +]) +def test_the_fuzz_draws_a_key_written_twice_that_pyyaml_reads(seed, + document): + """The seeded fuzz of `tests/test_yaml_indicator_openers.py` draws these, + which PyYAML reads, keeping the last; it counts them as refused by + design.""" + assert document in fuzz_documents(seed) + assert refusal_words(document) == TWICE.format(line=1, key="a") + + +#: A block scalar indented under a key that already has a value. +STRAY_BLOCK = "a: 1\n k: |\n x\n y" + + +def test_a_stray_block_scalar_is_named_by_its_headers_line(): + """A block scalar's record is on its header's line now, where its key + is, so a stray one is named there; on main the line was the body's + last, `line 4`.""" + assert refusal_words(STRAY_BLOCK) == "line 2: unexpected indentation" + + +@NEEDS_PYYAML +def test_pyyaml_refuses_the_stray_block_scalar_too(): + assert refused(stock(STRAY_BLOCK)) + + +# --- #917 U181: a block scalar's line indented less than its first ---------- + +#: `(document, the line refused, its spaces, the block's)`. On main the +#: issue's row read `'a\nd\ne\n'`, a line no longer than the block's +#: column was lost whole (`all-of-it`, `one-space-short`), and the key in +#: an item was read into the block as `' 1'`. +UNDER_INDENTED = [ + pytest.param("k: |\n a\n bcd\n e", 3, 2, 4, id="the-issue-row"), + pytest.param("k: >\n a\n bcd\n e", 3, 2, 4, id="folded"), + pytest.param("k: |-\n a\n bcd", 3, 2, 4, id="the-last-line"), + pytest.param("k: |\n a\n b", 3, 4, 6, id="all-of-it"), + pytest.param("k: |\n a\n b", 3, 3, 4, id="one-space-short"), + pytest.param("k: |\n a\n\n bcd", 4, 2, 4, id="after-a-blank-line"), + pytest.param("k:\n j: |\n a\n bcd", 4, 4, 6, id="nested"), + pytest.param("- k: |\n a\n j: 1", 3, 4, 6, id="a-key-in-an-item"), + pytest.param("k: |\n a\n \tb", 3, 2, 4, id="a-tab-after-the-spaces"), +] + +#: How a line indented less than its block is refused. +UNDER = ("line {line}: a line of a block scalar is indented {spaces} spaces, " + "fewer than the {block} of its first line") + + +@pytest.mark.parametrize("text, line, spaces, block", UNDER_INDENTED) +def test_a_line_indented_less_than_its_block_is_refused_by_its_line( + text, line, spaces, block): + assert refusal_words(text) == UNDER.format(line=line, spaces=spaces, + block=block) + + +@NEEDS_PYYAML +@pytest.mark.parametrize("text, line, spaces, block", UNDER_INDENTED) +def test_pyyaml_refuses_each_under_indented_line_too(text, line, spaces, + block): + assert refused(stock(text)), f"PyYAML reads {text!r} as {stock(text)!r}" + + +#: A comment indented less than the block ends it, as YAML's trailing +#: comment does; a `#` at the block's column or on its first line is text; +#: and a line deeper than the first keeps its extra spaces, as before. +BLOCK_STILL_READ = [ + pytest.param("k: |\n a\n # c\nj: 1", {"k": "a\n", "j": 1}, + id="a-comment-ends-the-block"), + pytest.param("k: |\n a\n #c", {"k": "a\n"}, + id="a-comment-with-no-space"), + pytest.param("- k: |\n a\n # c\n j: 1", [{"k": "a\n", "j": 1}], + id="a-comment-in-an-item"), + pytest.param("k: >-\n one\n two\n # c\nj: 1", + {"k": "one two", "j": 1}, + id="a-comment-after-a-folded-block"), + pytest.param("k: |\n a\n # c", {"k": "a\n# c\n"}, + id="a-hash-at-the-blocks-column-is-text"), + pytest.param("k: |\n # c\n a", {"k": "# c\n a\n"}, + id="a-hash-on-the-first-line-is-text"), + pytest.param("k: |\n a\n b\n c", {"k": "a\n b\nc\n"}, + id="deeper-is-kept"), +] + + +@pytest.mark.parametrize("text, expected", BLOCK_STILL_READ) +def test_a_comment_ends_the_block_and_the_rest_still_reads(text, expected): + assert parse_yaml(text + "\n") == expected + + +@NEEDS_PYYAML +@pytest.mark.parametrize("text, expected", BLOCK_STILL_READ) +def test_pyyaml_reads_each_block_the_same(text, expected): + assert stock(text) == expected + + +#: A line at the block's column after a comment that ended it. +UNDER_THE_COMMENT = "k: |\n a\n # c\n b" + + +def test_a_line_under_the_comment_is_not_read_into_the_block(): + """The comment ended the block, so the line after it stands where + nothing can, and is refused.""" + assert refusal_words(UNDER_THE_COMMENT) == ( + "line 4: unexpected indentation") + + +@NEEDS_PYYAML +def test_pyyaml_refuses_the_line_under_the_comment_too(): + assert refused(stock(UNDER_THE_COMMENT)) + + +def test_an_indentation_digit_is_still_not_read_and_now_refuses(): + """The header's indentation digit is accepted and never read: the + block's column is its first line's. `k: |2` over ` a` and ` b` is + `' a\\nb\\n'` to PyYAML, and main read `'a\\n'`, losing `b`; it is + refused now, by the rule above, and not misread.""" + assert refusal_words("k: |2\n a\n b") == UNDER.format( + line=3, spaces=2, block=4) + + +# --- #917: a file holding one, and the copy a project carries --------------- + +@pytest.mark.parametrize("body, words", [ + pytest.param(b"id: atlas\nid: other\n", TWICE.format(line=2, key="id"), + id="a-block-key"), + pytest.param(b"id: atlas\npins: {a: 1, a: 2}\n", + TWICE.format(line=2, key="a"), id="a-flow-key"), + pytest.param(b"id: atlas\nnote: |\n a\n bcd\n", + UNDER.format(line=4, spaces=2, block=4), + id="an-under-indented-line"), +]) +def test_a_file_holding_one_is_yaml_unparsable_naming_the_line_once( + tmp_path, body, words): + path = tmp_path / "project.yaml" + path.write_bytes(body) + with pytest.raises(Refusal) as caught: + load_yaml(path) + assert caught.value.code == "yaml-unparsable" + assert caught.value.detail == f"{path}: {words}" + assert caught.value.remediation == "" + + +def test_the_projects_own_validator_refuses_a_key_written_twice(project): + """The copy a project carries reads `project.yaml`: a second + `tracking_branch:` is no longer the branch, and its own + `validate-manifest.py` exits 2, naming the file and the line.""" + manifest = project / "project.yaml" + data = manifest.read_bytes() + old = b'tracking_branch: "main"\n' + assert data.count(old) == 1, f"fixture drift: {old!r} in project.yaml" + edited = data.replace(old, old + b"tracking_branch: other\n", 1) + manifest.write_bytes(edited) + line = edited[:edited.index(b"tracking_branch: other")].count(b"\n") + 1 + result = run_script(project / "scripts" / "validate-manifest.py", + cwd=project) + assert result.returncode == 2, result.stderr + assert (f"REFUSED yaml-unparsable: {manifest}: line {line}: key " + "'tracking_branch' is written twice\n") in result.stderr + assert "Traceback" not in result.stderr + assert "manifest ok" not in result.stdout + assert manifest.read_bytes() == edited From e63ca75c7c9b04bb7d6ee20b82469f904e7b5f4d Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 18:42:06 +0000 Subject: [PATCH 2/3] update-shape.py: its refusal of a manifest with two `shape:` blocks no longer says the reader keeps the last one, since #917's reader refuses a key written twice (#917) #917's reader refuses a key written twice, so the words `update-shape.py` gave a manifest holding two `shape:` blocks, "the reader keeps the last block" and "the validators read the last one", were false of the reader. The refusal, `update-manifest-unrecognised`, stays; its detail now says "the reader refuses a key written twice", and its remedy says "the reader refuses a manifest that holds it twice", so what a person is told is true of the reader they run. The finder's docstring says what the reader did and does. Two fixtures stated the old reading and failed with it: a doubled manifest in tests/test_update_shape_header_comments.py and the `"shape":` beside a bare `shape:` in tests/test_update_shape_apply_lines_and_headers.py each asserted `parse_yaml` returned the last block. Each now asserts the reader refuses it, `key 'shape' is written twice`, and the refusal's new words. The remedy's words had no assertion; one is added, so putting back the old remedy fails 8 cases. Co-Authored-By: Claude Sonnet 5.5 --- ...st_update_shape_apply_lines_and_headers.py | 13 ++++----- tests/test_update_shape_header_comments.py | 27 ++++++++++++------- update-shape.py | 24 +++++++++-------- 3 files changed, 38 insertions(+), 26 deletions(-) diff --git a/tests/test_update_shape_apply_lines_and_headers.py b/tests/test_update_shape_apply_lines_and_headers.py index 5d2bc764..e5c691c5 100644 --- a/tests/test_update_shape_apply_lines_and_headers.py +++ b/tests/test_update_shape_apply_lines_and_headers.py @@ -77,7 +77,7 @@ ) sys.path.insert(0, str(REPO / "scripts")) -from repo_shape import Refusal, parse_yaml # noqa: E402 +from repo_shape import Refusal, YamlError, parse_yaml # noqa: E402 UPDATE = REPO / "update-shape.py" @@ -179,8 +179,8 @@ def test_a_shape_header_the_reader_takes_is_found_and_kept( ids=["quoted-first", "quoted-last"]) def test_a_quoted_shape_header_beside_a_bare_one_is_the_key_written_twice( update_shape, quoted_first): - """The reader reads `"shape":` and `shape:` as ONE key written twice and - keeps the last block, so this finder, asking the reader's question, sees + """The reader reads `"shape":` and `shape:` as ONE key written twice, + refused since #917, so this finder, asking the reader's question, sees two headers and refuses by name, as #558 refuses a bare `shape:` twice. The bare-only finder saw one: with the bare header first it rewrote a block nobody reads, and the read-back blamed "this tool's line @@ -194,7 +194,8 @@ def test_a_quoted_shape_header_beside_a_bare_one_is_the_key_written_twice( quoted = first if quoted_first else last lines[quoted] = '"shape":' text = "\n".join(lines) + "\n" - assert parse_yaml(text)["shape"]["commit"] == OLD_COMMIT + with pytest.raises(YamlError, match="key 'shape' is written twice"): + parse_yaml(text) with pytest.raises(Refusal) as caught: update_shape.rewrite_manifest(text, NEW_COMMIT, NEW_TREE, @@ -203,8 +204,8 @@ def test_a_quoted_shape_header_beside_a_bare_one_is_the_key_written_twice( assert caught.value.code == "update-manifest-unrecognised" assert caught.value.detail.startswith( f"project.yaml holds the `shape:` key twice, at lines {first + 1} " - f"and {last + 1}, where exactly one was expected: the reader keeps " - "the last block"), caught.value.detail + f"and {last + 1}, where exactly one was expected: the reader " + "refuses a key written twice"), caught.value.detail assert "line rewrite" not in caught.value.detail + caught.value.remediation diff --git a/tests/test_update_shape_header_comments.py b/tests/test_update_shape_header_comments.py index 61200ba0..f779d177 100644 --- a/tests/test_update_shape_header_comments.py +++ b/tests/test_update_shape_header_comments.py @@ -50,7 +50,7 @@ scaffold_upstream_and_project) sys.path.insert(0, str(REPO / "scripts")) -from repo_shape import Refusal, parse_yaml # noqa: E402 +from repo_shape import Refusal, YamlError, parse_yaml # noqa: E402 UPDATE = REPO / "update-shape.py" PIN = "contracts/shape-pin.yaml" @@ -228,13 +228,15 @@ def test_a_manifest_with_no_shape_header_still_names_this_kinds_validator( def doubled_manifest() -> str: """A manifest whose `shape:` key is written twice, the second block holding - the old values: the reader keeps the LAST block.""" + the old values: the reader refuses it since #917 (it kept the LAST + block).""" doubled = (MANIFEST.format(member="e" * 40, commit=OLD_COMMIT, tree=OLD_TREE) + f'shape:\n commit: "{OLD_COMMIT}"\n digests:\n' f' tree_sha256: "{OLD_TREE}"\n') - assert parse_yaml(doubled)["shape"]["commit"] == OLD_COMMIT, ( - "fixture: the reader keeps the last block") + with pytest.raises(YamlError, match="^line 17: key 'shape' is written " + "twice$"): + parse_yaml(doubled) return doubled @@ -242,10 +244,11 @@ def doubled_manifest() -> str: @pytest.mark.parametrize("which", ["neither", "first", "last", "both"]) def test_a_doubled_shape_header_is_refused_by_name_as_a_key_written_twice( update_shape, kind_name, which): - """THE READER DOES NOT REFUSE A DOUBLED `shape:` (it keeps the last), and a - rewrite of the first block would move a pin nobody reads. It is refused - BY NAME, in words that are true of a key written twice, whichever header - carries a comment: the read-back's, "this tool's line rewrite", are not. + """THE READER REFUSES A DOUBLED `shape:` SINCE #917 (it kept the last), + and a rewrite of the first block would move a pin nobody reads. It is + refused BY NAME, in words that are true of a key written twice, whichever + header carries a comment: the read-back's, "this tool's line rewrite", + are not. On main a first header with a comment was skipped by the exact match, so that manifest was rewritten at its last block and went through (review of PR #558); it is refused here, as the uncommented one always was.""" @@ -267,11 +270,17 @@ def test_a_doubled_shape_header_is_refused_by_name_as_a_key_written_twice( assert caught.value.code == "update-manifest-unrecognised" assert caught.value.detail.startswith( f"{kind.manifest} holds the `shape:` key twice, at lines 8 and 17, " - "where exactly one was expected: the reader keeps the last block") + "where exactly one was expected: the reader refuses a key written " + "twice") assert "Remediation: delete the extra `shape:` block by hand" in \ caught.value.remediation + assert "(the reader refuses a manifest that holds it twice)" in \ + caught.value.remediation assert "line rewrite" not in (caught.value.detail + caught.value.remediation) + assert "the last" not in (caught.value.detail + + caught.value.remediation), ( + "the reader no longer keeps the last block (#917)") # --- `apply`, end to end ------------------------------------------------------ diff --git a/update-shape.py b/update-shape.py index b3816c22..c1e73dfa 100755 --- a/update-shape.py +++ b/update-shape.py @@ -1676,13 +1676,14 @@ def _shape_block_bounds(lines: list[str], kind: Kind) -> tuple[int, int]: written twice, as the reader reads it, and refused by name below. A HEADER THAT APPEARS TWICE IS REFUSED BY NAME (review of PR #558). The - reader keeps the last block, so a rewrite of the first would move a pin - nobody reads. That was left to `rewrite_fault`'s read-back, whose words - blame this tool's line rewrite, which is false of a key written twice. - And where the first header carries a comment, the exact match skipped it - and rewrote the last block, which is what the reader reads, so such a - manifest went through; this finder sees the first header, and would - have refused it in those same false words. + reader kept the last block, so a rewrite of the first would have moved a + pin nobody reads; since #917 it refuses the manifest, `key 'shape' is + written twice`, and so do the validators of a shape from then on. That + was left to `rewrite_fault`'s read-back, whose words blame this tool's + line rewrite, which is false of a key written twice. And where the first + header carries a comment, the exact match skipped it and rewrote the + last block, so such a manifest went through; this finder sees the first + header, and would have refused it in those same false words. """ starts = [index for index, line in enumerate(lines) if _is_block_header(line, "shape")] @@ -1691,11 +1692,12 @@ def _shape_block_bounds(lines: list[str], kind: Kind) -> tuple[int, int]: "update-manifest-unrecognised", f"{kind.manifest} holds the `shape:` key twice, at lines " + " and ".join(str(index + 1) for index in starts[:2]) - + ", where exactly one was expected: the reader keeps the last " - "block, so this tool will not guess which of them is the " - "manifest's", + + ", where exactly one was expected: the reader refuses a key " + "written twice, so this tool will not guess which of them is " + "the manifest's", "Remediation: delete the extra `shape:` block by hand (the " - "validators read the last one) and run the same command again.") + "reader refuses a manifest that holds it twice) and run the " + "same command again.") start = starts[0] if starts else None if start is None: raise Refusal( From dc82a4c49019f2c605a58e8dd40063517b3e9c0b Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 20:09:44 +0000 Subject: [PATCH 3/3] tests/test_adopt_entry_paths.py and tests/test_manifest_validator.py: an entry the test answers twice keeps one `resolution:`, and the code leg the test takes out of the manifest takes its `naming:` record with it, so neither fixture writes a key twice for #917's reader to refuse (#917) CI's Linux `tests` job on #974 (run 39155796072) failed three tests with `yaml-unparsable`, each over a file its own test had written: - `test_an_entry_answered_root_or_drop_is_not_judged[root]` and `[drop]` call the file's `answer` helper twice on `-notes.md`, null to spec and then spec to root or drop. Each call wrote a `resolution:` line below the new `leg:`, so the entry held two and `check` refused the plan: `line 86: key 'resolution' is written twice`. `plan` writes no `resolution:` at all. `answer` now replaces the line it wrote before. - `test_a_missing_role_is_a_finding` cut the code leg's `role:`, `repository:` and `path:` lines and left its `naming:` record, which the scaffold writes under every leg, so the spec entry above held two `naming:` keys: `line 167: key 'naming' is written twice`, exit 2 where the test asks for the `manifest-roles` finding's exit 1. The edit now cuts the whole entry; the scaffold writes one `naming:` per leg. Co-Authored-By: Claude Opus 5.5 --- tests/test_adopt_entry_paths.py | 14 +++++++++++--- tests/test_manifest_validator.py | 8 ++++++-- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/test_adopt_entry_paths.py b/tests/test_adopt_entry_paths.py index 292ecd01..70c957cc 100644 --- a/tests/test_adopt_entry_paths.py +++ b/tests/test_adopt_entry_paths.py @@ -101,16 +101,24 @@ def run_check(plan: Path): return run_script(ADOPT, "check", "--plan", str(plan)) +#: The `resolution:` line `answer` writes below the `leg:` it answers. +RESOLVED = " resolution: \"answered by the test\"\n" + + def answer(plan: Path, spelled: str, old: str, new: str) -> None: """Re-answer the entry whose `path:` is written `spelled`, from `old` to - `new`, LF as `plan` wrote it (see `conftest.resolve`).""" + `new`, LF as `plan` wrote it (see `conftest.resolve`). An entry this + helper answered before keeps its one `resolution:` line: a second one + below it would be a key written twice in one mapping, which the reader + refuses since #917, and `check` with it.""" text = plan.read_text(encoding=UTF8) needle = f" - path: {spelled}\n leg: {old}\n" assert needle in text, f"{spelled} is not an entry with leg: {old}" + if needle + RESOLVED in text: + needle += RESOLVED with plan.open("w", encoding=UTF8, newline="\n") as handle: handle.write(text.replace( - needle, f" - path: {spelled}\n leg: {new}\n" - " resolution: \"answered by the test\"\n", 1)) + needle, f" - path: {spelled}\n leg: {new}\n" + RESOLVED, 1)) def planned(tmp_path: Path, tree: dict, diff --git a/tests/test_manifest_validator.py b/tests/test_manifest_validator.py index 3f8d20e5..effabe6d 100644 --- a/tests/test_manifest_validator.py +++ b/tests/test_manifest_validator.py @@ -53,12 +53,16 @@ def test_a_topic_that_is_not_derived_from_the_id_is_a_finding(project): def test_a_missing_role_is_a_finding(project): + """The whole `code` entry goes, its `naming:` record with it: left + behind, that record is a second `naming:` of the `spec` entry above it, + a key written twice, which the reader refuses since #917.""" edit(project, ' - role: code\n repository: testorg/Atlas-code\n' - ' path: "code"\n', + ' path: "code"\n naming:\n form: project-leg\n' + ' role: code\n also_matches: []\n', "") result = validate(project) - assert result.returncode == 1 + assert result.returncode == 1, result.stderr assert "manifest-roles" in result.stderr