From d74801c9a07e52649fe431a566df404e1149dabd Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 10:08:29 +0100 Subject: [PATCH 01/30] fix(split): a comment ending a moved block keeps its line's CR In a CRLF root, the span of a moved block that ended in a trailing comment ran to the line's LF, so it took the CR with it: the $ref line that replaced the block ended in a bare LF among CRLF lines, in the root and in a fragment a nested part was cut from (finding SP-01). The span now stops before the CR, which stays to end the ref's line. The CRLF fixtures and the generated roots keep the comments they left out to avoid the bug. --- fluid_build/cli/_split_text.py | 4 +++- tests/fragments/test_split.py | 14 +++----------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/fluid_build/cli/_split_text.py b/fluid_build/cli/_split_text.py index 03ee256a..e2c6fcf2 100644 --- a/fluid_build/cli/_split_text.py +++ b/fluid_build/cli/_split_text.py @@ -168,9 +168,11 @@ def _content_end(text: str, node: Any) -> int: def _with_trailing_comment(text: str, end: int) -> int: - """``end``, moved over a comment that ends the same line.""" + """``end``, moved over a comment that ends the same line, not over its line break.""" newline = text.find("\n", end) line_end = len(text) if newline < 0 else newline + if line_end > end and text[line_end - 1] == "\r": + line_end -= 1 rest = text[end:line_end] return line_end if rest.strip().startswith("#") else end diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 4fb76300..4c833af5 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1910,11 +1910,9 @@ def test_i24_a_tag_or_an_unused_anchor_on_a_moved_section_splits( # ── I-25 / I-26: line ends, byte-order mark, the end of the file ──────── -#: SPLIT-ALL with CRLF line ends, minus the one comment that ends a moved block +#: SPLIT-ALL with CRLF line ends, the comment that ends a moved block included #: (see ``test_i25_a_comment_ending_a_moved_block...``, finding SP-01). -SPLIT_ALL_CRLF = SPLIT_ALL.replace( - " dataResidency: true # trailing comment on its last line\n", " dataResidency: true\n" -).replace("\n", "\r\n") +SPLIT_ALL_CRLF = SPLIT_ALL.replace("\n", "\r\n") def assert_one_line_end(files: Mapping[str, bytes], crlf: bool = True) -> None: @@ -1963,11 +1961,6 @@ def test_i25_every_part_on_a_crlf_root_keeps_crlf_on_every_line(tmp_path: Path, ], ids=["root", "nested-fragment"], ) -@pytest.mark.xfail( - strict=True, - reason="BUG SP-01: a trailing comment on the last line of a moved block takes that line's CR " - "with it, so the $ref line that replaces it ends in a bare LF in a CRLF file", -) def test_i25_a_comment_ending_a_moved_block_in_a_crlf_root_keeps_crlf( tmp_path: Path, text: str, parts: Optional[str], where: str ) -> None: @@ -1994,7 +1987,6 @@ def test_i25_a_comment_ending_a_moved_block_still_moves_with_it_in_a_crlf_root( @pytest.mark.parametrize("line_end", ["\r\n", "\n"], ids=["crlf", "lf"]) def test_i25_a_byte_order_mark_is_kept(tmp_path: Path, line_end: str) -> None: text = "" + SPLIT_ALL.replace("\n", line_end) - text = text.replace(" # trailing comment on its last line", "") pair = make_pair(tmp_path, {NAME: text.encode("utf-8"), "overlays/prod.yaml": PROD_OVERLAY}) after = split_and_check(pair, ALL_PARTS, envs=(None, "prod"), cli_envs=(None,)) @@ -3141,7 +3133,7 @@ def generated_root(seed: int) -> Tuple[str, List[str], bool]: rng = random.Random(seed) crlf = rng.random() < 0.3 for _ in range(50): - text = _with_comments(_render(_generated_contract(rng), rng), rng, trailing=not crlf) + text = _with_comments(_render(_generated_contract(rng), rng), rng, trailing=True) try: yaml.safe_load(text) except yaml.YAMLError: From 280a4779f75c96905598c5df062f4d03467ecf97 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 10:10:42 +0100 Subject: [PATCH 02/30] fix(split): the blank lines of a keep-chomped scalar move with it A block scalar's span ended at its last character of text, which is right for |, |-, > and >-, whose blank lines after the text are not their value. A keep-chomped one (|+, >+) keeps them: when it ended a moved part, the fragment lost them, the split contract no longer composed to the original one, and split refused a valid contract (finding SP-04). The span of a keep-chomped scalar now runs over its blank lines, up to the line break that ends the last of them, which stays in the root to end the ref's line. The value is the same in the fragment, with LF or CRLF. --- fluid_build/cli/_split_text.py | 17 ++++++++++++++++- tests/fragments/test_split.py | 30 ++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/fluid_build/cli/_split_text.py b/fluid_build/cli/_split_text.py index e2c6fcf2..62bfcb93 100644 --- a/fluid_build/cli/_split_text.py +++ b/fluid_build/cli/_split_text.py @@ -25,7 +25,8 @@ so no round-trip library is needed. A block node's end mark runs past its trailing comments and the next entry's indentation, so a span ends at the last character of the node's own content instead, plus a comment that ends -its last line. +its last line. The blank lines after a keep-chomped block scalar (``|+``) +are its content, so they move with it. """ from __future__ import annotations @@ -152,6 +153,11 @@ def _content_end(text: str, node: Any) -> int: if isinstance(node, yaml.ScalarNode): end = node.end_mark.index if node.style in ("|", ">"): + if node.value.endswith("\n\n"): + # Keep chomping (|+, >+): the blank lines after the text are + # in its value and in its span; the line break that ends the + # last of them ends the line of whatever replaces the scalar. + return _before_line_break(text, end) # A block scalar's end mark runs over the blank lines after it. start = node.start_mark.index end = start + len(text[start:end].rstrip()) @@ -167,6 +173,15 @@ def _content_end(text: str, node: Any) -> int: return max(_content_end(text, child) for child in children) +def _before_line_break(text: str, end: int) -> int: + """``end``, moved back over the line break just before it (``\\n`` or ``\\r\\n``).""" + if text.endswith("\r\n", 0, end): + return end - 2 + if text.endswith("\n", 0, end): + return end - 1 + return end + + def _with_trailing_comment(text: str, end: int) -> int: """``end``, moved over a comment that ends the same line, not over its line break.""" newline = text.find("\n", end) diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 4c833af5..2f582349 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1233,12 +1233,6 @@ def test_i10_every_block_scalar_chomping_survives_a_split( ], ids=["build-then-build", "last-item-then-key", "section-then-key"], ) -@pytest.mark.xfail( - strict=True, - reason="BUG SP-04: a keep-chomped block scalar (|+, >+) that ends a moved part loses its " - "trailing blank lines; split refuses ('does not compose to the original') instead of " - "moving them with it or splitting the data", -) def test_i10_a_keep_chomped_scalar_ending_a_moved_part_splits( tmp_path: Path, indicator: str, tail: str ) -> None: @@ -1247,6 +1241,30 @@ def test_i10_a_keep_chomped_scalar_ending_a_moved_part_splits( split_and_check(pair, "builds,sovereignty") +@pytest.mark.parametrize("line_end", ["\n", "\r\n"], ids=["lf", "crlf"]) +@pytest.mark.parametrize("parts", ["builds", "sql", "builds,sql"]) +def test_i10_the_blank_lines_of_a_keep_chomped_scalar_move_with_it( + tmp_path: Path, line_end: str, parts: str +) -> None: + """Finding SP-04: the blank lines after ``|+`` text are its value; they go to + the fragment, and the root keeps one line end after the ref.""" + text = ( + "id: x\nbuilds:\n - id: a\n properties:\n sql: |+\n SELECT 1\n\n\n" + "exposes: []\n" + ).replace("\n", line_end) + pair = solo(tmp_path, text) + + after = split_and_check(pair, parts) + + assert compose_contract(pair.split_contract).document["builds"][0]["properties"] == { + "sql": "SELECT 1\n\n\n" + } + assert text_of(after, NAME).endswith(f"{line_end}exposes: []{line_end}") + assert f"{line_end}{line_end}exposes" not in text_of(after, NAME) + if line_end == "\r\n": + assert_one_line_end(after) + + # ── I-16..I-20: quality, schema, extensions, description, part names ──── From a926e1f2af470e8a8384029cb7482c5c6566d714 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 10:13:59 +0100 Subject: [PATCH 03/30] fix(split): a tag or an anchor on a moved section's key line A node's start mark is at its tag or anchor when it has one. Written on the key's line ('sovereignty: !!map' or '&s', the mapping below), that start put the splice on the key's line, which got 'key: $ref: ...', a line that does not parse; split refused the contract (finding SP-06). The ref now takes the key's line as a flow mapping, and the tag or anchor starts the fragment, so the text is still spliced and every comment kept. The fragment's lines are dedented by the indentation of the part's first entry, not by the anchor's column, so a block scalar under it keeps its indentation. --- fluid_build/cli/_split_text.py | 26 +++++++++-- tests/fragments/test_split.py | 81 +++++++++++++++++++++++++++++++--- 2 files changed, 97 insertions(+), 10 deletions(-) diff --git a/fluid_build/cli/_split_text.py b/fluid_build/cli/_split_text.py index 62bfcb93..8c666c91 100644 --- a/fluid_build/cli/_split_text.py +++ b/fluid_build/cli/_split_text.py @@ -26,7 +26,9 @@ trailing comments and the next entry's indentation, so a span ends at the last character of the node's own content instead, plus a comment that ends its last line. The blank lines after a keep-chomped block scalar (``|+``) -are its content, so they move with it. +are its content, so they move with it. A node's start mark is at its tag or +anchor, when it has one: written on its key's line, the ref takes the key's +line, as a flow mapping. """ from __future__ import annotations @@ -140,6 +142,10 @@ def _span( if is_text or isinstance(node, yaml.ScalarNode) or node.flow_style: return start, end, flow end = _with_trailing_comment(text, end) + if key is not None and node.start_mark.line == key.start_mark.line: + # A tag or an anchor on the key's line ("key: &a", the mapping below): + # they go to the fragment with the rest, and the ref takes their place. + return start, end, flow if key is not None and node.start_mark.column <= key.start_mark.column: # A block sequence at its key's own indentation ("key:\n- item"): a # mapping in its place must sit deeper than the key. @@ -219,14 +225,26 @@ def _value(document: Any, path: Sequence[Any]) -> Any: def _fragment(text: str, cut: _Cut) -> str: """A YAML part's file: its text, moved to column 0, under its header.""" body = _render(text, cut.start, cut.end, cut.children) - column = cut.node.start_mark.column lines = body.split("\n") + column = cut.node.start_mark.column + if text.startswith(("&", "!"), cut.start) and not isinstance(cut.node, yaml.ScalarNode): + # A tag or an anchor starts the part: its entries are indented as + # the first line after it that is not blank or a comment. + column = next((_width(line) for line in lines[1:] if _is_content(line)), column) lines[1:] = [_dedent(line, column) for line in lines[1:]] newline = "\r\n" if "\r\n" in text else "\n" header = cut.site.header.replace("\n", newline) + newline if cut.site.header else "" return header + "\n".join(lines).rstrip("\r") + newline +def _is_content(line: str) -> bool: + stripped = line.strip() + return bool(stripped) and not stripped.startswith("#") + + +def _width(line: str) -> int: + return len(line) - len(line.lstrip(" ")) + + def _dedent(line: str, column: int) -> str: - width = len(line) - len(line.lstrip(" ")) - return line[min(width, column) :] + return line[min(_width(line), column) :] diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 2f582349..82a4bc87 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1912,12 +1912,6 @@ def test_i24_a_root_that_is_itself_a_ref_has_nothing_that_moves_or_only_its_own_ ], ids=["tag-on-a-section", "unused-anchor-on-a-section", "unused-anchor-on-an-extension"], ) -@pytest.mark.xfail( - strict=True, - reason="BUG SP-06: a tag or an unused anchor between a key and its block mapping puts the mapping " - "on the key's line, so the splice writes 'key: $ref: ...', which does not parse; split " - "refuses instead of falling back to a data split as it does when an alias is used", -) def test_i24_a_tag_or_an_unused_anchor_on_a_moved_section_splits( tmp_path: Path, text: str, parts: Optional[str] ) -> None: @@ -1926,6 +1920,81 @@ def test_i24_a_tag_or_an_unused_anchor_on_a_moved_section_splits( split_and_check(pair, parts) +@pytest.mark.parametrize( + "text,parts,target,root_line,body", + [ + ( + "# header\nid: x\nsovereignty: !!map\n jurisdiction: EU # kept\n", + None, + "fragments/sovereignty.yaml", + "\nsovereignty: {$ref: ./fragments/sovereignty.yaml}\n", + "!!map\njurisdiction: EU # kept\n", + ), + ( + "# header\nid: x\nsovereignty: &s # after the anchor\n jurisdiction: EU\n", + None, + "fragments/sovereignty.yaml", + "\nsovereignty: {$ref: ./fragments/sovereignty.yaml}\n", + "&s # after the anchor\njurisdiction: EU\n", + ), + ( + "# header\nid: x\nexposes:\n - exposeId: o\n contract:\n schema: !!seq\n" + " - {name: a, type: string}\n", + "schema", + "fragments/schemas/o.yaml", + "\n schema: {$ref: ./fragments/schemas/o.yaml}\n", + "!!seq\n- {name: a, type: string}\n", + ), + ( + "# header\nid: x\nbuilds:\n - &b\n id: b\n engine: sql\n", + None, + "fragments/builds/b.yaml", + "\n - $ref: ./fragments/builds/b.yaml\n", + "&b\nid: b\nengine: sql\n", + ), + ( + # The entries are indented as their own lines are, not as the anchor + # is: a block scalar under them keeps its indentation. + "# header\nid: x\nsovereignty: &s\n jurisdiction: EU\n note: | # head\n" + " kept as written\n", + None, + "fragments/sovereignty.yaml", + "\nsovereignty: {$ref: ./fragments/sovereignty.yaml}\n", + "&s\njurisdiction: EU\nnote: | # head\n kept as written\n", + ), + ( + "# header\nid: x\nbuilds:\n - &b\n id: b\n properties:\n sql: |\n" + " SELECT 1\n", + None, + "fragments/builds/b.yaml", + "\n - $ref: ./fragments/builds/b.yaml\n", + "&b\nid: b\nproperties:\n sql: |\n SELECT 1\n", + ), + ], + ids=[ + "tag", + "anchor-and-a-comment", + "tag-on-a-list", + "anchor-on-a-list-item", + "anchor-over-a-block-scalar", + "anchor-over-deeper-entries", + ], +) +def test_i24_a_tag_or_an_anchor_moves_with_its_part_and_the_ref_takes_the_key_line( + tmp_path: Path, text: str, parts: Optional[str], target: str, root_line: str, body: str +) -> None: + """Finding SP-06: the text is spliced, so every comment is kept (no data-split + fallback). The tag or anchor, and the rest of its line, start the fragment.""" + pair = solo(tmp_path, text) + + after = split_and_check(pair, parts) + + assert root_line in text_of(after, NAME) + assert text_of(after, NAME).startswith("# header\n") + assert text_of(after, target).split("\n\n", 1)[1] == body + assert_comments_conserved(text, after) + + # ── I-25 / I-26: line ends, byte-order mark, the end of the file ──────── #: SPLIT-ALL with CRLF line ends, the comment that ends a moved block included From 05a59d26b7012eaefcc0c8a587bdef11b0215513 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Wed, 28 Oct 2026 10:17:49 +0100 Subject: [PATCH 04/30] fix(split): a block scalar's header comment stays beside its ref 'sql: | # the main query' put the comment inside the span a moved text part replaces with a ref, and a text file cannot hold a YAML comment: the comment was in no file after the split, and nothing said so (finding SP-10). The comment on a block scalar's header line now stays in the file that holds the ref, on the ref's line: the root, or the fragment the part was cut from. The generated roots of the comment check put trailing comments on header lines too. --- fluid_build/cli/_split_text.py | 25 ++++++++++++++++-- tests/fragments/test_split.py | 47 +++++++++++++++++++++++++--------- 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/fluid_build/cli/_split_text.py b/fluid_build/cli/_split_text.py index 8c666c91..597ba1a6 100644 --- a/fluid_build/cli/_split_text.py +++ b/fluid_build/cli/_split_text.py @@ -19,7 +19,9 @@ lines. Each moved part takes its own text to its fragment, comments inside it included, and a ``$ref`` takes its place. A part inside another (a build's SQL) is cut out of its container's text first, so the container's -fragment holds the ref to it. +fragment holds the ref to it. The comment on a block scalar's header line +(``sql: | # the query``) stays beside the ref that replaces the scalar: a +text file cannot hold it. PyYAML's compose marks give each part's span; PyYAML is a base dependency, so no round-trip library is needed. A block node's end mark runs past its @@ -33,6 +35,7 @@ from __future__ import annotations +import re from typing import Any, Dict, List, Optional, Sequence, Tuple import yaml @@ -42,6 +45,10 @@ __all__ = ["SpliceRefused", "split_yaml_text"] +#: The comment on a block scalar's header line, the blanks before it included: +#: a ``#`` that follows a blank, so a ``#`` inside a tag is not one. +_HEADER_COMMENT = re.compile(r"[ \t]+#.*$") + class SpliceRefused(Exception): """The text cannot be spliced safely; the caller splits the parsed contract instead.""" @@ -140,7 +147,7 @@ def _span( end = _content_end(text, node) flow = "{$ref: " + _yaml_string(ref) + "}" if is_text or isinstance(node, yaml.ScalarNode) or node.flow_style: - return start, end, flow + return start, end, flow + _header_comment(text, node) end = _with_trailing_comment(text, end) if key is not None and node.start_mark.line == key.start_mark.line: # A tag or an anchor on the key's line ("key: &a", the mapping below): @@ -154,6 +161,20 @@ def _span( return start, end, "$ref: " + _yaml_string(ref) +def _header_comment(text: str, node: Any) -> str: + """The comment on a block scalar's header line, as written; ``""`` for none. + + The ref that replaces the scalar keeps it: a text file cannot hold it. + """ + if not (isinstance(node, yaml.ScalarNode) and node.style in ("|", ">")): + return "" + start = node.start_mark.index + newline = text.find("\n", start) + header = text[start : len(text) if newline < 0 else newline].rstrip("\r") + found = _HEADER_COMMENT.search(header) + return found.group(0) if found else "" + + def _content_end(text: str, node: Any) -> int: """Just past the last character of ``node``'s own content.""" if isinstance(node, yaml.ScalarNode): diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 82a4bc87..3db4e91f 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -2589,28 +2589,50 @@ def test_i34_a_refused_split_leaves_the_tree_exactly_as_it_was( assert digest(tmp_path) == before -def test_i03_a_comment_on_the_header_of_a_moved_text_scalar_is_dropped_without_a_word( - tmp_path: Path, capsys: pytest.CaptureFixture[str] +@pytest.mark.parametrize("line_end", ["\n", "\r\n"], ids=["lf", "crlf"]) +def test_i03_a_comment_on_the_header_of_a_moved_text_scalar_stays_beside_its_ref( + tmp_path: Path, capsys: pytest.CaptureFixture[str], line_end: str ) -> None: - """SG-32 characterisation (finding SP-10): ``sql: | # the main query``. The - comment is outside the string's value, inside the span that becomes a ref, - and a text file cannot hold it. Nothing says it was left out.""" + """Finding SP-10: ``sql: | # the main query``. A text file cannot hold the + comment, so it stays on the line of the ref that replaces the scalar.""" text = ( "id: x\nbuilds:\n - id: a\n engine: sql\n properties:\n" - " sql: | # the main query\n SELECT 1\n" + " sql: |- # the main query\n SELECT 1\n" "description:\n purpose: > # about\n A folded\n purpose.\n usage: u # kept\n" - ) + ).replace("\n", line_end) pair = solo(tmp_path, text) capsys.readouterr() after = split_and_check(pair, "sql,description") - everything = b"\n".join(after.values()).decode("utf-8") - assert "the main query" not in everything and "# about" not in everything - assert "# kept" in text_of(after, NAME) # a comment after a scalar's end is the root's + root = text_of(after, NAME) + assert f" sql: {{$ref: ./sql/a.sql}} # the main query{line_end}" in root + assert f" purpose: {{$ref: ./docs/purpose.md}} # about{line_end}" in root + assert f" usage: {{$ref: ./docs/usage.md}} # kept{line_end}" in root + assert after["sql/a.sql"] == b"SELECT 1" + assert_comments_conserved(text, after) assert "were not kept" not in capsys.readouterr().err +def test_i03_the_header_comment_of_text_cut_from_a_moved_part_stays_in_its_fragment( + tmp_path: Path, +) -> None: + """Finding SP-10: the comment stays in the file that holds the ref: here the + build's fragment, where the sql ref is.""" + text = ( + "id: x\nbuilds:\n - id: a\n engine: sql\n properties:\n" + " sql: !!str | # the main query\n SELECT 1\n parameters: {a: 1}\n" + ) + pair = solo(tmp_path, text) + + after = split_and_check(pair, "builds,sql") + + fragment = text_of(after, "fragments/builds/a.yaml") + assert " sql: {$ref: ../../sql/a.sql} # the main query\n" in fragment + assert after["sql/a.sql"] == b"SELECT 1\n" + assert_comments_conserved(text, after) + + @pytest.mark.parametrize( "name,content,rc", [ @@ -3183,7 +3205,8 @@ def _block_strings(dumper: yaml.SafeDumper, data: str) -> Any: def _with_comments(text: str, rng: random.Random, trailing: bool) -> str: - """Full-line comments at random lines, and trailing ones if ``trailing``. + """Full-line comments at random lines, and trailing ones if ``trailing``, + a block scalar's header line included. Lines inside a block scalar are left alone (a ``#`` there is text). """ @@ -3209,7 +3232,7 @@ def _with_comments(text: str, rng: random.Random, trailing: bool) -> str: for n, line in enumerate(lines): if n in full: out.append(" " * (len(line) - len(line.lstrip(" "))) + f"# full-{n}") - if n in tail and not re.search(r"[|>][-+0-9]*$", line.rstrip()): + if n in tail: line = f"{line} # trail-{n}" out.append(line) return "\n".join(out) From 0379c13539f637dd0d7d1ee4815390e2ec57db10 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:01:01 +0100 Subject: [PATCH 05/30] fix(split): a relative $ref in a moved part names the same file from there A relative $ref resolves against the directory of the file that holds it. A part that moved to fragments/builds/t.yaml took its refs along unchanged, so './sql/t.sql' now named fragments/builds/sql/t.sql: the split did not compose, and split refused any contract with a ref in a build or an expose, including one split once with --parts sql, while --dry-run listed the files and exited 0 (finding SP-02). Split now rewrites each relative ref in a part that moves to name the same file from the part's new one ('../../sql/t.sql', '../../cols.yaml#/a'), in the text splice (in the ref's own quoting, every comment kept) and in the data split. The ref's own path is kept as written: only the climb out of the fragment's directory goes in front, so a '..' in it is never folded past a symlinked directory. A URL, an absolute path and a same-document '#/...' pointer are left alone, by the predicate the ref resolver uses (ref_confinement.is_relative_file_ref). A second pass ('--parts sql', then '--parts builds') ends where one split of both parts does. What rebasing cannot fix is still refused and taken back: a file ref whose pointer runs through a part that moves out of the root ('./contract.fluid.yaml#/builds/0/id'). The compose-and-compare check stays. --- fluid_build/cli/_split_text.py | 110 ++++++++++++-- fluid_build/cli/forge_contract_fragments.py | 75 +++++++++- fluid_build/cli/split.py | 4 +- fluid_build/util/ref_confinement.py | 19 ++- tests/fragments/test_split.py | 154 +++++++++++++++++--- 5 files changed, 321 insertions(+), 41 deletions(-) diff --git a/fluid_build/cli/_split_text.py b/fluid_build/cli/_split_text.py index 597ba1a6..15972add 100644 --- a/fluid_build/cli/_split_text.py +++ b/fluid_build/cli/_split_text.py @@ -19,7 +19,9 @@ lines. Each moved part takes its own text to its fragment, comments inside it included, and a ``$ref`` takes its place. A part inside another (a build's SQL) is cut out of its container's text first, so the container's -fragment holds the ref to it. The comment on a block scalar's header line +fragment holds the ref to it. A relative ``$ref`` already in a moved part is +rewritten in its text, in its own quoting, to name the same file from the +fragment. The comment on a block scalar's header line (``sql: | # the query``) stays beside the ref that replaces the scalar: a text file cannot hold it. @@ -40,7 +42,12 @@ import yaml -from fluid_build.cli.forge_contract_fragments import FragmentSite, container_of, ref_between +from fluid_build.cli.forge_contract_fragments import ( + FragmentSite, + container_of, + rebased_ref, + ref_between, +) from fluid_build.util.safe_yaml import compose_yaml_safe __all__ = ["SpliceRefused", "split_yaml_text"] @@ -49,19 +56,35 @@ #: a ``#`` that follows a blank, so a ``#`` inside a tag is not one. _HEADER_COMMENT = re.compile(r"[ \t]+#.*$") +#: The tag of a node that parses as a string. +_STR = "tag:yaml.org,2002:str" + +#: A line width no scalar reaches: the dumper breaks none over two lines. +_ONE_LINE = 2**31 - 1 + class SpliceRefused(Exception): """The text cannot be spliced safely; the caller splits the parsed contract instead.""" -class _Cut: - def __init__(self, site: FragmentSite, node: Any, start: int, end: int, replacement: str): - self.site = site - self.node = node +class _Edit: + """A span of the text and what takes its place.""" + + def __init__(self, start: int, end: int, replacement: str): self.start = start self.end = end self.replacement = replacement - self.children: List["_Cut"] = [] + + +class _Cut(_Edit): + """A part that moves: its span, the ref that takes its place, and the edits + inside it (the parts cut out of it, and its rebased refs).""" + + def __init__(self, site: FragmentSite, node: Any, start: int, end: int, replacement: str): + super().__init__(start, end, replacement) + self.site = site + self.node = node + self.children: List[_Edit] = [] def split_yaml_text( @@ -73,8 +96,9 @@ def split_yaml_text( Raises: SpliceRefused: the text holds YAML aliases (moving an anchor or an - alias would change what the other one means), or a site is not - where the parsed contract says it is. + alias would change what the other one means), a site is not + where the parsed contract says it is, or a ``$ref`` to rebase is + a block scalar. """ if _has_aliases(text): raise SpliceRefused("the contract uses YAML aliases") @@ -102,6 +126,7 @@ def split_yaml_text( if cut.site.text: fragments[cut.site.target] = _value(contract, cut.site.path) else: + cut.children += _rebased_refs(cut) fragments[cut.site.target] = _fragment(text, cut) return _render(text, 0, len(text), top), fragments @@ -219,20 +244,75 @@ def _with_trailing_comment(text: str, end: int) -> int: return line_end if rest.strip().startswith("#") else end +def _rebased_refs(cut: _Cut) -> List[_Edit]: + """An edit for each ``$ref`` in the part ``cut`` moves (not in a part cut out + of it) that names a file relative to the root: it is rewritten to name that + file from the part's own file (:func:`rebased_ref`), in the quoting it had.""" + inner = {id(child.node) for child in cut.children if isinstance(child, _Cut)} + edits: List[_Edit] = [] + stack = [cut.node] + while stack: + node = stack.pop() + if id(node) in inner: + continue + if isinstance(node, yaml.SequenceNode): + stack.extend(node.value) + elif isinstance(node, yaml.MappingNode): + for key, value in node.value: + if _is_ref_value(key, value): + ref = rebased_ref(value.value, cut.site.target) + if ref != value.value: + edits.append(_ref_edit(value, ref)) + else: + stack.append(value) + return edits + + +def _is_ref_value(key: Any, value: Any) -> bool: + """``key: value`` is a ``$ref`` whose value is a string.""" + return ( + isinstance(key, yaml.ScalarNode) + and key.tag == _STR + and key.value == "$ref" + and isinstance(value, yaml.ScalarNode) + and value.tag == _STR + ) + + +def _ref_edit(node: Any, ref: str) -> _Edit: + """The edit that writes ``ref`` over the scalar ``node``.""" + if node.style in ("|", ">"): + # A block scalar's span runs into the lines after it; the data split + # rewrites the ref instead. + raise SpliceRefused("a $ref value is written as a block scalar") + if node.style == "'" and ref.isprintable(): + text = "'" + ref.replace("'", "''") + "'" + elif node.style == '"': + text = _double_quoted(ref) + else: + text = _yaml_string(ref) + return _Edit(node.start_mark.index, node.end_mark.index, text) + + def _yaml_string(value: str) -> str: """``value`` as a YAML scalar: plain when it is safe, quoted otherwise.""" if value and all(c.isalnum() or c in "._/-" for c in value): return value - return yaml.safe_dump(value, default_style='"').strip() + return _double_quoted(value) + + +def _double_quoted(value: str) -> str: + """``value`` as a double-quoted YAML scalar, on one line however long it is.""" + return yaml.safe_dump(value, default_style='"', width=_ONE_LINE).strip() -def _render(text: str, lo: int, hi: int, cuts: Sequence[_Cut]) -> str: +def _render(text: str, lo: int, hi: int, edits: Sequence[_Edit]) -> str: out: List[str] = [] pos = lo - for cut in sorted(cuts, key=lambda c: c.start): - out.append(text[pos : cut.start]) - out.append(cut.replacement) - pos = cut.end + for edit in sorted(edits, key=lambda e: e.start): + out.append(text[pos : edit.start]) + out.append(edit.replacement) + pos = edit.end out.append(text[pos:hi]) return "".join(out) diff --git a/fluid_build/cli/forge_contract_fragments.py b/fluid_build/cli/forge_contract_fragments.py index b9dfddb9..0e7fa4d2 100644 --- a/fluid_build/cli/forge_contract_fragments.py +++ b/fluid_build/cli/forge_contract_fragments.py @@ -24,6 +24,8 @@ :func:`fragment_sites` says where each part sits and which file it goes to, for this dict-level splitter and for ``fluid split``'s text splicer (``cli/_split_text.py``), so the two move the same parts to the same files. +Both rewrite a relative ``$ref`` inside a part that moves so that it names +the same file from the part's new file (:func:`rebased_ref`). The splitting is purely mechanical — no LLM involved. This guarantees deterministic, always-correct output regardless of the model used to @@ -50,6 +52,8 @@ import yaml +from fluid_build.util.ref_confinement import is_relative_file_ref + # ── Splittable parts ───────────────────────────────────────────────────── # Keys extracted into their own fragment file. Maps contract key → @@ -280,16 +284,79 @@ def ref_between(target: str, container: Optional[str]) -> str: return rel if rel.startswith("../") else f"./{rel}" +def rebased_ref(ref: Any, container: Optional[str]) -> Any: + """``ref``, a ``$ref`` value written in the root, as the file ``container`` + names the same target (``None``: the root, where it stays as it is). + + A relative ref resolves against the directory of the file that holds it, + so one in a part that moves to ``fragments/builds/t.yaml`` climbs back out + of that directory first: ``./sql/t.sql`` becomes ``../../sql/t.sql``, and + a ``#/pointer`` after the file stays. The ref's own path is kept as it was + written, so no ``..`` in it is folded away (that would be wrong past a + symlinked directory); only the directories it shares with the container's + lead, and a leading ``./``, are dropped. A URL, an absolute path, a + same-document ``#/...`` pointer and a value that is not a string are not + relative to the file, and come back unchanged. + """ + if container is None or not isinstance(ref, str): + return ref + file_part, mark, pointer = ref.partition("#") + if not is_relative_file_ref(file_part): + return ref + path = file_part + while path.startswith("./"): + path = path[2:].lstrip("/") + names = path.split("/") + climb = [name for name in posixpath.dirname(container).split("/") if name] + # A directory the container's path starts with is a real one (split writes + # through no symlink), so going into it and back out is no step at all. + while len(names) > 1 and climb and names[0] == climb[0]: + names.pop(0) + climb.pop(0) + rel = "/".join([".."] * len(climb) + names) + return (rel if rel.startswith("../") else f"./{rel}") + mark + pointer + + +def _rebased_refs(value: Any, at: Tuple[Any, ...], container: str, inner: Collection[Any]) -> Any: + """``value``, the part at ``at`` that goes to the file ``container``, with every + ``$ref`` written in the root rebased to that file (:func:`rebased_ref`). + + The parts in ``inner`` moved out of it first: each one's ref already names + its file from ``container``. What holds no ref to rebase is handed back as + it is, and what does is a copy, so a value a YAML alias shares with a part + that stays is never changed for both. + """ + out: Any + if at in inner: + return value + if isinstance(value, dict): + out = {k: _rebased_refs(v, (*at, k), container, inner) for k, v in value.items()} + if "$ref" in value: + out["$ref"] = rebased_ref(value["$ref"], container) + changed = any(out[k] is not value[k] for k in value) + elif isinstance(value, list): + out = [_rebased_refs(v, (*at, i), container, inner) for i, v in enumerate(value)] + changed = any(new is not old for new, old in zip(out, value, strict=True)) + else: + return value + return out if changed else value + + def container_of(site: FragmentSite, sites: Iterable[FragmentSite]) -> Optional[str]: """The fragment ``site`` ends up in: the nearest moved part around it, or the root.""" best: Optional[FragmentSite] = None for other in sites: - if len(other.path) < len(site.path) and site.path[: len(other.path)] == other.path: + if _inside(site.path, other.path): if best is None or len(other.path) > len(best.path): best = other return best.target if best is not None else None +def _inside(path: Tuple[Any, ...], outer: Tuple[Any, ...]) -> bool: + """``path`` is below ``outer`` (not ``outer`` itself).""" + return len(outer) < len(path) and path[: len(outer)] == outer + + def split_contract_to_fragments( contract: Dict[str, Any], parts: Iterable[str] = DEFAULT_PARTS, @@ -303,7 +370,9 @@ def split_contract_to_fragments( text part (``sql``, ``description``). Only parts that actually exist, are non-empty, and are not already a - ``$ref`` are split. The input *contract* is not mutated. + ``$ref`` are split. A relative ``$ref`` inside a part that moves is + rewritten to name the same file from the part's new one + (:func:`rebased_ref`). The input *contract* is not mutated. """ if not isinstance(contract, dict): return contract, {} @@ -319,6 +388,8 @@ def split_contract_to_fragments( if site.text: fragments[site.target] = value else: + inner = {other.path for other in sites if _inside(other.path, site.path)} + value = _rebased_refs(value, site.path, site.target, inner) fragments[site.target] = (site.header + "\n" if site.header else "") + yaml.dump( value, Dumper=_exact_dumper(), diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index e363574c..d2648cea 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -68,7 +68,9 @@ def register(subparsers: argparse._SubParsersAction) -> None: " extensions.: that one extension block\n" " description: the description texts to docs/.md\n\n" "A part that is already a $ref is left as it is, so splitting twice\n" - "changes nothing. A file that exists is never written over.\n\n" + "changes nothing. A file that exists is never written over. A relative\n" + "$ref in a part that moves is rewritten to name the same file from\n" + "the part's new one.\n\n" "Examples:\n" " fluid split contract.fluid.yaml # split in place\n" " fluid split contract.fluid.yaml --parts builds,sql # choose the parts\n" diff --git a/fluid_build/util/ref_confinement.py b/fluid_build/util/ref_confinement.py index 6dc124ac..2e1c7e66 100644 --- a/fluid_build/util/ref_confinement.py +++ b/fluid_build/util/ref_confinement.py @@ -70,6 +70,7 @@ "RefResolutionError", "confine_ref", "format_pointer", + "is_relative_file_ref", "is_within", "iter_external_refs", "ref_location", @@ -160,6 +161,20 @@ def _is_absolute(file_part: str) -> bool: ) +def _is_remote(file_part: str) -> bool: + return file_part.startswith("//") or bool( + _SCHEME_RE.match(file_part) and not _DRIVE_RE.match(file_part) + ) + + +def is_relative_file_ref(file_part: str) -> bool: + """True when ``file_part``, a ``$ref``'s part before ``#``, names a file by a + relative path: the only kind :func:`confine_ref` resolves (against the + directory of the file that holds the ref). Empty (a same-document ``#/...`` + pointer), a URL or an absolute path is not one.""" + return bool(file_part) and not _is_remote(file_part) and not _is_absolute(file_part) + + def confine_ref( ref: str, file_part: str, @@ -202,9 +217,7 @@ def _refuse(reason: str) -> RefConfinementError: ignored_ref_root_env=ignored_ref_root_env, ) - if file_part.startswith("//") or ( - _SCHEME_RE.match(file_part) and not _DRIVE_RE.match(file_part) - ): + if _is_remote(file_part): raise _refuse( "is a URL; remote refs (including file://) are not supported" + (" — use a relative path to a file inside the ref root" if root else "") diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 3db4e91f..156808e8 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1480,12 +1480,9 @@ def test_i20_a_part_named_twice_acts_like_one(tmp_path: Path) -> None: } -@pytest.mark.xfail( - strict=True, - reason="BUG SP-02: a relative $ref inside a moved part is copied unchanged into the fragment, " - "so it points one or two directories off; split refuses instead of rebasing it", -) def test_i21_refs_in_moved_parts_are_rebased_to_the_fragment(tmp_path: Path) -> None: + """Finding SP-02: a relative ref in a part that moves names the same file + from the fragment; its quoting and a ``#/pointer`` after the file stay.""" pair = make_pair(tmp_path, REFS_IN_PARTS_FILES) after = split_and_check(pair, envs=ENVS) @@ -1493,13 +1490,11 @@ def test_i21_refs_in_moved_parts_are_rebased_to_the_fragment(tmp_path: Path) -> assert "$ref: ../../sql/transform.sql" in text_of(after, "fragments/builds/transform.yaml") assert "../../schemas/orders.yaml" in text_of(after, "fragments/exposes/orders.yaml") assert "../../cols.yaml#/a" in text_of(after, "fragments/exposes/totals.yaml") + assert '- {$ref: "../../cols.yaml#/a", description: x}' in text_of( + after, "fragments/exposes/totals.yaml" + ) -@pytest.mark.xfail( - strict=True, - reason="BUG SP-02: split in two passes (--parts sql, then --parts builds) fails: the sql ref " - "written by the first pass is not rebased when the second moves the build", -) def test_i21_sql_first_then_builds_is_a_two_pass_split_that_works(tmp_path: Path) -> None: pair = make_pair( tmp_path, {NAME: "id: x\nbuilds:\n - id: t\n properties: {sql: SELECT 1}\n"} @@ -1509,21 +1504,143 @@ def test_i21_sql_first_then_builds_is_a_two_pass_split_that_works(tmp_path: Path assert split(pair.split_contract, "--parts", "builds") == 0 assert_same_bundle(pair) + # The second pass rebased the ref the first one wrote: the two passes end + # where one split of both parts does. + one_pass = make_pair(tmp_path / "one", {NAME: pair.flat_contract.read_text("utf-8")}) + assert split(one_pass.split_contract, "--parts", "builds,sql") == 0 + assert files_of(pair.split) == files_of(one_pass.split) + + +#: A file ref whose pointer runs through a part that moves: the place it names +#: is in the root's text only until the split, so no rebasing can keep it. +#: (A same-document pointer, ``#/builds/0/id``, survives any split: C-24.) +REF_INTO_A_MOVED_PART = """\ +id: x +builds: + - id: t + engine: sql +exposes: + - exposeId: o + kind: table + description: {$ref: "./contract.fluid.yaml#/builds/0/id"} +""" -def test_i21_a_refused_split_for_refs_it_cannot_rebase_changes_no_file( +def test_i21_a_file_ref_into_a_part_that_moves_refuses_the_split_and_changes_no_file( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: - """Today's safe outcome: the split is refused before anything is changed.""" - pair = make_pair(tmp_path, REFS_IN_PARTS_FILES) + """What rebasing cannot fix (SP-02): the ref is rebased, but the place its + pointer names leaves the root, so the split does not compose to the original; + it is refused and taken back.""" + pair = solo(tmp_path, REF_INTO_A_MOVED_PART) files = files_of(pair.split) assert split(pair.split_contract) == 1 - assert "Split not written" in capsys.readouterr().err + err = capsys.readouterr().err + assert "Split not written" in err and "../../contract.fluid.yaml#/builds/0/id" in err assert files_of(pair.split) == files +@pytest.mark.parametrize("parts", [None, "builds,exposes,schema"]) +def test_i21_the_data_split_rebases_the_refs_in_moved_parts_too( + tmp_path: Path, parts: Optional[str], capsys: pytest.CaptureFixture[str] +) -> None: + """SP-02 in the data split: a JSON root, and a YAML root with an alias, are + split as parsed data, and their refs are rebased the same way.""" + as_json = json.dumps(yaml.safe_load(REFS_IN_PARTS), indent=2) + "\n" + with_alias = REFS_IN_PARTS.replace("id: x\n", "id: x\nlabels: &l {a: 1}\ntags: *l\n") + for name, root in (("json", as_json), ("alias", with_alias)): + files = {**REFS_IN_PARTS_FILES, NAME: root} + contract = JSON_NAME if name == "json" else NAME + if name == "json": + files[JSON_NAME] = files.pop(NAME) + pair = make_pair(tmp_path / name, files, contract) + capsys.readouterr() + + after = split_and_check(pair, parts) + + def loaded(rel: str) -> Any: + return yaml.safe_load(text_of(after, rel)) + + build = loaded("fragments/builds/transform.yaml") + assert build["properties"]["sql"] == {"$ref": "../../sql/transform.sql"} + orders = loaded("fragments/exposes/orders.yaml") + assert orders["contract"]["schema"] == {"$ref": "../../schemas/orders.yaml"} + totals = loaded("fragments/exposes/totals.yaml")["contract"]["schema"] + if parts: # the columns went on to a file of their own, the ref with them + assert totals == {"$ref": "../schemas/totals.yaml"} + totals = loaded("fragments/schemas/totals.yaml") + assert totals == [{"$ref": "../../cols.yaml#/a", "description": "x"}] + if name == "alias": + assert "were not kept" in capsys.readouterr().err # it was the data split + + +def test_i21_the_dry_run_of_a_split_with_refs_lists_what_the_real_run_writes( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-02 repro: the dry run listed the fragment and exited 0 while the real run + refused; now both agree.""" + text = "id: x\nbuilds:\n - id: t\n engine: sql\n properties:\n sql: {$ref: ./sql/t.sql}\n" + pair = make_pair(tmp_path, {NAME: text, "sql/t.sql": "SELECT 1\n"}) + capsys.readouterr() + + assert split(pair.split_contract, "--dry-run") == 0 + listed = _listed(capsys.readouterr().err) + after = split_and_check(pair) + + assert listed == new_files(pair, after) == ["fragments/builds/t.yaml"] + assert "sql: {$ref: ../../sql/t.sql}" in text_of(after, "fragments/builds/t.yaml") + + +@pytest.mark.parametrize( + "ref,container,rebased", + [ + ("./sql/t.sql", "fragments/builds/t.yaml", "../../sql/t.sql"), + ("sql/t.sql", "fragments/builds/t.yaml", "../../sql/t.sql"), + ("./cols.yaml#/a/0", "fragments/exposes/o.yaml", "../../cols.yaml#/a/0"), + ("../shared/p.yaml", "fragments/builds/t.yaml", "../../../shared/p.yaml"), + ("./fragments/x.yaml", "fragments/sovereignty.yaml", "./x.yaml"), + ("fragments/other/x.yaml", "fragments/builds/t.yaml", "../other/x.yaml"), + # The ref's own path is kept as written: a ".." in it is not folded away. + ("sql/../sql/t.sql", "fragments/builds/t.yaml", "../../sql/../sql/t.sql"), + ("./sql/t.sql", "sql/t.yaml", "./t.sql"), + ("./x.yaml", None, "./x.yaml"), + ("#/builds/0/id", "fragments/builds/t.yaml", "#/builds/0/id"), + ("https://example.com/x.yaml", "fragments/builds/t.yaml", "https://example.com/x.yaml"), + ("//host/x.yaml", "fragments/builds/t.yaml", "//host/x.yaml"), + ("/abs/x.yaml", "fragments/builds/t.yaml", "/abs/x.yaml"), + ("C:\\x.yaml", "fragments/builds/t.yaml", "C:\\x.yaml"), + (5, "fragments/builds/t.yaml", 5), + ], +) +def test_i21_a_ref_is_rebased_by_its_path_alone(ref: Any, container: Any, rebased: Any) -> None: + from fluid_build.cli.forge_contract_fragments import rebased_ref + + assert rebased_ref(ref, container) == rebased + + +def test_i21_the_dict_level_splitter_rebases_a_ref_once_and_never_its_own() -> None: + """The sql ref split writes into the build's file names it from there already; + only the ref that was in the contract is rebased.""" + from fluid_build.cli.forge_contract_fragments import split_contract_to_fragments + + contract = { + "builds": [ + {"id": "a", "properties": {"sql": "SELECT 1\n", "parameters": {"$ref": "./p.yaml"}}} + ] + } + + root, files = split_contract_to_fragments(contract, ("builds", "sql")) + + build = yaml.safe_load(files["fragments/builds/a.yaml"]) + assert build["properties"] == { + "sql": {"$ref": "../../sql/a.sql"}, + "parameters": {"$ref": "../../p.yaml"}, + } + assert contract["builds"][0]["properties"]["parameters"] == {"$ref": "./p.yaml"} # unchanged + + def test_i21_refs_that_stay_in_the_root_are_not_touched(tmp_path: Path) -> None: text = REFS_IN_PARTS.replace( "id: x\n", "id: x\nmetadata:\n owner: {$ref: ./team.yaml}\n" @@ -2570,7 +2687,7 @@ def test_i34_a_refused_split_leaves_the_tree_exactly_as_it_was( tmp_path: Path, failure: str ) -> None: if failure == "refs-it-cannot-rebase": - pair = make_pair(tmp_path, REFS_IN_PARTS_FILES) + pair = solo(tmp_path, REF_INTO_A_MOVED_PART) args: List[Any] = [] elif failure == "out-elsewhere": text = ( @@ -2800,11 +2917,6 @@ def test_i36_refs_in_the_root_that_need_the_ref_root_survive_a_split( assert "owner: {$ref: ../shared/team.yaml}" in pair.split_contract.read_text("utf-8") -@pytest.mark.xfail( - strict=True, - reason="BUG SP-02: a relative $ref inside a moved part is not rebased; with FLUID_REF_ROOT set " - "(so the original composes and split can check its work) split refuses", -) def test_i36_a_moved_part_with_a_ref_that_needs_the_ref_root_splits_with_the_env_set( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -2815,6 +2927,8 @@ def test_i36_a_moved_part_with_a_ref_that_needs_the_ref_root_splits_with_the_env flat = _compose_with_ref_root(pair.flat_contract, pair.flat, monkeypatch) assert _compose_with_ref_root(pair.split_contract, pair.split, monkeypatch) == flat + build = (pair.split / "product/fragments/builds/a.yaml").read_text("utf-8") + assert "parameters: {$ref: ../../../shared/params.yaml}" in build def test_i36_split_without_the_ref_root_set_leaves_a_contract_that_still_composes( From c08f86cfcd0e44d15cd771d5a911bb514ffad11f Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:03:21 +0100 Subject: [PATCH 06/30] fix(split): a refused split takes back the directories it made A refused split said 'nothing was changed', and the module promises as much, but it left behind every directory it had made: an empty fragments/ and fragments/builds/, or the whole --out directory. A leftover fragments/ changes what 'fluid bundle ' does without --out (it writes contract.bundled.fluid.yaml instead of printing). A write that failed on a lone surrogate also left the empty file it had just created, which the next run numbered around (finding SP-05). Split now makes each directory itself and records the ones it creates, records a fragment the moment its file exists, and takes all of it back on a refusal, directories last and deepest first. Every text is encoded before anything is made, so one with no UTF-8 form is refused with nothing to take back, and the message names its file. 'nothing was changed' now ends the message only after the take-back. --- fluid_build/cli/split.py | 64 +++++++++++++++++++++++------------ tests/fragments/test_split.py | 21 ++++++++---- 2 files changed, 56 insertions(+), 29 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index d2648cea..6038d8e3 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -267,9 +267,11 @@ def _write_checked( original composed to. Fragments first and the root last, so the contract stays whole until the - root changes. When the split contract does not compose to the same - document (a part that could not be moved exactly), every file written is - taken back and the reason returned. + root changes. When a write fails, or the split contract does not compose + to the same document (a part that could not be moved exactly), all of it + is taken back: every file written, the root's old text, and every + directory made for them, ``--out`` included, so the tree is as it was. + The reason is returned. """ from fluid_build.loader import compose_contract @@ -281,33 +283,51 @@ def _write_checked( link = _symlink_on_the_way(out_dir, rel_path) if link is not None: return f"{link} is a symlink; split writes no file through one" - original_root: Optional[str] = None - if root_out.exists(): - with open(root_out, encoding="utf-8", newline="") as handle: - original_root = handle.read() + # Every text is encoded before a file or a directory is made: one with no + # UTF-8 form (a lone surrogate) is refused with nothing to take back. The + # bytes are written as they are, so a \r in a text stays a \r. + data: Dict[Path, bytes] = {} + for rel_path, content in fragments.items(): + try: + data[out_dir / rel_path] = content.encode("utf-8") + except UnicodeEncodeError as exc: + return f"{rel_path} cannot be written as UTF-8: {exc}" + root_data = root_text.encode("utf-8") + original_root: Optional[bytes] = root_out.read_bytes() if root_out.exists() else None + made: List[Path] = [] written: List[Path] = [] try: - for rel_path, content in fragments.items(): - target = out_dir / rel_path - target.parent.mkdir(parents=True, exist_ok=True) - # newline="" writes the text as it is: a \r in it stays a \r. - with open(target, "x", encoding="utf-8", newline="") as handle: + for target, content in data.items(): + _make_dirs(target.parent, made) + with open(target, "xb") as handle: + # Taken back from here on, whatever happens to the write. + written.append(target) handle.write(content) - written.append(target) - root_out.parent.mkdir(parents=True, exist_ok=True) - with open(root_out, "w", encoding="utf-8", newline="") as handle: - handle.write(root_text) + _make_dirs(root_out.parent, made) + with open(root_out, "wb") as handle: + handle.write(root_data) if compose_contract(root_out).document != before: - raise ValueError( - "the split contract does not compose to the original one; nothing was changed" - ) + raise ValueError("the split contract does not compose to the original one") except Exception as exc: # noqa: BLE001 - every failure is taken back for path in written: path.unlink(missing_ok=True) if original_root is not None: - with open(root_out, "w", encoding="utf-8", newline="") as handle: - handle.write(original_root) + root_out.write_bytes(original_root) elif root_out.exists(): root_out.unlink() - return str(exc) + for directory in reversed(made): + directory.rmdir() + return f"{exc}; nothing was changed" return None + + +def _make_dirs(directory: Path, made: List[Path]) -> None: + """``directory`` and each missing directory above it, every one it creates + added to ``made`` (``mkdir(parents=True)`` says nothing of which it made).""" + missing: List[Path] = [] + while directory != directory.parent and not directory.exists(): + missing.append(directory) + directory = directory.parent + for path in reversed(missing): + path.mkdir() + made.append(path) diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 156808e8..d9bee458 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -2630,6 +2630,7 @@ def test_i34_an_unwritable_fragment_directory_stops_the_split_and_leaves_the_roo locked.mkdir(parents=True) locked.chmod(0o555) root = pair.split_contract.read_bytes() + before = digest(pair.split) try: assert split(pair.split_contract) == 1 finally: @@ -2638,6 +2639,7 @@ def test_i34_an_unwritable_fragment_directory_stops_the_split_and_leaves_the_roo assert "Split not written" in capsys.readouterr().err assert pair.split_contract.read_bytes() == root assert not files_of(pair.split / "fragments") # what was written is taken back + assert digest(pair.split) == before # and fragments/builds/, made for it (SP-05) @needs_posix_permissions @@ -2677,15 +2679,13 @@ def test_i34_an_unwritable_root_file_is_an_unexpected_error_today( @pytest.mark.parametrize("failure", ["refs-it-cannot-rebase", "out-elsewhere", "lone-surrogate"]) -@pytest.mark.xfail( - strict=True, - reason="BUG SP-05: a refused split says 'nothing was changed' but leaves the directories it " - "made (an empty fragments/ changes what `fluid bundle` does without --out), and for a " - "failed write also the empty file it was writing", -) def test_i34_a_refused_split_leaves_the_tree_exactly_as_it_was( - tmp_path: Path, failure: str + tmp_path: Path, failure: str, capsys: pytest.CaptureFixture[str] ) -> None: + """Finding SP-05: a refusal takes back every directory it made, the whole + ``--out`` directory included (a leftover ``fragments/`` changes what + ``fluid bundle`` does without ``--out``), and the file it was writing; a + text with no UTF-8 form is refused before anything is made.""" if failure == "refs-it-cannot-rebase": pair = solo(tmp_path, REF_INTO_A_MOVED_PART) args: List[Any] = [] @@ -2700,10 +2700,17 @@ def test_i34_a_refused_split_leaves_the_tree_exactly_as_it_was( pair = solo(tmp_path, text) args = ["--parts", "sql"] before = digest(tmp_path) + capsys.readouterr() assert split(pair.split_contract, *args) == 1 assert digest(tmp_path) == before + err = capsys.readouterr().err + assert "Split not written" in err + if failure == "lone-surrogate": + assert "sql/t.sql cannot be written as UTF-8" in err + else: + assert err.rstrip().endswith("nothing was changed") @pytest.mark.parametrize("line_end", ["\n", "\r\n"], ids=["lf", "crlf"]) From 4b15bc93421866f0ca20290a4b951e9bcc7ec584 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:05:35 +0100 Subject: [PATCH 07/30] fix(split): a read-only root is refused like any failed write Every failed write ended in exit 1, 'Split not written', except the last one: a read-only root failed to open, and the undo then wrote the old text back to that same root, which raised the same PermissionError inside the handler: exit 2, 'Unexpected error', and the directories made stayed (finding SP-09). The undo now puts the root back only when it was opened for writing, so a root that could not be opened is left as it was, and every step of it runs whatever an earlier one did and none raises. A read-only root is now a clean exit 1 with the tree as it was; if a step of the undo does fail, the message names what is left instead of saying nothing was changed. --- fluid_build/cli/split.py | 41 +++++++++++++++++++++++++++-------- tests/fragments/test_split.py | 40 ++++++++++++++++++++++++++++------ 2 files changed, 65 insertions(+), 16 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 6038d8e3..1187e102 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -35,11 +35,12 @@ from __future__ import annotations import argparse +import functools import json import logging import sys from pathlib import Path -from typing import Any, Collection, Dict, List, Optional, Sequence, Tuple +from typing import Any, Callable, Collection, Dict, List, Optional, Sequence, Tuple COMMAND = "split" @@ -296,6 +297,7 @@ def _write_checked( original_root: Optional[bytes] = root_out.read_bytes() if root_out.exists() else None made: List[Path] = [] written: List[Path] = [] + root_opened = False try: for target, content in data.items(): _make_dirs(target.parent, made) @@ -305,22 +307,43 @@ def _write_checked( handle.write(content) _make_dirs(root_out.parent, made) with open(root_out, "wb") as handle: + root_opened = True handle.write(root_data) if compose_contract(root_out).document != before: raise ValueError("the split contract does not compose to the original one") except Exception as exc: # noqa: BLE001 - every failure is taken back - for path in written: - path.unlink(missing_ok=True) - if original_root is not None: - root_out.write_bytes(original_root) - elif root_out.exists(): - root_out.unlink() - for directory in reversed(made): - directory.rmdir() + left = _take_back(written, root_out if root_opened else None, original_root, made) + if left: + return f"{exc}; and split could not take back {'; '.join(left)}" return f"{exc}; nothing was changed" return None +def _take_back( + written: List[Path], root_out: Optional[Path], original_root: Optional[bytes], made: List[Path] +) -> List[str]: + """Undo a split that failed halfway: remove the files ``written``, put the + root back when it was opened for writing (``root_out``; a root that could + not be opened, a read-only one, is as it was), then remove the directories + ``made``, deepest first. Every step is tried, whatever an earlier one did, + and none raises: what could not be undone comes back, one line per path.""" + steps: List[Tuple[Path, Callable[[], Any]]] = [ + (path, functools.partial(path.unlink, missing_ok=True)) for path in written + ] + if root_out is not None and original_root is not None: + steps.append((root_out, functools.partial(root_out.write_bytes, original_root))) + elif root_out is not None: + steps.append((root_out, functools.partial(root_out.unlink, missing_ok=True))) + steps += [(directory, directory.rmdir) for directory in reversed(made)] + left: List[str] = [] + for path, step in steps: + try: + step() + except Exception as exc: # noqa: BLE001 - reported; the next step still runs + left.append(f"{path}: {getattr(exc, 'strerror', None) or exc}") + return left + + def _make_dirs(directory: Path, made: List[Path]) -> None: """``directory`` and each missing directory above it, every one it creates added to ``made`` (``mkdir(parents=True)`` says nothing of which it made).""" diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index d9bee458..a5bd0839 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -2658,23 +2658,49 @@ def test_i34_an_unwritable_output_directory_is_a_clean_refusal( @needs_posix_permissions -def test_i34_an_unwritable_root_file_is_an_unexpected_error_today( +def test_i34_an_unwritable_root_file_is_refused_like_any_failed_write( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: - """SG-32 characterisation (finding SP-09): every other failed write is exit 1, - 'Split not written'. A read-only root fails at the last write, and the undo - that tries to write the root back fails the same way inside the handler.""" + """Finding SP-09: a read-only root fails at the last write, which is reported + as every failed write is: exit 1, 'Split not written', the tree as it was. + The undo does not write back a root it could not open (that write raised the + same error inside the handler: exit 2, 'Unexpected error').""" pair = make_pair(tmp_path, SPLIT_ALL_FILES) pair.split_contract.chmod(0o444) + before = digest(pair.split) try: rc = split(pair.split_contract) finally: pair.split_contract.chmod(0o644) - assert rc == 2 + assert rc == 1 shown = capsys.readouterr() - assert "Unexpected error" in shown.out + shown.err - assert pair.split_contract.read_bytes() == SPLIT_ALL.encode("utf-8") + assert "Unexpected error" not in shown.out + shown.err + assert "Split not written" in shown.err and "Permission denied" in shown.err + assert shown.err.rstrip().endswith("nothing was changed") + assert digest(pair.split) == before + + +def test_i34_a_take_back_that_fails_is_reported_and_never_raises( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-09: every step of the undo runs whatever an earlier one did, none raises, + and the message names what is left instead of saying nothing was changed.""" + pair = solo(tmp_path, REF_INTO_A_MOVED_PART) + + def busy(self: Path) -> None: + raise OSError(16, "Device or resource busy", str(self)) + + monkeypatch.setattr(Path, "rmdir", busy) + + assert split(pair.split_contract) == 1 + + err = capsys.readouterr().err + assert "Split not written" in err and "nothing was changed" not in err + assert "split could not take back" in err + assert f"{pair.split / 'fragments' / 'builds'}: Device or resource busy" in err + # The other steps ran all the same: the root is put back, the files are gone. + assert pair.split_contract.read_text("utf-8") == REF_INTO_A_MOVED_PART assert not files_of(pair.split / "fragments") From 13487594222298619a15ec49c74f8499cf54291c Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:07:11 +0100 Subject: [PATCH 08/30] fix(split): a long id gets a short file name with a hash of the whole id The slug of an id was never shortened: an id of 300 characters named a 305-byte file, over the 255 bytes a file name may have. The dry run listed it and exited 0; the real run failed with ENAMETOOLONG (finding SP-07). A slug over 100 characters is now cut to 91 and ends in '-' and 8 hex digits of the SHA-256 of the whole id, so every name stays far under 255 bytes with a '-N' and the longest suffix, and two long ids that start alike still get a name each. Builds, exposes, their sql, schema and quality, and extension blocks are all named this way, and the dry run lists exactly the names the real run writes. The cap is pants' safe_filename default; the hash is kubespawner's strip_and_hash. --- fluid_build/cli/forge_contract_fragments.py | 19 +++++-- tests/fragments/test_split.py | 59 +++++++++++++++++---- 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/fluid_build/cli/forge_contract_fragments.py b/fluid_build/cli/forge_contract_fragments.py index 0e7fa4d2..c91031a2 100644 --- a/fluid_build/cli/forge_contract_fragments.py +++ b/fluid_build/cli/forge_contract_fragments.py @@ -35,6 +35,7 @@ from __future__ import annotations import copy +import hashlib import posixpath import re from typing import ( @@ -99,6 +100,15 @@ #: Description parts written as Markdown by the ``description`` part. _DESCRIPTION_PARTS = ("purpose", "usage", "limitations") +#: The longest slug a file is named by. A longer one is cut short and ends in +#: a hash of the whole id instead, so the name stays far under the 255 bytes a +#: file name may have, a ``-N`` and the longest suffix included, and two long +#: ids that start alike still get a name each. The cap is the default of +#: pants' ``safe_filename``; the hash that ends the cut slug, 8 hex digits of +#: SHA-256 over the whole id, is kubespawner's ``strip_and_hash``. +_MAX_SLUG = 100 +_SLUG_HASH = 8 + class FragmentSite(NamedTuple): """One part of a contract that goes to a file of its own.""" @@ -131,10 +141,13 @@ def parse_parts(spec: str) -> Tuple[str, ...]: def _slugify(value: str) -> str: - """Convert an identifier to a filesystem-safe slug.""" + """Convert an identifier to a filesystem-safe slug, at most :data:`_MAX_SLUG` long.""" slug = re.sub(r"[^a-zA-Z0-9_-]", "-", str(value).strip()) - slug = re.sub(r"-{2,}", "-", slug).strip("-") - return slug.lower() or "unnamed" + slug = re.sub(r"-{2,}", "-", slug).strip("-").lower() or "unnamed" + if len(slug) <= _MAX_SLUG: + return slug + whole = hashlib.sha256(str(value).encode("utf-8", "surrogatepass")).hexdigest() + return f"{slug[: _MAX_SLUG - _SLUG_HASH - 1].rstrip('-')}-{whole[:_SLUG_HASH]}" def _file_text(value: Any) -> bool: diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index a5bd0839..56a048a3 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -955,22 +955,59 @@ def test_i09_windows_device_names_are_not_avoided_today(tmp_path: Path) -> None: ] -def test_i09_a_300_character_id_cannot_be_written_but_the_dry_run_says_it_can( +def test_i09_a_300_character_id_gets_a_short_name_the_dry_run_lists( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: - """SG-32 characterisation (finding SP-07): the slug is not shortened, so the - file name is over 255 bytes; ``--dry-run`` lists it and exits 0, the real - run fails with ENAMETOOLONG.""" + """Finding SP-07: a slug over 100 characters is cut short and ends in a hash + of the whole id, so every name fits (255 bytes at most); two long ids that + start alike get a name each, and the dry run lists what the real run writes.""" big = "a" * 300 - pair = solo(tmp_path, f"id: x\nbuilds:\n - id: {big}\n engine: sql\n") - before = files_of(pair.split) + text = ( + f"id: x\nbuilds:\n - id: {big}\n engine: sql\n properties: {{sql: SELECT 1}}\n" + f" - id: {big}b\n engine: sql\nexposes:\n - exposeId: {big}\n kind: table\n" + f"extensions:\n {big}: {{a: 1}}\n" + ) + pair = solo(tmp_path, text) + capsys.readouterr() - assert split(pair.split_contract, "--dry-run") == 0 - assert big in capsys.readouterr().err - assert split(pair.split_contract) == 1 + assert split(pair.split_contract, "--dry-run", "--parts", ALL_PARTS) == 0 + listed = _listed(capsys.readouterr().err) + + after = split_and_check(pair, ALL_PARTS) - assert "File name too long" in capsys.readouterr().err - assert files_of(pair.split) == before + def short(name: str) -> str: + return f"{'a' * 91}-{hashlib.sha256(name.encode()).hexdigest()[:8]}" + + created = new_files(pair, after) + assert listed == created + assert created == sorted( + [ + f"fragments/builds/{short(big)}.yaml", + f"fragments/builds/{short(big + 'b')}.yaml", + f"fragments/exposes/{short(big)}.yaml", + f"fragments/extensions/{short(big)}.yaml", + f"sql/{short(big)}.sql", + ] + ) + assert all(len(rel.rsplit("/", 1)[1].encode("utf-8")) <= 255 for rel in created) + composed = compose_contract(pair.split_contract).document + assert [b["id"] for b in composed["builds"]] == [big, big + "b"] + assert text_of(after, f"fragments/builds/{short(big)}.yaml").startswith(f"# Build: {big}\n") + + +@pytest.mark.parametrize("length", [100, 101]) +def test_i09_a_slug_is_cut_only_past_100_characters(length: int) -> None: + from fluid_build.cli.forge_contract_fragments import _slugify + + name = ("Ab" * 51)[:length] + + slug = _slugify(name) + + assert len(slug) == 100 + if length == 100: + assert slug == name.lower() + else: + assert slug == name.lower()[:91] + "-" + hashlib.sha256(name.encode()).hexdigest()[:8] # ── I-10..I-15: the sql part ──────────────────────────────────────────── From e202c5c30ec68a57ca86e633157ac927c786df2f Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:09:02 +0100 Subject: [PATCH 09/30] fix(split): the dry run refuses a symlink on the way as the real run does Finding SP-08: with /fragments a symlink to a directory outside , split wrote the fragments through the link before a later check refused the split, and left the directories it made out there. The real run has refused such a link before writing anything since the fragments PR; the dry run never checked, so it listed fragments under the link and exited 0 for a split the real run refuses. The checks (a symlink at the root's name, a symlinked directory on the way to any fragment, however deep) now run before the dry run lists anything, with the same message and exit 1, and again before the first write. The test pins the whole tree, the outside directory included, after both runs. --- fluid_build/cli/split.py | 33 +++++++++++++++++++++++---------- tests/fragments/test_split.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 1187e102..b1092c34 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -40,7 +40,7 @@ import logging import sys from pathlib import Path -from typing import Any, Callable, Collection, Dict, List, Optional, Sequence, Tuple +from typing import Any, Callable, Collection, Dict, Iterable, List, Optional, Sequence, Tuple COMMAND = "split" @@ -154,6 +154,9 @@ def run(args: argparse.Namespace, logger: logging.Logger) -> int: return 0 before, problem = _composed(contract_path) + root_out = out_dir / contract_path.name + # What the real run would refuse before writing, the dry run refuses too. + problem = problem or _unsafe_target(out_dir, root_out, [site.target for site in sites]) if problem: sys.stderr.write(f"❌ Split not written: {problem}\n") return 1 @@ -163,7 +166,6 @@ def run(args: argparse.Namespace, logger: logging.Logger) -> int: return 0 root_text, fragments, lost_comments = _split(contract_path, contract, sites, parts, taken) - root_out = out_dir / contract_path.name problem = _write_checked(before, root_out, root_text, fragments, out_dir) if problem: sys.stderr.write(f"❌ Split not written: {problem}\n") @@ -206,6 +208,22 @@ def _print_dry_run(contract: Dict[str, Any], parts: Sequence[str], taken: Collec sys.stderr.write(f" {target}\n") +def _unsafe_target(out_dir: Path, root_out: Path, rel_paths: Iterable[str]) -> Optional[str]: + """Why the split cannot be written where it goes, if it cannot: a symlink at + the root's name, or a symlinked directory on the way to a fragment, where a + file written would land outside the output directory. Checked before the + dry run lists anything, and again before the first write.""" + if root_out.is_symlink(): + # open(..., "w") would write through it, a dangling one included, and the + # undo could only remove the link, leaving the written file where it points. + return f"{root_out} is a symlink; split writes no file through one" + for rel_path in rel_paths: + link = _symlink_on_the_way(out_dir, rel_path) + if link is not None: + return f"{link} is a symlink; split writes no file through one" + return None + + def _symlink_on_the_way(out_dir: Path, rel_path: str) -> Optional[Path]: """The first directory between ``out_dir`` and ``rel_path`` that is a symlink, if any: a fragment written through it would land outside the output directory.""" @@ -276,14 +294,9 @@ def _write_checked( """ from fluid_build.loader import compose_contract - if root_out.is_symlink(): - # open(..., "w") would write through it, a dangling one included, and the - # undo could only remove the link, leaving the written file where it points. - return f"{root_out} is a symlink; split writes no file through one" - for rel_path in fragments: - link = _symlink_on_the_way(out_dir, rel_path) - if link is not None: - return f"{link} is a symlink; split writes no file through one" + unsafe = _unsafe_target(out_dir, root_out, fragments) + if unsafe: + return unsafe # Every text is encoded before a file or a directory is made: one with no # UTF-8 form (a lone surrogate) is refused with nothing to take back. The # bytes are written as they are, so a \r in a text stays a \r. diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 56a048a3..33e36a88 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -2580,6 +2580,37 @@ def test_i33_a_symlinked_fragments_directory_never_leaves_a_fragment_outside_out assert entries_of(outside) == [] +@pytest.mark.skipif(sys.platform == "win32", reason="needs symlinks") +@pytest.mark.parametrize( + "link,parts", + [("fragments", None), ("fragments/exposes", None), ("sql", "sql")], + ids=["fragments", "a-directory-below-it", "sql"], +) +def test_i33_a_symlinked_directory_on_the_way_is_refused_by_the_dry_run_and_the_real_run( + tmp_path: Path, link: str, parts: Optional[str], capsys: pytest.CaptureFixture[str] +) -> None: + """SP-08: a directory on the way to a fragment that links outside ``--out`` is + refused before anything is written, there or through the link, however deep + it sits; and the dry run, which lists what the real run writes, refuses it + the same way instead of listing files that would never be written.""" + pair = make_pair(tmp_path, SPLIT_ALL_FILES) + outside = tmp_path / "outside" + outside.mkdir() + (pair.split / link).parent.mkdir(parents=True, exist_ok=True) + os.symlink(outside, pair.split / link) + args = ["--parts", parts] if parts else [] + before = digest(tmp_path) + capsys.readouterr() + + for dry_run in (["--dry-run"], []): + assert split(pair.split_contract, *dry_run, *args) == 1 + + err = capsys.readouterr().err + assert f"{pair.split / link} is a symlink; split writes no file through one" in err + assert "Would create" not in err + assert digest(tmp_path) == before # not a file or a directory, inside or out + + @pytest.mark.skipif(sys.platform == "win32", reason="needs symlinks") def test_i33_a_symlinked_sql_directory_is_refused_and_no_text_is_left_outside( tmp_path: Path, From 88ca931ffb85a91a014d093c5b9c45d13934163e Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:11:44 +0100 Subject: [PATCH 10/30] fix(split): the root in --out never writes over another file Fragments never overwrote a file, but the root did: 'fluid split c --out DIR' replaced a different DIR/contract.fluid.yaml without a word and exited 0, and the dry run did not mention it. A second split into the same --out met its own first-run files, wrote '-2' copies of every fragment and left the first run's fragments unreferenced (finding SP-11). The root may now be written only over the contract itself (an in-place split) or a byte-for-byte copy of it, where nothing is lost. Any other file there is refused before anything is written, the dry run included, with the file named and what to do: move it away or choose another --out. When the file there composes to the same document (the second-run case) the message says so and gives the command that splits that contract further. A split already there is not reused: the source may have changed since. A hard link to the contract in another directory is refused too, as writing it would change the contract in its own directory. --- fluid_build/cli/split.py | 48 ++++++++++++++++++- tests/fragments/test_split.py | 88 +++++++++++++++++++++++++---------- 2 files changed, 109 insertions(+), 27 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index b1092c34..1148c164 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -23,7 +23,8 @@ A YAML root keeps every comment and every other byte: only the moved parts are cut out of its text. The result is checked before the command ends: the split contract composes to the same document as the original, or nothing is -left changed. +left changed, no file and no directory. No file is written over but the +contract split in place: not by a fragment, and not by the root in ``--out``. Usage: fluid split contract.fluid.yaml @@ -156,7 +157,11 @@ def run(args: argparse.Namespace, logger: logging.Logger) -> int: before, problem = _composed(contract_path) root_out = out_dir / contract_path.name # What the real run would refuse before writing, the dry run refuses too. - problem = problem or _unsafe_target(out_dir, root_out, [site.target for site in sites]) + problem = ( + problem + or _unsafe_target(out_dir, root_out, [site.target for site in sites]) + or _root_in_the_way(contract_path, root_out, before) + ) if problem: sys.stderr.write(f"❌ Split not written: {problem}\n") return 1 @@ -224,6 +229,45 @@ def _unsafe_target(out_dir: Path, root_out: Path, rel_paths: Iterable[str]) -> O return None +def _root_in_the_way(contract_path: Path, root_out: Path, before: Dict[str, Any]) -> Optional[str]: + """Why the root cannot be written to ``root_out``, a file already there, if it + cannot. Split writes over no file but the contract it splits in place; a + byte-for-byte copy of it may be written over too, as nothing is lost. Any + other file is refused, a split of this contract written there before + included: a second split beside it would only add '-2' copies and leave + the first one's fragments unreferenced.""" + from fluid_build.loader import compose_contract + + if not root_out.exists(): + return None + try: + if root_out.samefile(contract_path): + # The contract itself, unless it is another link to it elsewhere. + if root_out.parent.samefile(contract_path.parent): + return None + elif ( + root_out.is_file() + and root_out.stat().st_size == contract_path.stat().st_size + and root_out.read_bytes() == contract_path.read_bytes() + ): + return None + except OSError: + pass # what cannot be compared is another file + try: + same = compose_contract(root_out).document == before + except Exception: # noqa: BLE001 - one that does not compose is another contract + same = False + if same: + return ( + f"{root_out} already holds this contract (it composes to the same document); " + f"split writes over no file. To split that one, run: fluid split {root_out}" + ) + return ( + f"{root_out} already exists; split writes over no file but the contract it " + f"splits in place. Move it away, or choose another --out" + ) + + def _symlink_on_the_way(out_dir: Path, rel_path: str) -> Optional[Path]: """The first directory between ``out_dir`` and ``rel_path`` that is a symlink, if any: a fragment written through it would land outside the output directory.""" diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 33e36a88..35eba0a5 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -2403,51 +2403,89 @@ def test_i29_the_same_directory_given_as_out_is_an_in_place_split(tmp_path: Path assert sorted(set(files_of(pair.split)) - set(files_of(pair.flat))) == DEFAULT_LAYOUT -def test_i29_an_existing_root_in_out_is_replaced_without_a_word_today( +def test_i29_an_existing_root_in_out_is_never_written_over( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: - """SG-32 characterisation (finding SP-11): fragments never overwrite a file, - but the root written to ``--out`` replaces a different file of that name.""" + """Finding SP-11: fragments never overwrote a file, but the root written to + ``--out`` replaced another file of its name without a word. Now the split + is refused, the dry run included, with the file named and what to do.""" pair = make_pair(tmp_path, SPLIT_ALL_FILES) out = tmp_path / "elsewhere" other = b"# somebody else's contract\nid: precious\n" write(out / NAME, other) + before = digest(tmp_path) capsys.readouterr() - assert split(pair.split_contract, "--dry-run", "--out", out) == 0 - assert NAME not in capsys.readouterr().err # the preview does not mention it either + for dry_run in (["--dry-run"], []): + assert split(pair.split_contract, *dry_run, "--out", out) == 1 + + err = capsys.readouterr().err + assert f"Split not written: {out / NAME} already exists" in err + assert "Move it away, or choose another --out" in err + assert "Would create" not in err + assert digest(tmp_path) == before + assert (out / NAME).read_bytes() == other + + +def test_i29_a_second_split_into_the_same_out_is_refused_and_leaves_the_first_alone( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-11: the source stays flat, so a second run into the same ``--out`` met + its own first-run files: it wrote ``-2`` copies and left the first run's + fragments unreferenced. It is refused now (a split there is not reused: + the source may have changed since), and the message says how to split the + contract already there; that works.""" + pair = make_pair(tmp_path, SPLIT_ALL_FILES) + out = tmp_path / "elsewhere" assert split(pair.split_contract, "--out", out) == 0 + first = digest(out) + capsys.readouterr() - assert (out / NAME).read_bytes() != other - assert b"precious" not in (out / NAME).read_bytes() + assert split(pair.split_contract, "--out", out, "--parts", "extensions") == 1 + + err = capsys.readouterr().err + assert f"{out / NAME} already holds this contract" in err + assert f"run: fluid split {out / NAME}" in err + assert digest(out) == first # no -2 copy, nothing rewritten + + assert split(out / NAME, "--parts", "extensions") == 0 # the way the message gives + assert compose_contract(out / NAME).document == compose_contract(pair.flat_contract).document + assert sorted(set(files_of(out)) - {NAME}) == sorted( + [*DEFAULT_LAYOUT, "fragments/extensions/acme.yaml"] + ) -def test_i29_splitting_twice_into_the_same_out_adds_numbered_copies(tmp_path: Path) -> None: - """SG-32 characterisation: the source stays flat, so a second run into the - same ``--out`` meets its own first-run files and writes ``-2`` copies; the - root now refers to those and the first run's fragments are left over.""" +def test_i29_a_copy_of_the_contract_in_out_is_split_there(tmp_path: Path) -> None: + """SP-11: a byte-for-byte copy of the contract is no other file; writing the + split root over it loses nothing.""" pair = make_pair(tmp_path, SPLIT_ALL_FILES) out = tmp_path / "elsewhere" + write(out / NAME, SPLIT_ALL) - assert split(pair.split_contract, "--out", out) == 0 - first = files_of(out) assert split(pair.split_contract, "--out", out) == 0 - second = files_of(out) - assert all(second[rel] == first[rel] for rel in first if rel != NAME) # nothing rewritten - assert sorted(set(second) - set(first)) == [ - "fragments/access-policy-2.yaml", - "fragments/builds/ingest-orders-2.yaml", - "fragments/builds/rollup-2.yaml", - "fragments/exposes/order_totals-2.yaml", - "fragments/exposes/orders-2.yaml", - "fragments/sovereignty-2.yaml", - ] - assert "ingest-orders-2.yaml" in text_of(second, NAME) - assert "ingest-orders.yaml" not in text_of(second, NAME) + assert sorted(files_of(out)) == sorted([NAME, *DEFAULT_LAYOUT]) assert compose_contract(out / NAME).document == compose_contract(pair.flat_contract).document +@pytest.mark.skipif(sys.platform == "win32", reason="needs hard links") +def test_i29_a_hard_link_to_the_contract_in_out_is_not_written_through( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-11: the same file under another directory is not an in-place split: + writing the root there would change the contract in its own directory too.""" + pair = make_pair(tmp_path, SPLIT_ALL_FILES) + out = tmp_path / "elsewhere" + out.mkdir() + os.link(pair.split_contract, out / NAME) + before = digest(tmp_path) + + assert split(pair.split_contract, "--out", out) == 1 + + assert "already holds this contract" in capsys.readouterr().err + assert digest(tmp_path) == before + + # ── I-30: an alias bomb ───────────────────────────────────────────────── From f56b20433c1ce2b8a7151eaadd7f6e03eecb9ce5 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:15:38 +0100 Subject: [PATCH 11/30] test(split): a rebased ref keeps its quoting, a block-scalar one goes to data The SP-02 rebase writes the new ref over the old one's scalar in the text splice, in the quoting it was written in; those branches (plain, single, double, tagged, a block mapping) and the fallback for a ref written as a block scalar, which the text splice refuses and the data split rebases, had no test of their own. Each one now checks the fragment's line, that every comment is kept, and the invariant. --- tests/fragments/test_split.py | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 35eba0a5..33eb2704 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1678,6 +1678,49 @@ def test_i21_the_dict_level_splitter_rebases_a_ref_once_and_never_its_own() -> N assert contract["builds"][0]["properties"]["parameters"] == {"$ref": "./p.yaml"} # unchanged +@pytest.mark.parametrize( + "written,rebased", + [ + ("{$ref: ./p.yaml} # plain", "{$ref: ../../p.yaml} # plain"), + ("{$ref: './p.yaml'} # single", "{$ref: '../../p.yaml'} # single"), + ('{"$ref": "./p.yaml"} # double', '{"$ref": "../../p.yaml"} # double'), + ("{$ref: !!str ./p.yaml} # tagged", "{$ref: ../../p.yaml} # tagged"), + ("\n $ref: ./p.yaml # block", "\n $ref: ../../p.yaml # block"), + ], + ids=["plain", "single-quoted", "double-quoted", "tagged", "block-mapping"], +) +def test_i21_a_rebased_ref_keeps_its_quoting_and_every_comment( + tmp_path: Path, written: str, rebased: str, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-02 in the text splice: only the ref's scalar is rewritten, in the + quoting it was written in (a tag on it goes: the string is a string).""" + text = f"# head\nid: x\nbuilds:\n - id: a\n properties:\n parameters: {written}\n" + pair = make_pair(tmp_path, {NAME: text, "p.yaml": "mode: full\n"}) + capsys.readouterr() + + after = split_and_check(pair) + + assert f"\nproperties:\n parameters: {rebased}\n" in text_of(after, "fragments/builds/a.yaml") + assert_comments_conserved(text, after) + assert "were not kept" not in capsys.readouterr().err + + +def test_i21_a_ref_written_as_a_block_scalar_is_rebased_by_the_data_split( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """A block scalar's span runs into the lines after it, so its text is not + rewritten: the data split rebases it, and says the comments were not kept.""" + text = "id: x\nbuilds:\n - id: a\n properties:\n parameters:\n $ref: |-\n ./p.yaml\n" + pair = make_pair(tmp_path, {NAME: text, "p.yaml": "mode: full\n"}) + capsys.readouterr() + + after = split_and_check(pair) + + build = yaml.safe_load(text_of(after, "fragments/builds/a.yaml")) + assert build["properties"]["parameters"] == {"$ref": "../../p.yaml"} + assert "a $ref value is written as a block scalar" in capsys.readouterr().err + + def test_i21_refs_that_stay_in_the_root_are_not_touched(tmp_path: Path) -> None: text = REFS_IN_PARTS.replace( "id: x\n", "id: x\nmetadata:\n owner: {$ref: ./team.yaml}\n" From ad00778f71edd2e00974516557a04c56ffa783de Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:30:28 +0100 Subject: [PATCH 12/30] docs(changelog): the fluid split fixes --- CHANGELOG.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14dcfdc4..3847bac0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`fluid split` rebases a relative `$ref` inside a part it moves.** A ref such as + `sql: {$ref: ./sql/transform.sql}` was copied unchanged into `fragments/builds/transform.yaml`, + where it named a file one or two directories off, so split refused the contract, and a split in + two passes (`--parts sql`, then `--parts builds`) could not work. The ref now names the same file + from the fragment (`../../sql/transform.sql`, a `#/pointer` kept); a URL, an absolute path and a + same-document pointer are left as written. +- **A refused `fluid split` leaves the tree as it was**, as its message says: it took back the + files it wrote but left the directories it made (an empty `fragments/` changed what other + commands did), and after a failed write the empty file. A read-only root ended in "Unexpected + error" (exit 2); it is now refused like any failed write (exit 1). The dry run refuses a + symlinked directory on the way, as the real run already did. +- **`fluid split --out DIR` never writes over another file.** An existing `DIR/contract.fluid.yaml` + that is not the contract being split was replaced without a word; a second split into the same + `--out` wrote `-2` copies and left the first run's fragments unreferenced. Both are refused now, + the dry run included, with the remedy. +- **A long id gets a file name a file system takes.** An id of 300 characters gave a 305-byte + name: the dry run listed it and the real run failed. A slug over 100 characters now ends in 8 hex + digits of a hash of the whole id, so two long ids that start alike keep names of their own. +- **`fluid split` keeps more of the root as written:** a comment on the last line of a moved block + keeps the line's CR in a CRLF file; a keep-chomped block scalar (`|+`, `>+`) keeps its trailing + blank lines; a tag or an unused anchor on a moved section's key line no longer makes the splice + write unparseable YAML; and a comment on a block scalar's header line stays beside its `$ref`. + ## [0.25.0] — 2026-10-28 ### Added From 029a6a271d6ef2195beb02af8c85220569431453 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:48:44 +0100 Subject: [PATCH 13/30] fix(split): a part in a container an alias shares moves from its own place The data split (a root with YAML aliases or merge keys, or a JSON root) replaced each part by its ref in the container that held it. An anchor or a merge key ('<<: *defaults') puts one container at several places, so the part was replaced at all of them, and the next part there moved that ref instead of itself (finding N1). With the sql of two builds merged from one defaults block, the second sql file got a dict: 'Unexpected error', exit 2 (main refused cleanly). With one aliased contract mapping at two exposes, the second schema fragment got the splitter's own ref, which the SP-02 rebase then sent out of the ref root: exit 1, where main split it. Each container on the way to a part is now replaced, in the one above it, by a shallow copy of itself, so the part is replaced where it moves from and nowhere else; what the part shares with the places that keep it stays as it is. Six contracts that share containers through anchors, aliases and merge keys pin it, both repros among them, each with the invariant. And a part that is not text when it is written, whatever put it there, is now a clean refusal with nothing made, never an unexpected error. --- fluid_build/cli/forge_contract_fragments.py | 27 ++++- fluid_build/cli/split.py | 2 + tests/fragments/test_split.py | 115 ++++++++++++++++++++ 3 files changed, 138 insertions(+), 6 deletions(-) diff --git a/fluid_build/cli/forge_contract_fragments.py b/fluid_build/cli/forge_contract_fragments.py index c91031a2..fe1c8e75 100644 --- a/fluid_build/cli/forge_contract_fragments.py +++ b/fluid_build/cli/forge_contract_fragments.py @@ -385,7 +385,9 @@ def split_contract_to_fragments( Only parts that actually exist, are non-empty, and are not already a ``$ref`` are split. A relative ``$ref`` inside a part that moves is rewritten to name the same file from the part's new one - (:func:`rebased_ref`). The input *contract* is not mutated. + (:func:`rebased_ref`). A part is replaced by its ref only where it moves + from, even when a YAML anchor or merge key put its container at other + places too (:func:`_own_parent`). The input *contract* is not mutated. """ if not isinstance(contract, dict): return contract, {} @@ -396,7 +398,7 @@ def split_contract_to_fragments( # Deepest first, so a part inside another (a build's SQL) is a ref in # its container's file by the time the container moves. for site in sorted(sites, key=lambda s: len(s.path), reverse=True): - parent = _walk(root, site.path[:-1]) + parent = _own_parent(root, site.path) value = parent[site.path[-1]] if site.text: fragments[site.target] = value @@ -423,10 +425,23 @@ def _exact_dumper() -> Any: return ExactDumper -def _walk(document: Any, path: Iterable[Union[str, int]]) -> Any: - for part in path: - document = document[part] - return document +def _own_parent(root: Dict[str, Any], path: Tuple[Union[str, int], ...]) -> Any: + """The container that holds the last step of ``path``, made this path's own on + the way down: each container below ``root`` is replaced, in the one above it, + by a shallow copy of itself. + + A YAML anchor or merge key (``<<: *defaults``) puts one object at several + places. Replaced by its ref in place, a part sharing a parent with another + one changed there too, and the other one then moved the ref instead of + itself: a dict as an sql file, or the splitter's own ref rebased as if it + had been written in the contract. + """ + parent: Any = root + for step in path[:-1]: + own = copy.copy(parent[step]) + parent[step] = own + parent = own + return parent def describe_fragment_layout( diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 1148c164..8cb52099 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -350,6 +350,8 @@ def _write_checked( data[out_dir / rel_path] = content.encode("utf-8") except UnicodeEncodeError as exc: return f"{rel_path} cannot be written as UTF-8: {exc}" + except Exception as exc: # noqa: BLE001 - a part that is no text: refused, nothing made + return f"{rel_path} cannot be written: {exc}" root_data = root_text.encode("utf-8") original_root: Optional[bytes] = root_out.read_bytes() if root_out.exists() else None made: List[Path] = [] diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 33eb2704..31b5c617 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1833,6 +1833,121 @@ def test_i22_the_notice_is_only_for_the_alias_fallback( assert "were not kept" not in capsys.readouterr().err +#: Defaults a merge key puts into two builds: one ``properties`` mapping, at +#: three places (the defaults and both builds). +MERGED_DEFAULTS = """\ +id: x +defaults: &d + pattern: embedded-logic + engine: sql + properties: + sql: SELECT 1 +builds: + - id: a + <<: *d + - id: b + <<: *d +""" + +#: One ``contract`` mapping, the schema in it, at two exposes. +ALIASED_CONTRACT = """\ +id: x +exposes: + - exposeId: a + contract: &c + schema: + - {name: c1, type: string} + - exposeId: b + contract: *c +""" + +#: Contracts where a YAML anchor, alias or merge key puts one container at two +#: places, each with the parts to split: (text, parts). +SHARED_CONTAINERS: Dict[str, Tuple[str, str]] = { + "merged-defaults-sql": (MERGED_DEFAULTS, "sql"), + "merged-defaults-builds-and-sql": (MERGED_DEFAULTS, "builds,sql"), + "aliased-contract": (ALIASED_CONTRACT, "exposes,schema"), + "aliased-contract-schema-only": (ALIASED_CONTRACT, "schema"), + "the-same-build-twice": ( + "id: x\nbuilds:\n - &b\n id: a\n engine: sql\n" + " properties: {sql: SELECT 1, parameters: {$ref: ./p.yaml}}\n - *b\n", + "builds,sql", + ), + "merged-properties-holding-a-ref": ( + "id: x\nprops: &p\n sql: SELECT 1\n parameters: {$ref: ./p.yaml}\nbuilds:\n" + " - id: a\n properties:\n <<: *p\n - id: b\n properties: *p\n", + ALL_PARTS, + ), +} + + +@pytest.mark.parametrize("case", sorted(SHARED_CONTAINERS)) +def test_i22_parts_in_a_container_an_alias_shares_split_as_data( + tmp_path: Path, case: str, capsys: pytest.CaptureFixture[str] +) -> None: + """Finding N1: the data split replaced a part by its ref in the container + itself, so at every place an anchor or a merge key had put that container. + The next part there then moved the ref instead of itself: a dict as an sql + file (an unexpected error), or the splitter's own ref rebased as if it had + been written in the contract (a refusal, where the split before SP-02 + worked). Each part is now replaced where it moves from, and nowhere else; + none of these splits worse than before, and none ends in an error.""" + text, parts = SHARED_CONTAINERS[case] + pair = make_pair(tmp_path, {NAME: text, "p.yaml": "mode: full\n"}) + capsys.readouterr() + + after = split_and_check(pair, parts) + + assert "the contract uses YAML aliases" in capsys.readouterr().err + for rel in new_files(pair, after): + if rel.startswith("sql/"): + assert after[rel] == b"SELECT 1" + elif rel.startswith("fragments/schemas/"): + assert yaml.safe_load(text_of(after, rel)) == [{"name": "c1", "type": "string"}] + if case.startswith("merged-defaults"): + defaults = yaml.safe_load(text_of(after, NAME))["defaults"] # stays as it was + assert defaults["properties"] == {"sql": "SELECT 1"} + assert {"sql/a.sql", "sql/b.sql"} <= set(after) + if case == "aliased-contract": + for expose in ("a", "b"): + moved = yaml.safe_load(text_of(after, f"fragments/exposes/{expose}.yaml")) + assert moved["contract"]["schema"] == {"$ref": f"../schemas/{expose}.yaml"} + + +def test_i22_the_dict_level_splitter_replaces_a_part_only_where_it_moves_from() -> None: + """N1 at the dict level: two builds hold one ``properties`` object.""" + from fluid_build.cli.forge_contract_fragments import split_contract_to_fragments + + shared = {"sql": "SELECT 1\n", "parameters": {"$ref": "./p.yaml"}} + contract = {"builds": [{"id": "a", "properties": shared}, {"id": "b", "properties": shared}]} + + root, files = split_contract_to_fragments(contract, ("builds", "sql")) + + assert files["sql/a.sql"] == files["sql/b.sql"] == "SELECT 1\n" + for build in ("a", "b"): + moved = yaml.safe_load(files[f"fragments/builds/{build}.yaml"]) + assert moved["properties"] == { + "sql": {"$ref": f"../../sql/{build}.sql"}, + "parameters": {"$ref": "../../p.yaml"}, + } + assert shared == {"sql": "SELECT 1\n", "parameters": {"$ref": "./p.yaml"}} # not mutated + + +def test_i22_a_part_that_is_no_text_is_refused_before_anything_is_written(tmp_path: Path) -> None: + """N1's crash: a dict where a text belongs ended in an unexpected error from + the writer. Whatever puts one there, the split is refused cleanly, nothing made.""" + from fluid_build.cli import split as split_command + + root = write(tmp_path / NAME, "id: x\n") + before = digest(tmp_path) + fragments: Dict[str, Any] = {"fragments/builds/a.yaml": "id: a\n", "sql/a.sql": {"$ref": "x"}} + + problem = split_command._write_checked({}, root, "id: y\n", fragments, tmp_path) + + assert problem is not None and problem.startswith("sql/a.sql cannot be written: ") + assert digest(tmp_path) == before + + # ── I-23: JSON roots ──────────────────────────────────────────────────── JSON_ROOT = json.dumps(yaml.safe_load(SPLIT_ALL), indent=2) + "\n" From e5d4d37e9d4db319a745b0df3ac7c71f627517c1 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:51:59 +0100 Subject: [PATCH 14/30] fix(split): a JSON root with no UTF-8 form is refused before anything is made JSON can escape a lone surrogate ('"\ud800"'), and a key with one that stays in a JSON root comes back in the root's text, which then has no UTF-8 form. The fragments were encoded before anything was made, but the root was encoded outside that guard: 'Unexpected error', exit 2 (finding N2). The root is now encoded in the same step, so it is refused as a part with no UTF-8 form is: 'Split not written: cannot be written as UTF-8', exit 1, with nothing made. --- fluid_build/cli/split.py | 10 +++++++--- tests/fragments/test_split.py | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 8cb52099..7bd50cd5 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -342,8 +342,9 @@ def _write_checked( if unsafe: return unsafe # Every text is encoded before a file or a directory is made: one with no - # UTF-8 form (a lone surrogate) is refused with nothing to take back. The - # bytes are written as they are, so a \r in a text stays a \r. + # UTF-8 form (a lone surrogate in a part, or in a key a JSON root keeps: + # JSON can escape one) is refused with nothing to take back. The bytes are + # written as they are, so a \r in a text stays a \r. data: Dict[Path, bytes] = {} for rel_path, content in fragments.items(): try: @@ -352,7 +353,10 @@ def _write_checked( return f"{rel_path} cannot be written as UTF-8: {exc}" except Exception as exc: # noqa: BLE001 - a part that is no text: refused, nothing made return f"{rel_path} cannot be written: {exc}" - root_data = root_text.encode("utf-8") + try: + root_data = root_text.encode("utf-8") + except UnicodeEncodeError as exc: + return f"{root_out} cannot be written as UTF-8: {exc}" original_root: Optional[bytes] = root_out.read_bytes() if root_out.exists() else None made: List[Path] = [] written: List[Path] = [] diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 31b5c617..a9901045 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1999,6 +1999,26 @@ def test_i23_a_json_root_holding_text_refs_is_valid_json_and_composes(tmp_path: assert root["builds"][0]["properties"]["sql"] == {"$ref": "./sql/ingest-orders.sql"} +def test_i23_a_json_root_with_no_utf_8_form_is_refused_and_changes_nothing( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Finding N2: JSON can escape a lone surrogate (``"\\ud800"``) in a key that + stays in the root, and the root written back then has no UTF-8 form. That was + an unexpected error, exit 2; it is refused now as a part with no UTF-8 form + is, before anything is made.""" + text = '{"id": "x", "\\ud800": 1, "builds": [{"id": "t", "engine": "sql"}]}\n' + pair = solo(tmp_path, text, name=JSON_NAME) + before = digest(tmp_path) + capsys.readouterr() + + assert split(pair.split_contract) == 1 + + shown = capsys.readouterr() + assert f"Split not written: {pair.split_contract} cannot be written as UTF-8" in shown.err + assert "Unexpected error" not in shown.out + shown.err + assert digest(tmp_path) == before + + # ── I-24: every YAML shape a section can be written in ────────────────── ZOO: Dict[str, Tuple[str, bool]] = { From 26b5755a179c39163175c2a690e530b621eacaaa Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:53:31 +0100 Subject: [PATCH 15/30] fix(split): a header line of blanks is searched for its comment in linear time The comment on a block scalar's header line was found with '[ \t]+#.*$', searched from every position: on a run of blanks with no '#' after it, each blank started a match that took the rest of the run and then gave it back one blank at a time. 80,000 blanks after 'sql: |' took 17 s, against 2.3 s for the whole split on main. A match now starts only where a run of blanks starts (a lookbehind), which finds the same comment, the blanks before it included, in time linear in the line. A test times the search over 100,000 blanks, with and without a comment after them. --- fluid_build/cli/_split_text.py | 6 ++++-- tests/fragments/test_split.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/fluid_build/cli/_split_text.py b/fluid_build/cli/_split_text.py index 15972add..6136a616 100644 --- a/fluid_build/cli/_split_text.py +++ b/fluid_build/cli/_split_text.py @@ -53,8 +53,10 @@ __all__ = ["SpliceRefused", "split_yaml_text"] #: The comment on a block scalar's header line, the blanks before it included: -#: a ``#`` that follows a blank, so a ``#`` inside a tag is not one. -_HEADER_COMMENT = re.compile(r"[ \t]+#.*$") +#: a ``#`` that follows a blank, so a ``#`` inside a tag is not one. A match +#: starts only where a run of blanks starts: tried from every blank of a long +#: run (a line of blanks with no ``#``), the search took time quadratic in it. +_HEADER_COMMENT = re.compile(r"(? None: + """A block scalar's header line of 100,000 blanks: looking for its comment + tried a match from every blank, time quadratic in the run (about 16 s here); + it starts only where the run does now.""" + from fluid_build.cli._split_text import _header_comment + + blanks = " " * 100_000 + text = f"sql: |{blanks}{comment}\n SELECT 1\n" + node = yaml.compose(text).value[0][1] + started = time.monotonic() + + found = _header_comment(text, node) + + assert time.monotonic() - started < 1 + assert found == (blanks + comment if comment else "") + + @pytest.mark.parametrize( "name,content,rc", [ From 8da79fc737bf6aa0cef954fbb64b1274f9c61e11 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:55:12 +0100 Subject: [PATCH 16/30] fix(split): the ./ steps a ref starts with are dropped in linear time rebased_ref stripped the './' steps a relative ref starts with one at a time, each step copying the rest of the ref: a ref of 200,000 './' steps took seconds to rebase. The steps ('./', './/', '././') are now matched once, by one anchored pattern with one way to match, and the rest is split from there. A dot that starts a name ('./.x/y.yaml') and a '../' after them stay as they were. A test rebases four 200,000-step refs under a second each. --- fluid_build/cli/forge_contract_fragments.py | 13 +++++++----- tests/fragments/test_split.py | 22 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/fluid_build/cli/forge_contract_fragments.py b/fluid_build/cli/forge_contract_fragments.py index fe1c8e75..d701c5e3 100644 --- a/fluid_build/cli/forge_contract_fragments.py +++ b/fluid_build/cli/forge_contract_fragments.py @@ -109,6 +109,11 @@ _MAX_SLUG = 100 _SLUG_HASH = 8 +#: The ``./`` steps a relative ref may start with (``./``, ``.//``, ``././``): +#: they name the directory it is written in, so say nothing once it is +#: rebased. Matched once, not stripped a step at a time (a copy per step). +_LEADING_DOT_STEPS = re.compile(r"(?:\./+)+") + class FragmentSite(NamedTuple): """One part of a contract that goes to a file of its own.""" @@ -307,7 +312,7 @@ def rebased_ref(ref: Any, container: Optional[str]) -> Any: a ``#/pointer`` after the file stays. The ref's own path is kept as it was written, so no ``..`` in it is folded away (that would be wrong past a symlinked directory); only the directories it shares with the container's - lead, and a leading ``./``, are dropped. A URL, an absolute path, a + lead, and the ``./`` steps it starts with, are dropped. A URL, an absolute path, a same-document ``#/...`` pointer and a value that is not a string are not relative to the file, and come back unchanged. """ @@ -316,10 +321,8 @@ def rebased_ref(ref: Any, container: Optional[str]) -> Any: file_part, mark, pointer = ref.partition("#") if not is_relative_file_ref(file_part): return ref - path = file_part - while path.startswith("./"): - path = path[2:].lstrip("/") - names = path.split("/") + lead = _LEADING_DOT_STEPS.match(file_part) + names = file_part[lead.end() if lead else 0 :].split("/") climb = [name for name in posixpath.dirname(container).split("/") if name] # A directory the container's path starts with is a real one (split writes # through no symlink), so going into it and back out is no step at all. diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index e79bbb18..be1c38a1 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1657,6 +1657,28 @@ def test_i21_a_ref_is_rebased_by_its_path_alone(ref: Any, container: Any, rebase assert rebased_ref(ref, container) == rebased +@pytest.mark.parametrize( + "lead,rest,rebased", + [ + ("./", "x.yaml#/a", "../../x.yaml#/a"), + (".//", "x.yaml", "../../x.yaml"), + ("./", ".x/y.yaml", "../../.x/y.yaml"), # a dot that starts a name stays + ("./", "../x.yaml", "../../../x.yaml"), + ], +) +def test_i21_the_dot_steps_a_ref_starts_with_are_dropped_in_linear_time( + lead: str, rest: str, rebased: str +) -> None: + """``./`` steps were stripped one at a time, a copy of the rest of the ref per + step: 200,000 of them took seconds. They are matched once now.""" + from fluid_build.cli.forge_contract_fragments import rebased_ref + + started = time.monotonic() + + assert rebased_ref(lead * 200_000 + rest, "fragments/builds/t.yaml") == rebased + assert time.monotonic() - started < 1 + + def test_i21_the_dict_level_splitter_rebases_a_ref_once_and_never_its_own() -> None: """The sql ref split writes into the build's file names it from there already; only the ref that was in the contract is rebased.""" From b02af18ba846c9150fe976070bd2bc2c1c380ca8 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:56:05 +0100 Subject: [PATCH 17/30] fix(split): a copy in --out that is a hard link to a third file is refused The SP-11 guard lets the root be written over a byte-for-byte copy of the contract in --out, as nothing is lost. But the root is written in place, and a copy that is also another name of a third file (a hard link) took that file with it: the third file was rewritten as the split root. The copy branch now also needs the file to have one name (st_nlink == 1); a hard link with the same bytes is refused like any other file there, before anything is written. A test hard-links a third file into --out and checks that file, and the whole tree, are as they were. --- fluid_build/cli/split.py | 24 ++++++++++++++---------- tests/fragments/test_split.py | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 7bd50cd5..d8e326f4 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -232,10 +232,10 @@ def _unsafe_target(out_dir: Path, root_out: Path, rel_paths: Iterable[str]) -> O def _root_in_the_way(contract_path: Path, root_out: Path, before: Dict[str, Any]) -> Optional[str]: """Why the root cannot be written to ``root_out``, a file already there, if it cannot. Split writes over no file but the contract it splits in place; a - byte-for-byte copy of it may be written over too, as nothing is lost. Any - other file is refused, a split of this contract written there before - included: a second split beside it would only add '-2' copies and leave - the first one's fragments unreferenced.""" + byte-for-byte copy of it that is the only name of its file may be written + over too, as nothing is lost. Any other file is refused, a split of this + contract written there before included: a second split beside it would + only add '-2' copies and leave the first one's fragments unreferenced.""" from fluid_build.loader import compose_contract if not root_out.exists(): @@ -245,12 +245,16 @@ def _root_in_the_way(contract_path: Path, root_out: Path, before: Dict[str, Any] # The contract itself, unless it is another link to it elsewhere. if root_out.parent.samefile(contract_path.parent): return None - elif ( - root_out.is_file() - and root_out.stat().st_size == contract_path.stat().st_size - and root_out.read_bytes() == contract_path.read_bytes() - ): - return None + elif root_out.is_file(): + info = root_out.stat() + # The root is written in place: a copy that is also another name + # of a third file (a hard link) would change that file too. + if ( + info.st_nlink == 1 + and info.st_size == contract_path.stat().st_size + and root_out.read_bytes() == contract_path.read_bytes() + ): + return None except OSError: pass # what cannot be compared is another file try: diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index be1c38a1..8e3e5311 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -2686,6 +2686,27 @@ def test_i29_a_hard_link_to_the_contract_in_out_is_not_written_through( assert digest(tmp_path) == before +@pytest.mark.skipif(sys.platform == "win32", reason="needs hard links") +def test_i29_a_copy_in_out_that_is_a_hard_link_to_a_third_file_is_not_written_through( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-11: a byte-for-byte copy may be written over, but not one that is also + another name of a third file: the root is written in place, so that file + would change too.""" + pair = make_pair(tmp_path, SPLIT_ALL_FILES) + victim = write(tmp_path / "victim" / NAME, SPLIT_ALL) + out = tmp_path / "elsewhere" + out.mkdir() + os.link(victim, out / NAME) + before = digest(tmp_path) + + assert split(pair.split_contract, "--out", out) == 1 + + assert f"Split not written: {out / NAME} already" in capsys.readouterr().err + assert digest(tmp_path) == before + assert victim.read_text("utf-8") == SPLIT_ALL + + # ── I-30: an alias bomb ───────────────────────────────────────────────── From 53f97eeefd38a95042cc6d97646410d99e3b52d7 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:57:05 +0100 Subject: [PATCH 18/30] fix(split): an interrupt halfway through the write is taken back too The take-back ran for an Exception only. A KeyboardInterrupt (Ctrl-C) or a SystemExit while the fragments and the root were written, or while the result was checked, went past it and left the split unchecked: fragments written, the root rewritten (C1). Any BaseException now takes the split back first; one that is not an Exception is then raised again, so the CLI reports the interrupt as it reports any ('Operation cancelled by user', exit 1), and what could not be taken back, if anything, is printed before it goes on. A test interrupts the check of a written split with each of the two and finds the tree as it was. --- fluid_build/cli/split.py | 11 +++++++++-- tests/fragments/test_split.py | 36 +++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index d8e326f4..42b14f6e 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -338,7 +338,8 @@ def _write_checked( to the same document (a part that could not be moved exactly), all of it is taken back: every file written, the root's old text, and every directory made for them, ``--out`` included, so the tree is as it was. - The reason is returned. + The reason is returned. An interrupt halfway (Ctrl-C, an exit) is taken + back too, and raised again. """ from fluid_build.loader import compose_contract @@ -378,8 +379,14 @@ def _write_checked( handle.write(root_data) if compose_contract(root_out).document != before: raise ValueError("the split contract does not compose to the original one") - except Exception as exc: # noqa: BLE001 - every failure is taken back + except BaseException as exc: # noqa: BLE001 - every failure is taken back left = _take_back(written, root_out if root_opened else None, original_root, made) + if not isinstance(exc, Exception): + # Ctrl-C or an exit, halfway: the unchecked split is taken back all the + # same, and the interrupt goes on. + if left: + sys.stderr.write(f"❌ Split interrupted; could not take back {'; '.join(left)}\n") + raise if left: return f"{exc}; and split could not take back {'; '.join(left)}" return f"{exc}; nothing was changed" diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 8e3e5311..43886912 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -3031,6 +3031,42 @@ def busy(self: Path) -> None: assert not files_of(pair.split / "fragments") +@pytest.mark.parametrize("interrupt", [KeyboardInterrupt, SystemExit]) +def test_i34_an_interrupt_halfway_takes_the_split_back_and_goes_on( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, interrupt: type +) -> None: + """C1: the take-back ran for an ``Exception`` only, so a Ctrl-C (or an exit) + while the split was written or checked left it unchecked: fragments written, + the root rewritten. It is taken back now, and the interrupt goes on; the CLI + then reports it as it reports any interrupt.""" + import argparse + import logging + + from fluid_build.cli import split as split_command + + pair = make_pair(tmp_path, SPLIT_ALL_FILES) + before = digest(tmp_path) + real = loader.compose_contract + + def interrupted(path: Any, *args: Any, **kwargs: Any) -> Any: + if (pair.split / "fragments").exists(): # the check of the split just written + raise interrupt() + return real(path, *args, **kwargs) + + monkeypatch.setattr(loader, "compose_contract", interrupted) + args = argparse.Namespace( + contract=str(pair.split_contract), out=None, parts="builds,exposes", dry_run=False + ) + + with pytest.raises(interrupt): + split_command.run(args, logging.getLogger("test")) + + assert digest(tmp_path) == before + if interrupt is KeyboardInterrupt: + assert split(pair.split_contract) == 1 # 'Operation cancelled by user' + assert digest(tmp_path) == before + + @pytest.mark.parametrize("failure", ["refs-it-cannot-rebase", "out-elsewhere", "lone-surrogate"]) def test_i34_a_refused_split_leaves_the_tree_exactly_as_it_was( tmp_path: Path, failure: str, capsys: pytest.CaptureFixture[str] From aea3db95ff687df131604537b6776234030eb449 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:57:51 +0100 Subject: [PATCH 19/30] fix(split): the bytes written get a name of their own _write_checked named the text of each fragment 'content' when it encoded it, then the same name for the bytes it wrote, so mypy took the second for a str: 'Incompatible types in assignment' and an 'incompatible type str' for the write (C2). The bytes are now 'encoded'. --- fluid_build/cli/split.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 42b14f6e..cd6381e6 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -367,12 +367,12 @@ def _write_checked( written: List[Path] = [] root_opened = False try: - for target, content in data.items(): + for target, encoded in data.items(): _make_dirs(target.parent, made) with open(target, "xb") as handle: # Taken back from here on, whatever happens to the write. written.append(target) - handle.write(content) + handle.write(encoded) _make_dirs(root_out.parent, made) with open(root_out, "wb") as handle: root_opened = True From 2923d178ecac78985c6f212d41bda30242d6a595 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:03:30 +0100 Subject: [PATCH 20/30] test(split): pin what mutation testing found unpinned Four properties had no test that fails when they break: - SP-05, the file being written: a fragment's file is made, then its write fails (a full disk, the second 'xb' open). It is taken back with the rest; recording it only after its write would leave it behind. - SP-02 in the data split: one ref object an alias puts in the root's metadata and in a build is rebased in the build's fragment only; a rebase in place would change the root's too. - SP-02 in the text splice: a ref in the columns, cut out of an expose that moves as well, is rewritten once, in the columns' own file; walking the columns again from the expose would write over the cut. - The generated roots now hold refs too (parameters with and without a sibling, a column, a whole schema), with their files beside the root, in the root's line ends. 300 seeds past the 24 in the suite pass. --- tests/fragments/test_split.py | 116 ++++++++++++++++++++++++++++++++-- 1 file changed, 111 insertions(+), 5 deletions(-) diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 43886912..d0dcc15a 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1743,6 +1743,49 @@ def test_i21_a_ref_written_as_a_block_scalar_is_rebased_by_the_data_split( assert "a $ref value is written as a block scalar" in capsys.readouterr().err +def test_i21_a_ref_an_alias_shares_with_a_part_that_stays_is_rebased_in_the_part_only( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-02 in the data split: an alias puts one ref object in the root's metadata + and in a build. The build's copy is rebased in its fragment; the one that + stays in the root is never changed with it.""" + text = ( + "id: x\nmetadata:\n other: &p {$ref: ./p.yaml}\nbuilds:\n - id: a\n engine: sql\n" + " properties:\n parameters: *p\n" + ) + pair = make_pair(tmp_path, {NAME: text, "p.yaml": "mode: full\n"}) + capsys.readouterr() + + after = split_and_check(pair) + + assert "the contract uses YAML aliases" in capsys.readouterr().err + assert yaml.safe_load(text_of(after, NAME))["metadata"]["other"] == {"$ref": "./p.yaml"} + build = yaml.safe_load(text_of(after, "fragments/builds/a.yaml")) + assert build["properties"]["parameters"] == {"$ref": "../../p.yaml"} + + +def test_i21_a_ref_in_a_part_cut_from_a_moved_part_is_rebased_to_its_own_file( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-02 in the text splice: a ref in the columns, cut out of the expose that + moves too, is rewritten once, in the columns' own file, and not again in the + expose's text the columns were cut from.""" + text = ( + "# head\nid: x\nexposes:\n - exposeId: o\n kind: table\n contract:\n" + " schema:\n - {$ref: ./cols.yaml#/a} # col\n" + ) + pair = make_pair(tmp_path, {NAME: text, "cols.yaml": "a: {name: z, type: string}\n"}) + capsys.readouterr() + + after = split_and_check(pair, "exposes,schema") + + assert '- {$ref: "../../cols.yaml#/a"} # col\n' in text_of(after, "fragments/schemas/o.yaml") + expose = yaml.safe_load(text_of(after, "fragments/exposes/o.yaml")) + assert expose["contract"]["schema"] == {"$ref": "../schemas/o.yaml"} + assert_comments_conserved(text, after) + assert "were not kept" not in capsys.readouterr().err # the splice, not the data split + + def test_i21_refs_that_stay_in_the_root_are_not_touched(tmp_path: Path) -> None: text = REFS_IN_PARTS.replace( "id: x\n", "id: x\nmetadata:\n owner: {$ref: ./team.yaml}\n" @@ -3067,6 +3110,52 @@ def interrupted(path: Any, *args: Any, **kwargs: Any) -> Any: assert digest(tmp_path) == before +def test_i34_a_write_that_fails_takes_back_the_file_it_was_writing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-05: the second fragment's file is made, then its write fails (a full disk). + That file is taken back with the first: it is counted as written the moment it + exists, not once its text is in.""" + import errno + + from fluid_build.cli import split as split_command + + class _FullDisk: + def __init__(self, handle: Any) -> None: + self.handle = handle + + def __enter__(self) -> "_FullDisk": + return self + + def __exit__(self, *exc: Any) -> None: + self.handle.close() + + def write(self, data: bytes) -> int: + raise OSError(errno.ENOSPC, "No space left on device") + + made: List[Any] = [] + + def full_on_the_second(file: Any, mode: str = "r", *args: Any, **kwargs: Any) -> Any: + handle = open(file, mode, *args, **kwargs) + if mode == "xb": + made.append(file) + if len(made) == 2: + return _FullDisk(handle) + return handle + + pair = make_pair(tmp_path, SPLIT_ALL_FILES) + before = digest(tmp_path) + monkeypatch.setattr(split_command, "open", full_on_the_second, raising=False) + capsys.readouterr() + + assert split(pair.split_contract) == 1 + + err = capsys.readouterr().err + assert "No space left on device" in err and err.rstrip().endswith("nothing was changed") + assert len(made) == 2 + assert digest(tmp_path) == before + + @pytest.mark.parametrize("failure", ["refs-it-cannot-rebase", "out-elsewhere", "lone-surrogate"]) def test_i34_a_refused_split_leaves_the_tree_exactly_as_it_was( tmp_path: Path, failure: str, capsys: pytest.CaptureFixture[str] @@ -3624,6 +3713,14 @@ def test_c24_a_pointer_to_a_part_that_became_a_file_ref_gets_the_file_content( "description", "extensions.acme", ] +#: The files the generated contracts' refs name, beside the root. +GENERATED_REF_FILES: Dict[str, str] = { + "params/full.yaml": "mode: full\nn: 1\ntags: [a, b]\n", + "cols.yaml": ( + "extra: {name: extra, type: string}\n" + "columns:\n - {name: id, type: string, required: true}\n - {name: v, type: number}\n" + ), +} _SQL_TEXTS = [ "SELECT 1", "SELECT 1\n", @@ -3637,14 +3734,16 @@ def test_c24_a_pointer_to_a_part_that_became_a_file_ref_gets_the_file_content( def _generated_contract(rng: random.Random) -> Dict[str, Any]: def build(i: int) -> Dict[str, Any]: + parameters: Dict[str, Any] = {"mode": "full", "n": rng.randint(1, 5), "tags": ["a", "b"]} + if rng.random() < 0.5: # a ref, rebased when the build moves; with a sibling or not + parameters = {"$ref": "./params/full.yaml"} + if rng.random() < 0.5: + parameters["n"] = rng.randint(1, 5) return { "id": rng.choice([f"b{i}", f"build.{i}", f"B-{i}"]), "pattern": "embedded-logic", "engine": "sql", - "properties": { - "sql": rng.choice(_SQL_TEXTS), - "parameters": {"mode": "full", "n": rng.randint(1, 5), "tags": ["a", "b"]}, - }, + "properties": {"sql": rng.choice(_SQL_TEXTS), "parameters": parameters}, } def expose(i: int) -> Dict[str, Any]: @@ -3658,6 +3757,11 @@ def expose(i: int) -> Dict[str, Any]: } if rng.random() < 0.3: del contract["quality"] + refs = rng.random() + if refs < 0.3: # a column from a file, rebased with the expose or the schema + contract["schema"].append({"$ref": "./cols.yaml#/extra"}) + elif refs < 0.5: # the whole schema from a file: it moves with the expose only + contract["schema"] = {"$ref": "./cols.yaml#/columns"} return { "exposeId": f"e{i}", "kind": "table", @@ -3788,7 +3892,9 @@ def generated_root(seed: int) -> Tuple[str, List[str], bool]: @pytest.mark.parametrize("seed", range(24)) def test_i02_generated_roots_keep_every_comment_and_the_bundle(tmp_path: Path, seed: int) -> None: text, parts, crlf = generated_root(seed) - pair = solo(tmp_path, text.encode("utf-8")) + line_end = "\r\n" if crlf else "\n" + files = {rel: body.replace("\n", line_end) for rel, body in GENERATED_REF_FILES.items()} + pair = make_pair(tmp_path, {NAME: text.encode("utf-8"), **files}) after = split_and_check(pair, ",".join(parts), cli_envs=()) From 0bbfcc6460934b2e0da60f826987d3466592bfd5 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:09:20 +0100 Subject: [PATCH 21/30] fix(split): the words say what split does, and a rebased ref keeps its letters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What the docstrings and the help said had drifted from what split does: - The module docstring said no file is written over but the contract split in place; a byte-for-byte copy of it in --out, the only name of its file, is written over too (SP-11). It says so now. - A rebased ref was said to keep 'its own quoting', and a test named so, but a plain './cols.yaml#/a' comes back double-quoted: its new path needs quotes. It keeps its quoting where the new path allows it, which is what the docstring and the test say now, with a case for each such path. - The double quotes were written without allow_unicode, so a ref with a letter that is not ASCII ('./sql/my file é.sql') came back escaped ('\xE9'). The letter is written as it is now; NEL, LS, PS and what is not printable are escaped as before, so the value reads back exactly. - The help did not say that with --out the files a relative $ref names must be under the output directory too (C5). One sentence says it. - The test module said findings are kept as strict xfails; none is left. --- fluid_build/cli/_split_text.py | 21 ++++++++++++++------- fluid_build/cli/split.py | 6 ++++-- tests/fragments/test_split.py | 29 ++++++++++++++++++++++------- 3 files changed, 40 insertions(+), 16 deletions(-) diff --git a/fluid_build/cli/_split_text.py b/fluid_build/cli/_split_text.py index 6136a616..adff89bf 100644 --- a/fluid_build/cli/_split_text.py +++ b/fluid_build/cli/_split_text.py @@ -20,10 +20,11 @@ it included, and a ``$ref`` takes its place. A part inside another (a build's SQL) is cut out of its container's text first, so the container's fragment holds the ref to it. A relative ``$ref`` already in a moved part is -rewritten in its text, in its own quoting, to name the same file from the -fragment. The comment on a block scalar's header line -(``sql: | # the query``) stays beside the ref that replaces the scalar: a -text file cannot hold it. +rewritten in its text to name the same file from the fragment, in the +quoting it was written in where the new path allows it: a plain one gets +double quotes when the path needs them (a ``#`` or a blank in it). The +comment on a block scalar's header line (``sql: | # the query``) stays +beside the ref that replaces the scalar: a text file cannot hold it. PyYAML's compose marks give each part's span; PyYAML is a base dependency, so no round-trip library is needed. A block node's end mark runs past its @@ -249,7 +250,8 @@ def _with_trailing_comment(text: str, end: int) -> int: def _rebased_refs(cut: _Cut) -> List[_Edit]: """An edit for each ``$ref`` in the part ``cut`` moves (not in a part cut out of it) that names a file relative to the root: it is rewritten to name that - file from the part's own file (:func:`rebased_ref`), in the quoting it had.""" + file from the part's own file (:func:`rebased_ref`), in the quoting it had + where the new path allows it (:func:`_ref_edit`).""" inner = {id(child.node) for child in cut.children if isinstance(child, _Cut)} edits: List[_Edit] = [] stack = [cut.node] @@ -304,8 +306,13 @@ def _yaml_string(value: str) -> str: def _double_quoted(value: str) -> str: - """``value`` as a double-quoted YAML scalar, on one line however long it is.""" - return yaml.safe_dump(value, default_style='"', width=_ONE_LINE).strip() + """``value`` as a double-quoted YAML scalar, on one line however long it is. + + A printable letter that is not ASCII (``é``) is written as it is, as the + contract had it; NEL, LS, PS and what is not printable are escaped all the + same, so the value reads back exactly. + """ + return yaml.safe_dump(value, default_style='"', width=_ONE_LINE, allow_unicode=True).strip() def _render(text: str, lo: int, hi: int, edits: Sequence[_Edit]) -> str: diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index cd6381e6..7b962197 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -24,7 +24,8 @@ are cut out of its text. The result is checked before the command ends: the split contract composes to the same document as the original, or nothing is left changed, no file and no directory. No file is written over but the -contract split in place: not by a fragment, and not by the root in ``--out``. +contract split in place, or a byte-for-byte copy of it in ``--out`` that is +the only name of its file: not by a fragment, and not by the root. Usage: fluid split contract.fluid.yaml @@ -72,7 +73,8 @@ def register(subparsers: argparse._SubParsersAction) -> None: "A part that is already a $ref is left as it is, so splitting twice\n" "changes nothing. A file that exists is never written over. A relative\n" "$ref in a part that moves is rewritten to name the same file from\n" - "the part's new one.\n\n" + "the part's new one. With --out, the files relative $refs name must be\n" + "under the output directory too, where they are beside the contract.\n\n" "Examples:\n" " fluid split contract.fluid.yaml # split in place\n" " fluid split contract.fluid.yaml --parts builds,sql # choose the parts\n" diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index d0dcc15a..ba62b1fc 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -20,9 +20,10 @@ builds the flat contract and its copy to split; ``split_and_check`` runs the split on the copy and asserts that invariant. -Findings are kept as ``xfail(strict=True)`` tests that assert the behaviour -the catalog asks for; characterisation tests pin today's behaviour where the -catalog has no oracle (SPEC GAP SG-32, SG-18). +Findings were kept as ``xfail(strict=True)`` tests that asserted the +behaviour the catalog asks for, until each was fixed: none is left, and each +test now passes and names its finding. Characterisation tests pin today's +behaviour where the catalog has no oracle (SPEC GAP SG-32, SG-18). """ from __future__ import annotations @@ -1708,16 +1709,30 @@ def test_i21_the_dict_level_splitter_rebases_a_ref_once_and_never_its_own() -> N ('{"$ref": "./p.yaml"} # double', '{"$ref": "../../p.yaml"} # double'), ("{$ref: !!str ./p.yaml} # tagged", "{$ref: ../../p.yaml} # tagged"), ("\n $ref: ./p.yaml # block", "\n $ref: ../../p.yaml # block"), + # A plain one whose new path needs quotes gets double ones: a "#" in it, + # or a blank, with a letter that is not ASCII written as it is. + ("{$ref: ./p.yaml#/mode} # hash", '{$ref: "../../p.yaml#/mode"} # hash'), + ("{$ref: ./my é.yaml} # blank", '{$ref: "../../my é.yaml"} # blank'), + ], + ids=[ + "plain", + "single-quoted", + "double-quoted", + "tagged", + "block-mapping", + "plain-needing-quotes", + "plain-with-a-blank-and-a-letter-not-ascii", ], - ids=["plain", "single-quoted", "double-quoted", "tagged", "block-mapping"], ) -def test_i21_a_rebased_ref_keeps_its_quoting_and_every_comment( +def test_i21_a_rebased_ref_keeps_its_quoting_where_it_can_and_every_comment( tmp_path: Path, written: str, rebased: str, capsys: pytest.CaptureFixture[str] ) -> None: """SP-02 in the text splice: only the ref's scalar is rewritten, in the - quoting it was written in (a tag on it goes: the string is a string).""" + quoting it was written in where the new path allows it (a tag on it goes: + the string is a string).""" text = f"# head\nid: x\nbuilds:\n - id: a\n properties:\n parameters: {written}\n" - pair = make_pair(tmp_path, {NAME: text, "p.yaml": "mode: full\n"}) + files = {NAME: text, "p.yaml": "mode: full\n", "my é.yaml": "mode: full\n"} + pair = make_pair(tmp_path, files) capsys.readouterr() after = split_and_check(pair) From a09186f14de5b9ccfc085067ae7060030659a067 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 13:15:24 +0100 Subject: [PATCH 22/30] docs(changelog): the fluid split fixes as they stand after review --- CHANGELOG.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3847bac0..16de5320 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,12 +18,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **A refused `fluid split` leaves the tree as it was**, as its message says: it took back the files it wrote but left the directories it made (an empty `fragments/` changed what other commands did), and after a failed write the empty file. A read-only root ended in "Unexpected - error" (exit 2); it is now refused like any failed write (exit 1). The dry run refuses a - symlinked directory on the way, as the real run already did. + error" (exit 2); it is now refused like any failed write (exit 1), as is a JSON root with a + character that has no UTF-8 form. An interrupt (Ctrl-C) halfway through the writes is taken + back too. The dry run refuses a symlinked directory on the way, as the real run already did. +- **A part in a container a YAML alias or `<<` merge shares is split from its own place.** Two + builds merging the same defaults (`<<: *defaults`) made `fluid split --parts sql` fail with + "write() argument must be str, not dict" and leave an empty `sql/b.sql`; each build's sql is now + a file of its own. - **`fluid split --out DIR` never writes over another file.** An existing `DIR/contract.fluid.yaml` - that is not the contract being split was replaced without a word; a second split into the same - `--out` wrote `-2` copies and left the first run's fragments unreferenced. Both are refused now, - the dry run included, with the remedy. + that is not the contract being split (or a byte-for-byte copy of it that is no hard link to + another file) was replaced without a word; a second split into the same `--out` wrote `-2` + copies and left the first run's fragments unreferenced. Both are refused now, the dry run + included, with the remedy. - **A long id gets a file name a file system takes.** An id of 300 characters gave a 305-byte name: the dry run listed it and the real run failed. A slug over 100 characters now ends in 8 hex digits of a hash of the whole id, so two long ids that start alike keep names of their own. From 014e11eecf2720e4775fe119005b61d0c3dd55b0 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:40:54 +0100 Subject: [PATCH 23/30] fix(split): the ./ steps of a ref are found in memory that does not grow The './' steps a relative ref starts with were matched with '(?:\./+)+', linear in time but holding state for every step it took: a million steps peaked at 124 MB, 2.5 million at 330 MB of RSS. Python 3.10 is supported, so a possessive quantifier cannot drop that state. The steps are now scanned: the run of '.' and '/' the path starts with, cut before its first '..' (a climb, kept) or before a last lone '.' (a name that starts with one). The scan keeps nothing per step. A test pins it against the pattern it replaced on every path of up to eight '.', '/' and 'x', and bounds the memory: the scan under 1 MB for a million steps, the whole rebase under 32 MB (the rest is reading the path for the ref root's own checks, a few bytes per character). --- fluid_build/cli/forge_contract_fragments.py | 28 ++++++++++---- tests/fragments/test_split.py | 43 ++++++++++++++++++++- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/fluid_build/cli/forge_contract_fragments.py b/fluid_build/cli/forge_contract_fragments.py index d701c5e3..a972c2ef 100644 --- a/fluid_build/cli/forge_contract_fragments.py +++ b/fluid_build/cli/forge_contract_fragments.py @@ -109,11 +109,6 @@ _MAX_SLUG = 100 _SLUG_HASH = 8 -#: The ``./`` steps a relative ref may start with (``./``, ``.//``, ``././``): -#: they name the directory it is written in, so say nothing once it is -#: rebased. Matched once, not stripped a step at a time (a copy per step). -_LEADING_DOT_STEPS = re.compile(r"(?:\./+)+") - class FragmentSite(NamedTuple): """One part of a contract that goes to a file of its own.""" @@ -321,8 +316,7 @@ def rebased_ref(ref: Any, container: Optional[str]) -> Any: file_part, mark, pointer = ref.partition("#") if not is_relative_file_ref(file_part): return ref - lead = _LEADING_DOT_STEPS.match(file_part) - names = file_part[lead.end() if lead else 0 :].split("/") + names = file_part[_dot_steps(file_part) :].split("/") climb = [name for name in posixpath.dirname(container).split("/") if name] # A directory the container's path starts with is a real one (split writes # through no symlink), so going into it and back out is no step at all. @@ -333,6 +327,26 @@ def rebased_ref(ref: Any, container: Optional[str]) -> Any: return (rel if rel.startswith("../") else f"./{rel}") + mark + pointer +def _dot_steps(path: str) -> int: + """How long the ``./`` steps are that ``path`` starts with (``./``, ``.//``, + ``././``): they name the directory the ref is written in, so they say + nothing once it is rebased. + + Scanned, not matched: the pattern ``(?:\\./+)+`` is linear in time but + keeps state for every step it takes (2.5 million steps held 330 MB), and + Python 3.10 has no possessive quantifier to drop it. The run of ``.`` and + ``/`` the path starts with ends, as the steps do, before a ``..`` (a climb, + kept) or before a last ``.`` (a name that starts with one, or ``.``). + """ + if not path.startswith("./"): + return 0 + end = len(path) - len(path.lstrip("./")) + climb = path.find("..", 0, end) + if climb >= 0: + return climb + return end - 1 if path[end - 1] == "." else end + + def _rebased_refs(value: Any, at: Tuple[Any, ...], container: str, inner: Collection[Any]) -> Any: """``value``, the part at ``at`` that goes to the file ``container``, with every ``$ref`` written in the root rebased to that file (:func:`rebased_ref`). diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index ba62b1fc..5fbf8b85 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1671,7 +1671,7 @@ def test_i21_the_dot_steps_a_ref_starts_with_are_dropped_in_linear_time( lead: str, rest: str, rebased: str ) -> None: """``./`` steps were stripped one at a time, a copy of the rest of the ref per - step: 200,000 of them took seconds. They are matched once now.""" + step: 200,000 of them took seconds. They are found in one pass now.""" from fluid_build.cli.forge_contract_fragments import rebased_ref started = time.monotonic() @@ -1680,6 +1680,47 @@ def test_i21_the_dot_steps_a_ref_starts_with_are_dropped_in_linear_time( assert time.monotonic() - started < 1 +def test_i21_the_dot_steps_are_found_in_memory_that_does_not_grow_with_them() -> None: + """The steps were then matched with ``(?:\\./+)+``: linear in time, but the + match kept state for every step, a peak of 124 MB for the million here (330 + MB of RSS for 2.5 million). The scan that finds them keeps nothing per step; + a rebase as a whole holds what reading the path for the ref root's own + checks takes, a few bytes per character.""" + import tracemalloc + + from fluid_build.cli.forge_contract_fragments import _dot_steps, rebased_ref + + path = "./" * 1_000_000 + "x.yaml" + ref = path + "#/a" + tracemalloc.start() + try: + assert _dot_steps(path) == 2_000_000 + scan_peak = tracemalloc.get_traced_memory()[1] + tracemalloc.reset_peak() + assert rebased_ref(ref, "fragments/builds/t.yaml") == "../../x.yaml#/a" + rebase_peak = tracemalloc.get_traced_memory()[1] + finally: + tracemalloc.stop() + + assert scan_peak < 1_000_000 + assert rebase_peak < 32_000_000 + + +def test_i21_the_dot_step_scan_ends_where_the_pattern_it_replaced_did() -> None: + """Every path of up to eight ``.``, ``/`` and ``x``: the scan finds the steps + the pattern ``(?:\\./+)+`` matched, no more and no fewer.""" + import itertools + + from fluid_build.cli.forge_contract_fragments import _dot_steps + + pattern = re.compile(r"(?:\./+)+") + for length in range(9): + for chars in itertools.product("./x", repeat=length): + path = "".join(chars) + matched = pattern.match(path) + assert _dot_steps(path) == (matched.end() if matched else 0), path + + def test_i21_the_dict_level_splitter_rebases_a_ref_once_and_never_its_own() -> None: """The sql ref split writes into the build's file names it from there already; only the ref that was in the contract is rebased.""" From 8bb9a22e377209d5bc4b414647fcb524a8c6792c Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:45:14 +0100 Subject: [PATCH 24/30] fix(split): the take-back puts the root back first, through any interrupt Two gaps were left in taking back an interrupted split. The flag that sent the take-back to the root was set after open(root, 'wb') returned, but open() empties the root before it returns: an interrupt just then left the contract empty, and the take-back passed it by. The flag is now set before the open, and the root is written back whenever its bytes differ from what they were, so a read-only root that could not be opened is still left alone. The take-back removed the fragments before it put the root back, and caught only an Exception, so a second Ctrl-C during it stopped it there: a split root over fragments it had already removed. The root now goes back first, every step runs whatever an earlier one raised, an interrupt included, and the first interrupt is raised again at the end (also when the split was being taken back after a plain refusal), with what could not be taken back printed before it. Tests interrupt the root's open just after it empties the file, and the take-back's first fragment removal after an interrupt or a refusal at the check; they find the root put back before any fragment was touched. --- fluid_build/cli/split.py | 69 ++++++++++++++++++--------- tests/fragments/test_split.py | 87 +++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 23 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 7b962197..45020aca 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -367,7 +367,7 @@ def _write_checked( original_root: Optional[bytes] = root_out.read_bytes() if root_out.exists() else None made: List[Path] = [] written: List[Path] = [] - root_opened = False + root_reached = False try: for target, encoded in data.items(): _make_dirs(target.parent, made) @@ -376,19 +376,24 @@ def _write_checked( written.append(target) handle.write(encoded) _make_dirs(root_out.parent, made) + # Before the open, not after it: open() truncates the root before it + # returns, so an interrupt that comes just then finds it empty. + root_reached = True with open(root_out, "wb") as handle: - root_opened = True handle.write(root_data) if compose_contract(root_out).document != before: raise ValueError("the split contract does not compose to the original one") except BaseException as exc: # noqa: BLE001 - every failure is taken back - left = _take_back(written, root_out if root_opened else None, original_root, made) - if not isinstance(exc, Exception): - # Ctrl-C or an exit, halfway: the unchecked split is taken back all the - # same, and the interrupt goes on. + left, interrupted = _take_back( + written, root_out if root_reached else None, original_root, made + ) + # Ctrl-C or an exit, halfway or during the take-back: the unchecked split + # is taken back all the same, and the first interrupt goes on. + interrupt = interrupted if isinstance(exc, Exception) else exc + if interrupt is not None: if left: sys.stderr.write(f"❌ Split interrupted; could not take back {'; '.join(left)}\n") - raise + raise interrupt if left: return f"{exc}; and split could not take back {'; '.join(left)}" return f"{exc}; nothing was changed" @@ -397,27 +402,45 @@ def _write_checked( def _take_back( written: List[Path], root_out: Optional[Path], original_root: Optional[bytes], made: List[Path] -) -> List[str]: - """Undo a split that failed halfway: remove the files ``written``, put the - root back when it was opened for writing (``root_out``; a root that could - not be opened, a read-only one, is as it was), then remove the directories - ``made``, deepest first. Every step is tried, whatever an earlier one did, - and none raises: what could not be undone comes back, one line per path.""" - steps: List[Tuple[Path, Callable[[], Any]]] = [ - (path, functools.partial(path.unlink, missing_ok=True)) for path in written - ] - if root_out is not None and original_root is not None: - steps.append((root_out, functools.partial(root_out.write_bytes, original_root))) - elif root_out is not None: - steps.append((root_out, functools.partial(root_out.unlink, missing_ok=True))) +) -> Tuple[List[str], Optional[BaseException]]: + """Undo a split that failed halfway. + + The root first (``root_out``, ``None`` when the write never reached it): + written back when its bytes differ from ``original_root``, so a read-only + one that could not be opened is left alone; removed when it was not there + before. Then the files ``written``, then the directories ``made``, deepest + first. The root goes first, so an undo cut short leaves the contract as it + was, not a split root whose fragments are gone. + + Every step is tried whatever an earlier one did, an interrupt (a second + Ctrl-C) included, and none raises. Returns what could not be undone, one + entry per path, and the first interrupt, for the caller to raise again. + """ + steps: List[Tuple[Path, Callable[[], Any]]] = [] + if root_out is not None: + steps.append((root_out, functools.partial(_put_back, root_out, original_root))) + steps += [(path, functools.partial(path.unlink, missing_ok=True)) for path in written] steps += [(directory, directory.rmdir) for directory in reversed(made)] left: List[str] = [] + interrupt: Optional[BaseException] = None for path, step in steps: try: step() - except Exception as exc: # noqa: BLE001 - reported; the next step still runs - left.append(f"{path}: {getattr(exc, 'strerror', None) or exc}") - return left + except BaseException as exc: # noqa: BLE001 - reported; the next step still runs + why = getattr(exc, "strerror", None) or str(exc) or type(exc).__name__ + left.append(f"{path}: {why}") + if interrupt is None and not isinstance(exc, Exception): + interrupt = exc + return left, interrupt + + +def _put_back(root: Path, original: Optional[bytes]) -> None: + """``root`` as it was before the split: ``original`` written back when its + bytes differ from it, or removed when there was none.""" + if original is None: + root.unlink(missing_ok=True) + elif not root.is_file() or root.read_bytes() != original: + root.write_bytes(original) def _make_dirs(directory: Path, made: List[Path]) -> None: diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 5fbf8b85..17452fb8 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -3166,6 +3166,93 @@ def interrupted(path: Any, *args: Any, **kwargs: Any) -> Any: assert digest(tmp_path) == before +def _run_split(contract: Path, parts: str) -> None: + """``fluid split --parts `` through the command's ``run``, + so an interrupt it lets go on reaches the caller (the CLI would report it).""" + import argparse + import logging + + from fluid_build.cli import split as split_command + + args = argparse.Namespace(contract=str(contract), out=None, parts=parts, dry_run=False) + split_command.run(args, logging.getLogger("test")) + + +def test_i34_an_interrupt_just_as_the_root_is_opened_puts_the_root_back( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """C1, the open: ``open(root, "wb")`` empties the root before it returns, and + the flag that sent the take-back to it was set after that. An interrupt just + then left the contract empty. The root is now put back whenever its bytes + differ, from the moment the write reaches it.""" + import builtins + + from fluid_build.cli import split as split_command + + pair = make_pair(tmp_path, SPLIT_ALL_FILES) + before = digest(tmp_path) + + def emptied_then_interrupted(file: Any, mode: str = "r", *args: Any, **kwargs: Any) -> Any: + handle = builtins.open(file, mode, *args, **kwargs) + if mode == "wb" and Path(file) == pair.split_contract: + handle.close() + raise KeyboardInterrupt + return handle + + monkeypatch.setattr(split_command, "open", emptied_then_interrupted, raising=False) + + with pytest.raises(KeyboardInterrupt): + _run_split(pair.split_contract, "builds,exposes") + + assert pair.split_contract.read_bytes() == SPLIT_ALL.encode("utf-8") + assert digest(tmp_path) == before + + +@pytest.mark.parametrize("first", ["an-interrupt-at-the-check", "a-refusal-at-the-check"]) +def test_i34_an_interrupt_during_the_take_back_puts_the_root_back_first_and_goes_on( + tmp_path: Path, first: str +) -> None: + """C1, the take-back: it removed the fragments before it put the root back, + and an interrupt in it (a second Ctrl-C) stopped it there: a split root over + fragments it had removed. The root goes back first now, every step runs all + the same, and the first interrupt goes on at the end, also when what was + being taken back was a plain refusal.""" + text = SPLIT_ALL if first == "an-interrupt-at-the-check" else REF_INTO_A_MOVED_PART + pair = make_pair(tmp_path, {NAME: text}) + real_compose = loader.compose_contract + real_unlink = Path.unlink + unlinked: List[Path] = [] + root_when_cut: List[bytes] = [] + + def check(path: Any, *args: Any, **kwargs: Any) -> Any: + if first == "an-interrupt-at-the-check" and (pair.split / "fragments").exists(): + raise KeyboardInterrupt + return real_compose(path, *args, **kwargs) + + def unlink(self: Path, *args: Any, **kwargs: Any) -> None: + unlinked.append(self) + if len(unlinked) == 1: + # Were the take-back cut short here (a kill, not an exception), the + # contract would be what it is now. + root_when_cut.append(pair.split_contract.read_bytes()) + raise KeyboardInterrupt + real_unlink(self, *args, **kwargs) + + with pytest.MonkeyPatch.context() as patch: + patch.setattr(loader, "compose_contract", check) + patch.setattr(Path, "unlink", unlink) + with pytest.raises(KeyboardInterrupt): + _run_split(pair.split_contract, "builds,exposes") + + assert root_when_cut == [text.encode("utf-8")] # the root went back before any fragment + assert pair.split_contract.read_text("utf-8") == text + left = sorted(set(files_of(pair.split)) - set(files_of(pair.flat))) + assert left == [unlinked[0].relative_to(pair.split).as_posix()] # the others went + assert compose_contract(pair.split_contract).document == ( + compose_contract(pair.flat_contract).document + ) + + def test_i34_a_write_that_fails_takes_back_the_file_it_was_writing( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: From 1157d8a7d5e3daa3f1084b7d3c671efd7aa6d9d8 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:48:23 +0100 Subject: [PATCH 25/30] fix(split): a rebased ref that holds a character that hides is written escaped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The double quotes of a rebased ref were written with allow_unicode, which PyYAML takes to mean writing a bidi control (U+202E RLO, the isolates, LRM, ALM), a zero-width space or joiner, a soft hyphen, a private-use or an unassigned code point as it is: a fragment line that shows other text than it holds, the Trojan Source pattern. A value is now written with its letters that are not ASCII ('é') only when it is all printable, the rule the single-quoted branch already had; one that holds anything else is escaped whole. NEL, LS and PS are escaped either way. Tests check each such character comes out escaped and reads back exactly, and that a ref the contract escapes ('\u202E') stays escaped in the fragment, no file the split writes holding the character itself. The test source spells every such character as an escape. --- fluid_build/cli/_split_text.py | 14 +++++++++---- tests/fragments/test_split.py | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/fluid_build/cli/_split_text.py b/fluid_build/cli/_split_text.py index adff89bf..4a9af178 100644 --- a/fluid_build/cli/_split_text.py +++ b/fluid_build/cli/_split_text.py @@ -308,11 +308,17 @@ def _yaml_string(value: str) -> str: def _double_quoted(value: str) -> str: """``value`` as a double-quoted YAML scalar, on one line however long it is. - A printable letter that is not ASCII (``é``) is written as it is, as the - contract had it; NEL, LS, PS and what is not printable are escaped all the - same, so the value reads back exactly. + A value that is all printable keeps its letters that are not ASCII + (``é``), as the contract had them. One that holds anything else is escaped + whole, as the single-quoted rule has it: PyYAML would write a bidi control + (U+202E), a zero-width or soft hyphen, a private-use or unassigned code + point as it is, and the line would show other text than it holds (Trojan + Source). NEL, LS and PS are escaped either way, so the value reads back + exactly. """ - return yaml.safe_dump(value, default_style='"', width=_ONE_LINE, allow_unicode=True).strip() + return yaml.safe_dump( + value, default_style='"', width=_ONE_LINE, allow_unicode=value.isprintable() + ).strip() def _render(text: str, lo: int, hi: int, edits: Sequence[_Edit]) -> str: diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 17452fb8..b18eb77f 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -1783,6 +1783,44 @@ def test_i21_a_rebased_ref_keeps_its_quoting_where_it_can_and_every_comment( assert "were not kept" not in capsys.readouterr().err +#: Characters a line can hold without showing them, or showing other text than +#: it holds: bidi controls (RLO, LRI and PDI, LRM, ALM), zero-width ones (ZWSP, +#: ZWJ), a soft hyphen, a private-use and an unassigned code point. +UNSEEN = "\u202e\u2066\u2069\u200e\u061c\u200b\u200d\xad\ue000\u0378" + + +@pytest.mark.parametrize("char", list(UNSEEN), ids=lambda c: f"U+{ord(c):04X}") +def test_i21_a_rebased_ref_holding_a_character_that_hides_is_written_escaped(char: str) -> None: + """Round-2 review: the rebased ref's double quotes were written with + ``allow_unicode``, which PyYAML takes to mean every one of these as it is: a + Trojan-Source-style line in the fragment. A value that is not all printable + is escaped whole now; one that is keeps its letters (``é``) as they were.""" + from fluid_build.cli._split_text import _double_quoted + + hidden = _double_quoted(f"../../a{char}é.yaml") + shown = _double_quoted("../../aé.yaml") + + assert hidden.isascii() # every character escaped (\u202E, \xAD, ...) + assert yaml.safe_load(hidden) == f"../../a{char}é.yaml" + assert shown == '"../../aé.yaml"' + + +def test_i21_a_moved_ref_whose_file_name_holds_an_rlo_is_never_written_raw( + tmp_path: Path, +) -> None: + """The same, end to end: a ref the contract escapes (``\\u202E``) stays escaped + in the fragment it moves to; no file the split writes holds the RLO itself.""" + text = ( + 'id: x\nbuilds:\n - id: a\n properties:\n parameters: {$ref: "./p\\u202Eq.yaml"}\n' + ) + pair = make_pair(tmp_path, {NAME: text, "p\u202eq.yaml": "mode: full\n"}) + + after = split_and_check(pair) + + assert 'parameters: {$ref: "../../p\\u202Eq.yaml"}' in text_of(after, "fragments/builds/a.yaml") + assert not [rel for rel, content in after.items() if "\u202e".encode("utf-8") in content] + + def test_i21_a_ref_written_as_a_block_scalar_is_rebased_by_the_data_split( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: From 8cf4f20ebfe3c6ecf7e73ed44922b83972acabbf Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:50:22 +0100 Subject: [PATCH 26/30] fix(split): a data split writes every object in full, with no alias The copies a data split makes on each part's way (N1) share what else they hold with the original, so one object a YAML alias put at two places can end up at many. The dumper named shared objects, and wrote an alias for each: a builds list aliased in the metadata, 60 builds with a 'tags' list each and '--parts sql', gave 60 aliases, past the 50 the loader reads when the split is checked. The split was refused ('more than 50 alias references'), where the split before N1 split it. The data split's root and fragments are now dumped with aliases off, every object in full wherever it is. Nothing grows past what the contract's own alias cap let in, and a document that contains itself is refused before (it does not compose). A test splits that contract and finds no anchor or alias in any file it writes. --- fluid_build/cli/forge_contract_fragments.py | 22 ++++++++++++++++++--- tests/fragments/test_split.py | 22 +++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/fluid_build/cli/forge_contract_fragments.py b/fluid_build/cli/forge_contract_fragments.py index a972c2ef..26f2e248 100644 --- a/fluid_build/cli/forge_contract_fragments.py +++ b/fluid_build/cli/forge_contract_fragments.py @@ -35,6 +35,7 @@ from __future__ import annotations import copy +import functools import hashlib import posixpath import re @@ -434,12 +435,27 @@ def split_contract_to_fragments( return root, fragments +@functools.lru_cache(maxsize=1) def _exact_dumper() -> Any: - """The YAML dumper ``fluid bundle`` writes with: text holding NEL, LS or PS is - written escaped, so it reads back exactly (a bare one folds into a space).""" + """The YAML dumper ``fluid bundle`` writes with (text holding NEL, LS or PS is + written escaped, so it reads back exactly: a bare one folds into a space), + writing every object in full wherever it is, with no anchor and no alias. + + A data split copies the containers on each part's way (:func:`_own_parent`), + and a copy shares what else it holds with the original: an object a YAML + alias put at two places can end up at many, and a dumper that names shared + objects wrote an alias for each, enough to pass the 50 aliases the loader + reads when the split is checked. Written in full, nothing grows past what + the contract's own alias cap let in; a document that contains itself is + refused before (it does not compose). + """ from fluid_build.forge.core.bundle import ExactDumper - return ExactDumper + class _InFull(ExactDumper): + def ignore_aliases(self, data: Any) -> bool: + return True + + return _InFull def _own_parent(root: Dict[str, Any], path: Tuple[Union[str, int], ...]) -> Any: diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index b18eb77f..5aee6313 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -2107,6 +2107,28 @@ def test_i22_a_part_that_is_no_text_is_refused_before_anything_is_written(tmp_pa assert digest(tmp_path) == before +def test_i22_a_data_split_writes_every_object_in_full_and_passes_no_alias_cap( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Round-2 review: the copies the data split makes on a part's way share what + else they hold with the original, so one object an alias put at two places + ended up at many. Dumped with anchors, the builds list aliased in the + metadata gave its 60 ``tags`` lists an alias each, past the 50 the loader + reads when the split is checked: refused, where the split before split it. + Every object is written in full now, in the root and in the fragments.""" + lines = ["id: x", "builds: &bl"] + for i in range(60): + lines += [f" - id: b{i}", " tags: [t]", " properties:", f" sql: SELECT {i}"] + pair = solo(tmp_path, "\n".join([*lines, "metadata:", " copy: *bl"]) + "\n") + capsys.readouterr() + + after = split_and_check(pair, "sql") + + assert "the contract uses YAML aliases" in capsys.readouterr().err # the data split + assert len(new_files(pair, after)) == 60 + assert not [rel for rel, content in after.items() if re.search(rb"[&*]id\d+", content)] + + # ── I-23: JSON roots ──────────────────────────────────────────────────── JSON_ROOT = json.dumps(yaml.safe_load(SPLIT_ALL), indent=2) + "\n" From c426d426ffb05374ad11e11f5935f4b635a430e2 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:51:29 +0100 Subject: [PATCH 27/30] test(split): pin the two mutants the round-2 review found alive - A fragment counted as written before its exclusive create: when another process makes a file at that name just before split does, the create fails and the take-back would remove that other file. A test makes one there at that moment and finds it kept, the refused split taken back. - The line an interrupt prints when its take-back leaves something: with the interrupt going on there is no message to carry it. A test fails a take-back step during an interrupt and finds the line, with the path and the reason, and the root put back all the same. --- tests/fragments/test_split.py | 60 +++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index 5aee6313..a1a50f4e 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -3313,6 +3313,66 @@ def unlink(self: Path, *args: Any, **kwargs: Any) -> None: ) +def test_i34_an_interrupt_whose_take_back_leaves_something_says_what( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """C1: when a step of the take-back fails during an interrupt, the interrupt + still goes on, and what is left is printed before it: there is no message to + put it in.""" + pair = make_pair(tmp_path, SPLIT_ALL_FILES) + real_compose = loader.compose_contract + + def check(path: Any, *args: Any, **kwargs: Any) -> Any: + if (pair.split / "fragments").exists(): + raise KeyboardInterrupt + return real_compose(path, *args, **kwargs) + + def locked(self: Path, *args: Any, **kwargs: Any) -> None: + raise PermissionError(13, "Permission denied", str(self)) + + capsys.readouterr() + with pytest.MonkeyPatch.context() as patch: + patch.setattr(loader, "compose_contract", check) + patch.setattr(Path, "unlink", locked) + with pytest.raises(KeyboardInterrupt): + _run_split(pair.split_contract, "builds,exposes") + + err = capsys.readouterr().err + assert "Split interrupted; could not take back " in err + stuck = pair.split / "fragments" / "builds" / "ingest-orders.yaml" + assert f"{stuck}: Permission denied" in err + assert pair.split_contract.read_text("utf-8") == SPLIT_ALL # the root went back all the same + + +def test_i34_a_file_someone_else_makes_just_before_split_does_survives_the_take_back( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """SP-05: a fragment is counted as written once split has made its file, not + before. A file another process makes at that name just before (so split's + exclusive create fails) is not split's to take back, and stays.""" + from fluid_build.cli import split as split_command + + text = "id: x\nsovereignty:\n jurisdiction: EU\nbuilds:\n - id: a\n pattern: p\n" + pair = solo(tmp_path, text) + theirs = pair.split / "fragments" / "builds" / "a.yaml" + real_open = open + + def made_just_before(file: Any, mode: str = "r", *args: Any, **kwargs: Any) -> Any: + if mode == "xb" and Path(file) == theirs: + theirs.write_text("someone else's file\n", encoding="utf-8") + return real_open(file, mode, *args, **kwargs) + + monkeypatch.setattr(split_command, "open", made_just_before, raising=False) + capsys.readouterr() + + assert split(pair.split_contract, "--parts", "sovereignty,builds") == 1 + + assert "File exists" in capsys.readouterr().err + assert theirs.read_text("utf-8") == "someone else's file\n" + assert pair.split_contract.read_text("utf-8") == text + assert not (pair.split / "fragments" / "sovereignty.yaml").exists() + + def test_i34_a_write_that_fails_takes_back_the_file_it_was_writing( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] ) -> None: From 30469af3991263f5eb02b6a7cf51987d164a2f72 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:01:00 +0100 Subject: [PATCH 28/30] fix(split): the help names the copy split writes over, the hard link a way out Two things the words got wrong after SP-11: - The help said 'a file that exists is never written over' without the one exception: a byte-for-byte copy of the contract in --out that is the only name of its file. It says so now. - When the file in --out composes to this contract but is a hard link (to the contract in its own directory, or to a third file), the refusal gave 'To split that one, run: fluid split /contract.fluid.yaml', which splits it in place, through the link: the other name changes with it. The refusal now says it is a hard link and gives a way out that writes through nothing: move it away, or choose another --out. The command stays the way out for a file that is the only name of its own. The two hard-link tests check the way out given, and a test reads the help. --- fluid_build/cli/split.py | 24 +++++++++++++++++++----- tests/fragments/test_split.py | 25 +++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 45020aca..74cc4442 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -71,10 +71,12 @@ def register(subparsers: argparse._SubParsersAction) -> None: " extensions.: that one extension block\n" " description: the description texts to docs/.md\n\n" "A part that is already a $ref is left as it is, so splitting twice\n" - "changes nothing. A file that exists is never written over. A relative\n" - "$ref in a part that moves is rewritten to name the same file from\n" - "the part's new one. With --out, the files relative $refs name must be\n" - "under the output directory too, where they are beside the contract.\n\n" + "changes nothing. A file that exists is never written over, but for a\n" + "byte-for-byte copy of the contract in --out that is the only name of\n" + "its file. A relative $ref in a part that moves is rewritten to name\n" + "the same file from the part's new one. With --out, the files relative\n" + "$refs name must be under the output directory too, where they are\n" + "beside the contract.\n\n" "Examples:\n" " fluid split contract.fluid.yaml # split in place\n" " fluid split contract.fluid.yaml --parts builds,sql # choose the parts\n" @@ -263,11 +265,23 @@ def _root_in_the_way(contract_path: Path, root_out: Path, before: Dict[str, Any] same = compose_contract(root_out).document == before except Exception: # noqa: BLE001 - one that does not compose is another contract same = False - if same: + try: + linked = root_out.stat().st_nlink > 1 + except OSError: + linked = True # not known to be the only name of its file + if same and not linked: return ( f"{root_out} already holds this contract (it composes to the same document); " f"split writes over no file. To split that one, run: fluid split {root_out}" ) + if same: + # Split in place, it would be written through the link: the other name, + # the contract in its own directory say, would change with it. + return ( + f"{root_out} already holds this contract, but is also another name of a " + f"file (a hard link) that splitting it would change too; split writes over " + f"no file. Move it away, or choose another --out" + ) return ( f"{root_out} already exists; split writes over no file but the contract it " f"splits in place. Move it away, or choose another --out" diff --git a/tests/fragments/test_split.py b/tests/fragments/test_split.py index a1a50f4e..b0784ec7 100644 --- a/tests/fragments/test_split.py +++ b/tests/fragments/test_split.py @@ -2841,7 +2841,11 @@ def test_i29_a_hard_link_to_the_contract_in_out_is_not_written_through( assert split(pair.split_contract, "--out", out) == 1 - assert "already holds this contract" in capsys.readouterr().err + err = capsys.readouterr().err + assert "already holds this contract" in err + # The way out it gives does not write through the link: splitting that name + # in place would change the contract in its own directory too. + assert "Move it away, or choose another --out" in err and "run: fluid split" not in err assert digest(tmp_path) == before @@ -2861,7 +2865,9 @@ def test_i29_a_copy_in_out_that_is_a_hard_link_to_a_third_file_is_not_written_th assert split(pair.split_contract, "--out", out) == 1 - assert f"Split not written: {out / NAME} already" in capsys.readouterr().err + err = capsys.readouterr().err + assert f"Split not written: {out / NAME} already" in err + assert "Move it away, or choose another --out" in err and "run: fluid split" not in err assert digest(tmp_path) == before assert victim.read_text("utf-8") == SPLIT_ALL @@ -3805,6 +3811,21 @@ def test_i39_split_help_lists_the_parts_option_and_every_part( assert "sovereignty,accessPolicy,builds,exposes" in out # the default +def test_i39_the_help_names_the_one_file_split_writes_over_but_the_contract( + capsys: pytest.CaptureFixture[str], +) -> None: + """SP-11 in the help: 'a file that exists is never written over' was said + without the one exception, a byte-for-byte copy of the contract in --out.""" + assert run_cli("split", "-h") == 0 + + out = " ".join(capsys.readouterr().out.split()) + assert ( + "A file that exists is never written over, but for a byte-for-byte copy of the " + "contract in --out that is the only name of its file." in out + ) + assert "With --out, the files relative $refs name must be under the output directory" in out + + # ── I-40: the authoring schema accepts what split writes ──────────────── From 4552d409d56d4ae3a762ded3b733822ea626061a Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:37:55 +0100 Subject: [PATCH 29/30] refactor(split): the encode step and the take-back report get helpers of their own --- fluid_build/cli/split.py | 84 ++++++++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 30 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index 74cc4442..b57833ce 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -42,7 +42,7 @@ import logging import sys from pathlib import Path -from typing import Any, Callable, Collection, Dict, Iterable, List, Optional, Sequence, Tuple +from typing import Any, Callable, Collection, Dict, Iterable, List, Optional, Sequence, Tuple, Union COMMAND = "split" @@ -362,22 +362,10 @@ def _write_checked( unsafe = _unsafe_target(out_dir, root_out, fragments) if unsafe: return unsafe - # Every text is encoded before a file or a directory is made: one with no - # UTF-8 form (a lone surrogate in a part, or in a key a JSON root keeps: - # JSON can escape one) is refused with nothing to take back. The bytes are - # written as they are, so a \r in a text stays a \r. - data: Dict[Path, bytes] = {} - for rel_path, content in fragments.items(): - try: - data[out_dir / rel_path] = content.encode("utf-8") - except UnicodeEncodeError as exc: - return f"{rel_path} cannot be written as UTF-8: {exc}" - except Exception as exc: # noqa: BLE001 - a part that is no text: refused, nothing made - return f"{rel_path} cannot be written: {exc}" - try: - root_data = root_text.encode("utf-8") - except UnicodeEncodeError as exc: - return f"{root_out} cannot be written as UTF-8: {exc}" + encoded = _encoded(root_out, root_text, fragments, out_dir) + if isinstance(encoded, str): + return encoded + data, root_data = encoded original_root: Optional[bytes] = root_out.read_bytes() if root_out.exists() else None made: List[Path] = [] written: List[Path] = [] @@ -398,22 +386,58 @@ def _write_checked( if compose_contract(root_out).document != before: raise ValueError("the split contract does not compose to the original one") except BaseException as exc: # noqa: BLE001 - every failure is taken back - left, interrupted = _take_back( - written, root_out if root_reached else None, original_root, made - ) - # Ctrl-C or an exit, halfway or during the take-back: the unchecked split - # is taken back all the same, and the first interrupt goes on. - interrupt = interrupted if isinstance(exc, Exception) else exc - if interrupt is not None: - if left: - sys.stderr.write(f"❌ Split interrupted; could not take back {'; '.join(left)}\n") - raise interrupt - if left: - return f"{exc}; and split could not take back {'; '.join(left)}" - return f"{exc}; nothing was changed" + return _taken_back(exc, written, root_out if root_reached else None, original_root, made) return None +def _encoded( + root_out: Path, root_text: str, fragments: Dict[str, str], out_dir: Path +) -> Union[str, Tuple[Dict[Path, bytes], bytes]]: + """Every text of the split as bytes, each fragment's by its path, and the + root's; or why one cannot be written. + + Encoded before a file or a directory is made: one with no UTF-8 form (a lone + surrogate in a part, or in a key a JSON root keeps: JSON can escape one) is + refused with nothing to take back. The bytes are written as they are, so a + \\r in a text stays a \\r. + """ + data: Dict[Path, bytes] = {} + for rel_path, content in fragments.items(): + try: + data[out_dir / rel_path] = content.encode("utf-8") + except UnicodeEncodeError as exc: + return f"{rel_path} cannot be written as UTF-8: {exc}" + except Exception as exc: # noqa: BLE001 - a part that is no text: refused, nothing made + return f"{rel_path} cannot be written: {exc}" + try: + return data, root_text.encode("utf-8") + except UnicodeEncodeError as exc: + return f"{root_out} cannot be written as UTF-8: {exc}" + + +def _taken_back( + exc: BaseException, + written: List[Path], + root_out: Optional[Path], + original_root: Optional[bytes], + made: List[Path], +) -> str: + """Take back a split that ``exc`` stopped halfway, and say why it stopped. + + Ctrl-C or an exit, halfway or during the take-back: the unchecked split is + taken back all the same, and the first interrupt is raised again. + """ + left, interrupted = _take_back(written, root_out, original_root, made) + interrupt = interrupted if isinstance(exc, Exception) else exc + if interrupt is not None: + if left: + sys.stderr.write(f"❌ Split interrupted; could not take back {'; '.join(left)}\n") + raise interrupt + if left: + return f"{exc}; and split could not take back {'; '.join(left)}" + return f"{exc}; nothing was changed" + + def _take_back( written: List[Path], root_out: Optional[Path], original_root: Optional[bytes], made: List[Path] ) -> Tuple[List[str], Optional[BaseException]]: From d05b8342e3571125390a1d595612006bab86b12e Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 29 Oct 2026 17:02:08 +0100 Subject: [PATCH 30/30] refactor(split): the take-back names the interrupts it handles instead of catching BaseException Ctrl-C and an exit are still taken back and raised again; nothing else that is not an Exception is caught (code-quality: except BaseException). --- fluid_build/cli/split.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/fluid_build/cli/split.py b/fluid_build/cli/split.py index b57833ce..f5d8a1d8 100644 --- a/fluid_build/cli/split.py +++ b/fluid_build/cli/split.py @@ -385,7 +385,9 @@ def _write_checked( handle.write(root_data) if compose_contract(root_out).document != before: raise ValueError("the split contract does not compose to the original one") - except BaseException as exc: # noqa: BLE001 - every failure is taken back + except Exception as exc: # every failure is taken back + return _taken_back(exc, written, root_out if root_reached else None, original_root, made) + except (KeyboardInterrupt, SystemExit) as exc: # an interrupt too, then raised again return _taken_back(exc, written, root_out if root_reached else None, original_root, made) return None @@ -464,14 +466,20 @@ def _take_back( for path, step in steps: try: step() - except BaseException as exc: # noqa: BLE001 - reported; the next step still runs - why = getattr(exc, "strerror", None) or str(exc) or type(exc).__name__ - left.append(f"{path}: {why}") - if interrupt is None and not isinstance(exc, Exception): + except Exception as exc: # reported; the next step still runs + left.append(f"{path}: {_why(exc)}") + except (KeyboardInterrupt, SystemExit) as exc: # the same, and raised again after + left.append(f"{path}: {_why(exc)}") + if interrupt is None: interrupt = exc return left, interrupt +def _why(exc: BaseException) -> str: + """What stopped a take-back step: the OS's reason, the message, or the name.""" + return getattr(exc, "strerror", None) or str(exc) or type(exc).__name__ + + def _put_back(root: Path, original: Optional[bytes]) -> None: """``root`` as it was before the split: ``original`` written back when its bytes differ from it, or removed when there was none."""