You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix CodeQL and SonarCloud findings repo-wide - #20
Repo-wide remediation of CodeQL and SonarCloud findings, split into sequential themed batches on this branch.
Fixes applied
CodeQL S2583 (MAJOR BUG): GlyfLocaReader unreachable-code bug (root cause of the SonarCloud reliability-rating gate failure) — removed redundant hasContent flag.
CodeQL cs/complex-block / cs/complex-condition: extracted DecodeSimpleGlyph phases into named helpers; replaced a hand-rolled hex-digit check with char.IsAsciiHexDigit.
CodeQL cs/equality-on-floats / cs/loss-of-precision: converted genuinely-computed float comparisons to tolerance-based checks (11 sites); fixed an integer-overflow-before-cast bug in a test. Left 10 sites unchanged where the comparison is against a caller/spec-supplied literal sentinel with documented intent (e.g. SVG zero-radius arc handling) — documented, not suppressed.
CodeQL cs/local-not-disposed: fixed 230 undisposed MemoryStream sites across the test project (using declarations). Left ~207 Surface disposal sites untouched — Surface.Dispose() is currently a no-op (separate, larger effort), documented not suppressed.
CodeQL cs/linq/missed-where, cs/linq/missed-select: converted 4 manual loops to LINQ .Where/.Select where behavior-preserving; left rasterization/kerning-order-dependent loops unchanged (documented).
CodeQL cs/path-combine: replaced Path.Combine with Path.Join at 23 fixture-test sites to remove the silent-discard-on-rooted-argument risk.
SonarCloud xUnit2032/xUnit2033, CA1859, IDE0028: mechanical style fixes (7 + 9 + 6 sites); 2 additional CA1859 suggestions reverted after they broke compilation or dropped an immutability guard (documented).
SonarCloud S3776 (Cognitive Complexity): judgment-based review of ~21 flagged methods. Refactored 10 where a genuine, valuable decomposition existed; left 11 as-is (inherent domain complexity), each with documented rationale. No refactoring done purely to silence the linter.
SonarCloud S107 (Too Many Parameters): judgment-based review of flagged methods. Refactored 8 internal methods using genuine reusable groupings (reusing existing types like PngHeader, CompositeWorkBuffers, Matrix3x2 where possible); left 8 as-is (including 2 public-API methods, to avoid a breaking change with no genuine clarity gain), each documented.
Explicit constraints followed
No suppressions anywhere (no #pragma, [SuppressMessage], or CodeQL/SonarCloud exclusion config) for any left-as-is finding — every skip is documented with rationale instead.
Single PR for the whole effort, developed as sequential batches on one shared branch.
Complexity/parameter-count findings were treated as "an invitation for closer scrutiny," not an obligation to refactor — see individual batch rationale above.
Validation
Every batch independently verified: pwsh ./build.ps1 (0 warnings/errors, all tests passing across net8.0/net9.0/net10.0) and pwsh ./fix.ps1 (auto-formatters) before each commit. Final state: 4104/4104 tests passing.
🤖 Generated with assistance from GitHub Copilot CLI
- GlyfLocaReader.DecodeCompositeGlyph: remove the dead 'hasContent' flag.
The do-while loop always runs at least once and unconditionally set
hasContent = true, making the final ternary always evaluate its true
branch (S2583) and the initial 'false' assignment dead (IDE0059).
PathBuilder.Build() already returns Path.Empty when no subpath was
ever committed, so builder.Build() alone is equivalent and correct.
- GlyfLocaReader.DecodeSimpleGlyph: extract the contour end-point,
flag-run-length, and coordinate-delta decoding, plus the final
contour-to-Path assembly, into named private helper methods to
address the complex-block finding, and replace the simple-glyph
flag-bit magic numbers with named constants.
- Rgba32.IsHexDigit: replace the multi-operator range-comparison
condition with char.IsAsciiHexDigit, which is exactly equivalent and
removes the complex-condition finding.
No behavior changes; full build and test suite (4104 tests) passes.
- StrokeOutliner: use a small tolerance instead of exact-zero when
guarding the outer/inner ring sign comparison (cs/equality-on-floats);
the areas are computed values, not caller-supplied sentinels.
- PathBuilder.TangentArcTo: keep radius == 0f as an intentional
exact-sentinel check (explicit caller request for 'no arc'), but use a
tolerance for the computed len1/len2 near-coincident-point checks.
- StrokeOutlinerTests: mirror the same tolerance-based area comparison
used in the production fix above.
- SurfaceTests.ComputeExpectedPremultiplied: cast the first operand to
double before multiplying rather than casting the product afterward,
resolving the cs/loss-of-precision finding about a possible integer
multiplication overflow before widening.
Other flagged sites (SvgCodec.ParseDashArray, DashSplitter.IsSolidDash/
LocatePhaseForward/LocatePhaseBackward, GradientEvaluator.BuildPlan,
CornerRoundEffect.Apply, Path.RoundRectangle, SvgArcConverter.ToBeziers)
were reviewed and left unchanged: each compares a value against a
literal zero sentinel that is caller/spec-defined (an explicit radius
of zero, an explicit zero-length dash entry, or two directly
caller-supplied gradient centers/radii), not a computed value prone to
floating drift - converting these to tolerance comparisons would risk
changing documented, intentional behavior without fixing a real bug.
No suppression comments were added; findings judged intentional are
left as documented exact comparisons per review.
Wrap MemoryStream instances that were created but never disposed in using declarations across CanvasNetTests, GifCodecTests, JpegCodecTests, SvgCodecTests, TiffCodecTests, TrueTypeFontTests, and TextRendererTests. Covers both locals assigned via 'var x = new MemoryStream(...)' without using, and MemoryStream temporaries passed inline as call arguments (e.g. JpegCodec.Load(new MemoryStream(bytes))), which CodeQL's cs/local-not-disposed also flags. No test behavior or assertions changed - purely resource-disposal fixes. Verified the codecs' Load/GetInfo APIs eagerly copy/read the input stream and do not retain a reference to it, so wrapping the argument stream in using at the call site is safe.
- StrokeOutliner.SimplifyPoints: convert manual consecutive-duplicate
filtering loop to points.Where(...).ToList() (cs/linq/missed-where).
- SvgCodec.ApplyAlphaToStops: convert manual per-stop transform loop to
stops.Select(...).ToList() (cs/linq/missed-select).
- SvgCodec.ScaleDashArray: convert manual per-entry scaling loop to
dashArray?.Select(...).ToArray() (cs/linq/missed-select).
- DashSplitter.NormalizeDashArray: convert manual identity-copy loop to
dashArray.ToArray() (cs/linq/missed-select).
PathStroker.cs and TextRenderer.cs loops were reviewed but left
unchanged: they are hot-path rendering loops with side-effecting
builder/canvas mutations and/or state that depends on the previous
iteration's result (kerning), so a Select-based rewrite would not be a
safe, behavior-preserving, or clearer simplification.
Replace remaining Path.Combine calls in fixture test helper properties
and inline call sites with Path.Join, which does not silently discard
preceding segments when a later segment happens to look rooted. This
completes the cs/path-combine remediation started by prior agents
(GifFixtureTests, JpegFixtureTests, PngSuiteTests already had a
Path.Join-based ResolveFixturePath helper for filename resolution, but
their AssetsPath/PngSuitePath base-directory properties, plus several
inline PngCodec.Load(Path.Combine(...)) calls, still used
Path.Combine).
- GifFixtureTests: AssetsPath now uses Path.Join
- JpegFixtureTests: AssetsPath/PngSuitePath now use Path.Join; the two
inline PngCodec.Load(Path.Combine(PngSuitePath, ...)) calls now use
the existing ResolveFixturePath helper
- PngSuiteTests: AssetsPath now uses Path.Join
- TiffFixtureTests: AssetsPath/PngSuitePath now use Path.Join; the four
inline PngCodec.Load(Path.Combine(PngSuitePath, ...)) calls now use
the existing ResolveFixturePath helper
- SvgFixtureTests: added a Path.Join-based ResolveFixturePath helper
(this class had none); AssetsPath and FontPath now use Path.Join; all
eight inline SvgCodec.Load(Path.Combine(AssetsPath, ...)) calls now
use ResolveFixturePath
- TrueTypeFontRealFontIntegrationTests: FontPath now uses Path.Join
No test behavior or resulting paths change - every second/third
argument is a compile-time literal fixture file name, never rooted, so
this purely satisfies CodeQL's cs/path-combine structural rule without
altering any test outcome.
Build succeeds with 0 warnings/errors; all 4104 tests pass across
net8.0/net9.0/net10.0.
…0028)
- xUnit2032: replace Assert.IsAssignableFrom<T> with Assert.IsType<T>(obj, exactMatch: false) (3 sites)
- xUnit2033: use the value returned by Assert.Single instead of re-deriving it (4 sites)
- CA1859: narrow private method/out-parameter declared types from interfaces to the
concrete types always produced internally, across DashSplitter, StrokeOutliner,
SvgCodec, Gradient, and PathFiller (9 sites; 2 additional narrow candidates were
reverted after discovering they broke compilation or the DashArray immutability
contract - see report)
- IDE0028: simplify collection initialization to match existing collection-expression
style (6 sites, including DashSplitterTests.cs x4)
Judgment-based S3776 pass: extract genuinely distinct, independently
testable phases where they add real value; leave methods intact where
their branching is inherent to the domain (sequential format parsers,
math case-analysis, tightly-coupled cursor state).
Refactored:
- GlyfLocaReader.DecodeCompositeGlyph: extract ReadComponentOffset/
ReadComponentTransform (distinct binary-format parsing steps).
- GifCodec.Load: extract BlitIndexedFrame (indexed-to-RGBA blit is a
reusable, independently testable operation).
- PngCodec.ProcessChunk: extract ProcessPlteChunk/ProcessTrnsChunk.
- PngCodec.MapSamplesToRgba: extract one mapping helper per PNG color
type (5 independent per-pixel formulas).
- CornerRoundEffect.Apply: extract ComputeWrapAroundCorner.
- DashSplitter.BuildOnIntervals: extract EstimatesExceedBudget
pre-flight cost estimate.
- ScanlineRasterizer.CoverageSweep.MoveNext: extract
ExpireEdgesForRow/ActivateEdgesThroughRow/AccumulateRowCoverage.
- CmapTable.TryParseFormat4/TryParseFormat12: extract the returned
lookup delegate bodies into LookupFormat4/LookupFormat12.
- Path.AccumulateCommandBounds: extract UnionPoints, eliminating a
3x-duplicated flatten-then-union loop.
- PathStroker.Stroke: extract AppendOutlinePolygon.
Left as-is (documented rationale in .agent-logs report):
GifCodec.Load's block dispatch, PngCodec.ValidateChunkLengthBeforeAllocation,
JpegCodec.Decoder.Decode, TiffCodec.ReadTiffImageInfo, SvgCodec.RenderElement,
SvgCodec.ParseStops, DashSplitter.Split, DashSplitter.ExtractIntervalPolyline,
GradientEvaluator.SolveRadialParameter - all inherent sequential
parsers/validators/math case-analysis already well-factored; splitting
further would fragment locality or produce purely mechanical helpers.
No suppression comments added anywhere; every left-as-is site is
documented with rationale in the completion report only.
… (S107)
- StrokeOutliner: introduce StrokeSideGeometry and ArcTessellation record
structs to bundle parameters that already travel together across
AppendOpenJoin, AppendStyledJoin, TryCreateMiter, AppendArcShortest,
AppendArcThrough, and AppendArc; reuse the existing frame-tuple
convention from BuildSegmentFrames for join methods.
- GlyfLocaReader.AppendTransformed: replace 6 individual transform
floats (a,b,c,d,dx,dy) with the existing System.Numerics.Matrix3x2,
matching Path.Transform's established usage.
- Surface: remove the redundant count parameter from
CompositeOverSpanCore/CompositeOverSpanCoreShared (always derivable
from coverage.Length); reuse the existing CompositeWorkBuffers type
in CompositeOverChannel instead of exploding its fields into 5
separate parameters.
- PngCodec.DecodeScanlines: reuse the existing PngHeader record struct
instead of exploding width/height/colorType/bitDepth into individual
parameters.
All other S107 sites (SvgCodec.RenderState/RenderElement/RenderUse,
ScanlineRasterizer.AccumulateSlantedSpan, GifCodec.BlitIndexedFrame,
CmapTable.LookupFormat4, Shapes.StrokeRoundRect, TextRenderer.DrawText,
PathCommand's private constructor) were reviewed and left as-is; see
completion report for per-site rationale.
No suppressions added. Behavior-preserving; verified via full build
and existing test suite (4104 tests passing).
This Where predicate mutates builder, so the query is no longer a pure filter and the correctness of the stroke depends on deferred-enumeration side effects. Keep the explicit loop and call AppendOutlinePolygon in its body; that is clearer and avoids introducing an iterator/closure into this hot path.
…uctive lint format check
- StrokeOutliner: replace absolute near-zero-area tolerance with one scaled to
ring coordinate magnitude, so tiny-but-legitimate closed strokes are no
longer misclassified as degenerate.
- PathBuilder.TangentArcTo: replace absolute coincidence tolerance with one
scaled to the corner/start/end coordinate magnitude, preserving arc
behavior for small coordinate systems.
- lint.ps1: stash any pre-existing uncommitted *.cs changes before running
dotnet format so the git-diff check reflects only the formatter's own
delta, then restore the original working tree afterward.
…tash handling
- StrokeOutliner: use ring bounding-box extent (not absolute coordinate
magnitude) for the area tolerance, so it is translation-invariant.
- PathBuilder.TangentArcTo: use the start/end chord length (not distance
from the origin) for the coincidence tolerance, so it is
translation-invariant.
- lint.ps1: detect pre-existing *.cs changes via 'git status --porcelain'
(catching untracked files too, not just 'git diff'), and restore the
stash with '--index' to preserve the original staged/unstaged split;
treat stash push/pop failures as lint errors.
…geometry skew
- StrokeOutliner: use the product of a ring's X and Y bounding-box spans
(rather than a single max-extent squared) for the area tolerance, so a
skinny ring's tolerance matches its own area order of magnitude instead
of being inflated by its unrelated long axis.
- PathBuilder.TangentArcTo: give each ray its own coincidence tolerance,
scaled only to the two points defining that ray, so a long/far ray on
the other side of the corner can no longer make the other ray look
coincident (and avoids relying on Vector2.Distance, which could
overflow for extreme coordinates).
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
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
Repo-wide remediation of CodeQL and SonarCloud findings, split into sequential themed batches on this branch.
Fixes applied
GlyfLocaReaderunreachable-code bug (root cause of the SonarCloud reliability-rating gate failure) — removed redundanthasContentflag.DecodeSimpleGlyphphases into named helpers; replaced a hand-rolled hex-digit check withchar.IsAsciiHexDigit.MemoryStreamsites across the test project (usingdeclarations). Left ~207Surfacedisposal sites untouched —Surface.Dispose()is currently a no-op (separate, larger effort), documented not suppressed..Where/.Selectwhere behavior-preserving; left rasterization/kerning-order-dependent loops unchanged (documented).Path.CombinewithPath.Joinat 23 fixture-test sites to remove the silent-discard-on-rooted-argument risk.PngHeader,CompositeWorkBuffers,Matrix3x2where possible); left 8 as-is (including 2 public-API methods, to avoid a breaking change with no genuine clarity gain), each documented.Explicit constraints followed
#pragma,[SuppressMessage], or CodeQL/SonarCloud exclusion config) for any left-as-is finding — every skip is documented with rationale instead.Validation
Every batch independently verified:
pwsh ./build.ps1(0 warnings/errors, all tests passing across net8.0/net9.0/net10.0) andpwsh ./fix.ps1(auto-formatters) before each commit. Final state: 4104/4104 tests passing.🤖 Generated with assistance from GitHub Copilot CLI
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com