From 2a53a8e64a0cce17ab84cd1bf5949bcb6fbd4484 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Wed, 21 Oct 2026 12:31:45 +0000 Subject: [PATCH 1/4] 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/4] 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 f61be9f59f6bc783326ac405db5e0841823fa838 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Fri, 23 Oct 2026 11:24:21 +0000 Subject: [PATCH 3/4] tests/test_adopt_plan_value_order.py: a bad `id:` or `org:` beside a `--work-dir` that is a file or a shared one, or beside a gone source, is the value's refusal in both `check` and `execute`, so the head ask is pinned against `_work_root`'s other refusals and the source (#1179) Six rows, with the plan unedited beside the same file, directory or path as the control. Test only: no change to adopt-project.py. Stacked on the head of #1125 (9549006fe). Co-Authored-By: Claude Sonnet 5.5 --- tests/test_adopt_plan_value_order.py | 103 ++++++++++++++++++++++++++- 1 file changed, 102 insertions(+), 1 deletion(-) diff --git a/tests/test_adopt_plan_value_order.py b/tests/test_adopt_plan_value_order.py index 351c6d8e..c5a9d758 100644 --- a/tests/test_adopt_plan_value_order.py +++ b/tests/test_adopt_plan_value_order.py @@ -50,6 +50,13 @@ `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`. +AND BEFORE `_work_root`'S OTHER REFUSALS AND A GONE SOURCE (#1179, RV1125b's +NIT-A). The one row that pinned that position against the work directory +used a directory `chmod` made unwritable. `_work_root` refuses two more +things before it makes anything, a `--work-dir` that is a file and one every +user can write in, and both commands refuse a plan whose source is gone: a +bad `id:` or `org:` beside any of them is the value's refusal, in both. + 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 @@ -60,14 +67,21 @@ from __future__ import annotations +import os import shutil +import sys from pathlib import Path import pytest -from conftest import ADOPT, git, make_source_repo, planned, run_script +from conftest import (ADOPT, REPO, WINDOWS_SKIP, 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 +from test_adopt_template_values import with_line + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import double_quoted # noqa: E402 UTF8 = "utf-8" PLAN_FILE = "adoption-plan.yaml" @@ -529,6 +543,93 @@ def test_the_org_or_the_id_is_named_before_an_unwritable_work_dir( assert "adopt-work-dir-unwritable" not in run.stderr, run.stderr +#: What a bad `id:` or `org:` is refused beside what `_work_root` and the +#: source refuse: none of the three codes is said by either command (#1179). +NOT_SAID_BESIDE_A_VALUE = ("adopt-work-dir-unwritable", + "adopt-work-dir-shared", "plan-source-gone") +BAD_ID_OR_ORG = pytest.mark.parametrize( + ("edit", "refused"), [(BAD_ID, ID_REFUSED), (BAD_ORG, ORG_REFUSED)], + ids=["id", "org"]) + + +def named_before_a_refusal(text: str, base: Path, edit: tuple[str, str], + refused: str, control: str, + work: Path | None = None) -> None: + """The plan `text` is one both commands refuse, exit 2, with the first + line `control` (the control: `check` asks the source and the directory + as `execute` does). Edited by `edit`, a bad `id:` or `org:` that + `refused` begins, it is refused for that value by both, exit 2, in the + same words, and neither says a word of the directory or the source.""" + for run in both_on(text, base, work): + assert run.returncode == 2, run.stdout + run.stderr + assert named_first(run) == control, run.stderr + checked, executed = both_on(edited(text, edit), base, 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): + for code in NOT_SAID_BESIDE_A_VALUE: + assert code not in run.stderr, run.stderr + + +@needs_filter_repo +@BAD_ID_OR_ORG +def test_the_org_or_the_id_is_named_before_a_work_dir_that_is_a_file( + answered, tmp_path, edit, refused): + """RV1125b's NIT-A (#1179): `_work_root` refuses a `--work-dir` that is + a file, `adopt-work-dir-unwritable`, before anything is made, so the + two asks sit before it in both commands, as before an unwritable one: + a bad `id:` or `org:` beside the file is that value's refusal to each, + and the plan unedited beside the same file is the control.""" + a_file = tmp_path / "a-file" + a_file.write_text("mine\n", encoding=UTF8) + control = ("REFUSED adopt-work-dir-unwritable: --work-dir " + f"`{os.path.realpath(a_file)}` cannot be made: making it a " + "directory failed (File exists)") + named_before_a_refusal(answered, tmp_path, edit, refused, control, a_file) + + +@needs_filter_repo +@WINDOWS_SKIP +@BAD_ID_OR_ORG +def test_the_org_or_the_id_is_named_before_a_shared_work_dir( + answered, tmp_path, edit, refused): + """RV1125b's NIT-A (#1179): `_work_root` refuses a `--work-dir` every + user can write in, without the sticky bit, `adopt-work-dir-shared`, + before anything is made in it, so the two asks sit before it in both + commands: a bad `id:` or `org:` beside one is that value's refusal to + each, and the plan unedited beside the same directory is the control. + The shared test is never asked on Windows.""" + work = tmp_path / "shared-work" + work.mkdir() + os.chmod(work, 0o777) + control = ("REFUSED adopt-work-dir-shared: --work-dir " + f"`{os.path.realpath(work)}` can be written in by every user, " + "without the sticky bit, so another user could rename or " + "replace what this run makes in it") + named_before_a_refusal(answered, tmp_path, edit, refused, control, work) + + +@needs_filter_repo +@BAD_ID_OR_ORG +def test_the_org_or_the_id_is_named_before_a_source_that_is_gone( + answered, tmp_path, edit, refused): + """RV1125b's NIT-A (#1179): both commands refuse a plan whose + `source.local_path` names nothing, which `plan` did not clone, + `plan-source-gone`, once the source is opened, so the two asks sit + before it: a bad `id:` or `org:` beside a gone source is that value's + refusal to each, `check` asking it before it opens the source, and the + plan unedited beside the same path is the control.""" + gone = tmp_path / "Gone" + text = with_line(answered, f" local_path: {double_quoted(str(gone))}") + control = ("REFUSED plan-source-gone: the plan's `source.local_path`, " + f"`{gone}`, names nothing that exists, and the plan does not " + "record it as a clone `plan` made (`source.cloned` is not " + "`true`), so nothing clones it again") + named_before_a_refusal(text, tmp_path, edit, refused, control) + + @pytest.mark.parametrize(("edit", "refused"), [(BAD_ID, ID_REFUSED), (BAD_ORG, ORG_REFUSED)], ids=["id", "org"]) From 3aa9d51390fc0b8116c74a3b50642bc412beb93f Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Tue, 10 Nov 2026 11:09:23 +0000 Subject: [PATCH 4/4] tests/test_adopt_plan_value_order.py: the module takes `double_quoted` and `with_line` from `test_adopt_template_values` and puts nothing on `sys.path` itself, and its docstring says `_work_root` refuses more, among them the file and the shared directory, no longer two things (#1234) RV1226's two NITs on #1226 (for #1179), tests only. The 46 collected ids and their 46 outcomes are the same before and after. NIT-1: `test_adopt_template_values` already puts `scripts/` on `sys.path` and imports `double_quoted` beside `load_yaml`, so the module's own insert and its `from repo_shape import double_quoted` put `scripts/` there a second time. It imports `double_quoted, with_line` from that module now, and `sys`, `REPO` and the insert are gone: nothing else in the file used them. NIT-2: the docstring called a `--work-dir` that is a file and one every user can write in "two more" refusals of `_work_root`. It also refuses a symlink loop, a symlink to what is no directory, a directory another user owns and a shared directory above the `--work-dir`. It says "more ... among them". Co-Authored-By: Claude Sonnet 5.5 --- tests/test_adopt_plan_value_order.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tests/test_adopt_plan_value_order.py b/tests/test_adopt_plan_value_order.py index c5a9d758..212e921d 100644 --- a/tests/test_adopt_plan_value_order.py +++ b/tests/test_adopt_plan_value_order.py @@ -52,8 +52,8 @@ AND BEFORE `_work_root`'S OTHER REFUSALS AND A GONE SOURCE (#1179, RV1125b's NIT-A). The one row that pinned that position against the work directory -used a directory `chmod` made unwritable. `_work_root` refuses two more -things before it makes anything, a `--work-dir` that is a file and one every +used a directory `chmod` made unwritable. `_work_root` refuses more before +it makes anything, among them a `--work-dir` that is a file and one every user can write in, and both commands refuse a plan whose source is gone: a bad `id:` or `org:` beside any of them is the value's refusal, in both. @@ -69,19 +69,15 @@ import os import shutil -import sys from pathlib import Path import pytest -from conftest import (ADOPT, REPO, WINDOWS_SKIP, git, make_source_repo, - planned, run_script) +from conftest import (ADOPT, WINDOWS_SKIP, 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 -from test_adopt_template_values import with_line - -sys.path.insert(0, str(REPO / "scripts")) -from repo_shape import double_quoted # noqa: E402 +from test_adopt_template_values import double_quoted, with_line UTF8 = "utf-8" PLAN_FILE = "adoption-plan.yaml"