diff --git a/adopt-project.py b/adopt-project.py index fb4f4926..835bdc6e 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -7797,6 +7797,16 @@ def _coverage_findings(entry_paths: list[str], tree_paths: list[str]) -> list[st return findings +def _unanswered_finding(entry: dict) -> str: + """`check`'s `plan-unresolved` for one entry whose `leg:` is null: its + path, `_spelled`, and the question it repeats, `_shown` (#328). The one + spelling of it, read by `_leg_findings` and by `_against_its_source`, + which `check` lists it from (#1197).""" + return (f"FINDING plan-unresolved: {_spelled(str(entry.get('path')))} " + "still has `leg: null`. The question was: " + f"{_shown(str(entry.get('question')))}") + + def _leg_findings(plan: Plan, tree: list | None, named_links: frozenset | set = frozenset(), source: Source | None = None, @@ -7855,9 +7865,7 @@ def _leg_findings(plan: Plan, tree: list | None, leg = entry.get("leg") path = _spelled(str(entry.get("path"))) if leg is None: - findings.append( - f"FINDING plan-unresolved: {path} still has `leg: null`. The " - f"question was: {_shown(str(entry.get('question')))}") + findings.append(_unanswered_finding(entry)) elif str(leg) not in LEG_VALUES: findings.append( f"FINDING plan-bad-leg: {path} declares leg " @@ -10592,7 +10600,10 @@ def _entry_findings(plan: Plan, source: Source, NUL byte among them (#388), which no answer in the plan fixes: the fix is a commit in the source, and the fresh `plan` after it takes back every answer given before, so they are listed before the - findings about those answers (`_source_path_findings`). Their + findings about those answers (`_source_path_findings`). (`check` lists + an unanswered entry before every finding here, with the plan's problems + against its source, as `execute` refuses it first: #1197, + `_against_its_source`.) Their remediation is said once after the findings, with the others. The same goes for each symlink whose target holds a NUL byte after its first, or is longer than Linux takes (#908), as @@ -14582,8 +14593,9 @@ def cmd_check(args) -> int: # 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. + # branch and the entries: another repository, a refused name, a moved + # source and an unanswered entry, each by its path (#1197), which is not + # listed again with the entries. names = plan.names() pins = set(plan.pins) unmended = list(_against_its_source(plan, source, names, naming, pins)) @@ -14604,7 +14616,9 @@ def cmd_check(args) -> int: local_remote_dir = getattr(args, "local_remote_dir", None) entry_findings, remediation, notes = _entry_findings(plan, source, local_remote_dir) - findings.extend(entry_findings) + # An unanswered entry the rule listed above is not listed again (#1197). + said = {line for problem in unmended for line in problem.found} + findings.extend(line for line in entry_findings if line not in said) # What is printed once after the findings, in their order. remediation = [fix for problem in unmended for fix in problem.fixes] + ( taken_remediation + remediation) @@ -14810,10 +14824,29 @@ def _against_its_source(plan: Plan, source: Source, names: dict, 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. + moved source as `check` said it; and an unanswered entry each by its + path, as `_leg_findings` says it, which `check` then does not list + again with the entries. The plan's `org:` and `id:` are asked before all four, by both (`_checked_org_and_id`, #1020). + + AN UNANSWERED ENTRY BEFORE WHAT `check` LISTS AFTER THE RULE (#1197). + `execute` refuses it here, before the plan's values, the split branch + and the entries; `check` listed it with the entries, after the split + branch taken (`plan-split-branch-taken`), the source's own paths + (`plan-unsafe-path`, `plan-empty-symlink`), the coverage + (`plan-uncovered`, `plan-empty-entry`, `plan-duplicate-path`, + `plan-covered-twice`), the leg paths (`plan-bad-leg-path`, + `plan-leg-path-collides`) and a bad `leg:` on an entry before it + (`plan-bad-leg`), so beside any of them the two named different + problems first. Listed here, it is first in both. Each of those is + mended in the plan, or by deleting a branch, and an answer given first + is kept; only a path the source must rename, or a symlink it must + mend, is mended by `plan` run again, which `check` listed first for + that reason (#303), and which `execute` has refused after an + unanswered entry since #168. Where the source has moved on, the moved + source is the rule's problem, and an unanswered entry is listed with + the entries, as before: the fresh plan asks it again. """ # The plan names the source, or `execute` acts on another repository # (#499): said before the names' own findings, as `execute` refuses it. @@ -14849,8 +14882,12 @@ def _against_its_source(plan: Plan, source: Source, names: dict, _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) + # Listed by `check` here, each entry by its path, as + # `_leg_findings` lists it, and not again with the entries + # (#1197). + yield _Unmended(exc, tuple( + _unanswered_finding(entry) for entry in plan.entries + if entry.get("leg") is None)) else: # `commit:` is a plan value people and AIs edit: sliced first, # so the spelling stays bounded, and `_shown`, so a real hash diff --git a/tests/test_adopt_entry_paths.py b/tests/test_adopt_entry_paths.py index 7ae6c788..97bb4eba 100644 --- a/tests/test_adopt_entry_paths.py +++ b/tests/test_adopt_entry_paths.py @@ -507,9 +507,10 @@ def test_plan_asks_about_a_path_execute_refuses(adopter, tmp_path, name, question saying `execute` will not extract the path as it is named, why, and that `root` or `drop` is the only answer; it prints that question with the paths still to answer. `check` and `execute` say of the plan as - written what they say of any entry still asked: `plan-unsafe-path` - (`leg: null`, judged so the person knows what `spec` would meet) and - `plan-unresolved`, and `execute` refuses before any leg exists.""" + written what they say of any entry still asked: `plan-unresolved`, first + to both (#1197), and then `check`'s `plan-unsafe-path` (`leg: null`, + judged so the person knows what `spec` would meet), and `execute` + refuses before any leg exists.""" text_name = name.decode(UTF8, "surrogateescape") in_the_file = in_the_file.replace( "{byte}", adopter._spelled_character("\udce9").replace("\\", "\\\\")) @@ -542,11 +543,12 @@ def test_plan_asks_about_a_path_execute_refuses(adopter, tmp_path, name, found = [line for line in checked.stderr.splitlines() if line.startswith("FINDING ")] named = adopter._spelled(text_name) - assert found[0] == ( + # The unanswered entry first, as `execute` refuses it (#1197). + assert found[0].startswith(f"FINDING plan-unresolved: {named} still has " + "`leg: null`. The question was: ") + assert found[1] == ( f"FINDING {UNSAFE}: {named} has leg: null, and `execute` will not " f"extract it into a leg: {adopter.entry_path_problem(text_name)}") - assert found[1].startswith(f"FINDING plan-unresolved: {named} still has " - "`leg: null`. The question was: ") assert len(found) == 2, found with pytest.raises(adopter.Refusal) as caught: adopter.cmd_execute(execute_args(tmp_path, plan)) diff --git a/tests/test_adopt_plan_against_its_source.py b/tests/test_adopt_plan_against_its_source.py index 08d35e57..b5c720a6 100644 --- a/tests/test_adopt_plan_against_its_source.py +++ b/tests/test_adopt_plan_against_its_source.py @@ -44,7 +44,25 @@ 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 +AN UNANSWERED ENTRY, TOO (#1197). On #1210's head 791106c2, `execute` +refused an unanswered entry with the rule, before all of these, and `check` +listed it with the entries, after the split branch taken, the source's own +paths, the coverage, the leg paths and a bad `leg:` on an entry before it. +So beside any of those the two named different problems first: +`plan-uncovered` against `plan-unresolved`, and the same for +`plan-split-branch-taken`, `plan-unsafe-path`, `plan-empty-entry`, +`plan-duplicate-path`, `plan-covered-twice`, `plan-bad-leg-path`, +`plan-leg-path-collides`, `plan-bad-leg` and `plan-empty-symlink`. `check` +now lists an unanswered entry with the rule's other problems, so both name +it first. Each of those findings alone is named by both, as before. +Two rows pin what the pair rows' `listed` cannot see, which counts each +code once (RV1197): two unanswered entries, `.claude/` and `examples/`, +each by its path ahead of the split branch taken, and a moved source +beside an unanswered entry, named `plan-stale`, `plan-uncovered`, and +then `plan-unresolved`, as before. + +Each row runs `check` and then `execute --yes` on one plan, hand-edited in +every row but the empty link's, which `plan` writes as it is (#1197), 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 @@ -53,12 +71,15 @@ from __future__ import annotations +import re + import pytest -from conftest import git +from conftest import git, make_source_repo, planned +from test_adopt_pins import ANSWERS 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_LEG_PATH, 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" @@ -69,6 +90,17 @@ #: taken, and an unsafe `tracking_branch:`. TAKEN = "plan-split-branch-taken" TRACKING = "unsafe-value" +#: More ordinary findings, each of which `check` listed before an +#: unanswered entry on 791106c2 (#1197). +UNCOVERED = "plan-uncovered" +BAD_LEG = "plan-bad-leg" +EMPTY_ENTRY = "plan-empty-entry" +UNSAFE_PATH = "plan-unsafe-path" +LEG_PATH = "plan-bad-leg-path" +DUPLICATE = "plan-duplicate-path" +COVERED_TWICE = "plan-covered-twice" +COLLIDES = "plan-leg-path-collides" +EMPTY_LINK = "plan-empty-symlink" #: The order both commands name the four in, the order a person mends them #: in (#1113). @@ -89,10 +121,32 @@ #: 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. +#: makes it, an uncovered path is `release.yaml` with its entry taken out, +#: and each other is a hand edit: `.claude/`, an entry sorted before +#: `examples/`, answered `banana`; an entry for a path the source does not +#: hold; `docs/` respelled `-docs/`, a path that is a `git` option; a leg +#: mounted at `../spec`; `LICENSE`'s entry written twice; an entry for a +#: file `docs/` covers; and a leg mounted at `README.md`, which the root +#: keeps. ORDINARY = { TAKEN: ((), {"taken"}), TRACKING: ((BAD_TRACKING_BRANCH,), set()), + UNCOVERED: ((), {"uncovered"}), + BAD_LEG: (((" - path: .claude/\n leg: root\n", + " - path: .claude/\n leg: banana\n"),), set()), + EMPTY_ENTRY: (((" - path: openspec/\n", + " - path: nothing.md\n leg: root\n" + " - path: openspec/\n"),), set()), + UNSAFE_PATH: (((" - path: docs/\n", " - path: -docs/\n"),), set()), + LEG_PATH: ((BAD_LEG_PATH,), set()), + DUPLICATE: (((" - path: Makefile\n", + " - path: LICENSE\n leg: root\n - path: Makefile\n"),), + set()), + COVERED_TWICE: (((" - path: examples/\n", + " - path: docs/architecture.md\n leg: spec\n" + " - path: examples/\n"),), set()), + COLLIDES: ((("\n spec_path: spec\n", "\n spec_path: README.md\n"),), + set()), } @@ -107,9 +161,28 @@ def holding(base, *problems: str) -> str: # `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") + if "uncovered" in holds: + # #1197's edit: `release.yaml` is in the source and in no entry. + entry = re.search(r" - path: release\.yaml\n( .*\n)*", text) + text = text.replace(entry[0], "", 1) return text +def an_empty_link(base) -> str: + """The text of the plan `plan` writes for a new source at `base` that + keeps `dangling`, a symlink whose target is empty (#303), made in the + index as `test_adopt_empty_symlinks.py` makes it, with every question + answered but the link's own: `plan` leaves it at `leg: null` itself.""" + source = make_source_repo(base / "Thing") + empty = base / "empty-target" + empty.write_bytes(b"") + blob = git("hash-object", "-w", str(empty), cwd=source).stdout.strip() + git("update-index", "--add", "--cacheinfo", f"120000,{blob},dangling", + cwd=source) + git("commit", "-q", "-m", "an empty link", cwd=source) + return planned(source, base, dict(ANSWERS)).read_text(encoding="utf-8") + + def code_of(line: str) -> str: """The code of a `REFUSED : ...` or `FINDING : ...` line.""" return line.split()[1].rstrip(":") if line else "" @@ -175,9 +248,10 @@ def test_each_problem_alone_is_named_by_its_own_code(tmp_path, problem): #: 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). +#: first. On 791106c2 `check` listed an unanswered entry with the entries, +#: so each `unanswered-and-` row but `unanswered-and-tracking` was named +#: differently by the two commands: `check` named the ordinary finding +#: first (#1197). WITH_AN_ORDINARY_FINDING = { "another-and-taken": (ANOTHER, TAKEN), "name-and-taken": (NAME, TAKEN), @@ -186,6 +260,15 @@ def test_each_problem_alone_is_named_by_its_own_code(tmp_path, problem): "name-and-tracking": (NAME, TRACKING), "moved-and-tracking": (MOVED, TRACKING), "unanswered-and-tracking": (UNANSWERED, TRACKING), + "unanswered-and-taken": (UNANSWERED, TAKEN), + "unanswered-and-uncovered": (UNANSWERED, UNCOVERED), + "unanswered-and-bad-leg": (UNANSWERED, BAD_LEG), + "unanswered-and-empty-entry": (UNANSWERED, EMPTY_ENTRY), + "unanswered-and-unsafe-path": (UNANSWERED, UNSAFE_PATH), + "unanswered-and-bad-leg-path": (UNANSWERED, LEG_PATH), + "unanswered-and-duplicate-path": (UNANSWERED, DUPLICATE), + "unanswered-and-covered-twice": (UNANSWERED, COVERED_TWICE), + "unanswered-and-collides": (UNANSWERED, COLLIDES), } #: The first `Remediation:` line `check` prints, where a row pins it: the @@ -221,3 +304,95 @@ def test_the_rules_problem_is_named_before_an_ordinary_finding(tmp_path, fixes = [line for line in checked.stderr.splitlines() if line.startswith("Remediation:")] assert fixes[0].startswith(FIRST_REMEDIATION[case]), checked.stderr + + +@needs_filter_repo +def test_an_empty_link_plan_leaves_unanswered_is_named_unanswered_by_both( + tmp_path): + """A PLAN AS `plan` WROTE IT, NO HAND EDIT (#1197): `plan` leaves the + entry of a symlink whose target is empty at `leg: null`, and `check` + finds the link too (#303). On 791106c2 `check` named + `plan-empty-symlink` first and `execute` refused `plan-unresolved`; both + now name the unanswered entry first, and `check` still lists the link, + with its remediation, in the same run.""" + checked, executed = both_on(an_empty_link(tmp_path), 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))) == (UNANSWERED, UNANSWERED), ( + checked.stderr + executed.stderr) + assert named_first(executed).startswith( + f"REFUSED {UNANSWERED}: 1 path(s) still have `leg: null`: " + "`dangling`"), executed.stderr + assert [code_of(line) for line in findings_of(checked)] == [ + UNANSWERED, EMPTY_LINK], checked.stderr + + +def two_unanswered(base) -> str: + """The text of the plan `plan` writes for a new source at `base` with + `examples/` and `.claude/` both left at `leg: null`, and `release.yaml` + answered: two unanswered entries, and nothing else wrong with it.""" + source = make_source_repo(base / "Thing") + return planned(source, base, {"release.yaml": "root"}).read_text( + encoding="utf-8") + + +@needs_filter_repo +def test_two_unanswered_entries_are_each_listed_before_the_split_branch_taken( + tmp_path): + """EACH BY ITS PATH (#1197, from RV1197): two unanswered entries, + `.claude/` and `examples/`, beside the split branch taken. `check` + lists the rule's `plan-unresolved` line for each, then the split branch + taken, and no entry's line after it; `execute` refuses both entries in + one refusal. A rule that listed only the first unanswered entry left the + other with the entries, after the split branch taken, and the pair rows, + which count each code once, did not see it.""" + text = two_unanswered(tmp_path) + git("branch", "adopt", cwd=tmp_path / "Thing") + checked, executed = both_on(text, tmp_path) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert [code_of(line) for line in findings_of(checked)] == [ + UNANSWERED, UNANSWERED, TAKEN], checked.stderr + assert [line.split("`")[1] for line in findings_of(checked) + if code_of(line) == UNANSWERED] == [".claude/", "examples/"], ( + checked.stderr) + assert named_first(executed).startswith( + f"REFUSED {UNANSWERED}: 2 path(s) still have `leg: null`: " + "`.claude/`, `examples/`"), executed.stderr + + +@needs_filter_repo +def test_a_moved_source_is_listed_before_the_uncovered_path_and_the_entry( + tmp_path): + """AS BEFORE (#1197, from RV1197): a source that has moved on, beside + an unanswered entry. The source's new file, `later.txt`, is in no entry, + so `check` lists exactly `plan-stale`, `plan-uncovered` and then the + entry's `plan-unresolved`, in that order: the rule lists the moved + source alone, and the unanswered entry stays with the entries. A rule + that listed the unanswered entry right after `plan-stale` put it before + `plan-uncovered`, and the pair rows, which count each code once, did + not see it. Both commands name the moved source first.""" + checked, executed = both_on(holding(tmp_path, MOVED, UNANSWERED), tmp_path) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert executed.returncode == 2, executed.stdout + executed.stderr + assert [code_of(line) for line in findings_of(checked)] == [ + MOVED, UNCOVERED, UNANSWERED], checked.stderr + assert named_first(executed).startswith(f"REFUSED {MOVED}: "), ( + executed.stderr) + + +@needs_filter_repo +@pytest.mark.parametrize("ordinary", sorted(ORDINARY)) +def test_each_ordinary_finding_alone_is_named_by_both(tmp_path, ordinary): + """THE CONTROLS, which pass on 791106c2 too: a plan that holds one + ordinary finding, every entry answered, is that finding first to + `check`, exit 1, and to `execute`, exit 2, before any leg exists, so + each hand edit above makes the finding it is named for.""" + checked, executed = both_on(holding(tmp_path, 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))) == (ordinary, ordinary), ( + checked.stderr + executed.stderr) + assert listed(checked) == [], checked.stderr