Repository navigation
Make the SonarCloud reliability gate reproducible, and clear its live findings - #233
Merged
Merged
Conversation
…epro The required "SonarCloud Code Analysis" check is the SonarQube Cloud GitHub App running Automatic Analysis. Nothing in this repository configures it: no scanner workflow, no sonar-project.properties, and Automatic Analysis ignores that file anyway. Its gate is Sonar's built-in "Sonar way" (coverage condition absent), and it requires reliability rating A on new code, which means ZERO findings of any severity -- a single MINOR is rating B and fails the merge. PR #224 fails it with three findings in a new frontend/src/api/ijsonJcs.ts: CRITICAL typescript:S2871 on a comparison-less Object.keys().sort(), and two MINOR typescript:S7758 on charCodeAt. The rule's own suggested fix for S2871, localeCompare, is wrong for that file: RFC 8785 orders members by UTF-16 code unit, and localeCompare applies collation, so the "fix" would break canonical JSON while turning the check green. This change: * adds byCodeUnit() to frontend/src/utils/text.ts -- the default string ordering, written out, so the correct fix is also the easy one -- with tests that pin the order literally and show localeCompare differs; * uses it at the two comparison-less sorts that are live on main, frontend/src/views/CompositionsView.tsx:23 and :28 (open since July), and in model.test.ts's state normalisation; * adds scripts/lint-sonar-reliability.sh, which reproduces S2871 and S7758 over frontend/src in ~0.1s with no toolchain, and runs first in the test job before anything is installed. Verified against the real files: it reports exactly lines 53, 8 and 10 of PR #224's ijsonJcs.ts and lines 23 and 28 of CompositionsView.tsx, matching the SonarCloud API; * documents the whole thing in docs/sonar-quality-gate.md and points at it from CONTRIBUTING. charCodeAt is allowlisted for frontend/src/tree/model.ts, whose FNV-1a hash is defined over UTF-16 code units, with the reason in the script. Suppressing it with // NOSONAR would leave the finding counted and add a typescript:S1291 maintainability finding of its own. Checks: bash -n and the script itself; bun test src (69 pass); tsc --noEmit; bun run build. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Fork governance
🟣 critical · commit
Hypatia scan: 140 finding(s)
panic-attack assail: 19 finding(s)
pons asinorum: 10 finding(s)
rhodibot (RSR compliance): not_run — enrolled, but no report-back was received
panicbot (panic recovery, error handling): not_run — enrolled, but no report-back was received
echidnabot (formal verification: proofs/): not_run — enrolled, but no report-back was received
seambot (integration health: API routes): not_run — enrolled, but no report-back was received
finishingbot (release readiness): not_run — enrolled, but no report-back was received
accessibilitybot (WCAG on the React UI): not_run — enrolled, but no report-back was received
glambot (presentation quality): not_run — enrolled, but no report-back was received
cipherbot: not_applicable — no cryptography is implemented in this repository
sustainabot: not_applicable — fleet adapter slot reserved; no adapter exists yet (gitbot-fleet README)
Scope: static findings reported by each row's tool at the named commit. Purple, red, yellow and blue grade the worst finding present; orange is evidence missing, malformed or a scanner that did not run; grey is an explicit scope exclusion with its reason. No row is a security certification. report.json, badge.json and the raw findings are in the verdict-grid artifact on the linked run. |
The first commit described rating A as "zero findings of any severity". The analysis of this very PR disproves it: two MAJOR code smells (typescript:S3358 in text.ts, shelldre:S7679 in the guard) and 10 minutes of technical debt left new_maintainability_rating at 1.0 (A), and the gate passed. Measured from /api/measures and /api/qualitygates/project_status: PR #233 new_bugs=0, new_code_smells=2, new_technical_debt=10min new_reliability_rating=1.0, new_maintainability_rating=1.0 So reliability and security are rated on the WORST bug/vulnerability on new code (MINOR->B, MAJOR->C, CRITICAL->D, BLOCKER->E) -- one bug fails it at any severity -- while maintainability is a debt ratio, where a handful of smells stays A. PR #224 fails on one CRITICAL bug (typescript:S2871); its two typescript:S7758 findings are code smells and were never what failed it. This makes the diagnosis sharper, not softer: a single comparison-less sort() is the entire cause. Corrected in docs/sonar-quality-gate.md, the guard's header, the byCodeUnit doc comment and CONTRIBUTING, and the two new smells this PR introduced are fixed rather than left standing. Checks: bash -n and the guard; bun test src (69 pass); tsc --noEmit. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8d5b4dd5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 ) |
There was a problem hiding this comment.
When an empty call is formatted across lines, such as values.sort(\n), grep processes each line independently, so [[:space:]]* cannot bridge the newline and this check reports success even though Sonar's syntax-based S2871 rule still reports the call. This lets the new CI guard pass while the required cloud gate fails; use a multiline-aware or syntax-aware matcher for comparator-less calls.
Useful? React with 👍 / 👎.
| while IFS= read -r hit; do | ||
| [[ -z "$hit" ]] && continue | ||
| file=${hit%%:*}; rest=${hit#*:}; line=${rest%%:*} | ||
| char_code_allowed "$file" && continue |
There was a problem hiding this comment.
Because char_code_allowed is checked only by filename, every future charCodeAt added anywhere in frontend/src/tree/model.ts is skipped, not just the documented FNV-1a use. Sonar will still inspect a newly added or changed call in that file and fail the required gate, while this guard reports success; scope the exception to the known hashing occurrence rather than the entire file.
Useful? React with 👍 / 👎.
| 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 ) |
There was a problem hiding this comment.
Because this searches raw source text rather than parsed calls, any comment, documentation example, regular expression, or string literal containing .charCodeAt( is counted as a finding even though Sonar ignores it. For example, adding a comment that explains why another API is preferable would make this CI step fail indefinitely; use a syntax-aware check or otherwise exclude comments and literals before matching.
Useful? React with 👍 / 👎.
… with it Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c455d14f28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 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') |
There was a problem hiding this comment.
When a comparison-less sort() or charCodeAt() is added to the tracked frontend/vite.config.ts, this pathspec omits it, so the local guard reports success while Sonar can still report the changed line. The repository has no source-scope configuration, and Sonar's default source scope is the project base directory, so the guard should scan all tracked TypeScript/JavaScript files in the analysis scope rather than only frontend/src (SonarQube Cloud analysis parameters).
Useful? React with 👍 / 👎.
Comment on lines
+100
to
+101
| while IFS= read -r -d '' tracked; do sources+=("$tracked"); done \ | ||
| < <(git ls-files -z -- 'frontend/src/*.ts' 'frontend/src/*.tsx') |
There was a problem hiding this comment.
When this script is run from an exported source tree without .git, or when git ls-files otherwise fails, the process-substitution failure is never checked: Git prints a fatal error, sources remains empty, and the script exits 0 with an OK message. This contradicts the documented exit code 2 outside a repository and can make the guard silently inspect nothing, so source enumeration must be performed in a way whose exit status is checked before reporting success.
Useful? React with 👍 / 👎.
| 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" |
There was a problem hiding this comment.
When S7758 is the only hit, this annotation tells contributors that charCodeAt fails the reliability gate, but the script's own header and the new documentation establish that S7758 is a code smell which does not affect the reliability rating and did not fail the measured cloud gate. The local job intentionally blocks it to keep the smell count at zero, so the diagnostic should describe that stricter repository policy rather than falsely attributing the failure to Sonar's reliability gate.
Useful? React with 👍 / 👎.
This was referenced Oct 23, 2026
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
…ings (#234) This is #227's contribution, re-landed on current `main` with its review findings fixed. ## Why not merge #227 directly #227's branch point is `8b76b71`, which predates #222, #229 and #233 on `main`, so its diff *also* reverted: - NHX `[&&NHX:B=…]` capture threading in `parse_newick` (#222) - `PLACEMENT_SINGLE_MIN_LWR` handling for single-candidate placements (#222/#233) - the `edge_number` graft walk fix (#229) - `unplaced_reads` in the clade-cumulus diversity payload and its test (#233) - Canonical I-JSON writers (`jcs_json`/`ijson_read`) and the `AbstractString` request boundaries in `routes/trees.jl`, `routes/clade_cumulus.jl`, `src/server/server.jl` Taking the union naively would have regressed all of that, so this branch takes **only** what `main` is genuinely missing. ## What `main` is actually missing `src/server/routes/cladistics.jl` calls `CC.asv_tip_name` (line 123) and `CC.analyse_phylogeny` (line 211), and `bench/cladistics.jl` calls the latter, but neither was defined after the #207/#206 merges — every cladistics build and every compute job died with `UndefVarError`/`MethodError`. Restored here as an indexed study-tree layer alongside the existing `PhyloNode` API: | Symbol | Purpose | | --- | --- | | `IndexedPhyloNode`, `PhylogeneticTree` | parents-precede-children node vector + `tips` index | | `parse_phylogenetic_tree` | strict Newick → `PhylogeneticTree` (quoted/Unicode labels, comments, support labels, non-binary nodes) | | `asv_tip_name` | content-addressed, tool-safe ASV tip names | | `clade_counts` | nodes × samples cumulative roll-up | | `unifrac` | raw samples × ASVs **or** an existing roll-up, unweighted/weighted | | `analyse_phylogeny` | the single call whose NamedTuple the cladistics route writes to `results.json` | The payload matches `CladisticsResult` in `frontend/src/api/types.ts` exactly. ## Codex findings from #227 1. **P1 — `SubString` from the byte buffer.** `_ip_label!` and `_ip_length!` called `SubString(p.bytes, first:last)` on a `Vector{UInt8}`, which has no such method, so every ordinary unquoted label and branch length raised `MethodError`. Labels/lengths are now sliced straight into `String`s; the label return is an explicit `String` instead of a `strip` SubString. 2. **P1 — graft walk read `u.edge_num`.** Already fixed on `main` by #229 and not reintroduced: `edge_num` now only ever names `QueryCandidate`'s field, and the new code touches no `PhyloNode` field. 3. **P2 — Faith PD accepted invalid cumulative counts.** `_faith_pd_indexed` treated negative/`NaN` entries as absent (`> 0` is false) and returned a plausible but underestimated PD. It now validates finite, non-negative counts like `clade_counts` and `unifrac`. 4. **P2 — raw-count UniFrac overload missing.** `unifrac(tree, counts, tip_names; weighted=…)` now exists and delegates through `clade_counts`, matching the adjacent `faith_pd(tree, counts, tip_names)`. ## Also - `bench/cladistics.jl` builds its tree with `parse_phylogenetic_tree` (it calls `analyse_phylogeny`, which needs the indexed type). - Dropped the duplicated `import ..Tools` in `src/pipeline/phylogeny.jl`. - New testset covers the parser (byte-buffer labels, multi-byte/quoted labels, trailing comments, duplicate tips, negative lengths, missing/extra `;`), the roll-up, Faith's PD including the new rejections, both UniFrac variants and the raw-count overload, and the JSON payload with the same row-major conversion the route performs. - `docs/cladistics/clade-cumulus.md` documents the new API. ## Verification - tree-sitter-julia parse of every touched file: clean (the two files over the JS binding's 32 KiB limit were parsed in top-level chunks; the inserted block parses standalone). - No Julia runtime is reachable from the authoring sandbox (julialang-s3 and GitHub release assets are blocked), so the Julia suite is CI on this PR. #227 can be closed once this lands. --------- Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Oct 23, 2026
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
…N with 400 (#244) ## Change and reason Three things, in the order the linked review asked for them. ### 1. PR #228's merge conflicts, resolved (and shown to be a no-op) PR #228 (`arena/7805092e`) branched from `8df1041`; all six files it touched have since been rewritten on `main` by #231, #233, #234, #236, #237 and #239. Resolving the six conflicted files in favour of `main` produces a tree that is **byte-identical** to `main` (`git diff --stat origin/main` is empty), so the merge commit records the resolution and changes no content. Concretely: * `ci.yml` - main publishes a per-assertion digest of every failing validated-statistics test plus a log tail; #228's awk windows are superseded. * `src/analysis/clade_cumulus.jl` - main already reads `PhyloNode.edge_number` in `_graft_jplace_queries`, already sums `rk.subtree_pd` plus the node's own branch, and additionally recomputes an emptied clade from `direct_count`. * `src/server/routes/clade_cumulus.jl` - main already uses `with_results_db` and `String(strip(...))`, and deliberately does **not** renormalise placement like-weights by the retained total: `parse_jplace` already rejects ratios outside [0, 1], a multi-candidate placement already partitions its mass, and dividing a lone 0.6 candidate by its own total would move the 0.4 the tool left unplaced onto the reported edge (#236/#237 report it as `unplaced_reads` instead). * `src/server/routes/validated_statistics.jl` - main validates raw tokens for the whole body after `ijson_read` proves it I-JSON, and streams bounded rows instead of materialising a DataFrame. Both are strict supersets. * `test/unit/*` - main's suites are far larger; the one test #228 had that main lacked ("Nested-clade drag-and-drop counts each active branch once") is restored here. ### 2. Codex P2 on PR #228: leaf subtree PD `_reaggregate_after_move` credits an active leaf with its own incoming branch, so a parent can sum `subtree_pd` alone and count each active edge once. The ordinary builder (`build_node`) hard-coded `0.0` for leaves, so validating a drag-and-drop changed the reported PD of leaves whose topology had not changed and made the proposed tree's metrics incomparable with the original. Both builders now take a leaf's PD from `clade_faith_pd` - the function the internal-node branch already used - so the conventions cannot drift again. Internal nodes and inactive tips are unchanged. ### 3. panic-attack `InputBoundary`: malformed bodies are 400s 35 unguarded `JSON3.read(String(req.body))` call sites across eight route files raised a JSON3 parse error out of the handler, so a truncated or hostile body produced a 500. Every route now parses through `Server._request_json`, which throws the existing `BadRequest`. The helper copies `req.body`, because `String(::Vector{UInt8})` can consume the buffer. Rescuing that turned out to need one more step, which CI caught: Oxygen builds its chain as `custom(serializer(router))` and its serializer catches every handler exception and answers 500 before our middleware runs, so `_bad_request_middleware` never saw the throw. Oxygen's error mapper is the hook inside the serializer, and it is already a dispatch point (`handle_error(::ValidationError)` answers 400). Extending it for `BadRequest` keeps this server's `{"error", "message"}` shape and makes the test's `status == 400` true. ### 4. pons asinorum: tree path building is linear The six `string-concat-in-loop` hits in `TreeShapes.tsx` were real: every node appended to four shared strings inside a `useMemo` over the whole layout, which is quadratic in the node count on a view that renders every node. The fragments are collected per node and joined once; the emitted order is unchanged. ### 5. panic-attack `ProofDrift` and a guarded benchmark baseline The `@test x isa Y` assertions the rule flagged were real tautologies: they passed for any string. The occupancy pair now asserts the text the code emits (`measured 201 KiB above the predicted 101 KiB`) and the evidence round trip asserts the ISO-8601 UTC stamp `save_document` writes. The advanced zero-handling benchmark parsed its baseline outside any guard, so a truncated baseline surfaced as a JSON3 failure from inside the timing loop; it now names the file it could not read. ## Validation - `cd frontend && npx tsc --noEmit -p .` - clean. - `cd frontend && npx vite build` - clean (chunk-size warning only, pre-existing). - `cd frontend && bun test src` - **72 pass, 0 fail** (11 files). - `bash scripts/check-governance-contract.sh` - **all 20 checks passed**. - `bash scripts/lint-sonar-reliability.sh` - clean. - `bash -n` on every `scripts/*.sh` - clean. - All four workflows parse as YAML; the DEED lints clean with the pinned `deed_lint.js` from `hyperpolymath/standards@75bd5994` (see below). - **Not run locally:** the Julia suite (no Julia runtime in this environment; the two changed Julia files are covered by CI's `Julia tests` job). ## Hypatia and K9/DEED conformance * **DEED** - `MetaManifold-WebUI_chora.deed` lints clean with the exact linter the reusable lane runs (`bun 1-formats/deed/tools/deed_lint.js`). Note the stdlib Python port in the same directory is older and rejects `#u8`; the JS linter the workflow uses accepts it, so the deed is correct as written. * **K9** - this repository has no `.machine_readable/contractiles/` registry or Mustfile, and `governance.yml` does not call `k9-contractile.yml`, so the K9 contractile lane is not in scope here. What *was* wrong is that the deed declared `:report-backs ".gitbot-fleet/report-backs/"` while the directory did not exist; it now does, with the envelope contract documented, and the grid keeps rendering those members orange until a real report-back lands. * **Hypatia** - the 43 critical findings are all `cicd_rules/banned_language_file` for `frontend/src/**/*.ts`. That rule is a repo-side-unfixable policy finding: TypeScript's only carve-outs are narrow path prefixes (`bindings/typescript/`, `.d.ts`, `vite.config.ts`, ...), none of which cover an upstream-authored React frontend, and the rule cannot be suppressed from the repository. Migrating upstream's frontend to AffineScript is an estate project, not a change this fork can make. * **panic-attack / pons asinorum** - most remaining findings are stale or false positives at `main`: `api/events.ts` and `tree/model.ts` already wrap `JSON.parse` in try/catch (hardened in #239), the `DynamicCodeExecution` hits are `RCall.reval` (R source strings, not Julia `eval`), and the `unreachable-after-jump` hits point at plain arrow-function bodies. The two classes that were real - unguarded request-body parsing and quadratic path building - are fixed above. The grid's residual after this PR: Hypatia 45 critical (all `banned_language_file` on upstream TypeScript), 61 high, 35 medium; panic-attack 6 high (all `RCall.reval` false positives), 7 medium; pons asinorum 2 medium (`unreachable-after-jump` on a `useMemo` return and a `useEffect` cleanup, both false positives). ## Scope - [x] Focused on the linked review plus conformance evidence - [x] Does not alter dependencies, lockfiles, generated assets, or vendored files - [x] Secrets, local paths, and run outputs are not committed ## Contribution checks - [x] I followed CONTRIBUTING.md - [x] My commits include `Signed-off-by:` (DCO) and the licence is unchanged --------- Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
PR #224's SonarCloud Code Analysis check fails with D Reliability Rating on New Code. This branch fixes the two comparison-less
sort()calls live onmainunder the same rule, adds a sub-second local reproduction of the rule that failed the gate, and documents why the rule's own suggested fix is wrong here.Root cause
The check is the SonarQube Cloud GitHub App running Automatic Analysis. There is no scanner workflow, no
sonar-project.properties, and no.sonarcloud.properties— so nothing here configures it. Automatic Analysis ignoressonar-project.propertiesin any case, and.sonarcloud.propertiesaccepts only scope/exclusion settings — not rule selection, suppression, or the new-code definition.The project's gate is the built-in Sonar way with the coverage condition absent. Its conditions, measured from
/api/qualitygates/project_status:new_reliability_ratingnew_security_ratingnew_maintainability_ratingnew_duplicated_lines_densitynew_security_hotspots_reviewedReliability is worst-severity, not a count.
MINOR-> B,MAJOR-> C,CRITICAL-> D,BLOCKER-> E, and anything above A fails. So one bug fails the gate at any severity, while a handful of code smells is usually free — maintainability is a ratio. Both directions were measured, on this PR and on #224:new_bugs=0,new_code_smells=2,new_technical_debt=10min-> maintainability A, gate passed.typescript:S2871BUG,CRITICAL-> reliability D, gate failed.PR #224 has exactly three findings, all in its new
frontend/src/api/ijsonJcs.ts:typescript:S2871(CRITICALbug) at line 53, andtypescript:S7758(MINOR, but a code smell) at lines 8 and 10. The singlesort()at line 53 is the entire failure. The two S7758s were never what failed it.Why the obvious fix is wrong
S2871's suggestion is
(a, b) => a.localeCompare(b). ForijsonJcs.tsthat is a correctness bug: RFC 8785 section 3.2.3 orders object members by UTF-16 code unit, andlocaleCompareapplies locale collation. Measured:So the "fix" turns the check green while breaking canonical JSON.
byCodeUnit()is added tofrontend/src/utils/text.tsso the correct fix is also the easy one.The same trap sits under
typescript:S7758:frontend/src/tree/model.ts's FNV-1a tip hash is defined over UTF-16 code units, andcodePointAtwould merge surrogate pairs and change every tip key. That file is allowlisted with its reason in the guard, rather than suppressed with// NOSONAR— which would still leave the finding counted and add atypescript:S1291maintainability finding of its own.What changed
frontend/src/utils/text.ts—byCodeUnit, the default string ordering written out, with the gate and thelocaleComparetrap documented.frontend/src/utils/text.test.ts— three tests: order pinned literally (not recomputed with anothersort),localeCompareshown to differ, comparator total and case-sensitive.frontend/src/views/CompositionsView.tsx— the twotypescript:S2871findings open onmainsince July 2026, at:23and:28.frontend/src/tree/model.test.ts—stateJson's six normalising sorts, so the new guard does not fire on them.scripts/lint-sonar-reliability.sh— reproduces S2871 and S7758 overfrontend/srcin ~0.1 s with no toolchain, emits::errorannotations in Actions, and has no waiver for S2871 because a comparison-less sort is never what is wanted..github/workflows/ci.yml— runs the guard first in theJulia testsjob, before anything is installed. Job name and shape untouched (test/unit/test_install_pins.jlstill passes: no matrix, no interpolation).docs/sonar-quality-gate.md,CONTRIBUTING.md— the measured gate, the new-code definition, both rules, and the fix.Verification
main'sCompositionsView.tsxreports lines 23 and 28. Each was checked by restoring the pre-fix file, then re-checking the fixed tree is clean; it also works from a subdirectory and exits 2 outside a repository.ijsonJcs.tspasses the genuineeslint-plugin-sonarjsrulesonarjs/no-alphabetical-sortunder typed linting (failure reproduced at53:38before, clean after). That file is not in this branch — it belongs to Resolve #221 against main's validated-statistics fixes #224, and the patch is posted there.codePointAtrewrite ofvalidStringwas compared against the original over 2,628,290 inputs — every BMP unit alone and sandwiched, every high x low surrogate pair, every noncharacter in every plane, 400,000 random hostile strings — zero disagreements.bun test src-> 69 pass, 0 fail;tsc --noEmitclean;bun run buildsucceeds.Left out
The gate itself. The built-in condition is
new_reliability_rating > 1; raising the threshold to2would tolerate a rating B (oneMINORbug) instead of blocking on it. That is a custom quality gate in the SonarCloud UI, and no file in this repository can create it.