From 3b78fae6ef6a17a2898e1dbc84131130d8db13b5 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 11:26:50 +0000 Subject: [PATCH 1/3] adopt-project.py, shape-doctor.py, validate-manifest.py: `execute` records the plan's answer to each question the path policy asked under `answered_paths:`, the doctor takes it as the answer in that leg, and a submodule moved since the adoption names its row instead of a plan, so an answered `examples/` is no longer asked again, a deliberate move no longer points at `--placement-plan`, and a removed submodule's row no longer goes unsaid (#525) N7 (#457): `execute` recorded the plan's answers about submodules only (#189), so the doctor's `placement` row asked `spec/examples/` (rule `ambiguous-examples`) again on every run after an adoption that had answered it. `execute` now appends `answered_paths:` after `submodules:`: one row per plan entry answered `spec` or `code` that covers a path the policy's own walk, re-run over the source at the plan's commit, leaves `leg: null`, with its path in the leg, `leg`, `resolution:` and `source_commit`. `.gitmodules` is left to `submodules:`. A plan that answered no such question into a leg appends nothing, so its manifest is byte for byte what it was. The doctor settles a question whose every file one row answers into the leg it is in; a row naming the other leg settles nothing and asserts nothing, and a row no leg holds is silent. The project's `validate-manifest.py` checks the rows with the `submodules:` row checker, generalised by key and finding-code prefix: a directory's row keeps its one `/`, and no row lies under another. L1-08 (#457): nothing said how a `submodules:` row changes. A submodule in a leg its row does not name, where the leg the row names was read and holds no gitlink there, is still MISPLACED and now says it moved; where every listed path is such a move, or the `.gitmodules` of the leg it moved into, the NEXT is this doctor again, as `unreadable_next` gives for a file only a person can mend, and the WHY gives both ways out: the row edited by hand in the pull request that bumps the pins (`leg:`, `resolution:`, `source_commit:` deleted), or the move undone. A row whose path no leg holds a submodule at, every leg read, is a `note` (`detail.unheld`), never a finding. AGENTS.md says who edits a row and when, line for line within its 490-line cap. #189's test of a submodule against its record in tests/test_shape_doctor.py, whose recorded leg holds no such submodule, now reads that case as the move it is. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 16 +- adopt-project.py | 124 +++- shape-doctor.py | 283 ++++++++- .../scripts/validate-manifest.py | 145 +++-- tests/test_doctor_recorded_answers.py | 578 ++++++++++++++++++ tests/test_manifest_validator.py | 88 ++- tests/test_shape_doctor.py | 16 +- 7 files changed, 1156 insertions(+), 94 deletions(-) create mode 100644 tests/test_doctor_recorded_answers.py diff --git a/AGENTS.md b/AGENTS.md index e579245c..dd91cfe6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -152,10 +152,9 @@ install command). Seven steps, in order, and step 4 is the one you must not skip both legs and the split are pushed and verified: finish with the command it prints and never re-run into fresh legs, which abandons that pair. -`execute` also records the plan's answers about the source's submodules in -the split's `project.yaml`, under `submodules:` — per submodule a leg keeps, -its `path`, `leg`, `resolution:` and `source_commit`. It confers nothing, -nothing rewrites it, and the doctor holds each submodule to it. +`execute` also records the plan's answers in the split's `project.yaml`, under +`submodules:` (per submodule a leg keeps) and `answered_paths:` (per other path +the policy asked about). They confer nothing, and no command rewrites them. `execute` opens a pull request, because these organisations are pull-request only. Never suggest a direct push to the default branch, and never `--admin`. @@ -248,10 +247,11 @@ that directory, which is the human's to delete once those commands have run. `review_required: true` is a question the policy is ASKING, not a defect found: answering it on their behalf puts a fact in their tree that nobody decided. The plan is `kind: placement-plan` and is not an adoption plan; - never hand one to `adopt-project.py`, which refuses it by name. A - submodule is judged by the adoption's `submodules:` record, not a rule - (`rule: null`, `confidence: recorded`), and is a `note` where no row names - it. One moved on purpose stays MISPLACED: its row is the human's to change. + never hand one to `adopt-project.py`, which refuses it by name. A submodule + is held to its `submodules:` row, not a rule (`rule: null`, `confidence: + recorded`); one no row names, or a row no leg holds, is a `note`. A move or + removal the human decided is recorded by editing its row by hand, in the + pull request that bumps the pins: that is not the edit 3 forbids. 5. **`NOT A SHAPE ROOT` is a question for the human, not a task.** There is no `project.yaml` and no `family.yaml`. The two ways in are `adopt-project.py plan` (a repository that already exists, converted in diff --git a/adopt-project.py b/adopt-project.py index e4fa0451..82e04afa 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -82,7 +82,9 @@ are the two leg mounts, each at the commit pushed to its leg. The split's `project.yaml` records the leg each source submodule kept in a leg was answered into, under `submodules:`, for - `shape-doctor.py` to check it against. + `shape-doctor.py` to check it against, and under + `answered_paths:` the leg each entry covering a path the policy + asked about was answered into, so the doctor asks it no more. THE PLAN IS AN ARTIFACT A HUMAN OR AN AI EDITS. That is why it is YAML with reasons in it rather than a pipe between two processes: the classifier is @@ -7995,6 +7997,95 @@ def _where_it_landed(assembly: Path, written: str) -> str | None: f"{SUBMODULE_RECORD}:", ) +#: The key `execute` appends after `submodules:` (#525): the plan's answer +#: for each path the policy itself asked about. Read by the doctor by this +#: name through the module it loads, and spelt once more by the project's +#: own `validate-manifest.py`, as `SUBMODULE_RECORD` is. +ANSWER_RECORD = "answered_paths" + +#: The comment above that block, for the same reader. +ANSWER_RECORD_PREAMBLE = ( + "", + "# THE PATHS THE ADOPTION'S POLICY ASKED ABOUT, and the leg each was", + "# ANSWERED into (#525). Written once, by `adopt-project.py execute`,", + "# from the adoption plan: one row per entry kept in a leg that covers a", + "# path `contracts/path-classification.yaml` could not call, its path", + "# inside that leg (a directory's with its `/`), the `resolution:` the", + "# plan gave it, and the source commit the plan was answered at.", + "# `shape-doctor.py` takes each row as the answer to the question it", + "# would ask again of that path in that leg. Like every field here, it", + "# confers nothing.", + f"{ANSWER_RECORD}:", +) + + +def _record_block(preamble: tuple, rows: list, entries: list, + commit: str) -> str: + """`preamble` and one row per `(path, leg, entry)` of `rows`: the shape + both records take (#189, #525), or "" when there is no row. + + The entry's `resolution:` travels with each row it covers, and every + path and reason is double-quoted, for the reasons `submodule_record` + gives. + """ + if not rows: + return "" + resolutions = {str(e.get("path")): _resolution_text(e.get("resolution")) + for e in entries} + lines = list(preamble) + for path, leg, entry in rows: + lines.append(f" - path: {double_quoted(path)}") + lines.append(f" leg: {leg}") + if resolutions.get(entry): + lines.append( + f" resolution: {double_quoted(resolutions[entry])}") + lines.append(f' source_commit: "{commit}"') + return "\n".join(lines) + "\n" + + +def answered_path_record(policy: PathPolicy, entries: list, tree: list, + commit: str) -> str: + """The `answered_paths:` block `execute` appends to `project.yaml` after + `submodules:`, or "" when the plan answered no question of the policy's + into a leg (#525). + + WHY THE ANSWER IS WRITTEN DOWN. A path the policy cannot call -- + `examples/` under `ambiguous-examples`, a directory whose extensions do + not agree -- is a question `plan` asks and a human answers in the plan, + which is committed nowhere. `shape-doctor.py` runs the same policy over + each leg after the split and, with no record, asks the same question + again on every run, of a path somebody already answered. + + ONLY THE POLICY'S OWN QUESTIONS, RE-ASKED HERE. The policy's `walk` is + run again over the source's paths at the plan's commit, gitlinks left + out as the doctor leaves them out, and a row is written for each entry + the plan answers `spec` or `code` (read as `_answered_legs` reads it) + that covers a path the walk leaves `leg: null`. Not from the plan's own + `review_required:`, which is a file a person edits: an entry they split + by hand to answer one question two ways is still recorded, each half in + its own leg. An entry answered `root` or `drop` is in no leg, and the + doctor never asks of it. `.gitmodules` is left out: its question is + which leg owns the submodules it registers, and `submodules:` answers + that per submodule, which is the reading the doctor holds a leg's + `.gitmodules` to. And a question `plan` asks for a reason of its own -- + a gitlink under the entry, a name `execute` will not extract -- is not + the policy's, and is recorded only where the policy asks one under it. + + A SOURCE WITH NO SUCH ENTRY GETS "" and so the `project.yaml` it always + got, byte for byte. + """ + gitlinks = {path for path, mode, _, _ in tree if mode == GITLINK_MODE} + files = [(path, size) for path, mode, _, size in tree + if path not in gitlinks] + asked = [entry.path for entry in walk(policy, files) if entry.leg is None] + questions = [path for path, _ in files if _covering(asked, path)] + answered = _answered_legs(entries, questions) + kept = sorted({entry: leg for leg, entry in answered.values() + if leg in EXTRACTED_LEGS and entry != GITMODULES}.items()) + return _record_block(ANSWER_RECORD_PREAMBLE, + [(entry, leg, entry) for entry, leg in kept], + entries, commit) + def submodule_record(entries: list, tree: list, commit: str) -> str: """The `submodules:` block `execute` appends to `project.yaml`, or "" @@ -8039,19 +8130,7 @@ def submodule_record(entries: list, tree: list, commit: str) -> str: """ gitlinks = [path for path, mode, _, _ in tree if mode == GITLINK_MODE] kept = _kept_in_a_leg(_answered_legs(entries, gitlinks), gitlinks) - if not kept: - return "" - resolutions = {str(e.get("path")): _resolution_text(e.get("resolution")) - for e in entries} - lines = list(SUBMODULE_RECORD_PREAMBLE) - for path, leg, entry in kept: - lines.append(f" - path: {double_quoted(path)}") - lines.append(f" leg: {leg}") - if resolutions.get(entry): - lines.append( - f" resolution: {double_quoted(resolutions[entry])}") - lines.append(f' source_commit: "{commit}"') - return "\n".join(lines) + "\n" + return _record_block(SUBMODULE_RECORD_PREAMBLE, kept, entries, commit) def _resolution_text(resolution) -> str | None: @@ -8071,8 +8150,8 @@ def _resolution_text(resolution) -> str | None: def _commit_the_split(plan: Plan, source: Source, assembly: Path, names: dict, urls: dict, values: dict, work_root: Path, paths_for: dict, seeded: list, spec_path: str, - code_path: str, branch: str, - leg_commits: dict) -> tuple[str, str, list] | None: + code_path: str, branch: str, leg_commits: dict, + answers: str = "") -> tuple[str, str, list] | None: """(c) ONE split commit on a branch of the source, pushed. Returns `(split_commit, message, collisions)` — the message travels on @@ -8138,6 +8217,10 @@ def _commit_the_split(plan: Plan, source: Source, assembly: Path, source.commit) if record: append["project.yaml"] = record + # And the plan's answers to the policy's own questions, after it + # (#525): `answered_path_record`, read before any leg existed. + if answers: + append["project.yaml"] = append.get("project.yaml", "") + answers # Each declared pin's file, `contracts/-pin.yaml`, is # written by the materializer as the scaffold's is, from the # values the plan records (#273); `_refuse_what_check_finds` has @@ -8241,6 +8324,13 @@ def cmd_execute(args) -> int: code_path, tracking, {}, {}, shape_commit, pins, naming) + # The plan's answers to the policy's own questions (#525), worked out + # here, before anything exists: the policy is this standard's file, the + # one the doctor will ask, and one that does not load refuses now and + # not after both legs are pushed. "" for a plan that answered none. + answers = answered_path_record(PathPolicy.load(PATH_POLICY), + plan.entries, source.tree(), source.commit) + # ---- (a) the two legs' remotes ---------------------------------------- _create_leg_remotes(plan, names, repositories, urls, tracking, local, work_root, source=source) @@ -8262,7 +8352,7 @@ def cmd_execute(args) -> int: assembly = work_root / names["assembly"] split = _commit_the_split(plan, source, assembly, names, urls, values, work_root, paths_for, seeded, spec_path, - code_path, branch, leg_commits) + code_path, branch, leg_commits, answers) if split is None: return 2 split_commit, message, collisions = split diff --git a/shape-doctor.py b/shape-doctor.py index 94f993c4..4786b219 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -204,6 +204,29 @@ that nothing recorded looks exactly like the second, and MISPLACED would put back the false positive #176 removed. +A PATH THE ADOPTION ANSWERED IS NOT ASKED AGAIN (#525). `execute` records the +plan's answer to each question the policy itself asked -- `examples/` under +`ambiguous-examples` -- under `answered_paths:`, one row per entry kept in a +leg. A walked question whose every file one row answers into the leg it is +in is settled: named in the row, in `--json` under `answered`, and no reason +to keep the row off `ok`. A row naming the OTHER leg settles nothing and +asserts nothing: the path stays the question it was, a `note`, and is never +MISPLACED, because a record confers nothing and a file named like one the +adoption put in the other repository is not that file. A row whose path no +leg holds is silent, because it can only settle a question, never make a +finding. And a leg's `.gitmodules` is answered by `submodules:` alone. + +A SUBMODULE MOVED OR REMOVED SINCE THE ADOPTION (#525). Its row is a +person's to change, by hand, in the pull request that bumps the pins; no +command rewrites one. Until then a submodule in a leg its row does not name, +where the leg the row names was read and holds no gitlink there, is still +MISPLACED and is said to have MOVED; where every path the row lists is such +a move, or the `.gitmodules` of the leg it moved into, the NEXT is this +doctor again, as for a file only a person can mend, and not a plan of the +move already made, and the WHY gives both ways out. A row whose path no leg +holds a submodule at, every leg read, is a `note`: removing one is not +wrong. + AND THE DOCTOR STILL MOVES NOTHING. A path changing legs is a pull request on the leg it leaves, a pull request on the leg it joins, and one pin bump in the assembly root -- a human's act, with a human's review. So the row's next @@ -535,6 +558,31 @@ def not_in_the_standard(detail: str) -> int: #: The key of a leg's entry naming the paths whose policy question the record #: answered -- its `.gitmodules` -- present only on a leg that has one. ANSWERED = "answered" +#: Where the plan's answers to the policy's own questions are (#525), said +#: the same way in the row and in `--json`. +THE_ANSWERS = "project.yaml's `answered_paths:`" +#: The key under `detail`, and under `counts`, of the questions that record +#: answered (#525); present only where it answered one. +ANSWERED_PATHS = "answered" +#: The key that marks a `misplaced` submodule as one its recorded leg no +#: longer holds (#525); present only on one. +MOVED = "moved" +#: The key under `detail`, and under `counts`, of the `submodules:` rows no +#: leg holds (#525); present only where there is one. +UNHELD = "unheld" +#: The `why` of a row whose every path in the wrong leg is a submodule moved +#: since the adoption (#525). Its NEXT is this doctor again, as the +#: `manifest kinds` row's is for a file only a person can mend +#: (`UNREADABLE_BY_HAND`): no command makes either way out. +MOVED_BY_HAND = ( + "each submodule above is in a leg its row in project.yaml's " + "`submodules:` does not name, and the leg the row names no longer holds " + "it, so it was moved. Moved on purpose, its row is a person's to change " + "by hand, in the pull request that bumps the pins: `leg:` to the leg it " + "is in, `resolution:` to why, and `source_commit:` deleted, which records " + "what both legs already carry. Moved by mistake, it goes back: a pull " + "request on each leg and a pin bump in the root. This row reads it again " + "once either is done") #: What belongs to EVERY repository, so the `placement` row never calls it #: misplaced in a leg. @@ -4143,11 +4191,52 @@ def held_clauses(detail: dict) -> str: names -- so that no reason ends on a separator with nothing after it. """ clauses = (kept_submodules(detail.get("submodules") or []), - answered_clause(detail.get("legs") or [])) + answered_clause(detail.get("legs") or []), + answers_clause(detail.get(ANSWERED_PATHS) or []), + unheld_clause(detail.get(UNHELD) or [])) said = "; ".join(clause for clause in clauses if clause) return f"; {said}" if said else "" +def answers_clause(answered: list) -> str: + """The clause naming the questions `answered_paths:` answered, each with + the rule that asked it, or "" where it answered none (#525). + + SAID, as the `.gitmodules` the record answers is said: the row no longer + counts these among the paths the policy will not call, and a reader who + knows the policy asks of them is owed where the answer came from. + """ + if not answered: + return "" + named = first_three([f"{row['path']} (rule {row['rule']})" + for row in answered]) + return (f"{THE_ANSWERS} answers the question the policy asks of " + f"{len(answered)} path(s) (#525): {named}") + + +def unheld_clause(unheld: list) -> str: + """The clause naming each `submodules:` row whose path no leg holds a + submodule at, or "" where there is none (#525). A NOTE'S CLAUSE: removing + a submodule is not wrong, so its row lingering is said and never found. + """ + if not unheld: + return "" + named = first_three([f"{row['path']} (recorded in the " + f"{row[RECORDED_LEG]} leg)" for row in unheld]) + return (f"{THE_RECORD} records {len(unheld)} submodule(s) no leg holds " + f"(#525): {named}; one removed on purpose has its row deleted by " + "hand, in the pull request that removed it") + + +def classified(detail: dict, what: str) -> str: + """`every tracked classifies as the leg it is in`, and `or is + answered there` where `answered_paths:` answered a question (#525), + because a path the record placed was not classified by the policy.""" + said = f"every tracked {what} classifies as the leg it is in" + return f"{said} or is answered there" if detail.get(ANSWERED_PATHS) \ + else said + + def recorded_placement(role: str, rule, row: dict) -> dict: """A submodule held to the row `project.yaml` records for its path (#189). @@ -4236,7 +4325,22 @@ def recorded_submodules(adopt, manifest) -> dict: this row has already loaded, so that the writer and this reader spell it once. """ - rows = (manifest or {}).get(adopt.SUBMODULE_RECORD) + return recorded_rows((manifest or {}).get(adopt.SUBMODULE_RECORD)) + + +def recorded_answers(adopt, manifest) -> dict: + """`{path in its leg: row}` for each row of `project.yaml`'s + `answered_paths:` an answer can be read from (#525): the rows the + project's own `validate-manifest.py` accepts, read as + `recorded_submodules` reads its record. A directory's path keeps the one + `/` the adoption plan wrote it with. + """ + return recorded_rows((manifest or {}).get(adopt.ANSWER_RECORD)) + + +def recorded_rows(rows) -> dict: + """`{path: row}` for each row of a record that says something, the + first of two that name one path deciding: see `recorded_submodules`.""" record: dict = {} claimed: set = set() for row in rows if isinstance(rows, list) else []: @@ -4398,8 +4502,53 @@ def walked_findings(walked: list, role: str, rel: str, return misplaced, review, settled +def answered_questions(adopt, review: list, role: str, files: list, + answers: dict) -> tuple: + """The leg's questions, less those `answered_paths:` answered into THIS + leg; and those, as entries of their own (#525). + + A QUESTION IS ANSWERED WHEN EVERY FILE UNDER IT IS. The doctor's walk + folds a leg's files as they fall in that leg, not as they fell in the + source the adoption plan was written over, so the question is held to + the record file by file: each tracked file under it is covered by + exactly one row -- the path's own, or a directory's above it, as + `adopt-project.py`'s `_covering` reads an entry -- and that row names + this leg. A file the record does not cover, or covers twice, leaves the + question asked. + + A ROW NAMING THE OTHER LEG SETTLES NOTHING AND ASSERTS NOTHING. The + record confers nothing, and a file in this repository named like one the + adoption put in the other is not that file, so it stays the question it + was -- a `note` -- and is never MISPLACED. Nor is the leg's + `.gitmodules` answered here: `submodules:` answers it, and only where it + places every submodule the file registers (`gitmodules_the_record_ + answers`). + """ + rows = list(answers) + still: list = [] + answered: list = [] + for row in review: + found = row["path_in_leg"] + under = [path for path in files + if adopt._covering([found], path)] + covering = [adopt._covering(rows, path) for path in under] + if (found == adopt.GITMODULES or not under + or any(len(by) != 1 or answers[by[0]].get("leg") != role + for by in covering)): + still.append(row) + continue + by = list(dict.fromkeys(paths[0] for paths in covering)) + resolution = answers[by[0]].get("resolution") + answered.append({ + **row, "confidence": SUBMODULE_RECORDED, "review_required": False, + RECORDED_LEG: role, "answered_by": by, + "resolution": (resolution if len(by) == 1 + and isinstance(resolution, str) else None)}) + return still, answered + + def audit_leg(adopt, policy, patterns: list, role: str, rel: str, - root: Path, record: dict) -> tuple: + root: Path, record: dict, answers: dict) -> tuple: """ONE leg: its summary, the paths in the wrong leg, the questions, and the submodules it keeps. @@ -4456,25 +4605,25 @@ def audit_leg(adopt, policy, patterns: list, role: str, rel: str, escaped = outside_the_root(root, mount) if escaped: entry["state"] = escaped - return entry, [], [], [] + return entry, [], [], [], [] if not (mount.is_dir() and any(mount.iterdir())): # The `legs` row already reports this, with `make bootstrap` beside # it. Here it is only why this leg has no answer. entry["state"] = "not populated" - return entry, [], [], [] + return entry, [], [], [], [] if role not in LEG_ROLES: entry["state"] = (f"role {role} is not one that the path policy " "classifies") - return entry, [], [], [] + return entry, [], [], [], [] borrowed = not_its_own_repository(mount) if borrowed: entry["state"] = borrowed - return entry, [], [], [] + return entry, [], [], [], [] try: files, unwritable, gitlinks = leg_tracked_files(mount) except Refusal as exc: entry["state"] = f"git could not list it: {exc.detail}" - return entry, [], [], [] + return entry, [], [], [], [] entry["unwritable"] = unwritable # Once each, in the index's own order: an unmerged gitlink is listed # once per stage, and it is one submodule. From `files` and not from what @@ -4490,6 +4639,8 @@ def audit_leg(adopt, policy, patterns: list, role: str, rel: str, submodules, wrong = leg_submodules(policy, role, rel, held, record) answered = gitmodules_the_record_answers(adopt, submodules, unwritable) misplaced, review, settled = walked_findings(walked, role, rel, answered) + review, recorded = answered_questions( + adopt, review, role, [path for path, _ in kept], answers) misplaced += wrong # FILES AND PATHS ARE DIFFERENT NUMBERS and the row prints both. `walk` # folds a directory whose files agree into ONE entry, so `src/` with two @@ -4503,7 +4654,7 @@ def audit_leg(adopt, policy, patterns: list, role: str, rel: str, if settled: # Only when there is one, so that no other leg's entry changes. entry[ANSWERED] = settled - return entry, misplaced, review, submodules + return entry, misplaced, review, submodules, recorded def placement_policy(ctx: Context, policy_path: Path) -> tuple: @@ -4580,7 +4731,9 @@ def misplaced_clauses(misplaced: list) -> str: reader disagrees with the one that decided. Each clause only when there is one, so a project with no submodule gets the sentence it always got. """ - held = [row for row in misplaced if row.get(AGAINST_THE_RECORD)] + held = [row for row in misplaced + if row.get(AGAINST_THE_RECORD) and not row.get(MOVED)] + moved = [row for row in misplaced if row.get(MOVED)] files = [row for row in misplaced if not row.get(AGAINST_THE_RECORD)] clauses = [] if files: @@ -4590,6 +4743,10 @@ def misplaced_clauses(misplaced: list) -> str: clauses.append(f"{len(held)} submodule(s) sit in a leg other than " f"the one {THE_RECORD} records for them (#189): " f"{named_against_the_record(held)}") + if moved: + clauses.append(f"{len(moved)} submodule(s) moved out of the leg " + f"{THE_RECORD} records for them, which no longer " + f"holds them (#525): {named_against_the_record(moved)}") return "; ".join(clauses) @@ -4607,10 +4764,11 @@ def submodules_verdict(detail: dict, audited: str) -> Row: """ kept = detail["submodules"] held = held_clauses(detail) - if all(item["placement"] == SUBMODULE_RECORDED for item in kept): + if (all(item["placement"] == SUBMODULE_RECORDED for item in kept) + and not detail.get(UNHELD)): return placement_row( - OK, f"every tracked file classifies as the leg it is in{held} " - f"[{audited}]", None, detail) + OK, f"{classified(detail, 'file')}{held} [{audited}]", None, + detail) return placement_row( NOTE, f"nothing is in the wrong leg{held} [{audited}]", None, detail) @@ -4695,6 +4853,15 @@ def placement_verdict(detail: dict, audited: str, plan: str, None, detail) if kept: return submodules_verdict(detail, audited) + if detail.get(UNHELD): + # A row no leg holds is a `note` (#525): said, never found. + return placement_row( + NOTE, f"nothing is in the wrong leg{held} [{audited}]", None, + detail) + if held: + return placement_row( + OK, f"{classified(detail, 'path')}{held} [{audited}]", None, + detail) return placement_row( OK, f"every tracked path classifies as the leg it is in: {audited}", None, detail) @@ -4774,19 +4941,22 @@ def check_placement(ctx: Context) -> Row: ignored = everywhere_patterns(ctx.shape) patterns = [glob_to_regex(pattern) for pattern in ignored] record = recorded_submodules(adopt, ctx.manifest) + answers = recorded_answers(adopt, ctx.manifest) per_leg = [] misplaced = [] review = [] submodules = [] + answered = [] for leg in legs: role = str(leg.get("role") or "?") rel = str(leg.get("path") or role) - entry, wrong, unsure, held = audit_leg(adopt, policy, patterns, role, - rel, ctx.root, record) + entry, wrong, unsure, held, recorded = audit_leg( + adopt, policy, patterns, role, rel, ctx.root, record, answers) per_leg.append(entry) misplaced.extend(wrong) review.extend(unsure) submodules.extend(held) + answered.extend(recorded) detail = {"policy": policy_path.as_posix(), "everywhere": ignored, "legs": per_leg, "misplaced": misplaced, @@ -4802,7 +4972,20 @@ def check_placement(ctx: Context) -> Row: # Only when there is one: see `placement_row`. detail["submodules"] = submodules detail["counts"]["submodules"] = len(submodules) + unheld = moved_and_unheld(per_leg, submodules, misplaced, record) + for key, found in ((ANSWERED_PATHS, answered), (UNHELD, unheld)): + if found: + # Only when there is one, as `submodules` is (#525). + detail[key] = found + detail["counts"][key] = len(found) audited = "; ".join(leg_audit_summary(entry) for entry in per_leg) + if moved_alone(adopt, detail): + # No command makes either way out of a move already made (#525): + # the NEXT is this doctor again, as `unreadable_next`'s is. + return placement_verdict( + detail, audited, + f"{PYTHON} {quote_arg(ctx.shape / 'shape-doctor.py')} --root " + f"{quote_arg(ctx.root)}", MOVED_BY_HAND) plan = (f"{PYTHON} {quote_arg(ctx.shape / 'shape-doctor.py')} --root " f"{quote_arg(ctx.root)} " "--placement-plan placement-plan.yaml") @@ -4813,6 +4996,76 @@ def check_placement(ctx: Context) -> Row: "command makes neither") +def moved_and_unheld(per_leg: list, submodules: list, misplaced: list, + record: dict) -> list: + """Mark each misplaced submodule whose recorded leg no longer holds it + as `moved`, and return the `submodules:` rows no leg holds (#525). + + A MOVE ALREADY MADE. A submodule in the leg its row does not name, where + the leg the row does name was read and holds no gitlink at that path, + has changed legs: the pull request on each leg and the pin bump in the + root are done, and only the row is left. It is still MISPLACED -- the + manifest asserts the other leg -- and its row is a person's to change. + Where that leg was not read, nobody can say, and it is MISPLACED as any + submodule against its record is. + + A ROW NO LEG HOLDS. A submodule removed since the adoption leaves its + row, and one added at that path later would be judged by it. Removing a + submodule is not wrong, so this is a `note` and never a finding, and + only where EVERY leg was read: a leg that was not could hold it. A + recorded name no report can carry is read no further, as + `leg_tracked_files` reads none. + """ + read = {entry["role"] for entry in per_leg + if entry.get("state") == "audited"} + held = {(item["leg"], item["path_in_leg"]) for item in submodules} + for row in misplaced: + if (row.get(AGAINST_THE_RECORD) and row["classified_as"] in read + and (row["classified_as"], row["path_in_leg"]) not in held): + row[MOVED] = True + if len(read) < len(per_leg): + return [] + holding = {path for _, path in held} + return [{"path": path, RECORDED_LEG: str(row["leg"]), + "resolution": (row.get("resolution") + if isinstance(row.get("resolution"), str) + else None)} + for path, row in record.items() + if path not in holding and not UNWRITABLE_IN_A_PLAN.search(path)] + + +def moved_alone(adopt, detail: dict) -> bool: + """Is every path the row lists a submodule moved since the adoption, or + the `.gitmodules` of the leg it moved into (#525)? + + THEN NO PLAN IS THE WAY OUT. The `--placement-plan` writes the paths to + move, and these have moved: what is left is the row in `project.yaml`, + a person's to change, or a move back. A leg's `.gitmodules` that + registers what moved in is the policy's question only because the row + still names the other leg, so it goes with the move -- where every + submodule that leg keeps is recorded there or moved in, and every name + in it could be read. Anything else in the wrong leg, or asked, keeps the + plan as the NEXT. + """ + misplaced = detail["misplaced"] + if not misplaced or not all(row.get(MOVED) for row in misplaced): + return False + moved = {row["path"] for row in misplaced} + for question in detail["review_required"]: + role = question["leg"] + leg = next(entry for entry in detail["legs"] if entry["role"] == role) + if (question["path_in_leg"] != adopt.GITMODULES + or leg.get("unwritable") + or not any(path.startswith(f"{leg['path']}/") + for path in moved) + or any(item["leg"] == role + and item["placement"] != SUBMODULE_RECORDED + and item["path"] not in moved + for item in detail.get("submodules") or [])): + return False + return True + + #: The header of the file `--placement-plan` writes. It is long because the #: file is EDITED by somebody who may never have seen one, and because the #: single most expensive mistake available here -- handing it to diff --git a/templates/assembly-root/scripts/validate-manifest.py b/templates/assembly-root/scripts/validate-manifest.py index 2263b58e..7f5308dc 100755 --- a/templates/assembly-root/scripts/validate-manifest.py +++ b/templates/assembly-root/scripts/validate-manifest.py @@ -67,6 +67,13 @@ `adopt-project.py execute` writes it, once, from the adoption plan, and `shape-doctor.py` holds each submodule a leg keeps to the row naming its path. A row whose `path` or `leg` is refused here holds nothing there. + - `answered_paths`, OPTIONAL: the record an adoption writes of the leg each + entry covering a path the path policy asked about was answered into + (#525), in the same rows, except that a directory's `path` ends with + the one `/` the adoption plan wrote it with, and no row's path lies + under another's. `shape-doctor.py` takes a row as the answer to the + question it would ask again of that path in that leg. Neither record is + checked against the legs: this runs where they are not checked out. EXIT CODES: 0 valid · 1 findings · 2 refusal (the file is missing or unreadable) """ @@ -665,99 +672,150 @@ def _leg_findings(leg, policy: NamingPolicy, pins: set[str], root, return out -#: The legs a source submodule may be recorded in (#189). The assembly is not -#: one of them: the root keeps no submodule but its two legs. +#: The legs a record's row may name (#189, #525). The assembly is not one of +#: them: the root keeps no submodule but its legs, and an answer that kept a +#: path in the root is recorded nowhere. SUBMODULE_LEGS = ("spec", "code") -#: What a recorded `path` is, said beside one that is not. -SUBMODULE_PATH = ("it is the submodule's path inside its leg, relative and " - "without `..`") +class _Record: + """One of the two records an adoption appends, each a list of `path:` / + `leg:` rows (#189, #525), and how its findings are spelled. + + ONE CHECKER FOR BOTH, because the rows are one shape and the doctor + reads both the same way. What differs is said here: the key, the stem of + each finding's code, what a row's path is, why a leg other than `spec` + or `code` says nothing, and whether a row names a directory -- with the + one `/` the adoption plan wrote it with -- that covers every path under + it. + """ + + def __init__(self, key: str, code: str, path_is: str, not_canonical: str, + why_leg: str, directories: bool): + self.key = key + self.code = code + self.path_is = path_is + self.not_canonical = not_canonical + self.why_leg = why_leg + self.directories = directories + + +SUBMODULE_ROWS = _Record( + "submodules", "manifest-submodule", + "it is the submodule's path inside its leg, relative and without `..`", + "Git records no path with a `.` segment, a trailing `/` or a doubled " + "`/`, so no submodule is ever at it", + "the assembly root keeps no submodule but its legs", False) + +ANSWER_ROWS = _Record( + "answered_paths", "manifest-answered", + "it is a path inside its leg, relative and without `..`, and a " + "directory's ends with one `/`", + "Git records no path with a `.` segment or a doubled `/`, and only a " + "directory's row ends with one `/`, so no path is ever at it", + "an answer that kept a path in the root is recorded nowhere", True) + + +def _covers(row_path: str, path: str) -> bool: + """Does a row naming `row_path` cover `path`: the path itself, or a + directory above it (#525).""" + return row_path == path or (row_path.endswith("/") + and path.startswith(row_path)) -def _submodule_path_problem(path, seen: set) -> str | None: - """Why one recorded `path` is not a submodule's path inside its leg, or - None. Records a good one in `seen`, so a second row for it is named. +def _record_path_problem(record: _Record, path, seen: list) -> str | None: + """Why one recorded `path` is not a path the record's reader can find, or + None. Records a good one in `seen`, so a second row for it is named, and, + where rows name directories, so is a row under another. SPELLED THE ONE WAY GIT RECORDS A PATH (`repo_shape.path_spelling_fault`, the rule a leg's mount path is held to as well). The doctor finds a submodule by the path its leg's index records, and Git records none with a leading `./`, a trailing `/`, a doubled `/` or a `.` segment, so a row - spelled that way would name nothing the doctor could ever hold to it. - A backslash or a drive letter is accepted: on POSIX each is a character - of a name Git records, and an adoption records a submodule at one. + spelled that way would name nothing the doctor could ever hold to it. A + directory's answer keeps the one `/` the plan wrote after it, and is + spelled as Git records a path without it (#525). A backslash or a drive + letter is accepted: on POSIX each is a character of a name Git records, + and an adoption records a submodule at one. """ - fault = path_spelling_fault(path) if isinstance(path, str) else None + named = (path[:-1] if record.directories and isinstance(path, str) + and path.endswith("/") else path) + fault = path_spelling_fault(named) if isinstance(named, str) else None if fault == PATH_NOT_CANONICAL: - return (f"path is {path!r}; Git records no path with a `.` segment, " - "a trailing `/` or a doubled `/`, so no submodule is ever at " - "it") + return f"path is {path!r}; {record.not_canonical}" if fault is not None or not isinstance(path, str): - return f"path is {path!r}; {SUBMODULE_PATH}" + return f"path is {path!r}; {record.path_is}" if path in seen: return f"path {path!r} is recorded twice" - seen.add(path) + under = next((other for other in seen + if _covers(other, path) or _covers(path, other)), None) + seen.append(path) + if under is not None: + return (f"path {path!r} overlaps the row for {under!r}: a path is " + "answered once") return None -def _submodule_row_findings(index: int, row, seen: set) -> list[str]: - """One row of the `submodules:` record. +def _record_row_findings(record: _Record, index: int, row, + seen: list) -> list[str]: + """One row of a record. - Split from `_submodules_findings` so that each reads as one question. + Split from `_record_findings` so that each reads as one question. """ - where = f"submodules[{index}]" + where = f"{record.key}[{index}]" if not isinstance(row, dict): return [_finding( - "manifest-submodules", + f"manifest-{record.key.replace('_', '-')}", f"{where} is {row!r}, expected a mapping with `path:` and " "`leg:`")] out = [] - problem = _submodule_path_problem(row.get("path"), seen) + problem = _record_path_problem(record, row.get("path"), seen) if problem: - out.append(_finding("manifest-submodule-path", f"{where}: {problem}")) + out.append(_finding(f"{record.code}-path", f"{where}: {problem}")) if row.get("leg") not in SUBMODULE_LEGS: out.append(_finding( - "manifest-submodule-leg", + f"{record.code}-leg", f"{where}: leg is {row.get('leg')!r}, expected one of " - f"{list(SUBMODULE_LEGS)}: the assembly root keeps no submodule " - "but its legs")) + f"{list(SUBMODULE_LEGS)}: {record.why_leg}")) resolution = row.get("resolution") if resolution is not None and not isinstance(resolution, str): out.append(_finding( - "manifest-submodule-resolution", + f"{record.code}-resolution", f"{where}: resolution is {resolution!r}, expected the text the " "adoption plan gave")) commit = row.get("source_commit") if commit is not None and not COMMIT_RE.match(str(commit)): out.append(_finding( - "manifest-submodule-commit", + f"{record.code}-commit", f"{where}: source_commit is {commit!r}, not 40 hex")) return out -def _submodules_findings(manifest: dict) -> list[str]: - """`submodules`, OPTIONAL: the leg each source submodule was answered - into when this project was adopted (#189). +def _record_findings(manifest: dict, record: _Record) -> list[str]: + """`submodules` or `answered_paths`, OPTIONAL: the leg each source + submodule (#189), or each path the policy asked about (#525), was + answered into when this project was adopted. A RECORD IS CHECKED BECAUSE SOMETHING READS IT. `shape-doctor.py` holds every submodule a leg keeps to the row naming its path, and calls one in - the other leg MISPLACED; a row it could not read would leave that - submodule unchecked, and nothing would say so. Absent is fine: a project - with no submodule of its source's has nothing to record, and nor does - one adopted before the record was written. + the other leg MISPLACED; it takes an answered path's row as the answer + to the question it would ask again of it. A row it could not read would + leave that submodule unchecked, or that question asked, and nothing + would say so. Absent is fine: a project with nothing to record has none, + and nor does one adopted before the record was written. """ - rows = manifest.get("submodules") + rows = manifest.get(record.key) if rows is None: return [] if not isinstance(rows, list): return [_finding( - "manifest-submodules", - f"submodules is {rows!r}, expected a list of `path:` and `leg:` " - "records")] - seen: set[str] = set() + f"manifest-{record.key.replace('_', '-')}", + f"{record.key} is {rows!r}, expected a list of `path:` and " + "`leg:` records")] + seen: list[str] = [] out: list[str] = [] for index, row in enumerate(rows): - out.extend(_submodule_row_findings(index, row, seen)) + out.extend(_record_row_findings(record, index, row, seen)) return out @@ -786,7 +844,8 @@ def _findings(manifest: dict, policy: NamingPolicy, root=None) -> list[str]: out.extend(_reference_finding(manifest)) out.extend(_visibility_finding(manifest)) out.extend(_shape_findings(manifest)) - out.extend(_submodules_findings(manifest)) + out.extend(_record_findings(manifest, SUBMODULE_ROWS)) + out.extend(_record_findings(manifest, ANSWER_ROWS)) legs = manifest.get("legs") if not isinstance(legs, list) or not legs: diff --git a/tests/test_doctor_recorded_answers.py b/tests/test_doctor_recorded_answers.py new file mode 100644 index 00000000..f6d652b2 --- /dev/null +++ b/tests/test_doctor_recorded_answers.py @@ -0,0 +1,578 @@ +# SPDX-License-Identifier: Apache-2.0 +"""The adoption's answers outlive the plan, and a record's row is a person's +to change (#525: #457's N7 and L1-08). + +THE DEFECTS, on main 2247dbff: + + * N7. `execute` recorded the plan's answers about submodules only (#189). + A path the policy cannot call -- `examples/`, rule `ambiguous-examples` + -- answered `spec` in the plan was asked again by the doctor's + `placement` row on every run after the split: `spec/examples/` stayed + `review_required`, and AGENTS.md's doctor step 4 had an assistant ask + the human the question the human had already answered. + * L1-08. Nothing said how a `submodules:` row changes. A submodule moved + to the other leg on purpose -- a pull request on each leg, `bump-leg.py` + for each pin -- stayed `MISPLACED (1 path)`, exit 1, with a NEXT of + `--placement-plan`, a plan of the move already made. A row whose + submodule was removed was silent: COMPLIANT, and `manifest ok`. + +THE FIX. `execute` appends `answered_paths:` after `submodules:`, one row +per entry kept in a leg that covers a path the policy's own walk leaves +`leg: null`, and the doctor takes each row as the answer to that question in +that leg -- never in the other, where the path stays a question, and never +as a finding. A submodule whose recorded leg no longer holds it is still +MISPLACED, says it moved, and names no plan: its NEXT is this doctor again, +as the `manifest kinds` row's is for a file only a person can mend, and its +WHY gives both ways out. A row no leg holds is a `note`. AGENTS.md says the +person edits the row in the pull request that bumps the pins. + +Most of these run on the doctor's own scaffolded project, a record written +by hand, and the index staged as a leg would hold it, so they need no `git +filter-repo`. The adoptions at the end are the real thing, end to end. +""" + +from __future__ import annotations + +import json +import shutil +import sys +from pathlib import Path + +import pytest + +from conftest import FILE_PROTOCOL, REPO, clone_just_pushed, git, run_script +#: The doctor's own fixtures and helpers, from the module that owns them: a +#: project made any other way would be a different project. +from test_shape_doctor import ( # noqa: F401 + ORG, adoption_module, commit_all, doctor, doctor_module, + needs_filter_repo, project, record_submodules, rows_of, scaffolded_here, + stage, stage_gitlink, standard, verdict_line, +) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import parse_yaml # noqa: E402 + +#: The paths the tests below name again and again. +RULES_PATH = "vendor/rules" +EXAMPLES = "examples/" +ASKED = "spec/examples/" +GITMODULES = ".gitmodules" +README = "README.md" +BLOB = "100644" +COMPLIANT = "COMPLIANT" +#: Why the plans below answered `examples/` into the spec leg. +EXAMPLES_ANSWER = "the specification cites the golden runs" +#: Why they answered the editor rules' submodule into the code leg. +RULES_ANSWER = "editor rules the code leg's tooling reads" +#: A golden run, as two files the policy cannot call (`ambiguous-examples`). +GOLDEN = ("examples/golden/case.json", "examples/golden/case.expected.json") +MOVED_CLAUSE = ("1 submodule(s) moved out of the leg project.yaml's " + "`submodules:` records for them, which no longer holds them " + "(#525): spec/vendor/rules (recorded in the code leg)") + + +def record_answers(project: Path, *rows: tuple) -> None: + """Append `answered_paths:`, one `(path, leg[, resolution])` row each, + in the very bytes `record_submodules` writes a `submodules:` row in: + the two records are one shape.""" + manifest = project / "project.yaml" + before = manifest.read_bytes() + record_submodules(project, *rows) + block = manifest.read_bytes()[len(before):] + assert block.startswith(b"\nsubmodules:\n"), block + manifest.write_bytes(before + b"\nanswered_paths:\n" + + block[len(b"\nsubmodules:\n"):]) + + +def placement(standard: Path, root: Path) -> tuple: + """`(result, verdict, the placement row)` from one `--json` run.""" + result = doctor(standard, root, "--json") + return (result, json.loads(result.stdout)["verdict"], + rows_of(result)["placement"]) + + +def doctor_again(standard: Path, root: Path) -> str: + """This doctor's own line for `root`, spelled as `unreadable_next` spells + it: the NEXT of a row whose way out is a person's.""" + module = doctor_module(standard) + return (f"{module.PYTHON} " + f"{module.quote_arg(standard.resolve() / 'shape-doctor.py')} " + f"--root {module.quote_arg(root.resolve())}") + + +def golden_run(project: Path, leg: str = "spec") -> None: + for name in GOLDEN: + stage(project / leg, name, '{"case": 1}\n') + + +# --- N7: the doctor takes a recorded answer as the answer ------------------- + +def test_an_answered_question_is_not_asked_again_in_its_leg(standard, + project): + """`spec/examples/` is the policy's question; a row answering it `spec` + settles it there. The row is `ok`, names where the answer came from, + and `--json` lists it under `answered`, never under `review_required`. + """ + golden_run(project) + asked = placement(standard, project)[2] + assert [row["path"] for row in asked["detail"]["review_required"]] == [ + ASKED], asked + record_answers(project, (EXAMPLES, "spec", EXAMPLES_ANSWER)) + result, verdict, row = placement(standard, project) + assert result.returncode == 0, result.stdout + result.stderr + assert verdict == COMPLIANT + assert rows_of(result)["manifest"]["status"] == "ok" + assert row["status"] == "ok", row + assert row["detail"]["review_required"] == [], row["detail"] + assert row["detail"]["counts"]["answered"] == 1 + [answered] = row["detail"]["answered"] + assert (answered["path"], answered["rule"], answered["recorded_leg"], + answered["answered_by"], answered["resolution"], + answered["review_required"]) == ( + ASKED, "ambiguous-examples", "spec", [EXAMPLES], + EXAMPLES_ANSWER, False), answered + assert row["reason"].startswith( + "every tracked path classifies as the leg it is in or is answered " + "there; project.yaml's `answered_paths:` answers the question the " + "policy asks of 1 path(s) (#525): spec/examples/ (rule " + "ambiguous-examples) ["), row["reason"] + assert row["next"] is None, row + + +def test_an_answer_for_the_other_leg_leaves_the_question_asked(standard, + project): + """RECORDS CONFER NOTHING. A row answering `examples/` into the code leg + is no answer to the spec leg's `examples/`, which is another + repository's file: the question stands, a `note`, and nothing is + MISPLACED.""" + golden_run(project) + record_answers(project, (EXAMPLES, "code", EXAMPLES_ANSWER)) + result, verdict, row = placement(standard, project) + assert result.returncode == 0, result.stdout + result.stderr + assert verdict == COMPLIANT + assert row["status"] == "note", row + assert row["detail"]["misplaced"] == [] + assert [item["path"] for item in row["detail"]["review_required"]] == [ + ASKED] + assert "answered" not in row["detail"], row["detail"] + + +@pytest.mark.parametrize("rows, answered", [ + ((("examples/golden/case.json", "spec"), + ("examples/golden/case.expected.json", "spec")), True), + ((("examples/golden/", "spec"),), True), + ((("examples/golden/case.json", "spec"),), False), + ((("examples/golden/case.json", "spec"), + ("examples/golden/case.expected.json", "code")), False), +], ids=["every-file-its-own-row", "a-directory-row", "one-file-unanswered", + "one-file-the-other-leg"]) +def test_a_question_is_answered_file_by_file(standard, project, rows, + answered): + """The doctor's walk folds a leg's files as they fall there, not as the + source's fell, so a question is answered only when every file under it + is, by exactly one row that names this leg.""" + golden_run(project) + record_answers(project, *rows) + row = placement(standard, project)[2] + asked = [item["path"] for item in row["detail"]["review_required"]] + assert asked == ([] if answered else [ASKED]), row["detail"] + assert row["status"] == ("ok" if answered else "note"), row + + +def test_gitmodules_is_answered_by_the_submodule_record_alone(standard, + project): + """`.gitmodules` asks which leg owns the submodules it registers, and + only `submodules:` answers that, where it places every one (#189). A + row of `answered_paths:` naming it settles nothing: beside a submodule + added by hand, the file stays the question it was.""" + stage_gitlink(project / "code", "vendor/lib") + stage(project / "code", GITMODULES, + '[submodule "vendor/lib"]\n\tpath = vendor/lib\n' + "\turl = https://example.invalid/lib.git\n") + record_answers(project, (GITMODULES, "code", RULES_ANSWER)) + row = placement(standard, project)[2] + assert [item["path"] for item in row["detail"]["review_required"]] == [ + "code/.gitmodules"], row["detail"] + assert "answered" not in row["detail"], row["detail"] + + +def test_an_answer_no_leg_holds_is_silent(standard, project): + """A recorded answer can only settle a question, never make a finding, + so one whose path has gone says nothing: the report is the scaffold's + own, word for word.""" + before = placement(standard, project)[2] + record_answers(project, (EXAMPLES, "spec", EXAMPLES_ANSWER)) + result, verdict, row = placement(standard, project) + assert result.returncode == 0, result.stdout + result.stderr + assert (verdict, row["status"], row["reason"]) == ( + COMPLIANT, "ok", before["reason"]), row + + +# --- L1-08: a submodule moved, and one removed ------------------------------ + +def moved_rules(project: Path, *, gitmodules: bool = False) -> None: + """`vendor/rules` recorded in the code leg and held by the spec leg + alone: the move, already made in both legs.""" + stage_gitlink(project / "spec", RULES_PATH) + if gitmodules: + stage(project / "spec", GITMODULES, + '[submodule "vendor/rules"]\n\tpath = vendor/rules\n' + "\turl = https://example.invalid/rules.git\n") + record_submodules(project, (RULES_PATH, "code", RULES_ANSWER)) + + +@pytest.mark.parametrize("gitmodules", [False, True], + ids=["the-gitlink", "with-its-gitmodules"]) +def test_a_submodule_moved_in_both_legs_names_its_row_not_a_plan( + standard, project, gitmodules): + """STILL MISPLACED, NO LONGER A PLAN. The record names the code leg, + which holds no `vendor/rules`, and the spec leg holds it: the move is + made, and the plan `--placement-plan` writes would be that move again. + The row says it moved; its NEXT is this doctor again and its WHY gives + both ways out, the row edited by hand or the move undone. The spec + leg's `.gitmodules`, a question only because the row names the other + leg, goes with the move.""" + moved_rules(project, gitmodules=gitmodules) + result, verdict, row = placement(standard, project) + assert result.returncode == 1, result.stdout + result.stderr + assert verdict == "MISPLACED (1 path)" + assert row["status"] == "FINDING", row + [moved] = row["detail"]["misplaced"] + assert (moved["path"], moved["moved"]) == ("spec/vendor/rules", True) + assert MOVED_CLAUSE in row["reason"], row["reason"] + assert row["next"] == doctor_again(standard, project), row["next"] + assert row["why"] == doctor_module(standard).MOVED_BY_HAND + for said in ("`leg:` to the leg it is in", "`source_commit:` deleted", + "in the pull request that bumps the pins", + "a pull request on each leg and a pin bump in the root"): + assert said in row["why"], (said, row["why"]) + text = doctor(standard, project) + assert verdict_line(text).startswith("MISPLACED (1 path) (exit 1)") + assert f"NEXT {row['next']}" in text.stdout, text.stdout + + +@pytest.mark.parametrize("also, moved", [ + ("code-holds-it-too", False), ("a-question-beside", True), + ("code-leg-unread", False)]) +def test_a_move_not_made_in_both_legs_keeps_the_plan(standard, project, + also, moved): + """WHERE NOT EVERYTHING IS A MOVE ALREADY MADE, THE PLAN STAYS. Both legs + holding it is a move half made, and a recorded leg nobody could read + could hold it: neither is said to have moved. A question beside a move + is the plan's to write, so the plan is the NEXT, and the move is still + said.""" + moved_rules(project) + if also == "code-holds-it-too": + stage_gitlink(project / "code", RULES_PATH) + elif also == "a-question-beside": + golden_run(project) + else: + shutil.rmtree(project / "code") + (project / "code").mkdir() + row = placement(standard, project)[2] + assert row["status"] == "FINDING", row + assert row["next"].endswith("--placement-plan placement-plan.yaml"), row + assert [bool(item.get("moved")) for item in row["detail"]["misplaced"] + ] == [moved], row["detail"]["misplaced"] + assert (MOVED_CLAUSE in row["reason"]) == moved, row["reason"] + + +def test_a_row_no_leg_holds_is_a_note(standard, project): + """REMOVING A SUBMODULE IS NOT WRONG. Its row lingering is said -- a + `note`, `detail.unheld` -- and never a finding, with no NEXT.""" + record_submodules(project, (RULES_PATH, "code", RULES_ANSWER)) + result, verdict, row = placement(standard, project) + assert result.returncode == 0, result.stdout + result.stderr + assert verdict == COMPLIANT + assert row["status"] == "note", row + assert row["detail"]["unheld"] == [{ + "path": RULES_PATH, "recorded_leg": "code", + "resolution": RULES_ANSWER}], row["detail"] + assert row["detail"]["counts"]["unheld"] == 1 + assert ("project.yaml's `submodules:` records 1 submodule(s) no leg " + "holds (#525): vendor/rules (recorded in the code leg); one " + "removed on purpose has its row deleted by hand, in the pull " + "request that removed it") in row["reason"], row["reason"] + assert row["next"] is None, row + + +def test_a_row_is_not_called_unheld_where_a_leg_was_not_read(standard, + project): + """A leg nobody could read could hold it: nothing is said.""" + record_submodules(project, (RULES_PATH, "code", RULES_ANSWER)) + shutil.rmtree(project / "code") + (project / "code").mkdir() + row = placement(standard, project)[2] + assert "unheld" not in row["detail"], row["detail"] + + +def test_agents_md_says_who_edits_a_row_and_when(): + """The way out the doctor's WHY names is the one AGENTS.md gives: a row + is the human's, edited by hand in the pull request that bumps the pins, + and that is not the manifest edit Checking 3 forbids.""" + said = " ".join((REPO / "AGENTS.md").read_text(encoding="utf-8").split()) + assert ("A move or removal the human decided is recorded by editing its " + "row by hand, in the pull request that bumps the pins: that is " + "not the edit 3 forbids.") in said + assert "no command rewrites them" in said + + +# --- the record `execute` writes --------------------------------------------- + +def tree_of(*rows: tuple) -> list: + """`Source.tree()`'s shape: `(path, mode, oid, size)`, sorted.""" + return sorted((path, mode, "0" * 40, 1) for path, mode in rows) + + +#: A source's tree: what the rules call, what they cannot, and a submodule. +SOURCE_TREE = tree_of( + (README, BLOB), ("src/app/util.py", BLOB), + ("docs/index.md", BLOB), ("examples/a/case.json", BLOB), + ("examples/b/case.json", BLOB), (GITMODULES, BLOB), + (RULES_PATH, "160000"), ("vendor/data.csv", BLOB)) +SOURCE_COMMIT = "ab" * 20 + + +def plan_entries(**legs: str) -> list: + """Plan entries covering `SOURCE_TREE`, each `path=leg`, with a reason + on every one; `_` in a keyword stands for `/` and `dot_` for `.`.""" + return [{"path": path.replace("dot_", ".").replace("_", "/"), + "leg": leg, "resolution": f"why {leg}"} + for path, leg in legs.items()] + + +@pytest.mark.parametrize("answers, rows", [ + ({"examples_": "spec"}, [(EXAMPLES, "spec")]), + ({"examples_": "root"}, []), + ({"examples_": "drop"}, []), + ({"examples_a_": "spec", "examples_b_": "code"}, + [("examples/a/", "spec"), ("examples/b/", "code")]), + ({"examples_": "spec", "docs_": "code"}, [(EXAMPLES, "spec")]), +], ids=["answered-spec", "answered-root", "answered-drop", "split-by-hand", + "a-confident-rule-overridden"]) +def test_execute_records_the_policys_own_questions_answered_into_a_leg( + standard, answers, rows): + """ONE ROW PER ENTRY KEPT IN A LEG THAT COVERS A PATH THE POLICY LEAVES + `leg: null`, read from the policy's own walk and not the plan's edited + `review_required:`: an entry split by hand is each half its own row. + `root` and `drop` are in no leg. `.gitmodules` and a gitlink are + `submodules:`'s, and `vendor/` is recorded only because the policy + cannot call the file beside its gitlink. A path a confident rule called + and the plan answered the other way is not a question the policy asked + (outside #525).""" + adopt = adoption_module(standard) + legs = {README: "root", "src_": "code", "docs_": "spec", + "dot_gitmodules": "code", "vendor_": "code", **answers} + entries = plan_entries(**{key: leg for key, leg in legs.items() + if key != README}) + entries.append({"path": README, "leg": "root"}) + block = adopt.answered_path_record(adopt.PathPolicy.load( + adopt.PATH_POLICY), entries, SOURCE_TREE, SOURCE_COMMIT) + recorded = parse_yaml(block)["answered_paths"] if block else [] + expected = sorted(rows + [("vendor/", "code")]) + assert [(row["path"], row["leg"]) for row in recorded] == expected + assert all(row["resolution"] == f"why {row['leg']}" + and row["source_commit"] == SOURCE_COMMIT + for row in recorded), recorded + + +def test_a_plan_that_answered_no_question_into_a_leg_appends_nothing( + standard): + """THE MANIFEST IT ALWAYS GOT. No question of the policy's answered into + a leg -- only `.gitmodules` and a gitlink, which `submodules:` answers, + and paths the rules call -- and the record is "", which `execute` adds + to nothing.""" + adopt = adoption_module(standard) + tree = [row for row in SOURCE_TREE + if not row[0].startswith((EXAMPLES, "vendor/data"))] + entries = plan_entries(src_="code", docs_="spec", dot_gitmodules="code", + vendor_="code") + entries.append({"path": README, "leg": "root"}) + assert adopt.answered_path_record( + adopt.PathPolicy.load(adopt.PATH_POLICY), entries, tree, + SOURCE_COMMIT) == "" + + +# --- end to end: `execute`, then the doctor --------------------------------- + +def adopt_source(standard: Path, base: Path, answers: dict) -> dict: + """A source with a golden run under `examples/` and the editor rules as + a submodule at `vendor/rules`, planned, answered as `answers` says + (`{entry: (leg, resolution)}`), executed into local remotes and cloned + recursively from the split branch.""" + dependency = base / "rules" + dependency.mkdir() + git("init", "-q", "-b", "main", ".", cwd=dependency) + (dependency / "rules.md").write_text("editor rules\n", encoding="utf-8") + commit_all(dependency, "rules") + source = base / "Thing" + source.mkdir() + git("init", "-q", "-b", "main", ".", cwd=source) + for name, body in ((README, "# Thing\n"), + ("src/app/util.py", "VALUE = 1\n"), + ("docs/index.md", "# Docs\n"), + *((name, '{"case": 1}\n') for name in GOLDEN)): + (source / name).parent.mkdir(parents=True, exist_ok=True) + (source / name).write_text(body, encoding="utf-8") + commit_all(source, "init") + git(*FILE_PROTOCOL, "submodule", "add", "-q", str(dependency), + RULES_PATH, cwd=source) + commit_all(source, "vendor the editor rules") + adopt = standard / "adopt-project.py" + plan = base / "adoption-plan.yaml" + written = run_script(adopt, "plan", "--source", str(source), "--project", + "Northwind", "--org", ORG, "--elected-by", "T", + "--elected-on", "2026-10-15", "--out", str(plan)) + assert written.returncode == 0, written.stdout + written.stderr + text = plan.read_text(encoding="utf-8") + for path, (leg, why) in answers.items(): + needle = f" - path: {path}\n leg: null\n" + assert needle in text, (path, text) + text = text.replace(needle, f" - path: {path}\n leg: {leg}\n" + f' resolution: "{why}"\n') + plan.write_bytes(text.encode("utf-8")) + done = run_script(adopt, "execute", "--plan", str(plan), "--yes", + "--local-remote-dir", str(base / "remotes"), + "--work-dir", str(base / "work")) + assert done.returncode == 0, done.stdout + done.stderr + assert "adoption verified" in done.stdout, done.stdout + clone = base / "clone" + proc = clone_just_pushed(source, clone, branch="adopt/three-repo-shape") + assert proc.returncode == 0, proc.stderr + return {"clone": clone, "dependency": dependency, + "commit": git("rev-parse", "main", cwd=source).stdout.strip()} + + +#: Each plan's answers to the submodule question: `.gitmodules` and the +#: entry holding the gitlink, into the code leg. +RULES_IN_CODE = {GITMODULES: ("code", RULES_ANSWER), + "vendor/": ("code", RULES_ANSWER)} + + +@pytest.fixture(scope="module") +def adopted_golden(standard, tmp_path_factory) -> dict: + """`examples/` answered into the spec leg, the rules into the code leg.""" + return adopt_source(standard, tmp_path_factory.mktemp("answered"), + {EXAMPLES: ("spec", EXAMPLES_ANSWER), + **RULES_IN_CODE}) + + +@pytest.fixture +def golden_clone(adopted_golden, tmp_path) -> Path: + """A private, mutable copy of that clone.""" + target = tmp_path / "Northwind" + shutil.copytree(adopted_golden["clone"], target, symlinks=True) + return target + + +@needs_filter_repo +def test_execute_records_the_answer_and_the_doctor_asks_no_more( + standard, adopted_golden, golden_clone): + """N7, END TO END. The split's manifest answers `examples/` beside the + submodule's row, and the doctor, which asked `spec/examples/` on main, + reads the adoption COMPLIANT with the row `ok`.""" + manifest = parse_yaml((golden_clone / "project.yaml").read_text( + encoding="utf-8")) + commit = adopted_golden["commit"] + assert manifest["answered_paths"] == [{ + "path": EXAMPLES, "leg": "spec", "resolution": EXAMPLES_ANSWER, + "source_commit": commit}], manifest["answered_paths"] + assert manifest["submodules"] == [{ + "path": RULES_PATH, "leg": "code", "resolution": RULES_ANSWER, + "source_commit": commit}], manifest["submodules"] + result, verdict, row = placement(standard, golden_clone) + assert result.returncode == 0, result.stdout + result.stderr + assert verdict == COMPLIANT + assert row["status"] == "ok", row + assert row["detail"]["review_required"] == [], row["detail"] + assert [item["path"] for item in row["detail"]["answered"]] == [ + ASKED] + + +def the_rules_row(commit: str) -> str: + """The `submodules:` row `execute` wrote for `vendor/rules`.""" + return (' - path: "vendor/rules"\n leg: code\n' + f' resolution: "{RULES_ANSWER}"\n' + f' source_commit: "{commit}"\n') + + +def edit_manifest(root: Path, old: str, new: str) -> None: + """The person's hand edit, in bytes, so the file keeps its LF endings.""" + manifest = root / "project.yaml" + text = manifest.read_text(encoding="utf-8") + assert text.count(old) == 1, text + manifest.write_bytes(text.replace(old, new).encode("utf-8")) + + +def drop_the_rules(leg: Path) -> None: + """`git rm` of the submodule, and of the `.gitmodules` it empties.""" + git("rm", "-q", RULES_PATH, cwd=leg) + if not (leg / GITMODULES).read_text(encoding="utf-8").strip(): + git("rm", "-q", "-f", GITMODULES, cwd=leg) + + +@needs_filter_repo +def test_a_move_made_in_both_legs_is_finished_by_its_row( + standard, adopted_golden, golden_clone): + """L1-08, END TO END. The rules leave the code leg and join the spec + leg as `git` does it: MISPLACED, the doctor's own line as the NEXT. The + person then edits the row as the WHY says -- `leg:`, `resolution:`, no + `source_commit:` -- and the doctor and the project's validator agree.""" + drop_the_rules(golden_clone / "code") + git(*FILE_PROTOCOL, "submodule", "add", "-q", + str(adopted_golden["dependency"]), RULES_PATH, + cwd=golden_clone / "spec") + result, verdict, row = placement(standard, golden_clone) + assert result.returncode == 1, result.stdout + result.stderr + assert verdict == "MISPLACED (1 path)" + assert MOVED_CLAUSE in row["reason"], row["reason"] + assert row["next"] == doctor_again(standard, golden_clone), row["next"] + edit_manifest(golden_clone, the_rules_row(adopted_golden["commit"]), + ' - path: "vendor/rules"\n leg: spec\n' + ' resolution: "the spec leg reads the rules now"\n') + result, verdict, row = placement(standard, golden_clone) + assert result.returncode == 0, result.stdout + result.stderr + assert verdict == COMPLIANT + assert rows_of(result)["manifest"]["status"] == "ok" + assert row["status"] == "ok", row + + +@needs_filter_repo +def test_a_removed_submodule_is_a_note_until_its_row_is_deleted( + standard, adopted_golden, golden_clone): + """L1-08's other half, END TO END: the rules removed, the row is a + `note`, COMPLIANT; the row deleted, the placement row is `ok` again.""" + drop_the_rules(golden_clone / "code") + result, verdict, row = placement(standard, golden_clone) + assert result.returncode == 0, result.stdout + result.stderr + assert (verdict, row["status"]) == (COMPLIANT, "note"), row + assert [item["path"] for item in row["detail"]["unheld"]] == [ + RULES_PATH] + edit_manifest(golden_clone, the_rules_row(adopted_golden["commit"]), "") + result, verdict, row = placement(standard, golden_clone) + assert result.returncode == 0, result.stdout + result.stderr + assert rows_of(result)["manifest"]["status"] == "ok" + assert row["status"] == "ok", row + + +@pytest.fixture(scope="module") +def adopted_unasked(standard, tmp_path_factory) -> dict: + """The same source with `examples/` kept in the root: no question of + the policy's answered into a leg.""" + return adopt_source(standard, tmp_path_factory.mktemp("unasked"), + {EXAMPLES: ("root", EXAMPLES_ANSWER), + **RULES_IN_CODE}) + + +@needs_filter_repo +def test_a_split_with_no_answered_question_gets_the_manifest_it_always_got( + standard, adopted_unasked): + """BYTE FOR BYTE. What `execute` appends to `project.yaml` is the #189 + block and nothing after it: no `answered_paths:` key, and the file ends + on the submodule's row exactly as `submodule_record` writes it.""" + adopt = adoption_module(standard) + text = (adopted_unasked["clone"] / "project.yaml").read_bytes() + block = ("\n".join(adopt.SUBMODULE_RECORD_PREAMBLE) + "\n" + + the_rules_row(adopted_unasked["commit"])).encode("utf-8") + assert text.endswith(block), text[-600:] + assert b"answered_paths" not in text diff --git a/tests/test_manifest_validator.py b/tests/test_manifest_validator.py index 028c8d02..3f8d20e5 100644 --- a/tests/test_manifest_validator.py +++ b/tests/test_manifest_validator.py @@ -614,13 +614,43 @@ def test_an_install_form_is_a_leg_of_nothing(project): ) +#: The record of the plan's other answers (#525), as `execute` writes it +#: after `submodules:`: a directory's row keeps its `/`, and a row may carry +#: no `resolution:`. +ANSWER_RECORD = ( + "\n# THE PATHS THE ADOPTION'S POLICY ASKED ABOUT (#525)\n" + "answered_paths:\n" + ' - path: "examples/"\n' + " leg: spec\n" + ' resolution: "the specification cites the golden runs"\n' + ' source_commit: "0123456789abcdef0123456789abcdef01234567"\n' + ' - path: "notebooks/fit.ipynb"\n' + " leg: code\n" + ' source_commit: "0123456789abcdef0123456789abcdef01234567"\n' +) + + def append_record(project, text: str = SUBMODULE_RECORD) -> None: manifest = project / "project.yaml" manifest.write_bytes((manifest.read_text() + text).encode("utf-8")) -def test_a_submodule_record_is_accepted(project): - append_record(project) +def assert_refused(project, record: str, old: str, new: str, + code: str) -> str: + """`record` with `old` made `new`, appended: a finding named `code`, and + the validator's stderr for a test that reads the sentence.""" + assert old in record, f"fixture drift: {old!r}" + append_record(project, record.replace(old, new, 1)) + result = validate(project) + assert result.returncode == 1, result.stderr + result.stdout + assert code in result.stderr, result.stderr + return result.stderr + + +@pytest.mark.parametrize("record", [SUBMODULE_RECORD, ANSWER_RECORD], + ids=["submodules", "answered-paths"]) +def test_a_record_is_accepted(project, record): + append_record(project, record) result = validate(project) assert result.returncode == 0, result.stderr + result.stdout assert "manifest ok" in result.stdout @@ -660,11 +690,59 @@ def test_a_submodule_record_is_accepted(project): "doubled-slash", "dot-segment", "the-leg-itself"]) def test_a_submodule_record_row_that_says_nothing_is_a_finding(project, old, new, code): - assert old in SUBMODULE_RECORD, f"fixture drift: {old!r}" - append_record(project, SUBMODULE_RECORD.replace(old, new, 1)) + assert_refused(project, SUBMODULE_RECORD, old, new, code) + + +#: The second row of `ANSWER_RECORD`, whose path the cases below respell. +SECOND_ANSWER = ' - path: "notebooks/fit.ipynb"\n' + + +@pytest.mark.parametrize("old, new, code", [ + (" leg: spec\n", " leg: root\n", "manifest-answered-leg"), + (' - path: "examples/"\n', ' - path: "../examples/"\n', + "manifest-answered-path"), + (' - path: "examples/"\n', ' - path: ""\n', "manifest-answered-path"), + (' - path: "examples/"\n', ' - path: "/"\n', "manifest-answered-path"), + (' - path: "examples/"\n', ' - path: "examples//"\n', + "manifest-answered-path"), + (' - path: "examples/"\n', ' - path: "./examples/"\n', + "manifest-answered-path"), + (SECOND_ANSWER, ' - path: "examples/"\n', "manifest-answered-path"), + (SECOND_ANSWER, ' - path: "examples/golden/a.json"\n', + "manifest-answered-path"), + (' resolution: "the specification cites the golden runs"\n', + " resolution: 3\n", "manifest-answered-resolution"), + (' source_commit: "0123456789abcdef0123456789abcdef01234567"\n', + ' source_commit: "main"\n', "manifest-answered-commit"), + (SECOND_ANSWER + " leg: code\n" + ' source_commit: "0123456789abcdef0123456789abcdef01234567"\n', + ' - "notebooks/fit.ipynb"\n', "manifest-answered-paths"), +], ids=["leg-root", "dotdot-path", "empty-path", "the-root", "doubled-slash", + "leading-dot-slash", "path-twice", "path-under-another", + "resolution-not-text", "commit-not-hex", "row-not-a-mapping"]) +def test_an_answer_record_row_that_says_nothing_is_a_finding(project, old, + new, code): + """THE #189 ROW'S RULES, AND ONE OF ITS OWN (#525): a directory's row + keeps the one `/` the plan wrote it with, and no row lies under + another, because `check` refuses a path two entries cover and the doctor + takes an answer from the one row that covers a file.""" + assert_refused(project, ANSWER_RECORD, old, new, code) + + +def test_an_answer_record_that_is_not_a_list_is_a_finding(project): + append_record(project, "\nanswered_paths: examples/\n") result = validate(project) assert result.returncode == 1, result.stderr + result.stdout - assert code in result.stderr, result.stderr + assert "manifest-answered-paths" in result.stderr, result.stderr + + +def test_an_answer_under_another_says_which_row_it_overlaps(project): + """The finding names both rows, so a reader knows which to keep.""" + said = assert_refused(project, ANSWER_RECORD, SECOND_ANSWER, + ' - path: "examples/golden/a.json"\n', + "manifest-answered-path") + assert ("answered_paths[1]: path 'examples/golden/a.json' overlaps the " + "row for 'examples/': a path is answered once") in said, said def test_a_submodule_record_that_is_not_a_list_is_a_finding(project): diff --git a/tests/test_shape_doctor.py b/tests/test_shape_doctor.py index d2ca4ed2..db0e7656 100644 --- a/tests/test_shape_doctor.py +++ b/tests/test_shape_doctor.py @@ -7524,7 +7524,10 @@ def test_a_submodule_in_the_other_leg_from_its_record_is_misplaced( like any other: in `misplaced`, counted in the verdict and the leg, named with the leg recorded, and an entry under `paths:` in the plan that the adoption's own reader accepts. Nothing else is in the wrong leg here, so - the reason is that one clause and no separator hangs after it. + the reason is that one clause and no separator hangs after it. The spec + leg holds no `.cursor`, so it has MOVED (#525): the clause says so, and + the NEXT is the doctor again with the hand edit in its WHY, not a plan + of the move already made -- though `--placement-plan` still writes one. """ stage_gitlink(project / "code", ".cursor") record_submodules(project, (".cursor", "spec", "rules the spec reads")) @@ -7541,7 +7544,7 @@ def test_a_submodule_in_the_other_leg_from_its_record_is_misplaced( "classified_as": "spec", "rule": None, "reason": reason, "confidence": "recorded", "question": None, "files": 1, "bytes": 0, "review_required": False, "direction": "spec in the code leg", - "submodule": True}] + "submodule": True, "moved": True}] [held] = row["detail"]["submodules"] assert held["placement"] == "misplaced", held assert held["recorded_leg"] == "spec", held @@ -7551,10 +7554,11 @@ def test_a_submodule_in_the_other_leg_from_its_record_is_misplaced( code = [leg for leg in row["detail"]["legs"] if leg["role"] == "code"][0] assert code["misplaced"] == 1, code assert row["reason"] == ( - "1 submodule(s) sit in a leg other than the one project.yaml's " - "`submodules:` records for them (#189): code/.cursor (recorded in " - f"the spec leg) [{audited_legs(row)}]"), row["reason"] - assert "--placement-plan" in row["next"], row + "1 submodule(s) moved out of the leg project.yaml's `submodules:` " + "records for them, which no longer holds them (#525): code/.cursor " + f"(recorded in the spec leg) [{audited_legs(row)}]"), row["reason"] + assert "--placement-plan" not in row["next"], row + assert row["why"] == doctor_module(standard).MOVED_BY_HAND, row adopt = adoption_module(standard) data = adopt.load_yaml(out) From b640d34e89be4dba42fc2702569fc6e59b2cb8c8 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 14:53:57 +0000 Subject: [PATCH 2/3] shape-doctor.py: a move whose `git rm` left the emptied `.gitmodules` tracked in the leg the submodule left names its row and not a plan, a project with no `answered_paths:` no longer holds each question against every file, and a row no leg holds names the pull request that bumps the pins (#525) Review r525 on 3b78fae6. F1: `git rm` of a leg's only submodule leaves `.gitmodules` tracked and 0 bytes long, and that file is the policy's question. `moved_alone` let a `.gitmodules` go with the move only in the leg the submodule moved INTO, so the most natural move still named `--placement-plan`. It now also lets it go in a leg a moved submodule LEFT, where that leg keeps no submodule at all. Test first: a third case of `test_a_submodule_moved_in_both_legs_names_its_row_not_a_plan` stages the emptied file in the code leg; it failed at 3b78fae6 on the NEXT. F2: `answered_questions` returns the questions as they are when there is no `answered_paths:`, its first statement, instead of holding each one against every file of its leg. F3: the unheld clause says the row is deleted in the pull request that bumps the pins, where `project.yaml` is, as AGENTS.md and `MOVED_BY_HAND` say. F4: the keep-the-plan test asserts #189's own MISPLACED clause word for word where the submodule did not move, and its absence where it did; and tests/test_doctor_next_is_a_command.py's CASES has the moved NEXT. F5: adopt-project.py spells the manifest `SHAPE_MANIFEST` in the new line. F6: the new `--json` key is `answered_paths` (under `detail` and `counts`), apart from the per-leg `answered` of #189. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 2 +- shape-doctor.py | 41 +++++++++++++++++--------- tests/test_doctor_next_is_a_command.py | 15 ++++++++++ tests/test_doctor_recorded_answers.py | 33 ++++++++++++++------- 4 files changed, 65 insertions(+), 26 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 82e04afa..c7b9c240 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -8220,7 +8220,7 @@ def _commit_the_split(plan: Plan, source: Source, assembly: Path, # And the plan's answers to the policy's own questions, after it # (#525): `answered_path_record`, read before any leg existed. if answers: - append["project.yaml"] = append.get("project.yaml", "") + answers + append[SHAPE_MANIFEST] = append.get(SHAPE_MANIFEST, "") + answers # Each declared pin's file, `contracts/-pin.yaml`, is # written by the materializer as the scaffold's is, from the # values the plan records (#273); `_refuse_what_check_finds` has diff --git a/shape-doctor.py b/shape-doctor.py index 4786b219..16564fd4 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -208,12 +208,12 @@ plan's answer to each question the policy itself asked -- `examples/` under `ambiguous-examples` -- under `answered_paths:`, one row per entry kept in a leg. A walked question whose every file one row answers into the leg it is -in is settled: named in the row, in `--json` under `answered`, and no reason -to keep the row off `ok`. A row naming the OTHER leg settles nothing and -asserts nothing: the path stays the question it was, a `note`, and is never -MISPLACED, because a record confers nothing and a file named like one the -adoption put in the other repository is not that file. A row whose path no -leg holds is silent, because it can only settle a question, never make a +in is settled: named in the row, in `--json` under `answered_paths`, and no +reason to keep the row off `ok`. A row naming the OTHER leg settles nothing +and asserts nothing: the path stays the question it was, a `note`, and is +never MISPLACED, because a record confers nothing and a file named like one +the adoption put in the other repository is not that file. A row whose path +no leg holds is silent, because it can only settle a question, never make a finding. And a leg's `.gitmodules` is answered by `submodules:` alone. A SUBMODULE MOVED OR REMOVED SINCE THE ADOPTION (#525). Its row is a @@ -563,7 +563,7 @@ def not_in_the_standard(detail: str) -> int: THE_ANSWERS = "project.yaml's `answered_paths:`" #: The key under `detail`, and under `counts`, of the questions that record #: answered (#525); present only where it answered one. -ANSWERED_PATHS = "answered" +ANSWERED_PATHS = "answered_paths" #: The key that marks a `misplaced` submodule as one its recorded leg no #: longer holds (#525); present only on one. MOVED = "moved" @@ -4225,7 +4225,7 @@ def unheld_clause(unheld: list) -> str: f"{row[RECORDED_LEG]} leg)" for row in unheld]) return (f"{THE_RECORD} records {len(unheld)} submodule(s) no leg holds " f"(#525): {named}; one removed on purpose has its row deleted by " - "hand, in the pull request that removed it") + "hand, in the pull request that bumps the pins") def classified(detail: dict, what: str) -> str: @@ -4523,7 +4523,14 @@ def answered_questions(adopt, review: list, role: str, files: list, `.gitmodules` answered here: `submodules:` answers it, and only where it places every submodule the file registers (`gitmodules_the_record_ answers`). + + NO RECORD, NO SCAN (r525's F2). A project with no `answered_paths:` -- + every one adopted before #525 -- returns its questions as they are, + which is what the loop below would return for them, without holding + each question against every file of its leg. """ + if not answers: + return review, [] rows = list(answers) still: list = [] answered: list = [] @@ -5036,7 +5043,7 @@ def moved_and_unheld(per_leg: list, submodules: list, misplaced: list, def moved_alone(adopt, detail: dict) -> bool: """Is every path the row lists a submodule moved since the adoption, or - the `.gitmodules` of the leg it moved into (#525)? + the `.gitmodules` of a leg the move went into or emptied (#525)? THEN NO PLAN IS THE WAY OUT. The `--placement-plan` writes the paths to move, and these have moved: what is left is the row in `project.yaml`, @@ -5044,24 +5051,30 @@ def moved_alone(adopt, detail: dict) -> bool: registers what moved in is the policy's question only because the row still names the other leg, so it goes with the move -- where every submodule that leg keeps is recorded there or moved in, and every name - in it could be read. Anything else in the wrong leg, or asked, keeps the - plan as the NEXT. + in it could be read. So does the `.gitmodules` a plain `git rm` leaves, + tracked and empty, in the leg a moved submodule LEFT, where that leg now + keeps no submodule at all (r525's F1). Anything else in the wrong leg, + or asked, keeps the plan as the NEXT. """ misplaced = detail["misplaced"] if not misplaced or not all(row.get(MOVED) for row in misplaced): return False moved = {row["path"] for row in misplaced} + left = {row["classified_as"] for row in misplaced} + kept = detail.get("submodules") or [] for question in detail["review_required"]: role = question["leg"] leg = next(entry for entry in detail["legs"] if entry["role"] == role) + went = any(path.startswith(f"{leg['path']}/") for path in moved) + emptied = role in left and not any(item["leg"] == role + for item in kept) if (question["path_in_leg"] != adopt.GITMODULES or leg.get("unwritable") - or not any(path.startswith(f"{leg['path']}/") - for path in moved) + or not (went or emptied) or any(item["leg"] == role and item["placement"] != SUBMODULE_RECORDED and item["path"] not in moved - for item in detail.get("submodules") or [])): + for item in kept)): return False return True diff --git a/tests/test_doctor_next_is_a_command.py b/tests/test_doctor_next_is_a_command.py index 3a7f1b86..f3968d4f 100644 --- a/tests/test_doctor_next_is_a_command.py +++ b/tests/test_doctor_next_is_a_command.py @@ -225,6 +225,17 @@ def _placement_plan(standard, project, holder, tmp_path): return rows_of(doctor(standard, project, "--json"))["placement"] +def _placement_moved(standard, project, holder, tmp_path): + # A submodule moved since the adoption (#525): the spec leg holds it, + # and the row names the code leg, which holds none. Its NEXT is the + # doctor again, as `unreadable_next`'s is for a file only a person can + # mend, and its `why` gives both ways out. + from test_shape_doctor import record_submodules, stage_gitlink + stage_gitlink(project / "spec", "vendor/rules") + record_submodules(project, ("vendor/rules", "code")) + return rows_of(doctor(standard, project, "--json"))["placement"] + + def _agent_files_pinned(standard, project, holder, tmp_path): (project / "AGENTS-shape.md").unlink() return rows_of(doctor(standard, project, "--json"))["agent-files"] @@ -544,6 +555,10 @@ def _way_in(standard, project, holder, tmp_path): "writes the paths above as a plan to resolve by hand. Moving one is a " "pull request on each leg and a pin bump in the root, so this " "command makes neither"), + "check_placement-moved": ( + _placement_moved, + "each submodule above is in a leg its row in project.yaml's " + "`submodules:` does not name"), "check_agent_files-pinned": ( _agent_files_pinned, "the pin has a `files:` row for it, so it was deleted rather than " diff --git a/tests/test_doctor_recorded_answers.py b/tests/test_doctor_recorded_answers.py index f6d652b2..4a417f49 100644 --- a/tests/test_doctor_recorded_answers.py +++ b/tests/test_doctor_recorded_answers.py @@ -69,6 +69,10 @@ MOVED_CLAUSE = ("1 submodule(s) moved out of the leg project.yaml's " "`submodules:` records for them, which no longer holds them " "(#525): spec/vendor/rules (recorded in the code leg)") +#: #189's own clause, for a submodule against its record that did not move. +HELD_CLAUSE = ("1 submodule(s) sit in a leg other than the one project.yaml's " + "`submodules:` records for them (#189): spec/vendor/rules " + "(recorded in the code leg)") def record_answers(project: Path, *rows: tuple) -> None: @@ -111,7 +115,8 @@ def test_an_answered_question_is_not_asked_again_in_its_leg(standard, project): """`spec/examples/` is the policy's question; a row answering it `spec` settles it there. The row is `ok`, names where the answer came from, - and `--json` lists it under `answered`, never under `review_required`. + and `--json` lists it under `answered_paths`, never under + `review_required`. """ golden_run(project) asked = placement(standard, project)[2] @@ -124,8 +129,8 @@ def test_an_answered_question_is_not_asked_again_in_its_leg(standard, assert rows_of(result)["manifest"]["status"] == "ok" assert row["status"] == "ok", row assert row["detail"]["review_required"] == [], row["detail"] - assert row["detail"]["counts"]["answered"] == 1 - [answered] = row["detail"]["answered"] + assert row["detail"]["counts"]["answered_paths"] == 1 + [answered] = row["detail"]["answered_paths"] assert (answered["path"], answered["rule"], answered["recorded_leg"], answered["answered_by"], answered["resolution"], answered["review_required"]) == ( @@ -154,7 +159,7 @@ def test_an_answer_for_the_other_leg_leaves_the_question_asked(standard, assert row["detail"]["misplaced"] == [] assert [item["path"] for item in row["detail"]["review_required"]] == [ ASKED] - assert "answered" not in row["detail"], row["detail"] + assert "answered_paths" not in row["detail"], row["detail"] @pytest.mark.parametrize("rows, answered", [ @@ -193,7 +198,7 @@ def test_gitmodules_is_answered_by_the_submodule_record_alone(standard, row = placement(standard, project)[2] assert [item["path"] for item in row["detail"]["review_required"]] == [ "code/.gitmodules"], row["detail"] - assert "answered" not in row["detail"], row["detail"] + assert "answered_paths" not in row["detail"], row["detail"] def test_an_answer_no_leg_holds_is_silent(standard, project): @@ -221,18 +226,23 @@ def moved_rules(project: Path, *, gitmodules: bool = False) -> None: record_submodules(project, (RULES_PATH, "code", RULES_ANSWER)) -@pytest.mark.parametrize("gitmodules", [False, True], - ids=["the-gitlink", "with-its-gitmodules"]) +@pytest.mark.parametrize("gitmodules, emptied", [ + (False, False), (True, False), (True, True)], + ids=["the-gitlink", "with-its-gitmodules", "the-emptied-one-left"]) def test_a_submodule_moved_in_both_legs_names_its_row_not_a_plan( - standard, project, gitmodules): + standard, project, gitmodules, emptied): """STILL MISPLACED, NO LONGER A PLAN. The record names the code leg, which holds no `vendor/rules`, and the spec leg holds it: the move is made, and the plan `--placement-plan` writes would be that move again. The row says it moved; its NEXT is this doctor again and its WHY gives both ways out, the row edited by hand or the move undone. The spec leg's `.gitmodules`, a question only because the row names the other - leg, goes with the move.""" + leg, goes with the move. So does the code leg's, emptied and left + tracked, which is what a plain `git rm` of a leg's only submodule + leaves: that leg keeps no submodule at all now (r525's F1).""" moved_rules(project, gitmodules=gitmodules) + if emptied: + stage(project / "code", GITMODULES, "") result, verdict, row = placement(standard, project) assert result.returncode == 1, result.stdout + result.stderr assert verdict == "MISPLACED (1 path)" @@ -275,6 +285,7 @@ def test_a_move_not_made_in_both_legs_keeps_the_plan(standard, project, assert [bool(item.get("moved")) for item in row["detail"]["misplaced"] ] == [moved], row["detail"]["misplaced"] assert (MOVED_CLAUSE in row["reason"]) == moved, row["reason"] + assert (HELD_CLAUSE in row["reason"]) != moved, row["reason"] def test_a_row_no_leg_holds_is_a_note(standard, project): @@ -292,7 +303,7 @@ def test_a_row_no_leg_holds_is_a_note(standard, project): assert ("project.yaml's `submodules:` records 1 submodule(s) no leg " "holds (#525): vendor/rules (recorded in the code leg); one " "removed on purpose has its row deleted by hand, in the pull " - "request that removed it") in row["reason"], row["reason"] + "request that bumps the pins") in row["reason"], row["reason"] assert row["next"] is None, row @@ -485,7 +496,7 @@ def test_execute_records_the_answer_and_the_doctor_asks_no_more( assert verdict == COMPLIANT assert row["status"] == "ok", row assert row["detail"]["review_required"] == [], row["detail"] - assert [item["path"] for item in row["detail"]["answered"]] == [ + assert [item["path"] for item in row["detail"]["answered_paths"]] == [ ASKED] From 45d080aece7521801ec3320c32b4ebf9aaa06671 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Thu, 15 Oct 2026 18:10:30 +0000 Subject: [PATCH 3/3] shape-doctor.py and its test: three comments cite #525, not a reviewer's working name (#525) Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 4 ++-- tests/test_doctor_recorded_answers.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index a4d2ce0a..e5945d48 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -5677,7 +5677,7 @@ def answered_questions(adopt, review: list, role: str, files: list, places every submodule the file registers (`gitmodules_the_record_ answers`). - NO RECORD, NO SCAN (r525's F2). A project with no `answered_paths:` -- + NO RECORD, NO SCAN (#525). A project with no `answered_paths:` -- every one adopted before #525 -- returns its questions as they are, which is what the loop below would return for them, without holding each question against every file of its leg. @@ -6208,7 +6208,7 @@ def moved_alone(adopt, detail: dict) -> bool: submodule that leg keeps is recorded there or moved in, and every name in it could be read. So does the `.gitmodules` a plain `git rm` leaves, tracked and empty, in the leg a moved submodule LEFT, where that leg now - keeps no submodule at all (r525's F1). Anything else in the wrong leg, + keeps no submodule at all (#525). Anything else in the wrong leg, or asked, keeps the plan as the NEXT. """ misplaced = detail["misplaced"] diff --git a/tests/test_doctor_recorded_answers.py b/tests/test_doctor_recorded_answers.py index 4a417f49..32c5ede0 100644 --- a/tests/test_doctor_recorded_answers.py +++ b/tests/test_doctor_recorded_answers.py @@ -239,7 +239,7 @@ def test_a_submodule_moved_in_both_legs_names_its_row_not_a_plan( leg's `.gitmodules`, a question only because the row names the other leg, goes with the move. So does the code leg's, emptied and left tracked, which is what a plain `git rm` of a leg's only submodule - leaves: that leg keeps no submodule at all now (r525's F1).""" + leaves: that leg keeps no submodule at all now (#525).""" moved_rules(project, gitmodules=gitmodules) if emptied: stage(project / "code", GITMODULES, "")