diff --git a/scripts/check-chart-freeze.py b/scripts/check-chart-freeze.py index 5d30d206fa..16c2ad336f 100755 --- a/scripts/check-chart-freeze.py +++ b/scripts/check-chart-freeze.py @@ -11,7 +11,10 @@ MeshWeaver#7127) and may do nothing else to them. A file the diff adds, modifies, retypes, copies or renames INTO a frozen path is refused, naming the file and where the chart lives now. A frozen DIRECTORY matches its own path as well as everything below it, so replacing it with a symlink (its -files deleted, the directory path added) is refused too. +files deleted, the directory path added) is refused too — and so is a non-deletion AT any directory +ABOVE a frozen path (MeshWeaver#7391), which would replace the whole frozen subtree the same way. +A saved listing is read strictly: the whole status token, the number of path fields it needs and +their non-emptiness are validated before a record counts. The build-host deployment followed (MeshWeaver.Deploy#5): its runner `.github/scripts/build-run.py`, `deploy/build-host/**`, the announcement script, the declaration `.github/build-hosts.json` and the @@ -40,6 +43,7 @@ from __future__ import annotations import argparse +import re import subprocess import sys @@ -82,8 +86,21 @@ "deploy/aks/INSTANCE-LIFECYCLE.md", ) -# The status letters `git diff --name-status` prints (C and R do not occur with --no-renames). -KNOWN_STATUS = set("ADMTUXBCR") +# The status TOKENS `git diff --name-status` prints (C and R do not occur with --no-renames). The +# WHOLE token is validated, not its first letter (MeshWeaver#7391): `Dgarbage` once read as a +# deletion and passed. A copy or rename carries a similarity score (`R100`); a modification may +# carry a dissimilarity score under `-B`. No other status is scored — a scored deletion (`D100`) +# is not something git prints, and reading it as a deletion would exempt it — and a score is a +# percentage, so it never exceeds 100. +STATUS_RE = re.compile(r"(?:[ADTUXB]|M(?P\d{1,3})?|[CR](?P\d{1,3}))") + + +def _valid_status(status: str) -> bool: + m = STATUS_RE.fullmatch(status) + if not m: + return False + score = m.group("m") or m.group("cr") + return score is None or int(score) <= 100 def _matches(path: str, prefixes: tuple[str, ...]) -> bool: @@ -97,11 +114,22 @@ def is_frozen(path: str) -> bool: return _matches(path, FROZEN) +def is_frozen_ancestor(path: str) -> bool: + """`path` is a directory ABOVE a frozen path (MeshWeaver#7391). A name-status listing names + files, so a non-deletion AT a directory's own path is a file or symlink replacing that whole + directory: moving `deploy/aks` aside and adding the symlink `deploy/aks -> ../editable-aks` + lists the frozen children as deletions and an addition at `deploy/aks`, which no frozen prefix + matches — and re-opens every frozen path below it for editing through the link.""" + prefix = path.rstrip("/") + "/" + return bool(path) and any(f.startswith(prefix) for f in FROZEN) + + def violations(listing: str) -> list[tuple[str, str]]: """Every (status, path) in a NUL-delimited name-status listing that touches a frozen path with anything but a deletion. The listing is a flat sequence of fields: status, path, status, path, … - A field this gate cannot read — an unknown status, or a status with no path after it — is itself - a violation: a change it could not read is a change it did not check.""" + A field this gate cannot read — an unknown or malformed status token, a status with fewer path + fields than it needs, or an empty path — is itself a violation: a change it could not read is a + change it did not check.""" out: list[tuple[str, str]] = [] fields = listing.split("\0") if fields and fields[-1] == "": @@ -109,15 +137,17 @@ def violations(listing: str) -> list[tuple[str, str]]: i = 0 while i < len(fields): status = fields[i] - if not status or status[0] not in KNOWN_STATUS or i + 1 >= len(fields): + # A C or R entry carries two paths; --no-renames should never produce one, but read both. + width = 3 if status[:1] in ("C", "R") else 2 + paths = fields[i + 1:i + width] + if (not _valid_status(status) or len(paths) != width - 1 + or any(not p for p in paths)): # Past an unreadable field the status/path pairing is lost, so nothing after it can be # read either: one violation names where reading stopped. out.append(("?", status)) break - # A C or R entry carries two paths; --no-renames should never produce one, but read both. - width = 3 if status[0] in "CR" else 2 - for path in fields[i + 1:i + width]: - if is_frozen(path) and status[0] != "D": + for path in paths: + if status[0] != "D" and (is_frozen(path) or is_frozen_ancestor(path)): out.append((status[0], path)) i += width return out @@ -201,6 +231,28 @@ def self_test() -> int: "M\0", 1), ("an empty listing is no change", "", 0), + # #7391: the ancestor replacement and the strict record reader. + ("a frozen path's ANCESTOR replaced by a symlink is refused (deploy/aks -> elsewhere)", + _z("D\tdeploy/aks/values.aks.yaml\nD\tdeploy/aks/manifests/x.yaml\nA\tdeploy/aks\n" + "A\teditable-aks/values.aks.yaml"), 1), + ("…at any depth (a file replacing .github/scripts)", + _z("A\t.github/scripts"), 1), + ("…but deleting at an ancestor path is allowed, and a sibling of it is not an ancestor", + _z("D\tdeploy/aks\nM\tdeploy/aksx\nM\tdeploy/aks-notes.md"), 0), + ("a rename with a MISSING destination is a violation, never a silent pass", + "R100\0docs/x.yaml\0", 1), + ("a status token with trailing garbage is not a deletion", + "Dgarbage\0deploy/helm/values.yaml\0", 1), + ("an EMPTY path field is a violation", + "M\0\0", 1), + ("a scored modification (-B) still reads", + "M073\0deploy/helm/values.yaml\0", 1), + ("a SCORED deletion is not a deletion (git never scores one)", + "D100\0deploy/helm/values.yaml\0", 1), + ("a score above 100 is malformed, never a silent pass", + "R101\0docs/x.yaml\0docs/y.yaml\0", 1), + ("…and a rename scored at the 100 bound between unfrozen paths stays clean", + "R100\0docs/x.yaml\0docs/y.yaml\0", 0), ] failed = 0 for name, text, expected in cases: diff --git a/scripts/check-cross-repo-pair.py b/scripts/check-cross-repo-pair.py index 4fe6a6baa7..d3fc04de5c 100755 --- a/scripts/check-cross-repo-pair.py +++ b/scripts/check-cross-repo-pair.py @@ -39,8 +39,11 @@ It consumes the surface-removal REPORT that `check-type-forwards.py --surface-json` already computes — one detector, proven by one set of self-tests, never a second one — and: - * removals empty -> PASS. Measured on this repo: `main~25 -> main` removes ZERO public types, - so an ordinary pull request never meets this gate at all. `main~100 -> main` removes 116, all + * removals empty -> PASS, UNLESS the body DECLARES a counterpart (MeshWeaver#7397): a + declared `Pairs-with:` is resolved whatever the detector found, because the author is asserting + a coupling the detector cannot see — a PackageReference or PackageVersion leaving core. Measured + on this repo: `main~25 -> main` removes ZERO public types, so an ordinary pull request (which + declares nothing) never meets this gate at all. `main~100 -> main` removes 116, all of them the Maps/Indexing carve-out (#2941) — the exact wave that produced #2689's incident shapes 4 and 5. * removals non-empty -> the pull-request body MUST carry a `Pairs-with:` declaration, and every @@ -445,10 +448,21 @@ def evaluate(surface: dict, body: str, fleet: set[str], resolve, f"Base tree holds {projects_at_base} `{PROJECT_SUFFIX}` file(s); " f"this diff deletes or renames {len(removed_projects)} of them." ) + declarations = parse_declarations(body) if not removed and not removed_projects: - out.append("No public type or member left src/ and no project path moved in this diff — " - "no cross-repo pair to gate.") - return 0, out + # 🚨 A DECLARED pair is resolved even when the detector found nothing (MeshWeaver#7397). + # The detector sees public types, public members and project paths — not a + # PackageReference or PackageVersion leaving core, which a satellite can depend on just as + # hard (#7205 dropped YamlDotNet; #7179 dropped pins a satellite read versionless). An + # author who writes `Pairs-with:` is asserting a coupling the detector cannot see, and a + # gate that returned before reading it held nothing while the body claimed it did. + if not any(isinstance(d, (PairRef, Unparseable)) for d in declarations): + out.append("No public type or member left src/ and no project path moved in this " + "diff — no cross-repo pair to gate.") + return 0, out + out.append("No public type or member left src/ and no project path moved, but the body " + "DECLARES a counterpart — resolving it, because the declaration is the author " + "asserting a coupling the detector cannot see (a dependency or pin removal).") for entry in removed[:40]: out.append(f" [{entry.get('category')}] {entry.get('assembly')} :: {entry.get('fullName')}") @@ -468,7 +482,6 @@ def evaluate(surface: dict, body: str, fleet: set[str], resolve, f"a counterpart may live in: {', '.join(sorted(fleet))}." ) - declarations = parse_declarations(body) bad = [d for d in declarations if isinstance(d, Unparseable)] if bad: for d in bad: @@ -912,6 +925,37 @@ def resolve(repo: str, number: int) -> dict: "", _resolver(_pr()), {"projectsAtBase": 2, "removed": []}, _FLEET, False, ("below the floor", "green on zero"), ), + ( + # 🚨 THE #7397 SHAPE. #7205 dropped a package dependency from core (no public type, no + # project path) and DECLARED its satellite counterpart; before the fix this row PASSED + # with "no cross-repo pair to gate" while the counterpart was still open. + "a DECLARED counterpart is resolved though the detector found no removal — OPEN holds it", + {"publicTypesAtBase": _HEALTHY, "removed": []}, + "Drops a package dependency.\n\nPairs-with: Systemorph/MeshWeaver.Plugins#4227\n", + _resolver(_pr(merged=False, state="open")), _NO_PROJECTS, _FLEET, False, + ("DECLARES a counterpart", "still OPEN", "must land LAST"), + ), + ( + "…and once that declared counterpart has merged, the change may follow", + {"publicTypesAtBase": _HEALTHY, "removed": []}, + "Pairs-with: Systemorph/MeshWeaver.Plugins#4227\n", + _resolver(_pr()), _NO_PROJECTS, _FLEET, True, + ("ok Systemorph/MeshWeaver.Plugins#4227", "may follow"), + ), + ( + "…a malformed declaration on a no-removal diff FAILS rather than reading as none", + {"publicTypesAtBase": _HEALTHY, "removed": []}, + "Pairs-with: MeshWeaver.Plugins 4227\n", + _resolver(_pr()), _NO_PROJECTS, _FLEET, False, + ("Unparseable declaration",), + ), + ( + "…and `Pairs-with: none` on a no-removal diff still passes", + {"publicTypesAtBase": _HEALTHY, "removed": []}, + "Pairs-with: none — nothing outside core consumed the dropped package\n", + _raising_resolver("must not be called"), _NO_PROJECTS, _FLEET, True, + ("no cross-repo pair to gate",), + ), ( "an ordinary diff that moves no project still never meets the gate", {"publicTypesAtBase": _HEALTHY, "removed": []}, diff --git a/scripts/check-record-signatures.py b/scripts/check-record-signatures.py index ac8133a186..c004a1872d 100644 --- a/scripts/check-record-signatures.py +++ b/scripts/check-record-signatures.py @@ -184,9 +184,120 @@ def _strip_bom(text: str) -> str: return text.lstrip("\ufeff") +# 🚨 A COMMENT is not a parameter (MeshWeaver#7366). `split_params` splits on top-level commas +# and `parse_param` takes the last token as the name, so a `//` line between two parameters +# whose prose holds a comma became a PHANTOM parameter, and a comment ending in an identifier +# was swallowed into the next parameter's TYPE. Measured on core#5077: `UserOnboardingRequest` +# read as 8 -> 10 parameters for a real 7 -> 8, with five lines of prose printed as the type of +# `Locale`. Rewording such a comment then read as a type change on a record whose signature did +# not move — and, worse, a comment GAINING a comma while a real parameter left kept the count +# equal. So comments go before anything is counted, on BOTH sides of the comparison, because both +# pass through here. +# +# String and char literals are copied verbatim (a `//` inside `"https://…"` is not a comment); +# `@"…"` verbatim strings use `""` as their only escape, and a raw literal opened by N >= 3 +# quotes runs to the next N quotes (a four-quote literal may hold `"""`). An interpolated `$"…"` +# string's `{…}` holes are scanned as CODE, so a nested literal such as +# `$"{string.Concat("/*", "x")}"` neither ends the outer string nor opens a comment. A literal +# the scanner mis-bounds is not harmless: text it leaves outside a string can open a phantom +# `/*` that swallows every record after it, and an arity change there would pass. Newlines inside +# a block comment are kept so nothing downstream sees two tokens fused into one. +def _skip_hole(text: str, j: int) -> int: + """`j` is just past an interpolation hole's `{`; returns the index just past its `}`.""" + n, depth = len(text), 0 + while j < n: + c = text[j] + if c in "\"'": + j = _skip_literal(text, j) + continue + if c in "{([": + depth += 1 + elif c in "})]": + if depth == 0 and c == "}": + return j + 1 + depth -= 1 + j += 1 + return n + + +def _skip_literal(text: str, i: int) -> int: + """`text[i]` opens a string or char literal; returns the index just past its end.""" + n = len(text) + if text[i] == "'": + j = i + 1 + while j < n: + c = text[j] + if c == "\\": + j += 2 + continue + if c in "'\n": + return j + 1 + j += 1 + return n + p = i + while p > 0 and text[p - 1] in "$@": + p -= 1 + prefix = text[p:i] + interpolated, verbatim = "$" in prefix, "@" in prefix + run = 0 + while i + run < n and text[i + run] == '"': + run += 1 + if run >= 3 and not verbatim: + close = text.find('"' * run, i + run) + return n if close == -1 else close + run + j = i + 1 + while j < n: + c = text[j] + if verbatim: + if c == '"': + if j + 1 < n and text[j + 1] == '"': + j += 2 + continue + return j + 1 + else: + if c == "\\": + j += 2 + continue + if c in '"\n': + return j + 1 + if interpolated and c == "{": + if j + 1 < n and text[j + 1] == "{": + j += 2 + continue + j = _skip_hole(text, j + 1) + continue + j += 1 + return n + + +def _strip_comments(text: str) -> str: + out: list[str] = [] + i, n = 0, len(text) + while i < n: + ch = text[i] + if ch == "/" and i + 1 < n and text[i + 1] == "/": + end = text.find("\n", i) + i = n if end == -1 else end + continue + if ch == "/" and i + 1 < n and text[i + 1] == "*": + end = text.find("*/", i + 2) + stop = n if end == -1 else end + 2 + out.append(" " + "\n" * text.count("\n", i, stop)) + i = stop + continue + if ch in "\"'": + stop = _skip_literal(text, i) + out.append(text[i:stop]) + i = stop + continue + out.append(ch) + i += 1 + return "".join(out) + + def records_in(source: str) -> dict[str, list[Param]]: """Every public record's primary-constructor parameter list, by record name.""" - source = _strip_bom(source) + source = _strip_comments(_strip_bom(source)) found: dict[str, list[Param]] = {} for m in RECORD_RE.finditer(source): open_idx = source.index("(", m.end() - 1) @@ -376,6 +487,55 @@ def check(label: str, cond: bool) -> None: check("an attribute is stripped, not counted", len(a) == 2 and a[0].name == "X") check("a params modifier is stripped", a[1].name == "Rest") + # ── comments inside a parameter list are not parameters (#7366) ───────────────────── + # The core#5077 shape: line comments between parameters, holding commas, an em dash, a + # trailing identifier and a `//` inside a string default that must SURVIVE. + commented = ( + "public sealed record Onboarding(\n" + " string Username,\n" + " string Email,\n" + " // The viewer's language (\"de\"), from the onboarding form's language picker —\n" + " // which itself defaults to the browser's, so the point of use reads it anyway.\n" + " string? Locale = null,\n" + " /* a block comment, with a comma,\n" + " spanning lines */ string Home = \"https://example.com/a,b\",\n" + " // not learning it, and is pinned to no learning path.\n" + " bool IsPlatformBootstrap = false);\n" + ) + oc = records_in(commented).get("Onboarding") or [] + check("comment text is not counted as parameters (#7366: real arity is 5)", + [p.name for p in oc] == ["Username", "Email", "Locale", "Home", "IsPlatformBootstrap"]) + check("…and a comment is not swallowed into the next parameter's TYPE", + [p.type for p in oc] == ["string", "string", "string?", "string", "bool"]) + reworded = commented.replace("so the point of use reads it anyway", "so callers, again, read it") + check("rewording a comment inside a parameter list is NOT a signature change", + not compare("Onboarding", oc, records_in(reworded).get("Onboarding") or [], "x.cs")) + dropped = commented.replace(" string Email,\n", " // Email moved, see the profile record,\n") + check("a real parameter leaving while a comment gains a comma is still an arity change", + [x.kind for x in compare("Onboarding", oc, + records_in(dropped).get("Onboarding") or [], "x.cs")] == ["arity"]) + check("a `//` inside a string literal is not a comment", + records_in('public record U(string Url = "http://x", int N = 1);').get("U") is not None + and [p.name for p in records_in('public record U(string Url = "http://x", int N = 1);')["U"]] + == ["Url", "N"]) + check("a commented-OUT record is not a record", + "Gone" not in records_in("// public record Gone(int A);\npublic record Here(int A);")) + # A mis-bounded literal leaves text outside any string, where a `/*` becomes a phantom + # comment that swallows every record after it — and an arity change there would pass. + four_quote = 'var s = """"\nhas """ /* inside\n"""";\npublic record After(int A, int B);\n' + check("a four-quote raw literal holding three quotes does not hide the record after it", + [p.name for p in records_in(four_quote).get("After") or []] == ["A", "B"]) + nested = ('var s = $"{string.Concat("/*", "x")}";\npublic record After(int A, int B);\n' + 'var t = "*/";\n') + check("a literal nested in an interpolation hole does not open a comment", + [p.name for p in records_in(nested).get("After") or []] == ["A", "B"]) + check("…and `{{` in an interpolated string is an escaped brace, not a hole", + [p.name for p in records_in('var s = $"{{ /* }}";\npublic record After(int A);\n') + .get("After") or []] == ["A"]) + check("a verbatim string opening with an escaped quote is not a raw literal", + [p.name for p in records_in('var s = @"""x"" /*";\npublic record After(int A);\n') + .get("After") or []] == ["A"]) + # ── the transitional allowance, end to end through run() ───────────────────────── # # 🚨 These call run() on a real repository rather than the classifier in isolation. A guard diff --git a/src/MeshWeaver.Documentation/Data/Architecture/ChartOwnershipAndRunnerPool.md b/src/MeshWeaver.Documentation/Data/Architecture/ChartOwnershipAndRunnerPool.md index da461f5e55..dea89a3a8e 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/ChartOwnershipAndRunnerPool.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/ChartOwnershipAndRunnerPool.md @@ -36,6 +36,17 @@ followed it (MeshWeaver.Deploy#5) and is frozen the same way: `deploy/build-host A chart change belongs in MeshWeaver.Deploy. The gate's message says so, and its self-test proves it refuses a modification and an addition and lets a pure deletion through. +Two shapes no well-meant edit reaches are refused as well (#7391). A non-deletion **at a directory +above a frozen path** — move `deploy/aks` aside and add the symlink `deploy/aks -> ../editable-aks` +— lists every frozen child as a deletion and one addition that no frozen prefix matches, yet +re-opens the whole subtree for editing through the link; so an addition, modification or retype at +any ancestor of a frozen path is a violation, while deleting there stays allowed. And a saved +`--diff-file` listing is read **strictly**: the whole status token must be one git prints (`Dgarbage` +used to read as a deletion). Only `C`, `R` and `M` carry a score, and a score never exceeds 100, so a +scored deletion such as `D100` is refused rather than exempted. And a record must carry the number of path fields its status needs, none +empty (`R100` with no destination used to count as nothing). An unreadable record stops the read and +is itself a violation. + ## The chart lives here; the application it deploys does not `deploy/helm/` is in the platform repo, so `Chart Gate` and `Chart Drift` correctly run here — **a diff --git a/src/MeshWeaver.Documentation/Data/Architecture/CrossRepoPairGate.md b/src/MeshWeaver.Documentation/Data/Architecture/CrossRepoPairGate.md index 0f07713baf..fca479c71f 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/CrossRepoPairGate.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/CrossRepoPairGate.md @@ -125,6 +125,18 @@ line.** A typo'd declaration that read as "no declaration" would put the author disagreement about whether a pair was declared, which is the same trapdoor as a gate that skips on a missing input. +🚨 **A declaration is resolved even when the set is EMPTY** (#7397). The detector sees public +types, public members and project paths — never a `PackageReference` or `PackageVersion` leaving +core, which a satellite can depend on just as hard: #7205 dropped a package dependency a satellite +still needed, and #7179 dropped pins a satellite read versionless. Both declared their counterpart, +and before #7397 the gate returned "no cross-repo pair to gate" BEFORE reading the body, so the +declaration held nothing while the body said it did. Now a `Pairs-with:` naming a pull request is +resolved under the verdict below whatever the detector found, a malformed one fails, and +`Pairs-with: none — …` (or no declaration) on an empty set still passes. An UNDECLARED +dependency-only removal is still not seen — `Satellite package pins (removal declared)` asks for +the `Satellite-pins:` statement there, and that statement orders nothing; the `Pairs-with:` line +is what orders it. + ### 3 · The verdict Each declared counterpart is resolved through the GitHub API and must be **merged into its @@ -822,6 +834,42 @@ case; and a `RECORD_RE` that stops matching entirely now reddens a **canary plac previously it raised `KeyError` from a later case and buried the verdict in a traceback, which is harder to read than the defect it found. +##### Comments are not parameters (#7366) + +`records_in()` slices a primary constructor's parameter list and `split_params()` splits it on +top-level commas, taking the last token of each piece as the parameter's name. Neither removed +comments, so a `//` line between two parameters whose prose holds a comma became a phantom +parameter, and a comment ending in an identifier was swallowed into the next parameter's TYPE. +Measured on core#5077: a record with 7 → 8 real parameters read as 8 → 10, with five lines of prose +printed as one parameter's type. Two consequences follow, and the second is why it is worth fixing: +rewording such a comment read as a type change on a record whose signature did not move, and a +comment gaining a comma while a real parameter left kept the count equal — a real break hidden. + +Comments are now stripped from the source before anything is counted, on both sides of the +comparison (both pass through `records_in()`), with string and char literals copied verbatim so a +`//` inside `"https://…"` survives. A census over `src/` when the fix landed: 837 public records, ONE +parsed differently — `NodeCompilationResult` (`IMeshNodeCompilationService.cs`) read as 10 +parameters for its real 9 — so the defect was live, not latent. The self-test carries the #5077 +shape, a reworded comment, a comment gaining a comma while a parameter leaves, and a `//` inside a +string default; removing the strip reddens three of them. + +The strip is only as good as its literal scanner. If it mis-bounds a literal, the text it leaves +outside a string can open a phantom `/*` that swallows every record after it, so an arity change +there passes. Two literal shapes are therefore scanned in full: + +- A raw literal opened by N ≥ 3 quotes closes only at N quotes, so a four-quote literal may hold + `"""`. +- An interpolated string's `{…}` holes are scanned as code, so `$"{string.Concat("/*", "x")}"` + neither ends the outer string nor opens a comment. + +The self-test carries both shapes with a record after them, and both fail on a scanner that +ignores them. `check-type-forwards.py` reads positional +record parameters too (`_positional_parameters`), for their NAMES only, and shares the blindness: +measured on the same record it indexes two phantom members, `Structured` and `table`, the last +words before a comma in its comments. It was not changed here, because it is a subject of the +parser-delta control and a change to its counts needs its own declared pull request; it is filed +for triage separately. + ## Member-level detection (the sixth shape) `check-type-forwards.py` indexes, under each public top-level type, the **names** of its public