From 2a53a8e64a0cce17ab84cd1bf5949bcb6fbd4484 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Wed, 21 Oct 2026 12:31:45 +0000 Subject: [PATCH 1/5] adopt-project.py: `check` and `execute` ask a plan's `org:` and `id:` right after its three repository names, so a bad one beside an unanswered entry, a moved source, a refused name or another repository is that one refusal in both, and a `pins:` entry that is no pin is said as `_read_as` says it (#1020) #901 made `check` and `execute` name the same first problem among a plan's options. Outside them, the two still named different problems first (#1020, found by lane 1's writer on #901). 1. **One rule, the refusal before what `check` lists.** `check` refuses a bad `org:` or `id:` outright, exit 2, and lists none of its findings. `execute` refuses at the first of them, but it asked the two only after it had refused four things that `check` lists as findings: a plan for another repository (`plan-not-the-source`), a name the naming policy refuses (`naming-unclassified`), an unanswered entry (`plan-unresolved`) and a source that has moved on (`plan-stale`). And `check` asked the two only where the policy took the plan's names (`_manifest_values`), so an unsafe `org:` beside a refused name was a finding about the name, or about another repository. Both commands now ask the two first of the plan's values, right after the three repository names (`plan.names()`, then `_checked_org_and_id`): `execute` in `cmd_execute`, ahead of `_refuse_a_plan_for_another_repository`, and `check` at its head, after the triad advisory. `_checked_plan_values` no longer asks them, since `cmd_execute` has. 2. **#716's L4-48, decided.** An `org:` that `checked_value` refuses, held to a repository, was `unsafe-value` to `check` and `plan-not-the-source` to `execute`. Such an `org:` names no repository, on GitHub or in the manifest, so it is not compared with the source's. It is `unsafe-value` in both commands now, held to a repository or not. Once it is mended, a plan for another repository is still `plan-not-the-source` in both. This reverses three tests in `tests/test_adopt_check_plan_name.py`, which pinned the old answers on purpose: - `test_an_unsafe_org_is_unsafe_in_check_and_compared_by_execute_if_held` expected `plan-not-the-source` from `execute` in its six held rows. As `test_an_unsafe_org_is_unsafe_in_check_and_in_execute_held_or_not`, all twelve expect `unsafe-value` from both, in the same words. - `test_check_compares_an_unsafe_org_where_no_manifest_is_built` expected `check` to compare `org: acme labs` beside the refused name `bad_name`: `plan-not-the-source` and `naming-unclassified`, exit 1. As `test_an_unsafe_org_is_asked_before_the_plans_refused_names`, it expects `unsafe-value`, exit 2, from `check` and from `execute`. - `test_check_asks_an_unsafe_org_first_where_a_kept_submodule_url_is_relative` expected `plan-not-the-source` from `execute`. It now expects `unsafe-value`, in `check`'s words. 3. **A `pins:` entry that is no pin** was spelled with `repr` (`` `pins:` entry 1 is 7 ``, `True`, `None`). #901 made `pins:` itself read as `_read_as` reads it, and an entry reads the same way now: "the number 7", "the boolean true", "null", "a list of 2 item(s)". Docstrings and comments now say this: `_checked_org_and_id`, `_checked_plan_values`, `_manifest_values`, `_held_findings`, `_not_the_source`, `_refuse_a_plan_for_another_repository`, `_read_pins` and the #499 block comment. No finding is added or removed: only which problem is named first changes, and the spelling of an entry. Measured on main at `6da757cb` and on this branch, with a stub `gh`, a bogus token, an empty `GH_CONFIG_DIR`, `GIT_SSH_COMMAND=false` and `--local-remote-dir`. No run made a leg. | The plan has | `check` on main | `execute` on main | both, here | |---|---|---|---| | a bad id beside an unanswered entry | `plan-bad-id` | `plan-unresolved` | `plan-bad-id` | | a bad id beside a source that has moved on | `plan-bad-id` | `plan-stale` | `plan-bad-id` | | a bad id beside a name the policy refuses | `plan-bad-id` | `naming-unclassified` | `plan-bad-id` | | a bad id beside another organisation, held to a repository | `plan-bad-id` | `plan-not-the-source` | `plan-bad-id` | | no `id:`, and `legs.assembly: north_wind` (a derived id the policy refuses too) | `plan-bad-id` | `naming-unclassified` | `plan-bad-id` | | an unsafe `org:` held to a repository (L4-48) | `unsafe-value` | `plan-not-the-source` | `unsafe-value` | | an unsafe `org:` beside an unanswered entry | `unsafe-value` | `plan-unresolved` | `unsafe-value` | | an unsafe `org:` beside a source that has moved on | `unsafe-value` | `plan-stale` | `unsafe-value` | | an unsafe `org:` beside a refused name | finding `naming-unclassified`, exit 1 | `naming-unclassified` | `unsafe-value` | | the same, held to a repository | finding `plan-not-the-source`, exit 1 | `plan-not-the-source` | `unsafe-value` | Each "here" row is exit 2 from both, word for word. The last two rows agreed on main. They change because `check` now asks the two where the policy refuses the names, as `execute` now does: without that, moving `execute` alone would have made them disagree. Plans with only one of these problems (an unanswered entry, a moved source, a refused name, another repository) are named as on main by both. **Tests.** - `tests/test_adopt_plan_value_order.py` gains 16 rows: - the 10 above, through both commands, comparing the first line; - four `pins:` entries that are no pin: number, boolean, null and list; - two guard rows, `id` and `org`: a leg name `checked_value` refuses (`legs.spec: "-spec"`) beside a bad `id:` or `org:` is that name's `unsafe-value` in both, so the three names are still asked first. - The 14 non-guard rows all fail against `6da757cb`'s `adopt-project.py` and pass here. The two guard rows pass on both, by design. - Existing rows changed for the new behaviour: - `test_adopt_check_plan_name.py`: the three tests above, and its docstring's L4-48 line. - `test_adopt_pins.py`: `a-number`. - `test_adopt_execute_refusals.py`: the three entry rows of `PIN_ENTRIES`. Closes #1020 Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 127 +++++++++++++-------- tests/test_adopt_check_plan_name.py | 93 +++++++++------- tests/test_adopt_execute_refusals.py | 9 +- tests/test_adopt_pins.py | 4 +- tests/test_adopt_plan_value_order.py | 160 ++++++++++++++++++++++++++- 5 files changed, 300 insertions(+), 93 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 556dbc53..3473019e 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -596,7 +596,8 @@ def _read_pins(plan: "Plan") -> tuple[list, list[str]]: `plan-pin-malformed`. A `pins:` that is no list is said as `_read_as` says what the plan's reader read, as each other key a plan is opened by is (`_plan_key_findings`, #901): `pins: true` is the boolean true, and - not Python's `True`.""" + not Python's `True`. So is an entry that is neither a name nor a + mapping (#1020): `- 7` is the number 7, and `- ~` is null, not `None`.""" rows = plan.get("pins") if rows is None: return [], [] @@ -620,9 +621,11 @@ def _read_pins(plan: "Plan") -> tuple[list, list[str]]: f"(`neutral-pin-missing`): {_pin_fix(row)}.") continue if not isinstance(row, dict): + # Said as `pins:` itself is said (#1020): `- 7` is the number 7 + # and `- true` the boolean true, not Python's `True`. findings.append( f"FINDING {PIN_MALFORMED}: `pins:` entry {number} is " - f"{row!r}, not a pin: {_pin_fix()}.") + f"{_read_as(row)}, not a pin: {_pin_fix()}.") continue product = row.get("product") said = f"`pins:` entry {number}" + ( @@ -5652,7 +5655,9 @@ def _manifest_values(plan: Plan, naming: NamingPolicy) -> dict | None: names, which both commands ask earlier (`plan.names()`), and before the plan's leg paths and branch names (`_checked_org_and_id`, #901), so a bad date beside a bad organisation or id does not make `check` and `execute` - name different problems first. + name different problems first. `check` has asked the two at its head + already, where the policy refuses the names too (#1020); `plan` asks + them here. """ if not isinstance(plan.legs, dict): # The doctor's PLACEMENT plan writes `legs:` as a list (see @@ -5667,9 +5672,10 @@ def _manifest_values(plan: Plan, naming: NamingPolicy) -> dict | None: return None # The organisation and the id are asked BEFORE the date's guard below, # and as `execute` asks them, after the three repository names above - # and before the leg paths and the branch names (#901): `unsafe-value` - # for an `org:` this tool will not put on a command line, and - # `plan-bad-id` for an `id:` that is not a project id + # and before the leg paths and the branch names (#901; `check` and + # `execute` ask them first of the plan's values since #1020): + # `unsafe-value` for an `org:` this tool will not put on a command line, + # and `plan-bad-id` for an `id:` that is not a project id # (`_template_table` reads it again). After the guard, neither was asked # of a plan whose date is a finding: `check` listed the date where # `execute` refused the organisation, and refused the id where `execute` @@ -12321,6 +12327,15 @@ def cmd_check(args) -> int: # findings below and never moves this command's exit code. _print_triad_advisory(source.path, plan.source_commit or source.commit) + # THE PLAN'S THREE REPOSITORY NAMES, THEN ITS `org:` AND `id:`, as + # `execute` asks them, before any finding below (#1020): either is + # refused outright, exit 2, and `execute` asked them after refusing a + # plan for another repository, a name the policy refuses, an unanswered + # entry or a source that has moved on, each a finding here, so the two + # named different problems first. `_manifest_values` asked them only + # where the policy took the names. + plan.names() + _checked_org_and_id(plan) if plan.source_commit and source.commit != plan.source_commit: # `commit:` is a plan value people and AIs edit: sliced first, so the @@ -13294,7 +13309,8 @@ def _held_findings(plan: Plan, names: dict, local_remote_dir: Path | None, first of (#863), each as a FINDING under its code, and the codes, which say whether the leg question is still asked (`RENAMES_THE_LEGS`). Asked only of an `org:` `execute` would get that far with: another is - `unsafe-value`, refused first in `_checked_plan_values` (#901).""" + `unsafe-value`, which both commands refuse first of the plan's values + (`_checked_org_and_id`, #1020), `check` at its head.""" try: org = checked_value("org", _plan_org(plan)) except Refusal: @@ -13808,13 +13824,36 @@ def _mount_the_legs(assembly: Path, work_root: Path, names: dict, urls: dict, def _checked_org_and_id(plan: Plan) -> str: """The plan's `org:` as `checked_value` hands it back, once its `id:` - is asked too. Both commands ask the two BEFORE THE PLAN'S LEG PATHS AND - BRANCH NAMES, and after its three repository names, which they ask - earlier (`plan.names()`, then `_check_names`): `check` in - `_manifest_values` and `execute` in `_checked_plan_values`. - `checked_value` refuses an `org:` this tool will not put on a `git` or - `gh` command line as `unsafe-value`, and `Plan.project_id` an `id:` - that is no project id as `plan-bad-id`. + is asked too. Both commands ask the two FIRST OF THE PLAN'S VALUES, right + after its three repository names (`plan.names()`): before the plan is + compared with the source (`plan-not-the-source`), before the policy is + asked about its names, before its unanswered entries and a source that + has moved on, and before its leg paths and branch names. `check` asks + them at its head (`cmd_check`), and `execute` right after the names + (`cmd_execute`). `checked_value` refuses an `org:` this tool will not + put on a `git` or `gh` command line as `unsafe-value`, and + `Plan.project_id` an `id:` that is no project id as `plan-bad-id`. + + A REFUSAL OF `check`'S IS ASKED BEFORE WHAT IT LISTS (#1020). `check` + refuses either value outright, exit 2, and so lists none of its + findings, and `execute` refuses at the first of them. So the two name + one problem first only where `execute` asks the two before anything + `check` lists. #901 asked them in `_checked_plan_values`, after + `execute` had refused the plan for another repository, a name the + policy refuses, an unanswered entry and a source that has moved on, each + a finding of `check`'s: a bad id beside an unanswered entry was + `plan-bad-id` to `check` and `plan-unresolved` to `execute`, and so on + for `plan-stale`, `naming-unclassified` and `plan-not-the-source`. And + `check` asked them only where the naming policy took the plan's names + (`_manifest_values`), so an unsafe `org:` beside a name the policy + refuses was `naming-unclassified` or `plan-not-the-source` to `check`, + a finding. Asked first by both, each is that value's refusal in both. + That decides #716's L4-48 too: an `org:` `checked_value` refuses names + no repository, on GitHub or in the manifest, so it is not compared with + the source's: it is `unsafe-value` in both commands, held to a + repository or not, where `execute` said `plan-not-the-source` of one + held to a repository. Once the `org:` is mended, a plan that names + another repository is still `plan-not-the-source` in both. ONE ORDER, THE MORE BASIC PROBLEM FIRST (#901). `execute` asked the two leg paths and the two branch names first, the organisation after them @@ -13840,15 +13879,16 @@ def _checked_org_and_id(plan: Plan) -> str: def _checked_plan_values(plan: Plan) -> tuple[str, str, str, str]: - """The plan's options, in the order `check` asks them (#901): its - `org:` and `id:` (`_checked_org_and_id`), after the three repository - names both commands ask earlier (`plan.names()`), then the four values - that reach a `git` command line as arguments, which it returns: - `(spec_path, code_path, branch, tracking)`. + """The four plan values that reach a `git` command line as arguments, in + the order `check` asks them: `(spec_path, code_path, branch, tracking)`. + `cmd_execute` has asked the plan's `org:` and `id:` before them + (`_checked_org_and_id`), right after its three repository names, as + `check` asks them at its head (#901, #1020). Split out of `cmd_execute` for #138, in the order the refusals came in - there, the leg paths first; #901 asks the organisation and the id - before them, as `check` does, so the two name one problem first. + there, the leg paths first; #901 asked the organisation and the id + before them, here, as `check` does, so the two name one problem first, + and #1020 asks them earlier still, in `cmd_execute`. A branch name `checked_value` refuses is `check`'s finding too, in the same words (`_branch_name_refusal`, #520), and so is a branch the plan's reader read as a number or a boolean, which is asked first @@ -13858,7 +13898,6 @@ def _checked_plan_values(plan: Plan) -> tuple[str, str, str, str]: git will not create is refused with `check`'s other findings, by `_refuse_what_check_finds`, and before any leg exists as well. """ - _checked_org_and_id(plan) spec_leg, code_leg = _plan_leg_paths(plan) # Before ANY leg exists: `check` finds a bad path and `execute` refuses it # on its own account, not because `check` happened to be run first (#169). @@ -14928,6 +14967,9 @@ def cmd_execute(args) -> int: names = plan.names() pins = set(plan.pins) + # The organisation and the id next, as `check` asks them, before + # anything `check` lists as a finding is refused (#1020). + _checked_org_and_id(plan) _refuse_a_plan_for_another_repository(plan, source, names, naming, pins) _check_names(naming, names, pins, source) _refuse_an_unrunnable_plan(plan, source) @@ -16257,15 +16299,13 @@ def _name_remedy(code: str, repository: str | None) -> str: # with its `project:`, as `plan-not-the-source`: a hand edit is caught too, # because `org:` is read as `execute` reads it (`_plan_org`), as `str()` of # what the file holds. One that `checked_value` refuses (a list, a mapping, -# an empty value, a space, a leading `-`, a character outside ASCII) is, in -# `execute`, `plan-not-the-source` where the plan is held to a repository -# (`_held_repository`, and the `org:` is not that repository's own) and -# `unsafe-value` where it is held to none. `check` asks `checked_value` of -# `org:` first (`_manifest_values`, and `_submodule_findings` where a kept -# submodule url is relative) and ends in `unsafe-value`, exit 2, before it -# compares anything; only where the plan's own names are refused, and no -# kept url is relative, does it reach the comparison with one (#716, -# L4-48). A source that names no repository (no `origin`, or one that ends +# an empty value, a space, a leading `-`, a character outside ASCII) names +# no repository, and is never compared: `check` and `execute` refuse it as +# `unsafe-value` first of the plan's values, held to a repository or not +# (`_checked_org_and_id`, #1020). Until then `execute` refused it as +# `plan-not-the-source` where the plan was held to a repository, and `check` +# compared it where the plan's own names were refused (#716, L4-48). A +# source that names no repository (no `origin`, or one that ends # in no `org/repo`) is asked nothing by `plan`, as before; `check` and # `execute` hold the plan's names to what the plan recorded for it, # `source: repository:`, where it recorded one (#716). @@ -16532,18 +16572,15 @@ def _not_the_source(plan: Plan, source: Source, read only as text: a misread one is not this question's to answer, and `execute` acts on `legs.assembly:`. - AN `org:` THAT `checked_value` REFUSES IS CAUGHT, BUT NOT ALWAYS AS - THIS (#716, L4-48): a list, a mapping, an empty value, one with a space, - a leading `-` or a character outside ASCII. `execute` asks this first, - so it refuses such an `org:` as `plan-not-the-source` where the plan is - held to a repository (`_held_repository`, and the `org:` is not that - repository's own), and as `unsafe-value`, from `_checked_plan_values` - (#901), where it is held to none. `check` asks - `checked_value` of `org:` first (`_manifest_values`, and - `_submodule_findings` where a kept submodule url is relative) and ends - in `unsafe-value`, exit 2, before it compares anything; only where the - plan's own names are refused, and no kept url is relative, does it - reach this with one.""" + AN `org:` THAT `checked_value` REFUSES IS NEVER COMPARED (#1020): a + list, a mapping, an empty value, one with a space, a leading `-` or a + character outside ASCII. Both commands refuse it as `unsafe-value` + before they ask this (`_checked_org_and_id`), held to a repository or + not, for it names no repository. Until #1020 `execute` asked this + first, and refused such an `org:` as `plan-not-the-source` where the + plan was held to a repository and the `org:` was not that repository's + own, where `check` ended in `unsafe-value`, exit 2, except where the + plan's own names were refused (#716, L4-48).""" repository = _held_repository(plan, source) if repository is None: return None @@ -16580,7 +16617,9 @@ def _refuse_a_plan_for_another_repository( naming policy about the plan's names or any remote anything (#499): what it would ask of GitHub it would ask of the repository the plan names, and a raw `git ls-remote` failure on that repository was the - first thing it said.""" + first thing it said. It is asked after the plan's `org:` and `id:`, + which `check` refuses before it lists this (`_checked_org_and_id`, + #1020).""" detail = _not_the_source(plan, source, names) if detail: raise Refusal( diff --git a/tests/test_adopt_check_plan_name.py b/tests/test_adopt_check_plan_name.py index 2d2aff1c..6ea3fa39 100644 --- a/tests/test_adopt_check_plan_name.py +++ b/tests/test_adopt_check_plan_name.py @@ -20,12 +20,13 @@ that has none (`--source`), checked `plan ok`, and `execute` went on to ask GitHub about `acme/Atlsa`. The plan's record holds it now, in both. - L4-48: the comments on #499's `org:` read said that a hand edit is caught - "whatever `org:` then holds". An `org:` that `checked_value` refuses is - `unsafe-value` in `check`, which asks it first, and, in `execute`, - `plan-not-the-source` where the plan is held to a repository and - `unsafe-value` where it is held to none: caught in both, compared only - by one. The last tests here pin that, so the comments say what the - commands do. + "whatever `org:` then holds". An `org:` that `checked_value` refuses was + `unsafe-value` in `check`, which asked it first, and, in `execute`, + `plan-not-the-source` where the plan was held to a repository and + `unsafe-value` where it was held to none: caught in both, compared only + by one. #1020 decided it: such an `org:` names no repository, and both + commands refuse it as `unsafe-value` first of the plan's values, held to + a repository or not, and never compare it. The last tests here pin that. - RV825's MUST-1: v1.0.0 and v1.0.1 read a path `origin` as its last two directories (#521), so a plan they wrote can record `repository: clones/Atlas` for a source whose `origin` is `/clones/Atlas.git`. @@ -407,7 +408,7 @@ def test_a_plan_that_v1_0_x_wrote_for_a_path_origin_is_still_a_correct_plan( tmp_path / "work") -# --- L4-48: what `check` does with an `org:` `checked_value` refuses -------- +# --- L4-48: an `org:` `checked_value` refuses, in `check` and `execute` ----- #: The value `execute` spells, and `check` asks `checked_value` about. UNSAFE_ORGS = [("org: acme labs", "'acme labs'"), @@ -418,22 +419,28 @@ def test_a_plan_that_v1_0_x_wrote_for_a_path_origin_is_still_a_correct_plan( ("org: {a: b}", "\"{'a': 'b'}\"")] +def unsafe_org(spelled: str) -> str: + """How `check` refuses an `org:` `checked_value` refuses, spelled + `spelled`, and `execute` in the same words.""" + return (f"REFUSED unsafe-value: org is {spelled}, which is not a value " + "this tool will put on a `git` or `gh` command line") + + @pytest.mark.parametrize("org, spelled", UNSAFE_ORGS, ids=["space", "list", "empty", "leading-dash", "not-ascii", "mapping"]) -@pytest.mark.parametrize("held, code", [ - (True, PLAN_NOT_THE_SOURCE), - (False, "unsafe-value"), -], ids=["held-to-a-repository", "held-to-none"]) -def test_an_unsafe_org_is_unsafe_in_check_and_compared_by_execute_if_held( - adopter, tmp_path, monkeypatch, org, spelled, held, code): - """L4-48: the comments on #499's `org:` read said any `org:` is caught - "whatever the file holds". It is caught in both commands, and not by the - same one: `check` asks `checked_value` first and ends in `unsafe-value`, - exit 2, never comparing it with the repository. `execute` compares it - first, so it refuses it as `plan-not-the-source` where the plan is held - to a repository, and as `unsafe-value` where it is held to none (a source - that recorded no repository), as `check` does in both.""" +@pytest.mark.parametrize("held", [True, False], + ids=["held-to-a-repository", "held-to-none"]) +def test_an_unsafe_org_is_unsafe_in_check_and_in_execute_held_or_not( + adopter, tmp_path, monkeypatch, org, spelled, held): + """L4-48, as #1020 decided it: the comments on #499's `org:` read said + any `org:` is caught "whatever the file holds", and it was, though not + by the same refusal: `execute` compared it first, and refused it as + `plan-not-the-source` where the plan is held to a repository, where + `check` said `unsafe-value`. Such an `org:` names no repository, so + neither compares it: both refuse it as `unsafe-value`, in the same + words, held to a repository or to none (a source that recorded no + repository).""" if held: planned_from_origin(tmp_path) else: @@ -445,33 +452,34 @@ def test_an_unsafe_org_is_unsafe_in_check_and_compared_by_execute_if_held( tmp_path / PLAN, tmp_path / "work") assert checked.returncode == 2, checked.stdout + checked.stderr - unsafe = (f"REFUSED unsafe-value: org is {spelled}, which is not a value " - "this tool will put on a `git` or `gh` command line") + unsafe = unsafe_org(spelled) assert unsafe in checked.stderr.splitlines() assert PLAN_NOT_THE_SOURCE not in checked.stderr - assert refusal.code == code - if not held: - assert unsafe[len("REFUSED "):] == f"{refusal.code}: {refusal.detail}" + assert unsafe[len("REFUSED "):] == f"{refusal.code}: {refusal.detail}" -def test_check_compares_an_unsafe_org_where_no_manifest_is_built( - tmp_path): - """Where `check` reaches the comparison with such an `org:`: where the - plan's own names are refused first, `_manifest_values` builds no - manifest and never asks `checked_value` (and no kept submodule url is - relative: the next test), so `check` finds the plan names another - repository, and the name, and exits 1.""" +def test_an_unsafe_org_is_asked_before_the_plans_refused_names( + adopter, tmp_path, monkeypatch): + """Where the plan's own names are refused, `_manifest_values` builds no + manifest and never asked `checked_value`, so `check` compared such an + `org:`, finding that the plan names another repository, and the name, + exit 1, as `execute` refused it. Both ask it first of the plan's values + now (#1020): `unsafe-value`, exit 2, in the same words, and neither + compares it.""" planned_from_origin(tmp_path) edited(tmp_path / PLAN, ("org: acme", "org: acme labs")) renamed(tmp_path / PLAN, "bad_name", OWN) checked = check(tmp_path / PLAN) + refusal = refused_before_any_remote(adopter, monkeypatch, + tmp_path / PLAN, tmp_path / "work") - assert checked.returncode == 1, checked.stdout + checked.stderr - assert [line.split(":")[0] for line in findings(checked.stderr)] == [ - f"FINDING {PLAN_NOT_THE_SOURCE}", - "FINDING naming-unclassified"], checked.stderr - assert "`org: acme labs`" in findings(checked.stderr)[0] + assert checked.returncode == 2, checked.stdout + checked.stderr + assert findings(checked.stderr) == [], checked.stderr + unsafe = unsafe_org("'acme labs'") + assert unsafe in checked.stderr.splitlines() + assert PLAN_NOT_THE_SOURCE not in checked.stderr + assert unsafe[len("REFUSED "):] == f"{refusal.code}: {refusal.detail}" def test_check_asks_an_unsafe_org_first_where_a_kept_submodule_url_is_relative( @@ -480,8 +488,8 @@ def test_check_asks_an_unsafe_org_first_where_a_kept_submodule_url_is_relative( `org:` earlier than `_manifest_values` does, wherever a kept submodule's url is relative. With the plan's names refused and `org: test org`, `check` ends in `unsafe-value`, exit 2, and never compares it, as it - does with no relative url (the test above); `execute` says - `plan-not-the-source`.""" + does with no relative url (the test above); and `execute`, which said + `plan-not-the-source`, says `unsafe-value` too (#1020).""" source = source_with_submodule(tmp_path, SIBLING, HTTPS_ORIGIN) made = planned_with_submodule(source, tmp_path) edited(made, ("org: testorg", "org: test org")) @@ -492,8 +500,7 @@ def test_check_asks_an_unsafe_org_first_where_a_kept_submodule_url_is_relative( tmp_path / "work") assert checked.returncode == 2, checked.stdout + checked.stderr - assert checked.stderr.splitlines()[0] == ( - "REFUSED unsafe-value: org is 'test org', which is not a value this " - "tool will put on a `git` or `gh` command line") + unsafe = unsafe_org("'test org'") + assert checked.stderr.splitlines()[0] == unsafe assert PLAN_NOT_THE_SOURCE not in checked.stderr - assert refusal.code == PLAN_NOT_THE_SOURCE + assert unsafe[len("REFUSED "):] == f"{refusal.code}: {refusal.detail}" diff --git a/tests/test_adopt_execute_refusals.py b/tests/test_adopt_execute_refusals.py index 5133c1eb..88eae9c6 100644 --- a/tests/test_adopt_execute_refusals.py +++ b/tests/test_adopt_execute_refusals.py @@ -1082,16 +1082,17 @@ def test_a_list_an_empty_list_a_bare_key_and_no_key_are_not_plan_bad_findings( #: was adopted as written: the pins `openGlass` and `True`, `2026`, none, and #: `{'a': 1}`. Since #273 every entry of a list is `_read_pins`' to judge, #: beside the entries, in its own words: a name alone is `plan-pin- -#: unrecorded`, anything else `plan` never writes there `plan-pin-malformed`. +#: unrecorded`, anything else `plan` never writes there `plan-pin-malformed`, +#: said as `_read_as` says what the plan's reader read (#1020). PIN_ENTRIES = { "name-and-true": ("pins: [openGlass, true]\n", [ (PIN_UNRECORDED, "`pins:` entry 1, 'openGlass', is a name with no " "commit and no digest"), - (PIN_MALFORMED, "`pins:` entry 2 is True, not a pin")]), + (PIN_MALFORMED, "`pins:` entry 2 is the boolean true, not a pin")]), "number": ("pins:\n - 2026\n", [ - (PIN_MALFORMED, "`pins:` entry 1 is 2026, not a pin")]), + (PIN_MALFORMED, "`pins:` entry 1 is the number 2026, not a pin")]), "null": ("pins: [null]\n", [ - (PIN_MALFORMED, "`pins:` entry 1 is None, not a pin")]), + (PIN_MALFORMED, "`pins:` entry 1 is null, not a pin")]), "mapping": ("pins: [{a: 1}]\n", [ (PIN_UNRECORDED, "`pins:` entry 1 records no `commit:` and no " "`tree_sha256:`")]), diff --git a/tests/test_adopt_pins.py b/tests/test_adopt_pins.py index 86c12462..949fee42 100644 --- a/tests/test_adopt_pins.py +++ b/tests/test_adopt_pins.py @@ -1237,7 +1237,9 @@ def malformed(glass) -> dict: # Said as `_read_as` says what the plan's reader read (#901). "not-a-list": ("pins: openGlass\n", "`pins:` is the text 'openGlass', not a list of pins"), - "a-number": ("pins:\n - 7\n", "`pins:` entry 1 is 7, not a pin"), + # And an entry the same way (#1020). + "a-number": ("pins:\n - 7\n", + "`pins:` entry 1 is the number 7, not a pin"), "short-commit": ("pins:\n" + pin_entry(commit=commit[:12], digest=digest), f"{entry} `commit: '{commit[:12]}'`, which is not " diff --git a/tests/test_adopt_plan_value_order.py b/tests/test_adopt_plan_value_order.py index 9e0ae549..710e0e70 100644 --- a/tests/test_adopt_plan_value_order.py +++ b/tests/test_adopt_plan_value_order.py @@ -32,6 +32,21 @@ (`_digest_source_written`). It ends "..., and how it was read, and nothing else" now, and no finding about another key of a pin says so. +AND BEFORE WHAT `check` LISTS (#1020), on main at 6da757cb, measured the +same way. `check` refuses a bad `org:` or `id:` outright, exit 2, and lists +none of its findings, but `execute` asked the two only after refusing a plan +for another repository, a name the naming policy refuses, an unanswered +entry or a source that has moved on, each a finding of `check`'s: a bad id +beside an unanswered entry was `plan-bad-id` to `check` and +`plan-unresolved` to `execute`, and so on. `check` asked them only where the +policy took the plan's names, so an unsafe `org:` beside a refused name was +a finding about the name, or about another repository, to `check`. Both +commands now ask the two first of the plan's values, right after the three +repository names (`_checked_org_and_id`). That decides #716's L4-48 as well: +an unsafe `org:` held to a repository is `unsafe-value` to both, where +`execute` said `plan-not-the-source`. And a `pins:` ENTRY that is no pin is +said as `pins:` itself is (`_read_as`): the number 7, not `7`. + Each test runs `check` and then `execute --yes`, both with the same `--local-remote-dir`, on one hand-edited plan, as the issue measured, and asserts that the directory the legs go in was never made. `execute` stops @@ -47,7 +62,7 @@ import pytest -from conftest import ADOPT, make_source_repo, planned, run_script +from conftest import ADOPT, git, make_source_repo, planned, run_script from test_adopt_pins import ANSWERS, pin_entry UTF8 = "utf-8" @@ -372,3 +387,146 @@ def test_a_finding_about_another_key_does_not_say_how_it_was_read( assert "how it was read" not in found[0], found[0] assert executed.returncode == 2, executed.stdout + executed.stderr assert refused_with(executed, tmp_path / PLAN_FILE, found) == code + + +# --- the organisation and the id before what `check` lists (#1020) --------- + +#: The answers `planned` gives, but `examples/`'s: its entry keeps `leg: +#: null`, an unanswered question. +UNANSWERED = tuple(answer for answer in ANSWERS if answer[0] != "examples/") +#: More hand edits: a leg name the naming policy refuses, another +#: organisation than the source's, and a plan with no `id:` whose assembly +#: name gives a bad derived id, which the policy refuses too. +REFUSED_NAME = ("\n spec: Northwind-spec\n", "\n spec: bad_name\n") +ANOTHER_ORG = ("\norg: testorg\n", "\norg: acme\n") +DERIVED_BAD_ID = (("\nid: northwind\n", "\n"), + ("\n assembly: Northwind\n", "\n assembly: north_wind\n")) +#: The repository the source's `origin` names, which `plan` records and a +#: plan for it is held to (`_held_repository`). +HELD = "testorg/Northwind" +#: What both commands name first about the derived id. +DERIVED_ID_REFUSED = ("REFUSED plan-bad-id: {plan}: id (not set in the plan, " + "so legs.assembly lowercased) is 'north_wind', which " + "must match ^[a-z0-9][a-z0-9-]*$") + +#: `(the edits, what else the plan holds, what both commands refuse first)`, +#: by test id. What else: `unanswered` leaves `examples/` unanswered, +#: `moved` commits to the source after `plan`, and `held` gives the source +#: an `origin` naming `HELD`. On 6da757cb `execute` named first, row by +#: row: `plan-unresolved`, `plan-stale`, `naming-unclassified`, +#: `plan-not-the-source`, `naming-unclassified`, `plan-not-the-source` +#: (L4-48), `plan-unresolved`, `plan-stale`, `naming-unclassified` and +#: `plan-not-the-source`; in the last two rows `check` named the same, as a +#: finding, exit 1, and in the others it refused the organisation or the id. +BESIDE = { + "id-and-unanswered": ((BAD_ID,), {"unanswered"}, ID_REFUSED), + "id-and-moved-on": ((BAD_ID,), {"moved"}, ID_REFUSED), + "id-and-refused-name": ((BAD_ID, REFUSED_NAME), set(), ID_REFUSED), + "id-and-another-repository": ((BAD_ID, ANOTHER_ORG), {"held"}, + ID_REFUSED), + "derived-id-and-its-refused-name": (DERIVED_BAD_ID, set(), + DERIVED_ID_REFUSED), + "org-held-to-a-repository": ((BAD_ORG,), {"held"}, ORG_REFUSED), + "org-and-unanswered": ((BAD_ORG,), {"unanswered"}, ORG_REFUSED), + "org-and-moved-on": ((BAD_ORG,), {"moved"}, ORG_REFUSED), + "org-and-refused-name": ((BAD_ORG, REFUSED_NAME), set(), ORG_REFUSED), + "org-held-and-refused-name": ((BAD_ORG, REFUSED_NAME), {"held"}, + ORG_REFUSED), +} + + +def planned_with(base: Path, holds: set) -> str: + """The text of the plan `plan` writes for a new source at `base`, with + what `holds` names (`BESIDE`): each needs a source of its own.""" + source = make_source_repo(base / "Thing") + if "held" in holds: + git("remote", "add", "origin", f"https://github.com/{HELD}.git", + cwd=source) + text = planned(source, base, dict( + UNANSWERED if "unanswered" in holds else ANSWERS)).read_text( + encoding=UTF8) + if "moved" in holds: + (source / "later.txt").write_text("later\n", encoding=UTF8) + git("add", "later.txt", cwd=source) + git("commit", "-q", "-m", "later work", cwd=source) + return text + + +@needs_filter_repo +@pytest.mark.parametrize("case", sorted(BESIDE)) +def test_the_org_or_the_id_is_named_before_what_check_lists(tmp_path, + case): + """`check` refuses a bad `org:` or `id:` before it lists a finding, so + `execute` asks the two before it refuses anything `check` lists: an + unanswered entry, a source that has moved on, a name the policy + refuses, a plan for another repository. Each such plan is refused for + the organisation or the id by both, exit 2, in the same words, and no + leg is made.""" + edits, holds, refused = BESIDE[case] + text = edited(planned_with(tmp_path, holds), *edits) + checked, executed = both_on(text, tmp_path) + assert checked.returncode == 2, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert named_first(checked).startswith( + refused.format(plan=tmp_path / PLAN_FILE)), checked.stderr + assert named_first(executed) == named_first(checked), executed.stderr + + +#: A leg's repository name `checked_value` refuses. +UNSAFE_LEG_NAME = ("\n spec: Northwind-spec\n", '\n spec: "-spec"\n') +#: What both commands name first about it. +LEG_NAME_REFUSED = ("REFUSED unsafe-value: legs.spec is '-spec', which is not " + "a value this tool will put on a `git` or `gh` command " + "line") + + +@needs_filter_repo +@pytest.mark.parametrize("edit", [BAD_ID, BAD_ORG], ids=["id", "org"]) +def test_the_three_repository_names_are_still_asked_first(answered, + tmp_path, edit): + """A GUARD, which passes on 6da757cb too: the organisation and the id + are asked right AFTER the plan's three repository names, by `check` at + its head as by `execute` (`plan.names()`), so a leg name + `checked_value` refuses beside a bad `id:` or `org:` is that name's + `unsafe-value` to both, as it was, and not the other value's.""" + checked, executed = both_on(edited(answered, UNSAFE_LEG_NAME, edit), + tmp_path) + assert checked.returncode == 2, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert named_first(checked) == LEG_NAME_REFUSED, checked.stderr + assert named_first(executed) == LEG_NAME_REFUSED, executed.stderr + + +# --- a `pins:` entry that is no pin, said as its reader read it ------------- + +#: `(the plan's `pins:` block, what entry 1 was read as, as `_read_as` says +#: it)`, by test id. On 6da757cb each was `repr`'s: `7`, `True`, `None`, +#: `[1, 2]`. +NOT_A_PIN = { + "number": ("pins:\n - 7\n", "the number 7"), + "boolean": ("pins:\n - true\n", "the boolean true"), + "null": ("pins:\n - ~\n", "null"), + "list": ("pins:\n - [1, 2]\n", "a list of 2 item(s)"), +} + + +@needs_filter_repo +@pytest.mark.parametrize("case", sorted(NOT_A_PIN)) +def test_an_entry_that_is_no_pin_is_said_as_its_reader_read_it( + answered, tmp_path, case): + """An entry of `pins:` that is neither a name nor a mapping was spelled + as Python spells it, `True` or `None`, where `pins:` itself is said as + `_read_as` says what the plan's reader read (#901): `check` finds it in + those words now, and `execute` refuses it in them.""" + block, read = NOT_A_PIN[case] + checked, executed = both_on( + edited(answered, ("\npins: []\n", f"\n{block}")), tmp_path) + found = findings_of(checked) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert len(found) == 1, checked.stderr + assert found[0].startswith( + f"FINDING plan-pin-malformed: `pins:` entry 1 is {read}, not a pin: " + "re-run `plan` with `--pin @<40 hex commit>`"), found[0] + assert executed.returncode == 2, executed.stdout + executed.stderr + assert refused_with(executed, tmp_path / PLAN_FILE, + found) == "plan-pin-malformed" From 9549006fed3b5828ee2e0a21fcb01abd53fe4658 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 22 Oct 2026 13:35:10 +0000 Subject: [PATCH 2/5] adopt-project.py: `check` and `execute` ask a plan's `org:` and `id:` right after the plan's keys and before `_work_root`, so a bad one beside an unwritable `--work-dir` is that one refusal in both (#1020) RV1125's SHOULD-1 on #1125. A bad `id:` or `org:` beside a `--work-dir` this user cannot write in was `plan-bad-id` (or `unsafe-value`) to `check` and `adopt-work-dir-unwritable` to `execute`: `check` lists the directory as a finding (#524), and `execute` refuses it before it opens the source, ahead of the two asks, which #1125 had placed after `_work_root`. The comments in `cmd_execute` and `_checked_org_and_id` said that cannot happen. - `cmd_check` asks `plan.names()` and `_checked_org_and_id(plan)` right after its mistyped-plan question, before `_work_root`, and `cmd_execute` asks them right after `_refuse_a_mistyped_plan`, before `_work_root`. Both read only the plan. `execute` now asks them before `_require_filter_repo()` too. - A side effect, said in the body: `check` printed the Triad advisory, once it had opened the source, before it refused a bad `org:` or `id:`; it prints nothing now, since the refusal comes before the source is opened. - The comments and docstrings that placed the asks "at its head, after the triad advisory" or "right after the names" say where they are now. - tests/test_adopt_plan_value_order.py: `both_on` takes a `--work-dir` for both commands; one row per bad `id:` and bad `org:` beside an unwritable `--work-dir` (skipped where `chmod` cannot keep this user from writing); and, for NIT-2, one row per value that asks `check` alone and holds stdout empty, the position being told by the Triad advisory the unedited plan prints first. - tests/test_adopt_check_plan_name.py: the docstring of the kept-submodule row says it is a regression row since `check` asks `org:` at its head (NIT-1). Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 68 ++++++++++++++----------- tests/test_adopt_check_plan_name.py | 16 +++--- tests/test_adopt_plan_value_order.py | 76 ++++++++++++++++++++++++---- 3 files changed, 116 insertions(+), 44 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 3473019e..4300f48c 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -12302,6 +12302,16 @@ def cmd_check(args) -> int: print(line, file=sys.stderr) print(f"\n{len(mistyped)} finding(s) in {plan.path}", file=sys.stderr) return 1 + # THE PLAN'S THREE REPOSITORY NAMES, THEN ITS `org:` AND `id:`, right + # after the plan's keys and before anything else is read, made or opened, + # as `execute` asks them (#1020): either is refused outright, exit 2, and + # `execute` asked them after refusing an unwritable `--work-dir`, a plan + # for another repository, a name the policy refuses, an unanswered entry + # or a source that has moved on, each a finding here, so the two named + # different problems first. `_manifest_values` asked them only where the + # policy took the names. They read only the plan. + plan.names() + _checked_org_and_id(plan) work_root = _work_root(args) # Asked before the source is opened, as `execute` asks it (#524). An # `org/repo` source is cloned into the work directory, so where that @@ -12327,15 +12337,6 @@ def cmd_check(args) -> int: # findings below and never moves this command's exit code. _print_triad_advisory(source.path, plan.source_commit or source.commit) - # THE PLAN'S THREE REPOSITORY NAMES, THEN ITS `org:` AND `id:`, as - # `execute` asks them, before any finding below (#1020): either is - # refused outright, exit 2, and `execute` asked them after refusing a - # plan for another repository, a name the policy refuses, an unanswered - # entry or a source that has moved on, each a finding here, so the two - # named different problems first. `_manifest_values` asked them only - # where the policy took the names. - plan.names() - _checked_org_and_id(plan) if plan.source_commit and source.commit != plan.source_commit: # `commit:` is a plan value people and AIs edit: sliced first, so the @@ -13824,26 +13825,29 @@ def _mount_the_legs(assembly: Path, work_root: Path, names: dict, urls: dict, def _checked_org_and_id(plan: Plan) -> str: """The plan's `org:` as `checked_value` hands it back, once its `id:` - is asked too. Both commands ask the two FIRST OF THE PLAN'S VALUES, right - after its three repository names (`plan.names()`): before the plan is - compared with the source (`plan-not-the-source`), before the policy is - asked about its names, before its unanswered entries and a source that - has moved on, and before its leg paths and branch names. `check` asks - them at its head (`cmd_check`), and `execute` right after the names - (`cmd_execute`). `checked_value` refuses an `org:` this tool will not - put on a `git` or `gh` command line as `unsafe-value`, and - `Plan.project_id` an `id:` that is no project id as `plan-bad-id`. + is asked too. Both commands ask the two FIRST OF WHAT THEY READ OF THE + PLAN, right after the plan's keys (`_plan_type_findings`) and its three + repository names (`plan.names()`), and before `_work_root` makes the work + directory or the source is opened: so before an unwritable `--work-dir`, + before the plan is compared with the source (`plan-not-the-source`), + before the policy is asked about its names, before its unanswered + entries and a source that has moved on, and before its leg paths and + branch names. Both ask them at their head (`cmd_check`, `cmd_execute`). + `checked_value` refuses an `org:` this tool will not put on a `git` or + `gh` command line as `unsafe-value`, and `Plan.project_id` an `id:` that + is no project id as `plan-bad-id`. Both read only the plan. A REFUSAL OF `check`'S IS ASKED BEFORE WHAT IT LISTS (#1020). `check` refuses either value outright, exit 2, and so lists none of its findings, and `execute` refuses at the first of them. So the two name one problem first only where `execute` asks the two before anything `check` lists. #901 asked them in `_checked_plan_values`, after - `execute` had refused the plan for another repository, a name the - policy refuses, an unanswered entry and a source that has moved on, each - a finding of `check`'s: a bad id beside an unanswered entry was - `plan-bad-id` to `check` and `plan-unresolved` to `execute`, and so on - for `plan-stale`, `naming-unclassified` and `plan-not-the-source`. And + `execute` had refused an unwritable `--work-dir`, the plan for another + repository, a name the policy refuses, an unanswered entry and a source + that has moved on, each a finding of `check`'s: a bad id beside an + unanswered entry was `plan-bad-id` to `check` and `plan-unresolved` to + `execute`, and so on for `plan-stale`, `naming-unclassified`, + `plan-not-the-source` and `adopt-work-dir-unwritable`. And `check` asked them only where the naming policy took the plan's names (`_manifest_values`), so an unsafe `org:` beside a name the policy refuses was `naming-unclassified` or `plan-not-the-source` to `check`, @@ -13882,8 +13886,9 @@ def _checked_plan_values(plan: Plan) -> tuple[str, str, str, str]: """The four plan values that reach a `git` command line as arguments, in the order `check` asks them: `(spec_path, code_path, branch, tracking)`. `cmd_execute` has asked the plan's `org:` and `id:` before them - (`_checked_org_and_id`), right after its three repository names, as - `check` asks them at its head (#901, #1020). + (`_checked_org_and_id`), at its head, right after the plan's keys and + its three repository names and before `_work_root`, as `check` asks + them (#901, #1020). Split out of `cmd_execute` for #138, in the order the refusals came in there, the leg paths first; #901 asked the organisation and the id @@ -14958,6 +14963,15 @@ def _commit_the_split(plan: Plan, source: Source, assembly: Path, def cmd_execute(args) -> int: plan = Plan.load(Path(args.plan)) _refuse_a_mistyped_plan(plan) + # THE PLAN'S THREE REPOSITORY NAMES, THEN ITS `org:` AND `id:`, right + # after the plan's keys and before anything else is read, made or opened, + # as `check` asks them (#1020): `check` refuses either outright, exit 2, + # before it lists any finding, among them an unwritable `--work-dir`, a + # plan for another repository, a name the policy refuses, an unanswered + # entry and a source that has moved on, so `execute` asks the two before + # it refuses any of them. They read only the plan. + names = plan.names() + _checked_org_and_id(plan) work_root = _work_root(args, NO_LEG_YET) # Before the source is opened, so before any leg exists (#524). _refuse_an_unwritable_work_dir(args, work_root) @@ -14965,11 +14979,7 @@ def cmd_execute(args) -> int: naming = NamingPolicy.load(NAMING_POLICY) _require_filter_repo() - names = plan.names() pins = set(plan.pins) - # The organisation and the id next, as `check` asks them, before - # anything `check` lists as a finding is refused (#1020). - _checked_org_and_id(plan) _refuse_a_plan_for_another_repository(plan, source, names, naming, pins) _check_names(naming, names, pins, source) _refuse_an_unrunnable_plan(plan, source) diff --git a/tests/test_adopt_check_plan_name.py b/tests/test_adopt_check_plan_name.py index 6ea3fa39..136d1685 100644 --- a/tests/test_adopt_check_plan_name.py +++ b/tests/test_adopt_check_plan_name.py @@ -484,12 +484,16 @@ def test_an_unsafe_org_is_asked_before_the_plans_refused_names( def test_check_asks_an_unsafe_org_first_where_a_kept_submodule_url_is_relative( adopter, tmp_path, monkeypatch): - """RV825's SHOULD-1(b): `_submodule_findings` asks `checked_value` of - `org:` earlier than `_manifest_values` does, wherever a kept submodule's - url is relative. With the plan's names refused and `org: test org`, - `check` ends in `unsafe-value`, exit 2, and never compares it, as it - does with no relative url (the test above); and `execute`, which said - `plan-not-the-source`, says `unsafe-value` too (#1020).""" + """A REGRESSION ROW, kept from RV825's SHOULD-1(b), where + `_submodule_findings` asked `checked_value` of `org:` earlier than + `_manifest_values` did, wherever a kept submodule's url is relative, so + that `check` ended in `unsafe-value` where it asked no earlier. Since + #1020 `check` asks `org:` at its head, before either, whatever the + submodule urls are, so the relative url decides nothing and this row + says what the one above it says. With the plan's names refused and `org: + test org`, `check` ends in `unsafe-value`, exit 2, and never compares + it; and `execute`, which said `plan-not-the-source`, says `unsafe-value` + too.""" source = source_with_submodule(tmp_path, SIBLING, HTTPS_ORIGIN) made = planned_with_submodule(source, tmp_path) edited(made, ("org: testorg", "org: test org")) diff --git a/tests/test_adopt_plan_value_order.py b/tests/test_adopt_plan_value_order.py index 710e0e70..351c6d8e 100644 --- a/tests/test_adopt_plan_value_order.py +++ b/tests/test_adopt_plan_value_order.py @@ -41,11 +41,14 @@ `plan-unresolved` to `execute`, and so on. `check` asked them only where the policy took the plan's names, so an unsafe `org:` beside a refused name was a finding about the name, or about another repository, to `check`. Both -commands now ask the two first of the plan's values, right after the three -repository names (`_checked_org_and_id`). That decides #716's L4-48 as well: -an unsafe `org:` held to a repository is `unsafe-value` to both, where -`execute` said `plan-not-the-source`. And a `pins:` ENTRY that is no pin is -said as `pins:` itself is (`_read_as`): the number 7, not `7`. +commands now ask the two first of what they read of the plan, right after +its keys and its three repository names and before `_work_root` +(`_checked_org_and_id`): before an unwritable `--work-dir` too, which `check` +lists as a finding and `execute` refused before it opened the source +(RV1125's SHOULD-1). That decides #716's L4-48 as well: an unsafe `org:` held +to a repository is `unsafe-value` to both, where `execute` said +`plan-not-the-source`. And a `pins:` ENTRY that is no pin is said as `pins:` +itself is (`_read_as`): the number 7, not `7`. Each test runs `check` and then `execute --yes`, both with the same `--local-remote-dir`, on one hand-edited plan, as the issue measured, and @@ -64,6 +67,7 @@ from conftest import ADOPT, git, make_source_repo, planned, run_script from test_adopt_pins import ANSWERS, pin_entry +from test_adopt_root_symlinks import unwritable_work # noqa: F401 - a fixture UTF8 = "utf-8" PLAN_FILE = "adoption-plan.yaml" @@ -108,18 +112,21 @@ def edited(text: str, *edits: tuple[str, str]) -> str: return text -def both_on(text: str, base: Path): +def both_on(text: str, base: Path, work: Path | None = None): """`check` and then `execute --yes` on the plan `text`, written at `base`, each given the same `--local-remote-dir`: `(check's run, - execute's run)`. Neither makes the directory the legs go in.""" + execute's run)`. Neither makes the directory the legs go in. A `work` + is the `--work-dir` of both; without one, `execute` is given `base / + "work"` and `check` none.""" plan = base / PLAN_FILE plan.write_bytes(text.encode(UTF8)) remotes = base / "remotes" checked = run_script(ADOPT, "check", "--plan", str(plan), - "--local-remote-dir", str(remotes)) + "--local-remote-dir", str(remotes), + *(("--work-dir", str(work)) if work else ())) executed = run_script(ADOPT, "execute", "--plan", str(plan), "--yes", "--local-remote-dir", str(remotes), - "--work-dir", str(base / "work")) + "--work-dir", str(work or base / "work")) assert not remotes.exists(), executed.stdout + executed.stderr return checked, executed @@ -497,6 +504,57 @@ def test_the_three_repository_names_are_still_asked_first(answered, assert named_first(executed) == LEG_NAME_REFUSED, executed.stderr +@needs_filter_repo +@pytest.mark.parametrize(("edit", "refused"), + [(BAD_ID, ID_REFUSED), (BAD_ORG, ORG_REFUSED)], + ids=["id", "org"]) +def test_the_org_or_the_id_is_named_before_an_unwritable_work_dir( + answered, tmp_path, unwritable_work, edit, refused): + """RV1125's SHOULD-1: `check` lists a `--work-dir` this user cannot write + in as a finding (#524), and `execute` refuses it before it opens the + source, so both asked the plan's `org:` and `id:` after it, and a bad + `id:` or `org:` beside one was `plan-bad-id` or `unsafe-value` to + `check` and `adopt-work-dir-unwritable` to `execute`. Both ask the two + right after the plan's keys and its three repository names, before + `_work_root`, so each is refused for the organisation or the id, exit 2, + in the same words, and neither says a word of the directory. The tests + skip where `chmod` cannot keep this user from writing a directory.""" + checked, executed = both_on(edited(answered, edit), tmp_path, + unwritable_work) + assert checked.returncode == 2, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert named_first(checked).startswith(refused), checked.stderr + assert named_first(executed) == named_first(checked), executed.stderr + for run in (checked, executed): + assert "adopt-work-dir-unwritable" not in run.stderr, run.stderr + + +@pytest.mark.parametrize(("edit", "refused"), + [(BAD_ID, ID_REFUSED), (BAD_ORG, ORG_REFUSED)], + ids=["id", "org"]) +def test_check_asks_the_org_and_the_id_before_it_says_anything_else( + answered, tmp_path, edit, refused): + """RV1125's NIT-2: `check` prints its findings only at its end, so none + of them tells a refusal asked at its head from one asked later, and the + first row of this section could not. What it does print is the Triad + advisory, on stdout, once it has made the work directory and opened the + source, so a refusal asked after that comes after the advisory. Asked at + its head, a bad `id:` or `org:` is refused before anything is read, made + or said: nothing is on stdout, and stderr starts with the refusal. The plan + unedited is the control: it says the advisory first.""" + plan = tmp_path / PLAN_FILE + remotes = ("--local-remote-dir", str(tmp_path / "remotes")) + plan.write_bytes(answered.encode(UTF8)) + said = run_script(ADOPT, "check", "--plan", str(plan), *remotes) + assert said.returncode == 0, said.stdout + said.stderr + assert said.stdout.startswith("triad "), said.stdout + plan.write_bytes(edited(answered, edit).encode(UTF8)) + checked = run_script(ADOPT, "check", "--plan", str(plan), *remotes) + assert checked.returncode == 2, checked.stdout + checked.stderr + assert checked.stdout == "", checked.stdout + assert named_first(checked).startswith(refused), checked.stderr + + # --- a `pins:` entry that is no pin, said as its reader read it ------------- #: `(the plan's `pins:` block, what entry 1 was read as, as `_read_as` says From 90eca414488f0433758b189523ef82c8412bd184 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 22 Oct 2026 21:54:15 +0000 Subject: [PATCH 3/5] adopt-project.py: `check` and `execute` read one list of a plan's problems against its source, in the order a person mends them (another repository, a refused name, a moved source, an unanswered entry), so `check`'s first finding is `execute`'s refusal for each pair (#1113) `check` listed a moved source first, then the plan's entries, then another repository and then the names; `execute` refused another repository, a refused name, an unanswered entry and then a moved source. So for five of the six pairs of the four, `check`'s first finding was not `execute`'s refusal: the four of #1113's table, and a refused name beside a moved source. `_against_its_source` asks the four in one order, and both commands read it: `execute` refuses its first problem, and `check` lists each, in that order and in its own words, before the split branch and the entries. Each of the first three is mended by running `plan` again, which takes back every answer, so an unanswered entry is named last; another repository comes first, before the naming policy or any remote is asked (#499); and a refused name is mended by a `plan` that reads the source as it is now, so it comes before a moved source. `_refuse_an_unrunnable_plan` asks a moved source before an unanswered entry. No refusal's code or wording changes: only which is named first. tests/test_adopt_plan_against_its_source.py runs both commands on one plan per pair, on all four, and on each of the four alone. In tests/test_adopt_relative_submodule_url.py, six rows hand-edit a plan's `org:` to another organisation beside a relative url, and pinned `check`'s old order, the url first. Such a plan is for another repository, which `execute` refuses first (#499), so the rows now expect `plan-not-the-source` first to `check` too. A new row runs `check`, with and without `--local-remote-dir`, and `execute` on one such plan, and asserts that each names it first and that no leg is made. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 204 +++++++++++++++----- tests/test_adopt_plan_against_its_source.py | 144 ++++++++++++++ tests/test_adopt_relative_submodule_url.py | 35 +++- 3 files changed, 326 insertions(+), 57 deletions(-) create mode 100644 tests/test_adopt_plan_against_its_source.py diff --git a/adopt-project.py b/adopt-project.py index 4300f48c..52dd7c39 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -12338,17 +12338,18 @@ def cmd_check(args) -> int: _print_triad_advisory(source.path, plan.source_commit or source.commit) - if plan.source_commit and source.commit != plan.source_commit: - # `commit:` is a plan value people and AIs edit: sliced first, so the - # spelling stays bounded, and `_shown`, so a real hash reads as it did - # and a line break or an ESC in it (#328) cannot leave the line. - findings.append( - f"FINDING plan-stale: the plan was written against " - f"{_shown(plan.source_commit[:12])} but " - f"{_spelled(source.branch)} is now at {source.commit[:12]}. " - "Re-run `plan`: a coverage check " - "against a tree that has moved proves nothing about the tree that " - "will be split.") + # THE PLAN AGAINST ITS SOURCE, FIRST OF WHAT IT LISTS (#1113): each + # problem of `_against_its_source`, the one list `execute` refuses the + # first of, in its order and in this command's words, before the split + # branch and the entries: another repository, a refused name and a moved + # source. An unanswered entry is listed with the entries, after them. + names = plan.names() + pins = set(plan.pins) + unmended = list(_against_its_source(plan, source, names, naming, pins)) + for problem in unmended: + findings.extend(problem.found) + named = not {UNCLASSIFIED, ROLE_MISMATCH}.intersection( + problem.refusal.code for problem in unmended) # Before the entries (#320): the history scan their findings make reads # a split branch the source keeps, and the branch is named first. @@ -12363,41 +12364,15 @@ def cmd_check(args) -> int: entry_findings, remediation, notes = _entry_findings(plan, source, local_remote_dir) findings.extend(entry_findings) - remediation = taken_remediation + remediation + # What is printed once after the findings, in their order. + remediation = [fix for problem in unmended for fix in problem.fixes] + ( + taken_remediation + remediation) - names = plan.names() - pins = set(plan.pins) - # The plan names the source, or `execute` acts on another repository - # (#499): said before the names' own findings, as `execute` refuses it. - another = _not_the_source(plan, source, names) - if another: - findings.append(f"FINDING {PLAN_NOT_THE_SOURCE}: {another}") - remediation.append(_own_name_remediation( - _held_repository(plan, source), naming, pins, "")) - # THE SOURCE IS HANDED TO `_check_names` WHERE `execute` HANDS IT (#716): - # when the plan does not name another repository, so that a refusal of - # the assembly root's own name says to rename it first, as `execute` - # says it on the same plan. Where the plan does name another, - # `execute` stops at `plan-not-the-source` and never asks the names, and - # a refusal of the plan's name is then about a name that is not the - # source's: `_own_name_remediation` has said what to do. - in_place = source.repository if not another else None - try: - _check_names(naming, names, pins, None if another else source) + if named: for role, name in names.items(): found = naming.classify(name, role, pins) print(f" {role:<9} {name:<28} {found.family}" + (f"/{found.role}" if found.role else "")) - except Refusal as exc: - findings.append(f"FINDING {exc.code}: {exc.detail}") - # Only the rename-first remedy is printed, once after the findings: - # it is the one `check` was missing. `--project takes ...` is not, - # and `check` never printed it (#716). It says the rule its new name - # follows first, as `_check_names` says it (#902). - if in_place is not None and exc.remediation == _name_remedy( - exc.code, in_place): - remediation.append(exc.remediation) - else: # WHAT THE DISK OR GITHUB WILL NOT HOLD (#863): the names, the # organisation and the topic, `execute`'s one list, which it refuses # the first of before either leg exists, each a finding here under @@ -12509,16 +12484,14 @@ def _refuse_an_unrunnable_plan(plan: Plan, source: Source) -> None: An unanswered question is never an implicit `root`, and a plan written against a tree that has since moved proves nothing about the tree that would be split — which is how a path goes missing. + + THE MOVED SOURCE FIRST (#1113), as `_against_its_source` asks the two, + the last of its four problems: a moved source is mended by running + `plan` again, and the fresh plan takes back every answer given before, + so an answer given first to an entry of the stale one is lost. `check` + listed the moved source first, and `execute` refused the unanswered + entry first, so the two named different problems first. """ - unresolved = [e for e in plan.entries if e.get("leg") is None] - if unresolved: - raise Refusal( - "plan-unresolved", - f"{len(unresolved)} path(s) still have `leg: null`: " - + _listed([str(e.get("path")) for e in unresolved[:8]]), - "Remediation: answer each entry's `question:` by setting `leg:` " - "and adding a `resolution:` line, then run `check`. An unanswered " - "question is never an implicit `root`.") if plan.source_commit and source.commit != plan.source_commit: # `commit:` is `_shown` as `check` shows it (#328). raise Refusal( @@ -12529,6 +12502,125 @@ def _refuse_an_unrunnable_plan(plan: Plan, source: Source) -> None: "Remediation: re-run `plan`, re-answer anything new, then " "`check`. Splitting a tree the plan has not seen is how a path " "goes missing.") + unresolved = [e for e in plan.entries if e.get("leg") is None] + if unresolved: + raise Refusal( + "plan-unresolved", + f"{len(unresolved)} path(s) still have `leg: null`: " + + _listed([str(e.get("path")) for e in unresolved[:8]]), + "Remediation: answer each entry's `question:` by setting `leg:` " + "and adding a `resolution:` line, then run `check`. An unanswered " + "question is never an implicit `root`.") + + +class _Unmended(NamedTuple): + """One problem of a plan against its source, as `_against_its_source` + yields it, in each command's own words: the `Refusal` `execute` refuses + it with, and the FINDING lines `check` lists it as and what `check` + prints once after its findings.""" + refusal: Refusal + found: tuple = () + fixes: tuple = () + + +def _against_its_source(plan: Plan, source: Source, names: dict, + naming: NamingPolicy, pins: set[str]): + """Each problem of the plan against its source (`_Unmended`), in the + order a person mends them (#1113): a plan for another repository + (`plan-not-the-source`), a name the naming policy refuses + (`_check_names`), and then `_refuse_an_unrunnable_plan`'s two, a source + that has moved on since `plan` (`plan-stale`) and an unanswered entry + (`plan-unresolved`), of which it refuses the first. + + ONE ORDER, TWO READERS (#1113). `execute` refuses the first problem, + once the work directory and the source are opened; `check` lists + each, in this order, before the split branch and the plan's entries. + Each command had an order of its own: `check` listed a moved source + first, then the entries, each unanswered one among them, then another + repository and then the names; `execute` refused another repository, + then a refused name, then an unanswered entry and then a moved source. + So for five of the six pairs of the four, `check`'s first finding was + not `execute`'s refusal: a moved source beside an unanswered entry, + another repository beside either, and a refused name beside either. + Read from here by both, it is. A plan with one of the four is named as + before by each command, in the same words. + + IN THE ORDER A PERSON MENDS THEM. Each of the first three is mended by + running `plan` again, as its remediation says, and a fresh plan takes + back every answer given before, so the unanswered entry, mended by an + answer in the plan, is named last: an answer given first is lost. Of + the three: + - another repository first. Nothing in such a plan is this source's + own: it is mended by `plan` under the source's own name, or from the + source it is about (`_own_name_remediation`), and its commit and its + entries are of what it was written from. And `execute` asks the naming + policy nothing about the plan's names, and no remote anything, before + it refuses it (`_refuse_a_plan_for_another_repository`, #499): this + is a generator, so `execute` asks nothing after the first problem. + - then a refused name, as `execute` asked it: it is mended by `plan` + given another `--project`, or after the source is renamed on GitHub + (`_rename_first`), and that run reads the source as it is now, so it + mends a moved source too, where `plan` run again as it was for the + moved source would leave the name to mend. + - then a moved source, mended by `plan` run again as it was. + + `check` SAYS EACH IN ITS OWN WORDS, as it did: another repository with + no plan path, and with `_own_name_remediation` saying nothing of legs, + which `check` never makes; a refused name with the rename-first remedy + only where the source is adopted in place under that name (#716); a + moved source as `check` said it; and an unanswered entry not here but + with the entries, each by its path (`_leg_findings`), after these. + The plan's `org:` and `id:` are asked before all four, by both + (`_checked_org_and_id`, #1020). + """ + # The plan names the source, or `execute` acts on another repository + # (#499): said before the names' own findings, as `execute` refuses it. + another = None + try: + _refuse_a_plan_for_another_repository(plan, source, names, naming, + pins) + except Refusal as exc: + another = _not_the_source(plan, source, names) + yield _Unmended(exc, (f"FINDING {PLAN_NOT_THE_SOURCE}: {another}",), + (_own_name_remediation(_held_repository(plan, source), + naming, pins, ""),)) + # THE SOURCE IS HANDED TO `_check_names` WHERE `execute` HANDS IT (#716): + # when the plan does not name another repository, so that a refusal of + # the assembly root's own name says to rename it first, as `execute` + # says it on the same plan. Where the plan does name another, + # `execute` stops at `plan-not-the-source` and never asks the names, and + # a refusal of the plan's name is then about a name that is not the + # source's: `_own_name_remediation` has said what to do. + in_place = source.repository if not another else None + try: + _check_names(naming, names, pins, None if another else source) + except Refusal as exc: + # Only the rename-first remedy is printed by `check`, once after the + # findings: it is the one `check` was missing. `--project takes ...` + # is not, and `check` never printed it (#716). It says the rule its + # new name follows first, as `_check_names` says it (#902). + yield _Unmended( + exc, (f"FINDING {exc.code}: {exc.detail}",), + (exc.remediation,) if in_place is not None + and exc.remediation == _name_remedy(exc.code, in_place) else ()) + try: + _refuse_an_unrunnable_plan(plan, source) + except Refusal as exc: + if exc.code != "plan-stale": + # Listed by `check` with the entries, each by its path. + yield _Unmended(exc) + else: + # `commit:` is a plan value people and AIs edit: sliced first, + # so the spelling stays bounded, and `_shown`, so a real hash + # reads as it did and a line break or an ESC in it (#328) cannot + # leave the line. + yield _Unmended(exc, ( + f"FINDING plan-stale: the plan was written against " + f"{_shown(plan.source_commit[:12])} but " + f"{_spelled(source.branch)} is now at " + f"{source.commit[:12]}. Re-run `plan`: a coverage check " + "against a tree that has moved proves nothing about the tree " + "that will be split.",)) #: The adoption's own ruleset hint (#177). `RULESET_HINT` is the scaffold's, @@ -14980,9 +15072,15 @@ def cmd_execute(args) -> int: _require_filter_repo() pins = set(plan.pins) - _refuse_a_plan_for_another_repository(plan, source, names, naming, pins) - _check_names(naming, names, pins, source) - _refuse_an_unrunnable_plan(plan, source) + # THE PLAN AGAINST ITS SOURCE (#1113): the first problem of + # `_against_its_source`, which `check` lists first, in the order a person + # mends them: another repository, refused before the naming policy is + # asked about the plan's names or any remote anything (#499), then a + # refused name, a moved source and an unanswered entry. + unmended = next(_against_its_source(plan, source, names, naming, pins), + None) + if unmended is not None: + raise unmended.refusal # THE PLAN IS UNTRUSTED INPUT. It is a file a human or an AI edited, and # every value below becomes an argument to `git` or `gh`, so each one is diff --git a/tests/test_adopt_plan_against_its_source.py b/tests/test_adopt_plan_against_its_source.py new file mode 100644 index 00000000..6a89fe16 --- /dev/null +++ b/tests/test_adopt_plan_against_its_source.py @@ -0,0 +1,144 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`check` and `execute` name the same first problem of a plan against its +source (#1113): a plan for another repository (`plan-not-the-source`), a +name the naming policy refuses (`naming-unclassified`), a source that has +moved on since `plan` (`plan-stale`) and an unanswered entry +(`plan-unresolved`). + +ON #1125'S HEAD 9549006f, with a stub `gh` and local remotes, as #1113 +measured them on main at 16f9a0e81, `check` listed a moved source first, +then the plan's entries, then another repository and then the names, and +`execute` refused another repository, then a refused name, then an +unanswered entry and then a moved source. So five of the six pairs of these +four were named differently by the two commands, `check`'s first finding +against `execute`'s refusal: +- a moved source and an unanswered entry: `plan-stale` against + `plan-unresolved`; +- another repository and an unanswered entry: `plan-unresolved` against + `plan-not-the-source`; +- another repository and a moved source: `plan-stale` against + `plan-not-the-source`; +- a refused name and an unanswered entry: `plan-unresolved` against + `naming-unclassified`; +- a refused name and a moved source: `plan-stale` against + `naming-unclassified`. + +The first four are #1113's table; the fifth is not in it, and the same rule +decides it. The sixth pair, another repository beside a refused name, was +`plan-not-the-source` to both, and still is. + +ONE RULE, `_against_its_source`, read by both commands: `execute` refuses +the first of its problems, and `check` lists each of them, in its order, +before the split branch and the entries. The order is the one a person +mends them in: another repository, a refused name, a moved source, an +unanswered entry. Each of the first three is mended by running `plan` again, +and a fresh plan takes back every answer given before, so an unanswered +entry, mended by an answer in the plan, comes last. + +Each row runs `check` and then `execute --yes` on one hand-edited plan, both +with the same `--local-remote-dir`, so no `gh` runs and no remote is asked, +and asserts that no leg was made. `execute` refuses every plan here before +it reads a remote: it asks for `git filter-repo` first, so the rows skip +where it is not installed. +""" + +from __future__ import annotations + +import pytest + +from test_adopt_plan_value_order import ( + ANOTHER_ORG, REFUSED_NAME, both_on, edited, named_first, + needs_filter_repo, planned_with) + +ANOTHER = "plan-not-the-source" +NAME = "naming-unclassified" +MOVED = "plan-stale" +UNANSWERED = "plan-unresolved" + +#: The order both commands name the four in, the order a person mends them +#: in (#1113). +MENDED_FIRST = (ANOTHER, NAME, MOVED, UNANSWERED) + +#: Each problem: `(the hand edits it takes, what the source and the plan +#: hold for it)`, as `planned_with` reads `holds`. Another repository is an +#: `org:` other than the source's, in a plan held to `testorg/Northwind` by +#: the source's `origin`; a refused name is `legs.spec: bad_name`; a moved +#: source is a commit made after `plan`; an unanswered entry is `examples/` +#: left at `leg: null`. +PROBLEM = { + ANOTHER: ((ANOTHER_ORG,), {"held"}), + NAME: ((REFUSED_NAME,), set()), + MOVED: ((), {"moved"}), + UNANSWERED: ((), {"unanswered"}), +} + + +def holding(base, *problems: str) -> str: + """The text of a plan for a new source at `base` that holds each of + `problems`, and nothing else wrong with it.""" + edits = [edit for problem in problems for edit in PROBLEM[problem][0]] + holds = set().union(*(PROBLEM[problem][1] for problem in problems)) + return edited(planned_with(base, holds), *edits) + + +def code_of(line: str) -> str: + """The code of a `REFUSED : ...` or `FINDING : ...` line.""" + return line.split()[1].rstrip(":") if line else "" + + +def listed(checked) -> list[str]: + """The codes of `MENDED_FIRST` that `check` listed, each once, in the + order it listed them first.""" + codes = [code_of(line) for line in checked.stderr.splitlines() + if line.startswith("FINDING ")] + return list(dict.fromkeys(code for code in codes if code in MENDED_FIRST)) + + +#: The pairs, and the four together, by test id: `(the problems the plan +#: holds, what both commands name first)`. Each but `another-and-name` +#: was named differently by the two commands on 9549006f. +PAIRS = { + "moved-and-unanswered": ((MOVED, UNANSWERED), MOVED), + "another-and-unanswered": ((ANOTHER, UNANSWERED), ANOTHER), + "another-and-moved": ((ANOTHER, MOVED), ANOTHER), + "name-and-unanswered": ((NAME, UNANSWERED), NAME), + "name-and-moved": ((NAME, MOVED), NAME), + "another-and-name": ((ANOTHER, NAME), ANOTHER), + "all-four": (MENDED_FIRST, ANOTHER), +} + + +@needs_filter_repo +@pytest.mark.parametrize("case", sorted(PAIRS)) +def test_check_and_execute_name_the_same_first_problem(tmp_path, case): + """`check` names first the problem `execute` refuses first, the first + of `MENDED_FIRST` the plan holds: `check` lists every one the plan + holds, exit 1, in that order, and `execute` refuses the first, exit 2, + before any leg exists.""" + problems, first = PAIRS[case] + checked, executed = both_on(holding(tmp_path, *problems), tmp_path) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert (code_of(named_first(checked)), + code_of(named_first(executed))) == (first, first), ( + checked.stderr + executed.stderr) + assert named_first(executed).startswith(f"REFUSED {first}: "), ( + executed.stderr) + assert listed(checked) == [problem for problem in MENDED_FIRST + if problem in problems], checked.stderr + + +@needs_filter_repo +@pytest.mark.parametrize("problem", MENDED_FIRST) +def test_each_problem_alone_is_named_by_its_own_code(tmp_path, problem): + """THE CONTROLS, which pass on 9549006f too: a plan that holds one of + the four is that one finding to `check`, exit 1, and that one refusal + to `execute`, exit 2, as before.""" + checked, executed = both_on(holding(tmp_path, problem), tmp_path) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert named_first(checked).startswith(f"FINDING {problem}: "), ( + checked.stderr) + assert named_first(executed).startswith(f"REFUSED {problem}: "), ( + executed.stderr) + assert listed(checked) == [problem], checked.stderr diff --git a/tests/test_adopt_relative_submodule_url.py b/tests/test_adopt_relative_submodule_url.py index b959af3f..307a62ea 100644 --- a/tests/test_adopt_relative_submodule_url.py +++ b/tests/test_adopt_relative_submodule_url.py @@ -178,7 +178,8 @@ def its_org(origin: str) -> str: def in_another_org(plan: Path, org: str) -> Path: """`plan` with its `org:` edited by hand to `org`: legs in another organisation than the source's, which no `plan` writes since #499, so - `check` finds the plan `plan-not-the-source` beside anything else.""" + `check` finds the plan `plan-not-the-source` beside anything else, and + lists it first, as `execute` refuses it before anything else (#1113).""" text = plan.read_bytes().decode() line = f"\norg: {ORG}\n" assert line in text, text @@ -292,7 +293,8 @@ def test_check_keeps_a_relative_url_its_github_leg_resolves_alike( remediation once after it. A url with a scheme is printed through `redacted`, which hides its user too (`ssh://***@github.com:22/...`). Only a hand edit puts the legs in another organisation than the - source's, which `check` finds too (#499).""" + source's, which `check` finds too (#499), first, as `execute` refuses + it first (#1113).""" source = source_with(tmp_path, SIBLING, origin) same = check(planned(source, tmp_path)) passes(same) @@ -302,7 +304,7 @@ def test_check_keeps_a_relative_url_its_github_leg_resolves_alike( result = check(in_another_org( planned(source, tmp_path, name=f"plan-{OTHER_ORG}"), OTHER_ORG)) assert result.returncode == 1, result.stderr + result.stdout - relative, (other, _) = found(result.stderr) + (other, _), relative = found(result.stderr) assert relative == (RELATIVE, moved( adopter, SIBLING, adopter.redacted(origin), ITS_ORIGIN, f"to `{adopter.redacted(resolved)}`", CODE_LEG.format(org=OTHER_ORG), @@ -435,7 +437,7 @@ def test_a_credential_in_the_source_remote_is_never_printed(adopter, result = check(in_another_org( planned(source, tmp_path, name=f"plan-{OTHER_ORG}"), OTHER_ORG)) assert secret not in result.stderr + result.stdout - relative, (other, _) = found(result.stderr) + (other, _), relative = found(result.stderr) assert relative == (RELATIVE, moved( adopter, SIBLING, "https://***@github.com/testorg/Thing.git", ITS_ORIGIN, @@ -812,3 +814,28 @@ def test_check_and_execute_given_one_local_remote_dir_agree(tmp_path, local): assert cloned.returncode == 0, cloned.stderr assert (clone / "code" / DEPENDENCY / VALUE).read_text() \ == DEPENDENCY_TREE[VALUE] + + +@needs_filter_repo +def test_check_names_another_repository_first_as_execute_refuses_it( + tmp_path): + """A plan for another repository beside a relative url its leg resolves + elsewhere (#1113): `execute` refuses `plan-not-the-source` before it + resolves any url (#499), so `check`, given the same `--local-remote-dir` + or none, lists that first and the url after it. On #1125's head `check` + listed the url first, so its first finding was not `execute`'s refusal. + Neither makes a leg.""" + source = source_with(tmp_path, SIBLING, HTTPS_ORIGIN) + plan = in_another_org( + planned(source, tmp_path, name=f"plan-{OTHER_ORG}"), OTHER_ORG) + remotes = tmp_path / REMOTES + for checked in (check(plan), check(plan, remotes)): + assert checked.returncode == 1, checked.stderr + checked.stdout + assert [code for code, _ in found(checked.stderr)] \ + == [NOT_THE_SOURCE, RELATIVE], checked.stderr + result = execute(plan, tmp_path / WORK, remotes) + assert result.returncode == 2, result.stderr + result.stdout + [refused] = [line for line in result.stderr.splitlines() + if line.startswith("REFUSED ")] + assert refused.startswith(f"REFUSED {NOT_THE_SOURCE}: "), refused + assert not remotes.exists(), "a refused plan made the remotes directory" From 791106c2be694e333e92b0b3acc38f254dc80dce Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Fri, 23 Oct 2026 21:32:59 +0000 Subject: [PATCH 4/5] tests/test_adopt_plan_against_its_source.py: a row for each of the rule's problems beside the split branch taken and beside an unsafe `tracking_branch:` pins that `check` lists the rule's problem, and its remediation, first and `execute` refuses it first, so the rule moved after either finding fails a row (#1113) RV1210 measured two placements of the rule that no row pinned, each bringing back a disagreement #1210 fixes: - `check` listing the rule's problems after the split branch: another repository, a refused name and a moved source beside a branch `adopt` in the source, as `test_adopt_split_branch.py` makes it (`another-and-taken`, `name-and-taken`, `moved-and-taken`); - `execute` asking the rule after `_checked_plan_values`, `_repository_urls` and `_refuse_an_unwritable_clone_url`: each of the four beside `tracking_branch: "a b"`, which `BAD_TRACKING_BRANCH` from `test_adopt_plan_value_order.py` edits in (`another-and-tracking`, `name-and-tracking`, `moved-and-tracking`, `unanswered-and-tracking`). Each row asserts that `check` exits 1 and names the rule's problem first, and lists the ordinary finding after it, and that `execute` exits 2 and refuses the rule's problem first. `another-and-taken` also asserts that `check`'s first `Remediation:` line is the rule's (NIT-4), so the remediation lines follow the findings' order. `unanswered-and-taken` stays out: it is #1197's. Test only; adopt-project.py is unchanged. Co-Authored-By: Claude Sonnet 5.5 --- tests/test_adopt_plan_against_its_source.py | 89 +++++++++++++++++++-- 1 file changed, 84 insertions(+), 5 deletions(-) diff --git a/tests/test_adopt_plan_against_its_source.py b/tests/test_adopt_plan_against_its_source.py index 6a89fe16..08d35e57 100644 --- a/tests/test_adopt_plan_against_its_source.py +++ b/tests/test_adopt_plan_against_its_source.py @@ -35,6 +35,15 @@ and a fresh plan takes back every answer given before, so an unanswered entry, mended by an answer in the plan, comes last. +WHERE THE RULE SITS AMONG THE OTHER FINDINGS. `check` lists the rule's +problems before the split branch and the entries, and `execute` asks the +rule before every refusal `check` lists after it and before the plan's +values (`_checked_plan_values`, #499). A row beside an ordinary finding pins +each: the source keeps a branch `adopt`, which `check` finds as +`plan-split-branch-taken` after the rule's problems (`check` lists it +before the entries), or the plan's `tracking_branch:` is a value `execute` +refuses as `unsafe-value` once the rule's problem is not in the way. + Each row runs `check` and then `execute --yes` on one hand-edited plan, both with the same `--local-remote-dir`, so no `gh` runs and no remote is asked, and asserts that no leg was made. `execute` refuses every plan here before @@ -46,14 +55,20 @@ import pytest +from conftest import git from test_adopt_plan_value_order import ( - ANOTHER_ORG, REFUSED_NAME, both_on, edited, named_first, - needs_filter_repo, planned_with) + ANOTHER_ORG, BAD_TRACKING_BRANCH, REFUSED_NAME, both_on, edited, + findings_of, named_first, needs_filter_repo, planned_with) ANOTHER = "plan-not-the-source" NAME = "naming-unclassified" MOVED = "plan-stale" UNANSWERED = "plan-unresolved" +#: Two ordinary findings, which `check` lists after the rule's problems and +#: `execute` refuses only once none of them is in the way: the split branch +#: taken, and an unsafe `tracking_branch:`. +TAKEN = "plan-split-branch-taken" +TRACKING = "unsafe-value" #: The order both commands name the four in, the order a person mends them #: in (#1113). @@ -72,13 +87,27 @@ UNANSWERED: ((), {"unanswered"}), } +#: Each ordinary finding, as `PROBLEM` reads it. The split branch taken is +#: a branch `adopt` the source keeps, made as `test_adopt_split_branch.py` +#: makes it, and an unsafe `tracking_branch:` is a hand edit. +ORDINARY = { + TAKEN: ((), {"taken"}), + TRACKING: ((BAD_TRACKING_BRANCH,), set()), +} + def holding(base, *problems: str) -> str: """The text of a plan for a new source at `base` that holds each of `problems`, and nothing else wrong with it.""" - edits = [edit for problem in problems for edit in PROBLEM[problem][0]] - holds = set().union(*(PROBLEM[problem][1] for problem in problems)) - return edited(planned_with(base, holds), *edits) + held = {**PROBLEM, **ORDINARY} + edits = [edit for problem in problems for edit in held[problem][0]] + holds = set().union(*(held[problem][1] for problem in problems)) + text = edited(planned_with(base, holds), *edits) + if "taken" in holds: + # `planned_with` makes the source at `base / "Thing"`; the branch + # comes after `plan`, which would find it taken itself. + git("branch", "adopt", cwd=base / "Thing") + return text def code_of(line: str) -> str: @@ -142,3 +171,53 @@ def test_each_problem_alone_is_named_by_its_own_code(tmp_path, problem): assert named_first(executed).startswith(f"REFUSED {problem}: "), ( executed.stderr) assert listed(checked) == [problem], checked.stderr + + +#: The rule's problem beside an ordinary finding, by test id: `(the rule's +#: problem, the ordinary finding)`. Both commands name the rule's problem +#: first. `unanswered-and-taken` is left out: `check` lists an unanswered +#: entry with the entries, after the split branch, and `execute` refuses it +#: first, so the two disagree on it before and after this fix (#1197). +WITH_AN_ORDINARY_FINDING = { + "another-and-taken": (ANOTHER, TAKEN), + "name-and-taken": (NAME, TAKEN), + "moved-and-taken": (MOVED, TAKEN), + "another-and-tracking": (ANOTHER, TRACKING), + "name-and-tracking": (NAME, TRACKING), + "moved-and-tracking": (MOVED, TRACKING), + "unanswered-and-tracking": (UNANSWERED, TRACKING), +} + +#: The first `Remediation:` line `check` prints, where a row pins it: the +#: remediation lines follow the findings' order, so the rule's come first. +FIRST_REMEDIATION = { + "another-and-taken": "Remediation: Run `plan` again with `--project", +} + + +@needs_filter_repo +@pytest.mark.parametrize("case", sorted(WITH_AN_ORDINARY_FINDING)) +def test_the_rules_problem_is_named_before_an_ordinary_finding(tmp_path, + case): + """`check` lists the rule's problem before the split branch taken or an + unsafe `tracking_branch:`, and lists that finding too, and `execute` + refuses the rule's problem before it: both name it first, `check` exit + 1 and `execute` exit 2, before any leg exists. Where `FIRST_REMEDIATION` + names one, `check`'s first `Remediation:` line is the rule's.""" + problem, ordinary = WITH_AN_ORDINARY_FINDING[case] + checked, executed = both_on(holding(tmp_path, problem, ordinary), + tmp_path) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert (code_of(named_first(checked)), + code_of(named_first(executed))) == (problem, problem), ( + checked.stderr + executed.stderr) + assert named_first(executed).startswith(f"REFUSED {problem}: "), ( + executed.stderr) + assert ordinary in [code_of(line) for line in findings_of(checked)][1:], ( + checked.stderr) + assert listed(checked) == [problem], checked.stderr + if case in FIRST_REMEDIATION: + fixes = [line for line in checked.stderr.splitlines() + if line.startswith("Remediation:")] + assert fixes[0].startswith(FIRST_REMEDIATION[case]), checked.stderr From e6f2526e44dd49479045369232846dae3fabb939 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 10 Nov 2026 12:58:24 +0000 Subject: [PATCH 5/5] adopt-project.py: `check`, given no `--local-remote-dir`, asks GitHub nothing about the two legs of a plan for another repository and prints no NOTE that `execute` asks again, as `execute` refuses `plan-not-the-source` first and asks nothing; under `--local-remote-dir` it still asks the disk (#1232) On #1210's head 791106c2, `check` on a plan for another repository, given no `--local-remote-dir`, ran `gh repo view /-spec --json nameWithOwner` and the same for `-code`, under the plan's own `org:`, and where gh could not answer it printed a NOTE that `execute` asks again before it makes either leg. `execute` refuses `plan-not-the-source` first and asks no remote anything (#499). `cmd_check` now skips the leg question (`_leg_remote_findings`) there, as it skips it after a finding in `RENAMES_THE_LEGS`. The source's own plan is asked as before. Under `--local-remote-dir` the disk is still asked beside `plan-not-the-source`: it asks no remote, and a leg's bare repository is `/.git`, which no `org:` is part of. Measured, the `leg-remote-exists` it lists beside `plan-not-the-source` names the very path the source's own plan, the one the remedy writes, is refused for. Tests: tests/test_adopt_plan_against_its_source.py gains 2 rows that fail on 791106c2 (a plan for another repository by its `org:` and by its `legs.assembly:`, under a `gh` that logs each call), a row for the choice under `--local-remote-dir`, and a control for the source's own plan. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 14 ++ tests/test_adopt_plan_against_its_source.py | 138 +++++++++++++++++++- 2 files changed, 149 insertions(+), 3 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 52dd7c39..9ada2c19 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -12397,9 +12397,23 @@ def cmd_check(args) -> int: # shows both (#871's review, S1), as it is asked beside the # manifest's and the split branch's findings, which `execute` # refuses first too. + # NOR ASKED OF GITHUB BESIDE A PLAN FOR ANOTHER REPOSITORY (#1232): + # `execute` refuses `plan-not-the-source` before it asks any remote + # anything (#499), so it never asks GitHub about the two legs such + # a plan names, where `check` ran `gh repo view` of each, under the + # plan's `org:`, another organisation's where that is the edit, and + # its NOTE said `execute` asks again. Under `--local-remote-dir` it + # is still asked: the disk answers, nothing leaves the machine, and + # a leg's bare repository is `/.git`, which no `org:` is + # part of, so the plan the remedy writes under the source's own + # name is asked of the same two paths wherever it keeps the legs' + # names, and one round shows both. # A question gh cannot answer is a NOTE here, not a finding. + another = PLAN_NOT_THE_SOURCE in {problem.refusal.code + for problem in unmended} legs, fixes, unasked = ( ([], [], []) if RENAMES_THE_LEGS.intersection(held_codes) + or (another and local_remote_dir is None) else _leg_remote_findings(plan, names, local_remote_dir)) findings.extend(legs) remediation.extend(fixes) diff --git a/tests/test_adopt_plan_against_its_source.py b/tests/test_adopt_plan_against_its_source.py index 08d35e57..d3d3fbdc 100644 --- a/tests/test_adopt_plan_against_its_source.py +++ b/tests/test_adopt_plan_against_its_source.py @@ -49,16 +49,30 @@ and asserts that no leg was made. `execute` refuses every plan here before it reads a remote: it asks for `git filter-repo` first, so the rows skip where it is not installed. + +THE LEG QUESTION BESIDE ANOTHER REPOSITORY (#1232). The rows of #1232 are +the exception to the paragraph above: each runs `check`, and `execute` +where it is refused first, given no `--local-remote-dir` or one that holds +a leg's bare repository, with a `gh` of its own ahead of the session's +guard that logs each call and exits 97. On 791106c2 `check`, given no +`--local-remote-dir`, ran `gh repo view` of both legs of a plan for another +repository, and its NOTE said `execute` asks again, where `execute` refuses +`plan-not-the-source` first and asks nothing. Now it asks GitHub nothing +there. The source's own plan is asked as before, and under +`--local-remote-dir` the disk still is, beside `plan-not-the-source`: a +leg's bare repository is `/.git`, which no `org:` is part of. """ from __future__ import annotations +import os + import pytest -from conftest import git +from conftest import ADOPT, git, run_script from test_adopt_plan_value_order import ( - ANOTHER_ORG, BAD_TRACKING_BRANCH, REFUSED_NAME, both_on, edited, - findings_of, named_first, needs_filter_repo, planned_with) + ANOTHER_ORG, BAD_TRACKING_BRANCH, PLAN_FILE, REFUSED_NAME, both_on, + edited, findings_of, named_first, needs_filter_repo, planned_with) ANOTHER = "plan-not-the-source" NAME = "naming-unclassified" @@ -173,6 +187,124 @@ def test_each_problem_alone_is_named_by_its_own_code(tmp_path, problem): assert listed(checked) == [problem], checked.stderr +#: How `check` names, in a NOTE on stdout, a question about a leg's +#: repository that gh could not answer (#798). +COULD_NOT_ASK = "NOTE `check` could not ask GitHub whether " +#: A leg's bare repository already there, under `--local-remote-dir`. +LEG_TAKEN = "leg-remote-exists" +#: The two ways a plan names another repository than its source's (#499): +#: an `org:` other than the source's, and a `legs.assembly:` other than its +#: name, which keeps the source's `org:` and the legs' names, so `check` +#: asked GitHub about the very legs the plan the remedy writes names. +ANOTHER_REPOSITORY = { + "org": ANOTHER_ORG, + "assembly": ("\n assembly: Northwind\n", "\n assembly: Elsewhere\n"), +} + +needs_a_posix_gh = pytest.mark.skipif( + os.name == "nt", reason="the logging gh is a #! script") + + +def with_a_logging_gh(log, *args: str): + """`adopt-project.py ` with a `gh` first on PATH, ahead of the + session's guard, that appends each call's arguments to `log`, one line + each, and exits 97, as #1232 measured: `(the run, each call it made)`.""" + stub = log.parent / "logging-gh" + stub.mkdir(exist_ok=True) + (stub / "gh").write_bytes( + b'#!/bin/sh\necho "$*" >> "$GH_STUB_LOG"\nexit 97\n') + (stub / "gh").chmod(0o755) + ran = run_script(ADOPT, *args, env={ + "PATH": f"{stub}{os.pathsep}{os.environ['PATH']}", + "GH_STUB_LOG": str(log)}) + return ran, (log.read_text(encoding="utf-8").splitlines() + if log.exists() else []) + + +@needs_filter_repo +@needs_a_posix_gh +@pytest.mark.parametrize("case", sorted(ANOTHER_REPOSITORY)) +def test_check_asks_github_nothing_about_another_repositorys_legs(tmp_path, + case): + """#1232: given no `--local-remote-dir`, `check` on a plan for another + repository runs no `gh` and prints no NOTE about a leg's repository, as + `execute` asks nothing before it refuses `plan-not-the-source` (#499), + and both still name that first, `check` exit 1 and `execute` exit 2. On + 791106c2 `check` ran `gh repo view` of each leg, under the plan's + `org:`, and its NOTE said `execute` asks again.""" + plan = tmp_path / PLAN_FILE + plan.write_bytes(edited(planned_with(tmp_path, {"held"}), + ANOTHER_REPOSITORY[case]).encode("utf-8")) + checked, check_asked = with_a_logging_gh( + tmp_path / "check.log", "check", "--plan", str(plan)) + executed, execute_asked = with_a_logging_gh( + tmp_path / "execute.log", "execute", "--plan", str(plan), "--yes", + "--work-dir", str(tmp_path / "work")) + assert (check_asked, execute_asked) == ([], []), checked.stdout + assert COULD_NOT_ASK not in checked.stdout, checked.stdout + assert checked.returncode == 1, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert (code_of(named_first(checked)), + code_of(named_first(executed))) == (ANOTHER, ANOTHER), ( + checked.stderr + executed.stderr) + assert named_first(executed).startswith(f"REFUSED {ANOTHER}: "), ( + executed.stderr) + + +@needs_a_posix_gh +def test_a_local_remote_dir_is_still_asked_beside_another_repository( + tmp_path): + """THE CHOICE UNDER `--local-remote-dir` (#1232), as on 791106c2: the + disk is still asked beside `plan-not-the-source`. It asks no remote, + and a leg's bare repository is `/.git`, which no `org:` is + part of: with the spec leg's taken, `check` lists `leg-remote-exists` + after `plan-not-the-source`, the very finding it lists for the + source's own plan, the one the remedy writes, under the same + directory. Neither run asks `gh` anything.""" + remotes = tmp_path / "remotes" + (remotes / "Northwind-spec.git").mkdir(parents=True) + own = planned_with(tmp_path, {"held"}) + runs = {} + for name, text in (("own", own), ("another", edited(own, ANOTHER_ORG))): + plan = tmp_path / f"{name}.yaml" + plan.write_bytes(text.encode("utf-8")) + runs[name] = with_a_logging_gh( + tmp_path / f"{name}.log", "check", "--plan", str(plan), + "--local-remote-dir", str(remotes)) + (own_checked, own_asked), (checked, asked) = runs["own"], runs["another"] + assert (own_asked, asked) == ([], []), (own_asked, asked) + assert (own_checked.returncode, checked.returncode) == (1, 1), ( + own_checked.stderr + checked.stderr) + assert [code_of(line) for line in findings_of(checked)] == [ + ANOTHER, LEG_TAKEN], checked.stderr + assert findings_of(checked)[1:] == findings_of(own_checked), ( + own_checked.stderr + checked.stderr) + + +@needs_a_posix_gh +def test_check_still_asks_github_about_the_sources_own_legs(tmp_path): + """THE CONTROL, which passes on 791106c2 too (#1232): the source's own + plan, given no `--local-remote-dir`, is asked `gh repo view` of each + leg under its `org:`, and where gh cannot answer, `check` prints a NOTE + for each that says `execute` asks again, and passes.""" + plan = tmp_path / PLAN_FILE + plan.write_bytes(planned_with(tmp_path, {"held"}).encode("utf-8")) + checked, asked = with_a_logging_gh(tmp_path / "check.log", "check", + "--plan", str(plan)) + views = [f"repo view testorg/Northwind-{leg} --json nameWithOwner" + for leg in ("spec", "code")] + assert asked == views, asked + assert [line for line in checked.stdout.splitlines() + if line.startswith(COULD_NOT_ASK)] == [ + f"{COULD_NOT_ASK}testorg/Northwind-{leg}, the {leg} leg's " + f"repository, already exists: `gh {view}` exited 97. It is no " + "finding here, so that `check` stays usable offline; `execute` asks " + "again before it makes either leg, and refuses as " + "`adopt-remote-unreadable` while gh cannot answer." + for leg, view in zip(("spec", "code"), views)], checked.stdout + assert checked.returncode == 0, checked.stdout + checked.stderr + + #: The rule's problem beside an ordinary finding, by test id: `(the rule's #: problem, the ordinary finding)`. Both commands name the rule's problem #: first. `unanswered-and-taken` is left out: `check` lists an unanswered