From 557a259556cb4bbe1b8da9b428799f08edf62490 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Wed, 14 Oct 2026 17:09:49 +0000 Subject: [PATCH 1/3] shape-doctor.py: `manifest kinds` prints one command for its NEXT, the `git rm`, `git clean` or `git status` its files need as git has them, where it printed prose joined by `; ` whose backticks ran `family.py` in zsh, and counts a manifest `validated` only at a path its gate reads, so a `contracts/-pin.yaml` no leg or product names is a FINDING (#548, #439) #548. The rule across the doctor is that a NEXT is one pasteable command or nothing, and prose is the row's `why` (#277). `manifest kinds` broke it: each fault's sentence was the NEXT, joined by "; ", and #211's FAMILY_BESIDE_FIX held `family.py init` and `family.py add` in backticks, which zsh, interactive or `-c`, runs where the line is pasted. Now every fault but a file that does not load is a file to take out of the root, so the NEXT is the one command `removal_next` names for them as git has them: `git rm` (committed), `git clean -f` (`-x` where one is ignored), or the `git status` that shows changes `git rm` will not discard. The `why` says what it does, names files the next run handles, and gives each fault's other way out as its own sentence (register the kind, move what it describes to its home, move the membership to a holder, declare the pin), as the human decides. A name git would read as a pathspec pattern is passed after `--literal-pathspecs`: `git rm -- 'contracts/shape-pin[.]yaml'` took the shape pin out with it. A refusal's remedy reaches no NEXT either (#408): `run_checks`, for a check that lets a `Refusal` out, and `placement_policy`, for a standard whose path policy does not load, print no NEXT and put the remedy, less its `Remediation:` label, in the `why`. The #286 linked-record row's NEXT was the one other prose NEXT left in the doctor; it is now that row's `why`. #439. Brett Heap ruled on 2026-10-12: "#439 (registered kind at a path no check reads): FINDING". `ManifestKind.read_at` names where the check that runs on this root reads each kind: `project.yaml`, `family.yaml`, `contracts/repository-naming.yaml`, and for pins the shape pin plus, in an assembly root, `declared_pins` (a pin per leg and per `neutral_product_pins:` entry). A file of a kind whose check runs here, at any other path, is listed under `unread` with the path, the kind and where that kind is read, and is a FINDING whose NEXT takes it out as git has it. Only a pin in an assembly root is offered the human declaring it, and the row never declares one. A file is read by any name it has (`samefile`), so a case-folded or hard-linked leg pin stays `validated`; while `project.yaml` does not load, no pin is called unread. `path-classification-policy` is not decided by path, as before. An untouched root's row is byte for byte what it was. Tests: `rows_of` holds every row the suite reads to "no backtick outside a quoted value, no `Remediation:`" (the guard #408 asked for), shown to fail on the lines main printed; the #277 property covers five new manifest kinds fixtures; a zsh paste shows the old #211 line running `family.py` and the new NEXT running none; #439's five files each run to COMPLIANT by their NEXT; refusal rows from `run_checks` and `placement_policy` carry no NEXT. Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 471 +++++++++++++++------ tests/test_doctor_next_is_a_command.py | 203 +++++++++- tests/test_shape_doctor.py | 539 +++++++++++++++++++------ 3 files changed, 964 insertions(+), 249 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 343a83ba..54a653c8 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -53,7 +53,11 @@ runs on THIS kind of root is that failure too, and the same FINDING (#327): `validate-family.py` is registered for `family-manifest` and runs in a family holder only, so a `contracts/family.yaml` in an assembly root -- or a -`contracts/project.yaml` in a holder -- is a manifest nothing here reads. +`contracts/project.yaml` in a holder -- is a manifest nothing here reads. So +is one at a path its gate never opens, in a root that runs it (#439): every +gate reads fixed paths, so a `contracts/project.yaml` in an assembly root, +or a `contracts/-pin.yaml` no leg and no product names, is the same +FINDING, and its NEXT takes the file out as git has it (#548). THE ROWS, for an assembly root, in this order: @@ -74,7 +78,8 @@ pins the project's own `scripts/validate-pins.py` likewise: the lockstep invariant, and the shape-copy digests manifest kinds every `kind:`-bearing manifest in the root, against - MANIFEST_VALIDATORS and the kind of root it is in + MANIFEST_VALIDATORS, the kind of root it is in and the + path it sits at shape currency `update-shape.py check`'s per-file verdicts, summarised legs each leg mounted, and its checked-out commit against the pin (no fetch) @@ -818,14 +823,30 @@ class ManifestKind: is one sentence on every platform, so it names `setup-project.py` beside `setup.sh`, which is a bash script Windows cannot run -- the reason `scaffold_command` spells that line per platform (Copilot, PR #102). + + AND ONLY AT A PATH THAT CHECK READS (#439). Each gate opens fixed paths: + `validate-manifest.py` the root's `project.yaml` (and the naming policy + at `contracts/repository-naming.yaml`), `validate-pins.py` the shape pin + and each pin `project.yaml` declares, `validate-family.py` the holder's + `family.yaml`, the shape pin and the naming policy. So a + `contracts/project.yaml` in an assembly root, or a + `contracts/release-pin.yaml` that no leg and no product names, is read + by nothing, though its kind's check runs here. `read_at(ctx)` says where + the check that runs here reads the kind, as root paths, and + `check_manifest_kinds` counts a file `validated` only at one of them. + None is a kind this row does not decide by path: + `path-classification-policy`, which the registry covers with this row + itself, and a pin in an assembly root whose `project.yaml` does not + load, which declares nothing this run can read (`pins_read_here`). """ def __init__(self, kind: str, covered_by: tuple, how: str, - home: str | None = None): + home: str | None = None, read_at=None): self.kind = kind self.covered_by = covered_by self.how = how self.home = home + self.read_at = read_at def runs_in(self) -> tuple: """The kinds of root a check that `covered_by` names runs on.""" @@ -834,24 +855,55 @@ def runs_in(self) -> tuple: and root in check.applies_to for check in CHECKS)) + def reads(self, ctx: Context) -> set | None: + """The root paths a check that runs here reads this kind at, or None + where this row does not decide by path (#439).""" + return None if self.read_at is None else self.read_at(ctx) + + +#: Where an assembly root keeps the naming policy `validate-manifest.py` +#: reads, and a holder the one `validate-family.py` reads (#439). +NAMING_COPY = "contracts/repository-naming.yaml" + + +def pins_read_here(ctx: Context) -> set | None: + """Where this root's gate reads a `pinned_contract_manifest` (#439). + + In a holder, `validate-family.py` reads the shape pin and no other: a + holder has no legs and declares no product. In an assembly root, + `validate-pins.py` reads `declared_pins` -- the shape pin, a pin per leg + and a pin per `neutral_product_pins:` entry. Where `project.yaml` does + not load, it declares nothing this run can read, and None leaves every + pin counted as before: a leg pin called read by nothing would be a NEXT + that takes it out, over a manifest the `manifest` row already names. + """ + if ctx.kind == FAMILY: + return {SHAPE_PIN} + if ctx.manifest_refusal is not None: + return None + return declared_pins(ctx) + MANIFEST_VALIDATORS: dict[str, ManifestKind] = { "project-manifest": ManifestKind( "project-manifest", ("manifest",), "scripts/validate-manifest.py", "an assembly root of its own (`setup.sh`, or `setup-project.py` on " "Windows, for a new one; `adopt-project.py plan` for a repository " - "that exists), which a holder pins with `family.py add`"), + "that exists), which a holder pins with `family.py add`", + read_at=lambda ctx: {PROJECT_MANIFEST}), "family-manifest": ManifestKind( "family-manifest", ("family",), "scripts/validate-family.py", "a family holder of its own (`family.py init`, then `family.py add` " - "there)"), + "there)", read_at=lambda ctx: {FAMILY_MANIFEST}), "pinned_contract_manifest": ManifestKind( "pinned_contract_manifest", ("pins", "family"), - "scripts/validate-pins.py (or validate-family.py in a holder)"), + "scripts/validate-pins.py (or validate-family.py in a holder)", + read_at=pins_read_here), "repository-naming-policy": ManifestKind( "repository-naming-policy", ("naming", "family"), "scripts/validate-repository-naming.py, through NamingPolicy.load " - "(validate-family.py, through the same, in a holder)"), + "(validate-family.py, through the same, in a holder)", + read_at=lambda ctx: {NAMING_COPY}), "path-classification-policy": ManifestKind( "path-classification-policy", ("manifest-kinds",), "scripts/path_classify.py reads it; nothing validates it in a root"), @@ -1558,6 +1610,25 @@ def refusal_line(refusal: Refusal) -> str: return line.removesuffix("\n" + refusal.remediation) +def remedy_as_why(refusal: Refusal) -> str | None: + """A refusal's remedy as a row's `why`, less its `Remediation:` label, + or None where it carries none (#408, #548). + + NEVER AS A ROW'S NEXT. A remedy is prose a tool prints under its own + refusal, and the one most tools used to print, `repo_shape.REMEDIATION`, + held `git submodule update` and `validate-pins.py` in backticks, which + an interactive zsh runs where the line is pasted (#277). Two places -- + `run_checks`, for a check that lets a refusal out, and + `placement_policy`, for a standard whose own path policy does not load + -- printed it as the NEXT. They print no NEXT now, and the remedy, when + there is one, is the row's `why`, so what the refusal had to say is not + lost. + """ + said = (refusal.remediation or "").strip() + said = said.removeprefix("Remediation:").strip() + return said or None + + def unloaded_manifest_row(ctx: Context, check_id: str, label: str, own: str, template: str) -> Row: """The `manifest` (or `family`) row for a manifest that is THERE and does @@ -2199,28 +2270,47 @@ def family_beside_project(ctx: Context, data: dict) -> tuple[str, dict]: "undeclared_gitlinks": undeclared} -#: The `manifest kinds` row's next command for #211, named once so the row -#: and `tests/test_shape_doctor.py` spell it the same way. NOT `family.py -#: add`, which is what `validate-family.py`'s own refusal offers: run here, -#: it would mount the members INSIDE a project, which is the one shape the -#: family ruling rules out. Which of the two exits is right is a fact about -#: the repository, so the line offers both and decides neither. -FAMILY_BESIDE_FIX = ( - "an assembly root is not a family holder: take family.yaml out of this " - "root, or move the membership to a holder of its own (`family.py init`, " - "then `family.py add` there), as the human decides -- never `family.py " - "add` here, which would mount the members inside a project") +#: The other way out of #211's finding, a sentence of the `manifest kinds` +#: row's `why` and never its NEXT (#548): the backticks it holds are +#: commands an interactive zsh runs where the line is pasted. NOT `family.py +#: add` here, which is what `validate-family.py`'s own refusal offers: run +#: in an assembly root it would mount the members INSIDE a project, which is +#: the one shape the family ruling rules out. Which exit is right is a fact +#: about the repository, so the row names both and decides neither. +FAMILY_BESIDE_WAY = ( + "Or move the membership family.yaml records to a holder of its own " + "(`family.py init`, then `family.py add` there), as the human decides " + "-- never `family.py add` here, which would mount the members inside a " + "project") + + +def listed(names: list) -> str: + """`names` as a sentence lists them.""" + return ", ".join(names) -def registered_elsewhere(ctx: Context, found: list) -> tuple[str, str, list]: +def register_way(names: list) -> str: + """The other way out for files of a kind nothing registers, a sentence + of the `manifest kinds` row's `why` (#548): what its NEXT said, less the + removal its command now is.""" + one = len(names) == 1 + return (f"Or register the kind{'' if one else 's'} {listed(names)} " + f"declare{'s' if one else ''} in shape-doctor.py's " + "MANIFEST_VALIDATORS, beside the check that validates " + f"{'it' if one else 'each'}, as the human decides") + + +def registered_elsewhere(ctx: Context, found: list, + states: dict) -> tuple[str, list, list]: """Manifests whose kind is registered to a gate this root never runs: - the sentence, the next command, and the facts (#327). + the sentence, the other way out, and the facts (#327). - `found` is `(path, kind, entry)` for each, in the row's own order. The - sentence opens the way the unregistered kind's does -- "no validator - ... is registered for kind" -- because to this root it is the same - fact: the registry has a validator for the kind, and none that runs - here. Each file says which validator that is and where it does run. + `found` is `(path, kind, entry)` for each, in the row's own order, and + `states` each file's git state, from `git_states`. The sentence opens + the way the unregistered kind's does -- "no validator ... is registered + for kind" -- because to this root it is the same fact: the registry has + a validator for the kind, and none that runs here. Each file says which + validator that is and where it does run. A FINDING, NOT A NOTE, for the reason an unregistered kind is one. A `note` is a difference nothing asserts, and a file declaring `kind:` @@ -2229,32 +2319,16 @@ def registered_elsewhere(ctx: Context, found: list) -> tuple[str, str, list]: in a root whose gate never reads it. Nor could the same bytes be a FINDING as `family.yaml` (#211) and a note as `contracts/family.yaml`. - THE NEXT COMMAND IS ONE THAT EXISTS, FOR THE FILE AS GIT HAS IT. The - doctor reads the disk, so the file may be one nobody has committed yet, - and `git rm` takes out only a file whose every byte a commit holds: it - says "did not match any files" (exit 128) for one git does not track, - ignored or not, and refuses one with changes no commit holds. So - `git_states` asks first, and each file gets the exit that works: - committed `git rm`, quoted for the reader's shell like every path - this report prints, committed on a branch and landed as - a pull request, the way every change here lands; - untracked, deleting it, with nothing to commit; - ignored - changed committing or dropping those changes first, since `git - rm` will not discard them, and the next run names the - `git rm`. - Where git cannot say, the line is #211's, which holds in any state: - take it out of this root. Each file's state is `detail`'s `git`. - - The other exit is the kind's `home`, as the human decides. Renaming the - file to the root's own manifest name is ruled out in the line itself, + The row's NEXT takes the file out as git has it (`removal_next`), and + this names the other exit, the kind's `home`, as the human decides -- + a sentence of the row's `why`, never of its NEXT (#548). Renaming the + file to the root's own manifest name is ruled out in that sentence, because that is the one move that looks like a fix: in a holder it makes the root read as a project, and in an assembly root it is #211's - finding instead of this one. + finding instead of this one. Each file's state is `detail`'s `git`. """ names = [rel for rel, _, _ in found] one = len(names) == 1 - states = git_states(ctx.root, names) clauses, detail = [], [] for rel, kind, entry in found: runs = entry.runs_in() @@ -2270,18 +2344,84 @@ def registered_elsewhere(ctx: Context, found: list) -> tuple[str, str, list]: homes = {rel: entry.home or "a root whose gate runs its validator" for rel, _, entry in found} if len(set(homes.values())) == 1: - other_exit = (f"move what {'it describes' if one else 'they describe'}" - f" to {homes[names[0]]}") + other_exit = (f"move what {listed(names)} " + f"describe{'s' if one else ''} to {homes[names[0]]}") else: other_exit = "move what each describes to " + "; ".join( f"{rel}: {home}" for rel, home in homes.items()) - fix = (f"take {', '.join(names)} out of this root" - f"{removal_ways(ctx, names, states)}, or {other_exit}, as the " - f"human decides -- never rename {'it' if one else 'one'} to " - f"{PROJECT_MANIFEST} or {FAMILY_MANIFEST} in this root: a family " - "holder is not a project, and an assembly root is not a family " - "holder") - return sentence, fix, detail + way = (f"Or {other_exit}, as the human decides -- never rename " + f"{'it' if one else 'one'} to {PROJECT_MANIFEST} or " + f"{FAMILY_MANIFEST} in this root: a family holder is not a " + "project, and an assembly root is not a family holder") + return sentence, [way], detail + + +def read_nowhere(ctx: Context, found: list, + states: dict) -> tuple[str, list, list]: + """Manifests of a kind whose check runs here, at a path that check never + reads: the sentence, the other ways out, and the facts (#439). + + `found` is `(path, kind, paths)` for each, in the row's own order, where + `paths` are the root paths the check that runs here reads that kind at + (`ManifestKind.read_at`), and `states` each file's git state. + + A FINDING, as Brett Heap ruled on 2026-10-12 ("#439 (registered kind at + a path no check reads): FINDING"), for #327's reason: a file that + declares `kind:` asserts what it is, and a kind whose check runs here is + no better answered at a path that check never opens than in a root + where it never runs. The sentence names the path, the kind and where a + gate here does read that kind. + + The row's NEXT takes the file out as git has it (`removal_next`). The + other way out is the human declaring it, and only a pin in an assembly + root can be declared: a product this root descends from is named under + `neutral_product_pins:`, and AGENTS.md says no pin is declared that the + human has not asked for, so the sentence says that too and never + declares one itself. Nothing here suggests renaming the file onto a path + a gate reads -- that would put its bytes where another file's are read + -- and the last sentence says so. + """ + clauses, detail = [], [] + for rel, kind, paths in found: + clauses.append(f"{rel} ({kind}, which a gate here reads only at " + f"{listed(paths)})") + detail.append({"manifest": rel, "kind": kind, "read_at": paths, + "git": states.get(rel)}) + sentence = "no gate here reads " + ", ".join(clauses) + pins = [rel for rel, kind, _ in found + if ctx.kind == PROJECT and kind == "pinned_contract_manifest"] + ways = [] + if pins: + ways.append( + f"Or, if {listed(pins)} pin{'s' if len(pins) == 1 else ''} a " + "product this root descends from, the human names that product " + f"under `neutral_product_pins:` in {PROJECT_MANIFEST} -- and " + "nobody declares a pin the human has not asked for") + ways.append(f"Never rename {'it' if len(found) == 1 else 'one'} onto a " + "path a gate here reads") + return sentence, ways, detail + + +def read_here(ctx: Context, path: Path, rel: str, paths: set) -> bool: + """Whether a gate here reads the file at `path` (`rel` from the root), + `paths` being the root paths it reads that kind at (#439). + + BY THE FILE, NOT ONLY BY ITS SPELLING. A filesystem that folds case + hands `validate-pins.py` the file `contracts/Spec-pin.yaml` when it + opens `contracts/spec-pin.yaml`, and a hard link is the file its other + name is. Called read by nothing, such a file would get a NEXT that takes + out a pin a gate does read. So a name that is not one of `paths` is + still read where it is the same file as one of them. + """ + if rel in paths: + return True + for other in paths: + try: + if path.samefile(ctx.root / other): + return True + except OSError: + continue + return False def git_states(root: Path, names: list) -> dict: @@ -2335,44 +2475,73 @@ def git_states(root: Path, names: list) -> dict: return states -def removal_ways(ctx: Context, names: list, states: dict) -> str: - """How each of `names` comes out of the root, as git has it: the - parenthesis the `manifest kinds` row's NEXT carries (#327), or nothing - where git could not say. A file is named again only where the files - do not all come out the same way.""" - root = quote_arg(ctx.root) +#: What git reads as a pattern in a pathspec. A name holding one is passed +#: after `--literal-pathspecs`, or `git rm -- 'contracts/shape-pin[.]yaml'` +#: would take `contracts/shape-pin.yaml` out with it. +PATHSPEC_MAGIC = re.compile(r"[*?\[\\]") + + +def removal_next(ctx: Context, names: list, + states: dict) -> tuple[str | None, str]: + """`(command, said)`: the ONE command that takes `names` out of the + root as git has them, and the sentence that says what it does -- the + `manifest kinds` row's NEXT and the start of its `why` (#327, #548). + + THE NEXT IS ONE COMMAND AND NOTHING ELSE (#548). It used to be prose: + "take ... out of this root" with each way in a parenthesis, joined by + `; ` to the next fault's sentence and holding commands in backticks, + which an interactive zsh runs where the line is pasted (#277). Now the + command is the first of these that applies and the sentence goes in the + row's `why`; a file that comes out another way is named there, and the + next run names its command. + + FOR THE FILE AS GIT HAS IT. The doctor reads the disk, so the file may + be one nobody has committed yet, and `git rm` takes out only a file + whose every byte a commit holds: it says "did not match any files" + (exit 128) for one git does not track, ignored or not, and refuses one + with changes no commit holds. So `git_states` asks first, and: + committed `git rm`, committed on a branch and landed as a pull + request, the way every change here lands; + untracked, `git clean -f`, `-x` too where one is ignored, which + ignored `clean` otherwise leaves: nothing to commit; + changed `git status`, which shows the changes, since `git rm` + will not discard them: commit or drop them first. + Each path is quoted for the reader's shell like every path this report + prints, and passed after `--literal-pathspecs` where git would read it + as a pattern. Where git cannot say, there is no command, and the + sentence is #211's, which holds in any state: take it out of this root. + """ groups = {state: [name for name in names if states.get(name) in kinds] for state, kinds in (("committed", ("committed",)), ("loose", ("untracked", "ignored")), ("changed", ("changed",)))} groups = {state: group for state, group in groups.items() if group} - alone = len(groups) == 1 - - def subject(group: list, pronoun: str) -> str: - return pronoun if alone else ", ".join(group) - - ways = [] - if "committed" in groups: - ways.append(f"`git -C {root} rm -- " - + " ".join(quote_arg(n) for n in groups["committed"]) - + "`, committed on a branch and landed as a pull " - "request") - if "loose" in groups: - group = groups["loose"] - single = len(group) == 1 - ways.append(f"delete {subject(group, 'it' if single else 'them')}, " - "which git does not track, so there is nothing to " - "commit") - if "changed" in groups: - group = groups["changed"] - single = len(group) == 1 - ways.append( - f"{subject(group, 'it' if single else 'they')} " - f"{'has' if single else 'have'} changes no commit holds, which " - "`git rm` will not discard: commit or drop them (`git -C " - f"{root} status -- {' '.join(quote_arg(n) for n in group)}` " - "shows them), and the next run names the `git rm`") - return f" ({'; '.join(ways)})" if ways else "" + if not groups: + return None, f"take {listed(names)} out of this root" + state, group = next(iter(groups.items())) + literal = (" --literal-pathspecs" + if any(PATHSPEC_MAGIC.search(name) for name in group) else "") + git_c = f"git -C {quote_arg(ctx.root)}{literal}" + spelled = " ".join(quote_arg(name) for name in group) + if state == "committed": + command = f"{git_c} rm -- {spelled}" + said = (f"takes {listed(group)} out of this root: commit that on a " + "branch and land it as a pull request") + elif state == "loose": + ignored = any(states[name] == "ignored" for name in group) + command = f"{git_c} clean -f{' -x' if ignored else ''} -- {spelled}" + said = (f"deletes {listed(group)}, which git does not track, so " + "there is nothing to commit") + else: + command = f"{git_c} status -- {spelled}" + said = (f"shows the changes no commit holds in {listed(group)}, " + "which `git rm` will not discard: commit or drop them, and " + "the next run names the way out") + rest = [name for other, more in groups.items() if other != state + for name in more] + if rest: + said += f". The next run names the way out for {listed(rest)}" + return command, said def check_manifest_kinds(ctx: Context) -> Row: @@ -2394,6 +2563,26 @@ def check_manifest_kinds(ctx: Context) -> Row: assembly root is the same fact with more to say: `family_beside_project` also checks the members it pins against the gitlinks. + AND SO IS ONE AT A PATH ITS GATE NEVER READS, in a root that runs that + gate (#439, ruled a FINDING by Brett Heap on 2026-10-12). Each gate + opens fixed paths, and `ManifestKind.read_at` says which, so a + `contracts/project.yaml` in an assembly root, a `contracts/family.yaml` + or `contracts/spec-pin.yaml` in a holder, or a `contracts/-pin.yaml` + no leg and no product names, is listed under `unread` and is the FINDING + `read_nowhere()` explains. Every file a root's own gates read is + `validated` as before, so an untouched root's row is byte for byte the + row it was. + + THE NEXT IS ONE COMMAND, OR NOTHING (#548). Each fault but a file that + does not load is a file to take out of the root, so the NEXT is the one + command `removal_next` names for all of them, as git has them, and the + `why` says what it does and then, a sentence each, the other way out of + each fault, which is the human's to choose: register the kind, move + what the file describes to its `home`, declare the pin, move a + membership to a holder. Those sentences were the NEXT, joined by `; `, + and held `family.py init` and `family.py add` in backticks, which an + interactive zsh runs where the line is pasted (#277). + THE ROOT'S OWN MANIFEST, WHEN IT DOES NOT LOAD, IS NOT THIS ROW'S (#205). The `manifest` (or `family`) row quotes its refusal and names its gate, and the kind of the root was read off its filename for exactly that @@ -2410,21 +2599,25 @@ def check_manifest_kinds(ctx: Context) -> Row: and `unreadable_next` picks the command. That sentence stays the unregistered kind's alone, and where a row has more than one fault -- an unregistered kind, a kind registered to a gate this root never runs, a - `family.yaml` beside `project.yaml` -- it names the restore alone, first, - because the row cannot say what kind a file declares until it loads, and - because a command and a sentence joined by `;` would be a second command - in a pasted line (#277). The reason still names every fault, and the - next run names the rest. `restore` and `refusals` join `detail` only - when a file does not load, so every other root reports the row it - always did. + `family.yaml` beside `project.yaml`, a kind at a path no gate reads -- + it names the restore alone, first, because the row cannot say what kind + a file declares until it loads, and because a command and a sentence + joined by `;` would be a second command in a pasted line (#277). The + reason still names every fault, and the next run names the rest. + `restore` and `refusals` join `detail` only when a file does not load, + so every other root reports the row it always did. """ known: list[str] = [] - unknown: list[str] = [] + #: `(path, kind)` for each manifest of a kind the registry does not know. + unknown: list[tuple] = [] unreadable: list[str] = [] refusals: dict[str, dict] = {} #: `(path, kind, entry)` for each manifest whose registered gate never #: runs on this kind of root (#327). ungated: list[tuple] = [] + #: `(path, kind, paths)` for each manifest whose gate runs here and + #: never reads its path, with the paths it does read that kind at (#439). + unread: list[tuple] = [] elsewhere = (ctx.manifest_path if ctx.manifest_refusal is not None else None) beside: tuple[str, dict] | None = None @@ -2457,19 +2650,36 @@ def check_manifest_kinds(ctx: Context) -> Row: continue entry = MANIFEST_VALIDATORS.get(kind) if entry is None: - unknown.append(f"{rel} ({kind})") - elif ctx.kind not in entry.runs_in(): + unknown.append((rel, kind)) + continue + if ctx.kind not in entry.runs_in(): ungated.append((rel, kind, entry)) - else: + continue + paths = entry.reads(ctx) + if paths is None or read_here(ctx, path, rel, paths): known.append(f"{rel} ({kind})") - detail = {"validated": known, "unregistered": unknown, + else: + unread.append((rel, kind, sorted(paths))) + detail = {"validated": known, + "unregistered": [f"{rel} ({kind})" for rel, kind in unknown], "unreadable": unreadable, "registered_kinds": sorted(MANIFEST_VALIDATORS)} - ungated_said = registered_elsewhere(ctx, ungated) if ungated else None + #: Every file the row's NEXT takes out of the root, in the row's order, + #: and how git has each, asked once (#548). + names = ([rel for rel, _ in unknown] + [rel for rel, _, _ in ungated] + + [rel for rel, _, _ in unread] + + ([FAMILY_MANIFEST] if beside is not None else [])) + states = git_states(ctx.root, names) if names else {} + ungated_said = (registered_elsewhere(ctx, ungated, states) if ungated + else None) if ungated_said is not None: # ONLY WHEN THERE IS ONE, like `family_beside` below, so every root # without such a file reports the row it reported before #327. detail["registered_elsewhere"] = ungated_said[2] + unread_said = read_nowhere(ctx, unread, states) if unread else None + if unread_said is not None: + # Likewise, so every root without one reports the row of #327. + detail["unread"] = unread_said[2] # Only when there is one, so every other root's row is the row it was. aside = "" if elsewhere is not None: @@ -2481,31 +2691,33 @@ def check_manifest_kinds(ctx: Context) -> Row: # ONLY WHEN THERE IS ONE, so every root without it reports the same # row, byte for byte, as it did before #211. detail["family_beside"] = beside[1] - if (unknown or unreadable or ungated_said is not None - or beside is not None): + if names or unreadable: parts = [] - fixes = [] + #: The sentences of the `why` that name the other way out of each + #: fault, in the row's order: never the NEXT's (#548). + ways = [] if unknown: - parts.append("no validator registered for kind " - + ", ".join(unknown)) - fixes.append("register the kind in shape-doctor.py's " - "MANIFEST_VALIDATORS, beside the check that " - "validates it, or remove the file from the root") + parts.append("no validator registered for kind " + ", ".join( + detail["unregistered"])) + ways.append(register_way([rel for rel, _ in unknown])) if unreadable: said = {rel: ": ".join(filter(None, (why["code"], why["detail"]))) for rel, why in refusals.items()} parts.append("unreadable as YAML: " + ", ".join( f"{rel} ({said[rel]})" for rel in unreadable)) - if ungated_said is not None: - parts.append(ungated_said[0]) - fixes.append(ungated_said[1]) + for found in (ungated_said, unread_said): + if found is not None: + parts.append(found[0]) + ways += found[1] if beside is not None: parts.append(beside[0]) - fixes.append(FAMILY_BESIDE_FIX) - command, why = "; ".join(fixes), None + ways.append(FAMILY_BESIDE_WAY) if unreadable: restore, command, why = unreadable_next(ctx, unreadable, detail) detail.update({"restore": restore, "refusals": refusals}) + else: + command, taken = removal_next(ctx, names, states) + why = ". ".join([taken, *ways]) return Row("manifest-kinds", LABEL_MANIFEST_KINDS, FINDING, "; ".join(parts) + aside, command, detail, why=why) return Row("manifest-kinds", LABEL_MANIFEST_KINDS, OK, @@ -3346,10 +3558,11 @@ def linked_record_row(ctx: Context, us) -> Row | None: linked pin is answered as a link, whatever else reads it next. THE REFUSAL, QUOTED AS `update-shape.py` PRINTS IT, and its remedy for - the NEXT: replace the link with what it points at, commit that, run the - doctor again. Said, not spelled as a command: what the link points at - may be shared with other trees, and removing a link -- a junction above - all -- is a person's call. + the `why`: replace the link with what it points at, commit that, run + the doctor again. Said, not spelled as a command: what the link points + at may be shared with other trees, and removing a link -- a junction + above all -- is a person's call. So the row has no NEXT, which is one + pasteable command or nothing (#548); the sentence was the NEXT once. """ linked = us.linked_record(ctx.root, us.root_kind(ctx.root)) if linked is None: @@ -3358,13 +3571,13 @@ def linked_record_row(ctx: Context, us) -> Row | None: refusal = us.rewrite_link_refusal([linked]) return Row("shape-currency", LABEL_SHAPE_CURRENCY, FINDING, "update-shape.py cannot re-pin this root: " - f"REFUSED {refusal.code}: {refusal.detail}", - f"replace {name} with the file or directory it points at, " - "or with what it replaced where it points at nothing, commit " - "that, and run this doctor again -- update-shape.py never " - f"rewrites {record} through a link", + f"REFUSED {refusal.code}: {refusal.detail}", None, {"error": refusal.code, "path": record, "link": name, - "points": points}) + "points": points}, + why=f"replace {name} with the file or directory it points " + "at, or with what it replaced where it points at " + "nothing, commit that, and run this doctor again -- " + f"update-shape.py never rewrites {record} through a link") def head_of(path: Path) -> str | None: @@ -4540,10 +4753,12 @@ def placement_policy(ctx: Context, policy_path: Path) -> tuple: try: policy = adopt.PathPolicy.load(policy_path) except Refusal as exc: + # No NEXT: a remedy is prose, and goes in the `why` (#408, #548). return None, None, placement_row( NA, f"the standard's own path policy could not be read: {exc.detail}", - exc.remediation or None, {"policy": policy_path.as_posix()}) + None, {"policy": policy_path.as_posix()}, + why=remedy_as_why(exc)) return adopt, policy, None @@ -6487,9 +6702,11 @@ def run_checks(ctx: Context) -> list[Row]: if row is not None: rows.append(row) except Refusal as exc: + # No NEXT: a refusal's remedy is prose, never a command to paste, + # so it is the row's `why` (#408, #548). rows.append(Row(check.id, check.label, FINDING, - f"refused: {exc.detail}", exc.remediation or None, - {"error": exc.code})) + f"refused: {exc.detail}", None, + {"error": exc.code}, why=remedy_as_why(exc))) except OSError as exc: rows.append(Row(check.id, check.label, FINDING, f"could not be run: {exc}", None, {})) diff --git a/tests/test_doctor_next_is_a_command.py b/tests/test_doctor_next_is_a_command.py index 3a7f1b86..8871f4c1 100644 --- a/tests/test_doctor_next_is_a_command.py +++ b/tests/test_doctor_next_is_a_command.py @@ -75,6 +75,17 @@ project, recommit_a_good_pin, red_root, rows_of, scaffolded_here, stage, standard, standard_ahead, standard_of_another_history, ) +#: The `manifest kinds` row's roots and its guard (#548), from the module +#: that owns them: the row is that module's, and so is `rows_of`, which asks +#: the guard of every row it reads. +from test_shape_doctor import ( + READ_NOWHERE, assert_next_holds_no_prose, commit_all, family_beside, + plant_family_yaml, write_inventory, +) +#: The lockstep remedy `validate-pins.py` prints under its refusals, and the +#: one every `Refusal` carried before #370, from the module that spells it; +#: importing `tests/test_shape_doctor.py` put `scripts/` on the path. +from repo_shape import REMEDIATION #: Lane 2's ways to make a file under `contracts/` not load (#291), from the #: module that owns them: the `manifest kinds` row's NEXTs are #291's, so the #: roots are too. @@ -427,13 +438,68 @@ def _way_in(standard, project, holder, tmp_path): return rows_of(doctor(standard, here, "--json"))["way-in"] +def _kinds(standard, root): + return rows_of(doctor(standard, root, "--json"))["manifest-kinds"] + + +def _read_nowhere(project: Path, case: str) -> str: + """One of #439's files, written into `project` and not committed; its + path.""" + _where, rel, _kind, body, _read_at = READ_NOWHERE[case] + (project / rel).write_bytes(body) + return rel + + +def _removal_next_committed(standard, project, holder, tmp_path): + # #439: a kind whose gate runs here, at a path it never reads, committed: + # the `git rm` of it (#548). + rel = _read_nowhere(project, "contracts-project-yaml") + commit_all(project, f"{rel}, which no gate reads") + return _kinds(standard, project) + + +def _removal_next_loose(standard, project, holder, tmp_path): + # A kind nothing registers, never added: the `git clean` of it, where + # the NEXT was "register the kind ..., or remove the file" (#548). + write_inventory(project) + return _kinds(standard, project) + + +def _removal_next_changed(standard, project, holder, tmp_path): + # #327's file, staged and never committed: the `git status` that shows + # the change `git rm` will not discard. + plant_family_yaml(project, holder) + git("add", "--", "contracts/family.yaml", cwd=project) + return _kinds(standard, project) + + +def _family_beside(standard, project, holder, tmp_path): + # #211's file, whose NEXT held `family.py init` and `family.py add` in + # backticks: the `git rm` of it, and those two in the `why` (#548). + family_beside(project, holder) + return _kinds(standard, project) + + +def _every_fault(standard, project, holder, tmp_path): + # Every fault that is a file to take out, at once and in every git state: + # #211's committed, an unregistered kind and #327's untracked, #439's + # staged. ONE command, where the NEXT was their sentences joined by `; `. + family_beside(project, holder) + write_inventory(project) + plant_family_yaml(project, holder) + git("add", "--", _read_nowhere(project, "a-pin-nothing-declares"), + cwd=project) + return _kinds(standard, project) + + #: One fixture per NEXT #277 converted, by the builder and branch it reaches, #: and how the sentence that used to follow ` # ` on it now opens -- moved #: VERBATIM, less the `#`, so these are the words main, doctors A and B and -#: lane 2's #237, #291 and #496 printed. Not the prose NEXTs -#: `check_manifest_kinds` prints for a kind no gate here runs (#211, #327), -#: and not a `Refusal.remediation` printed as one; its NEXT for a file that -#: does not load (#291's `unreadable_next`) is here. +#: lane 2's #237, #291 and #496 printed. The `manifest kinds` NEXTs #277 +#: left as prose (#211, #327) are here too since #548, by the git state the +#: one command they print now reaches (`removal_next`), with the sentence +#: that now opens their `why`; a `Refusal.remediation` reaches no NEXT +#: (`test_a_refusal_a_check_lets_out_has_no_next_and_keeps_its_remedy`). CASES = { "validator_row-neither-readable": ( _validator_neither_readable, @@ -576,6 +642,32 @@ def _way_in(standard, project, holder, tmp_path): "typed yes"), "check_the_way_in": ( _way_in, "without --yes; it asks"), + "removal_next-committed": ( + _removal_next_committed, + "takes contracts/project.yaml out of this root: commit that on a " + "branch and land it as a pull request. Never rename it onto a path a " + "gate here reads"), + "removal_next-loose": ( + _removal_next_loose, + "deletes contracts/inventory.yaml, which git does not track, so there " + "is nothing to commit. Or register the kind"), + "removal_next-changed": ( + _removal_next_changed, + "shows the changes no commit holds in contracts/family.yaml, which " + "`git rm` will not discard: commit or drop them, and the next run " + "names the way out. Or move what contracts/family.yaml describes"), + "check_manifest_kinds-family-beside": ( + _family_beside, + "takes family.yaml out of this root: commit that on a branch and land " + "it as a pull request. Or move the membership family.yaml records to " + "a holder of its own (`family.py init`, then `family.py add` there)"), + "check_manifest_kinds-every-fault": ( + _every_fault, + "takes family.yaml out of this root: commit that on a branch and land " + "it as a pull request. The next run names the way out for " + "contracts/inventory.yaml, contracts/family.yaml, " + "contracts/release-pin.yaml. Or register the kind " + "contracts/inventory.yaml declares"), } @@ -842,6 +934,48 @@ def test_the_legs_next_pasted_into_an_interactive_zsh_runs_no_make( "here, or this test is proving nothing about the backticks") +#: The `manifest kinds` NEXT for a `family.yaml` beside `project.yaml` before +#: #548 (`FAMILY_BESIDE_FIX` at 5ed3e5de), spelled here because nothing +#: prints it any more. +FAMILY_BESIDE_NEXT_BEFORE_548 = ( + "an assembly root is not a family holder: take family.yaml out of this " + "root, or move the membership to a holder of its own (`family.py init`, " + "then `family.py add` there), as the human decides -- never `family.py " + "add` here, which would mount the members inside a project") + + +@WINDOWS_SKIP +@pytest.mark.skipif(shutil.which("zsh") is None, reason="needs zsh") +def test_the_family_beside_next_pasted_into_zsh_runs_no_family_py( + standard, project, holder, tmp_path, monkeypatch): + """#548's own defect, pasted where a `family.py` first on `PATH` leaves a + marker. The NEXT now is the `git rm` that takes `family.yaml` out, and + leaves none; the line the doctor printed before #548, pasted the same + way, runs `family.py init` and `family.py add` out of its backticks and + leaves it, so this harness is shown to run a backtick.""" + family_beside(project, holder) + row = rows_of(doctor(standard, project, "--json"))["manifest-kinds"] + assert row["status"] == "FINDING", row + stub = tmp_path / "stub" + stub.mkdir() + marker = tmp_path / "MARKER" + (stub / "family.py").write_text( + f"#!/bin/sh\ntouch {shlex.quote(str(marker))}\n", encoding="utf-8") + (stub / "family.py").chmod(0o755) + monkeypatch.setenv("PATH", f"{stub}{os.pathsep}{os.environ['PATH']}") + + status, transcript = paste_into_zsh(row["next"], project) + assert status == 0, transcript + assert not marker.exists(), transcript + assert git("ls-files", "--", "family.yaml", cwd=project).stdout == "", ( + "the NEXT did not take family.yaml out", transcript) + + paste_into_zsh(FAMILY_BESIDE_NEXT_BEFORE_548, project) + assert marker.exists(), ( + "the line the doctor printed before #548 must run `family.py` here, " + "or this test is proving nothing about the backticks") + + # --- the report says the sentence, off the command's line ------------------- def test_the_table_prints_why_on_its_own_line_under_next(standard, project, @@ -897,6 +1031,67 @@ def test_a_cannot_answer_row_with_no_command_still_says_why( assert said[at + 1:] == [" Why: what to do instead"], said +# --- a refusal's remedy reaches no NEXT, and the guard over the rows -------- +# +# #408: a check that let a `Refusal` out became a FINDING whose NEXT was the +# refusal's remedy, and a standard whose own path policy did not load an +# `n/a` row whose NEXT was the same: prose a tool prints under its refusal, +# `repo_shape.REMEDIATION` holding `git submodule update` and +# `validate-pins.py` in backticks. #494 left both with no known reach; #548 +# makes it none. + + +@pytest.mark.parametrize("remedy", ["none", "lockstep"]) +def test_a_refusal_a_check_lets_out_has_no_next_and_keeps_its_remedy( + standard, project, remedy): + """Both rows a refusal makes -- `run_checks`' FINDING for a check that + lets one out, `placement_policy`'s `n/a` for a path policy that does not + load -- have no NEXT, whatever the refusal carries. A remedy, where + there is one, is the row's `why`, less its `Remediation:` label, so what + it said is still printed, on a `WHY` line.""" + module = doctor_module(standard) + carried = REMEDIATION if remedy == "lockstep" else "" + + def refused(_ctx): + raise module.Refusal("probe-refused", "a check let this out", carried) + + class Unloadable: + @staticmethod + def load(path): + raise module.Refusal("policy-refused", f"{path}: it does not load", + carried) + + module.CHECKS = (module.Check("probe", "probe", (module.PROJECT,), + refused),) + ctx = module.Context(project.resolve()) + [escaped] = module.run_checks(ctx) + ctx.adopt().PathPolicy = Unloadable + _adopt, _policy, unread = module.placement_policy( + ctx, standard / "contracts" / "path-classification.yaml") + expected = (REMEDIATION.removeprefix("Remediation: ") + if remedy == "lockstep" else None) + for row in (escaped.as_dict(), unread.as_dict()): + assert row["next"] is None, row + assert row["why"] == expected, row + assert_next_holds_no_prose(row) + assert escaped.status == module.FINDING, escaped.as_dict() + + +def test_the_rows_guard_finds_a_backtick_or_a_remediation_in_a_next(): + """`assert_next_holds_no_prose`, which `rows_of` asks of every row the + suite reads, shown to fail on each NEXT #548 and #408 were filed about + and on prose that opens with `Remediation:`, and to pass a row with no + NEXT and a command whose quoted path holds a backtick, which is a + character of the path.""" + for line in (FAMILY_BESIDE_NEXT_BEFORE_548, REMEDIATION, + "Remediation: run the gate again"): + with pytest.raises(AssertionError): + assert_next_holds_no_prose({"id": "before", "next": line}) + for line in (None, "git -C '/w/it`s here' status", + "git -C 'C:\\it''s `here`' status"): + assert_next_holds_no_prose({"id": "after", "next": line}) + + # --- the guard: no tail comes back ------------------------------------------- #: What the guard calls a tail: two or more blanks, or a tab, then `#` and a diff --git a/tests/test_shape_doctor.py b/tests/test_shape_doctor.py index 7a420fdf..78052a0e 100644 --- a/tests/test_shape_doctor.py +++ b/tests/test_shape_doctor.py @@ -174,11 +174,45 @@ def doctor(standard: Path, root, *args) -> subprocess.CompletedProcess: def rows_of(result) -> dict: """`{id: row}` from a `--json` run, so a test names a row rather than a - line number of a table that is free to be re-laid-out.""" + line number of a table that is free to be re-laid-out. + + AND EVERY ROW IT HANDS BACK IS HELD TO `assert_next_holds_no_prose` + (#548), so each row any test here reads is a case of that guard.""" payload = json.loads(result.stdout) + for row in payload["rows"]: + assert_next_holds_no_prose(row) return {row["id"]: row for row in payload["rows"]} +#: A single-quoted span, as `quote_arg` spells a value in either shell: what +#: is inside it is the value's own characters, never shell syntax. +QUOTED_SPAN = re.compile(r"'[^']*'") + + +def assert_next_holds_no_prose(row: dict) -> None: + """A row's NEXT is one command, or nothing (#548): no backtick outside a + quoted value, and no `Remediation:`. + + A backtick runs the command it opens where the line is pasted into + zsh, interactive or `-c`: the `manifest kinds` NEXT for a `family.yaml` + beside `project.yaml` ran `family.py init` and `family.py add` there. + `Remediation:` opens the prose a tool prints under its own refusal, and + a check that let a refusal out printed it as the NEXT (#408). + + OVER THE ROWS, NOT THE SOURCE, which is #408's point: a remedy arrives at + run time, out of another module, and no scan of `shape-doctor.py`'s + strings sees it. `rows_of` asks this of every row a `--json` run + returns, so every row the suite builds, from whichever builder, is a + case; `tests/test_doctor_next_is_a_command.py` shows it catching each + form and builds the rows a refusal makes. + """ + command = row.get("next") + if not command: + return + assert "`" not in QUOTED_SPAN.sub("", command), (row["id"], command) + assert "Remediation:" not in command, (row["id"], command) + + def verdict_line(result) -> str: lines = [line for line in result.stdout.splitlines() if line.strip()] return lines[-1] @@ -549,24 +583,45 @@ def test_a_family_holder_gets_the_family_rows(standard, holder): "registered to, never runs on an assembly root -- a family holder is not " "a project -- so nothing checks the 1 member(s) it pins") -#: The row's next command. NOT `family.py add`, which is what -#: `validate-family.py`'s own refusal offers: run in an assembly root it would -#: mount the members inside a project. -FAMILY_BESIDE_NEXT = ( - "an assembly root is not a family holder: take family.yaml out of this " - "root, or move the membership to a holder of its own (`family.py init`, " - "then `family.py add` there), as the human decides -- never `family.py " - "add` here, which would mount the members inside a project") +#: The row's other way out, the last sentence of its `why` (#548): it was the +#: NEXT, and its backticks ran `family.py init` and `family.py add` where the +#: line was pasted into zsh. NOT `family.py add` here, which is what +#: `validate-family.py`'s own refusal offers: run in an assembly root it +#: would mount the members inside a project. +FAMILY_BESIDE_WAY = ( + "Or move the membership family.yaml records to a holder of its own " + "(`family.py init`, then `family.py add` there), as the human decides -- " + "never `family.py add` here, which would mount the members inside a " + "project") + +#: What the `why` says first where the NEXT is a `git rm` of `names`. +TAKES_OUT = ("takes {names} out of this root: commit that on a branch and " + "land it as a pull request") #: The `manifest kinds` row's `--json` keys before #211, which every root #: without a family.yaml beside its project.yaml must still get exactly. MANIFEST_KINDS_DETAIL_KEYS = {"validated", "unregistered", "unreadable", "registered_kinds"} -#: The next command an unregistered kind has always carried, byte for byte. -UNREGISTERED_NEXT = ("register the kind in shape-doctor.py's " - "MANIFEST_VALIDATORS, beside the check that validates " - "it, or remove the file from the root") +#: The other way out for the suite's `contracts/inventory.yaml`, which was +#: the NEXT an unregistered kind carried, less the removal (#548). +UNREGISTERED_WAY = ("Or register the kind contracts/inventory.yaml declares " + "in shape-doctor.py's MANIFEST_VALIDATORS, beside the " + "check that validates it, as the human decides") + +#: What the `why` says first where the NEXT deletes the suite's untracked +#: `contracts/inventory.yaml`. +INVENTORY_DELETED = ("deletes contracts/inventory.yaml, which git does not " + "track, so there is nothing to commit") + + +def removal_command(payload: dict, verb: str, names: list) -> str: + """The NEXT that takes `names` out of the root a `--json` run reports + on: `git -C -- `, every value spelled by the + doctor's own `quote_arg`, as the reader's shell hands it back (#548).""" + module = doctor_module(REPO) + return (f"git -C {module.quote_arg(Path(payload['root']))} {verb} -- " + + " ".join(module.quote_arg(name) for name in names)) def family_beside(project: Path, holder: dict) -> str: @@ -594,12 +649,17 @@ def record_gitlink(root: Path, path: str, commit: str) -> None: def beside_row(result) -> dict: """The `manifest kinds` row of a `--json` run, asserted to be the #211 - finding and to be the only row that is.""" + finding and to be the only row that is. Its NEXT takes the committed + `family.yaml` out of the root, and its `why` says so and names the + other way out (#548).""" rows = rows_of(result) row = rows["manifest-kinds"] assert row["status"] == "FINDING", row assert row["reason"].startswith(FAMILY_BESIDE_OPENING), row["reason"] - assert row["next"] == FAMILY_BESIDE_NEXT, row["next"] + assert row["next"] == removal_command(json.loads(result.stdout), "rm", + ["family.yaml"]), row["next"] + assert row["why"] == (TAKES_OUT.format(names="family.yaml") + ". " + + FAMILY_BESIDE_WAY), row["why"] assert set(row["detail"]) == MANIFEST_KINDS_DETAIL_KEYS | { "family_beside"}, row["detail"] validated = row["detail"]["validated"] @@ -757,17 +817,27 @@ def test_the_text_report_and_json_carry_the_same_family_beside_row( assert lines[at].endswith(" " + row["reason"]), lines[at] assert lines[at + 1].split()[0] == "NEXT", lines[at + 1] assert lines[at + 1].endswith(" " + row["next"]), lines[at + 1] + assert lines[at + 2].split()[0] == "WHY", lines[at + 2] + assert lines[at + 2].endswith(" " + row["why"]), lines[at + 2] assert verdict_line(text) == f"{payload['verdict']} (exit 1)" +def write_inventory(root: Path) -> None: + """The suite's manifest of a kind nothing registers, uncommitted.""" + (root / "contracts" / "inventory.yaml").write_bytes( + b"schema_version: 1\nkind: service-inventory\nservices: []\n") + + def test_a_family_yaml_beside_an_unregistered_kind_says_both( standard, project, holder): - """Two reasons in one row, each with its own exit, in the order the row - has always listed them: the unregistered kind first.""" + """Two reasons in one row, in the order the row has always listed them: + the unregistered kind first. ONE command, not the two exits joined by + `; ` (#548): the `git rm` of the committed `family.yaml`, and the `why` + names the file that comes out another way and each fault's other way + out. Run as printed, the next run names the untracked file's command, + and run as printed that is COMPLIANT.""" family_beside(project, holder) - (project / "contracts" / "inventory.yaml").write_text( - "schema_version: 1\nkind: service-inventory\nservices: []\n", - encoding="utf-8") + write_inventory(project) result = doctor(standard, project, "--json") assert result.returncode == 1, result.stdout + result.stderr row = rows_of(result)["manifest-kinds"] @@ -777,11 +847,27 @@ def test_a_family_yaml_beside_an_unregistered_kind_says_both( f"(service-inventory); {FAMILY_BESIDE_OPENING}: " f"members/{FAMILY_MEMBER} has no gitlink (mode 160000)"), \ row["reason"] - assert row["next"] == f"{UNREGISTERED_NEXT}; {FAMILY_BESIDE_NEXT}", \ + payload = json.loads(result.stdout) + assert row["next"] == removal_command(payload, "rm", ["family.yaml"]), \ row["next"] + assert row["why"] == ( + TAKES_OUT.format(names="family.yaml") + ". The next run names the " + f"way out for contracts/inventory.yaml. {UNREGISTERED_WAY}. " + f"{FAMILY_BESIDE_WAY}"), row["why"] assert row["detail"]["unregistered"] == [ "contracts/inventory.yaml (service-inventory)"] + run_pasted(row["next"]) + # What `git rm` staged and nothing else: the untracked file stays so. + commit_in(project, "Take the family.yaml beside project.yaml out") + again = doctor(standard, project, "--json") + row = only_finding_is_manifest_kinds(again) + assert row["next"] == removal_command(json.loads(again.stdout), "clean -f", + ["contracts/inventory.yaml"]), row + assert row["why"] == f"{INVENTORY_DELETED}. {UNREGISTERED_WAY}", row + run_pasted(row["next"]) + compliant_again(standard, project) + def test_a_root_with_no_family_yaml_beside_gets_the_row_it_always_got( standard, project, holder): @@ -790,6 +876,8 @@ def test_a_root_with_no_family_yaml_beside_gets_the_row_it_always_got( family holder -- whose own `family.yaml` is the manifest its `family` row validates, and stays `validated` here. The row's detail carries exactly the keys it carried, and the sentences are the ones it printed. + The unregistered kind's NEXT alone is new (#548): a command, where it + was the sentence its `why` now ends with. """ row = rows_of(doctor(standard, project, "--json"))["manifest-kinds"] assert row["status"] == "ok", row @@ -800,15 +888,17 @@ def test_a_root_with_no_family_yaml_beside_gets_the_row_it_always_got( assert row["next"] is None assert "family" not in json.dumps(row["detail"]["validated"]), row - (project / "contracts" / "inventory.yaml").write_text( - "schema_version: 1\nkind: service-inventory\nservices: []\n", - encoding="utf-8") - row = rows_of(doctor(standard, project, "--json"))["manifest-kinds"] + write_inventory(project) + result = doctor(standard, project, "--json") + row = rows_of(result)["manifest-kinds"] assert row["status"] == "FINDING", row assert set(row["detail"]) == MANIFEST_KINDS_DETAIL_KEYS, row["detail"] assert row["reason"] == ("no validator registered for kind " "contracts/inventory.yaml (service-inventory)") - assert row["next"] == UNREGISTERED_NEXT, row["next"] + assert row["next"] == removal_command(json.loads(result.stdout), + "clean -f", + ["contracts/inventory.yaml"]), row + assert row["why"] == f"{INVENTORY_DELETED}. {UNREGISTERED_WAY}", row result = doctor(standard, holder["root"], "--json") assert result.returncode == 0, result.stdout + result.stderr @@ -826,14 +916,14 @@ def test_a_root_with_no_family_yaml_beside_gets_the_row_it_always_got( # `contracts/project.yaml` in a family holder were `ok` and COMPLIANT, exit # 0, though the gate registered for each kind never runs on that kind of root. -#: The words the row's next command closes with, spelt out here for the -#: reason `FAMILY_BESIDE_NEXT` is: a test that read them back from the doctor -#: would pass whatever it said. +#: The words the row's `why` closes with, spelt out here for the reason +#: `FAMILY_BESIDE_WAY` is: a test that read them back from the doctor would +#: pass whatever it said. NEVER_RENAME = ("never rename it to project.yaml or family.yaml in this " "root: a family holder is not a project, and an assembly " "root is not a family holder") -#: Where each kind's manifest goes instead, as the next command offers it. +#: Where each kind's manifest goes instead, as the `why` offers it. FAMILY_HOME = ("a family holder of its own (`family.py init`, then " "`family.py add` there)") #: `setup-project.py` beside `setup.sh`, on every OS: `setup.sh` is a bash @@ -844,30 +934,25 @@ def test_a_root_with_no_family_yaml_beside_gets_the_row_it_always_got( "which a holder pins with `family.py add`") -def removal_named(standard: Path, row: dict, payload: dict, - names: list) -> list: - """The `git rm` a registered-elsewhere row's NEXT names, as the argv it - is, after asserting the NEXT spells exactly that command. +def removal_named(row: dict, payload: dict, names: list) -> list: + """The `git rm` a `manifest kinds` row's NEXT is, as the argv it is, + after asserting the NEXT is exactly that command and nothing else (#548). The spelling is held to the doctor's own `quote_arg` -- the quoter whose two dialects the tests further down pin -- so the line is the one the reader's shell hands back whole, on Windows as on POSIX. Where the reader's shell IS POSIX `sh`, `shlex.split` reads the line back too. """ - module = doctor_module(standard) - root = Path(payload["root"]) - spelled = (f"git -C {module.quote_arg(root)} rm -- " - + " ".join(module.quote_arg(name) for name in names)) - assert f"(`{spelled}`, committed on a branch and landed as a pull " \ - "request)" in row["next"], row["next"] - argv = ["git", "-C", str(root), "rm", "--", *names] - if os.name != "nt": - assert shlex.split(spelled) == argv, spelled + assert row["next"] == removal_command(payload, "rm", names), row["next"] + # Through `Path`: `--json` spells the root `as_posix()`, and the NEXT + # spells it as the platform does. + argv = ["git", "-C", str(Path(payload["root"])), "rm", "--", *names] + assert pasted(row["next"]) == argv, row["next"] return argv def run_the_removal(argv: list, root: Path) -> None: - """Run the NEXT's own `git rm`, and commit it as the line says.""" + """Run the NEXT's own `git rm`, and commit it as its `why` says.""" proc = subprocess.run(argv, capture_output=True, text=True, check=False) assert proc.returncode == 0, proc.stdout + proc.stderr commit_all(root, "Take the manifest no gate here reads out of the root") @@ -892,6 +977,8 @@ def test_a_contracts_family_yaml_in_an_assembly_root_is_a_finding( FINDING the same bytes are at the top of the root (#211) now: not `validated`, named with the validator and where it runs, and a next command that exists -- run here, it puts the root back to COMPLIANT. + The NEXT is that command alone, and the other way out is its `why`'s + (#548). """ shutil.copy2(holder["root"] / "family.yaml", project / "contracts" / "family.yaml") @@ -915,12 +1002,11 @@ def test_a_contracts_family_yaml_in_an_assembly_root_is_a_finding( {"manifest": "contracts/family.yaml", "kind": "family-manifest", "validator": "scripts/validate-family.py", "runs_in": ["family"], "git": "committed"}] - argv = removal_named(standard, row, payload, ["contracts/family.yaml"]) - assert row["next"].startswith( - "take contracts/family.yaml out of this root (`git -C "), row["next"] - assert row["next"].endswith( - f", or move what it describes to {FAMILY_HOME}, as the human " - f"decides -- {NEVER_RENAME}"), row["next"] + argv = removal_named(row, payload, ["contracts/family.yaml"]) + assert row["why"] == ( + TAKES_OUT.format(names="contracts/family.yaml") + ". Or move what " + f"contracts/family.yaml describes to {FAMILY_HOME}, as the human " + f"decides -- {NEVER_RENAME}"), row["why"] text = doctor(standard, project) assert text.returncode == 1, text.stdout + text.stderr @@ -931,6 +1017,7 @@ def test_a_contracts_family_yaml_in_an_assembly_root_is_a_finding( lines[at] assert lines[at].endswith(" " + row["reason"]), lines[at] assert lines[at + 1].endswith(" " + row["next"]), lines[at + 1] + assert lines[at + 2].endswith(" " + row["why"]), lines[at + 2] assert verdict_line(text) == "INVALID (manifest-kinds) (exit 1)", \ text.stdout @@ -974,10 +1061,10 @@ def test_a_contracts_project_yaml_in_a_family_holder_is_a_finding( "contracts/repository-naming.yaml (repository-naming-policy)", "contracts/shape-pin.yaml (pinned_contract_manifest)", "family.yaml (family-manifest)"], row["detail"] - argv = removal_named(standard, row, payload, ["contracts/project.yaml"]) - assert row["next"].endswith( - f", or move what it describes to {PROJECT_HOME}, as the human " - f"decides -- {NEVER_RENAME}"), row["next"] + argv = removal_named(row, payload, ["contracts/project.yaml"]) + assert row["why"].endswith( + f". Or move what contracts/project.yaml describes to {PROJECT_HOME}, " + f"as the human decides -- {NEVER_RENAME}"), row["why"] run_the_removal(argv, root) after = doctor(standard, root, "--json") @@ -988,9 +1075,10 @@ def test_a_contracts_project_yaml_in_a_family_holder_is_a_finding( def test_a_contracts_family_yaml_and_one_beside_project_yaml_say_both( standard, project, holder): """Two family manifests in one assembly root, one under `contracts/` and - one at the top: two reasons in one row, each with its own exit, the - general one first and #211's -- which also checks the members -- after - it. Neither is counted `validated`.""" + one at the top: two reasons in one row, the general one first and + #211's -- which also checks the members -- after it, and ONE `git rm` + that takes both out, with each one's other way out in the `why` (#548). + Neither is counted `validated`.""" shutil.copy2(holder["root"] / "family.yaml", project / "contracts" / "family.yaml") family_beside(project, holder) @@ -1004,8 +1092,10 @@ def test_a_contracts_family_yaml_and_one_beside_project_yaml_say_both( f"runs only in a family holder); {FAMILY_BESIDE_OPENING}: " f"members/{FAMILY_MEMBER} has no gitlink (mode 160000)"), \ row["reason"] - assert row["next"].endswith(f"{NEVER_RENAME}; {FAMILY_BESIDE_NEXT}"), \ - row["next"] + removal_named(row, json.loads(result.stdout), + ["contracts/family.yaml", "family.yaml"]) + assert row["why"].endswith(f"{NEVER_RENAME}. {FAMILY_BESIDE_WAY}"), \ + row["why"] assert set(row["detail"]) == MANIFEST_KINDS_DETAIL_KEYS | { "registered_elsewhere", "family_beside"}, row["detail"] assert not any("family" in entry for entry in row["detail"]["validated"]) @@ -1016,7 +1106,8 @@ def test_a_contracts_family_yaml_and_one_beside_project_yaml_say_both( # The doctor reads the disk, and `git rm` takes out only a file a commit # holds whole. The NEXT named it for every file, and it said "did not match # any files" (exit 128) for one git does not track, ignored or not, and -# refused one with changes no commit holds. +# refused one with changes no commit holds. Each state has its own command +# now, and the NEXT is that command alone (#548). def plant_family_yaml(project: Path, holder: dict, name: str = "family.yaml") -> Path: @@ -1027,11 +1118,10 @@ def plant_family_yaml(project: Path, holder: dict, return planted -def next_for_one(ways: str) -> str: - """The NEXT for one `contracts/family.yaml`, `ways` its parenthesis.""" - return (f"take contracts/family.yaml out of this root ({ways}), or move " - f"what it describes to {FAMILY_HOME}, as the human decides -- " - f"{NEVER_RENAME}") +def why_for_one(said: str) -> str: + """The `why` for one `contracts/family.yaml`, opening with `said`.""" + return (f"{said}. Or move what contracts/family.yaml describes to " + f"{FAMILY_HOME}, as the human decides -- {NEVER_RENAME}") def git_rm_refuses(project: Path, planted: Path) -> None: @@ -1050,8 +1140,9 @@ def test_a_contracts_family_yaml_git_does_not_track_is_deleted( standard, project, holder, state): """A `contracts/family.yaml` nobody has committed -- copied in and not added, or hidden by the root's own excludes -- is the same FINDING, and - its NEXT is to delete it, with nothing to commit. It names no `git rm`, - which fails on it; deleting it is COMPLIANT again.""" + its NEXT is `git clean -f` of it, with nothing to commit, `-x` too where + the excludes hide it, which `clean` otherwise leaves. It names no `git + rm`, which fails on it; run as printed, the root is COMPLIANT again.""" planted = plant_family_yaml(project, holder) if state == "ignored": exclude = project / git("rev-parse", "--git-path", "info/exclude", @@ -1061,21 +1152,24 @@ def test_a_contracts_family_yaml_git_does_not_track_is_deleted( handle.write(b"\ncontracts/family.yaml\n") result = doctor(standard, project, "--json") assert result.returncode == 1, result.stdout + result.stderr - assert json.loads(result.stdout)["verdict"] == "INVALID (manifest-kinds)" + payload = json.loads(result.stdout) + assert payload["verdict"] == "INVALID (manifest-kinds)", payload row = only_finding_is_manifest_kinds(result) - assert row["next"] == next_for_one( - "delete it, which git does not track, so there is nothing to " - "commit"), row["next"] + verb = "clean -f -x" if state == "ignored" else "clean -f" + assert row["next"] == removal_command(payload, verb, + ["contracts/family.yaml"]), row + assert row["why"] == why_for_one( + "deletes contracts/family.yaml, which git does not track, so there " + "is nothing to commit"), row["why"] assert row["detail"]["registered_elsewhere"] == [ {"manifest": "contracts/family.yaml", "kind": "family-manifest", "validator": "scripts/validate-family.py", "runs_in": ["family"], "git": state}] git_rm_refuses(project, planted) - planted.unlink() - after = doctor(standard, project, "--json") - assert after.returncode == 0, after.stdout + after.stderr - assert json.loads(after.stdout)["verdict"] == "COMPLIANT" + run_pasted(row["next"]) + assert not planted.exists() + compliant_again(standard, project) @pytest.mark.parametrize("state", ["edited", "staged"]) @@ -1083,9 +1177,9 @@ def test_a_contracts_family_yaml_with_changes_no_commit_holds( standard, project, holder, state): """Committed and then edited, or staged and never committed: `git rm` refuses both, because it will not discard changes no commit holds. The - NEXT says to commit or drop them, names the `git status` that shows - them, and leaves the `git rm` to the next run -- which, once they are - committed, names it, and running that is COMPLIANT again.""" + NEXT is the `git status` that shows them, the `why` says to commit or + drop them, and the next run names the way out -- which, once they are + committed, is the `git rm`, and running that is COMPLIANT again.""" planted = plant_family_yaml(project, holder) if state == "edited": commit_all(project, "A family manifest under contracts/") @@ -1098,32 +1192,33 @@ def test_a_contracts_family_yaml_with_changes_no_commit_holds( payload = json.loads(result.stdout) assert payload["verdict"] == "INVALID (manifest-kinds)", payload row = only_finding_is_manifest_kinds(result) - root = doctor_module(standard).quote_arg(Path(payload["root"])) - assert row["next"] == next_for_one( - "it has changes no commit holds, which `git rm` will not discard: " - f"commit or drop them (`git -C {root} status -- " - "contracts/family.yaml` shows them), and the next run names the " - "`git rm`"), row["next"] + assert row["next"] == removal_command(payload, "status", + ["contracts/family.yaml"]), row + assert row["why"] == why_for_one( + "shows the changes no commit holds in contracts/family.yaml, which " + "`git rm` will not discard: commit or drop them, and the next run " + "names the way out"), row["why"] assert [entry["git"] for entry in row["detail"]["registered_elsewhere"]] == ["changed"], row git_rm_refuses(project, planted) + run_pasted(row["next"]) commit_all(project, "Commit what the row said to commit first") again = doctor(standard, project, "--json") row = only_finding_is_manifest_kinds(again) - argv = removal_named(standard, row, json.loads(again.stdout), + argv = removal_named(row, json.loads(again.stdout), ["contracts/family.yaml"]) run_the_removal(argv, project) - after = doctor(standard, project, "--json") - assert after.returncode == 0, after.stdout + after.stderr - assert json.loads(after.stdout)["verdict"] == "COMPLIANT" + compliant_again(standard, project) def test_manifests_that_come_out_differently_are_each_named( standard, project, holder): """Three family manifests under `contracts/` -- one committed, one git - does not track, one edited since its commit -- get one NEXT with a way - for each, each naming its own file, in the row's order. + does not track, one edited since its commit -- get ONE command, the + `git rm` of the committed one, and a `why` that names the other two for + the next run, in the row's order (#548). It used to be one sentence + with a way for each, three commands in backticks among them. AND THE DOCTOR ASKED GIT WITHOUT WRITING THE INDEX. The committed one is rewritten with its own bytes and a newer mtime, so its index entry is @@ -1150,20 +1245,15 @@ def test_manifests_that_come_out_differently_are_each_named( payload = json.loads(result.stdout) assert payload["verdict"] == "INVALID (manifest-kinds)", payload row = only_finding_is_manifest_kinds(result) - root = doctor_module(standard).quote_arg(Path(payload["root"])) - assert row["next"] == ( - "take contracts/family.yaml, contracts/old-family.yaml, " - "contracts/x-family.yaml out of this root " - f"(`git -C {root} rm -- contracts/family.yaml`, committed on a " - "branch and landed as a pull request; delete " - "contracts/old-family.yaml, which git does not track, so there is " - "nothing to commit; contracts/x-family.yaml has changes no commit " - "holds, which `git rm` will not discard: commit or drop them " - f"(`git -C {root} status -- contracts/x-family.yaml` shows them), " - "and the next run names the `git rm`), or move what they describe " - f"to {FAMILY_HOME}, as the human decides -- never rename one to " + removal_named(row, payload, ["contracts/family.yaml"]) + assert row["why"] == ( + TAKES_OUT.format(names="contracts/family.yaml") + ". The next run " + "names the way out for contracts/old-family.yaml, " + "contracts/x-family.yaml. Or move what contracts/family.yaml, " + "contracts/old-family.yaml, contracts/x-family.yaml describe to " + f"{FAMILY_HOME}, as the human decides -- never rename one to " "project.yaml or family.yaml in this root: a family holder is not a " - "project, and an assembly root is not a family holder"), row["next"] + "project, and an assembly root is not a family holder"), row["why"] assert [(entry["manifest"], entry["git"]) for entry in row["detail"]["registered_elsewhere"]] == [ ("contracts/family.yaml", "committed"), @@ -1171,13 +1261,38 @@ def test_manifests_that_come_out_differently_are_each_named( ("contracts/x-family.yaml", "changed")], row["detail"] +def test_a_name_git_reads_as_a_pattern_is_passed_literally(project): + """`git rm -- 'contracts/shape-pin[.]yaml'` takes out every tracked path + the pattern matches, the shape pin with it. So a name holding a pattern + character is passed after `--literal-pathspecs`, and the command, run + as printed, takes out that file alone; a name with none is spelled as + it always was.""" + module = doctor_module(REPO) + ctx = SimpleNamespace(root=project) + odd = "contracts/shape-pin[.]yaml" + (project / odd).write_bytes(b"kind: service-inventory\n") + commit_all(project, "a manifest named like a pattern") + command, said = module.removal_next(ctx, [odd], {odd: "committed"}) + assert pasted(command) == ["git", "-C", str(project), + "--literal-pathspecs", "rm", "--", odd], \ + command + assert said == TAKES_OUT.format(names=odd), said + run_pasted(command) + assert not (project / odd).exists() + assert (project / SHAPE_PIN).is_file(), "the shape pin went with it" + plain, _ = module.removal_next(ctx, ["contracts/x.yaml"], + {"contracts/x.yaml": "committed"}) + assert "--literal-pathspecs" not in plain, plain + + def test_git_states_answers_only_in_a_repository_of_its_own(tmp_path): """`git -C` discovers. In a plain directory inside another repository, `status` answers about THAT repository, with paths relative to its top, so `contracts/family.yaml` was never listed, read as `committed`, and the NEXT named a `git rm` aimed at the enclosing repository. Now git is - not asked there, and the NEXT carries no way at all: #211's "take it out - of this root", which holds in any state. Its own repository is asked.""" + not asked there, and the row has no command at all: its `why` is #211's + "take it out of this root", which holds in any state. Its own + repository is asked.""" module = doctor_module(REPO) outer = tmp_path / "outer" outer.mkdir() @@ -1188,7 +1303,8 @@ def test_git_states_answers_only_in_a_repository_of_its_own(tmp_path): b"kind: family-manifest\n") names = ["contracts/family.yaml"] assert module.git_states(root, names) == {} - assert module.removal_ways(SimpleNamespace(root=root), names, {}) == "" + assert module.removal_next(SimpleNamespace(root=root), names, {}) == ( + None, "take contracts/family.yaml out of this root") git("init", "-q", "-b", "main", ".", cwd=root) assert module.git_states(root, names) == { @@ -1220,6 +1336,195 @@ def test_a_registered_kind_is_answered_only_where_its_check_runs(): assert set(entry.covered_by) <= checks, (kind, entry.covered_by) +# --- a registered kind at a path no gate reads (#439) ----------------------- +# +# #327 asked whether a kind's check RUNS on this kind of root, and not whether +# it READS the file's path, though every gate opens fixed paths. So each file +# below was `validated`, and its root COMPLIANT, exit 0, though nothing read +# it. Brett Heap ruled it on 2026-10-12: "#439 (registered kind at a path no +# check reads): FINDING". + +#: A pin no leg and no product names, as #439 measured one. +STRAY_PIN = b"kind: pinned_contract_manifest\ncommit: not-a-commit\n" + +#: #439's five files, each with the kind of root it is put in, its kind, its +#: bytes, and the paths a gate in that root reads the kind at. Spelt out here +#: for the reason `FAMILY_BESIDE_WAY` is. +READ_NOWHERE = { + "contracts-project-yaml": ( + "project", "contracts/project.yaml", "project-manifest", + b'kind: project-manifest\nid: "007"\n', ["project.yaml"]), + "a-pin-nothing-declares": ( + "project", "contracts/release-pin.yaml", "pinned_contract_manifest", + STRAY_PIN, ["contracts/code-pin.yaml", "contracts/shape-pin.yaml", + "contracts/spec-pin.yaml"]), + "an-old-naming-policy": ( + "project", "contracts/naming-old.yaml", "repository-naming-policy", + b"kind: repository-naming-policy\nforms: 7\n", + ["contracts/repository-naming.yaml"]), + "contracts-family-yaml-in-a-holder": ( + "family", "contracts/family.yaml", "family-manifest", + b"kind: family-manifest\nmembers:\n - project: Ghost\n", + ["family.yaml"]), + "a-leg-pin-in-a-holder": ( + "family", "contracts/spec-pin.yaml", "pinned_contract_manifest", + STRAY_PIN, ["contracts/shape-pin.yaml"]), +} + +#: The one other way out a pin in an assembly root has, and only it. +DECLARE_IT = ("the human names that product under `neutral_product_pins:` " + "in project.yaml -- and nobody declares a pin the human has " + "not asked for") + + +@pytest.mark.parametrize("case", list(READ_NOWHERE)) +def test_a_registered_kind_at_a_path_no_gate_reads_is_a_finding( + standard, project, holder, tmp_path, case): + """Each of #439's files, committed in a root that was COMPLIANT: a + FINDING naming the path, the kind and where a gate here reads that kind, + listed under `unread` and never `validated`, and INVALID, exit 1. + + THE NEXT TAKES THE FILE OUT AS GIT HAS IT (the issue's direction), one + command and nothing else (#548): run as printed, the root is COMPLIANT + again. The `why` never suggests renaming the file onto a path a gate + reads, and only the pin in an assembly root is offered the other way + out, the human declaring the product it pins -- which the row never does + itself. + """ + where, rel, kind, body, read_at = READ_NOWHERE[case] + root = project + if where == "family": + root = tmp_path / FAMILY_NAME + shutil.copytree(holder["root"], root, symlinks=True) + (root / rel).write_bytes(body) + commit_all(root, f"{rel}, which no gate reads") + result = doctor(standard, root, "--json") + assert result.returncode == 1, result.stdout + result.stderr + payload = json.loads(result.stdout) + assert (payload["kind"], payload["verdict"]) == ( + where, "INVALID (manifest-kinds)"), payload + row = only_finding_is_manifest_kinds(result) + assert row["reason"] == ( + f"no gate here reads {rel} ({kind}, which a gate here reads only at " + f"{', '.join(read_at)})"), row["reason"] + assert set(row["detail"]) == MANIFEST_KINDS_DETAIL_KEYS | {"unread"}, \ + row["detail"] + assert row["detail"]["unread"] == [ + {"manifest": rel, "kind": kind, "read_at": read_at, + "git": "committed"}], row["detail"] + assert f"{rel} ({kind})" not in row["detail"]["validated"], row + argv = removal_named(row, payload, [rel]) + assert row["why"].startswith(TAKES_OUT.format(names=rel) + ". "), row + assert row["why"].endswith( + ". Never rename it onto a path a gate here reads"), row["why"] + assert (DECLARE_IT in row["why"]) == ( + case == "a-pin-nothing-declares"), row["why"] + + run_the_removal(argv, root) + compliant_again(standard, root) + + +def test_each_kind_is_read_at_the_paths_its_gate_opens(standard, project, + holder): + """THE TABLE #439 READS, off the registry: where the check that runs on + each kind of root reads each kind. `validate-manifest.py` reads the + root's `project.yaml` and the naming policy, `validate-pins.py` the shape + pin and the two leg pins `project.yaml` declares, and `validate-family.py` + the holder's `family.yaml`, the shape pin and the naming policy. The + path policy is not decided by path, as before.""" + module = doctor_module(standard) + + def read_at(root: Path) -> dict: + ctx = module.Context(root.resolve()) + return {kind: entry.reads(ctx) + for kind, entry in module.MANIFEST_VALIDATORS.items()} + + both = {"project-manifest": {"project.yaml"}, + "family-manifest": {"family.yaml"}, + "repository-naming-policy": {"contracts/repository-naming.yaml"}, + "path-classification-policy": None} + assert read_at(project) == {**both, "pinned_contract_manifest": { + "contracts/code-pin.yaml", "contracts/shape-pin.yaml", + "contracts/spec-pin.yaml"}} + assert read_at(holder["root"]) == { + **both, "pinned_contract_manifest": {"contracts/shape-pin.yaml"}} + + +def test_a_pin_a_gate_reads_stays_validated_by_any_name_it_has(standard, + project): + """The other side of #439: what a gate here reads is `validated`, as it + was, and the row says nothing new about it. + + A FILE IS READ BY ANY NAME IT HAS. A hard link to a leg pin is that pin. + A leg pin spelled in another case is the pin `validate-pins.py` opens + where the filesystem folds case, and is read by nothing where it does + not: a NEXT taking it out on the first would take out a pin. + + A NEUTRAL PRODUCT PIN IS READ WHILE `project.yaml` DECLARES IT, and read + by nothing once the declaration is gone, when the human declaring it is + the other way out. And while `project.yaml` does not load it declares + nothing this run can read, so no pin is called read by nothing: the + `manifest` row names that file. + + IN PROCESS, through the row alone, as `declare_a_neutral_pin`'s other + caller is: a declared neutral pin makes the `pins` row recheck its + digest, which asks `gh`. + """ + module = doctor_module(standard) + + def kinds() -> dict: + row = module.check_manifest_kinds(module.Context(project)).as_dict() + assert_next_holds_no_prose(row) + return row + + pins = project / "contracts" + spec = pins / "spec-pin.yaml" + os.link(spec, pins / "linked-pin.yaml") + row = kinds() + assert row["status"] == "ok", row + assert "contracts/linked-pin.yaml (pinned_contract_manifest)" in \ + row["detail"]["validated"], row + (pins / "linked-pin.yaml").unlink() + + spec.rename(pins / "Spec-Pin.yaml") + folds = spec.exists() + row = kinds() + named = "contracts/Spec-Pin.yaml" + if folds: + assert f"{named} (pinned_contract_manifest)" in \ + row["detail"]["validated"], row + else: + assert [entry["manifest"] for entry in row["detail"]["unread"]] == [ + named], row + (pins / "Spec-Pin.yaml").rename(spec) + + declare_a_neutral_pin(project) + neutral = project / NEUTRAL_PIN + neutral.write_bytes(b"kind: pinned_contract_manifest\n" + + neutral.read_bytes()) + row = kinds() + assert row["status"] == "ok", row + assert f"{NEUTRAL_PIN} (pinned_contract_manifest)" in \ + row["detail"]["validated"], row + manifest = project / "project.yaml" + manifest.write_bytes(manifest.read_bytes().replace( + b"neutral_product_pins:\n - openGlass\n", + b"neutral_product_pins: []\n")) + row = kinds() + assert [entry["manifest"] for entry in row["detail"]["unread"]] == [ + NEUTRAL_PIN], row + assert DECLARE_IT in row["why"], row["why"] + + edit, code = BREAKAGES["tab-indent"] + break_manifest(manifest, edit, code) + row = kinds() + assert row["status"] == "ok", row + assert "unread" not in row["detail"], row + for pin in (NEUTRAL_PIN, SPEC_PIN, CODE_PIN): + assert f"{pin} (pinned_contract_manifest)" in \ + row["detail"]["validated"], (pin, row) + + def test_gitlinks_under_lists_one_directory_and_nothing_above_it(project): """`gitlinks_under` is asked for the members directory a hand-edited manifest names, so an empty one, an absolute one and one that climbs @@ -2607,8 +2912,9 @@ def assert_a_linked_record_is_their_finding(result, linked: str, re-pin through the link, and the doctor had filed that refusal with the ones about THIS checkout of the standard: `CANNOT ANSWER`, exit 3, with a `git fetch --all` that changes nothing. It is a finding now: - `INVALID (shape-currency)`, exit 1, the refusal quoted, and the NEXT is - the refusal's own remedy. + `INVALID (shape-currency)`, exit 1, the refusal quoted, and the `why` is + the refusal's own remedy. It has no NEXT: the remedy is a person's act, + said and not a command, and a NEXT is one command or nothing (#548). """ payload = json.loads(result.stdout) assert (payload["verdict"], payload["exit"]) == ( @@ -2624,10 +2930,10 @@ def assert_a_linked_record_is_their_finding(result, linked: str, assert "REFUSED update-rewrite-symlink: `apply` would rewrite " \ f"{record}, but {linked} in this root is a symlink" in row["reason"], \ row["reason"] - assert "fetch" not in row["next"], row["next"] - assert row["next"].startswith(f"replace {linked} with the file or " - "directory it points at"), row["next"] - assert "run this doctor again" in row["next"], row["next"] + assert row["next"] is None, row + assert row["why"].startswith(f"replace {linked} with the file or " + "directory it points at"), row["why"] + assert "run this doctor again" in row["why"], row["why"] # Every other row reads THROUGH the link and finds nothing wrong: the # verdict is this row's alone. red = [key for key, other in rows_of(result).items() @@ -5271,12 +5577,9 @@ def test_an_unregistered_kind_beside_a_file_that_does_not_load(standard, """Both in the reason, and the NEXT is the restore: the row cannot say what kind a file declares until it loads. Run, the row is the one the unregistered kind alone got, sentence, keys and all.""" - (project / "contracts" / "inventory.yaml").write_text( - "schema_version: 1\nkind: service-inventory\nservices: []\n", - encoding="utf-8") + write_inventory(project) alone = rows_of(doctor(standard, project, "--json"))["manifest-kinds"] - assert alone["next"].startswith( - "register the kind in shape-doctor.py's MANIFEST_VALIDATORS"), alone + assert alone["why"] == f"{INVENTORY_DELETED}. {UNREGISTERED_WAY}", alone assert set(alone["detail"]) == {"validated", "unregistered", "unreadable", "registered_kinds"}, alone["detail"] break_the_pin(project, conflicted_pin) @@ -5484,7 +5787,7 @@ def test_a_contracts_family_yaml_beside_a_file_that_does_not_load_names_the_rest assert row["reason"] == elsewhere, row["reason"] assert set(row["detail"]) == MANIFEST_KINDS_DETAIL_KEYS | { "registered_elsewhere"}, row["detail"] - run_the_removal(removal_named(standard, row, json.loads(after.stdout), + run_the_removal(removal_named(row, json.loads(after.stdout), ["contracts/family.yaml"]), project) final = doctor(standard, project, "--json") assert final.returncode == 0, final.stdout + final.stderr From abbce43570a2e2afd501deb2dc8fcf060bb44f6f Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Wed, 14 Oct 2026 18:32:12 +0000 Subject: [PATCH 2/3] shape-doctor.py: `manifest kinds` never calls a file a gate reads `unread` or takes it out, whatever the fault, so a misspelt leg role or a wrong `kind:` in the naming policy no longer gets a NEXT that removes the spec pin or the policy, and its NEXT shows a file git does not track instead of deleting it (#548, #439, review of #555) Fix round for the independent review of 557a2595. A file a gate reads is that gate's to judge. `read_by_a_gate` is every root path a gate that runs here reads, as any kind: each kind's `read_at` for the kinds whose check runs on this root, plus `own_pins`, the pins the shape itself writes (the shape pin, and in an assembly root `contracts/spec-pin.yaml` and `contracts/code-pin.yaml`, whatever the legs declare). `pins_read_here` counts `own_pins` too. On a scaffolded root with one committed edit, main says `manifest kinds ok` and 557a2595 said: - `- role: specs`: "no gate here reads contracts/spec-pin.yaml (... only at ... contracts/specs-pin.yaml)", NEXT `git rm -- contracts/spec-pin.yaml`, beside the `manifest` row's FINDING manifest-roles; - `contracts/repository-naming.yaml` declaring `kind: pinned_contract_manifest`: "no gate here reads contracts/repository-naming.yaml", NEXT `git rm` of it, beside the `manifest` row's REFUSED policy-wrong-kind. Both now say `ok manifest kinds 5 manifest(s)`, as main does. Beyond the review's two cases, `taking_out` keeps every such path out of the NEXT for any fault: an unregistered kind, or one whose check runs elsewhere, at a gated path is still this row's finding, as on main, but the NEXT never takes the file out, and the `why` says the gate's own row judges it. A file no commit holds is shown, never deleted. `git clean` cannot be undone, and AGENTS.md tells an agent to run the NEXT, so for an untracked or ignored file the NEXT is `git -C status --ignored -- `, and the `why` names the `git clean -f` (`-x` where ignored) for a person to run, as `pin_restore` shows a file a person may have edited. A committed file keeps `git rm`, which history can undo; a changed one keeps `git status`. Tests: `tests/test_update_shape_merged_away.py`, which skips without git-filter-repo, asserted the linked-record row's NEXT as prose; it now asserts no NEXT and the sentence in `why`. The guard reads a NEXT with `outside_single_quotes`, moved beside `rows_of`, which reads `quote_arg`'s `'\''` where the regular expression did not. The pathspec test goes through the row with `contracts/*-pin.yaml` (POSIX) and `contracts/[s]hape-pin.yaml`, and checks the `git rm` stages that file alone. New: both review cases through the doctor, and the gated-path NEXT. The #205 paragraph of `check_manifest_kinds` no longer describes main's NEXT. Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 140 ++++++++++++--- tests/test_doctor_next_is_a_command.py | 39 ++-- tests/test_shape_doctor.py | 240 ++++++++++++++++++++----- tests/test_update_shape_merged_away.py | 4 +- 4 files changed, 319 insertions(+), 104 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 54a653c8..91e0cca3 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -866,22 +866,61 @@ def reads(self, ctx: Context) -> set | None: NAMING_COPY = "contracts/repository-naming.yaml" +def own_pins(ctx: Context) -> set: + """The pins the shape itself writes in this kind of root, whatever its + manifest declares: the shape pin, and in an assembly root + `contracts/-pin.yaml` for each role the schema has (review of PR + #555). + + A leg's role is the schema's, and `validate-manifest.py` refuses any + other (`manifest-roles`). So a `- role: specs` typo leaves + `contracts/spec-pin.yaml` the spec leg's pin, which the gate reads again + once the typo is fixed, and not a file nothing reads: `declared_pins` + alone named `contracts/specs-pin.yaml`, and the row's NEXT took the + real pin out, over a manifest the `manifest` row already names. + """ + if ctx.kind == FAMILY: + return {SHAPE_PIN} + return {SHAPE_PIN} | {f"contracts/{role}-pin.yaml" for role in LEG_ROLES} + + def pins_read_here(ctx: Context) -> set | None: """Where this root's gate reads a `pinned_contract_manifest` (#439). In a holder, `validate-family.py` reads the shape pin and no other: a holder has no legs and declares no product. In an assembly root, `validate-pins.py` reads `declared_pins` -- the shape pin, a pin per leg - and a pin per `neutral_product_pins:` entry. Where `project.yaml` does - not load, it declares nothing this run can read, and None leaves every - pin counted as before: a leg pin called read by nothing would be a NEXT - that takes it out, over a manifest the `manifest` row already names. + and a pin per `neutral_product_pins:` entry -- and `own_pins` are + counted read whatever the legs declare. Where `project.yaml` does not + load, it declares nothing this run can read, and None leaves every pin + counted as before: a pin called read by nothing would be a NEXT that + takes it out, over a manifest the `manifest` row already names. """ if ctx.kind == FAMILY: - return {SHAPE_PIN} + return own_pins(ctx) if ctx.manifest_refusal is not None: return None - return declared_pins(ctx) + return own_pins(ctx) | declared_pins(ctx) + + +def read_by_a_gate(ctx: Context) -> set: + """Every root path a gate that runs here reads a file at, as any kind + (review of PR #555): each kind's `read_at` for the kinds whose check + runs on this root, and `own_pins`. + + A FILE AT ONE OF THESE PATHS IS THAT GATE'S TO JUDGE. A + `contracts/repository-naming.yaml` declaring `kind: + pinned_contract_manifest` is refused by `validate-manifest.py` + (`policy-wrong-kind`), and the `manifest` row names it. Called read by + nothing here, it got a NEXT that took out the file the gate reads. So + such a file is never `unread`, and the row's NEXT never takes out a + file at one of these paths, whatever this row finds in it. + """ + paths = own_pins(ctx) + for entry in MANIFEST_VALIDATORS.values(): + if ctx.kind in entry.runs_in(): + paths |= entry.reads(ctx) or set() + return paths MANIFEST_VALIDATORS: dict[str, ManifestKind] = { @@ -2483,7 +2522,7 @@ def git_states(root: Path, names: list) -> dict: def removal_next(ctx: Context, names: list, states: dict) -> tuple[str | None, str]: - """`(command, said)`: the ONE command that takes `names` out of the + """`(command, said)`: the ONE command for taking `names` out of the root as git has them, and the sentence that says what it does -- the `manifest kinds` row's NEXT and the start of its `why` (#327, #548). @@ -2501,9 +2540,17 @@ def removal_next(ctx: Context, names: list, (exit 128) for one git does not track, ignored or not, and refuses one with changes no commit holds. So `git_states` asks first, and: committed `git rm`, committed on a branch and landed as a pull - request, the way every change here lands; - untracked, `git clean -f`, `-x` too where one is ignored, which - ignored `clean` otherwise leaves: nothing to commit; + request, the way every change here lands, which the + history can undo; + untracked, `git status --ignored`, which shows them. NEVER the + ignored deletion as the NEXT (review of PR #555): no commit + holds such a file, so deleting it cannot be undone, and + AGENTS.md tells an agent to run the NEXT -- a person's + draft would be gone. The `why` names it, `git clean -f` + (`-x` too where one is ignored, which `clean` otherwise + leaves), for a person to run, as `pin_restore` shows a + file a person may have edited rather than putting it back + (#291, #358); changed `git status`, which shows the changes, since `git rm` will not discard them: commit or drop them first. Each path is quoted for the reader's shell like every path this report @@ -2529,9 +2576,13 @@ def removal_next(ctx: Context, names: list, "branch and land it as a pull request") elif state == "loose": ignored = any(states[name] == "ignored" for name in group) - command = f"{git_c} clean -f{' -x' if ignored else ''} -- {spelled}" - said = (f"deletes {listed(group)}, which git does not track, so " - "there is nothing to commit") + command = f"{git_c} status --ignored -- {spelled}" + them = "it" if len(group) == 1 else "them" + said = (f"shows {listed(group)}, which git does not track: no " + f"commit holds {them}, so deleting {them} cannot be undone " + f"and is a person's act, never this row's -- `{git_c} clean " + f"-f{' -x' if ignored else ''} -- {spelled}` deletes {them}, " + "and there is nothing to commit") else: command = f"{git_c} status -- {spelled}" said = (f"shows the changes no commit holds in {listed(group)}, " @@ -2544,6 +2595,34 @@ def removal_next(ctx: Context, names: list, return command, said +def taking_out(ctx: Context, names: list, states: dict, gated: set, + ways: list) -> tuple[str | None, str]: + """`(command, why)` for the `manifest kinds` row's files to take out, + `ways` being each fault's other way out, a sentence each (#548). + + A FILE A GATE HERE READS IS NEVER TAKEN OUT (review of PR #555). An + unregistered kind, or one whose check runs elsewhere, at a path in + `gated` is still this row's finding, as it always was, but the gate + that reads that path judges it in its own row, and a NEXT that took it + out would take out the shape pin, a leg pin or the naming policy. So + `removal_next` is asked only of the rest, and the `why` says which + files it leaves, and why. With none left, the row has no NEXT. + """ + judged = [name for name in names + if read_here(ctx, ctx.root / name, name, gated)] + rest = [name for name in names if name not in judged] + command, taken = (removal_next(ctx, rest, states) if rest + else (None, None)) + said = [taken] if taken else [] + if judged: + one = len(judged) == 1 + said.append(f"{listed(judged)} {'is' if one else 'are'} read by a " + "gate here, whose own row judges " + f"{'it' if one else 'them'}: this row never takes out a " + "file a gate reads") + return command, ". ".join([*said, *ways]) + + def check_manifest_kinds(ctx: Context) -> Row: """Every `kind:`-bearing manifest in this root, against the registry. @@ -2575,21 +2654,26 @@ def check_manifest_kinds(ctx: Context) -> Row: THE NEXT IS ONE COMMAND, OR NOTHING (#548). Each fault but a file that does not load is a file to take out of the root, so the NEXT is the one - command `removal_next` names for all of them, as git has them, and the - `why` says what it does and then, a sentence each, the other way out of - each fault, which is the human's to choose: register the kind, move - what the file describes to its `home`, declare the pin, move a - membership to a holder. Those sentences were the NEXT, joined by `; `, - and held `family.py init` and `family.py add` in backticks, which an - interactive zsh runs where the line is pasted (#277). + command `removal_next` names for them, as git has them -- a `git rm` of + what a commit holds, and for the rest a `git status` that shows them, + never a deletion no history can undo -- and the `why` says what it does + and then, a sentence each, the other way out of each fault, which is + the human's to choose: register the kind, move what the file describes + to its `home`, declare the pin, move a membership to a holder. Those + sentences were the NEXT, joined by `; `, and held `family.py init` and + `family.py add` in backticks, which an interactive zsh runs where the + line is pasted (#277). A path a gate here reads (`read_by_a_gate`) is + never `unread` and never taken out: what is wrong with a file there is + that gate's finding, and `taking_out` says so (review of PR #555). THE ROOT'S OWN MANIFEST, WHEN IT DOES NOT LOAD, IS NOT THIS ROW'S (#205). The `manifest` (or `family`) row quotes its refusal and names its gate, and the kind of the root was read off its filename for exactly that reason. Listed here as well, it would be the same fault twice, under a - next command -- register a kind, or remove the file from the root -- - that is the wrong remedy for a merge conflict in a project's own - manifest. So it is skipped, and the row says where it went. + second NEXT beside the gate that row names, and on main before #291 + that NEXT was "register a kind, or remove the file from the root", the + wrong remedy for a merge conflict in a project's own manifest. So it is + skipped, and the row says where it went. A FILE THAT DOES NOT LOAD IS NOT AN UNREGISTERED KIND (#291). It used to share that case's next command -- register the kind, or remove the file @@ -2621,6 +2705,9 @@ def check_manifest_kinds(ctx: Context) -> Row: elsewhere = (ctx.manifest_path if ctx.manifest_refusal is not None else None) beside: tuple[str, dict] | None = None + #: Every path a gate here reads, as any kind: never `unread`, and never + #: taken out by the NEXT (review of PR #555). + gated = read_by_a_gate(ctx) for path in manifest_candidates(ctx.root): if path == elsewhere: continue @@ -2655,8 +2742,10 @@ def check_manifest_kinds(ctx: Context) -> Row: if ctx.kind not in entry.runs_in(): ungated.append((rel, kind, entry)) continue + # At a path a gate here reads as ANY kind, a wrong kind is that + # gate's own finding, not a file nothing reads (review of PR #555). paths = entry.reads(ctx) - if paths is None or read_here(ctx, path, rel, paths): + if paths is None or read_here(ctx, path, rel, gated): known.append(f"{rel} ({kind})") else: unread.append((rel, kind, sorted(paths))) @@ -2716,8 +2805,7 @@ def check_manifest_kinds(ctx: Context) -> Row: restore, command, why = unreadable_next(ctx, unreadable, detail) detail.update({"restore": restore, "refusals": refusals}) else: - command, taken = removal_next(ctx, names, states) - why = ". ".join([taken, *ways]) + command, why = taking_out(ctx, names, states, gated, ways) return Row("manifest-kinds", LABEL_MANIFEST_KINDS, FINDING, "; ".join(parts) + aside, command, detail, why=why) return Row("manifest-kinds", LABEL_MANIFEST_KINDS, OK, diff --git a/tests/test_doctor_next_is_a_command.py b/tests/test_doctor_next_is_a_command.py index 8871f4c1..4bf79350 100644 --- a/tests/test_doctor_next_is_a_command.py +++ b/tests/test_doctor_next_is_a_command.py @@ -80,7 +80,7 @@ #: the guard of every row it reads. from test_shape_doctor import ( READ_NOWHERE, assert_next_holds_no_prose, commit_all, family_beside, - plant_family_yaml, write_inventory, + outside_single_quotes, plant_family_yaml, write_inventory, ) #: The lockstep remedy `validate-pins.py` prints under its refusals, and the #: one every `Refusal` carried before #370, from the module that spells it; @@ -459,8 +459,10 @@ def _removal_next_committed(standard, project, holder, tmp_path): def _removal_next_loose(standard, project, holder, tmp_path): - # A kind nothing registers, never added: the `git clean` of it, where - # the NEXT was "register the kind ..., or remove the file" (#548). + # A kind nothing registers, never added: the `git status --ignored` that + # shows it, where the NEXT was "register the kind ..., or remove the + # file" (#548), and never the deletion no history can undo (review of + # PR #555). write_inventory(project) return _kinds(standard, project) @@ -649,8 +651,9 @@ def _every_fault(standard, project, holder, tmp_path): "gate here reads"), "removal_next-loose": ( _removal_next_loose, - "deletes contracts/inventory.yaml, which git does not track, so there " - "is nothing to commit. Or register the kind"), + "shows contracts/inventory.yaml, which git does not track: no commit " + "holds it, so deleting it cannot be undone and is a person's act, " + "never this row's -- `git -C "), "removal_next-changed": ( _removal_next_changed, "shows the changes no commit holds in contracts/family.yaml, which " @@ -694,28 +697,6 @@ def unquoted_operators(command: str) -> list[str]: LIVE_OPERATORS = ";&|()" -def outside_single_quotes(command: str) -> str: - """`command` less what `sh` reads literally: single-quoted spans and the - character after a backslash. A `$(` left in it is run -- inside double - quotes as much as outside them -- which `shlex`'s tokens cannot show: - they read `"$(make)"` as the one word `$(make)`, exactly as they read - the quoted path `'$(make)'`, which runs nothing.""" - kept, single, double, at = [], False, False, 0 - while at < len(command): - char = command[at] - if single: - single = char != "'" - elif char == "\\": - at += 1 - elif char == "'" and not double: - single = True - else: - double = double != (char == '"') - kept.append(char) - at += 1 - return "".join(kept) - - def test_the_operator_readers_see_what_sh_runs_and_not_a_quoted_path(): """The property below is only as good as these two readers, so each is shown to find an operator `sh` acts on, and to pass one that sits in a @@ -1082,12 +1063,14 @@ def test_the_rows_guard_finds_a_backtick_or_a_remediation_in_a_next(): suite reads, shown to fail on each NEXT #548 and #408 were filed about and on prose that opens with `Remediation:`, and to pass a row with no NEXT and a command whose quoted path holds a backtick, which is a - character of the path.""" + character of the path -- in each spelling `quote_arg` has for an + apostrophe, `sh`'s `'\\''` and PowerShell's `''`.""" for line in (FAMILY_BESIDE_NEXT_BEFORE_548, REMEDIATION, "Remediation: run the gate again"): with pytest.raises(AssertionError): assert_next_holds_no_prose({"id": "before", "next": line}) for line in (None, "git -C '/w/it`s here' status", + "git -C '/w/it'\\''s `here`' status", "git -C 'C:\\it''s `here`' status"): assert_next_holds_no_prose({"id": "after", "next": line}) diff --git a/tests/test_shape_doctor.py b/tests/test_shape_doctor.py index 78052a0e..8bec8086 100644 --- a/tests/test_shape_doctor.py +++ b/tests/test_shape_doctor.py @@ -184,9 +184,31 @@ def rows_of(result) -> dict: return {row["id"]: row for row in payload["rows"]} -#: A single-quoted span, as `quote_arg` spells a value in either shell: what -#: is inside it is the value's own characters, never shell syntax. -QUOTED_SPAN = re.compile(r"'[^']*'") +def outside_single_quotes(command: str) -> str: + """`command` less what `sh` reads literally: single-quoted spans and the + character after a backslash. A `$(` left in it is run -- inside double + quotes as much as outside them -- which `shlex`'s tokens cannot show: + they read `"$(make)"` as the one word `$(make)`, exactly as they read + the quoted path `'$(make)'`, which runs nothing. + + HERE, beside `rows_of`, because the guard below reads a NEXT with it and + `tests/test_doctor_next_is_a_command.py`, which imports this module, + reads one with it too; `quote_arg`'s `'\\''` form is read as `sh` reads + it, where a regular expression for a quoted span is not.""" + kept, single, double, at = [], False, False, 0 + while at < len(command): + char = command[at] + if single: + single = char != "'" + elif char == "\\": + at += 1 + elif char == "'" and not double: + single = True + else: + double = double != (char == '"') + kept.append(char) + at += 1 + return "".join(kept) def assert_next_holds_no_prose(row: dict) -> None: @@ -209,7 +231,7 @@ def assert_next_holds_no_prose(row: dict) -> None: command = row.get("next") if not command: return - assert "`" not in QUOTED_SPAN.sub("", command), (row["id"], command) + assert "`" not in outside_single_quotes(command), (row["id"], command) assert "Remediation:" not in command, (row["id"], command) @@ -609,21 +631,37 @@ def test_a_family_holder_gets_the_family_rows(standard, holder): "in shape-doctor.py's MANIFEST_VALIDATORS, beside the " "check that validates it, as the human decides") -#: What the `why` says first where the NEXT deletes the suite's untracked -#: `contracts/inventory.yaml`. -INVENTORY_DELETED = ("deletes contracts/inventory.yaml, which git does not " - "track, so there is nothing to commit") - - def removal_command(payload: dict, verb: str, names: list) -> str: """The NEXT that takes `names` out of the root a `--json` run reports - on: `git -C -- `, every value spelled by the - doctor's own `quote_arg`, as the reader's shell hands it back (#548).""" + on, or shows them: `git -C -- `, every value + spelled by the doctor's own `quote_arg`, as the reader's shell hands it + back (#548).""" module = doctor_module(REPO) return (f"git -C {module.quote_arg(Path(payload['root']))} {verb} -- " + " ".join(module.quote_arg(name) for name in names)) +#: The suite's manifest of a kind nothing registers, as the row names it. +INVENTORY = ["contracts/inventory.yaml"] + +#: The NEXT for files git does not track: it shows them, and never deletes +#: them, which no history could undo (review of PR #555). +SHOW_UNTRACKED = "status --ignored" + + +def shown_untracked(payload: dict, names: list, ignored: bool = False) -> str: + """What the `why` says first where the NEXT shows `names`, which git does + not track: that deleting them cannot be undone, and the `git clean` + that does it, for a person to run (review of PR #555).""" + them = "it" if len(names) == 1 else "them" + clean = removal_command(payload, "clean -f -x" if ignored else "clean -f", + names) + return (f"shows {', '.join(names)}, which git does not track: no commit " + f"holds {them}, so deleting {them} cannot be undone and is a " + f"person's act, never this row's -- `{clean}` deletes {them}, " + "and there is nothing to commit") + + def family_beside(project: Path, holder: dict) -> str: """Commit the holder's `family.yaml` into an assembly root, beside its `project.yaml` -- the hand copy #211 was filed about -- and return the @@ -862,10 +900,15 @@ def test_a_family_yaml_beside_an_unregistered_kind_says_both( commit_in(project, "Take the family.yaml beside project.yaml out") again = doctor(standard, project, "--json") row = only_finding_is_manifest_kinds(again) - assert row["next"] == removal_command(json.loads(again.stdout), "clean -f", - ["contracts/inventory.yaml"]), row - assert row["why"] == f"{INVENTORY_DELETED}. {UNREGISTERED_WAY}", row + payload = json.loads(again.stdout) + assert row["next"] == removal_command(payload, SHOW_UNTRACKED, + INVENTORY), row + assert row["why"] == (shown_untracked(payload, INVENTORY) + ". " + + UNREGISTERED_WAY), row run_pasted(row["next"]) + assert (project / INVENTORY[0]).is_file() + # The deletion the `why` names, as a person would run it. + run_pasted(removal_command(payload, "clean -f", INVENTORY)) compliant_again(standard, project) @@ -895,10 +938,11 @@ def test_a_root_with_no_family_yaml_beside_gets_the_row_it_always_got( assert set(row["detail"]) == MANIFEST_KINDS_DETAIL_KEYS, row["detail"] assert row["reason"] == ("no validator registered for kind " "contracts/inventory.yaml (service-inventory)") - assert row["next"] == removal_command(json.loads(result.stdout), - "clean -f", - ["contracts/inventory.yaml"]), row - assert row["why"] == f"{INVENTORY_DELETED}. {UNREGISTERED_WAY}", row + payload = json.loads(result.stdout) + assert row["next"] == removal_command(payload, SHOW_UNTRACKED, + INVENTORY), row + assert row["why"] == (shown_untracked(payload, INVENTORY) + ". " + + UNREGISTERED_WAY), row result = doctor(standard, holder["root"], "--json") assert result.returncode == 0, result.stdout + result.stderr @@ -1136,13 +1180,17 @@ def git_rm_refuses(project: Path, planted: Path) -> None: @pytest.mark.parametrize("state", ["untracked", "ignored"]) -def test_a_contracts_family_yaml_git_does_not_track_is_deleted( +def test_a_contracts_family_yaml_git_does_not_track_is_shown_not_deleted( standard, project, holder, state): """A `contracts/family.yaml` nobody has committed -- copied in and not - added, or hidden by the root's own excludes -- is the same FINDING, and - its NEXT is `git clean -f` of it, with nothing to commit, `-x` too where - the excludes hide it, which `clean` otherwise leaves. It names no `git - rm`, which fails on it; run as printed, the root is COMPLIANT again.""" + added, or hidden by the root's own excludes -- is the same FINDING. Its + NEXT is the `git status --ignored` that shows it, and never the deletion + (review of PR #555): no commit holds it, so deleting it cannot be + undone, and an agent runs the NEXT. The `why` names the `git clean -f` + that deletes it, `-x` too where the excludes hide it, which `clean` + otherwise leaves. It names no `git rm`, which fails on it. Run as + printed, the NEXT leaves the file where it is; the `why`'s deletion, run + as a person would, leaves the root COMPLIANT.""" planted = plant_family_yaml(project, holder) if state == "ignored": exclude = project / git("rev-parse", "--git-path", "info/exclude", @@ -1155,12 +1203,11 @@ def test_a_contracts_family_yaml_git_does_not_track_is_deleted( payload = json.loads(result.stdout) assert payload["verdict"] == "INVALID (manifest-kinds)", payload row = only_finding_is_manifest_kinds(result) - verb = "clean -f -x" if state == "ignored" else "clean -f" - assert row["next"] == removal_command(payload, verb, - ["contracts/family.yaml"]), row + names = ["contracts/family.yaml"] + assert row["next"] == removal_command(payload, SHOW_UNTRACKED, names), \ + row assert row["why"] == why_for_one( - "deletes contracts/family.yaml, which git does not track, so there " - "is nothing to commit"), row["why"] + shown_untracked(payload, names, state == "ignored")), row["why"] assert row["detail"]["registered_elsewhere"] == [ {"manifest": "contracts/family.yaml", "kind": "family-manifest", "validator": "scripts/validate-family.py", "runs_in": ["family"], @@ -1168,6 +1215,9 @@ def test_a_contracts_family_yaml_git_does_not_track_is_deleted( git_rm_refuses(project, planted) run_pasted(row["next"]) + assert planted.is_file(), "the NEXT deleted a file no commit holds" + run_pasted(removal_command( + payload, "clean -f -x" if state == "ignored" else "clean -f", names)) assert not planted.exists() compliant_again(standard, project) @@ -1261,26 +1311,39 @@ def test_manifests_that_come_out_differently_are_each_named( ("contracts/x-family.yaml", "changed")], row["detail"] -def test_a_name_git_reads_as_a_pattern_is_passed_literally(project): - """`git rm -- 'contracts/shape-pin[.]yaml'` takes out every tracked path - the pattern matches, the shape pin with it. So a name holding a pattern - character is passed after `--literal-pathspecs`, and the command, run - as printed, takes out that file alone; a name with none is spelled as - it always was.""" - module = doctor_module(REPO) - ctx = SimpleNamespace(root=project) - odd = "contracts/shape-pin[.]yaml" +#: Windows takes no `*` in a file name. +NO_STAR_IN_A_NAME = pytest.mark.skipif( + os.name == "nt", reason="Windows takes no `*` in a file name") + + +@pytest.mark.parametrize("odd", [ + pytest.param("contracts/*-pin.yaml", id="every-pin", + marks=NO_STAR_IN_A_NAME), + pytest.param("contracts/[s]hape-pin.yaml", id="the-shape-pin"), +]) +def test_a_name_git_reads_as_a_pattern_is_passed_literally(standard, project, + odd): + """A manifest of a kind nothing registers, committed under a name git's + pathspec reads as a pattern: `contracts/*-pin.yaml` matches every pin in + the root, `contracts/[s]hape-pin.yaml` the shape pin. The row's NEXT + passes it after `--literal-pathspecs`, so run as printed it stages the + removal of that file alone. Through the row, since a name that does not + end `.yaml` is no candidate at all (review of PR #555); a name with no + pattern in it is spelled as it always was.""" + module = doctor_module(standard) (project / odd).write_bytes(b"kind: service-inventory\n") commit_all(project, "a manifest named like a pattern") - command, said = module.removal_next(ctx, [odd], {odd: "committed"}) - assert pasted(command) == ["git", "-C", str(project), - "--literal-pathspecs", "rm", "--", odd], \ - command - assert said == TAKES_OUT.format(names=odd), said - run_pasted(command) - assert not (project / odd).exists() - assert (project / SHAPE_PIN).is_file(), "the shape pin went with it" - plain, _ = module.removal_next(ctx, ["contracts/x.yaml"], + matched = git("ls-files", "-z", "--", odd, cwd=project).stdout + assert len([name for name in matched.split("\0") if name]) > 1, matched + row = module.check_manifest_kinds(module.Context(project)).as_dict() + assert row["next"] == (f"git -C {module.quote_arg(project)} " + "--literal-pathspecs rm -- " + f"{module.quote_arg(odd)}"), row + run_pasted(row["next"]) + staged = git("diff", "--cached", "--name-only", "-z", cwd=project).stdout + assert [name for name in staged.split("\0") if name] == [odd], staged + plain, _ = module.removal_next(SimpleNamespace(root=project), + ["contracts/x.yaml"], {"contracts/x.yaml": "committed"}) assert "--literal-pathspecs" not in plain, plain @@ -1525,6 +1588,82 @@ def kinds() -> dict: row["detail"]["validated"], (pin, row) +#: The review of PR #555's two roots: a file a gate reads, with the fault +#: somewhere else -- a typo in a leg's role, or a wrong `kind:` in the naming +#: policy -- as the file edited, the bytes before and after, the file the +#: `manifest kinds` row must still count `validated`, and the code the +#: `manifest` row names. Main said `manifest kinds ok` over both; the row +#: then called the spec leg's pin, and the naming policy, read by nothing, +#: and its NEXT took each out. +FAULT_ELSEWHERE = { + "a-leg-role-typo": ( + "project.yaml", b" - role: spec\n", b" - role: specs\n", + "contracts/spec-pin.yaml (pinned_contract_manifest)", + "manifest-roles"), + "a-wrong-kind-where-a-gate-reads": ( + "contracts/repository-naming.yaml", + b"kind: repository-naming-policy\n", + b"kind: pinned_contract_manifest\n", + "contracts/repository-naming.yaml (pinned_contract_manifest)", + "policy-wrong-kind"), +} + + +@pytest.mark.parametrize("case", list(FAULT_ELSEWHERE)) +def test_a_file_a_gate_reads_is_never_unread_when_the_fault_is_elsewhere( + standard, project, case): + """THE GATE'S FINDING, NOT THIS ROW'S (review of PR #555). With a leg's + role misspelt, `contracts/spec-pin.yaml` is still the spec leg's pin, + the shape's own path (`own_pins`), which the gate reads again once the + role is right. With the naming policy declaring another kind, it is + still the file `validate-manifest.py` reads, and refuses. Each is the + `manifest` row's FINDING; `manifest kinds` says `ok`, as main did, and + lists the file `validated` -- never `unread`, so no NEXT takes it out. + """ + rel, before, after, validated, code = FAULT_ELSEWHERE[case] + path = project / rel + body = path.read_bytes() + assert body.count(before) == 1, body + path.write_bytes(body.replace(before, after)) + commit_all(project, f"{case}, committed") + rows = rows_of(doctor(standard, project, "--json")) + assert rows["manifest"]["status"] == "FINDING", rows["manifest"] + assert code in rows["manifest"]["reason"], rows["manifest"]["reason"] + kinds = rows["manifest-kinds"] + assert kinds["status"] == "ok", kinds + assert set(kinds["detail"]) == MANIFEST_KINDS_DETAIL_KEYS, kinds + assert validated in kinds["detail"]["validated"], kinds + + +def test_the_next_never_takes_out_a_file_a_gate_reads(standard, project): + """A kind nothing registers in the naming policy is still this row's + finding, as on main, but the gate that reads that path judges the file, + and a NEXT that took it out would take out the naming policy. So the row + has no NEXT for it, and its `why` says so; beside a file the row may take + out, the NEXT shows that file alone (review of PR #555).""" + module = doctor_module(standard) + naming = project / NAMING_COPY + body = naming.read_bytes() + assert body.count(b"kind: repository-naming-policy\n") == 1, body + naming.write_bytes(body.replace(b"kind: repository-naming-policy\n", + b"kind: service-inventory\n")) + judged = ("contracts/repository-naming.yaml is read by a gate here, " + "whose own row judges it: this row never takes out a file a " + "gate reads") + + row = module.check_manifest_kinds(module.Context(project)).as_dict() + assert row["detail"]["unregistered"] == [ + "contracts/repository-naming.yaml (service-inventory)"], row + assert row["next"] is None, row + assert row["why"].startswith(judged + ". Or register the kind "), row + + write_inventory(project) + row = module.check_manifest_kinds(module.Context(project)).as_dict() + assert row["next"] == (f"git -C {module.quote_arg(project)} " + f"{SHOW_UNTRACKED} -- {INVENTORY[0]}"), row + assert judged in row["why"], row["why"] + + def test_gitlinks_under_lists_one_directory_and_nothing_above_it(project): """`gitlinks_under` is asked for the members directory a hand-edited manifest names, so an empty one, an absolute one and one that climbs @@ -5578,8 +5717,11 @@ def test_an_unregistered_kind_beside_a_file_that_does_not_load(standard, what kind a file declares until it loads. Run, the row is the one the unregistered kind alone got, sentence, keys and all.""" write_inventory(project) - alone = rows_of(doctor(standard, project, "--json"))["manifest-kinds"] - assert alone["why"] == f"{INVENTORY_DELETED}. {UNREGISTERED_WAY}", alone + result = doctor(standard, project, "--json") + alone = rows_of(result)["manifest-kinds"] + assert alone["why"] == ( + shown_untracked(json.loads(result.stdout), INVENTORY) + ". " + + UNREGISTERED_WAY), alone assert set(alone["detail"]) == {"validated", "unregistered", "unreadable", "registered_kinds"}, alone["detail"] break_the_pin(project, conflicted_pin) diff --git a/tests/test_update_shape_merged_away.py b/tests/test_update_shape_merged_away.py index d6634f9b..0979928e 100644 --- a/tests/test_update_shape_merged_away.py +++ b/tests/test_update_shape_merged_away.py @@ -339,7 +339,9 @@ def test_the_pins_row_keeps_the_gate_while_check_refuses_a_linked_record( assert "update-shape.py" not in pins_row["next"], pins_row["next"] assert "merged-away" not in pins_row["reason"], pins_row["reason"] assert currency["detail"]["error"] == "update-rewrite-symlink", currency - assert currency["next"].startswith(f"replace {linked} "), currency["next"] + # No NEXT: replacing the link is a person's act, said in the `why` (#548). + assert currency["next"] is None, currency + assert currency["why"].startswith(f"replace {linked} "), currency["why"] (root / linked).unlink() shutil.move(str(end), str(root / linked)) From f112441cf7b94014060e923e3864a27ad51a7304 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Wed, 14 Oct 2026 19:06:51 +0000 Subject: [PATCH 3/3] shape-doctor.py: a wrong `kind:` in a file a gate reads gets a read-only NEXT that shows the commit or change that set it, and a `why` naming the kind a gate reads that path as and the row that really judges it, so the shape pin declaring `pinned_contract_manifes` or `family-manifest` is no longer a FINDING with no NEXT whose `why` points at a row that says `ok` (#548, #439, review of #555) Round 2 of the review of PR #555. What was wrong. Round 1 kept a file a gate reads out of every NEXT and said "its own row judges it". For the shape pin and a declared neutral product pin no row does: validate-pins.py's _check_shape_pin_header reads their fields and never their kind. So `kind: pinned_contract_manifes` (E1) or `kind: family-manifest` (E2) in contracts/shape-pin.yaml was INVALID (manifest-kinds) with no NEXT, a `why` sending the reader to a row that said ok, and an offer to register the kind or move the pin to a family holder. A leg pin of the wrong kind (E3), which the pins row does refuse (pin-wrong-kind), was still offered "Or register the kind". What changes. - kinds_read_here(ctx) maps each path a gate here reads to the kind it reads it as; kind_checked_by(ctx) names the paths whose gate refuses another kind, and its row: the naming policy (manifest, or family in a holder) and each declared leg's pin (pins). Nothing else, and nothing while the root's own manifest does not load. - A file at such a path declaring another kind is `judged`. The NEXT is read-only (showing_the_kind): `git -C diff HEAD -- ` for a change no commit holds, else `git -C log -p -1 -- `, or `git status --ignored` where git does not track it. The `why` names what that shows, the declared kind, the kind a gate here reads the path as, which row judges it ("the row's gate finds that too" only where kind_checked_by says so; otherwise "no gate here checks the kind it declares, so this row is the one that says so"), and to put the kind line back. No register, move-to-a-holder or rename way is offered for a judged file; files beside it to take out are named for the next run. - A registered kind whose check runs here, at a gated path read as another kind that no gate checks (the shape pin declaring `repository-naming-policy`), was `validated`, COMPLIANT. It is now a FINDING of this row, listed under the new `misdeclared` detail key, present only when there is one. - The untracked file's `why` spells the deleting `git clean` after a colon, with no backticks a pasted line would run, and the changed group's sentence says git rm without backticks too. Tests. tests/test_shape_doctor.py: E1 committed and uncommitted, E2 and E4 on the shape pin through the whole doctor, E3 on the spec leg's pin, and a declared neutral pin in process; each NEXT is run and shown to change nothing and to print the kind line, and the shape pin, put back, is COMPLIANT. run_read_only takes a `log` NEXT too. The naming policy's round-1 test now expects the `diff HEAD` NEXT and the manifest row as its judge. tests/test_doctor_next_is_a_command.py holds both new NEXTs to the one-command property and the new untracked sentence. Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 315 +++++++++++++++++-------- tests/test_doctor_next_is_a_command.py | 49 +++- tests/test_shape_doctor.py | 212 +++++++++++++++-- 3 files changed, 457 insertions(+), 119 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 91e0cca3..17936b87 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -903,24 +903,51 @@ def pins_read_here(ctx: Context) -> set | None: return own_pins(ctx) | declared_pins(ctx) -def read_by_a_gate(ctx: Context) -> set: - """Every root path a gate that runs here reads a file at, as any kind - (review of PR #555): each kind's `read_at` for the kinds whose check - runs on this root, and `own_pins`. - - A FILE AT ONE OF THESE PATHS IS THAT GATE'S TO JUDGE. A - `contracts/repository-naming.yaml` declaring `kind: - pinned_contract_manifest` is refused by `validate-manifest.py` - (`policy-wrong-kind`), and the `manifest` row names it. Called read by - nothing here, it got a NEXT that took out the file the gate reads. So - such a file is never `unread`, and the row's NEXT never takes out a - file at one of these paths, whatever this row finds in it. - """ - paths = own_pins(ctx) - for entry in MANIFEST_VALIDATORS.values(): +def kinds_read_here(ctx: Context) -> dict: + """`{path: kind}`: every root path a gate that runs here reads a file + at, and the kind it reads that file as -- each kind's `read_at`, for the + kinds whose check runs on this root, and `own_pins` as pins (review of + PR #555). + + A FILE AT ONE OF THESE PATHS IS NEVER `unread`, AND THE NEXT NEVER TAKES + IT OUT, whatever this row finds in it. A `contracts/repository-naming.yaml` + declaring `kind: pinned_contract_manifest` called read by nothing got a + NEXT that took out the file `validate-manifest.py` reads. What is wrong + with such a file is the kind it declares, and the row says which kind + that path is read as (`taking_out`). + """ + kinds = {pin: "pinned_contract_manifest" for pin in own_pins(ctx)} + for kind, entry in MANIFEST_VALIDATORS.items(): if ctx.kind in entry.runs_in(): - paths |= entry.reads(ctx) or set() - return paths + kinds.update((path, kind) for path in entry.reads(ctx) or ()) + return kinds + + +def kind_checked_by(ctx: Context) -> dict: + """`{path: row}`: the paths whose gate refuses a file declaring another + kind than it reads the path as, and the row that runs that gate (review + of PR #555, round 2). + + ONLY THESE, because the row says that gate judges the file, and that + has to be true. `validate-pins.py` checks the `kind:` of each LEG pin + `project.yaml` declares (`pin-wrong-kind`), and `NamingPolicy.load`, + which `validate-manifest.py` and `validate-family.py` both call, that of + the naming policy (`policy-wrong-kind`). Nothing checks the kind of the + shape pin (`_check_shape_pin_header` reads its fields, not its kind), of + a neutral product pin, or of `contracts/spec-pin.yaml` while no leg + declares the role `spec`. And where the root's own manifest does not + load, its gate stops there and checks nothing. For every other path in + `kinds_read_here`, this row is the one that says the kind is wrong. + """ + if ctx.manifest_refusal is not None: + return {} + if ctx.kind == FAMILY: + return {NAMING_COPY: "family"} + checked = {NAMING_COPY: "manifest"} + checked.update((f"contracts/{leg['role']}-pin.yaml", "pins") + for leg in ctx.legs() + if isinstance(leg.get("role"), str) and leg["role"]) + return checked MANIFEST_VALIDATORS: dict[str, ManifestKind] = { @@ -2339,17 +2366,22 @@ def register_way(names: list) -> str: f"{'it' if one else 'each'}, as the human decides") -def registered_elsewhere(ctx: Context, found: list, - states: dict) -> tuple[str, list, list]: +def registered_elsewhere(ctx: Context, found: list, states: dict, + held: set = frozenset()) -> tuple[str, list, list]: """Manifests whose kind is registered to a gate this root never runs: the sentence, the other way out, and the facts (#327). - `found` is `(path, kind, entry)` for each, in the row's own order, and - `states` each file's git state, from `git_states`. The sentence opens - the way the unregistered kind's does -- "no validator ... is registered - for kind" -- because to this root it is the same fact: the registry has - a validator for the kind, and none that runs here. Each file says which - validator that is and where it does run. + `found` is `(path, kind, entry)` for each, in the row's own order, + `states` each file's git state, from `git_states`, and `held` the + files a gate here reads, which are offered no other way out: what is + wrong with the shape pin declaring `family-manifest` is its kind line, + not where a family holder is (review of PR #555, round 2). + + The sentence opens the way the unregistered kind's does -- "no + validator ... is registered for kind" -- because to this root it is the + same fact: the registry has a validator for the kind, and none that + runs here. Each file says which validator that is and where it does + run. A FINDING, NOT A NOTE, for the reason an unregistered kind is one. A `note` is a difference nothing asserts, and a file declaring `kind:` @@ -2366,8 +2398,6 @@ def registered_elsewhere(ctx: Context, found: list, makes the root read as a project, and in an assembly root it is #211's finding instead of this one. Each file's state is `detail`'s `git`. """ - names = [rel for rel, _, _ in found] - one = len(names) == 1 clauses, detail = [], [] for rel, kind, entry in found: runs = entry.runs_in() @@ -2381,7 +2411,11 @@ def registered_elsewhere(ctx: Context, found: list, sentence = (f"no validator that runs in {ROOT_IN_WORDS[ctx.kind]} is " "registered for kind " + ", ".join(clauses)) homes = {rel: entry.home or "a root whose gate runs its validator" - for rel, _, entry in found} + for rel, _, entry in found if rel not in held} + if not homes: + return sentence, [], detail + names = list(homes) + one = len(names) == 1 if len(set(homes.values())) == 1: other_exit = (f"move what {listed(names)} " f"describe{'s' if one else ''} to {homes[names[0]]}") @@ -2441,26 +2475,27 @@ def read_nowhere(ctx: Context, found: list, return sentence, ways, detail -def read_here(ctx: Context, path: Path, rel: str, paths: set) -> bool: - """Whether a gate here reads the file at `path` (`rel` from the root), - `paths` being the root paths it reads that kind at (#439). +def read_here(ctx: Context, path: Path, rel: str, paths) -> str | None: + """Which of `paths` -- the root paths a gate here reads -- is the file at + `path` (`rel` from the root), or None where it is none of them (#439). BY THE FILE, NOT ONLY BY ITS SPELLING. A filesystem that folds case hands `validate-pins.py` the file `contracts/Spec-pin.yaml` when it opens `contracts/spec-pin.yaml`, and a hard link is the file its other name is. Called read by nothing, such a file would get a NEXT that takes out a pin a gate does read. So a name that is not one of `paths` is - still read where it is the same file as one of them. + still read where it is the same file as one of them, and the path it + is read at is the one returned. """ if rel in paths: - return True + return rel for other in paths: try: if path.samefile(ctx.root / other): - return True + return other except OSError: continue - return False + return None def git_states(root: Path, names: list) -> dict: @@ -2566,9 +2601,7 @@ def removal_next(ctx: Context, names: list, if not groups: return None, f"take {listed(names)} out of this root" state, group = next(iter(groups.items())) - literal = (" --literal-pathspecs" - if any(PATHSPEC_MAGIC.search(name) for name in group) else "") - git_c = f"git -C {quote_arg(ctx.root)}{literal}" + git_c = git_on(ctx, group) spelled = " ".join(quote_arg(name) for name in group) if state == "committed": command = f"{git_c} rm -- {spelled}" @@ -2578,15 +2611,18 @@ def removal_next(ctx: Context, names: list, ignored = any(states[name] == "ignored" for name in group) command = f"{git_c} status --ignored -- {spelled}" them = "it" if len(group) == 1 else "them" + # The deletion is spelled after a colon and never in backticks, + # which an interactive zsh or bash would run where this sentence was + # pasted whole (review of PR #555, round 2). said = (f"shows {listed(group)}, which git does not track: no " f"commit holds {them}, so deleting {them} cannot be undone " - f"and is a person's act, never this row's -- `{git_c} clean " - f"-f{' -x' if ignored else ''} -- {spelled}` deletes {them}, " - "and there is nothing to commit") + f"and is a person's act, never this row's, with nothing to " + f"commit after. A person deletes {them} with: {git_c} clean " + f"-f{' -x' if ignored else ''} -- {spelled}") else: command = f"{git_c} status -- {spelled}" said = (f"shows the changes no commit holds in {listed(group)}, " - "which `git rm` will not discard: commit or drop them, and " + "which git rm will not discard: commit or drop them, and " "the next run names the way out") rest = [name for other, more in groups.items() if other != state for name in more] @@ -2595,31 +2631,83 @@ def removal_next(ctx: Context, names: list, return command, said -def taking_out(ctx: Context, names: list, states: dict, gated: set, +def git_on(ctx: Context, names: list) -> str: + """`git -C `, quoted for the reader's shell, and + `--literal-pathspecs` where git would read one of `names` as a + pattern.""" + literal = (" --literal-pathspecs" + if any(PATHSPEC_MAGIC.search(name) for name in names) else "") + return f"git -C {quote_arg(ctx.root)}{literal}" + + +def showing_the_kind(ctx: Context, rel: str, + states: dict) -> tuple[str | None, str | None]: + """`(command, said)`: the read-only command that shows how `rel`, a file + a gate here reads, came to declare the kind it does, and what it shows + (review of PR #555, round 2): its uncommitted change where it has one, + else the last commit that changed it, else the file itself where git + does not track it. None where git cannot say.""" + git_c, spelled = git_on(ctx, [rel]), quote_arg(rel) + state = states.get(rel) + if state == "changed": + return (f"{git_c} diff HEAD -- {spelled}", + f"shows the change to {rel} that no commit holds") + if state == "committed": + return (f"{git_c} log -p -1 -- {spelled}", + f"shows the last commit that changed {rel}") + if state in ("untracked", "ignored"): + return (f"{git_c} status --ignored -- {spelled}", + f"shows {rel}, which git does not track") + return None, None + + +def misdeclared_said(rel: str, kind: str, read_as: str, + row: str | None) -> str: + """The sentence of the `manifest kinds` row's `why` for a file a gate + here reads as `read_as` that declares `kind` instead (review of PR + #555, round 2): which row judges it -- that gate's own, only where the + gate checks the kind (`kind_checked_by`), else this one -- and to put + its kind line back.""" + judge = (f"the {row} row's gate finds that too" if row else + "no gate here checks the kind it declares, so this row is the " + "one that says so") + return (f"{rel} declares kind {kind} where a gate here reads it as " + f"{read_as}, and {judge}: put its kind line back to {read_as}") + + +def taking_out(ctx: Context, names: list, states: dict, judged: list, ways: list) -> tuple[str | None, str]: - """`(command, why)` for the `manifest kinds` row's files to take out, - `ways` being each fault's other way out, a sentence each (#548). - - A FILE A GATE HERE READS IS NEVER TAKEN OUT (review of PR #555). An - unregistered kind, or one whose check runs elsewhere, at a path in - `gated` is still this row's finding, as it always was, but the gate - that reads that path judges it in its own row, and a NEXT that took it - out would take out the shape pin, a leg pin or the naming policy. So - `removal_next` is asked only of the rest, and the `why` says which - files it leaves, and why. With none left, the row has no NEXT. - """ - judged = [name for name in names - if read_here(ctx, ctx.root / name, name, gated)] - rest = [name for name in names if name not in judged] - command, taken = (removal_next(ctx, rest, states) if rest - else (None, None)) - said = [taken] if taken else [] + """`(command, why)` for the `manifest kinds` row's files, `names` being + those to take out, `judged` those a gate here reads, as `(path, kind, + read_as, row)`, and `ways` each other fault's other way out, a sentence + each (#548). + + A FILE A GATE HERE READS IS NEVER TAKEN OUT (review of PR #555). It is + the shape pin, a leg pin, a neutral product pin or the naming policy, + and what is wrong with it is the kind it declares. So the NEXT is the + read-only `showing_the_kind` for the first of them, and the `why` names, + for each, the kind a gate here reads its path as, the kind it declares + instead, and which row judges it, and says to put its kind line back. + No other way out is offered for such a file: registering its kind, or + moving what it describes to a holder, is no answer for a pin (round 2). + The files to take out are named for the next run. With none judged, the + NEXT is `removal_next`'s, as before. + """ + said = [] if judged: - one = len(judged) == 1 - said.append(f"{listed(judged)} {'is' if one else 'are'} read by a " - "gate here, whose own row judges " - f"{'it' if one else 'them'}: this row never takes out a " - "file a gate reads") + command, shown = showing_the_kind(ctx, judged[0][0], states) + said += [shown] if shown else [] + said += [misdeclared_said(*entry) for entry in judged] + if len(judged) > 1: + said.append("The next run shows " + + listed([entry[0] for entry in judged[1:]])) + if names: + said.append(f"The next run names the way out for {listed(names)}") + elif names: + command, taken = removal_next(ctx, names, states) + said.append(taken) + else: + command = None return command, ". ".join([*said, *ways]) @@ -2662,9 +2750,17 @@ def check_manifest_kinds(ctx: Context) -> Row: to its `home`, declare the pin, move a membership to a holder. Those sentences were the NEXT, joined by `; `, and held `family.py init` and `family.py add` in backticks, which an interactive zsh runs where the - line is pasted (#277). A path a gate here reads (`read_by_a_gate`) is - never `unread` and never taken out: what is wrong with a file there is - that gate's finding, and `taking_out` says so (review of PR #555). + line is pasted (#277). + + A FILE AT A PATH A GATE HERE READS IS NEVER `unread` AND NEVER TAKEN OUT + (review of PR #555). `kinds_read_here` says which kind each such path is + read as. Declaring another, the file is a fault in the kind line, and + the NEXT shows how it came to say that (`showing_the_kind`). The `why` + names both kinds and the row that judges it: the gate's own, where that + gate checks the kind (`kind_checked_by`), else this one. A kind whose + check runs here is this row's finding only there, as `misdeclared`: the + shape pin declaring `repository-naming-policy` read COMPLIANT, though + no gate checks the shape pin's kind (round 2). THE ROOT'S OWN MANIFEST, WHEN IT DOES NOT LOAD, IS NOT THIS ROW'S (#205). The `manifest` (or `family`) row quotes its refusal and names its gate, @@ -2702,12 +2798,20 @@ def check_manifest_kinds(ctx: Context) -> Row: #: `(path, kind, paths)` for each manifest whose gate runs here and #: never reads its path, with the paths it does read that kind at (#439). unread: list[tuple] = [] + #: `(path, kind, read_as)` for each file of a kind whose check runs here, + #: at a path a gate reads as another kind and checks no kind at (review + #: of PR #555, round 2): this row is the only one that can say so. + misdeclared: list[tuple] = [] + #: `(path, kind, read_as, row)` for each file this row finds a fault in + #: at a path a gate here reads: never taken out (`taking_out`). + judged: list[tuple] = [] elsewhere = (ctx.manifest_path if ctx.manifest_refusal is not None else None) beside: tuple[str, dict] | None = None - #: Every path a gate here reads, as any kind: never `unread`, and never - #: taken out by the NEXT (review of PR #555). - gated = read_by_a_gate(ctx) + #: Every path a gate here reads, and the kind it reads it as, and the + #: paths whose gate refuses another kind (review of PR #555). + read_as = kinds_read_here(ctx) + checked = kind_checked_by(ctx) for path in manifest_candidates(ctx.root): if path == elsewhere: continue @@ -2736,31 +2840,46 @@ def check_manifest_kinds(ctx: Context) -> Row: beside = family_beside_project(ctx, data) continue entry = MANIFEST_VALIDATORS.get(kind) - if entry is None: - unknown.append((rel, kind)) - continue - if ctx.kind not in entry.runs_in(): - ungated.append((rel, kind, entry)) + # The path a gate here reads this file at, as any kind, if any. + at = read_here(ctx, path, rel, read_as) + wrong = (rel, kind, read_as[at], checked.get(at)) \ + if at is not None and read_as[at] != kind else None + if entry is None or ctx.kind not in entry.runs_in(): + if entry is None: + unknown.append((rel, kind)) + else: + ungated.append((rel, kind, entry)) + judged += [wrong] if wrong else [] continue - # At a path a gate here reads as ANY kind, a wrong kind is that - # gate's own finding, not a file nothing reads (review of PR #555). paths = entry.reads(ctx) - if paths is None or read_here(ctx, path, rel, gated): - known.append(f"{rel} ({kind})") - else: + if at is None and paths is not None: unread.append((rel, kind, sorted(paths))) + elif wrong is not None and wrong[3] is None: + # A kind whose check runs here, at a path a gate reads as + # another kind and never checks the kind of: no row but this + # one says so (review of PR #555, round 2). + misdeclared.append(wrong[:3]) + judged.append(wrong) + else: + # Its own path, or one whose gate refuses the kind it declares + # in its own row, which is that gate's finding (round 1). + known.append(f"{rel} ({kind})") detail = {"validated": known, "unregistered": [f"{rel} ({kind})" for rel, kind in unknown], "unreadable": unreadable, "registered_kinds": sorted(MANIFEST_VALIDATORS)} #: Every file the row's NEXT takes out of the root, in the row's order, - #: and how git has each, asked once (#548). - names = ([rel for rel, _ in unknown] + [rel for rel, _, _ in ungated] - + [rel for rel, _, _ in unread] - + ([FAMILY_MANIFEST] if beside is not None else [])) - states = git_states(ctx.root, names) if names else {} - ungated_said = (registered_elsewhere(ctx, ungated, states) if ungated - else None) + #: and how git has each and every file it judges, asked once (#548). + held = {entry[0] for entry in judged} + names = [rel for rel in + ([rel for rel, _ in unknown] + [rel for rel, _, _ in ungated] + + [rel for rel, _, _ in unread] + + ([FAMILY_MANIFEST] if beside is not None else [])) + if rel not in held] + states = (git_states(ctx.root, names + [entry[0] for entry in judged]) + if names or judged else {}) + ungated_said = (registered_elsewhere(ctx, ungated, states, held) + if ungated else None) if ungated_said is not None: # ONLY WHEN THERE IS ONE, like `family_beside` below, so every root # without such a file reports the row it reported before #327. @@ -2769,6 +2888,11 @@ def check_manifest_kinds(ctx: Context) -> Row: if unread_said is not None: # Likewise, so every root without one reports the row of #327. detail["unread"] = unread_said[2] + if misdeclared: + # Likewise. + detail["misdeclared"] = [ + {"manifest": rel, "kind": kind, "read_as": read, + "git": states.get(rel)} for rel, kind, read in misdeclared] # Only when there is one, so every other root's row is the row it was. aside = "" if elsewhere is not None: @@ -2780,15 +2904,17 @@ def check_manifest_kinds(ctx: Context) -> Row: # ONLY WHEN THERE IS ONE, so every root without it reports the same # row, byte for byte, as it did before #211. detail["family_beside"] = beside[1] - if names or unreadable: + if names or judged or unreadable: parts = [] #: The sentences of the `why` that name the other way out of each - #: fault, in the row's order: never the NEXT's (#548). + #: fault, in the row's order: never the NEXT's (#548), and never for + #: a file a gate here reads (`taking_out`). ways = [] if unknown: parts.append("no validator registered for kind " + ", ".join( detail["unregistered"])) - ways.append(register_way([rel for rel, _ in unknown])) + free = [rel for rel, _ in unknown if rel not in held] + ways += [register_way(free)] if free else [] if unreadable: said = {rel: ": ".join(filter(None, (why["code"], why["detail"]))) for rel, why in refusals.items()} @@ -2798,6 +2924,11 @@ def check_manifest_kinds(ctx: Context) -> Row: if found is not None: parts.append(found[0]) ways += found[1] + if misdeclared: + parts.append(", ".join( + f"{rel} declares kind {kind} where a gate here reads it as " + f"{read}, and no gate here checks the kind it declares" + for rel, kind, read in misdeclared)) if beside is not None: parts.append(beside[0]) ways.append(FAMILY_BESIDE_WAY) @@ -2805,7 +2936,7 @@ def check_manifest_kinds(ctx: Context) -> Row: restore, command, why = unreadable_next(ctx, unreadable, detail) detail.update({"restore": restore, "refusals": refusals}) else: - command, why = taking_out(ctx, names, states, gated, ways) + command, why = taking_out(ctx, names, states, judged, ways) return Row("manifest-kinds", LABEL_MANIFEST_KINDS, FINDING, "; ".join(parts) + aside, command, detail, why=why) return Row("manifest-kinds", LABEL_MANIFEST_KINDS, OK, diff --git a/tests/test_doctor_next_is_a_command.py b/tests/test_doctor_next_is_a_command.py index 4bf79350..86060bff 100644 --- a/tests/test_doctor_next_is_a_command.py +++ b/tests/test_doctor_next_is_a_command.py @@ -79,8 +79,9 @@ #: that owns them: the row is that module's, and so is `rows_of`, which asks #: the guard of every row it reads. from test_shape_doctor import ( - READ_NOWHERE, assert_next_holds_no_prose, commit_all, family_beside, - outside_single_quotes, plant_family_yaml, write_inventory, + MISSPELT_KIND, READ_NOWHERE, assert_next_holds_no_prose, commit_all, + family_beside, give_a_kind, outside_single_quotes, plant_family_yaml, + write_inventory, ) #: The lockstep remedy `validate-pins.py` prints under its refusals, and the #: one every `Refusal` carried before #370, from the module that spells it; @@ -469,12 +470,29 @@ def _removal_next_loose(standard, project, holder, tmp_path): def _removal_next_changed(standard, project, holder, tmp_path): # #327's file, staged and never committed: the `git status` that shows - # the change `git rm` will not discard. + # the change git rm will not discard. plant_family_yaml(project, holder) git("add", "--", "contracts/family.yaml", cwd=project) return _kinds(standard, project) +def _taking_out_committed(standard, project, holder, tmp_path): + # The shape pin given a kind nothing registers, committed: no NEXT and a + # `why` naming a row that said `ok` (review of PR #555, round 2), now + # the `git log -p -1` that shows the commit. + give_a_kind(project, SHAPE_PIN, MISSPELT_KIND, True) + return _kinds(standard, project) + + +def _taking_out_changed(standard, project, holder, tmp_path): + # The naming policy given a kind nothing registers, and not committed: + # the `git diff HEAD` that shows the change, never a NEXT taking it out. + naming = project / NAMING_COPY + naming.write_bytes(naming.read_bytes().replace( + b"kind: repository-naming-policy\n", b"kind: service-inventory\n")) + return _kinds(standard, project) + + def _family_beside(standard, project, holder, tmp_path): # #211's file, whose NEXT held `family.py init` and `family.py add` in # backticks: the `git rm` of it, and those two in the `why` (#548). @@ -499,8 +517,10 @@ def _every_fault(standard, project, holder, tmp_path): #: VERBATIM, less the `#`, so these are the words main, doctors A and B and #: lane 2's #237, #291 and #496 printed. The `manifest kinds` NEXTs #277 #: left as prose (#211, #327) are here too since #548, by the git state the -#: one command they print now reaches (`removal_next`), with the sentence -#: that now opens their `why`; a `Refusal.remediation` reaches no NEXT +#: one command they print now reaches (`removal_next`), or, for a file a +#: gate here reads, the read-only one that shows its kind line (`taking_out`, +#: review of PR #555), with the sentence that now opens their `why`; a +#: `Refusal.remediation` reaches no NEXT #: (`test_a_refusal_a_check_lets_out_has_no_next_and_keeps_its_remedy`). CASES = { "validator_row-neither-readable": ( @@ -653,12 +673,27 @@ def _every_fault(standard, project, holder, tmp_path): _removal_next_loose, "shows contracts/inventory.yaml, which git does not track: no commit " "holds it, so deleting it cannot be undone and is a person's act, " - "never this row's -- `git -C "), + "never this row's, with nothing to commit after. A person deletes it " + "with: git -C "), "removal_next-changed": ( _removal_next_changed, "shows the changes no commit holds in contracts/family.yaml, which " - "`git rm` will not discard: commit or drop them, and the next run " + "git rm will not discard: commit or drop them, and the next run " "names the way out. Or move what contracts/family.yaml describes"), + "taking_out-committed": ( + _taking_out_committed, + "shows the last commit that changed contracts/shape-pin.yaml. " + "contracts/shape-pin.yaml declares kind pinned_contract_manifes " + "where a gate here reads it as pinned_contract_manifest, and no gate " + "here checks the kind it declares, so this row is the one that says " + "so"), + "taking_out-changed": ( + _taking_out_changed, + "shows the change to contracts/repository-naming.yaml that no commit " + "holds. contracts/repository-naming.yaml declares kind " + "service-inventory where a gate here reads it as " + "repository-naming-policy, and the manifest row's gate finds that " + "too"), "check_manifest_kinds-family-beside": ( _family_beside, "takes family.yaml out of this root: commit that on a branch and land " diff --git a/tests/test_shape_doctor.py b/tests/test_shape_doctor.py index 8bec8086..ba18d4db 100644 --- a/tests/test_shape_doctor.py +++ b/tests/test_shape_doctor.py @@ -652,14 +652,16 @@ def removal_command(payload: dict, verb: str, names: list) -> str: def shown_untracked(payload: dict, names: list, ignored: bool = False) -> str: """What the `why` says first where the NEXT shows `names`, which git does not track: that deleting them cannot be undone, and the `git clean` - that does it, for a person to run (review of PR #555).""" + that does it, for a person to run (review of PR #555). The command + follows a colon and is in no backticks, which a shell the sentence was + pasted into whole would run (round 2).""" them = "it" if len(names) == 1 else "them" clean = removal_command(payload, "clean -f -x" if ignored else "clean -f", names) return (f"shows {', '.join(names)}, which git does not track: no commit " f"holds {them}, so deleting {them} cannot be undone and is a " - f"person's act, never this row's -- `{clean}` deletes {them}, " - "and there is nothing to commit") + f"person's act, never this row's, with nothing to commit after. " + f"A person deletes {them} with: {clean}") def family_beside(project: Path, holder: dict) -> str: @@ -1246,7 +1248,7 @@ def test_a_contracts_family_yaml_with_changes_no_commit_holds( ["contracts/family.yaml"]), row assert row["why"] == why_for_one( "shows the changes no commit holds in contracts/family.yaml, which " - "`git rm` will not discard: commit or drop them, and the next run " + "git rm will not discard: commit or drop them, and the next run " "names the way out"), row["why"] assert [entry["git"] for entry in row["detail"]["registered_elsewhere"]] == ["changed"], row @@ -1637,31 +1639,37 @@ def test_a_file_a_gate_reads_is_never_unread_when_the_fault_is_elsewhere( def test_the_next_never_takes_out_a_file_a_gate_reads(standard, project): """A kind nothing registers in the naming policy is still this row's - finding, as on main, but the gate that reads that path judges the file, - and a NEXT that took it out would take out the naming policy. So the row - has no NEXT for it, and its `why` says so; beside a file the row may take - out, the NEXT shows that file alone (review of PR #555).""" + finding, as on main, but a NEXT that took the file out would take out + the naming policy (review of PR #555). The NEXT shows the change that + set the kind, the `why` names the kind a gate here reads that path as + and the `manifest` row, whose gate checks it, and offers no other way + out -- registering the kind is no answer for the naming policy (round + 2). Beside a file the row may take out, the next run names that one.""" module = doctor_module(standard) naming = project / NAMING_COPY body = naming.read_bytes() assert body.count(b"kind: repository-naming-policy\n") == 1, body naming.write_bytes(body.replace(b"kind: repository-naming-policy\n", b"kind: service-inventory\n")) - judged = ("contracts/repository-naming.yaml is read by a gate here, " - "whose own row judges it: this row never takes out a file a " - "gate reads") + shown = (f"shows the change to {NAMING_COPY} that no commit holds. " + f"{NAMING_COPY} declares kind service-inventory where a gate " + "here reads it as repository-naming-policy, and the manifest " + "row's gate finds that too: put its kind line back to " + "repository-naming-policy") row = module.check_manifest_kinds(module.Context(project)).as_dict() assert row["detail"]["unregistered"] == [ "contracts/repository-naming.yaml (service-inventory)"], row - assert row["next"] is None, row - assert row["why"].startswith(judged + ". Or register the kind "), row + assert row["next"] == (f"git -C {module.quote_arg(project)} diff HEAD " + f"-- {NAMING_COPY}"), row + assert row["why"] == shown, row["why"] write_inventory(project) row = module.check_manifest_kinds(module.Context(project)).as_dict() - assert row["next"] == (f"git -C {module.quote_arg(project)} " - f"{SHOW_UNTRACKED} -- {INVENTORY[0]}"), row - assert judged in row["why"], row["why"] + assert row["next"] == (f"git -C {module.quote_arg(project)} diff HEAD " + f"-- {NAMING_COPY}"), row + assert row["why"] == (f"{shown}. The next run names the way out for " + f"{INVENTORY[0]}. {UNREGISTERED_WAY}"), row["why"] def test_gitlinks_under_lists_one_directory_and_nothing_above_it(project): @@ -6029,11 +6037,13 @@ def merge_by_hand(path: Path) -> None: def run_read_only(standard: Path, root: Path, row: dict) -> bytes: - """Run the printed NEXT, which must be `git -C diff ...`, and - require that it changed nothing: the file's bytes, the status, HEAD. - Its output.""" + """Run the printed NEXT, which must be `git -C diff ...`, or the + `git -C log -p ...` that shows the last commit to change a file + (review of PR #555, round 2), and require that it changed nothing: the + file's bytes, the status, HEAD. Its output.""" argv = pasted(row["next"]) - assert argv[:2] == ["git", "-C"] and argv[3] == "diff", row["next"] + assert argv[:2] == ["git", "-C"], row["next"] + assert argv[3] in ("diff", "log"), row["next"] assert "checkout" not in row["next"], row["next"] rel = argv[-1] before = ((root / rel).read_bytes(), @@ -6380,6 +6390,168 @@ def test_a_neutral_product_pin_is_shown_never_put_back(standard, project, assert b"2" * 40 in before +# --- a wrong `kind:` in a file a gate reads (review of PR #555, round 2) ---- +# +# Round 1 kept such a file out of every NEXT and said that "its own row +# judges it". For the shape pin and a neutral product pin no row does: +# `validate-pins.py` reads their fields and never their kind. So the row was +# a FINDING with no NEXT, and its `why` sent the reader to a row that said +# `ok`. The NEXT now shows, read-only, how the file came to declare that +# kind, and the `why` names the kind a gate here reads its path as, and the +# row that judges it. + +#: The kind a gate here reads every pin as. +PIN_KIND = "pinned_contract_manifest" +#: A kind one letter short of it, which nothing registers. +MISSPELT_KIND = "pinned_contract_manifes" + +#: Each wrong kind given to the shape pin, whether it is committed, the +#: row's reason, and the `detail` keys beyond main's. E1 and E2 are the +#: review's: a kind nothing registers, and one registered to a gate that +#: runs only in a holder. E4 is a kind whose check runs here, which the row +#: counted `validated`, COMPLIANT, though no gate checks the shape pin's kind. +SHAPE_PIN_WRONG_KIND = { + "e1-a-kind-nothing-registers": ( + MISSPELT_KIND, True, + f"no validator registered for kind {SHAPE_PIN} ({MISSPELT_KIND})", + set()), + "e1-uncommitted": ( + MISSPELT_KIND, False, + f"no validator registered for kind {SHAPE_PIN} ({MISSPELT_KIND})", + set()), + "e2-a-kind-a-holder-runs": ( + "family-manifest", True, + "no validator that runs in an assembly root is registered for kind " + f"{SHAPE_PIN} (family-manifest: scripts/validate-family.py runs only " + "in a family holder)", {"registered_elsewhere"}), + "e4-a-kind-checked-here": ( + "repository-naming-policy", True, + f"{SHAPE_PIN} declares kind repository-naming-policy where a gate " + f"here reads it as {PIN_KIND}, and no gate here checks the kind it " + "declares", {"misdeclared"}), +} + + +def misdeclared_why(rel: str, kind: str, judge: str | None, + committed: bool) -> str: + """The `manifest kinds` row's whole `why` for `rel`, a pin a gate here + reads, declaring `kind`: what its NEXT shows, both kinds, which row + judges it -- `judge`'s gate, or this row where no gate checks the kind + -- and to put the kind line back. No other way out: registering the + kind, or moving what the file describes to a holder, is no answer for a + pin. Spelt out here for the reason `FAMILY_BESIDE_WAY` is.""" + shown = (f"shows the last commit that changed {rel}" if committed else + f"shows the change to {rel} that no commit holds") + judged = (f"the {judge} row's gate finds that too" if judge else + "no gate here checks the kind it declares, so this row is the " + "one that says so") + return (f"{shown}. {rel} declares kind {kind} where a gate here reads " + f"it as {PIN_KIND}, and {judged}: put its kind line back to " + f"{PIN_KIND}") + + +def give_a_kind(root: Path, rel: str, kind: str, commit: bool) -> bytes: + """`rel`'s `kind:` line made to declare `kind`, committed or not, and + the bytes it held before, to put back.""" + path = root / rel + body = path.read_bytes() + line = f"kind: {PIN_KIND}\n".encode() + assert body.count(line) == 1, body + path.write_bytes(body.replace(line, f"kind: {kind}\n".encode())) + if commit: + commit_all(root, f"{rel} declares {kind}") + return body + + +def shows_the_kind(standard: Path, root: Path, row: dict, kind: str) -> None: + """Run the row's read-only NEXT, which changes nothing, and require that + it shows the line that declares `kind`.""" + shown = run_read_only(standard, root, row).splitlines() + assert f"+kind: {kind}".encode() in shown, shown + + +@pytest.mark.parametrize("case", list(SHAPE_PIN_WRONG_KIND)) +def test_a_wrong_kind_in_the_shape_pin_is_shown_never_a_dead_end( + standard, project, case): + """E1, E2 and E4: the shape pin given another kind, a FINDING of this + row alone, INVALID. Its NEXT is the `git log -p -1` of the commit that + gave it that kind, or the `git diff HEAD` of a change no commit holds, + and its `why` says no gate here checks the kind, so this row says so, + and to put the kind line back -- nothing else. Run as printed, the NEXT + shows the line and changes nothing; with the line put back, the root is + COMPLIANT again. + """ + kind, commit, reason, more = SHAPE_PIN_WRONG_KIND[case] + body = give_a_kind(project, SHAPE_PIN, kind, commit) + result = doctor(standard, project, "--json") + assert result.returncode == 1, result.stdout + result.stderr + payload = json.loads(result.stdout) + assert payload["verdict"] == "INVALID (manifest-kinds)", payload + row = only_finding_is_manifest_kinds(result) + assert row["reason"] == reason, row["reason"] + assert set(row["detail"]) == MANIFEST_KINDS_DETAIL_KEYS | more, row + if "misdeclared" in more: + assert row["detail"]["misdeclared"] == [ + {"manifest": SHAPE_PIN, "kind": kind, "read_as": PIN_KIND, + "git": "committed"}], row["detail"] + assert row["next"] == removal_command( + payload, "log -p -1" if commit else "diff HEAD", [SHAPE_PIN]), row + assert row["why"] == misdeclared_why(SHAPE_PIN, kind, None, commit), \ + row["why"] + shows_the_kind(standard, project, row, kind) + + (project / SHAPE_PIN).write_bytes(body) + if commit: + commit_all(project, "Put the shape pin's kind line back") + compliant_again(standard, project) + + +def test_a_leg_pin_of_the_wrong_kind_is_offered_no_registration(standard, + project): + """E3: the spec leg's pin given a kind nothing registers. The `pins` + row's gate refuses it `pin-wrong-kind`, and this row's `why` says that + gate finds it too, where it offered to register the kind; the NEXT + shows the commit that gave it.""" + give_a_kind(project, SPEC_PIN, MISSPELT_KIND, True) + result = doctor(standard, project, "--json") + assert result.returncode == 1, result.stdout + result.stderr + payload = json.loads(result.stdout) + assert payload["verdict"] == "INVALID (pins, manifest-kinds)", payload + rows = rows_of(result) + assert "pin-wrong-kind" in rows["pins"]["reason"], rows["pins"] + row = rows["manifest-kinds"] + assert row["next"] == removal_command(payload, "log -p -1", + [SPEC_PIN]), row + assert row["why"] == misdeclared_why(SPEC_PIN, MISSPELT_KIND, "pins", + True), row["why"] + shows_the_kind(standard, project, row, MISSPELT_KIND) + + +def test_a_declared_neutral_pin_of_the_wrong_kind_is_this_rows_to_say( + standard, project): + """A neutral product pin `project.yaml` declares, given a kind nothing + registers: like the shape pin's, no gate checks its kind, so this row + is the one that says so, and its NEXT shows the commit. + + In process, for the reason `test_a_pin_is_what_the_root_declares_not_ + what_it_is_called` gives: a declared neutral pin sends the `pins` row + to `gh`.""" + declare_a_neutral_pin(project) + pin = project / NEUTRAL_PIN + pin.write_bytes(f"kind: {MISSPELT_KIND}\n".encode() + pin.read_bytes()) + commit_all(project, "The neutral pin declares a kind nothing registers") + module = doctor_module(standard) + row = module.check_manifest_kinds(module.Context(project)).as_dict() + assert_next_holds_no_prose(row) + assert row["detail"]["unregistered"] == [ + f"{NEUTRAL_PIN} ({MISSPELT_KIND})"], row["detail"] + assert row["next"] == (f"git -C {module.quote_arg(project)} log -p -1 " + f"-- {NEUTRAL_PIN}"), row + assert row["why"] == misdeclared_why(NEUTRAL_PIN, MISSPELT_KIND, None, + True), row["why"] + shows_the_kind(standard, project, row, MISSPELT_KIND) + + # --- the shape pin and a copy conflicted together (#485) -------------------- # # Review of PR #358 (probes OD and OD2), filed as #485. Two branches that