ci: reject raw bidi control characters in first-party files (fixes #628) - #644
Conversation
Yaraslaut
changed the title
Runner verificationThe lane is right and I was wrong. #642 was my error: my checkout was still at Two things from that worth carrying into this gate, both of which the lane had already reasoned its way to:
Verified the load-bearing claim — that the tree is clean, so every failure path is exercised by the self-test rather than by the tree — by mutation rather than by reading: 1244 files against 12 codepoints is a real denominator, not a green tick over an empty set; the injected control is caught with file, line and column; the diagnostic renders it as Not merged this sweep: CI incomplete at hand-off. Next sweep counts the checks. 🤖 Generated with Claude Code |
Runner:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 <U+200E> 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 c55ea5b 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW
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 <name> 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW
5085f26 to
44d59ff
Compare
Runner verification of the fixThe fix does not weaken the fixtures — checked, not assumed. That was the one risk I flagged when dispatching, so I counted the raw controls in every fixture on both sides of the fix commit: Not one byte of fixture payload moved. The self-test agrees on the rebased branch — 15 cases, all pass, each Reproduced the clang-tidy mutation independently: Both halves are load-bearing, so the fix addresses the diagnostics rather than hiding them. Two of my own probes were vacuous before this one worked, which is worth recording since this PR is about controls that measure nothing. First I ran The load-bearing claim checks out: all five C++ fixtures contain zero Convention followed, not invented — the six pre-existing #650 is the durable finding here — that the six existing fixture directories are green by accident of content, with the convention written down nowhere, so the eighth directory learns it the way the seventh just did. That is the reusable lesson from this red leg, and filing it beats fixing three files and moving on. Not merged: CI incomplete at hand-off. Next sweep counts the checks. 🤖 Generated with Claude Code |
1 participant
Closes #628. (#642 is closed separately as
triage: invalid— its premise was the runner's stale-checkout measurement, not a defect this PR fixes.)#610 asked for this lint in its own closing note and deferred it as "a bigger
decision than this issue". #628 filed it. This is it.
What the gate rejects
Twelve codepoints in three classes — exactly the list #610 wrote:
U+061CU+200E,U+200FU+202A–U+202EU+2066–U+2069Escaped spellings (
\u200E,\u{200E},‎) are always accepted; onlythe literal codepoint is rejected.
Comments are in scope, deliberately. #610's original sketch said "outside
comments". #628's own reproduction was inside comments, and the trojan-source
shape lives in comments by construction. In a literal a raw control is a
silently wrong assertion; in a comment it is source that stops saying what it
means.
The one claim this branch's safety rests on
The tree contains zero raw bidi controls, so this gate cannot break anyone
today, and every one of its failure paths is exercised by the self-test rather
than by the tree.
Measured on
c55ea5b7, over all 1242 tracked files, reading bytes rather thantrusting a regex:
The second line is the control on the first: the same detector, pointed at a
string holding all twelve codepoints, finds all twelve. Without it,
raw_bidi_hits=0is equally consistent with a clean tree and a dead scan.If that claim is wrong — if some file does carry one —
prose-lintgoes red onthis PR and the fix is either an escape or an
EXEMPTentry. No build, notoolchain, no timing in the failure mode.
#642: the five raw controls are not in the merged tree
#642's premise does not reproduce. The five raw
U+200E/U+200Fit reportsin
src/qt/forms/tests/tst_i18n.qmldo not exist at513145a1— the squashcommit that closed #610 and #591 — and do not exist at
c55ea5b7:That pattern is live, not dead — pointed at a file holding one raw
U+200Eitreturns 1:
The excerpt in #642 also shows a different function signature from the one on
master— positional arguments (normalizeLocaleNumber(s, ".", "", "-", sign))where
masterhas an options object (normalizeLocaleNumber(s, { decimalSeparator: ".", ... })).So, inferred rather than measured: #642 was taken against an intermediate
revision of #630's branch, and the review round that converted the signature
also escaped the controls. I did not verify that inference against #630's
pre-squash branch history. What I verified is the merged result, which is what
masterhas.There is therefore no fix commit for #642, because there is nothing to fix.
Its own closing condition is met by this PR: "Close as resolved when the five
are escaped and #628's lint exists" — they are (they never landed
unescaped), and it now does.
This contradicts my dispatch brief, which expected a separate #642 commit.
Recorded here rather than shipped around, per AGENTS.md.
The gate caught its own author, on this PR, before the push
Not a hypothetical, and the strongest evidence in this PR.
The first draft of the workflow comment said "typing \u061C through a
JSON-payload tool that decoded the escape". The tool payload decoded it. A
single invisible
U+061Clanded in.github/workflows/drift-guard.yml, wentinto the commit, and was invisible in
git show:That is #628's reproduction a fourth time, on the PR that closes it, in the
file that adds the gate. Nothing else in the repository would have reported it.
The comment now spells the character in ASCII and says so in place.
Closing the vacuity traps
The tree is already clean, so this gate ships green and would be worth nothing
unless every way of going blind is closed. Three exist, each with its own
control:
the checker probes itself against each declared codepoint, one at a time, and
refuses to run if any comes back unflagged. Pinned by two self-test cases: a
detect()narrowed toU+200E(the shape "only guard what we have actuallyhit"), and a
detect()returning nothing. Both fail on a clean tree.Pinned by a self-test case pointing the checker at an empty directory — the
same handling
check_nolint_directives.shgives the same problem.iterates the same table. So
scripts/test_check_bidi_controls.shholds itsown independent list of the twelve, runs the checker over
tests/lint/bidi_controls/invalid/, and requires every codepoint on itslist to be named in the output. One fixture directory per character class,
each rejected on its own and each required to name its own file.
Fourth, specific to this gate: the diagnostic must itself be readable. Every
offending line is echoed with each control replaced by a visible
<U+200E>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.
Self-test output, run locally on
5085f268:And the end-to-end mutation, which is the case that matters: reintroducing
#642's exact defect into
tst_i18n.qml(un-escaping line 284) makes therepo-wide run red, naming file, line, column and codepoint:
Exemptions
EXEMPTmaps a repository-relative path to a written reason, and is empty —which is the measured state of the tree, not an aspiration. Both hygiene rules
from
check_workflow_option_coverage.py'sEXEMPTandcheck_workflow_job_banners.py'sUNBANNEREDapply, and all three cases arepinned by the self-test against a scratch tree holding a patched copy of the
checker:
entry for a file that does carry one clears the tree.
The grain is the file, not the line. Deliberate: a file that genuinely needs a
raw control is a file whose whole subject is that character, and the argument
for it belongs in prose once, rather than sprinkled per line where it would
drift back into being invisible.
Where it runs
drift-guard.yml'sprose-lintjob, where #635 put the NOLINT-directive scanfor the same reason: a fast, dependency-free text scan that compiles nothing,
guarding the neighbouring class of defect. No new job, no new runner, and no
change to any existing job's
name:, so no required-check names move.Review reasoning, done inline
grep -Pin arun:block? It could not have aself-test, could not carry reasoned exemptions, and would print the raw line —
reproducing the invisible character in the failure message. The three vacuity
controls above are the whole value here; a bare grep has none of them.
*.cpp/*.hpp/*.qml? No lint keeps raw bidi controls out of first-party sources, and the authoring path that introduces them was hit twice while fixing #610 #628'sreproduction hit a header and a test, and the trojan-source class applies to
YAML, JSON, shell and Markdown equally. The tree is clean everywhere today, so
breadth costs nothing now; narrowing it later is a change someone would have
to argue for.
legitimately quoting RTL text would trip this. That is what
EXEMPTis for,and the hygiene rules stop it accumulating. No lint keeps raw bidi controls out of first-party sources, and the authoring path that introduces them was hit twice while fixing #610 #628 flagged this as its own
unverified point and it remains one.
reported separately. Zero today, across 1242 files.
tests/lint/bidi_controls/is pruned from the repo-wide run, since itholds raw controls on purpose — the same treatment
check_nolint_directives.shgives its own inert-directive fixtures. Naming apath inside it still scans it, which is how the self-test reaches it.
clang-format-clean (clang-format 22.1.8 --dry-run -Werror, exit 0), becauseci.yml'sclang-formatjob checks every tracked.hpp/.cpp,tests/lint/included.What I did not verify
on. Everything above is a local measurement, on Linux, with Python 3 and bash
only.
the self-test is bash; neither runs outside
ubuntu-24.04in this workflow.Filed, not folded
check_workflow_job_banners.py's docstring still namessuppression-guard.yml, which ci: split mutation_survivors.json's citations by what they are, and fold the NOLINT gate into drift-guard (fixes #613, fixes #633) #635 deleted. Documentation drift inside a gatewhose subject is documentation drift. Not touched here.
Gates run locally before pushing
check_workflow_job_banners.py(24 banners, ok) ·test_check_workflow_job_banners.sh(all cases passed) ·
check_nolint_directives.sh(164 directives, ok) ·check_mutation_survivors.py(15 citations resolve) ·check_spec_citations.sh·check_catch_test_names.sh(2986 names) ·check_deprecated_markers.sh·check_test_type_names.sh·check_journal_stamps.sh·check_tidy_suppression_scope.sh·test_check_bidi_controls.sh·check_bidi_controls.py· YAML parse ofdrift-guard.yml. All green.Follow-up commit: the fixtures under
clang-tidy-diff(red → green)The first push was red on
clang-tidy-diff(job106163759717, run35542939709) with 14 findings, every one inside the new fixtures:Cause. Every line of a new file is a changed line, and that job applies no
path exclusion at all: there is no
-excludein itsclang-tidy-diff.pyinvocation (deliberately — see the long comment on that step) and there is no
.clang-tidyanywhere undertests/lint/. The.hppand.qmlfixturesproduced nothing; only the three
.cppones did.Reproduced locally before and after, clang-tidy 22.1.8 (CI's pinned major),
with the warning set
cmake/compiler_options.cmakebuilds for Clang —-Weverythingminus every suppression in its Clang branch. That flag setreproduces CI's list exactly: same 14 findings, same files, same lines, same
columns, and it also reproduces the absence of
pre-c++23-compatonescaped_literals.cpp:16, which a plain-Weverythingdoes raise. Before:After: empty output, exit 0.
The fix, and why it beat the alternatives
Followed the existing convention. The six fixture directories already under
tests/lint/(nolint_directives,automoc_includes,catch_test_names,deprecated_markers,journal_stamps,test_type_names— 30.cppand 12.hppbetween them) carry no.clang-tidyand no exclusion. They are greenbecause they are written so clang-tidy has nothing to say about them. These
fixtures now are too.
inlineon each namespace-scope constant. Aconstexprthere hasinternal linkage, so an unused one is a diagnostic; an
inlineone hasexternal linkage and is not. It is house style anyway (
include/morph/spells36 of its constants that way) and it changes no byte of any string
literal — the raw controls in every
invalid/fixture are untouched.NOLINTNEXTLINE(misc-misleading-bidirectional), reason on the linesabove the directive, never wrapped onto a second line (lint: make wrapped NOLINT directives apply, gate the ones that cannot, and clear clang-tidy batch A (fixes #627, fixes #600) #631/ci: split mutation_survivors.json's citations by what they are, and fold the NOLINT gate into drift-guard (fixes #613, fixes #633) #635). Verified
with
check_nolint_directives.sh tests/lint/bidi_controls— 3 directives, allannotating code.
Rejected: a
tests/lint/.clang-tidy. It would be a second convention whereone already works, and it would put a new suppression file into exactly the
area #632 landed
check_tidy_suppression_scope.shto audit. Suppressingmisc-misleading-bidirectionalfor a whole directory would also have thrownaway the coverage described next.
Rejected: renaming the three
.cppfixtures to.hpp. That would dodgeunused-const-variableonly because clang does not raise it for a header — asubtle behaviour to depend on — and it would not have touched a single one of
the three
misc-misleading-bidirectionalfindings, which fire on.hppjustthe same.
Rejected: deleting the raw controls. That destroys the fixture.
test_check_bidi_controls.shrequires eachinvalid/directory to be rejectedon its own.
misc-misleading-bidirectionaland this gate are complementary, not redundantWorth stating plainly, because it is nearly this PR's own subject.
misc-misleading-bidirectionalis an existing clang-tidy check that fires onbidi controls in string literals — and it fires on
escaped_literals.cpp:11and
:12, whose controls are written as escapes and whose bytes on diskare pure ASCII. That is not a bug in either tool:
in the compiled string, so
""is as misleading to it as a raw RLO.and a diff renders.
Neither subsumes the other, and the difference is directional:
misc-misleading-bidirectionalcheck_bidi_controls.pycompile_commands.json.qml,.md,.yml,.json,.shtst_i18n.qml, this workflow's own YAMLThe two cases that motivated #628 are both outside clang-tidy's reach: #628's
own reproduction was a raw
U+061Cin a comment, and #610's nine lines werein
src/qt/forms/tests/tst_i18n.qml. The fourth reproduction this PR caughtwas in
.github/workflows/drift-guard.yml. clang-tidy would have reported noneof the three. Conversely this gate reports neither of the two escaped literals
clang-tidy flags. Both gates stay.
Verification of the follow-up
Mutation, not assertion. Each half of the fix was reverted on its own, in
place in the worktree so clang-tidy resolves the identical
.clang-tidychain,and the diagnostics came back:
So both halves are load-bearing and the probe measures something rather than
being satisfied by a dead command.
The fixtures still fail when they should.
test_check_bidi_controls.sh,all 15 cases, on the rebased tree:
Also green locally after the rebase:
check_bidi_controls.py·check_nolint_directives.sh(164 directives repo-wide; and 3 withtests/lint/bidi_controlsnamed explicitly, since the default target prunestests/lint) ·check_tidy_suppression_scope.sh(13 subtracted checks, bothbehavioural probes) ·
check_workflow_job_banners.py(24 banners) ·check_spec_citations.sh·check_ci_clang_pin.sh·check_catch_test_names.sh(2986 names) ·
clang-format 22.1.8 --dry-run -Werrorover all five C++fixtures.
Filed, not folded (follow-up)
tests/lint/fixtures are insideclang-tidy-diff's scope andnothing says so until CI is red. The six pre-existing fixture directories are
green by accident of content, not by design, and the convention that keeps
them green is written down nowhere. Not fixed here: this PR follows the
convention, it does not record or enforce it.
Rebase
Rebased onto
f7c231df(#647), which changed this very job — it now buildstwo
*_autogentargets so the two AUTOMOC self-including sources parse.#646's 84 newly-reachable findings intest_qml_surface.cppandtst_main.cppare not this branch's and are not addressed here; if theyappear on this run, that is why. The tracked-text-file count moves 1242 → 1244
in three comments as a consequence of the new base; the one stamped
at c55ea5b7incheck_bidi_controls.pynames its revision and is left alone.What the follow-up did not verify
hand-off are incomplete.
database, so clang-tidy used the flag set above verbatim, while CI
interpolates a command from a neighbouring
compile_commands.jsonentry.The one claim the fix rests on: none of the five C++ fixtures
#includeanything, so the extra
-I/-Dflags interpolation adds cannot change thefinding set. That is an argument from the fixtures' contents, not a
measurement against the real database — a local
clang-tidy-diff.pyrunneeds the full Qt + ladder configure this job does.
not:
clang-tidy-diffis not runnable without that configure and was not init. Noted rather than glossed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW