fix(codeql): Clear cs/useless-upcast alerts and gate the class at build time - #134
Merged
Merged
Conversation
Code scanning filed three cs/useless-upcast alerts (#88-#90) against the `Returns((string?)null)` / `Returns((WorkspaceEntity?)null)` calls added with the NSubstitute 6 upgrade -- the null literal converts implicitly, so the cast is redundant. Swapped for `Returns(default(T))`. The alerts are only a symptom: CodeQL runs post-push, so this class of defect is always found late. Enforce the Roslyn twins in .editorconfig instead -- IDE0004 (redundant cast), IDE0035 (unreachable code), IDE0059 (unused assignment) and IDE0060 (unused parameter) at `warning`, which EnforceCodeStyleInBuild + TreatWarningsAsErrors turn into build errors. Turning the gate on surfaced three pre-existing violations, all fixed: two redundant `(decimal)` casts in CostCalculator (int converts implicitly in a decimal expression), a redundant `(double?)null` in a target-typed conditional in KcLiveBenchTests, and the unused `ex` parameter on PipelineOrchestrator.FailMidway -- the exception is already logged at the call site, so the parameter and its argument are dropped. Documented the convention in CLAUDE.md: when a new code-scanning alert lands, add its Roslyn twin to the block so the class cannot recur. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Change description
Clears the three open code-scanning alerts, and — more to the point — stops this class of alert from being filed again by moving the check into the build.
The alerts (#88, #89, #90, all
cs/useless-upcast) came from theReturns((string?)null)/Returns((WorkspaceEntity?)null)calls introduced by the NSubstitute 6 upgrade in #132. The null literal converts implicitly, so the cast is redundant → swapped forReturns(default(T)).The actual fix is the gate. CodeQL runs post-push, so anything it catches is caught late, in the security tab, after review. Every CodeQL C# quality query with a Roslyn twin is now enforced in
.editorconfig:IDE0004cs/useless-upcast,cs/useless-cast-to-selfIDE0035cs/unreachable-codeIDE0059cs/useless-assignment-to-localIDE0060cs/unused-parameterSet to
warning, which the existingEnforceCodeStyleInBuild+TreatWarningsAsErrorspromote to build errors — sodotnet buildfails locally and the alert is never filed. The convention is documented inCLAUDE.md: when a new code-scanning alert lands, add its Roslyn twin to that block, don't just fix the site.Note the boundary: this only covers syntactic queries. Taint/dataflow queries (log forging, path traversal, SSRF) have no compiler equivalent and still depend on the CodeQL workflow — fix in code, never dismiss.
Turning the gate on surfaced three pre-existing violations, all fixed here:
CostCalculator.cs— two redundant(decimal)casts;intconverts implicitly inside adecimalexpression.KcLiveBenchTests.cs— redundant(double?)nullin a target-typed conditional.PipelineOrchestrator.FailMidway— unusedexparameter. The exception is already logged at the call site (_logger.LogError(ex, …)immediately above), so the parameter and its argument are dropped rather than threaded into an unused field.Type of change
Related issues
Closes code-scanning alerts #88, #89, #90. Follows #132.
Checklist
dotnet build AgentOs.slnx --configuration Releasepasses locally — 0 warnings, 0 errors (with the four new gates active)dotnet test AgentOs.slnx --configuration Releasepasses — 832 passed, 0 failed, 14 skipped (+ 29 E2E skipped,RUN_AGENTOS_E2Eunset)CLAUDE.mdrecords the CodeQL-parity conventionNo UI surface touched, so no design-system round-trip or desktop screenshot applies.