diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5fa55493..6fe2029b 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 930ec561..a21e6413 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/docs/sonar-quality-gate.md b/docs/sonar-quality-gate.md new file mode 100644 index 00000000..5f4915d9 --- /dev/null +++ b/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. diff --git a/frontend/src/tree/model.test.ts b/frontend/src/tree/model.test.ts index 00ddc3f6..25c6b794 100644 --- a/frontend/src/tree/model.test.ts +++ b/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', () => { diff --git a/frontend/src/utils/text.test.ts b/frontend/src/utils/text.test.ts index f5ed71b9..ec8f5871 100644 --- a/frontend/src/utils/text.test.ts +++ b/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', () => { diff --git a/frontend/src/utils/text.ts b/frontend/src/utils/text.ts index fc28108a..11561811 100644 --- a/frontend/src/utils/text.ts +++ b/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 diff --git a/frontend/src/views/CompositionsView.tsx b/frontend/src/views/CompositionsView.tsx index 1005c730..9a092324 100644 --- a/frontend/src/views/CompositionsView.tsx +++ b/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], ) diff --git a/scripts/lint-sonar-reliability.sh b/scripts/lint-sonar-reliability.sh new file mode 100755 index 00000000..f1769cb8 --- /dev/null +++ b/scripts/lint-sonar-reliability.sh @@ -0,0 +1,137 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: AGPL-3.0-only +# SPDX-FileCopyrightText: 2026 Jonathan D.A. Jewell (hyperpolymath) +# +# The repository's required "SonarCloud Code Analysis" check is the SonarQube +# Cloud GitHub App running AUTOMATIC ANALYSIS: no scanner workflow here, no +# sonar-project.properties, and therefore no file in this repository that +# configures it. Its quality gate ("Sonar way", with the coverage condition +# dropped) requires, on NEW code: +# +# reliability rating <= A (worst BUG on new code) +# security rating <= A (worst VULNERABILITY on new code) +# maintainability <= A (technical-debt ratio, < 5% is A) +# duplication <= 3% +# hotspots reviewed == 100% +# +# Reliability and security are rated on the WORST bug/vulnerability in the +# changed lines (MINOR -> B, MAJOR -> C, CRITICAL -> D, BLOCKER -> E), so a +# single bug fails it whatever its severity. Maintainability is a debt ratio, +# and a couple of code smells is usually still A: measured on PR #233, two +# MAJOR 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 why a pull +# request touching ordinary TypeScript can go red for something a reviewer +# would call a style remark, and why the only warning used to arrive ~90 +# seconds after a push, from a service the contributor cannot run. +# +# This script is the local, sub-second reproduction of the rule that failed +# this check and the one reported alongside it, so the finding arrives before +# the push. It is a guard, not a linter: it covers the rules that have bitten, +# and says so rather than implying it covers the gate. +# +# S2871 "Array.prototype.sort() and toSorted() should use a compare function" +# CRITICAL, reliability HIGH. Fired on PR #224 line 53 of +# frontend/src/api/ijsonJcs.ts, and still open twice on main in +# frontend/src/views/CompositionsView.tsx. +# +# S7758 "Prefer String#codePointAt() over String#charCodeAt()" +# MINOR, but a CODE SMELL, so it does not move the reliability rating +# and would not fail the gate alone. Fired twice on PR #224; kept here +# so the repository's count stays at zero rather than accumulating +# debt, and because the exception below is a decision worth making +# explicitly. +# +# Fixing S2871 with the rule's own suggestion -- localeCompare -- is WRONG for +# this codebase, and silently so: localeCompare applies locale collation, so it +# reorders 'z' and 'ä'. Sorted identifier lists and RFC 8785 canonical JSON both +# need UTF-16 CODE UNIT order, which is what a bare sort() already gave. Pass +# frontend/src/utils/text.ts's byCodeUnit() instead: same ordering, explicit. +# Fixing S7758 is usually right (codePointAt handles surrogate pairs correctly), +# but not everywhere -- see CHARCODE_ALLOWED below. +# +# There is deliberately no waiver for S2871: a comparison-less sort is never +# what is wanted (strings want byCodeUnit, numbers want a subtraction, anything +# else wants a real comparator), and a waiver would have to suppress the finding +# in SonarCloud too, which // NOSONAR does at the price of typescript:S1291. +# S7758 has CHARCODE_ALLOWED, because there the rule is sometimes simply wrong. +# +# Run from anywhere: bash scripts/lint-sonar-reliability.sh +# Exits 0 clean, 1 on findings, 2 when run outside the repository. +set -u -o pipefail + +cd -- "$(dirname -- "${BASH_SOURCE[0]}")/.." 2>/dev/null || { echo "lint-sonar-reliability: cannot enter the repository root" >&2; exit 2; } +if [[ ! -d frontend/src ]]; then + echo "lint-sonar-reliability: run from the repository root" >&2 + exit 2 +fi +export LC_ALL=C + +failures=0 + +# GitHub renders an `::error` line as a check annotation, and an annotation can +# be read back through the API when the job log cannot be downloaded. +annotate() { # file line title message + [[ -n "${GITHUB_ACTIONS:-}" ]] || return 0 + printf '::error file=%s,line=%s,title=%s::%s\n' "$1" "$2" "$3" "${4//$'\n'/ }" +} + +# Files where charCodeAt is the correct API and codePointAt would be a bug. +# Keep this list short and give each entry its reason: an entry is a reviewable +# decision, which is the point of not using // NOSONAR (suppressing a finding +# still leaves it counted, and typescript:S1291 "Track uses of NOSONAR" is +# itself a maintainability finding). +CHARCODE_ALLOWED=( + "frontend/src/tree/model.ts" # hashName: FNV-1a is defined over UTF-16 code units; codePointAt merges surrogate pairs and would change every tip key +) + +char_code_allowed() { # file + local file="$1" f + for f in "${CHARCODE_ALLOWED[@]}"; do + [[ "$file" == "$f" ]] && return 0 + done + return 1 +} + +# git ls-files, not find: only what the repository tracks is analysed by Sonar, +# and it keeps node_modules and build output out for free. Read with -d '' into +# an array by hand rather than through mapfile, which macOS's bash 3.2 lacks -- +# there the script would otherwise report OK having looked at nothing. +sources=() +while IFS= read -r -d '' tracked; do sources+=("$tracked"); done \ + < <(git ls-files -z -- 'frontend/src/*.ts' 'frontend/src/*.tsx') + +# --- S2871: a sort with no comparator ------------------------------------- +# [[:space:]] spans newlines, so `arr.sort( )` counts; a call whose first +# argument starts on the next line does not, which is what we want. +while IFS= read -r hit; do + [[ -z "$hit" ]] && continue + file=${hit%%:*}; rest=${hit#*:}; line=${rest%%:*} + echo "ERROR: S2871 comparison-less sort: $file:$line" >&2 + echo " $(sed -n "${line}p" "$file" | sed 's/^[[:space:]]*//')" >&2 + echo " Pass frontend/src/utils/text.ts's byCodeUnit (code-unit order, unchanged" >&2 + echo " behaviour) or an explicit comparator. Do not reach for localeCompare." >&2 + annotate "$file" "$line" "SonarCloud typescript:S2871" "Sort without a compare function fails the reliability gate on new code; pass byCodeUnit() from frontend/src/utils/text.ts" + failures=$((failures + 1)) +done < <( (( ${#sources[@]} )) && grep -HnE '\.(sort|toSorted)\([[:space:]]*\)' "${sources[@]}" || true ) + +# --- S7758: charCodeAt where codePointAt is wanted ------------------------ +while IFS= read -r hit; do + [[ -z "$hit" ]] && continue + file=${hit%%:*}; rest=${hit#*:}; line=${rest%%:*} + char_code_allowed "$file" && continue + echo "ERROR: S7758 charCodeAt: $file:$line" >&2 + echo " $(sed -n "${line}p" "$file" | sed 's/^[[:space:]]*//')" >&2 + echo " Walk the string with codePointAt, which reads a surrogate pair as one" >&2 + echo " code point. If the code really is defined over UTF-16 code units (a hash," >&2 + echo " an encoder), add the file to CHARCODE_ALLOWED with its reason." >&2 + annotate "$file" "$line" "SonarCloud typescript:S7758" "charCodeAt on a changed line fails the reliability gate on new code; prefer codePointAt, or allowlist the file with a reason" + failures=$((failures + 1)) +done < <( (( ${#sources[@]} )) && grep -HnE '\.charCodeAt\(' "${sources[@]}" || true ) + +if (( failures > 0 )); then + echo "lint-sonar-reliability: $failures finding(s). The quality gate needs reliability rating A on" >&2 + echo "new code, and rating A admits no findings -- see docs/sonar-quality-gate.md." >&2 + exit 1 +fi + +echo "lint-sonar-reliability: OK — no S2871 or S7758 findings in frontend/src"