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)

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,19 +1097,25 @@ 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
         kv = _split_key(rec.text)
         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)

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,

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
 
 

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
 
 

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,24 +228,27 @@ 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
 
 
 @pytest.mark.parametrize("kind_name", ["PROJECT_KIND", "FAMILY_KIND"])
 @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 ------------------------------------------------------

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))