From aed9190bce93dd3242606615038ee65b561e7756 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 22:38:57 +0000 Subject: [PATCH 1/8] adopt-project.py: a value `execute` writes into a file is asked whether a file can hold it, in `plan`, in `check` and in `execute` before any leg exists, so a byte that is not UTF-8 in `--elected-by`, `--reference` or a local `--source` directory no longer pushes both legs before `write_lf` raises (#255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #247 the plan writer keeps a lone surrogate (a byte that is not UTF-8, as Python decodes argv and paths) as `\udce9`, and the reader gives it back. `plan --elected-by b'Jos\xe9'`, `--reference b'ref-\xe9'` and a source under a directory `caf\xe9` with local remotes therefore passed `plan` and `check`, and `execute` created and pushed both leg remotes before `write_lf` raised `UnicodeEncodeError` (exit 1). Before #247 `plan` raised and nothing was created. A hand-edited `\ud800` in `elected_by`, `elected_on`, `reference`, `visibility`, a pin or a follow-up ended the same way, and a NUL or a line break there went into the manifest. `shape_materialize.unwritable_reason` asks the question, beside `one_printable_line`: one line, no lone surrogate, no control character. It is `one_printable_line` less `Cf`, and it takes the empty value: a Persian name joins with U+200C, which a file holds and `execute` adopted before #247 and since, and an empty `elected_by` is what `plan` records when `git config user.name` names nobody. - `plan` refuses `--elected-by` and `--reference` as `adopt-unwritable-value` and writes no plan. - `check` reports `elected_by`, `elected_on`, `reference`, `visibility`, each pin and each follow-up as `plan-unwritable-value`, through `_entry_findings`, so `execute` refuses the same findings in `check`'s words through `_refuse_what_check_finds`, before `_create_leg_remotes`. - `execute` refuses a local clone URL (the source's own path under `--local-remote-dir`) as `adopt-unwritable-value`, right after `_repository_urls`. `check` cannot know it will be written, and against GitHub it is an https URL, so `plan` and `check` still pass such a directory, as #247 made them. - `Source.open` refuses a `local_path` that does not exist and is not text as `source-unresolvable`, where a NUL or a surrogate reached `print` or `subprocess` as a traceback. - `check`'s WHAT WILL HAPPEN shows the plan's visibility, adopt branch, leg paths and follow-ups through `_shown`, so a surrogate there no longer raises on a strict stdout before the finding is printed. tests/test_adopt_template_values.py covers each argv route, the local source directory, each hand-edited value, `local_path`, the printed values, and controls (`José`, U+200C, a `café` source directory) that still adopt. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 171 +++++++++- scripts/shape_materialize.py | 39 +++ tests/test_adopt_template_values.py | 507 ++++++++++++++++++++++++++++ 3 files changed, 709 insertions(+), 8 deletions(-) create mode 100644 tests/test_adopt_template_values.py diff --git a/adopt-project.py b/adopt-project.py index 6958d153..d505b2c2 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -120,6 +120,9 @@ MOVE_THE_LEG, first_and_more as _first_and_more, path_overlap as _overlap, shape_path_collisions, ) +# Whether a value this tool writes into a file is text a file can hold +# (#255), asked beside `one_printable_line`, the trailer's question. +from shape_materialize import unwritable_reason # noqa: E402 #: The naming policy this tool classifies leg names against. One constant, #: because three spellings of the same path is how the second one goes stale. @@ -395,13 +398,20 @@ def open(cls, spec: str, work_root: Path) -> "Source": path = candidate.resolve() repository = _remote_repository(path) else: - if "/" not in spec or WINDOWS_ABSOLUTE_RE.match(spec): + if ("/" not in spec or WINDOWS_ABSOLUTE_RE.match(spec) + or unwritable_reason(spec)): # A DRIVE-LETTER PATH IS A PATH THAT IS NOT THERE, never an # `org/repo`. `D:/work/Thing` carries a `/` and would # otherwise be handed to `gh repo clone` as an owner named # `D:`; `os.path.isabs` cannot be asked instead, because it # answers False for that string on Linux and macOS and this # refusal must read the same on every platform. + # NOR IS A NAME THAT IS NOT TEXT (#255). Since #247 a plan's + # `local_path:` can spell a NUL, a control character or a + # lone surrogate, and the reader gives it back: a path that + # cannot exist, which reached `print` (a surrogate raised on + # a strict stdout) or `subprocess` (a NUL raised there) as a + # traceback, where main had refused it. raise Refusal( "source-unresolvable", f"--source {spec!r} is neither a path that exists nor an " @@ -1235,6 +1245,18 @@ def _checked_plan_inputs(args, source: Source, election_date(args.elected_on) args.reference = args.reference or default_reference(args.elected_on) args.elected_by = args.elected_by or _elector() + # Before the plan is written, in the words `check` would use (#255): a + # value `execute` writes into a file must be text one can hold. The + # date has had its answer above, `election_date`. + unwritable = unwritable_values( + (("--elected-by", args.elected_by, ROOT_MANIFEST_AND_README), + ("--reference", args.reference, ROOT_MANIFEST_AND_README))) + if unwritable: + raise Refusal("adopt-unwritable-value", + " ".join(f"{problem}." for problem in unwritable), + "Remediation: no plan was written. Pass each value " + "named again, retyped (`--elected-by` defaults to " + "`git config user.name`). " + WRITTEN_AS_TEXT) names = _names(args.project) pins = [_checked_pin_name(p) for p in (args.pin or []) if p.strip()] _check_names(naming, names, set(pins)) @@ -1903,7 +1925,8 @@ def _entry_findings(plan: Plan, source: Source ) -> tuple[list[str], list[str]]: """Every FINDING `check` prints about the plan's ENTRIES, in its order, and what it prints once after them: coverage, then each entry's `leg:` - and the two mount paths, then the source's own submodules. + and the two mount paths, then the source's own submodules, then each + value `execute` writes into a file that no file can hold (#255). ONE LIST, read by `check` and by `execute`'s `_refuse_what_check_finds`. #168 and #166 each added findings to `check` and a refusal to `execute` @@ -1923,7 +1946,9 @@ def _entry_findings(plan: Plan, source: Source [path for path, _, _, _ in tree]) + _leg_findings(plan, tree)) submodule_findings, remediation = _submodule_findings(plan, source, tree) - return findings + submodule_findings, remediation + value_findings, value_remediation = _value_findings(plan) + return (findings + submodule_findings + value_findings, + remediation + value_remediation) def _topics_line(topic: str, local: bool) -> str: @@ -1945,23 +1970,27 @@ def _print_what_will_happen(plan: Plan, source: Source, names: dict, seeded = seeded_legs(assigned_paths(plan.entries)) where = source.repository or _shown(str(source.path)) print(f"\nWHAT WILL HAPPEN, against {where} @ {source.commit[:12]}:") + # Every value below that is the plan's own is `_shown`, as a name is: + # since #247 the reader gives back a lone surrogate, which raised on a + # strict stdout before the FINDING about it was printed (#255). print(f" create {names['spec']} and {names['code']} " - f"({plan.get('visibility', 'private')})") + f"({_shown(str(plan.get('visibility', 'private')))})") print(f" extract {len(moved)} path(s) with `git filter-repo`, history " "preserved") for role in seeded: print(f" SEED the {role} leg from {SEED_TEMPLATE[role]}/ — no path is " "assigned to it") - print(f" ONE split commit on branch {plan.get('adopt_branch', ADOPT_BRANCH)} " + print(f" ONE split commit on branch " + f"{_shown(str(plan.get('adopt_branch', ADOPT_BRANCH)))} " f"of {names['assembly']}: `git rm -r` those paths, mount the legs at " - f"{_plan_leg_path(plan, 'spec')}/ and " - f"{_plan_leg_path(plan, 'code')}/") + f"{_shown(str(_plan_leg_path(plan, 'spec')))}/ and " + f"{_shown(str(_plan_leg_path(plan, 'code')))}/") print(f" {len(stays)} path(s) stay in the assembly root; " f"{len(drops)} dropped") print(" the source is never deleted, never renamed, never force-pushed") print(_topics_line(topic, local)) for item in plan.get("follow_ups", []): - print(f" follow-up: {item}") + print(f" follow-up: {_shown(str(item))}") def cmd_check(args) -> int: @@ -2779,6 +2808,7 @@ def cmd_execute(args) -> int: # validated before it is used rather than after something has gone wrong. spec_path, code_path, branch, tracking = _checked_plan_values(plan) local, repositories, urls = _repository_urls(args, plan, names, source) + _refuse_an_unwritable_clone_url(urls["assembly"]) paths_for = _leg_paths(plan) _refuse_what_check_finds(plan, source) @@ -2839,6 +2869,131 @@ def cmd_execute(args) -> int: return 2 if topics_failed else 0 +# --------------------------------------------------------------------------- +# What `execute` writes into a file is text a file can hold (#255) +# --------------------------------------------------------------------------- +# +# `_template_values` puts `elected_by`, `elected_on`, `reference`, +# `visibility`, each pin and the assembly root's clone URL into the templates +# as they stand, and `write_lf` writes the result as strict UTF-8; +# `_commit_the_split` puts each follow-up into the split commit's message. +# Since #247 a plan can carry a lone surrogate -- a byte that is not UTF-8, +# as `sys.argv` and `os.fsdecode` keep one -- and the reader gives it back. +# So `plan` and `check` passed `--elected-by b'Jos\xe9'`, and `execute` +# created and pushed both legs before `write_lf` raised `UnicodeEncodeError`. +# Before #247, `plan` raised on it and nothing was made. A line break or a +# NUL in the plan went on into the manifest. +# +# So `unwritable_reason` is asked of each such value before anything exists. +# `plan` refuses `--elected-by` and `--reference` as +# `adopt-unwritable-value`, and writes no plan (`--elected-on` is a date, and +# `election_date` refuses anything else). `check` reports each plan value as +# `plan-unwritable-value`, and `execute` refuses those findings through +# `_refuse_what_check_finds`, before `_create_leg_remotes`. Every other value +# the templates take is a `checked_value`, the project id, or one of this +# tool's own. +# +# THE CLONE URL IS `execute`'S OWN. Under `--local-remote-dir` it is the +# source's path, and `check` has no such flag to know it will be written. +# Against GitHub it is `https://github.com//.git`, and the path is +# written nowhere. So `execute` refuses an unwritable one itself, right +# after `_repository_urls`, and `plan` and `check` still pass a source +# directory whose name is not UTF-8, as #247 made them. + +#: `check`'s finding, and the code `execute` refuses it under. +UNWRITABLE_VALUE = "plan-unwritable-value" + +#: Where `execute` writes the values it is asked about. +ROOT_MANIFEST = "the assembly root's `project.yaml`" +ROOT_MANIFEST_AND_README = "the assembly root's `project.yaml` and `README.md`" +SPLIT_MESSAGE = "the split commit's message" + +#: Each plan value `_template_values` substitutes as it stands, by its key, +#: and where it lands. `pins` and `follow_ups` are lists, asked item by item. +WRITTEN_PLAN_VALUES = {"elected_by": ROOT_MANIFEST_AND_README, + "elected_on": ROOT_MANIFEST_AND_README, + "reference": ROOT_MANIFEST_AND_README, + "visibility": ROOT_MANIFEST} + +#: Why such a value must be text, said after every refusal of one. +WRITTEN_AS_TEXT = ( + "`execute` writes each one as it stands, into the assembly root's files " + "or the split commit's message, so it must be one line of UTF-8 text. A " + "byte that is not UTF-8, or a lone surrogate, cannot be written at all, " + "and a line break or a control character would be written raw into a " + "value that is read back as one line. A byte that is not UTF-8 reaches a " + "command line from a terminal or a script whose encoding is not the " + "locale's.") + +PLAN_VALUE_REMEDIATION = ("Remediation: correct each value named in the plan, " + "or re-run `plan` with it retyped. " + + WRITTEN_AS_TEXT) + + +def unwritable_values(fields) -> list[str]: + """A sentence for each `(what, value, where)` whose `value` is not text + a file can hold; `[]` when every one is. + + `what` names the value as the person has it in front of them -- + `--elected-by` to `plan`, `elected_by` read out of a plan -- and `where` + is the file `execute` writes it into. The value is spelled as Python + spells it, as `unsafe-value` spells one, so the sentence stays one line + of text whatever the value holds. + """ + out = [] + for what, value, where in fields: + reason = unwritable_reason(value) + if reason: + out.append(f"{what} is {value!r}, which `execute` writes into " + f"{where}, and {reason}") + return out + + +def _plan_values(plan: Plan) -> list[tuple[str, str, str]]: + """`(what, value, where)` for each value of the plan's that `execute` + writes into a file, as `str` makes it, which is how `_template_values` + and `_commit_the_split` take it. A key the plan does not carry is filled + by this tool (`private`, today's date) and is left out.""" + fields = [(key, str(plan.get(key)), where) + for key, where in WRITTEN_PLAN_VALUES.items() + if plan.get(key) is not None] + fields += [("a `pins:` item", pin, ROOT_MANIFEST) for pin in plan.pins] + return fields + [("a `follow_ups:` item", str(item), SPLIT_MESSAGE) + for item in plan.get("follow_ups", [])] + + +def _value_findings(plan: Plan) -> tuple[list[str], list[str]]: + """`unwritable_values` of the plan as `check` prints them, one FINDING + each, and what to print once after every finding: how to correct them + when there is one, nothing when there is none.""" + found = [f"FINDING {UNWRITABLE_VALUE}: {problem}" + for problem in unwritable_values(_plan_values(plan))] + return found, [PLAN_VALUE_REMEDIATION] if found else [] + + +def _refuse_an_unwritable_clone_url(url: str) -> None: + """Refuse the assembly root's clone URL when no file can hold it, before + any leg exists. + + Only a local one can be refused: under `--local-remote-dir` the URL is + the source's own path, which the assembly root's `README.md` names (and + a seeded leg's), so a source directory whose name is not UTF-8 pushed + both legs and then raised in `write_lf` (#255). + """ + reason = unwritable_reason(url) + if reason: + raise Refusal( + "adopt-unwritable-value", + "under --local-remote-dir the assembly root's clone URL is the " + f"source's own path, {url!r}, which `execute` writes into its " + f"`README.md`, and {reason}", + "Remediation: nothing was created. Clone the source to a path " + "that is one line of UTF-8 text, and pass that path to `execute` " + "with --source; or adopt against GitHub, without " + "--local-remote-dir, where the clone URL is " + "`https://github.com//.git`.") + + def _template_values(plan: Plan, names, repositories, urls, spec_path, code_path, tracking, leg_commits, leg_digests, shape_commit, pins, naming) -> dict[str, str]: diff --git a/scripts/shape_materialize.py b/scripts/shape_materialize.py index 81780a70..8c4905b2 100644 --- a/scripts/shape_materialize.py +++ b/scripts/shape_materialize.py @@ -777,6 +777,45 @@ def is_trailer(value: str) -> bool: return bool(TRAILER_RE.fullmatch(value)) and one_printable_line(value) +def unwritable_reason(value: str) -> str | None: + r"""Why `value` is not one line of text a file can hold, in words; None + when it is (#255). + + `adopt-project.py` writes a value a person gave it -- `--elected-by`, + `--reference`, the source's own path as a local clone URL -- into the + assembly root's `project.yaml` and `README.md` through `write_lf`, which + is strict UTF-8. Since #247 the plan keeps a lone surrogate (a byte that + is not UTF-8, as `sys.argv` and `os.fsdecode` keep it) as `\udce9`, and + the reader gives it back, so `plan` and `check` passed such a value and + `write_lf` raised `UnicodeEncodeError` after both legs were pushed. This + is the question asked of every such value before anything exists. + + `one_printable_line` LESS `Cf`, AND THE EMPTY VALUE, because both are + text a file holds and both were adopted before #247 and since. A `Cf` + character is invisible, not unwritable: UTF-8 encodes it, the reader + reads it back, and a name may need one -- a Persian name joins its parts + with U+200C ZERO WIDTH NON-JOINER, which `one_printable_line` refuses. + An empty `elected_by` is what `plan` records when `git config user.name` + names nobody, and the manifest holds it as `""`. What is left is + refused: a lone surrogate, which UTF-8 cannot encode; a line break, + which ends the line the manifest's reader reads the value from (CR, + U+0085 and U+2028 among them, as `str.splitlines` ends lines); and any + other control character, NUL among them, which YAML does not allow raw + (TAB aside, and no name, date or reference holds one either). + """ + if value.splitlines() not in ([], [value]): + return "it holds a line break, and it is written as one line" + for char in value: + category = unicodedata.category(char) + if category == "Cs": + return ("it holds a byte that is not UTF-8" + if "\udc80" <= char <= "\udcff" else + "it holds a lone surrogate, which is no character") + if category in UNPRINTABLE_CATEGORIES and category != "Cf": + return "it holds a control character" + return None + + #: The git that learned `commit --trailer` (2.32, 2021-06-06). An older one #: still has `git interpret-trailers`, but not the commit option — so on that #: host the trailer is appended to the message TEXT instead, which produces diff --git a/tests/test_adopt_template_values.py b/tests/test_adopt_template_values.py new file mode 100644 index 00000000..1fd177e3 --- /dev/null +++ b/tests/test_adopt_template_values.py @@ -0,0 +1,507 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A value `execute` writes into a file is asked, before anything exists, +whether a file can hold it (#255). + +Since #247 the plan writer keeps a lone surrogate -- a byte that is not +UTF-8, as Python decodes argv and paths -- as `\\udce9`, and the reader gives +it back. So `plan --elected-by b'Jos\\xe9'`, `--reference b'ref-\\xe9'`, or a +`--source` under a directory `caf\\xe9` with local remotes (the clone URL +carries it) passed `plan` and `check`, and `execute` created and pushed both +leg remotes before `write_lf` raised `UnicodeEncodeError`, exit 1. Before +#247, `plan` raised on the same input and nothing was created. A hand-edited +`\\ud800` in `elected_by`, `elected_on`, `reference`, `visibility`, a pin or a +follow-up ended the same way; a NUL or a line break there went on into the +manifest; and a hand-edited `\\x00` or surrogate in `local_path` ended +`check` and `execute` in a traceback where main had refused it. + +Now `plan` refuses such a flag as `adopt-unwritable-value` and writes no +plan; `check` reports such a plan value as `plan-unwritable-value`, and +`execute` refuses the same finding before any leg exists; `execute` refuses +a local clone URL no file can hold as `adopt-unwritable-value`; and a +`local_path` that is not text is `source-unresolvable`. Every run is read +for a traceback, and asked afterwards whether a leg remote exists. + +THE QUESTION IS `one_printable_line` LESS `Cf`. A Persian name joins its +parts with U+200C ZERO WIDTH NON-JOINER, which UTF-8 holds and the reader +reads back, and `execute` adopted it before #247 and since: it still adopts, +as `José` and a source directory `café` do. + +Every subprocess runs under `PYTHONIOENCODING=utf-8:strict`, so that a value +printed raw raises on every platform as it does on a strict terminal, and is +answered in BYTES: a value that is not UTF-8 is not text to decode. +""" + +from __future__ import annotations + +import os +import re +import shutil +import subprocess +import sys +import unicodedata +from pathlib import Path + +import pytest + +from conftest import (ADOPT, REPO, blank_unnamed_pin_sources, + make_source_repo, resolve) + +sys.path.insert(0, str(REPO / "scripts")) +import shape_materialize # noqa: E402 +from repo_shape import double_quoted, load_yaml # noqa: E402 + +PLAN_FILE = "adoption-plan.yaml" +REMOTES = "remotes" +WORK = "work" +PROJECT = "Northwind" +#: Where `execute` pushes the split commit: back into the source, under +#: `--local-remote-dir`. +SPLIT_BRANCH = "adopt/three-repo-shape" +#: The entries of `SYNTHETIC_TREE` that `plan` asks about, answered. +ANSWERS = (("examples/", "spec"), (".claude/", "root"), + ("release.yaml", "root")) +#: A stdout that raises on what it cannot encode, as a strict terminal does. +STRICT = {"PYTHONIOENCODING": "utf-8:strict"} +#: What the git that `execute` runs needs, as `conftest.run_script` sets it. +IDENTITY = {"GIT_AUTHOR_NAME": "openRepoShape tests", + "GIT_AUTHOR_EMAIL": "tests@openreposhape.invalid", + "GIT_COMMITTER_NAME": "openRepoShape tests", + "GIT_COMMITTER_EMAIL": "tests@openreposhape.invalid"} + +TRACEBACK = b"Traceback" +VERIFIED = b"adoption verified" +PLAN_OK = b"\nplan ok\n" +ADOPT_REFUSED = b"REFUSED adopt-unwritable-value: " +PLAN_REFUSED = b"REFUSED plan-unwritable-value: " +FINDING = b"FINDING plan-unwritable-value: " +SOURCE_REFUSED = b"REFUSED source-unresolvable: " + +needs_filter_repo = pytest.mark.skipif( + shutil.which("git-filter-repo") is None, + reason="git filter-repo is not installed: `pip install git-filter-repo`") + + +def adopted(*args: str) -> subprocess.CompletedProcess: + """`adopt-project.py ` on a strict stdout, answered in bytes, each + stream with LF line ends whatever the platform printed.""" + env = blank_unnamed_pin_sources( + {**IDENTITY, **os.environ, "LANES_LANE": "", **STRICT}, STRICT) + done = subprocess.run([sys.executable, str(ADOPT), *args], + capture_output=True, env=env, + stdin=subprocess.DEVNULL, check=False) + done.stdout = done.stdout.replace(b"\r\n", b"\n") + done.stderr = done.stderr.replace(b"\r\n", b"\n") + return done + + +def planned(base: Path, source: Path, + *flags: str) -> subprocess.CompletedProcess: + """`plan`, as `conftest.write_plan` runs it, with `flags` after it: a + flag given twice is read as its last value.""" + return adopted("plan", "--source", str(source), "--project", PROJECT, + "--org", "testorg", "--elected-by", "Test Human", + "--elected-on", "2026-09-02", "--out", + str(base / PLAN_FILE), "--work-dir", str(base / WORK), + *flags) + + +def checked(base: Path) -> subprocess.CompletedProcess: + return adopted("check", "--plan", str(base / PLAN_FILE), "--work-dir", + str(base / WORK)) + + +def executed(base: Path) -> subprocess.CompletedProcess: + return adopted("execute", "--plan", str(base / PLAN_FILE), "--yes", + "--local-remote-dir", str(base / REMOTES), "--work-dir", + str(base / WORK)) + + +def answer(base: Path) -> None: + for path, leg in ANSWERS: + resolve(base / PLAN_FILE, path, leg) + + +def plan_check_execute(base: Path, source: Path, + *flags: str) -> dict[str, subprocess.CompletedProcess]: + """`plan` with `flags`, then, if it wrote a plan, that plan answered, + `check`, and `execute` with local remotes: each run there was.""" + runs = {"plan": planned(base, source, *flags)} + if runs["plan"].returncode == 0: + answer(base) + runs["check"] = checked(base) + runs["execute"] = executed(base) + return runs + + +def said(run: subprocess.CompletedProcess) -> str: + """A run's output, readable in an assertion message.""" + return (run.stdout + run.stderr).decode("utf-8", "backslashreplace") + + +def assert_no_traceback(runs: dict) -> None: + for name, run in runs.items(): + assert TRACEBACK not in run.stderr, f"{name}:\n{said(run)}" + + +def assert_no_traceback_and_no_leg(base: Path, runs: dict) -> None: + """The issue's two symptoms, asked first so that a failure names them: + a traceback in any run, and a leg remote left behind.""" + assert_no_traceback(runs) + remotes = base / REMOTES + left = (sorted(path.name for path in remotes.iterdir()) + if remotes.exists() else []) + assert not left, f"leg remotes exist: {left}" + + +def in_the_split(source: Path, path: str) -> str: + """`path` as the split commit `execute` pushed into `source` holds it.""" + return subprocess.run(["git", "show", f"{SPLIT_BRANCH}:{path}"], + cwd=str(source), capture_output=True, + check=True).stdout.decode("utf-8") + + +# --- `plan`: a flag no file can hold ----------------------------------------- + +#: `(flag, value)`, as `plan`'s argv carries it. `\udce9` is how Python keeps +#: the byte 0xE9 of a command line that is not UTF-8, which is what a +#: terminal or a script whose encoding is not the locale's sends; the other +#: two are text a file holds, but on no one line. +FLAGS = [ + ("--elected-by", "Jos\udce9"), + ("--reference", "ref-\udce9"), + ("--elected-by", "Jos
e"), + ("--reference", "openxFactory\tdocs/project-repo-schema.md"), +] + + +@pytest.mark.parametrize("flag, value", FLAGS, + ids=[f"{flag}={ascii(value)}" + for flag, value in FLAGS]) +def test_plan_refuses_a_flag_no_file_can_hold(tmp_path, flag, value): + """The issue's first two rows. On main `plan` exited 0, `check` said + `plan ok`, and `execute` raised in `write_lf` after both legs were + pushed (a surrogate), or adopted with the line break or the tab in the + manifest. Now `plan` refuses, and writes no plan.""" + source = make_source_repo(tmp_path / "Thing") + runs = plan_check_execute(tmp_path, source, flag, value) + assert_no_traceback_and_no_leg(tmp_path, runs) + refused = runs["plan"] + assert refused.returncode == 2, said(refused) + assert ADOPT_REFUSED + f"{flag} is {value!r}, which ".encode() \ + in refused.stderr, said(refused) + assert b"no plan was written" in refused.stderr + assert not (tmp_path / PLAN_FILE).exists() + + +# --- `execute`: a local clone URL no file can hold --------------------------- + +@needs_filter_repo +def test_execute_refuses_a_local_source_directory_that_is_not_utf8(tmp_path): + """The issue's third row. Under `--local-remote-dir` the assembly root's + clone URL is the source's own path, which its `README.md` names: `plan` + and `check` pass such a directory, as #247 made them, and `execute` + refuses it before any leg exists, where it raised after both were + pushed. Skipped where no directory can be named with the byte 0xE9 + (Windows, macOS's APFS) or git will not work in one.""" + try: + parent = Path(os.fsdecode(os.path.join(os.fsencode(tmp_path), + b"caf\xe9"))) + parent.mkdir() + except (OSError, UnicodeError) as exc: + pytest.skip(f"no directory here is named b'caf\\xe9': {exc!r}") + try: + source = make_source_repo(parent / "Thing") + except AssertionError as exc: + pytest.skip(f"git will not work under b'caf\\xe9' here: {exc}") + runs = plan_check_execute(tmp_path, source) + assert_no_traceback_and_no_leg(tmp_path, runs) + assert runs["plan"].returncode == 0, said(runs["plan"]) + assert runs["check"].returncode == 0, said(runs["check"]) + assert PLAN_OK in runs["check"].stdout, said(runs["check"]) + refused = runs["execute"] + assert refused.returncode == 2, said(refused) + assert ADOPT_REFUSED + b"under --local-remote-dir the assembly root's " \ + b"clone URL is the source's own path, " \ + + repr(str(source.resolve())).encode() in refused.stderr, said(refused) + assert b"nothing was created" in refused.stderr + + +# --- `check` and `execute`: a plan value edited by hand ---------------------- + +@pytest.fixture(scope="module") +def answered(tmp_path_factory) -> tuple[Path, str]: + """A source and the plan `plan` wrote for it, answered and `plan ok`: + the text each test edits a line of, in a copy of its own.""" + base = tmp_path_factory.mktemp("answered") + source = make_source_repo(base / "Thing") + written = planned(base, source) + assert written.returncode == 0, said(written) + answer(base) + ok = checked(base) + assert ok.returncode == 0, said(ok) + return source, (base / PLAN_FILE).read_text(encoding="utf-8") + + +def with_line(text: str, line: str) -> str: + """`text` with the one line holding `line`'s key -- `elected_by`, or an + indented ` spec_path` -- replaced by `line`.""" + key = line.split(": ", 1)[0] + text, done = re.subn(rf"^{re.escape(key)}: .*$", lambda _: line, text, + count=1, flags=re.M) + assert done == 1, line + return text + + +def edited_plan(base: Path, answered: tuple[Path, str], line: str, + follow_up: bool = False) -> None: + """The answered plan, its `local_path:` a copy of the source made for + this test alone (on main `execute` pushes its split branch into the + source), with `line` in place of the line of its key -- or, as + `follow_up`, added as the first `follow_ups:` item -- written LF as + `plan` writes it.""" + source, text = answered + copy = base / "Thing" + shutil.copytree(source, copy, symlinks=True) + text = with_line(text, f" local_path: {double_quoted(str(copy))}") + if follow_up: + text, done = re.subn(r"^follow_ups:$", + lambda _: f"follow_ups:\n{line}", text, count=1, + flags=re.M) + assert done == 1 + else: + text = with_line(text, line) + with (base / PLAN_FILE).open("w", encoding="utf-8", + newline="\n") as handle: + handle.write(text) + + +#: `(id, key, value, what the FINDING calls it)`. Each value is spelled in +#: the plan by `double_quoted`, as the plan writer escapes it and as a person +#: editing the plan would. +EDITS = [ + ("elected_by-surrogate", "elected_by", "T\ud800", "elected_by"), + ("elected_by-byte", "elected_by", "T\udce9", "elected_by"), + ("elected_by-nul", "elected_by", "T\x00", "elected_by"), + ("elected_by-cr", "elected_by", "T\rX", "elected_by"), + ("elected_by-nel", "elected_by", "T\x85X", "elected_by"), + ("elected_by-u2028", "elected_by", "T
X", "elected_by"), + ("elected_by-tab", "elected_by", "T\tX", "elected_by"), + ("elected_on-surrogate", "elected_on", "2026-09-02\ud800", "elected_on"), + ("reference-surrogate", "reference", "x\ud800", "reference"), + ("reference-nul", "reference", "x\x00", "reference"), + ("visibility-surrogate", "visibility", "priv\ud800ate", "visibility"), + ("pins-surrogate", "pins", "openX\ud800", "a `pins:` item"), + ("follow_ups-surrogate", "follow_ups", "fix x\ud800", + "a `follow_ups:` item"), +] + + +def edit(base: Path, answered: tuple[Path, str], key: str, + value: str) -> None: + """The answered plan, with `key` given `value`.""" + if key == "follow_ups": + edited_plan(base, answered, f" - {double_quoted(value)}", + follow_up=True) + elif key == "pins": + edited_plan(base, answered, f"pins: [{double_quoted(value)}]") + else: + edited_plan(base, answered, f"{key}: {double_quoted(value)}") + + +@needs_filter_repo +@pytest.mark.parametrize("key, value, what", [row[1:] for row in EDITS], + ids=[row[0] for row in EDITS]) +def test_check_finds_and_execute_refuses_a_hand_edited_value( + tmp_path, answered, key, value, what): + """On main `check` said `plan ok` (or raised printing the value), and + `execute` raised in `write_lf` after both legs were pushed, or adopted + with the NUL or the line break in the manifest. Now `check` has a + FINDING, and `execute` refuses it through `_refuse_what_check_finds`, in + `check`'s words, before any leg exists.""" + edit(tmp_path, answered, key, value) + runs = {"check": checked(tmp_path), "execute": executed(tmp_path)} + assert_no_traceback_and_no_leg(tmp_path, runs) + found, refused = runs["check"], runs["execute"] + finding = FINDING + f"{what} is {value!r}, which `execute` writes " \ + "into ".encode() + assert found.returncode == 1, said(found) + assert finding in found.stderr, said(found) + assert refused.returncode == 2, said(refused) + assert PLAN_REFUSED in refused.stderr, said(refused) + assert b"\n " + finding in refused.stderr, said(refused) + assert b"no leg repository was created" in refused.stderr + + +def test_plan_and_check_say_the_same_sentence(tmp_path, answered): + """`plan`'s refusal of `--elected-by` and `check`'s finding about + `elected_by` are one sentence about one value, each naming it as the + person has it in front of them.""" + value = "Jos\udce9" + flagged, by_hand = tmp_path / "flag", tmp_path / "edit" + flagged.mkdir() + by_hand.mkdir() + refused = planned(flagged, answered[0], "--elected-by", value) + edit(by_hand, answered, "elected_by", value) + found = checked(by_hand) + assert ADOPT_REFUSED in refused.stderr, said(refused) + assert FINDING in found.stderr, said(found) + detail = refused.stderr.split(ADOPT_REFUSED, 1)[1].split(b"\n", 1)[0] + finding = found.stderr.split(FINDING, 1)[1].split(b"\n", 1)[0] + assert detail.startswith(b"--elected-by is "), said(refused) + assert finding.startswith(b"elected_by is "), said(found) + assert detail[len(b"--elected-by"):] == finding[len(b"elected_by"):] + b"." + + +# --- a value `check` prints, edited by hand ---------------------------------- + +#: `(id, line key, value, how `check` shows it, check's exit, what it says, +#: execute's refusal)`: a value `check` printed raw under `WHAT WILL +#: HAPPEN`, which raised on a strict stdout before any finding about it was +#: printed. It is shown as a name is (`_shown`), a byte that is not UTF-8 as +#: `\xe9`. A leg path is `plan-bad-leg-path`; the adopt branch passes `check` +#: and is `unsafe-value` to `execute`, as each was on main before #247. +PRINTED = [ + ("spec_path-surrogate", " spec_path", "sp\ud800ec", rb"`sp\ud800ec`/", + 1, b"FINDING plan-bad-leg-path: legs.spec_path is ", + b"plan-bad-leg-path"), + ("code_path-byte", " code_path", "co\udce9de", rb"`co\xe9de`/", 1, + b"FINDING plan-bad-leg-path: legs.code_path is ", b"plan-bad-leg-path"), + ("adopt_branch-surrogate", "adopt_branch", "adopt/x\ud800", + rb"`adopt/x\ud800` of", 0, PLAN_OK, b"unsafe-value"), +] + + +@needs_filter_repo +@pytest.mark.parametrize("key, value, shown, exit_code, says, code", + [row[1:] for row in PRINTED], + ids=[row[0] for row in PRINTED]) +def test_check_prints_a_hand_edited_value_spelled(tmp_path, answered, key, + value, shown, exit_code, + says, code): + edited_plan(tmp_path, answered, f"{key}: {double_quoted(value)}") + runs = {"check": checked(tmp_path), "execute": executed(tmp_path)} + assert_no_traceback_and_no_leg(tmp_path, runs) + found, refused = runs["check"], runs["execute"] + assert found.returncode == exit_code, said(found) + assert says in found.stdout + found.stderr, said(found) + assert shown in found.stdout, said(found) + assert refused.returncode == 2, said(refused) + assert b"REFUSED " + code + b": " in refused.stderr, said(refused) + + +# --- `local_path`, edited by hand -------------------------------------------- + +#: How each `local_path` is made from the source's own path: the path that +#: exists with a NUL or a surrogate after it, and paths that do not exist. +LOCAL_PATHS = { + "nul": lambda source: f"{source}\x00", + "surrogate": lambda source: f"{source}\ud800", + "missing-nul": lambda _: "/nonexistent\x00/Thing", + "missing-byte": lambda _: "/nonexistent/caf\udce9", +} + + +@pytest.mark.parametrize("make", list(LOCAL_PATHS.values()), + ids=list(LOCAL_PATHS)) +def test_a_local_path_that_is_not_text_is_unresolvable(tmp_path, answered, + make): + """On main `check` and `execute` ended in a traceback: `print` raised on + the surrogate and `subprocess` on the NUL, as the value was about to be + handed to `gh repo clone` as an `org/repo`. It is no path that exists + and no `org/repo` name, which is what `source-unresolvable` says, and + what Windows, where such a path holds no `/`, said already.""" + value = make(tmp_path / "Thing") + edited_plan(tmp_path, answered, f" local_path: {double_quoted(value)}") + assert load_yaml(tmp_path / PLAN_FILE)["source"]["local_path"] == value + runs = {"check": checked(tmp_path), "execute": executed(tmp_path)} + assert_no_traceback_and_no_leg(tmp_path, runs) + for name, run in runs.items(): + assert run.returncode == 2, f"{name}:\n{said(run)}" + assert SOURCE_REFUSED + f"--source {value!r} is neither".encode() \ + in run.stderr, f"{name}:\n{said(run)}" + + +# --- controls: valid text still adopts, as on main --------------------------- + +@needs_filter_repo +@pytest.mark.parametrize("elected_by", ["José", "Ali‌Reza"], + ids=["accented", "zero-width-non-joiner"]) +def test_a_valid_non_ascii_elector_still_adopts(tmp_path, elected_by): + """`José`, and a name joined with U+200C, which `one_printable_line` + refuses and a file holds: adopted, and written into the manifest.""" + source = make_source_repo(tmp_path / "Thing") + runs = plan_check_execute(tmp_path, source, "--elected-by", elected_by) + assert_no_traceback(runs) + assert runs["check"].returncode == 0, said(runs["check"]) + done = runs["execute"] + assert done.returncode == 0, said(done) + assert VERIFIED in done.stdout, said(done) + assert f'\nelected_by: "{elected_by}"\n' in in_the_split(source, + "project.yaml") + + +@needs_filter_repo +def test_a_utf8_source_directory_still_adopts_with_local_remotes(tmp_path): + """A source under a directory `café`: its path is the clone URL, which a + file holds, so it adopts, and the shape's README (beside the source's + own) names it.""" + source = make_source_repo(tmp_path / "café" / "Thing") + runs = plan_check_execute(tmp_path, source) + assert_no_traceback(runs) + assert runs["check"].returncode == 0, said(runs["check"]) + done = runs["execute"] + assert done.returncode == 0, said(done) + assert VERIFIED in done.stdout, said(done) + readme = in_the_split(source, "shape/README.md") + assert f"git clone --recurse-submodules {source.resolve()}\n" in readme + + +# --- the question itself ----------------------------------------------------- + +#: Text a file holds: the empty value `plan` records when `git config +#: user.name` names nobody, a name, a date, the default reference, a literal +#: backslash, invisible `Cf` characters, and a space that is not ASCII's. +WRITABLE = ["", "José", "Ali‌Reza", "2026-09-02", + "openxFactory docs/project-repo-schema.md", "CORP\\rsmith", + "日本", "a b", "‮evil", "bom", "a­b"] + +#: `(value, the words its reason holds)`. +UNWRITABLE = [ + ("Jos\udce9", "a byte that is not UTF-8"), + ("x\ud800", "a lone surrogate"), + ("x\udfff", "a lone surrogate"), + ("a\nb", "a line break"), ("a\rb", "a line break"), + ("a\r\nb", "a line break"), ("end\n", "a line break"), + ("a\x0bb", "a line break"), ("a\x0cb", "a line break"), + ("a\x1cb", "a line break"), ("a\x85b", "a line break"), + ("a
b", "a line break"), ("a
b", "a line break"), + ("a\x00b", "a control character"), ("a\tb", "a control character"), + ("a\x1bb", "a control character"), ("a\x7fb", "a control character"), + ("a\x9fb", "a control character"), +] + + +@pytest.mark.parametrize("value", WRITABLE, ids=[ascii(v) for v in WRITABLE]) +def test_text_a_file_holds_has_no_reason(value): + assert shape_materialize.unwritable_reason(value) is None + assert value.encode("utf-8").decode("utf-8") == value + + +@pytest.mark.parametrize("value, words", UNWRITABLE, + ids=[ascii(v) for v, _ in UNWRITABLE]) +def test_what_no_file_holds_is_named(value, words): + reason = shape_materialize.unwritable_reason(value) + assert reason is not None and words in reason, reason + assert reason.isprintable() and reason.isascii() + + +def test_the_question_is_one_printable_line_less_cf(): + """Every code point, alone: refused exactly when `one_printable_line` + refuses it and it is not `Cf`.""" + reason = shape_materialize.unwritable_reason + printable = shape_materialize.one_printable_line + wrong = [hex(code) for code in range(sys.maxunicode + 1) + if (reason(chr(code)) is None) + != (printable(chr(code)) + or unicodedata.category(chr(code)) == "Cf")] + assert not wrong, wrong[:20] From 7c3c0205283f3b24f974eb8703ff2f69da4e5f99 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 00:16:37 +0000 Subject: [PATCH 2/8] adopt-project.py: a bidi embedding, override or isolate control in a value `execute` writes is refused before anything exists, and the clone-URL refusal's remediation moves the source and plans again, so no README line is shown reversed and following the remediation adopts instead of raising (#255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix round 1 on PR #270, answering lane openRepoShape-2's review at aed9190 (F1, F2, F3, and N1's test point). F1. `unwritable_reason` was `one_printable_line` less all of `Cf`, so U+202A..U+202E and U+2066..U+2069 passed `plan`, `check` and `execute` and went raw into `project.yaml`, the README's `Elected by` line and the split commit's message; a viewer showed the rest of that line reversed. Issue #255's question, `one_printable_line`, refuses them. The question is now `one_printable_line` less `Cf` other than those nine (`BIDI_CONTROLS`, by code point), plus the empty value. The joiners U+200C and U+200D, U+00AD, U+FEFF and the marks U+200E, U+200F and U+061C stay writable: a Persian name needs U+200C, and a mark sets no span's direction. `plan` refuses such a flag as `adopt-unwritable-value`, `check` finds such a plan value as `plan-unwritable-value`, `execute` refuses it before any leg exists, and a local clone URL holding one is `adopt-unwritable-value`. F2. The test file held a raw U+202E, which SonarCloud's text:S6389 flags, and other invisible literals a reader cannot see. Each is now an escape; only `é` and `日本` stay literal. The two composite assertions SonarCloud's python:S9073 flagged are four, each with a message. F3. The clone-URL refusal said to clone the source to a UTF-8 path and pass it to `execute` with --source. Followed, that raised: the clone's `origin` is the old path, and `_remote_repository` reads it as UTF-8. The remediation now says to rename or move the source and run `plan --source `, `check` and `execute` again, and the test follows it to `adoption verified`, for a byte that is not UTF-8 and for a bidi override in the directory's name. N1. The same-sentence test now asserts both exit codes and that no run raised. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 28 +++-- scripts/shape_materialize.py | 43 +++++-- tests/test_adopt_template_values.py | 169 +++++++++++++++++++++------- 3 files changed, 176 insertions(+), 64 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index d505b2c2..2bb8e161 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -2882,7 +2882,8 @@ def cmd_execute(args) -> int: # So `plan` and `check` passed `--elected-by b'Jos\xe9'`, and `execute` # created and pushed both legs before `write_lf` raised `UnicodeEncodeError`. # Before #247, `plan` raised on it and nothing was made. A line break or a -# NUL in the plan went on into the manifest. +# NUL in the plan went on into the manifest, and a bidi override or isolate +# went into the README, which then showed the rest of its line reversed. # # So `unwritable_reason` is asked of each such value before anything exists. # `plan` refuses `--elected-by` and `--reference` as @@ -2919,9 +2920,11 @@ def cmd_execute(args) -> int: WRITTEN_AS_TEXT = ( "`execute` writes each one as it stands, into the assembly root's files " "or the split commit's message, so it must be one line of UTF-8 text. A " - "byte that is not UTF-8, or a lone surrogate, cannot be written at all, " - "and a line break or a control character would be written raw into a " - "value that is read back as one line. A byte that is not UTF-8 reaches a " + "byte that is not UTF-8, or a lone surrogate, cannot be written at all; " + "a line break or a control character would be written raw into a value " + "that is read back as one line; and a bidirectional embedding, override " + "or isolate control would show the rest of the line it is written into " + "in another order than it is written. A byte that is not UTF-8 reaches a " "command line from a terminal or a script whose encoding is not the " "locale's.") @@ -2978,7 +2981,13 @@ def _refuse_an_unwritable_clone_url(url: str) -> None: Only a local one can be refused: under `--local-remote-dir` the URL is the source's own path, which the assembly root's `README.md` names (and a seeded leg's), so a source directory whose name is not UTF-8 pushed - both legs and then raised in `write_lf` (#255). + both legs and then raised in `write_lf` (#255), and one whose name holds + a bidi override was written into the README raw. + + THE REMEDIATION MOVES THE SOURCE AND PLANS AGAIN. A clone of it at a + path that is text, passed to `execute` with --source, still has the old + path as its `origin`, and `_remote_repository` reads that as UTF-8 and + raises; renaming the directory and running `plan` again adopts. """ reason = unwritable_reason(url) if reason: @@ -2987,10 +2996,11 @@ def _refuse_an_unwritable_clone_url(url: str) -> None: "under --local-remote-dir the assembly root's clone URL is the " f"source's own path, {url!r}, which `execute` writes into its " f"`README.md`, and {reason}", - "Remediation: nothing was created. Clone the source to a path " - "that is one line of UTF-8 text, and pass that path to `execute` " - "with --source; or adopt against GitHub, without " - "--local-remote-dir, where the clone URL is " + "Remediation: nothing was created. Rename or move the source " + "repository so that its path is one line of UTF-8 text with no " + "control character in it, then run `plan --source `, `check` and `execute` again; or adopt against GitHub, " + "without --local-remote-dir, where the clone URL is " "`https://github.com//.git`.") diff --git a/scripts/shape_materialize.py b/scripts/shape_materialize.py index 8c4905b2..c00fd4c7 100644 --- a/scripts/shape_materialize.py +++ b/scripts/shape_materialize.py @@ -777,6 +777,15 @@ def is_trailer(value: str) -> bool: return bool(TRAILER_RE.fullmatch(value)) and one_printable_line(value) +#: The nine bidirectional embedding, override and isolate controls, by code +#: point: U+202A..U+202E (LRE, RLE, PDF, LRO, RLO) and U+2066..U+2069 (LRI, +#: RLI, FSI, PDI). Each is `Cf`, and each sets the direction of the text +#: after it, so a line that holds one is shown in another order than it is +#: written. The marks U+200E, U+200F and U+061C are not among them: a mark +#: is one character of one direction, and sets no span's. +BIDI_CONTROLS = frozenset((*range(0x202A, 0x202F), *range(0x2066, 0x206A))) + + def unwritable_reason(value: str) -> str | None: r"""Why `value` is not one line of text a file can hold, in words; None when it is (#255). @@ -790,18 +799,24 @@ def unwritable_reason(value: str) -> str | None: `write_lf` raised `UnicodeEncodeError` after both legs were pushed. This is the question asked of every such value before anything exists. - `one_printable_line` LESS `Cf`, AND THE EMPTY VALUE, because both are - text a file holds and both were adopted before #247 and since. A `Cf` - character is invisible, not unwritable: UTF-8 encodes it, the reader - reads it back, and a name may need one -- a Persian name joins its parts - with U+200C ZERO WIDTH NON-JOINER, which `one_printable_line` refuses. - An empty `elected_by` is what `plan` records when `git config user.name` - names nobody, and the manifest holds it as `""`. What is left is - refused: a lone surrogate, which UTF-8 cannot encode; a line break, - which ends the line the manifest's reader reads the value from (CR, - U+0085 and U+2028 among them, as `str.splitlines` ends lines); and any - other control character, NUL among them, which YAML does not allow raw - (TAB aside, and no name, date or reference holds one either). + `one_printable_line` LESS `Cf` OTHER THAN THE NINE `BIDI_CONTROLS`, AND + THE EMPTY VALUE, because both are text a file holds and both were + adopted before #247 and since. Such a `Cf` character is invisible, not + unwritable: UTF-8 encodes it, the reader reads it back, and a name may + need one -- a Persian name joins its parts with U+200C ZERO WIDTH + NON-JOINER, which `one_printable_line` refuses. An empty `elected_by` is + what `plan` records when `git config user.name` names nobody, and the + manifest holds it as `""`. What is left is refused: a lone surrogate, + which UTF-8 cannot encode; a line break, which ends the line the + manifest's reader reads the value from (CR, U+0085 and U+2028 among + them, as `str.splitlines` ends lines); a bidi embedding, override or + isolate control, which a file holds but which shows the rest of the line + it is written into -- the README's `Elected by` line, its date and + reference among it -- in another order than it is written, as + `double_quoted` in `scripts/repo_shape.py` spells one in the plan for + the same reason; and any other control character, NUL among them, which + YAML does not allow raw (TAB aside, and no name, date or reference holds + one either). """ if value.splitlines() not in ([], [value]): return "it holds a line break, and it is written as one line" @@ -811,6 +826,10 @@ def unwritable_reason(value: str) -> str | None: return ("it holds a byte that is not UTF-8" if "\udc80" <= char <= "\udcff" else "it holds a lone surrogate, which is no character") + if ord(char) in BIDI_CONTROLS: + return ("it holds a bidirectional control (an embedding, " + "override or isolate), which shows the rest of the line " + "in another order than it is written") if category in UNPRINTABLE_CATEGORIES and category != "Cf": return "it holds a control character" return None diff --git a/tests/test_adopt_template_values.py b/tests/test_adopt_template_values.py index 1fd177e3..3c5f6d32 100644 --- a/tests/test_adopt_template_values.py +++ b/tests/test_adopt_template_values.py @@ -21,14 +21,19 @@ `local_path` that is not text is `source-unresolvable`. Every run is read for a traceback, and asked afterwards whether a leg remote exists. -THE QUESTION IS `one_printable_line` LESS `Cf`. A Persian name joins its -parts with U+200C ZERO WIDTH NON-JOINER, which UTF-8 holds and the reader -reads back, and `execute` adopted it before #247 and since: it still adopts, -as `José` and a source directory `café` do. +THE QUESTION IS `one_printable_line` LESS `Cf`, EXCEPT THE NINE BIDI +CONTROLS. A Persian name joins its parts with U+200C ZERO WIDTH NON-JOINER, +which UTF-8 holds and the reader reads back, and `execute` adopted it before +#247 and since: it still adopts, as `José` and a source directory `café` do. +The embedding, override and isolate controls U+202A..U+202E and +U+2066..U+2069 are refused: written raw into `README.md`, one shows the rest +of its line in another order than it is written. Every subprocess runs under `PYTHONIOENCODING=utf-8:strict`, so that a value printed raw raises on every platform as it does on a strict terminal, and is -answered in BYTES: a value that is not UTF-8 is not text to decode. +answered in BYTES: a value that is not UTF-8 is not text to decode. Every +invisible character in this file is spelled as an escape: a reader cannot +see one typed raw, and a raw bidi control reorders the line it is on. """ from __future__ import annotations @@ -164,13 +169,16 @@ def in_the_split(source: Path, path: str) -> str: #: `(flag, value)`, as `plan`'s argv carries it. `\udce9` is how Python keeps #: the byte 0xE9 of a command line that is not UTF-8, which is what a -#: terminal or a script whose encoding is not the locale's sends; the other -#: two are text a file holds, but on no one line. +#: terminal or a script whose encoding is not the locale's sends; the next +#: two are text a file holds, but on no one line; the last two hold a bidi +#: override or isolate, which a file holds and a reader is shown reordered. FLAGS = [ ("--elected-by", "Jos\udce9"), ("--reference", "ref-\udce9"), - ("--elected-by", "Jos
e"), + ("--elected-by", "Jos\u2028e"), ("--reference", "openxFactory\tdocs/project-repo-schema.md"), + ("--elected-by", "\u202eevil"), + ("--reference", "ref \u2066x\u2069 y"), ] @@ -180,8 +188,9 @@ def in_the_split(source: Path, path: str) -> str: def test_plan_refuses_a_flag_no_file_can_hold(tmp_path, flag, value): """The issue's first two rows. On main `plan` exited 0, `check` said `plan ok`, and `execute` raised in `write_lf` after both legs were - pushed (a surrogate), or adopted with the line break or the tab in the - manifest. Now `plan` refuses, and writes no plan.""" + pushed (a surrogate), or adopted with the line break, the tab or the + bidi control in the manifest and the README. Now `plan` refuses, and + writes no plan.""" source = make_source_repo(tmp_path / "Thing") runs = plan_check_execute(tmp_path, source, flag, value) assert_no_traceback_and_no_leg(tmp_path, runs) @@ -195,35 +204,84 @@ def test_plan_refuses_a_flag_no_file_can_hold(tmp_path, flag, value): # --- `execute`: a local clone URL no file can hold --------------------------- +#: What the clone-URL refusal tells the person to do. `refused_then_moved` +#: does it, as it is worded. +MOVE_REMEDIATION = ( + b"Remediation: nothing was created. Rename or move the source repository " + b"so that its path is one line of UTF-8 text with no control character " + b"in it, then run `plan --source `, `check` and `execute` " + b"again") + + +def refused_then_moved(base: Path, parent: Path, reason: bytes) -> None: + """A source in the directory `parent`: `plan` and `check` pass it, as + #247 made them, and `execute` refuses its path as the assembly root's + clone URL, giving `reason`, before any leg exists. Then the remediation + the refusal gives, followed as it is worded -- `parent` renamed, then + `plan --source `, `check` and `execute` run again -- ends + `adoption verified`.""" + try: + source = make_source_repo(parent / "Thing") + except AssertionError as exc: + pytest.skip(f"git will not work under {parent.name!r} here: {exc}") + runs = plan_check_execute(base, source) + assert_no_traceback_and_no_leg(base, runs) + assert runs["plan"].returncode == 0, said(runs["plan"]) + assert runs["check"].returncode == 0, said(runs["check"]) + assert PLAN_OK in runs["check"].stdout, said(runs["check"]) + refused = runs["execute"] + assert refused.returncode == 2, said(refused) + assert ADOPT_REFUSED + b"under --local-remote-dir the assembly root's " \ + b"clone URL is the source's own path, " \ + + repr(str(source.resolve())).encode() + b", which " \ + in refused.stderr, said(refused) + assert reason in refused.stderr, said(refused) + assert MOVE_REMEDIATION in refused.stderr, said(refused) + + moved = parent.rename(parent.with_name("cafe")) / "Thing" + again = base / "again" + again.mkdir() + runs = plan_check_execute(again, moved) + assert_no_traceback(runs) + assert runs["check"].returncode == 0, said(runs["check"]) + done = runs["execute"] + assert done.returncode == 0, said(done) + assert VERIFIED in done.stdout, said(done) + + @needs_filter_repo def test_execute_refuses_a_local_source_directory_that_is_not_utf8(tmp_path): """The issue's third row. Under `--local-remote-dir` the assembly root's clone URL is the source's own path, which its `README.md` names: `plan` - and `check` pass such a directory, as #247 made them, and `execute` - refuses it before any leg exists, where it raised after both were - pushed. Skipped where no directory can be named with the byte 0xE9 - (Windows, macOS's APFS) or git will not work in one.""" + and `check` pass such a directory, and `execute` refuses it before any + leg exists, where it raised after both were pushed. Its remediation + once said to clone the source to a UTF-8 path and pass that with + --source, which ended in a traceback: the clone's `origin` is the old + path, which `_remote_repository` reads as UTF-8. Skipped where no + directory can be named with the byte 0xE9 (Windows, macOS's APFS) or + git will not work in one.""" try: parent = Path(os.fsdecode(os.path.join(os.fsencode(tmp_path), b"caf\xe9"))) parent.mkdir() except (OSError, UnicodeError) as exc: pytest.skip(f"no directory here is named b'caf\\xe9': {exc!r}") + refused_then_moved(tmp_path, parent, b"a byte that is not UTF-8") + + +@needs_filter_repo +def test_execute_refuses_a_local_source_directory_holding_a_bidi_control( + tmp_path): + """A source under a directory whose name begins with U+202E RIGHT-TO-LEFT + OVERRIDE: on main `execute` adopted it, and the shape's README showed + the rest of its `git clone` line reversed. Now it is refused as the + byte is, and moving the source adopts.""" + parent = tmp_path / "\u202ecaf" try: - source = make_source_repo(parent / "Thing") - except AssertionError as exc: - pytest.skip(f"git will not work under b'caf\\xe9' here: {exc}") - runs = plan_check_execute(tmp_path, source) - assert_no_traceback_and_no_leg(tmp_path, runs) - assert runs["plan"].returncode == 0, said(runs["plan"]) - assert runs["check"].returncode == 0, said(runs["check"]) - assert PLAN_OK in runs["check"].stdout, said(runs["check"]) - refused = runs["execute"] - assert refused.returncode == 2, said(refused) - assert ADOPT_REFUSED + b"under --local-remote-dir the assembly root's " \ - b"clone URL is the source's own path, " \ - + repr(str(source.resolve())).encode() in refused.stderr, said(refused) - assert b"nothing was created" in refused.stderr + parent.mkdir() + except OSError as exc: + pytest.skip(f"no directory here is named '\\u202ecaf': {exc!r}") + refused_then_moved(tmp_path, parent, b"a bidirectional control") # --- `check` and `execute`: a plan value edited by hand ---------------------- @@ -284,7 +342,7 @@ def edited_plan(base: Path, answered: tuple[Path, str], line: str, ("elected_by-nul", "elected_by", "T\x00", "elected_by"), ("elected_by-cr", "elected_by", "T\rX", "elected_by"), ("elected_by-nel", "elected_by", "T\x85X", "elected_by"), - ("elected_by-u2028", "elected_by", "T
X", "elected_by"), + ("elected_by-u2028", "elected_by", "T\u2028X", "elected_by"), ("elected_by-tab", "elected_by", "T\tX", "elected_by"), ("elected_on-surrogate", "elected_on", "2026-09-02\ud800", "elected_on"), ("reference-surrogate", "reference", "x\ud800", "reference"), @@ -293,6 +351,10 @@ def edited_plan(base: Path, answered: tuple[Path, str], line: str, ("pins-surrogate", "pins", "openX\ud800", "a `pins:` item"), ("follow_ups-surrogate", "follow_ups", "fix x\ud800", "a `follow_ups:` item"), + ("elected_by-override", "elected_by", "\u202eevil", "elected_by"), + ("visibility-override", "visibility", "private\u202e", "visibility"), + ("follow_ups-isolate", "follow_ups", "fix \u2066x", + "a `follow_ups:` item"), ] @@ -315,9 +377,10 @@ def test_check_finds_and_execute_refuses_a_hand_edited_value( tmp_path, answered, key, value, what): """On main `check` said `plan ok` (or raised printing the value), and `execute` raised in `write_lf` after both legs were pushed, or adopted - with the NUL or the line break in the manifest. Now `check` has a - FINDING, and `execute` refuses it through `_refuse_what_check_finds`, in - `check`'s words, before any leg exists.""" + with the NUL, the line break or the bidi control in the manifest or the + split commit's message. Now `check` has a FINDING, and `execute` refuses + it through `_refuse_what_check_finds`, in `check`'s words, before any + leg exists.""" edit(tmp_path, answered, key, value) runs = {"check": checked(tmp_path), "execute": executed(tmp_path)} assert_no_traceback_and_no_leg(tmp_path, runs) @@ -343,6 +406,9 @@ def test_plan_and_check_say_the_same_sentence(tmp_path, answered): refused = planned(flagged, answered[0], "--elected-by", value) edit(by_hand, answered, "elected_by", value) found = checked(by_hand) + assert_no_traceback({"plan": refused, "check": found}) + assert refused.returncode == 2, said(refused) + assert found.returncode == 1, said(found) assert ADOPT_REFUSED in refused.stderr, said(refused) assert FINDING in found.stderr, said(found) detail = refused.stderr.split(ADOPT_REFUSED, 1)[1].split(b"\n", 1)[0] @@ -424,7 +490,7 @@ def test_a_local_path_that_is_not_text_is_unresolvable(tmp_path, answered, # --- controls: valid text still adopts, as on main --------------------------- @needs_filter_repo -@pytest.mark.parametrize("elected_by", ["José", "Ali‌Reza"], +@pytest.mark.parametrize("elected_by", ["José", "Ali\u200cReza"], ids=["accented", "zero-width-non-joiner"]) def test_a_valid_non_ascii_elector_still_adopts(tmp_path, elected_by): """`José`, and a name joined with U+200C, which `one_printable_line` @@ -460,10 +526,17 @@ def test_a_utf8_source_directory_still_adopts_with_local_remotes(tmp_path): #: Text a file holds: the empty value `plan` records when `git config #: user.name` names nobody, a name, a date, the default reference, a literal -#: backslash, invisible `Cf` characters, and a space that is not ASCII's. -WRITABLE = ["", "José", "Ali‌Reza", "2026-09-02", +#: backslash, a space that is not ASCII's, and the invisible `Cf` characters +#: that are not bidi controls -- the joiners U+200C and U+200D, a byte-order +#: mark, a soft hyphen, and the marks U+200E, U+200F and U+061C, which set +#: no span's direction. +WRITABLE = ["", "José", "Ali\u200cReza", "2026-09-02", "openxFactory docs/project-repo-schema.md", "CORP\\rsmith", - "日本", "a b", "‮evil", "bom", "a­b"] + "日本", "a\u00a0b", "\ufeffbom", "a\u00adb", "a\u200db", + "a\u200eb", "a\u200fb", "a\u061cb"] + +#: The nine bidi embedding, override and isolate controls, by code point. +BIDI_CONTROLS = [*range(0x202A, 0x202F), *range(0x2066, 0x206A)] #: `(value, the words its reason holds)`. UNWRITABLE = [ @@ -474,10 +547,14 @@ def test_a_utf8_source_directory_still_adopts_with_local_remotes(tmp_path): ("a\r\nb", "a line break"), ("end\n", "a line break"), ("a\x0bb", "a line break"), ("a\x0cb", "a line break"), ("a\x1cb", "a line break"), ("a\x85b", "a line break"), - ("a
b", "a line break"), ("a
b", "a line break"), + ("a\u2028b", "a line break"), ("a\u2029b", "a line break"), ("a\x00b", "a control character"), ("a\tb", "a control character"), ("a\x1bb", "a control character"), ("a\x7fb", "a control character"), ("a\x9fb", "a control character"), + ("\u202eevil", "a bidirectional control"), + ("\u202a", "a bidirectional control"), + ("\u2066x\u2069", "a bidirectional control"), + ("\u2069", "a bidirectional control"), ] @@ -491,17 +568,23 @@ def test_text_a_file_holds_has_no_reason(value): ids=[ascii(v) for v, _ in UNWRITABLE]) def test_what_no_file_holds_is_named(value, words): reason = shape_materialize.unwritable_reason(value) - assert reason is not None and words in reason, reason - assert reason.isprintable() and reason.isascii() + assert reason is not None, ascii(value) + assert words in reason, reason + assert reason.isprintable(), reason + assert reason.isascii(), reason -def test_the_question_is_one_printable_line_less_cf(): +def test_the_question_is_one_printable_line_less_cf_but_the_bidi_controls(): """Every code point, alone: refused exactly when `one_printable_line` - refuses it and it is not `Cf`.""" + refuses it and it is not `Cf`, or when it is one of the nine bidi + controls, which are `Cf` and still refused.""" reason = shape_materialize.unwritable_reason printable = shape_materialize.one_printable_line + assert {unicodedata.category(chr(code)) + for code in BIDI_CONTROLS} == {"Cf"} wrong = [hex(code) for code in range(sys.maxunicode + 1) if (reason(chr(code)) is None) - != (printable(chr(code)) - or unicodedata.category(chr(code)) == "Cf")] + != (code not in BIDI_CONTROLS + and (printable(chr(code)) + or unicodedata.category(chr(code)) == "Cf"))] assert not wrong, wrong[:20] From 3aa0e5d5dcce2493637e7953013fff1056256d84 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 00:16:53 +0000 Subject: [PATCH 3/8] adopt-project.py: `plan` names an elector it read from `git config user.name` as `--elected-by (not given, so `git config user.name`)` when it refuses it, so the refusal no longer names a flag the person never passed (#255) Fix round 1 on PR #270, N2 of lane openRepoShape-2's review at aed9190. With no --elected-by, `plan` records `git config user.name`. A TAB there was refused as `--elected-by is 'Jo\tSmith'`, naming a flag the operator never passed. It is now named as #181's `FLAG_DEFAULTS` names a value `plan` filled in, and the remediation says to pass the flag or correct `git config user.name`. A test runs `plan` under `GIT_CONFIG_GLOBAL` holding a TAB name, and skips where a repository's own `user.name` answers first. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 11 ++++++--- tests/test_adopt_template_values.py | 37 +++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 2bb8e161..4d733007 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -1244,19 +1244,24 @@ def _checked_plan_inputs(args, source: Source, # manifest field instead of as a question. election_date(args.elected_on) args.reference = args.reference or default_reference(args.elected_on) + # A value `plan` filled in by itself is named by where it came from, as + # #181 names one, and not as a flag the person never passed. + elector = ("--elected-by" if args.elected_by else + "--elected-by (not given, so `git config user.name`)") args.elected_by = args.elected_by or _elector() # Before the plan is written, in the words `check` would use (#255): a # value `execute` writes into a file must be text one can hold. The # date has had its answer above, `election_date`. unwritable = unwritable_values( - (("--elected-by", args.elected_by, ROOT_MANIFEST_AND_README), + ((elector, args.elected_by, ROOT_MANIFEST_AND_README), ("--reference", args.reference, ROOT_MANIFEST_AND_README))) if unwritable: raise Refusal("adopt-unwritable-value", " ".join(f"{problem}." for problem in unwritable), "Remediation: no plan was written. Pass each value " - "named again, retyped (`--elected-by` defaults to " - "`git config user.name`). " + WRITTEN_AS_TEXT) + "named again, retyped; when `--elected-by` is not " + "given, `plan` reads `git config user.name`, so pass " + "the flag or correct that. " + WRITTEN_AS_TEXT) names = _names(args.project) pins = [_checked_pin_name(p) for p in (args.pin or []) if p.strip()] _check_names(naming, names, set(pins)) diff --git a/tests/test_adopt_template_values.py b/tests/test_adopt_template_values.py index 3c5f6d32..6dc4442b 100644 --- a/tests/test_adopt_template_values.py +++ b/tests/test_adopt_template_values.py @@ -86,11 +86,14 @@ reason="git filter-repo is not installed: `pip install git-filter-repo`") -def adopted(*args: str) -> subprocess.CompletedProcess: - """`adopt-project.py ` on a strict stdout, answered in bytes, each - stream with LF line ends whatever the platform printed.""" +def adopted(*args: str, + extra: dict | None = None) -> subprocess.CompletedProcess: + """`adopt-project.py ` on a strict stdout, with `extra` in its + environment, answered in bytes, each stream with LF line ends whatever + the platform printed.""" env = blank_unnamed_pin_sources( - {**IDENTITY, **os.environ, "LANES_LANE": "", **STRICT}, STRICT) + {**IDENTITY, **os.environ, "LANES_LANE": "", **(extra or {}), + **STRICT}, STRICT) done = subprocess.run([sys.executable, str(ADOPT), *args], capture_output=True, env=env, stdin=subprocess.DEVNULL, check=False) @@ -202,6 +205,32 @@ def test_plan_refuses_a_flag_no_file_can_hold(tmp_path, flag, value): assert not (tmp_path / PLAN_FILE).exists() +def test_plan_names_the_elector_it_took_from_git_config(tmp_path): + """With no --elected-by, `plan` records `git config user.name`. A TAB + there is refused like a TAB passed with the flag, and named by where + it came from, as #181 names a value `plan` filled in: not as a flag + the person never passed.""" + config = tmp_path / "gitconfig" + config.write_bytes(b'[user]\n\tname = "Jo\\tSmith"\n') + extra = {"GIT_CONFIG_GLOBAL": str(config)} + read = subprocess.run(["git", "config", "user.name"], cwd=str(REPO), + capture_output=True, env={**os.environ, **extra}, + check=False) + if read.stdout.rstrip(b"\r\n") != b"Jo\tSmith": + pytest.skip("`git config user.name` here is not read from " + f"GIT_CONFIG_GLOBAL: {read.stdout!r}") + source = make_source_repo(tmp_path / "Thing") + refused = adopted("plan", "--source", str(source), "--project", PROJECT, + "--org", "testorg", "--elected-on", "2026-09-02", + "--out", str(tmp_path / PLAN_FILE), "--work-dir", + str(tmp_path / WORK), extra=extra) + assert_no_traceback({"plan": refused}) + assert refused.returncode == 2, said(refused) + assert ADOPT_REFUSED + b"--elected-by (not given, so `git config " \ + b"user.name`) is 'Jo\\tSmith', which " in refused.stderr, said(refused) + assert not (tmp_path / PLAN_FILE).exists() + + # --- `execute`: a local clone URL no file can hold --------------------------- #: What the clone-URL refusal tells the person to do. `refused_then_moved` From 7e9cdc3a698a1e0ffba2d051daa5e3bf03b7755c Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 00:34:44 +0000 Subject: [PATCH 4/8] tests/test_adopt_template_values.py: the valid-elector control reads the manifest back through the reader instead of matching its line, so it holds on main, which since #250 writes a YAML template's value as `double_quoted` spells it (#255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix round 1 on PR #270. Built on the merge with main be2790e, the U+200C control failed: `execute` adopted, but main b30595c (#250) renders a value between a YAML template's double quotes through `double_quoted`, so `project.yaml` holds `"Ali‌Reza"` with the character spelled as an escape, and the test looked for it raw. The reader gives back U+200C either way, so the control now asks the reader, and also asks for the name in the shape's README, which is not YAML and holds it as it stands on this branch and on main. GitHub's checks run on the pull request's merge with main, so they would have failed here. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- tests/test_adopt_template_values.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/test_adopt_template_values.py b/tests/test_adopt_template_values.py index 6dc4442b..0150c0c7 100644 --- a/tests/test_adopt_template_values.py +++ b/tests/test_adopt_template_values.py @@ -523,7 +523,12 @@ def test_a_local_path_that_is_not_text_is_unresolvable(tmp_path, answered, ids=["accented", "zero-width-non-joiner"]) def test_a_valid_non_ascii_elector_still_adopts(tmp_path, elected_by): """`José`, and a name joined with U+200C, which `one_printable_line` - refuses and a file holds: adopted, and written into the manifest.""" + refuses and a file holds: adopted, read back from the manifest as + itself, and named in the shape's README. + + The manifest is asked through the reader, not for a line: since #250 a + YAML template's value is written as `double_quoted` spells it, so the + line holds U+200C as an escape there, and as it stands before.""" source = make_source_repo(tmp_path / "Thing") runs = plan_check_execute(tmp_path, source, "--elected-by", elected_by) assert_no_traceback(runs) @@ -531,8 +536,11 @@ def test_a_valid_non_ascii_elector_still_adopts(tmp_path, elected_by): done = runs["execute"] assert done.returncode == 0, said(done) assert VERIFIED in done.stdout, said(done) - assert f'\nelected_by: "{elected_by}"\n' in in_the_split(source, - "project.yaml") + manifest = tmp_path / "project.yaml" + manifest.write_bytes(in_the_split(source, "project.yaml").encode("utf-8")) + assert load_yaml(manifest)["elected_by"] == elected_by + assert f"Elected by {elected_by} on 2026-09-02" in in_the_split( + source, "shape/README.md") @needs_filter_repo From 8e09208fd86ee866cdac37c3df62093032e6459b Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:13:28 +0000 Subject: [PATCH 5/8] adopt-project.py: a `--source` that begins with `-` is `source-unresolvable` before anything runs, so `gh repo clone` is no longer handed it as its first argument, where gh's own parser read it as one of its options (#255) SonarCloud's S8705 on #270 asked whether a value reaches a command as an option. The lane's re-verify found one that does: `plan --source=-u/x` is no path that exists and holds a `/`, so `Source.open` took it for an `org/repo` name and ran `gh repo clone -u/x -- --quiet`. gh read `-u` as `--upstream-remote-name` with the value `/x`, and `--source=--upload-pack=x/y` as a flag it does not know. A hand-edited `local_path:` or `repository:` reaches the same call from `check` and `execute`. It cannot reach git as an option (gh hands git only what follows its `--`), and main does the same. The guard sits in the condition this PR already edits. No GitHub owner name begins with `-`, so nothing gh could clone is refused, and an existing path that begins with `-` is still taken as a path. The tests put a stub `gh` first on PATH that records each call: `plan` with either value, and `check` and `execute` on a plan whose `local_path:` holds it, each exit 2 `source-unresolvable` with no traceback and no leg, and the stub records nothing. Against 7e9cdc3's code all four fail with the stub's record of `repo clone -u/x ...` and `repo clone --upload-pack=x/y ...`. A control shows the stub records the clone of `testorg/Thing`. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 7 +- tests/test_adopt_template_values.py | 103 ++++++++++++++++++++++++++-- 2 files changed, 104 insertions(+), 6 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 4d733007..653b9bc9 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -399,7 +399,7 @@ def open(cls, spec: str, work_root: Path) -> "Source": repository = _remote_repository(path) else: if ("/" not in spec or WINDOWS_ABSOLUTE_RE.match(spec) - or unwritable_reason(spec)): + or unwritable_reason(spec) or spec.startswith("-")): # A DRIVE-LETTER PATH IS A PATH THAT IS NOT THERE, never an # `org/repo`. `D:/work/Thing` carries a `/` and would # otherwise be handed to `gh repo clone` as an owner named @@ -412,6 +412,11 @@ def open(cls, spec: str, work_root: Path) -> "Source": # cannot exist, which reached `print` (a surrogate raised on # a strict stdout) or `subprocess` (a NUL raised there) as a # traceback, where main had refused it. + # NOR IS A NAME THAT BEGINS WITH `-`. It is the first + # argument `gh repo clone` is given, and gh's own parser + # reads it as an option: `-u/x` set `--upstream-remote-name` + # to `/x`. No GitHub owner name begins with `-`, so nothing + # gh could clone is refused here. raise Refusal( "source-unresolvable", f"--source {spec!r} is neither a path that exists nor an " diff --git a/tests/test_adopt_template_values.py b/tests/test_adopt_template_values.py index 0150c0c7..251bfff3 100644 --- a/tests/test_adopt_template_values.py +++ b/tests/test_adopt_template_values.py @@ -41,6 +41,7 @@ import os import re import shutil +import stat import subprocess import sys import unicodedata @@ -48,7 +49,7 @@ import pytest -from conftest import (ADOPT, REPO, blank_unnamed_pin_sources, +from conftest import (ADOPT, REPO, WINDOWS_SKIP, blank_unnamed_pin_sources, make_source_repo, resolve) sys.path.insert(0, str(REPO / "scripts")) @@ -113,15 +114,17 @@ def planned(base: Path, source: Path, *flags) -def checked(base: Path) -> subprocess.CompletedProcess: +def checked(base: Path, + extra: dict | None = None) -> subprocess.CompletedProcess: return adopted("check", "--plan", str(base / PLAN_FILE), "--work-dir", - str(base / WORK)) + str(base / WORK), extra=extra) -def executed(base: Path) -> subprocess.CompletedProcess: +def executed(base: Path, + extra: dict | None = None) -> subprocess.CompletedProcess: return adopted("execute", "--plan", str(base / PLAN_FILE), "--yes", "--local-remote-dir", str(base / REMOTES), "--work-dir", - str(base / WORK)) + str(base / WORK), extra=extra) def answer(base: Path) -> None: @@ -516,6 +519,96 @@ def test_a_local_path_that_is_not_text_is_unresolvable(tmp_path, answered, in run.stderr, f"{name}:\n{said(run)}" +# --- `--source`: a name `gh` would read as one of its options ---------------- + +#: `--source` values that begin with `-`. Neither is a path that exists, and +#: each holds a `/`, so `Source.open` took it for an `org/repo` name and ran +#: `gh repo clone -- --quiet`, where gh's own parser read it +#: as an option: `-u` is `--upstream-remote-name`, and `--upload-pack=x/y` +#: is a flag gh does not know. No GitHub owner name begins with `-`. +OPTION_SOURCES = ["-u/x", "--upload-pack=x/y"] + + +def gh_stub(base: Path) -> tuple[dict, Path]: + """The environment that puts a `gh` first on PATH, and the file it + records each call's arguments in, one to a line. It refuses every call. + It is a `#!` script, which Windows does not execute: there nothing is + recorded, and the refusal's id alone says that `gh` did not run.""" + binary = base / "bin" + binary.mkdir() + script = binary / "gh" + script.write_bytes(b"#!/bin/sh\n" + b"printf '%s\\n' \"$@\" >> \"$GH_STUB_LOG\"\n" + b"echo 'stub gh: refusing' >&2\n" + b"exit 1\n") + script.chmod(script.stat().st_mode | stat.S_IEXEC) + log = base / "gh.log" + return ({"PATH": f"{binary}{os.pathsep}{os.environ.get('PATH', '')}", + "GH_STUB_LOG": str(log)}, log) + + +def gh_calls(log: Path) -> str: + """What the stub was given, readable in an assertion message.""" + return log.read_text(encoding="utf-8") if log.exists() else "" + + +@pytest.mark.parametrize("spec", OPTION_SOURCES, ids=OPTION_SOURCES) +def test_plan_refuses_a_source_gh_would_read_as_an_option(tmp_path, spec): + """On main `plan` handed `spec` to `gh repo clone` as its first + argument (SonarCloud's S8705 asked the question). Now it is + `source-unresolvable` before anything runs: no path that exists, and no + `org/repo` name.""" + stub, log = gh_stub(tmp_path) + refused = adopted("plan", f"--source={spec}", "--project", PROJECT, + "--org", "testorg", "--elected-by", "Test Human", + "--elected-on", "2026-09-02", "--out", + str(tmp_path / PLAN_FILE), "--work-dir", + str(tmp_path / WORK), extra=stub) + assert not log.exists(), f"gh was run with:\n{gh_calls(log)}" + assert_no_traceback({"plan": refused}) + assert refused.returncode == 2, said(refused) + assert SOURCE_REFUSED + f"--source {spec!r} is neither".encode() \ + in refused.stderr, said(refused) + assert not (tmp_path / PLAN_FILE).exists() + + +@pytest.mark.parametrize("spec", OPTION_SOURCES, ids=OPTION_SOURCES) +def test_check_and_execute_refuse_a_hand_edited_source_gh_would_read( + tmp_path, answered, spec): + """The plan file is edited between `plan` and `execute`, on purpose, and + its `local_path:` reaches the same `Source.open`. On main `check` and + `execute` each ran `gh repo clone` with it first.""" + edited_plan(tmp_path, answered, f" local_path: {double_quoted(spec)}") + assert load_yaml(tmp_path / PLAN_FILE)["source"]["local_path"] == spec + stub, log = gh_stub(tmp_path) + runs = {"check": checked(tmp_path, stub), + "execute": executed(tmp_path, stub)} + assert not log.exists(), f"gh was run with:\n{gh_calls(log)}" + assert_no_traceback_and_no_leg(tmp_path, runs) + for name, run in runs.items(): + assert run.returncode == 2, f"{name}:\n{said(run)}" + assert SOURCE_REFUSED + f"--source {spec!r} is neither".encode() \ + in run.stderr, f"{name}:\n{said(run)}" + + +@WINDOWS_SKIP # the stub `gh` is a `#!` script; Windows executes none +def test_the_gh_stub_records_an_org_repo_source(tmp_path): + """The control for the two tests above: an `org/repo` name is handed to + `gh repo clone`, and the stub records the call it refuses. A stub that + recorded nothing would let them pass without asking anything.""" + stub, log = gh_stub(tmp_path) + refused = adopted("plan", "--source=testorg/Thing", "--project", PROJECT, + "--org", "testorg", "--elected-by", "Test Human", + "--elected-on", "2026-09-02", "--out", + str(tmp_path / PLAN_FILE), "--work-dir", + str(tmp_path / WORK), extra=stub) + assert_no_traceback({"plan": refused}) + assert refused.returncode == 2, said(refused) + assert b"REFUSED source-unclonable: " in refused.stderr, said(refused) + assert gh_calls(log).splitlines()[:3] == ["repo", "clone", + "testorg/Thing"], gh_calls(log) + + # --- controls: valid text still adopts, as on main --------------------------- @needs_filter_repo From cae1bf534c842b5290af15460e08a8de11040909 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:27:51 +0000 Subject: [PATCH 6/8] adopt-project.py: `_elector` keeps a `user.name` byte that is not UTF-8 for #255's question, `_remote_repository` reads such an origin URL as no `org/repo`, and `plan` asks `adopt-no-org` before `checked_value`, so a Latin-1 name is refused as `adopt-unwritable-value` naming `--elected-by`, and `plan`, `check` and `execute` go on past such an origin, where both ended in a UnicodeDecodeError traceback (#292) git keeps a config value and a remote URL as the bytes it was given, so a repository can hold either without anyone having done anything wrong: a name saved in a legacy code page, a clone of a directory named in one. `_elector` (`git config user.name`, asked when `--elected-by` is not given) and `_remote_repository` (`git remote get-url origin` of a local source) read both through `git_out`, which decodes strictly, and caught only `Refusal`. `UnicodeDecodeError` is a `ValueError`, so `main` did not catch it either: `plan` (and, for the URL, `check` and `execute`) ended in a traceback, exit 1, naming neither the setting nor the flag past it. `_elector` now reads the name as bytes and decodes it with `surrogateescape`, as `sys.argv` keeps such a byte. It does not carry on with it: #270's `unwritable_values` asks it, in `_checked_plan_inputs`, right after `_elector` returns, and refuses it as `adopt-unwritable-value`, named as `--elected-by (not given, so `git config user.name`)`, with the remediation to pass the flag or correct the setting, before any plan is written. `_remote_repository` returns None for a URL it cannot decode, as for a source with no origin: no `org/repo` is named by it. `plan` takes `--org` and records `repository: null`, and `check` and `execute` name the source by its path and adopt it, as does `execute --source` with a UTF-8 clone of a source whose own path is not UTF-8 (whose origin is that path). Without `--org`, such a source was to meet `adopt-no-org`. It never could: since v0.2 (#5) the `or "-"` that stood in for a missing org reached `checked_value` first, which refused `--org is '-'`, a value nobody passed. `adopt-no-org` is now asked first, for a source with no origin and for one whose origin is not UTF-8. `git_out` itself is untouched: `scripts/repo_shape.py` is a pinned copy, and a change there would move every project's shape pin. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 47 +++++- tests/test_adopt_git_reads.py | 270 ++++++++++++++++++++++++++++++++++ 2 files changed, 309 insertions(+), 8 deletions(-) create mode 100644 tests/test_adopt_git_reads.py diff --git a/adopt-project.py b/adopt-project.py index eb972407..593b9093 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -832,9 +832,21 @@ def commit_count(self) -> int: def _remote_repository(path: Path) -> str | None: + """The `org/repo` the source's `origin` URL ends in; None when it has + no origin, or one that ends in no such pair. + + NOR DOES A URL THAT IS NOT UTF-8 NAME ONE (#292). git keeps the URL as + the bytes it was given: a clone of a directory named in a code page that + is not UTF-8 has that path as its origin, and nobody did anything wrong. + Read strictly, it ended `plan`, `check` and `execute` in a + `UnicodeDecodeError` traceback in `Source.open`, before any question was + asked. Now it is no origin: `plan` takes `--org`, or refuses as + `adopt-no-org` and asks for it, and `check` and `execute` name the + source by its path. + """ try: url = git_out(["remote", "get-url", "origin"], cwd=path) - except Refusal: + except (Refusal, UnicodeDecodeError): return None match = re.search(r"[:/]([^/:]+/[^/]+?)(?:\.git)?$", url) return match.group(1) if match else None @@ -1554,12 +1566,17 @@ def _checked_plan_inputs(args, source: Source, raise Refusal("adopt-bad-id", f"--id {args.id!r} must match {PROJECT_ID_RE.pattern}", "Remediation: pass an explicit lowercase --id.") - args.org = checked_value( - "--org", args.org or (source.repository or "/").split("/")[0] or "-") + # NO ORG IS `adopt-no-org`, asked before `checked_value` (#292). The `-` + # that once stood in for a missing one reached `checked_value` first, + # which refused `--org is '-'`, a value nobody passed; this refusal was + # never reached. A source with no `origin`, or one whose URL is not + # UTF-8 (`_remote_repository`), names no org. + args.org = args.org or (source.repository or "/").split("/")[0] if not args.org: raise Refusal( "adopt-no-org", "the source has no `origin` to read an " "organisation from", "Remediation: pass --org .") + args.org = checked_value("--org", args.org) args.elected_on = args.elected_on or _dt.date.today().isoformat() # Read before the plan is written, whether or not it is what chooses the # reference: a date the tools cannot parse would reach `execute` as a @@ -3855,10 +3872,11 @@ def _refuse_an_unwritable_clone_url(url: str) -> None: both legs and then raised in `write_lf` (#255), and one whose name holds a bidi override was written into the README raw. - THE REMEDIATION MOVES THE SOURCE AND PLANS AGAIN. A clone of it at a - path that is text, passed to `execute` with --source, still has the old - path as its `origin`, and `_remote_repository` reads that as UTF-8 and - raises; renaming the directory and running `plan` again adopts. + THE REMEDIATION MOVES THE SOURCE AND PLANS AGAIN: renaming the directory + and running `plan` again adopts. A clone of it at a path that is text, + passed to `execute` with --source, has the old path as its `origin`; + `_remote_repository` read that as UTF-8 and raised until #292, and now + reads it as no origin, so that route adopts too. """ reason = unwritable_reason(url) if reason: @@ -4418,10 +4436,23 @@ def _check_names(policy: NamingPolicy, names: dict[str, str], def _elector() -> str: + """`git config user.name`, which `plan` records when `--elected-by` is + not given; "" when it names nobody. + + A BYTE THAT IS NOT UTF-8 IS KEPT, AS `sys.argv` KEEPS ONE (#292). git + keeps the name as the bytes it was given, and one saved in a legacy code + page is no UTF-8. Read strictly, it ended `plan` in a `UnicodeDecodeError` + traceback, exit 1, naming neither the setting nor `--elected-by`. Kept + as a lone surrogate, it is asked what `_checked_plan_inputs` asks of + every value `execute` writes (#255), and is refused there as + `adopt-unwritable-value`, named as `git config user.name`, with + `--elected-by` as the way past it, before any plan is written. + """ try: - return git_out([GIT_CONFIG, "user.name"], cwd=SHAPE_ROOT) + name = git_out([GIT_CONFIG, "user.name"], cwd=SHAPE_ROOT, binary=True) except Refusal: return "" + return name.decode("utf-8", "surrogateescape").strip() def _work_root(args) -> Path: diff --git a/tests/test_adopt_git_reads.py b/tests/test_adopt_git_reads.py new file mode 100644 index 00000000..7038265d --- /dev/null +++ b/tests/test_adopt_git_reads.py @@ -0,0 +1,270 @@ +# SPDX-License-Identifier: Apache-2.0 +"""What `adopt-project.py` asks git, read as bytes it did not write (#292). + +`_elector` asks `git config user.name` when `--elected-by` is not given, and +`_remote_repository` asks `git remote get-url origin` of a local source. git +keeps both as the bytes it was given, so either can hold a byte that is not +UTF-8 without anyone having done anything wrong: a name saved in a legacy +code page, a clone of a directory named in one. Both were decoded strictly, +so `plan` (and, for the URL, `check` and `execute`) ended in a +`UnicodeDecodeError` traceback, exit 1, naming neither the git setting nor +the flag that avoids it, and nothing was created. + +Now the name is kept as a lone surrogate, as `sys.argv` keeps one, and +`plan` refuses it as #255's `adopt-unwritable-value`, named as `git config +user.name`, with `--elected-by` as the way past it, and writes no plan. A +URL that is not UTF-8 is no origin: `plan` takes `--org`, or refuses as +`adopt-no-org` -- reached at last: `checked_value` refused the `-` that stood +in for a missing org first, as `--org is '-'`, a value nobody passed -- and +`check` and `execute` adopt the source by its path. + +The runs are `test_adopt_template_values.py`'s: on a strict stdout, answered +in bytes, and each read for a traceback. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import REPO, git, make_source_repo +from test_adopt_template_values import (ADOPT_REFUSED, PLAN_FILE, PLAN_OK, + PROJECT, REMOTES, VERIFIED, WORK, + adopted, answer, + assert_no_traceback, + assert_no_traceback_and_no_leg, + checked, executed, needs_filter_repo, + planned, said) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import load_yaml # noqa: E402 + +#: `José` as a legacy code page saves it: the byte 0xE9 is Latin-1's `é`, +#: and no UTF-8. +LATIN1_NAME = b"Jos\xe9" +#: An origin URL holding that byte, as a clone of a directory named `café` +#: in Latin-1 has one; and the same URL in ASCII, which ends in an +#: `org/repo` the tool reads. +NOT_UTF8_URL = b"../caf\xe9/repo.git" +ASCII_URL = b"../cafe/repo.git" + +NO_ORG = (b"REFUSED adopt-no-org: the source has no `origin` to read an " + b"organisation from\nRemediation: pass --org .\n") + + +def by_git_config(base: Path, source: Path, extra: dict, + *flags: str) -> subprocess.CompletedProcess: + """`plan` as `planned` runs it, but with no --elected-by unless `flags` + gives one, so the elector is `git config user.name`.""" + return adopted("plan", "--source", str(source), "--project", PROJECT, + "--org", "testorg", "--elected-on", "2026-09-02", + "--out", str(base / PLAN_FILE), "--work-dir", + str(base / WORK), *flags, extra=extra) + + +def without_org(base: Path, source: Path) -> subprocess.CompletedProcess: + """`plan` as `planned` runs it, but with no --org, so the org is what + the source's `origin` names.""" + return adopted("plan", "--source", str(source), "--project", PROJECT, + "--elected-by", "Test Human", "--elected-on", "2026-09-02", + "--out", str(base / PLAN_FILE), "--work-dir", + str(base / WORK)) + + +def global_user_name(base: Path, name: bytes) -> dict: + """The environment of a person whose global `user.name` is `name`, + written as bytes: `GIT_CONFIG_GLOBAL` (git 2.32) stands in for + `~/.gitconfig` for every git the run starts. Skipped where `git config + user.name`, asked where `_elector` asks it, does not answer `name`: a + `user.name` in this checkout's own config wins over the global one.""" + config = base / "gitconfig" + config.write_bytes(b"[user]\n\tname = " + name + b"\n") + extra = {"GIT_CONFIG_GLOBAL": str(config)} + read = subprocess.run(["git", "config", "user.name"], cwd=str(REPO), + capture_output=True, env={**os.environ, **extra}, + check=False) + if read.stdout.rstrip(b"\r\n") != name: + pytest.skip("`git config user.name` here is not read from " + f"GIT_CONFIG_GLOBAL: {read.stdout!r}") + return extra + + +def with_origin(source: Path, url: bytes) -> Path: + """`source`, its `origin` set to `url`. Written into its config as + bytes, so the byte reaches git as it is on every platform (a Windows + command line is text). Skipped where git does not give it back so.""" + config = source / ".git" / "config" + config.write_bytes(config.read_bytes() + + b'[remote "origin"]\n\turl = ' + url + b"\n") + read = subprocess.run(["git", "remote", "get-url", "origin"], + cwd=str(source), capture_output=True, check=False) + if read.stdout.rstrip(b"\r\n") != url: + pytest.skip(f"`git remote get-url origin` here is {read.stdout!r}, " + f"not {url!r}") + return source + + +# --- `git config user.name` --------------------------------------------------- + +def test_plan_refuses_a_user_name_that_is_not_utf8(tmp_path): + """The issue's first row: on the base `plan` ended in a + `UnicodeDecodeError` traceback from `_elector`, exit 1. Now it is #255's + refusal, which names the setting and the flag, and no plan is + written.""" + extra = global_user_name(tmp_path, LATIN1_NAME) + source = make_source_repo(tmp_path / "Thing") + refused = by_git_config(tmp_path, source, extra) + assert_no_traceback_and_no_leg(tmp_path, {"plan": refused}) + assert refused.returncode == 2, said(refused) + assert ADOPT_REFUSED + b"--elected-by (not given, so `git config " \ + b"user.name`) is 'Jos\\udce9', which `execute` writes into the " \ + b"assembly root's `project.yaml` and `README.md`, and it holds a " \ + b"byte that is not UTF-8.\n" in refused.stderr, said(refused) + assert b"Remediation: no plan was written. Pass each value named " \ + b"again, retyped; when `--elected-by` is not given, `plan` reads " \ + b"`git config user.name`, so pass the flag or correct that." \ + in refused.stderr, said(refused) + assert not (tmp_path / PLAN_FILE).exists() + + +@pytest.mark.parametrize("name", [b"Jose", "José".encode("utf-8")], + ids=["ascii", "utf8"]) +def test_plan_records_a_user_name_that_is_utf8(tmp_path, name): + """Controls: a name git holds as UTF-8 is recorded as itself, as on the + base. `surrogateescape` changes nothing a strict decode reads.""" + extra = global_user_name(tmp_path, name) + source = make_source_repo(tmp_path / "Thing") + done = by_git_config(tmp_path, source, extra) + assert_no_traceback({"plan": done}) + assert done.returncode == 0, said(done) + plan = load_yaml(tmp_path / PLAN_FILE) + assert plan["elected_by"] == name.decode("utf-8") + + +@needs_filter_repo +def test_elected_by_given_adopts_under_a_user_name_that_is_not_utf8( + tmp_path): + """Control: with --elected-by given, `_elector` is not asked, and the + same Latin-1 `user.name` stops nothing: `plan`, `check` and `execute`, + all under it, adopt, and the plan records the flag's value.""" + extra = global_user_name(tmp_path, LATIN1_NAME) + source = make_source_repo(tmp_path / "Thing") + runs = {"plan": by_git_config(tmp_path, source, extra, + "--elected-by", "Test Human")} + assert runs["plan"].returncode == 0, said(runs["plan"]) + assert load_yaml(tmp_path / PLAN_FILE)["elected_by"] == "Test Human" + answer(tmp_path) + runs["check"] = checked(tmp_path, extra) + runs["execute"] = executed(tmp_path, extra) + assert_no_traceback(runs) + assert PLAN_OK in runs["check"].stdout, said(runs["check"]) + assert runs["execute"].returncode == 0, said(runs["execute"]) + assert VERIFIED in runs["execute"].stdout, said(runs["execute"]) + + +# --- `git remote get-url origin` ---------------------------------------------- + +@pytest.mark.parametrize("url, repository", + [(NOT_UTF8_URL, None), (ASCII_URL, "cafe/repo")], + ids=["not-utf8", "ascii"]) +def test_plan_reads_an_origin_that_is_not_utf8_as_none(tmp_path, url, + repository): + """The issue's last row, with --org: on the base `plan` ended in a + `UnicodeDecodeError` traceback from `_remote_repository`, exit 1, and + wrote no plan. Now it plans, and records no repository. The ASCII URL + is the control: its `org/repo` is read, so the None is the byte's.""" + source = with_origin(make_source_repo(tmp_path / "Thing"), url) + done = planned(tmp_path, source) + assert_no_traceback({"plan": done}) + assert done.returncode == 0, said(done) + assert load_yaml(tmp_path / PLAN_FILE)["source"]["repository"] \ + == repository + + +@pytest.mark.parametrize("url", [NOT_UTF8_URL, None], + ids=["not-utf8", "no-origin"]) +def test_plan_without_org_refuses_a_source_that_names_none(tmp_path, url): + """With no --org, an origin that is not UTF-8 is `adopt-no-org`, as a + source with no origin at all is. On the base the first was the + traceback, and the second `REFUSED unsafe-value: --org is '-'`: the + stand-in for a missing org met `checked_value` before `adopt-no-org` + was asked, which no run had ever reached.""" + source = make_source_repo(tmp_path / "Thing") + if url is not None: + with_origin(source, url) + refused = without_org(tmp_path, source) + assert_no_traceback({"plan": refused}) + assert refused.returncode == 2, said(refused) + assert NO_ORG in refused.stderr, said(refused) + assert b"unsafe-value" not in refused.stderr, said(refused) + assert not (tmp_path / PLAN_FILE).exists() + + +def test_plan_without_org_takes_the_org_an_ascii_origin_names(tmp_path): + """Control: an origin that names `cafe/repo` is the org, as on the + base.""" + source = with_origin(make_source_repo(tmp_path / "Thing"), ASCII_URL) + done = without_org(tmp_path, source) + assert_no_traceback({"plan": done}) + assert done.returncode == 0, said(done) + assert load_yaml(tmp_path / PLAN_FILE)["org"] == "cafe" + + +@needs_filter_repo +def test_check_and_execute_adopt_a_source_whose_origin_is_not_utf8( + tmp_path): + """The issue's `check` and `execute` rows: a plan written before the + origin was set, then both ended in the traceback, before any leg + existed. Now `check` says `plan ok`, and `execute` adopts.""" + source = make_source_repo(tmp_path / "Thing") + runs = {"plan": planned(tmp_path, source)} + assert runs["plan"].returncode == 0, said(runs["plan"]) + with_origin(source, NOT_UTF8_URL) + answer(tmp_path) + runs["check"] = checked(tmp_path) + runs["execute"] = executed(tmp_path) + assert_no_traceback(runs) + assert runs["check"].returncode == 0, said(runs["check"]) + assert PLAN_OK in runs["check"].stdout, said(runs["check"]) + assert runs["execute"].returncode == 0, said(runs["execute"]) + assert VERIFIED in runs["execute"].stdout, said(runs["execute"]) + + +@needs_filter_repo +def test_a_utf8_clone_of_a_source_whose_path_is_not_utf8_adopts(tmp_path): + """#255 refuses, under --local-remote-dir, a source whose own path is + not UTF-8: that path is the clone URL `execute` writes. A clone of it + at a path that is text has the old path as its origin, so `execute + --source ` ended in this issue's traceback; it now adopts. + Skipped where no directory can be named with the byte 0xE9 (Windows, + macOS's APFS) or git will not work in one.""" + try: + parent = Path(os.fsdecode(os.path.join(os.fsencode(tmp_path), + b"caf\xe9"))) + parent.mkdir() + except (OSError, UnicodeError) as exc: + pytest.skip(f"no directory here is named b'caf\\xe9': {exc!r}") + try: + source = make_source_repo(parent / "Thing") + except AssertionError as exc: + pytest.skip(f"git will not work under {parent.name!r} here: {exc}") + clone = tmp_path / "clone" + git("clone", "-q", "--", str(source), str(clone), cwd=tmp_path) + runs = {"plan": planned(tmp_path, source)} + assert runs["plan"].returncode == 0, said(runs["plan"]) + answer(tmp_path) + runs["refused"] = executed(tmp_path) + assert_no_traceback_and_no_leg(tmp_path, runs) + assert runs["refused"].returncode == 2, said(runs["refused"]) + assert ADOPT_REFUSED in runs["refused"].stderr, said(runs["refused"]) + runs["execute"] = adopted("execute", "--plan", str(tmp_path / PLAN_FILE), + "--source", str(clone), "--yes", + "--local-remote-dir", str(tmp_path / REMOTES), + "--work-dir", str(tmp_path / "work-again")) + assert_no_traceback(runs) + assert runs["execute"].returncode == 0, said(runs["execute"]) + assert VERIFIED in runs["execute"].stdout, said(runs["execute"]) From 344028effa251b34f02c5cbf1aa78e5343962276 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:33:06 +0000 Subject: [PATCH 7/8] adopt-project.py: `adopt-no-org` says all three of its causes, so a source whose `origin` is not UTF-8 or ends in no `org/repo` is no longer told it has no `origin`, and a passed `--org` is pinned to its `unsafe-value` refusal (#292) Review of dcbc37b, N1 and N3. N1: #292 routes a source whose origin URL is not UTF-8 to `adopt-no-org`. The sentence said "the source has no `origin` to read an organisation from", which is false of that source: `git remote -v` shows its origin. It is false too of a source whose URL ends in no `org/repo`. The refusal, its exit (2) and its remediation (`pass --org `) are unchanged; the sentence now reads "the source's `origin` names no organisation this tool can read: it has none, its URL is not UTF-8, or its URL ends in no `org/repo`". `test_plan_without_org_refuses_a_source_that_names_none` takes the new bytes and a third source, `no-pair`, an origin of `repo.git`. N3: #292 moved `args.org = checked_value("--org", args.org)` below `adopt-no-org`. Deleting that line survived this file and `tests/test_adopt_plan.py`: `plan --org=-evil` exited 0 and wrote `org: "-evil"`, `check` said `plan ok`, and only `execute` refused it, at `_repository_urls`. `test_plan_refuses_an_unsafe_org_that_is_passed` pins exit 2, `REFUSED unsafe-value: --org is '-evil'` and no plan file, for `-evil`, `-` and `my org`. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 12 ++++++---- tests/test_adopt_git_reads.py | 44 ++++++++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index ffec2641..65892ab3 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -1611,13 +1611,17 @@ def _checked_plan_inputs(args, source: Source, # NO ORG IS `adopt-no-org`, asked before `checked_value` (#292). The `-` # that once stood in for a missing one reached `checked_value` first, # which refused `--org is '-'`, a value nobody passed; this refusal was - # never reached. A source with no `origin`, or one whose URL is not - # UTF-8 (`_remote_repository`), names no org. + # never reached. A source with no `origin`, one whose URL is not UTF-8, + # and one whose URL ends in no `org/repo` (`_remote_repository`) all name + # no org, and the refusal says each of the three: it is asked of a source + # that HAS an origin as often as of one that has none. args.org = args.org or (source.repository or "/").split("/")[0] if not args.org: raise Refusal( - "adopt-no-org", "the source has no `origin` to read an " - "organisation from", "Remediation: pass --org .") + "adopt-no-org", "the source's `origin` names no organisation " + "this tool can read: it has none, its URL is not UTF-8, or its " + "URL ends in no `org/repo`", + "Remediation: pass --org .") args.org = checked_value("--org", args.org) args.elected_on = args.elected_on or _dt.date.today().isoformat() # Read before the plan is written, whether or not it is what chooses the diff --git a/tests/test_adopt_git_reads.py b/tests/test_adopt_git_reads.py index 7038265d..0ce23b08 100644 --- a/tests/test_adopt_git_reads.py +++ b/tests/test_adopt_git_reads.py @@ -52,8 +52,15 @@ NOT_UTF8_URL = b"../caf\xe9/repo.git" ASCII_URL = b"../cafe/repo.git" -NO_ORG = (b"REFUSED adopt-no-org: the source has no `origin` to read an " - b"organisation from\nRemediation: pass --org .\n") +#: Said of all three sources that name no org: one with no origin, one whose +#: origin URL is not UTF-8, and one whose URL ends in no `org/repo`. The +#: sentence was "the source has no `origin`", which is false of the last two. +NO_ORG = (b"REFUSED adopt-no-org: the source's `origin` names no " + b"organisation this tool can read: it has none, its URL is not " + b"UTF-8, or its URL ends in no `org/repo`\n" + b"Remediation: pass --org .\n") +#: An origin URL that is UTF-8 and ends in no `org/repo` at all. +NO_PAIR_URL = b"repo.git" def by_git_config(base: Path, source: Path, extra: dict, @@ -185,14 +192,17 @@ def test_plan_reads_an_origin_that_is_not_utf8_as_none(tmp_path, url, == repository -@pytest.mark.parametrize("url", [NOT_UTF8_URL, None], - ids=["not-utf8", "no-origin"]) +@pytest.mark.parametrize("url", [NOT_UTF8_URL, None, NO_PAIR_URL], + ids=["not-utf8", "no-origin", "no-pair"]) def test_plan_without_org_refuses_a_source_that_names_none(tmp_path, url): """With no --org, an origin that is not UTF-8 is `adopt-no-org`, as a - source with no origin at all is. On the base the first was the - traceback, and the second `REFUSED unsafe-value: --org is '-'`: the - stand-in for a missing org met `checked_value` before `adopt-no-org` - was asked, which no run had ever reached.""" + source with no origin at all is, and one whose URL ends in no `org/repo`. + On the base the first was the traceback, and the second `REFUSED + unsafe-value: --org is '-'`: the stand-in for a missing org met + `checked_value` before `adopt-no-org` was asked, which no run had ever + reached. The sentence is the same for the three, and says each: the + first and the last are sources that HAVE an origin, so "the source has + no `origin`" was false of them.""" source = make_source_repo(tmp_path / "Thing") if url is not None: with_origin(source, url) @@ -204,6 +214,24 @@ def test_plan_without_org_refuses_a_source_that_names_none(tmp_path, url): assert not (tmp_path / PLAN_FILE).exists() +@pytest.mark.parametrize("org", ["-evil", "-", "my org"], + ids=["leading-dash", "dash-only", "space"]) +def test_plan_refuses_an_unsafe_org_that_is_passed(tmp_path, org): + """`--org` is `checked_value`'s question too, and #292 moved it below + `adopt-no-org`: a `plan` that stopped asking it wrote `org: "-evil"` and + exited 0, `check` said `plan ok`, and only `execute` refused the value, + at `_repository_urls`. A value passed is refused as `unsafe-value`, + naming `--org`, before any plan is written.""" + source = make_source_repo(tmp_path / "Thing") + refused = planned(tmp_path, source, f"--org={org}") + assert_no_traceback({"plan": refused}) + assert refused.returncode == 2, said(refused) + assert b"REFUSED unsafe-value: --org is " + repr(org).encode("ascii") \ + + b", which is not a value this tool will put on a `git` or `gh` " \ + b"command line\n" in refused.stderr, said(refused) + assert not (tmp_path / PLAN_FILE).exists() + + def test_plan_without_org_takes_the_org_an_ascii_origin_names(tmp_path): """Control: an origin that names `cafe/repo` is the org, as on the base.""" From 96ff0c7a23baadf595ff709c10d2e484dc53373f Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:33:15 +0000 Subject: [PATCH 8/8] tests/test_adopt_git_reads.py: the four `user.name` tests give the name in git's command scope as well as in `GIT_CONFIG_GLOBAL`, so they run, and the `_elector` fix is tested, in a checkout whose own config sets `user.name` (#292) Review of dcbc37b, N2. `global_user_name` skipped whenever `git config user.name`, asked in the checkout, did not answer the name the test wrote to its `GIT_CONFIG_GLOBAL` file, and a `user.name` in the checkout's own `.git/config` outranks that file. In such a checkout (the shared one had one for a while) the file reported 7 passed, 4 skipped, and mutants of `_elector`'s decode (strict, `replace`, `ignore`, no `.strip()`), which only those four tests kill, went uncaught. On POSIX the name is now also given as `GIT_CONFIG_COUNT`, `GIT_CONFIG_KEY_` and `GIT_CONFIG_VALUE_`, git's command scope, which outranks every config file. The entry is APPENDED to a count the environment already holds: the Windows job sets `GIT_CONFIG_COUNT=2` (`core.autocrlf`, `core.longpaths`), and replacing it would drop both. The `GIT_CONFIG_GLOBAL` file stays, as the only route where an environment variable cannot carry the byte (Windows). The byte goes in through `os.fsdecode`. Two tests pin the route without depending on whose machine runs them: `test_the_user_name_given_outranks_a_checkouts_own` makes a checkout with a local `user.name` in `tmp_path` and reads the name given, and `test_the_user_name_given_is_appended_to_the_config_the_environment_holds` sets a count of 2 and reads both entries and the name. Lane: openreposhape-2 (openRepoShape-2) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_adopt_git_reads.py | 95 +++++++++++++++++++++++++++++++++-- 1 file changed, 90 insertions(+), 5 deletions(-) diff --git a/tests/test_adopt_git_reads.py b/tests/test_adopt_git_reads.py index 0ce23b08..69f93c9f 100644 --- a/tests/test_adopt_git_reads.py +++ b/tests/test_adopt_git_reads.py @@ -82,15 +82,45 @@ def without_org(base: Path, source: Path) -> subprocess.CompletedProcess: str(base / WORK)) +def command_scope(key: str, value: bytes) -> dict: + """`key` set to `value` in git's command scope, as an environment + (`GIT_CONFIG_COUNT`, `GIT_CONFIG_KEY_`, `GIT_CONFIG_VALUE_`, git + 2.31), for every git the run starts. That scope outranks every config + file, this checkout's own `.git/config` included, which `~/.gitconfig` + and `GIT_CONFIG_GLOBAL` do not. + + APPENDED to what the environment already holds, never in place of it: + the Windows job sets `GIT_CONFIG_COUNT=2` (`core.autocrlf` and + `core.longpaths`, `.github/workflows/tests.yml`), and a count of 1 here + would drop both. The last entry of a key is the one git reads. + + The value goes in as the bytes it is, through `os.fsdecode`, which is how + `sys.argv` and `os.environ` carry a byte that is not UTF-8 on POSIX. A + Windows environment is text, so the caller does not use this there.""" + held = int(os.environ.get("GIT_CONFIG_COUNT") or 0) + return {"GIT_CONFIG_COUNT": str(held + 1), + f"GIT_CONFIG_KEY_{held}": key, + f"GIT_CONFIG_VALUE_{held}": os.fsdecode(value)} + + def global_user_name(base: Path, name: bytes) -> dict: - """The environment of a person whose global `user.name` is `name`, - written as bytes: `GIT_CONFIG_GLOBAL` (git 2.32) stands in for - `~/.gitconfig` for every git the run starts. Skipped where `git config - user.name`, asked where `_elector` asks it, does not answer `name`: a - `user.name` in this checkout's own config wins over the global one.""" + """The environment of a person whose `user.name` is `name`, written as + bytes, for `git config user.name` asked where `_elector` asks it. + + Two routes, because two things outrank a global config. `GIT_CONFIG_GLOBAL` + (git 2.32) stands in for `~/.gitconfig` for every git the run starts, and + is the only route where an environment variable cannot carry the byte + (Windows). A `user.name` in this checkout's own config wins over that + file, though, and the shared checkout has one wherever somebody has set + their identity there: so on POSIX the name is given in command scope too + (`command_scope`), which wins over both. Without it the four tests that + use this skipped in such a checkout, and the `_elector` fix went untested. + Skipped where `git config user.name` still does not answer `name`.""" config = base / "gitconfig" config.write_bytes(b"[user]\n\tname = " + name + b"\n") extra = {"GIT_CONFIG_GLOBAL": str(config)} + if os.name == "posix": + extra.update(command_scope("user.name", name)) read = subprocess.run(["git", "config", "user.name"], cwd=str(REPO), capture_output=True, env={**os.environ, **extra}, check=False) @@ -115,6 +145,61 @@ def with_origin(source: Path, url: bytes) -> Path: return source +# --- the `user.name` these tests give ---------------------------------------- + +def asked(key: str, env: dict, cwd: Path = REPO) -> bytes: + """`git config ` in `cwd` under `env`: what git answers, as bytes.""" + read = subprocess.run(["git", "config", key], cwd=str(cwd), + capture_output=True, env=env, check=False) + return read.stdout.rstrip(b"\r\n") + + +posix_only = pytest.mark.skipif( + os.name != "posix", reason="a Windows environment is text: there the " + "name is given in `GIT_CONFIG_GLOBAL`'s file only") + + +@posix_only +def test_the_user_name_given_outranks_a_checkouts_own(tmp_path): + """The four tests below ask `git config user.name` in this checkout, and + skipped, with the `_elector` fix untested, wherever its own `.git/config` + set one: that outranks `GIT_CONFIG_GLOBAL`. Here is such a checkout, + made in `tmp_path`, so the answer does not depend on whose machine this + is: the environment `global_user_name` gives is read as `name`, not as the + checkout's `Local Name`.""" + checkout = tmp_path / "local" + checkout.mkdir() + git("init", "-q", cwd=checkout) + git("config", "user.name", "Local Name", cwd=checkout) + local = git("config", "--local", "user.name", cwd=checkout) + assert local.stdout.strip() == "Local Name", local.stderr + extra = global_user_name(tmp_path, LATIN1_NAME) + assert asked("user.name", {**os.environ, **extra}, checkout) \ + == LATIN1_NAME + + +@posix_only +def test_the_user_name_given_is_appended_to_the_config_the_environment_holds( + tmp_path, monkeypatch): + """The Windows job sets `GIT_CONFIG_COUNT=2`, `core.autocrlf` and + `core.longpaths`, for the whole run. A count of 1 for the name would + drop both, and every test after it would run with git's own line-end and + long-path defaults. The two slots here are the job's, under keys no + config of a machine this runs on sets.""" + for variable, value in {"GIT_CONFIG_COUNT": "2", + "GIT_CONFIG_KEY_0": "test.heldone", + "GIT_CONFIG_VALUE_0": "one", + "GIT_CONFIG_KEY_1": "test.heldtwo", + "GIT_CONFIG_VALUE_1": "two"}.items(): + monkeypatch.setenv(variable, value) + extra = global_user_name(tmp_path, LATIN1_NAME) + assert extra["GIT_CONFIG_COUNT"] == "3", extra + env = {**os.environ, **extra} + assert asked("test.heldone", env) == b"one" + assert asked("test.heldtwo", env) == b"two" + assert asked("user.name", env) == LATIN1_NAME + + # --- `git config user.name` --------------------------------------------------- def test_plan_refuses_a_user_name_that_is_not_utf8(tmp_path):