From 612e3cf5b4cb9492136173dd7ef1bb3779d916da Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 00:50:01 +0200 Subject: [PATCH 1/2] ci: reject raw bidi control characters in first-party files (fixes #628) A bidirectional control renders as nothing and occupies no column, so a diff containing one looks exactly like a diff that does not. In a string literal that is a silently wrong assertion (#610: nine lines of tst_i18n.qml whose expected values held raw U+200E / U+200F / U+061C). In a comment it is source that stops saying what it means (#628 reproduced that on itself, twice, in two files, typing an escape through a JSON-payload tool that decoded it before the file was written). Between them is the trojan-source shape, where an override or isolate makes a line render as a different program from the one that compiles. #610 asked for this lint and deferred it as "a bigger decision". Review is not a control for this class -- the PR that closed #610 was reviewed by people who could not have seen a raw control had one survived -- so a gate is the only thing that can hold the line. scripts/check_bidi_controls.py rejects twelve codepoints in three classes, the list #610 wrote: U+061C, U+200E/U+200F, U+202A-U+202E and U+2066-U+2069. Comments are in scope, not excluded: #628's own reproduction was inside comments, and trojan source lives in comments by construction. The tree is already clean -- 0 raw controls across 1242 tracked files -- so the gate ships green and would be worth nothing unless every way of going blind is closed: - The detector is probed against each declared codepoint, one at a time, before any file is opened. A detect() narrowed to the two marks this repository has actually hit fails on a clean tree. - Scanning zero files is an error, not a pass. - scripts/test_check_bidi_controls.sh holds its own independent list of the twelve and requires each to be found in tests/lint/bidi_controls/. That is what notices a codepoint being deleted from the checker's table, which the checker's in-process probe cannot see, because both sides read it. - The diagnostic replaces each control with a visible marker, and the self-test asserts no diagnostic contains a raw control: a gate against invisible characters that prints them reports the defect by committing it. The gate caught its own author before this branch was pushed. The first draft of the workflow comment below said the six characters of a U+061C escape; the tool payload decoded them, a single invisible U+061C landed in drift-guard.yml, and nothing but this scan would have reported it. That is #628's reproduction a fourth time, on the PR that closes it. EXEMPT maps a path to a written reason and is empty, which is the measured state of the tree rather than an aspiration. Both hygiene rules from check_workflow_option_coverage.py's EXEMPT and check_workflow_job_banners.py's UNBANNERED apply and are pinned by the self-test: an entry for a file that does not exist is an error, and an entry for a file with no raw controls is an error. Rides drift-guard.yml's prose-lint job, where #635 put the NOLINT-directive scan for the same reason: a fast, dependency-free text scan that compiles nothing, guarding the neighbouring class of defect -- a file that looks correct to every reader while saying something else. Also closes #642, whose five raw U+200E/U+200F do not exist in the merged tree: `git ls-files` lists 1242 files at c55ea5b7 and none of them contains any of the twelve codepoints. See the PR body for the measurement. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- .github/workflows/drift-guard.yml | 49 ++- scripts/check_bidi_controls.py | 357 ++++++++++++++++++ scripts/test_check_bidi_controls.sh | 254 +++++++++++++ .../arabic_letter_mark/alm_in_a_comment.cpp | 9 + .../directional_marks/marks_in_literals.qml | 11 + .../embeddings_and_overrides/reordering.cpp | 10 + .../invalid/isolates/isolates.hpp | 11 + .../bidi_controls/valid/escaped_literals.cpp | 16 + .../valid/no_controls_at_all.qml | 11 + .../valid/prose_naming_the_codepoints.hpp | 14 + 10 files changed, 740 insertions(+), 2 deletions(-) create mode 100755 scripts/check_bidi_controls.py create mode 100755 scripts/test_check_bidi_controls.sh create mode 100644 tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp create mode 100644 tests/lint/bidi_controls/invalid/directional_marks/marks_in_literals.qml create mode 100644 tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp create mode 100644 tests/lint/bidi_controls/invalid/isolates/isolates.hpp create mode 100644 tests/lint/bidi_controls/valid/escaped_literals.cpp create mode 100644 tests/lint/bidi_controls/valid/no_controls_at_all.qml create mode 100644 tests/lint/bidi_controls/valid/prose_naming_the_codepoints.hpp diff --git a/.github/workflows/drift-guard.yml b/.github/workflows/drift-guard.yml index 71ef6405..7a7b3987 100644 --- a/.github/workflows/drift-guard.yml +++ b/.github/workflows/drift-guard.yml @@ -8,8 +8,14 @@ name: Drift guard # which is the same shape of defect one level down: a control that parses, looks # applied, and measures nothing. # -# Sections below: the spec <-> code gates and the NOLINT-directive gate, the -# ladder rung-list gate, then the scenario-coverage gate. +# And one for the class below that again: a character that is not there to be +# read at all (the raw-bidi-control scan, #628). Same family -- a file that +# looks correct to every reader while saying something else -- which is why it +# rides prose-lint alongside the NOLINT scan rather than living elsewhere. +# +# Sections below: the spec <-> code gates, the NOLINT-directive gate and the +# raw-bidi-control gate, the ladder rung-list gate, then the scenario-coverage +# gate. # # Two independent gates for the "spec <-> code drift" class of bug (a # docs/spec/*.md file stating a mechanical fact -- an enum cardinality, a @@ -202,6 +208,45 @@ jobs: - name: Check every NOLINTNEXTLINE annotates a line of code run: bash scripts/check_nolint_directives.sh + # ── Raw bidi control characters in first-party files (#628) ─────────── + # A bidirectional control renders as nothing and occupies no column, so a + # diff containing one looks exactly like a diff that does not. In a + # string literal that is a silently wrong assertion (#610's subject: nine + # lines of src/qt/forms/tests/tst_i18n.qml whose expected values held raw + # U+200E / U+200F / U+061C). In a comment it is source that stops saying + # what it means -- #628 reproduced that on itself, typing the six + # characters of a U+061C escape through a JSON-payload tool that + # decoded it before the file was written, twice, in two files. This + # very comment made it a fourth time, and the gate below caught it + # before the branch was pushed. Between them lies the trojan-source + # shape, where an override or isolate makes a line render as a different + # program from the one that compiles. + # + # Review is not a control for this class, and that is the argument for a + # gate rather than a habit: #610 itself asked for this lint and deferred + # it, and the PR that closed #610 was reviewed by people who could not + # have seen a raw control had one survived, because the diff renders them + # as nothing. + # + # A fast, dependency-free text scan over every tracked file that compiles + # nothing, so it rides this job for the same reason the NOLINT-directive, + # CI-clang-pin and Catch2-name scans above do. + # + # This one ships already green -- 0 raw controls across 1242 tracked + # files -- which makes the self-test the whole of its evidence rather + # than a formality. "0 occurrences found" is exactly what a broken + # detector prints, so the checker probes its own detector against every + # declared codepoint before opening a file, treats a scan of zero files + # as a failure, and the self-test below holds its own independent list of + # the twelve and requires each to be found in tests/lint/bidi_controls/ -- + # which is what notices a codepoint being deleted from the checker's + # table, something the checker's own probe cannot see. + - name: Self-test the raw-bidi-control checker + run: bash scripts/test_check_bidi_controls.sh + + - name: Check no first-party file carries a raw bidi control + run: python3 scripts/check_bidi_controls.py + # ── Ladder rung list <-> the CI filters that are supposed to track it ── # examples/rungs.txt is the ladder's single authoritative rung list, and # almost every consumer now derives from it at run time. Three cannot -- diff --git a/scripts/check_bidi_controls.py b/scripts/check_bidi_controls.py new file mode 100755 index 00000000..cdcba49b --- /dev/null +++ b/scripts/check_bidi_controls.py @@ -0,0 +1,357 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +"""Usage: python3 scripts/check_bidi_controls.py [PATH...] + +Fails if any first-party file carries a *raw* Unicode bidirectional control +character. The escaped spelling (`\\u200E`, `\\u{200E}`, `‎` -- whatever +the file's own language provides) is always accepted; only the literal +codepoint is rejected. + +With no PATH, scans every tracked file in the repository (`git ls-files`), +minus this gate's own fixtures. With PATHs, walks them on disk -- which is how +scripts/test_check_bidi_controls.sh reaches the fixtures, and how a caller +scopes the scan to one subtree. + +## Why this gate exists (morph#628, morph#642, morph#610) + +A bidi control is invisible. It renders as nothing, occupies no column, and +survives copy-paste, so a diff containing one looks exactly like a diff not +containing one. Every consequence of that is silent: + + - In a *string literal* it is a silently wrong test. morph#610's subject was + nine assertion lines in src/qt/forms/tests/tst_i18n.qml whose expected + values contained raw U+200E / U+200F / U+061C; a wrong one there asserts + something nobody can read. + - In a *comment* it is source that stops saying what it means. morph#628 + reproduced exactly this: typing the six characters `\\u061C` through a + JSON-payload tool decoded the escape before the file was written, so a + single invisible U+061C landed in include/morph/render/locale_format.hpp + and in tests/test_render_locale_format.cpp. + - Between the two lies the "trojan source" shape: an override or isolate + that reorders how a line *renders* relative to how it *compiles*, so the + reviewer and the compiler read different programs. + +Comments are therefore in scope, not excluded. morph#610's original sketch said +"outside comments"; morph#628's own reproduction was inside comments, and the +trojan-source shape lives in comments by construction. + +Review is not a control for this class. The PR that closed morph#610 -- the +ticket about this exact hazard -- was itself reviewed, and the review could not +have seen a raw control had one survived, because the diff renders them as +nothing. That is the argument for a lint rather than a habit. + +## What is rejected + +Twelve codepoints, in three classes, which is morph#610's list: + + U+061C ARABIC LETTER MARK + U+200E, U+200F LEFT-TO-RIGHT / RIGHT-TO-LEFT MARK + U+202A .. U+202E the embeddings, overrides and their terminator + U+2066 .. U+2069 the isolates and their terminator + +The marks are the ones this repository has actually hit; the embeddings, +overrides and isolates are the sharper hazard, because those are the ones that +reorder rendered text rather than merely nudging a neighbouring run. + +## The two vacuity traps, and how each is closed + +A scan that reports "0 raw controls" is satisfied by a broken pattern exactly +as well as by a clean tree, and the tree *is* clean today (0 occurrences across +1242 tracked files), so this gate ships already green. It would therefore be +worth nothing unless both ways of going blind are closed: + + 1. **The detector drifts away from the declared set.** Before scanning + anything, the checker feeds itself a probe holding each declared + codepoint, one at a time, and requires every one to come back flagged. A + `detect()` rewritten as a range with the wrong bound, or narrowed to the + two marks this repository happens to have hit, fails here on every run, + with no fixture involved and on a clean tree. + 2. **The file set stops matching.** Scanning zero files is an error, not a + pass -- that is what a wrong path argument, a `git ls-files` that failed, + or a prune rule that swallowed the tree all look like from outside. + +What that probe cannot see is the declared set itself *shrinking*: delete an +entry from BIDI_CONTROLS and the probe stops asking about it, because both +sides derive from the same table. Closing that is the job of +scripts/test_check_bidi_controls.sh, which holds its own independent list of +the twelve, drives the checker over the fixtures in tests/lint/bidi_controls/, +and requires every codepoint on *its* list to be named in the output. It also +pins that a directory with no scannable files is rejected rather than called +clean, that each diagnostic names its own file, that no diagnostic contains a +raw control, and that both EXEMPT hygiene rules fire. + +## The diagnostic must be readable + +A gate against invisible characters must not print them. Every offending line +is echoed with each control replaced by a visible `` marker, so the +failure message says where the character is instead of reproducing the exact +problem it is reporting. The self-test pins this: the diagnostic itself must +contain no raw control. + +## Exemptions + +EXEMPT below maps a repository-relative path to a written reason. The reason is +the payload: an exemption without one is the hand-maintained record this gate +replaces. + +Two rules keep it from becoming stale, copied from +scripts/check_workflow_option_coverage.py's EXEMPT and +scripts/check_workflow_job_banners.py's UNBANNERED: an entry for a file that +does not exist is an error, and an entry for a file that has no raw controls is +an error. An exemption has to be necessary to be allowed to stay. + +The grain is the file, not the line. That is deliberate: a file that genuinely +needs a raw control -- one proving a parser accepts a literal, say -- is a file +whose whole subject is that character, and an exemption for it should be +argued once in prose rather than sprinkled per line where it would drift back +into being invisible. + +Reads text only; compiles and runs nothing. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +# Files allowed to carry raw bidi controls, and why. +# +# Empty, and that is the measured state of the tree rather than an aspiration: +# `git ls-files` lists 1242 files at c55ea5b7 and none of them contains any of +# the twelve codepoints below. Adding an entry needs an argument a reviewer can +# check; removing one is always allowed. +EXEMPT: dict[str, str] = {} + +# The twelve rejected codepoints, written out one by one. The detector below is +# built from this list, and so is the probe that proves the detector works -- +# but the probe tests each codepoint *individually*, so a mistake that narrows +# the set is caught even though both derive from here. +BIDI_CONTROLS: dict[int, str] = { + 0x061C: "ARABIC LETTER MARK", + 0x200E: "LEFT-TO-RIGHT MARK", + 0x200F: "RIGHT-TO-LEFT MARK", + 0x202A: "LEFT-TO-RIGHT EMBEDDING", + 0x202B: "RIGHT-TO-LEFT EMBEDDING", + 0x202C: "POP DIRECTIONAL FORMATTING", + 0x202D: "LEFT-TO-RIGHT OVERRIDE", + 0x202E: "RIGHT-TO-LEFT OVERRIDE", + 0x2066: "LEFT-TO-RIGHT ISOLATE", + 0x2067: "RIGHT-TO-LEFT ISOLATE", + 0x2068: "FIRST STRONG ISOLATE", + 0x2069: "POP DIRECTIONAL ISOLATE", +} + +# This gate's own fixtures hold raw controls on purpose, so the repository-wide +# run must not see them -- exactly as scripts/check_nolint_directives.sh prunes +# tests/lint for its own inert directives. Naming a path inside the fixture +# tree still scans it, which is how the self-test reaches them. +FIXTURE_ROOT = "tests/lint/bidi_controls" + +# Directories a disk walk never descends into. Only needed in PATH mode; the +# default mode asks git, which already knows. +PRUNE_DIRS = {".git", "_deps", "__pycache__", "node_modules", ".venv"} + + +def detect(text: str) -> set[int]: + """Every rejected codepoint present in `text`. The one detector both the + scan and the self-probe go through, so the probe cannot pass against a + different rule from the one the tree is judged by.""" + return {ord(ch) for ch in text if ord(ch) in BIDI_CONTROLS} + + +def self_probe() -> list[str]: + """Assert the detector still sees each of the twelve codepoints, one at a + time. This is what stops a clean tree and a broken detector from producing + the same green.""" + missed = [ + f"U+{cp:04X} {name} is no longer detected" + for cp, name in sorted(BIDI_CONTROLS.items()) + if detect(chr(cp)) != {cp} + ] + return missed + + +def render(line: str) -> str: + """The line with every control replaced by a visible marker, so a message + about invisible characters is itself readable.""" + return "".join( + f"" if ord(ch) in BIDI_CONTROLS else ch for ch in line + ) + + +def tracked_files(root: Path) -> list[Path]: + """Every tracked file, which is the repository's own answer to 'what is + first-party here' -- derived rather than a list this file would have to + keep in step.""" + out = subprocess.run( + ["git", "-C", str(root), "ls-files", "-z"], + capture_output=True, + text=True, + check=True, + ).stdout + return [root / name for name in out.split("\0") if name] + + +def walked_files(path: Path) -> list[Path]: + if path.is_file(): + return [path] + found: list[Path] = [] + for child in sorted(path.rglob("*")): + if any(part in PRUNE_DIRS for part in child.parts): + continue + if child.is_file() and not child.is_symlink(): + found.append(child) + return found + + +def scan(path: Path) -> tuple[list[tuple[int, int, int, str]], bool]: + """Occurrences as (line, column, codepoint, line text), plus whether the + file was read as text at all. + + A file holding a NUL byte is git's own definition of binary, and a byte + sequence that happens to spell a control there is not source anybody reads. + Decoding with `errors="replace"` is safe for the rest: a replacement + character is never one of the twelve.""" + data = path.read_bytes() + if b"\0" in data: + return [], False + text = data.decode("utf-8", errors="replace") + hits: list[tuple[int, int, int, str]] = [] + for lineno, line in enumerate(text.splitlines(), start=1): + for col, ch in enumerate(line, start=1): + if ord(ch) in BIDI_CONTROLS: + hits.append((lineno, col, ord(ch), line)) + return hits, True + + +def main(argv: list[str]) -> int: + root = Path.cwd() + errors: list[str] = [] + + missed = self_probe() + if missed: + for miss in missed: + print(f"::error::{miss}", file=sys.stderr) + print( + "\nThe raw-bidi-control detector no longer recognises every codepoint " + "it claims to. A scan run with it would report a clean tree whether " + "or not the tree is clean, which is the one outcome this gate must " + "never produce. Fix BIDI_CONTROLS / detect() before trusting any " + "result from this script.", + file=sys.stderr, + ) + return 1 + + if argv: + candidates: list[Path] = [] + for arg in argv: + path = Path(arg) + if not path.exists(): + print(f"error: no such path: {arg}", file=sys.stderr) + return 1 + candidates.extend(walked_files(path)) + else: + try: + candidates = tracked_files(root) + except subprocess.CalledProcessError as exc: + print( + f"error: `git ls-files` failed in {root}: {exc.stderr.strip()}\n" + f" Without it this gate does not know which files are " + f"first-party, and must not report success.", + file=sys.stderr, + ) + return 1 + candidates = [ + p + for p in candidates + if not p.relative_to(root).as_posix().startswith(f"{FIXTURE_ROOT}/") + ] + + scanned = 0 + binary = 0 + offenders: dict[str, list[tuple[int, int, int, str]]] = {} + found_any: set[str] = set() + + for path in candidates: + try: + rel = path.relative_to(root).as_posix() + except ValueError: + rel = path.as_posix() + hits, is_text = scan(path) + if not is_text: + binary += 1 + continue + scanned += 1 + if hits: + found_any.add(rel) + if rel not in EXEMPT: + offenders[rel] = hits + + if scanned == 0: + target = " ".join(argv) if argv else str(root) + print( + f"error: no text files found under: {target}\n" + f" This gate has nothing to check and must not report success. " + f"Either the scan's file set or the path it was given has stopped " + f"matching the tree.", + file=sys.stderr, + ) + return 1 + + for rel, hits in sorted(offenders.items()): + for lineno, col, cp, line in hits: + errors.append( + f"{rel}:{lineno}:{col}: raw U+{cp:04X} {BIDI_CONTROLS[cp]}\n" + f" {lineno} | {render(line)}" + ) + + for rel, reason in sorted(EXEMPT.items()): + if not (root / rel).exists(): + errors.append( + f"{rel} is exempted by this checker (reason: {reason}) but no such " + f"file exists. Delete the exemption: it is a stale record of a " + f"file that has moved or gone." + ) + elif rel not in found_any: + errors.append( + f"{rel} is exempted by this checker (reason: {reason}) but contains " + f"no raw bidi control. Delete the exemption -- one that is not " + f"doing anything is cover for the next one that is wrong." + ) + + if errors: + for err in errors: + print(f"::error::{err}", file=sys.stderr) + print( + f""" +{len(errors)} problem(s). Scanned {scanned} text file(s), skipped {binary} binary. + +A raw bidi control is invisible: it renders as nothing and a diff containing +one looks exactly like a diff that does not. In a string literal that is a +silently wrong assertion (morph#610); in a comment it is source that stops +saying what it means (morph#628); an override or isolate can make a line render +as a different program from the one that compiles. + +Write the escape your file's language provides instead -- `\\u200E` in C++, QML +and JSON, `\\u{{200E}}` where braces are accepted, `‎` in markup. The +escaped form is what every one of the thirteen surviving occurrences in +src/qt/forms/tests/tst_i18n.qml uses. + +If a raw control is genuinely necessary, add the file to EXEMPT in +scripts/check_bidi_controls.py with a reason a reviewer can check.""", + file=sys.stderr, + ) + return 1 + + exemptions = f", {len(EXEMPT)} exemption(s)" if EXEMPT else "" + print( + f"bidi-control lint OK: {scanned} text file(s) scanned " + f"({binary} binary skipped), {len(BIDI_CONTROLS)} codepoint(s) searched " + f"for, 0 raw occurrence(s){exemptions}." + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/test_check_bidi_controls.sh b/scripts/test_check_bidi_controls.sh new file mode 100755 index 00000000..4ba04f59 --- /dev/null +++ b/scripts/test_check_bidi_controls.sh @@ -0,0 +1,254 @@ +#!/usr/bin/env bash +# Usage: bash scripts/test_check_bidi_controls.sh +# +# Self-test for scripts/check_bidi_controls.py, the gate that keeps raw Unicode +# bidirectional control characters out of first-party files (morph#628). +# +# This gate needs its own test more than most, for a reason its own subject +# makes sharp. The tree is already clean -- 0 raw controls across 1242 tracked +# files -- so the gate passes on day one whether or not it can detect anything +# at all, and the thing it would be failing to detect is by definition +# invisible. "0 occurrences found" is exactly what a broken detector prints. +# +# So the cases below are not about the tree. They are about whether the gate +# would still be a gate on the day something arrives: +# +# - Every declared codepoint must be found in a fixture that contains it. +# This list is written out HERE, independently of BIDI_CONTROLS in the +# checker: the checker's own start-up probe iterates that same table, so it +# cannot notice an entry being deleted from it. This case can. +# - Each invalid fixture must be rejected on its own and must name its own +# file, so a rejection for some unrelated reason does not read as detection. +# - No diagnostic may contain a raw control. A gate against invisible +# characters that prints them is reporting the defect by committing it. +# - A directory with no scannable files must be rejected, not called clean. +# - Both EXEMPT hygiene rules must fire, and an exemption must actually work. +# +# Mutations are applied one at a time to a scratch copy: applied together, a +# single detection would mask every other. +set -euo pipefail + +readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +readonly checker="scripts/check_bidi_controls.py" +readonly fixtures="tests/lint/bidi_controls" + +# The twelve codepoints this gate claims to reject, restated independently of +# the checker. If the checker's table loses one, the coverage case below fails +# even though the checker's own probe is content. +readonly -a declared=( + 061C 200E 200F + 202A 202B 202C 202D 202E + 2066 2067 2068 2069 +) + +failures=0 + +note() { printf 'ok: %s\n' "$*"; } +fail() { printf 'error: %s\n' "$*" >&2; failures=$((failures + 1)); } + +scratch="$(mktemp -d)" +trap 'rm -rf "$scratch"' EXIT + +# ── the real tree must pass ───────────────────────────────────────────────── +# Not evidence on its own -- that is this whole file's premise -- but a +# regression here means the gate cannot be landed at all. +if output="$( cd "$repo_root" && python3 "$checker" 2>&1 )"; then + note "the repository passes: ${output}" +else + fail "the gate rejects the repository as it stands:" + printf '%s\n' "$output" >&2 +fi + +# ── valid fixtures must be accepted ───────────────────────────────────────── +# Including the one whose prose NAMES the codepoints in ASCII, which must not +# be flagged: a gate that rejected it would make its own documentation +# unwritable, the same allowance check_nolint_directives.sh makes for the +# sentence naming NOLINTNEXTLINE. +if output="$( cd "$repo_root" && python3 "$checker" "${fixtures}/valid" 2>&1 )"; then + note "valid fixtures accepted" +else + fail "valid fixtures were rejected -- escaped spellings or ASCII prose about \ +these characters are being read as occurrences:" + printf '%s\n' "$output" >&2 +fi + +# ── every invalid fixture directory must be rejected, on its own ──────────── +shopt -s nullglob +invalid_dirs=("${repo_root}/${fixtures}"/invalid/*/) +shopt -u nullglob + +if [ "${#invalid_dirs[@]}" -eq 0 ]; then + fail "no fixtures found under ${fixtures}/invalid -- this self-test would pass vacuously" +fi + +for dir in "${invalid_dirs[@]}"; do + name="$(basename "$dir")" + rel="${fixtures}/invalid/${name}" + if output="$( cd "$repo_root" && python3 "$checker" "$rel" 2>&1 )"; then + fail "invalid fixture ${name} was accepted; the checker no longer detects it" + continue + fi + # A nonzero exit is not enough: the checker's other failure paths are "no + # text files found" and the start-up probe, and a fixture whose files went + # missing would take the first -- so the fixture would still look + # "rejected" while nothing was scanned. Require the occurrence diagnostic, + # and require it to name a file under this fixture. + if ! grep -q 'raw U+' <<<"$output"; then + fail "invalid fixture ${name} was rejected, but not as a raw bidi control \ +-- the checker failed for some other reason (a vacuous scan, most likely):" + printf '%s\n' "$output" >&2 + elif ! grep -qF "${rel}/" <<<"$output"; then + fail "invalid fixture ${name} was rejected without naming any file under \ +${rel}; the diagnostic does not point at the offender:" + printf '%s\n' "$output" >&2 + else + note "invalid fixture ${name} rejected" + fi +done + +# ── every declared codepoint must actually be found ───────────────────────── +# The case that decides whether the declared set still means anything. Run the +# checker over the whole invalid corpus and require each of the twelve above to +# be named. Delete a codepoint from BIDI_CONTROLS and this is what goes red. +all_output="$( cd "$repo_root" && python3 "$checker" "${fixtures}/invalid" 2>&1 || true )" +missing=() +for cp in "${declared[@]}"; do + if ! grep -qF "raw U+${cp} " <<<"$all_output"; then + missing+=("U+${cp}") + fi +done +if [ "${#missing[@]}" -ne 0 ]; then + fail "the fixtures contain these codepoints but the checker reported none of \ +them: ${missing[*]}. Either the checker's declared set has shrunk or the \ +fixture holding them has been edited away." +else + note "all ${#declared[@]} declared codepoints found in the invalid fixtures" +fi + +# ── the diagnostic must itself be readable ────────────────────────────────── +# A gate against invisible characters must not print them: a message a reader +# cannot see is the defect being reported, one level up. Every control in an +# echoed line is replaced by a visible marker. +if grep -qP '[\x{061C}\x{200E}\x{200F}\x{202A}-\x{202E}\x{2066}-\x{2069}]' <<<"$all_output"; then + fail "the checker's own diagnostics contain raw bidi controls; the failure \ +message reproduces the defect it reports" +else + note "no diagnostic contains a raw bidi control" +fi + +# ── a directory with no scannable files must be rejected ──────────────────── +# "Found nothing" is the one outcome this gate must never call clean: it is +# what a wrong path, a swallowed prune rule or a dead file walk all look like. +empty_dir="${scratch}/empty" +mkdir -p "$empty_dir" +if ( cd "$repo_root" && python3 "$checker" "$empty_dir" >/dev/null 2>&1 ); then + fail "a directory containing no files was accepted; the gate can pass vacuously" +else + note "directory with no scannable files rejected" +fi + +# ── mutation cases against a scratch copy ─────────────────────────────────── +# `sample/` holds one clean file and one carrying a raw U+200E, so an exemption +# has something to be right about and something to be wrong about. +readonly pristine="${scratch}/pristine" +mkdir -p "${pristine}/scripts" "${pristine}/sample" +cp "${repo_root}/${checker}" "${pristine}/scripts/" +printf 'constexpr const char* kSign = "\\u200E+";\n' > "${pristine}/sample/clean.cpp" +python3 -c 'import pathlib,sys; pathlib.Path(sys.argv[1]).write_text("constexpr const char* kSign = \"\u200E+\";\n", encoding="utf-8")' \ + "${pristine}/sample/dirty.cpp" + +make_tree() { + rm -rf "${scratch}/case" + mkdir -p "${scratch}/case" + cp -R "${pristine}/." "${scratch}/case" +} + +# `sed -i` is not portable between GNU and BSD sed; edit through a temp file. +edit() { + local file="$1"; shift + sed "$@" "$file" > "${file}.new" + mv "${file}.new" "$file" +} + +expect_caught() { + local description="$1" mutator="$2" expected="$3" output + make_tree + if ! ( cd "${scratch}/case" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "${scratch}/case" && python3 "$checker" sample 2>&1 )"; then + fail "NOT caught: ${description} -- the gate passed a tree it should reject" + printf '%s\n' "$output" >&2 + return + fi + if grep -qF "$expected" <<<"$output"; then + note "caught: ${description}" + else + fail "caught for the WRONG reason: ${description} -- no diagnostic \ +containing '${expected}':" + printf '%s\n' "$output" >&2 + fi +} + +expect_accepted() { + local description="$1" mutator="$2" output + make_tree + if ! ( cd "${scratch}/case" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "${scratch}/case" && python3 "$checker" sample 2>&1 )"; then + note "accepted: ${description}" + else + fail "FALSE POSITIVE: ${description} -- the gate rejected a tree it \ +should accept:" + printf '%s\n' "$output" >&2 + fi +} + +# The baseline the exemption cases are measured against: unexempted, the raw +# U+200E in sample/dirty.cpp is reported and the escaped one in +# sample/clean.cpp is not. +expect_caught "an unexempted file carrying a raw U+200E" \ + "true" \ + "sample/dirty.cpp:1:32: raw U+200E" + +# The mechanism has to work, or every case below tests an exemption that could +# not be used. Exempting the file that is actually dirty clears the tree. +expect_accepted "an exemption for the file that carries the control" \ + "edit ${checker} -e 's|^EXEMPT: dict\[str, str\] = {}|EXEMPT: dict[str, str] = {\"sample/dirty.cpp\": \"a reason\"}|'" + +# Hygiene rule one: an exemption for a file with nothing to exempt. This is the +# entry that outlives its cause and quietly covers whatever lands in that file +# next. +expect_caught "an exemption for a file with no raw controls" \ + "edit ${checker} -e 's|^EXEMPT: dict\[str, str\] = {}|EXEMPT: dict[str, str] = {\"sample/clean.cpp\": \"a reason\"}|'" \ + "contains no raw bidi control" + +# Hygiene rule two: an exemption for a file that does not exist. +expect_caught "an exemption naming a file that does not exist" \ + "edit ${checker} -e 's|^EXEMPT: dict\[str, str\] = {}|EXEMPT: dict[str, str] = {\"sample/gone.cpp\": \"a reason\"}|'" \ + "no such file exists" + +# The start-up probe: a detector narrowed to the two marks this repository has +# actually hit still passes over a clean tree, and would pass over the whole +# repository today. The probe is what makes that impossible -- and note it +# fires here even though sample/ is a tree the gate would otherwise reject for +# an ordinary reason, because it runs before any file is opened. +expect_caught "the detector narrowed to a subset of the declared codepoints" \ + "edit ${checker} -e 's|^ return {ord(ch) for ch in text if ord(ch) in BIDI_CONTROLS}| return {ord(ch) for ch in text if ord(ch) == 0x200E}|'" \ + "is no longer detected" + +# The same failure written the other way round: a detector that finds nothing +# at all, which is the shape a mistyped range or an emptied table produces. +expect_caught "the detector disabled outright" \ + "edit ${checker} -e 's|^ return {ord(ch) for ch in text if ord(ch) in BIDI_CONTROLS}| return set()|'" \ + "is no longer detected" + +if [ "$failures" -ne 0 ]; then + printf '\n%d case(s) failed.\n' "$failures" >&2 + exit 1 +fi + +printf '\nall cases passed.\n' diff --git a/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp b/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp new file mode 100644 index 00000000..bf708421 --- /dev/null +++ b/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// morph#628's own reproduction, byte for byte: a JSON-payload tool decoded +// the escape before the file was written, so the comment below carries a +// single raw U+061C where six characters were typed. + +// Before morph#591 this edge emitted "؜-1050.25" -- the sign is invisible. + +constexpr const char* kSign = "؜-"; diff --git a/tests/lint/bidi_controls/invalid/directional_marks/marks_in_literals.qml b/tests/lint/bidi_controls/invalid/directional_marks/marks_in_literals.qml new file mode 100644 index 00000000..b953f918 --- /dev/null +++ b/tests/lint/bidi_controls/invalid/directional_marks/marks_in_literals.qml @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// morph#642's shape: assertion literals carrying raw U+200E / U+200F while +// their neighbours in the same file use the escape. + +import QtQuick + +Item { + property string azIR: "‎+‎" + property string ckbIQ: "‏+" +} diff --git a/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp b/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp new file mode 100644 index 00000000..40b33c08 --- /dev/null +++ b/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// The embeddings, their overrides and their terminator. These are the +// trojan-source characters: they change how the line renders relative to +// how it compiles, so the reviewer and the compiler read different +// programs. + +constexpr const char* kEmbed = "‪a‬"; +constexpr const char* kEmbedRtl = "‫a‬"; +constexpr const char* kOverride = "‭a‮b"; diff --git a/tests/lint/bidi_controls/invalid/isolates/isolates.hpp b/tests/lint/bidi_controls/invalid/isolates/isolates.hpp new file mode 100644 index 00000000..54486ad1 --- /dev/null +++ b/tests/lint/bidi_controls/invalid/isolates/isolates.hpp @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// The isolates and their terminator -- the modern spelling of the same +// reordering hazard, and the class morph#628's triage called the sharper +// one. + +#pragma once + +constexpr const char* kLri = "⁦x⁩"; +constexpr const char* kRli = "⁧x⁩"; +constexpr const char* kFsi = "⁨x⁩"; diff --git a/tests/lint/bidi_controls/valid/escaped_literals.cpp b/tests/lint/bidi_controls/valid/escaped_literals.cpp new file mode 100644 index 00000000..bc5afc4b --- /dev/null +++ b/tests/lint/bidi_controls/valid/escaped_literals.cpp @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// The accepted spelling. Every bidi control here is an escape, so the bytes on +// disk are ASCII and a reviewer reads exactly what the compiler reads. This is +// what src/qt/forms/tests/tst_i18n.qml does for all thirteen of its +// occurrences, and what morph#610 converted nine assertion lines to. + +constexpr const char* kArabicLetterMark = "\u061C"; +constexpr const char* kLeftToRightMark = "\u200E"; +constexpr const char* kRightToLeftMark = "\u200F"; +constexpr const char* kRightToLeftOverride = "\u202E"; +constexpr const char* kFirstStrongIsolate = "\u2068"; +constexpr const char* kPopDirectionalIsolate = "\u2069"; + +// The brace form too, which is how a u8 literal often spells it. +constexpr const char* kBraced = "\u{200E}+\u{200E}"; diff --git a/tests/lint/bidi_controls/valid/no_controls_at_all.qml b/tests/lint/bidi_controls/valid/no_controls_at_all.qml new file mode 100644 index 00000000..91530bb9 --- /dev/null +++ b/tests/lint/bidi_controls/valid/no_controls_at_all.qml @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// An ordinary file with nothing to say about bidi at all. The valid set must +// contain one, or "accepted" would only ever mean "accepted a file that +// mentions the subject". + +import QtQuick + +Item { + property string sign: "+" +} diff --git a/tests/lint/bidi_controls/valid/prose_naming_the_codepoints.hpp b/tests/lint/bidi_controls/valid/prose_naming_the_codepoints.hpp new file mode 100644 index 00000000..8a0aa662 --- /dev/null +++ b/tests/lint/bidi_controls/valid/prose_naming_the_codepoints.hpp @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Prose that NAMES the characters must not be flagged, or this gate would make +// its own documentation unwritable -- the same allowance +// scripts/check_nolint_directives.sh makes for the sentence in +// include/morph/detail/fixed_string.hpp that names NOLINTNEXTLINE. +// +// A raw U+200E LEFT-TO-RIGHT MARK in a string literal is a silently wrong +// assertion; U+202E RIGHT-TO-LEFT OVERRIDE and U+2066..U+2069, the isolates, +// are the ones that reorder rendered text. Write them as \u200E, \u202E and +// so on. U+061C, the ARABIC LETTER MARK, is the one morph#628 typed by +// accident. + +#pragma once From 44d59ff1736172e200ad7413e05c8d4bd67bc8b8 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 02:45:50 +0200 Subject: [PATCH 2/2] ci: keep the bidi-control fixtures clean under clang-tidy-diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every line of a new file is a changed line, and the clang-tidy job applies no path exclusion -- no -exclude in its clang-tidy-diff.py invocation, and no .clang-tidy anywhere under tests/lint/. The three .cpp fixtures added for morph#628 therefore produced 14 findings, all inside themselves: 11 clang-diagnostic-unused-const-variable 3 misc-misleading-bidirectional Reproduced locally on clang-tidy 22.1.8 (CI's pinned major) with the warning set cmake/compiler_options.cmake builds for Clang -- -Weverything minus the suppressions in its Clang branch -- and the result matches CI's deduplicated list file for file, line for line, column for column. Fixed the way the six fixture directories already under tests/lint/ avoid this: the fixtures are written so clang-tidy has nothing to say about them. No new .clang-tidy, no path exclusion, no byte of a raw control removed from an invalid/ fixture. - `inline` on each namespace-scope constant. A `constexpr` there has internal linkage, so an unused one is a diagnostic; an `inline` one has external linkage and is not. This is house style in any case -- include/morph/ spells 36 of its constants that way -- and it changes no byte of any string literal. - Three NOLINTNEXTLINE(misc-misleading-bidirectional), reason above the directive, never wrapped onto a second line (morph#631/#635). misc-misleading-bidirectional is not the same gate as this PR's, and the suppressions are where the two are recorded as disagreeing. It reads the string's *content*, so it fires on escaped_literals.cpp's `"‮"` and `"⁨"` although those are written as escapes and the file on disk is pure ASCII; this gate reads the *source bytes*, so it does not. Neither subsumes the other: clang-tidy sees only C++ translation units the configure put in the compile database, so it would never have looked at tst_i18n.qml, a Markdown file or a workflow YAML, while this scan covers all 1244 tracked text files and cannot see a control that exists only after escape expansion. Suppressing the check globally, or for tests/lint/ as a directory, would have thrown away the half of the coverage this PR does not provide. Verified, not asserted. Before: 14 findings. After: 0. Mutated back one change at a time, in place so the .clang-tidy chain resolves identically: dropping `inline` from escaped_literals.cpp restores 7 unused-const-variable findings, and deleting the three directives restores all 3 misc-misleading-bidirectional findings -- so both halves are load-bearing and the probe still measures something. The fixtures still fail when they should: scripts/test_check_bidi_controls.sh passes all 15 cases, including "invalid fixture rejected" for each of the four invalid directories on its own. Also green locally: check_bidi_controls.py, check_nolint_directives.sh (and again with tests/lint/bidi_controls named explicitly, since the default target prunes tests/lint), check_tidy_suppression_scope.sh, check_workflow_job_banners.py, check_spec_citations.sh, check_ci_clang_pin.sh, check_catch_test_names.sh, and clang-format --dry-run -Werror over the fixtures (the clang-format job is whole-tree). The tracked-text-file count in drift-guard.yml and in the self-test's header moves 1242 -> 1244: the same measurement, restated on this branch's new base after morph#647, morph#641 and morph#630 landed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- .github/workflows/drift-guard.yml | 2 +- scripts/check_bidi_controls.py | 2 +- scripts/test_check_bidi_controls.sh | 2 +- .../arabic_letter_mark/alm_in_a_comment.cpp | 8 +++++- .../embeddings_and_overrides/reordering.cpp | 13 +++++++-- .../bidi_controls/valid/escaped_literals.cpp | 28 ++++++++++++++----- 6 files changed, 41 insertions(+), 14 deletions(-) diff --git a/.github/workflows/drift-guard.yml b/.github/workflows/drift-guard.yml index 7a7b3987..1d23102e 100644 --- a/.github/workflows/drift-guard.yml +++ b/.github/workflows/drift-guard.yml @@ -232,7 +232,7 @@ jobs: # nothing, so it rides this job for the same reason the NOLINT-directive, # CI-clang-pin and Catch2-name scans above do. # - # This one ships already green -- 0 raw controls across 1242 tracked + # This one ships already green -- 0 raw controls across 1244 tracked # files -- which makes the self-test the whole of its evidence rather # than a formality. "0 occurrences found" is exactly what a broken # detector prints, so the checker probes its own detector against every diff --git a/scripts/check_bidi_controls.py b/scripts/check_bidi_controls.py index cdcba49b..4913a6c3 100755 --- a/scripts/check_bidi_controls.py +++ b/scripts/check_bidi_controls.py @@ -57,7 +57,7 @@ A scan that reports "0 raw controls" is satisfied by a broken pattern exactly as well as by a clean tree, and the tree *is* clean today (0 occurrences across -1242 tracked files), so this gate ships already green. It would therefore be +1244 tracked files at f7c231df), so this gate ships already green. It would therefore be worth nothing unless both ways of going blind are closed: 1. **The detector drifts away from the declared set.** Before scanning diff --git a/scripts/test_check_bidi_controls.sh b/scripts/test_check_bidi_controls.sh index 4ba04f59..7ad84cdf 100755 --- a/scripts/test_check_bidi_controls.sh +++ b/scripts/test_check_bidi_controls.sh @@ -5,7 +5,7 @@ # bidirectional control characters out of first-party files (morph#628). # # This gate needs its own test more than most, for a reason its own subject -# makes sharp. The tree is already clean -- 0 raw controls across 1242 tracked +# makes sharp. The tree is already clean -- 0 raw controls across 1244 tracked # files -- so the gate passes on day one whether or not it can detect anything # at all, and the thing it would be failing to detect is by definition # invisible. "0 occurrences found" is exactly what a broken detector prints. diff --git a/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp b/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp index bf708421..5533a236 100644 --- a/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp +++ b/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp @@ -3,7 +3,13 @@ // morph#628's own reproduction, byte for byte: a JSON-payload tool decoded // the escape before the file was written, so the comment below carries a // single raw U+061C where six characters were typed. +// +// `inline` is load-bearing, not style: a namespace-scope `constexpr` +// has internal linkage, and clang-tidy-diff analyses every line of a +// new file, so an unused one is reported as +// clang-diagnostic-unused-const-variable and fails the job. Nothing +// here is compiled, but the gate that reads it does not know that. // Before morph#591 this edge emitted "؜-1050.25" -- the sign is invisible. -constexpr const char* kSign = "؜-"; +inline constexpr const char* kSign = "؜-"; diff --git a/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp b/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp index 40b33c08..863fb921 100644 --- a/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp +++ b/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp @@ -4,7 +4,14 @@ // trojan-source characters: they change how the line renders relative to // how it compiles, so the reviewer and the compiler read different // programs. +// +// `inline` keeps clang-diagnostic-unused-const-variable off a fixture +// nothing compiles; see invalid/arabic_letter_mark/. -constexpr const char* kEmbed = "‪a‬"; -constexpr const char* kEmbedRtl = "‫a‬"; -constexpr const char* kOverride = "‭a‮b"; +inline constexpr const char* kEmbed = "‪a‬"; +inline constexpr const char* kEmbedRtl = "‫a‬"; +// LRE with no terminating PDF: misc-misleading-bidirectional reads the +// string's *content* and is right about it. This gate reads the source +// *bytes*, which is why the fixture has to keep them. +// NOLINTNEXTLINE(misc-misleading-bidirectional) +inline constexpr const char* kOverride = "‭a‮b"; diff --git a/tests/lint/bidi_controls/valid/escaped_literals.cpp b/tests/lint/bidi_controls/valid/escaped_literals.cpp index bc5afc4b..4baf551b 100644 --- a/tests/lint/bidi_controls/valid/escaped_literals.cpp +++ b/tests/lint/bidi_controls/valid/escaped_literals.cpp @@ -4,13 +4,27 @@ // disk are ASCII and a reviewer reads exactly what the compiler reads. This is // what src/qt/forms/tests/tst_i18n.qml does for all thirteen of its // occurrences, and what morph#610 converted nine assertion lines to. +// +// Two things keep clang-tidy-diff green over a file every line of which +// is a changed line. `inline` gives each constant external linkage, so +// clang-diagnostic-unused-const-variable does not fire on a fixture +// nothing compiles. And misc-misleading-bidirectional still fires on two +// of the escapes below -- an escape produces the same bytes in the +// compiled string, so that check judges the string's content while this +// gate judges the source bytes. The two are complementary, and the +// per-line suppressions record where they disagree. -constexpr const char* kArabicLetterMark = "\u061C"; -constexpr const char* kLeftToRightMark = "\u200E"; -constexpr const char* kRightToLeftMark = "\u200F"; -constexpr const char* kRightToLeftOverride = "\u202E"; -constexpr const char* kFirstStrongIsolate = "\u2068"; -constexpr const char* kPopDirectionalIsolate = "\u2069"; +inline constexpr const char* kArabicLetterMark = "\u061C"; +inline constexpr const char* kLeftToRightMark = "\u200E"; +inline constexpr const char* kRightToLeftMark = "\u200F"; +// RLO with no terminating PDF, and FSI with no terminating PDI: the +// content check reads the expanded literal, which is exactly the point +// of writing them as escapes. +// NOLINTNEXTLINE(misc-misleading-bidirectional) +inline constexpr const char* kRightToLeftOverride = "\u202E"; +// NOLINTNEXTLINE(misc-misleading-bidirectional) +inline constexpr const char* kFirstStrongIsolate = "\u2068"; +inline constexpr const char* kPopDirectionalIsolate = "\u2069"; // The brace form too, which is how a u8 literal often spells it. -constexpr const char* kBraced = "\u{200E}+\u{200E}"; +inline constexpr const char* kBraced = "\u{200E}+\u{200E}";