ci: manual CodeQL dispatch + consolidated full-repo findings - #423
Merged
Conversation
Lets a full-repo CodeQL run be triggered on demand from the Actions tab instead of waiting for a push to main or the weekly schedule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (75.86%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
==========================================
- Coverage 87.30% 87.27% -0.03%
==========================================
Files 41 41
Lines 3837 3828 -9
Branches 986 978 -8
==========================================
- Hits 3350 3341 -9
Misses 283 283
Partials 204 204 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Owner
Author
|
Update: alert #244 ( 78 findings remain from the list above (the 79 minus #244). |
…Builder - Wrap the filename-derived second argument to Path.Combine in Path.GetFileName() in both output-path builders, so an unexpected path separator or rooted segment can never make Path.Combine silently drop the output directory (cs/path-combine, alerts #249/#250). - IsOutputUpToDate/IsGeocodingOutputUpToDate: both had a foreach loop that was really an Any() check (early-return on first match), and IsGeocodingOutputUpToDate had a separate foreach that was really a Select+Max fold. Replaced both with the LINQ equivalents (cs/linq/missed-where, alerts #13/#14; cs/linq/missed-select, alert #17). Left the top-level catch (Exception) in Main alone (cs/catch-of-all-exceptions, alert #275): it's the standard CLI entry-point idiom, logs to stderr and returns a non-zero exit code rather than swallowing anything. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ineGeocoder MetadataFilter.cs: - ComputeComplement's two filter-and-collect loops rewritten with .Where() (cs/linq/missed-where, alerts #10/#11). - ShouldDrop: TryGetValue instead of ContainsKey+indexer, avoiding the double dictionary lookup (cs/inefficient-containskey, alert #25). PhoneNumberOfflineGeocoder.cs: GetCountryNameForNumber's loop wasn't a pure filter (it early-returns once a *second* valid region turns up), so a naive .Where().ToList() would lose that short-circuit and evaluate IsValidNumberForRegion for every remaining region needlessly. .Where().Take(2) preserves the same lazy stopping point while still expressing the filter explicitly (cs/linq/missed-where, alert #12). Left PrefixFileReader.cs's LoadFileNamesFromManifestResources alone (cs/linq/missed-select, alert #15): it's a multi-step parse with three early-continue guard clauses and dictionary mutation, not a pure map; forcing it into .Select() would reduce clarity, not improve it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ingBuilder - 9 spots created a match-iterator with manual .Dispose() after assertions that can throw on failure, leaking the enumerator on the failure path. Switched to using/using-var, in some cases splitting a method into scoped blocks so each of two sequential enumerators is still disposed promptly, matching the original ordering (cs/dispose-not-called-on-throw, alerts #361-#369). - FindMatchesInContexts: two foreach loops that immediately mapped context -> text and never used context again, rewritten as .Select() (cs/linq/missed-select, alerts #19/#20). - EnsureTermination: the per-iteration StringBuilder was write-only — appended to but never read, returned, or asserted against. The method's whole point (per its doc comment) is forcing full enumeration to confirm it terminates; the StringBuilder was dead weight, not an allocation to hoist out of the loop (cs/stringbuilder-creation-in-loop, alert #24). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…MapFromString
5 cases used a leftover try { ...; Assert.True(false); } catch
(Exception) { } pattern where every other case in the same method
already uses the cleaner Assert.Throws<Exception>(() => ...) idiom for
the exact same kind of assertion. Made these 5 consistent with the
rest (cs/catch-of-all-exceptions, alerts #279-#283).
Left TestEquals_WhenNull_ReturnsFalse alone (cs/null-argument-to-equals,
alert #370): MetadataFilter.Equals uses `obj is not MetadataFilter
other` pattern matching, which handles a null argument safely (no
NRE) - the test is correctly verifying that exact contract, not an
accidental risky Equals(null) call.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ataFromXml Same simplification as the TestMetadataFilter commit: 3 "should throw" assertions used a manual try/Assert.True(false)/catch(Exception) block instead of the standard xUnit Assert.Throws<Exception>(...) idiom (cs/catch-of-all-exceptions, alerts #276-#278). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…erToTimeZonesMapper - numbers and MapTestData are never reassigned; marked readonly (cs/missed-readonly-modifier, alerts #271/#272). - ContainsKey+indexer double lookups replaced with TryGetValue (cs/inefficient-containskey, alerts #29/#30). - Three foreach loops that immediately mapped pn -> a per-number result and never touched pn again, rewritten with .Select() (cs/linq/missed-select, alerts #21-#23). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ampleNumbers - TestBuildPrefixMapFromBin.cs: ContainsKey+indexer double lookups replaced with TryGetValue in both round-trip assertions (cs/inefficient-containskey, alerts #27/#28). - TestExampleNumbers.cs: TestGlobalNetworkNumbers' loop immediately mapped callingCode -> exampleNumber and never used callingCode again, rewritten with .Select() (cs/linq/missed-select, alert #18). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Final update: all 76 findings from the list above (79 minus the 3
Skipped, with why (left as comments in the relevant commits too):
Every fix that touched library code (not test-only) was verified against the upstream Java source where relevant, built across all three TFMs (netstandard2.0/net8.0/net10.0), and the full test suite (450 tests) was run after each batch — all green throughout. |
Removes the redundant costForRegion local by folding the mapping into the foreach's source sequence; the fold/early-return logic is unchanged. Addresses CodeQL cs/linq/missed-select (alert #16). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The loop's outer condition was a pure filter (elements failing it are skipped entirely, no side effect); folding it into .Where() on the source sequence makes that explicit without changing which lengths get added or throw. Addresses CodeQL cs/linq/missed-where (alert #9). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
int.GetHashCode() returns the int itself, so XOR-ing Start directly is equivalent. Addresses CodeQL cs/useless-gethashcode-call (alert #239). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phonemetadata.cs: all 17 PhoneNumberDesc-typed MergeXxx builder methods share the same if/else-assigns-same-variable shape; collapsed each to a ternary (cs/missed-ternary-operator, alerts #286-#302). Also dropped a redundant int.GetHashCode() call in PhoneMetadata.GetHashCode (cs/useless-gethashcode-call, alert #240). PhoneNumberDesc.cs: dropped two redundant int.GetHashCode() calls in the possibleLength_/possibleLengthLocalOnly_ hash folds (cs/useless-gethashcode-call, alerts #237/#238). int.GetHashCode() returns the int itself, so XOR-ing the value directly is equivalent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
char is unsigned in C#, so `letter >= 0x0000` can never be false. Addresses CodeQL cs/constant-condition (alert #252). Left the six-way Unicode-block range check itself alone (cs/coupled-types alert #371 and cs/complex-condition alert #8 also flagged this area): restructuring it would mean departing from the line-by-line block layout on a hot classification path for a pure readability heuristic, and the PhoneNumberMatcher/PhoneNumberUtil coupling CodeQL flags reflects real, intentional architecture shared with the upstream Java port rather than something a local fix should change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- FormatNumberForMobileDialing: collapse if/else assigning the same variable into a ternary (cs/missed-ternary-operator, alert #284). Left the MX/CL/UZ branch's if/else alone (alert #285): its condition carries ~15 lines of explanatory comments that a ternary would make harder to read, not easier. - MaybeExtractCountryCode: replace the `as object ==` reference-equality hack with an explicit ReferenceEquals call and a comment explaining why value equality would be wrong here (the sentinel check needs to distinguish the specific default-object instance from any region's real prefix that happens to equal the literal text). Same behavior, self-documenting instead of looking like a value-equality bug (cs/reference-equality-with-object, alert #31). - ChooseFormattingPatternForNumber: combine the nested ifs, no behavior change (cs/nested-if-statements, alert #255). - GetExampleNumberForType: the empty catch block was silent by design (falls through to `return null`, matching a metadata-quality issue to "no example number" rather than surfacing it) but had no comment saying so; upstream Java logs the exception here instead, which this port doesn't have a logging story for elsewhere either. Added a comment rather than introducing a logging dependency for one call site (cs/empty-catch-block, alert #248). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Path.Combine -> Path.Join in BuildGeocoding/BuildMetadata output paths (cs/path-combine: Path.Combine can silently drop earlier args if a later one looks absolute; Path.Join has no such behavior) - move the terminates-only comment inside the empty foreach body in EnsureTermination (cs/empty-block-without-comment wants the comment inside the block, not just above it)
2 tasks
📊 Benchmark Results
PR branch
PR base
|
pull Bot
pushed a commit
to LoadsAForks/libphonenumber-csharp
that referenced
this pull request
Aug 27, 2026
Closes out alert twcclegg#247 (NumberParseException.cs) and the broader gap left open in twcclegg#423, which deliberately skipped fixing it there because the bare-prose XML doc-comment style was assumed to be a pervasive, deliberate convention across ~31 files. Auditing the full csharp/PhoneNumbers, PhoneNumbers.MetadataBuilder, PhoneNumbers.Extensions and *.Test trees against origin/main shows nearly all of that convention has since been cleaned up elsewhere; only two files still had genuinely untagged prose: - NumberParseException.cs: the enum-level summary and the two ErrorType member comments (INVALID_COUNTRY_CODE, NOT_A_NUMBER) - PhoneNumberUtil.cs: the helper-initialiser comment above the extension regex constants Each bare block is wrapped in <summary>...</summary> with no change to the existing prose.
This was referenced Aug 31, 2026
Closed
Open
Merged
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
2 participants
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.
Summary
workflow_dispatch:tocodeql.ymlso the full-repo CodeQL scan (already running on every push tomainand weekly via cron) can also be triggered on demand from the Actions tab.mainis posted as a single comment on this PR below, since CodeQL's native PR annotations only ever surface alerts on lines touched by that PR's own diff — there's no built-in way to see the full-repo backlog in one place otherwise.Test plan