Make the SonarCloud reliability gate reproducible, and clear its live findings

.github/workflows/ci.yml

@@ -191,6 +191,15 @@ jobs:
     steps:
       - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262  # v4.4.0
 
+      # The required "SonarCloud Code Analysis" check is the GitHub App's
+      # automatic analysis -- nothing in this repository configures or runs it,
+      # so its rules cannot be read here and its failure arrives only after the
+      # push. This script reproduces the rule that failed the gate and the one
+      # that came with it, needs no toolchain, and runs in about 0.1s: first,
+      # before anything is installed, so the message beats the wait.
+      - name: Check the Sonar reliability rules the gate enforces
+        run: bash scripts/lint-sonar-reliability.sh
+
       - name: Set up Julia
         run: bash scripts/install-ci-julia.sh
 

CONTRIBUTING.md

@@ -32,6 +32,13 @@ Each requirement is checked before the code is read.
    A branch that cannot be re-applied cleanly on top of `main` is not ready.
 3. **One logical change per PR.**
 4. **CI must run and pass.** A PR with no reported checks is not reviewed.
+   The required **SonarCloud Code Analysis** check is a third-party cloud
+   service that nothing in this repository configures, and it fails on a single
+   *bug* — at any severity — in the changed lines. It has no local equivalent, so run
+   `bash scripts/lint-sonar-reliability.sh` before pushing a frontend change
+   and read [docs/sonar-quality-gate.md](docs/sonar-quality-gate.md). Do not
+   silence it by swapping a `sort()` for `localeCompare`; the guard explains
+   why that changes behaviour.
 5. **Write the description:** what the change does, the issues it closes, how to
    run its tests, what it leaves out, and the Julia, R and bun versions you tested
    with.

docs/sonar-quality-gate.md

@@ -0,0 +1,168 @@
+# The SonarCloud check, and why it fails on a frontend change
+
+**Status: diagnosis and local guard.** `scripts/lint-sonar-reliability.sh`
+reproduces, locally and in about 0.1 s, the SonarCloud rule that failed this
+repository's required check and the one that was reported alongside it. It does
+not replace the gate.
+
+## What the check is
+
+The required **SonarCloud Code Analysis** check is the SonarQube Cloud GitHub
+App running **Automatic Analysis**. There is no scanner workflow in
+`.github/workflows/`, no `sonar-project.properties`, and therefore no file in
+this repository that configures it. Two consequences follow, and both have cost
+time:
+
+- **Nothing here can change what it analyses or what it reports.** Automatic
+  analysis ignores `sonar-project.properties`, and `.sonarcloud.properties`
+  accepts only source/test scope, exclusions and duplicate-detection settings —
+  not rule selection, not new-code definition, not issue suppression. Changing
+  the gate is a SonarCloud UI action (Administration → Analysis Method /
+  New Code / Quality Gate), not a pull request.
+- **The only feedback is the check itself.** No lint step runs in CI, so a
+  finding appears after a push, about 90 seconds later, on a third-party
+  dashboard.
+
+## What it enforces
+
+The project's gate is Sonar's built-in **Sonar way**, with the coverage
+condition absent (automatic analysis reports no coverage). Measured from
+`/api/qualitygates/project_status` on PR #224:
+
+| Condition                | Threshold | What drives it                           |
+| ------------------------ | --------- | ---------------------------------------- |
+| `new_reliability_rating` | ≤ A       | the worst **bug** on new code             |
+| `new_security_rating`    | ≤ A       | the worst **vulnerability** on new code   |
+| `new_maintainability_rating` | ≤ A   | the **technical-debt ratio** (< 5% is A)  |
+| `new_duplicated_lines_density` | ≤ 3% | duplicate lines                      |
+| `new_security_hotspots_reviewed` | = 100% | hotspots reviewed                   |
+
+The two ratings do not work the same way, and the difference is the whole
+failure mode. **Reliability is worst-severity, not a count**: a single bug of
+`MINOR` or above drops the rating (`MINOR` → B, `MAJOR` → C, `CRITICAL` → D,
+`BLOCKER` → E), and any rating above A fails the gate. **Maintainability is a
+ratio**, so a few code smells are usually free: measured on PR #233, two
+`MAJOR` code smells and 10 minutes of debt left the rating at `A` and the gate
+green.
+
+So the gate is failed by **one bug**, not by untidiness. That is exactly what
+happened: PR #224's single `CRITICAL` bug (`typescript:S2871`) put reliability
+at `D`. Its two `MINOR` `typescript:S7758` findings are classified as *code
+smells*, not bugs — they contributed 10 minutes of debt and were never what
+failed the check.
+
+"New code" is the pull request's changed lines. On `main` it is a **three-day
+sliding window**, so a finding merged into `main` keeps `main` red for three
+days and re-running the analysis does not clear it.
+
+## The two rules SonarCloud reported on PR #224
+
+### `typescript:S2871` — a `sort()` with no compare function
+
+*"Provide a compare function that depends on `String.localeCompare`, to reliably
+sort elements alphabetically."* Reported as a **CRITICAL** bug with a HIGH
+reliability impact, on:
+
+- `frontend/src/api/ijsonJcs.ts:53` in PR #224 (the check under discussion);
+- `frontend/src/views/CompositionsView.tsx:23` and `:28` on `main` (open since
+  July 2026).
+
+**The rule's own suggestion is wrong here.** `localeCompare` applies locale
+collation; a bare `sort()` applies UTF-16 code unit order. They disagree —
+`['z', 'ä'].sort()` gives `['z', 'ä']`, `localeCompare` gives `['ä', 'z']`:
+
+```
+node> ['z','ä','A','a','Z','é','e']
+default    : A, Z, a, e, z, ä, é
+byCodeUnit : A, Z, a, e, z, ä, é     same
+localeCompare: a, A, ä, e, é, z, Z   different
+```
+
+For a sorted identifier list that is a silent behaviour change; for RFC 8785
+canonical JSON, which defines member order as UTF-16 code unit order, it is a
+correctness bug that would break canonicalisation *while making the check go
+green*. The fix that satisfies both the rule and the specification is to pass
+the comparator explicitly:
+
+```ts
+import { byCodeUnit } from '../utils/text'
+Object.keys(library.filters).sort(byCodeUnit)
+```
+
+`byCodeUnit` lives in `frontend/src/utils/text.ts`, is tested in
+`frontend/src/utils/text.test.ts`, and is byte-for-byte the same ordering as the
+bare `sort()` it replaces.
+
+### `typescript:S7758` — `charCodeAt` where `codePointAt` is wanted
+
+*"Prefer `String#codePointAt()` over `String#charCodeAt()`."* Reported as
+**MINOR**, but as a **code smell**, not a bug — so it does not move the
+reliability rating and would not, on its own, have failed the gate. It fired
+twice on `ijsonJcs.ts` (`:8`, `:10`) because the I-JSON validator walked UTF-16
+*units* to reject unpaired surrogates. It is still worth fixing, and this
+repository now reports none, but it is not the emergency it looks like.
+
+The rewrite iterates by code point:
+
+```ts
+for (let i = 0; i < value.length; i++) {
+  const code = value.codePointAt(i)!
+  if (code >= 0xd800 && code <= 0xdfff) throw new TypeError(/* unpaired surrogate */)
+  if ((code & 0xfffe) === 0xfffe || (code >= 0xfdd0 && code <= 0xfdef)) throw new TypeError(/* noncharacter */)
+  if (code > 0xffff) i++   // skip the low half of a pair
+}
+```
+
+`codePointAt` returns a surrogate's own value only when it is unpaired, so one
+test covers lone high and lone low surrogates; a valid pair arrives as the
+combined code point and its low half is skipped. This was verified against the
+original implementation over 2,628,290 inputs — every single BMP unit alone and
+sandwiched between ASCII, every high×low surrogate pair, every noncharacter in
+every plane, and 400,000 random strings from a hostile alphabet — with zero
+disagreements.
+
+**This rule is sometimes simply wrong** and must not be applied blindly:
+`frontend/src/tree/model.ts` hashes tip names with FNV-1a, which is defined over
+UTF-16 code units; `codePointAt` would merge surrogate pairs and change every
+tip key. That file is listed in `CHARCODE_ALLOWED` in the guard script, with its
+reason, rather than suppressed with `// NOSONAR` — a suppression still leaves
+the finding counted, and `typescript:S1291` ("Track uses of NOSONAR") would add
+a maintainability finding of its own.
+
+## Reproducing it locally
+
+```
+bash scripts/lint-sonar-reliability.sh
+```
+
+Exits 0 clean, 1 on findings, 2 outside the repository. In GitHub Actions the
+findings are also emitted as `::error` annotations, so they are readable from
+the API. The `Julia tests` job runs it first, before any toolchain is installed.
+
+It is a **guard, not a linter**. It covers S2871 and S7758 because those are the
+rules that have failed the required check; it cannot cover the rest of the Sonar
+TypeScript profile. If a new rule bites, add it here *and* to this page, with
+the measurement that justifies it — the point is that each addition is a
+decision, not that the script encodes the profile.
+
+## Why this keeps happening
+
+1. **No local feedback.** No ESLint, no lint step, no configuration file. The
+   gate is a cloud service, so the first signal is the red check.
+2. **One bug is fatal, and a bug is a classification, not a severity.** A
+   single `MINOR` bug fails the reliability rating as surely as a `BLOCKER`
+   one. Whether a rule reports a bug or a code smell is Sonar's decision, and
+   nothing in the repository states it — `sort()` without a comparator is a
+   bug; `charCodeAt` is a smell.
+3. **The suggested fix is dangerous.** S2871's `localeCompare` suggestion is
+   wrong for canonical JSON and for stable identifier ordering, so the obvious
+   fix trades a red check for a silent behaviour change. `byCodeUnit` exists so
+   the correct fix is also the easy one.
+4. **The new-code window is invisible.** Three days on `main`; PR diff on a
+   pull request; neither is stated anywhere in the repository.
+
+Points 1, 3 and 4 are what the guard and this page address. Point 2 is a
+SonarCloud UI setting: the built-in gate's reliability condition is
+`new_reliability_rating > 1`, so raising that threshold to `2` would tolerate a
+rating B (one `MINOR` bug) instead of blocking on it. That is a custom quality
+gate, and it cannot be created from this repository.

frontend/src/tree/model.test.ts

@@ -5,17 +5,18 @@ import {
   buildDisplay, foldOps, indexTree, matchOps, midpointOp, parseJplace, parseNewick,
   pointOnBranch, stateOps, toNewick, type SourceTree, type TreeOp, type ViewState,
 } from './model'
+import { byCodeUnit } from '../utils/text'
 
 const splitSet = (src: SourceTree) => new Set(indexTree(src).splitEdge.keys())
 const byName = (src: SourceTree, name: string) => src.nodes.findIndex(n => n.name === name)
 const stateJson = (st: ViewState) => JSON.stringify({
   root: st.root,
-  collapsed: [...st.collapsed].sort(),
-  rotated: [...st.rotated].sort(),
-  names: [...st.names].sort(),
-  nameSides: [...st.nameSides].sort(),
-  styles: [...st.styles].sort(),
-  support: [...st.support].sort(),
+  collapsed: [...st.collapsed].sort(byCodeUnit),
+  rotated: [...st.rotated].sort(byCodeUnit),
+  names: [...st.names].sort(byCodeUnit),
+  nameSides: [...st.nameSides].sort(byCodeUnit),
+  styles: [...st.styles].sort(byCodeUnit),
+  support: [...st.support].sort(byCodeUnit),
 })
 
 describe('parseNewick', () => {

frontend/src/utils/text.test.ts

@@ -1,7 +1,32 @@
 // © 2026 Joshua Benjamin Jewell. All rights reserved.
 // Licensed under the GNU Affero General Public License version 3 (AGPLv3).
 import { describe, expect, test } from 'bun:test'
-import { splitLines, stripHtmlTags } from './text'
+import { byCodeUnit, splitLines, stripHtmlTags } from './text'
+
+describe('byCodeUnit', () => {
+  test('orders exactly as a comparison-less sort() does', () => {
+    // The point of the helper: it is sort()'s default ordering, made explicit.
+    // If it ever drifts from that default, every caller migrated off a bare
+    // sort() has silently changed behaviour -- so the expected order is written
+    // out in full rather than recomputed with another sort implementation.
+    const values = ['b', 'A', 'a', 'B', 'Z', 'z', 'é', 'ä', '0', '_']
+    expect([...values].sort(byCodeUnit))
+      .toEqual(['0', 'A', 'B', 'Z', '_', 'a', 'b', 'z', 'ä', 'é'])
+  })
+
+  test('is not locale-collated, which is why localeCompare is not a substitute', () => {
+    // Code units put z (U+007A) before ä (U+00E4); a locale collator does not.
+    // The locale is pinned so the assertion does not depend on the runner's LANG.
+    expect(['ä', 'z'].sort(byCodeUnit)).toEqual(['z', 'ä'])
+    expect(['ä', 'z'].sort((a, b) => a.localeCompare(b, 'en'))).toEqual(['ä', 'z'])
+  })
+
+  test('is case-sensitive and total, so equal strings compare equal', () => {
+    expect(byCodeUnit('A', 'a')).toBeLessThan(0)
+    expect(byCodeUnit('a', 'A')).toBeGreaterThan(0)
+    expect(byCodeUnit('same', 'same')).toBe(0)
+  })
+})
 
 describe('stripHtmlTags', () => {
   test('removes the markup Plotly allows in a title', () => {

frontend/src/utils/text.ts

@@ -1,6 +1,30 @@
 // © 2026 Joshua Benjamin Jewell. All rights reserved.
 // Licensed under the GNU Affero General Public License version 3 (AGPLv3).
 
+/**
+ * Compare two strings by UTF-16 code unit: the ordering `Array.prototype.sort`
+ * applies when it is handed no comparator, written out so the intent is
+ * visible at the call site.
+ *
+ * It exists because a comparison-less `sort()` is reported as a BUG by
+ * SonarCloud's `typescript:S2871`, and the repository's required "SonarCloud
+ * Code Analysis" check rates reliability on the worst bug in the changed lines:
+ * one `CRITICAL` bug is rating D, and anything above A fails the gate. That is
+ * a classification Sonar makes, not a severity a reviewer would recognise — the
+ * compiler is perfectly happy with the bare call.
+ *
+ * The rule's own suggestion, `(a, b) => a.localeCompare(b)`, is NOT equivalent:
+ * it applies locale collation, so `['z', 'ä'].sort(byCodeUnit)` keeps `z` first
+ * while `localeCompare` moves `ä` in front of it. For a list of identifiers
+ * that is a silent behaviour change, and for RFC 8785 canonicalisation it is a
+ * correctness bug, so the code-unit comparison is spelled out instead.
+ */
+export function byCodeUnit(a: string, b: string): number {
+  if (a < b) return -1
+  if (a > b) return 1
+  return 0
+}
+
 /**
  * Turn the contents of a multi-line textarea into a list of values: one per
  * line, trimmed, with blank lines dropped. The shape every "paste a list

frontend/src/views/CompositionsView.tsx

@@ -10,6 +10,7 @@ import { NameDialog } from '../components/NameDialog'
 import { FilterEditor } from '../components/FilterEditor'
 import { CategorySetEditor } from '../components/CategorySetEditor'
 import type { CompositionFilter, CompositionSet } from '../api/types'
+import { byCodeUnit } from '../utils/text'
 
 export function CompositionsView() {
   const toast = useToast()
@@ -20,12 +21,12 @@ export function CompositionsView() {
   const [showNewSet, setShowNewSet] = useState(false)
 
   const filterNames = useMemo(
-    () => (library ? Object.keys(library.filters).sort() : []),
+    () => (library ? Object.keys(library.filters).sort(byCodeUnit) : []),
     [library],
   )
 
   const setNames = useMemo(
-    () => (library ? Object.keys(library.sets).sort() : []),
+    () => (library ? Object.keys(library.sets).sort(byCodeUnit) : []),
     [library],
   )