Skip to content

chore: cut code-scanning alerts on main (#231) - #235

Merged
Chris-Wolfgang merged 5 commits into
mainfrom
chore/close-code-scanning-231
Aug 22, 2026
Merged

Chris-Wolfgang merged 5 commits into
mainfrom
chore/close-code-scanning-231

Conversation

@Chris-Wolfgang

Copy link
Copy Markdown
Owner

Summary

Drops the code-scanning alert backlog on main from 248 to a floor of config-only Scorecard leftovers, matching the noise-floor approach the fleet-wide pilot proved in Extensions-Logging-Data (repo-template#431).

Closes #231.

InspectCode: 222 → ~0

Two moves:

  1. AuditTrail.slnx.DotSettings (new) suppresses the eight R# rules that account for every one of the 222 findings and are either analyzer noise (already covered by the Roslyn stack) or fleet-shape false positives — mirrors the canonical profile from Extensions-Logging-Data:

    Rule Count Rationale
    CheckNamespace 110 Library deliberately declares the flat Wolfgang.AuditTrail.* namespace across all packages, independent of package folder. Polyfills sit under System.* on purpose.
    RedundantUsingDirective 75 <ImplicitUsings> gated on TFM; explicit System.* usings are required on netstandard2.0 / net6.0 but look redundant on net10.0.
    RedundantNameQualifier 5 Roslynator/IDE00xx already covers real cases.
    RedundantSuppressNullableWarningExpression 4 SonarAnalyzer S8969 is the rule we actually gate on — see below.
    ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract 2 Defensive null checks in library code called from nullable-oblivious consumers.
    NullCoalescingConditionIsAlwaysNotNullAccordingToAPIContract 1 Same.
    UnusedAutoPropertyAccessor.Global 4 Public API; R# can't see NuGet consumers.
    NotAccessedPositionalProperty.Global 2 Public record positional props; same reason.
  2. Actual defects fixed (not suppressed):

    • S8969 x6 — redundant ! after Assert.NotNull / flow-narrowed TryGetValue / IsNullOrWhiteSpace. Files: DbContextItemBag.cs (x2), AuditSchemaInstaller.cs, UseAuditingTests.cs, SmallCoverageGapsTests.cs, MigrateTests.cs.
    • UnusedParameter.Local x2 (src) — context → _ in the ConfigureServices / ConfigureAppConfiguration lambdas in Program.cs and IHostBuilderExtensions.cs.
    • UnusedMember.Local (test) — deleted the single-arg Create(DbContext) overload from the UncachedModelCacheKeyFactory test double; that signature is not part of the current IModelCacheKeyFactory interface, so it was genuinely dead.
    • Misc Redundant* (x3) — dropped an explicit type argument on RoundTrip<byte[]>, an explicit object?[] on a Serialize call, and a redundant default arg on new StaticAuditUserProvider("u", null) → ("u").
    • UnusedAutoPropertyAccessor.Local x4 + UnusedMember.Local x1 — scoped // ReSharper disable UnusedAutoPropertyAccessor.Local / UnusedMember.Local blocks on three EF-hydrated test POCOs (MappedItem, Widget, Color.Red). EF reads these via reflection; R# can't see it.

zizmor: 10 → 0

Rule Count Fix
template-injection 5 pr.yaml codeql-completion step and release.yaml NUGET_USER check now bind context values through env: and reference them as $env:… / "$NUGET_USER" in the run body.
dependabot-cooldown 2 Added cooldown: { default-days: 7 } to both dependabot ecosystems.
artipacked 1 integration.yaml checkout now sets persist-credentials: false.
superfluous-actions 1 Release attach step swaps softprops/action-gh-release for the runner-bundled gh release upload --clobber. Behavior unchanged — release exists (workflow only runs on release:published); this just attaches.
dangerous-triggers 1 pull_request_target on pr.yaml waived via new zizmor.yml with documented rationale. Full migration to pull_request is a workflow-wide refactor (1400+ lines, resolves the 7 Scorecard DangerousWorkflowID findings at the same time) — tracked separately, not landed here.

Scorecard: 16 (already below the <25 bar; not touched here)

Left as-is for now, all tracked under the same follow-up as needed:

  • DangerousWorkflowID x7 — all trace to the same pull_request_target + refs/pull/${{ pr.number }}/head checkout pattern; resolved by the same migration as dangerous-triggers.
  • PinnedDependenciesID x5 — dotnet restore calls; would need NuGet lockfiles across the repo. Fleet reference reference_lockfile_and_coverage_gate_release_traps flags a known gh-pages interaction, so deferring.
  • SASTID, CodeReviewID, CIIBestPracticesID, BranchProtectionID — repo/config-level, no file change would move them.

Test-code changes (per feedback_test_changes_need_approval — flagging for review)

  • ModelBuilderConfigurationTests.cs — dropped an unused overload of UncachedModelCacheKeyFactory.Create and removed a redundant null arg on StaticAuditUserProvider.
  • AuditCaptureColumnNameTests.cs, AuditCapturePostSaveSnapshotTests.cs, StringAuditValueSerializerExactFormatTests.cs — scoped ReSharper suppression comments on EF-hydrated POCOs.
  • UseAuditingTests.cs, SmallCoverageGapsTests.cs, MigrateTests.cs, PipeDelimitedEntityKeySerializerTests.cs — removed redundant ! / explicit type args (S8969 / Redundant* rules).

No test intent changed. All 99 affected unit tests still pass locally on net10.0.

Test plan

  • dotnet build clean on all four src/ projects in Release
  • dotnet build clean on affected test projects in Release
  • Affected unit tests pass (net10.0): 72 in EntityFrameworkCore.Tests.Unit + 27 in Cli.Tests.Unit
  • CI green (pr.yaml gated stages, workflow-security zizmor with --config zizmor.yml)
  • After merge: gh api "repos/Chris-Wolfgang/AuditTrail/code-scanning/alerts?state=open" --jq 'length' returns a number that meets the DoD (target: InspectCode <25, Scorecard <25, zizmor 0)
  • Umbrella [Maintenance] security: 248 open code-scanning alerts (3 tools) #231 auto-closes on merge to main (Closes keyword)

🤖 Generated with Claude Code

Drops the code-scanning alert backlog on `main` from 248 to a floor of
config-only Scorecard leftovers, matching the noise-floor approach the
fleet-wide pilot proved in Extensions-Logging-Data.

InspectCode (222 → expected ~0)
- AuditTrail.slnx.DotSettings suppresses the eight rules that account for
  all 222 findings and are either analyzer noise (RedundantUsingDirective,
  RedundantNameQualifier, RedundantSuppressNullableWarningExpression), the
  library-defensive-null-check false positives (Condition/NullCoalescing…
  APIContract), the flat-root-namespace false positive (CheckNamespace),
  or the public-API "unused" false positives (UnusedAutoPropertyAccessor
  and NotAccessedPositionalProperty, .Global variants).
- Real S8969 findings addressed by removing redundant `!` after
  Assert.NotNull / flow-narrowed nullable checks in DbContextItemBag,
  AuditSchemaInstaller, and three test files.
- Real Unused/Redundant findings addressed: `context` renamed to `_` in
  two IHostBuilder lambdas; single-arg `IModelCacheKeyFactory.Create`
  overload removed (not part of the current interface); explicit type
  arguments dropped from two invocations. Test entity POCOs get scoped
  `// ReSharper disable UnusedAutoPropertyAccessor.Local` blocks since
  EF hydrates them via reflection.

zizmor (10 → 0)
- template-injection (5): pr.yaml codeql step and release.yaml
  NUGET_USER check bind context values through env vars.
- dependabot-cooldown (2): 7-day cooldown added to both ecosystems.
- artipacked (1): integration.yaml checkout gets persist-credentials: false.
- superfluous-actions (1): release.yaml attach step swaps
  softprops/action-gh-release for `gh release upload` (release already
  exists — workflow only runs on release:published).
- dangerous-triggers (1): `pull_request_target` on pr.yaml waived via
  new zizmor.yml with a documented rationale — migration to
  `pull_request` is a workflow-wide refactor tracked separately.

Scorecard (16, already below the <25 bar)
- DangerousWorkflowID x7 all trace to the same `pull_request_target` +
  refs/pull/… checkout pattern — resolved by the same follow-up.
- PinnedDependenciesID x5 are `dotnet restore` calls; requires NuGet
  lockfiles (has known ETL-family gh-pages interaction — defer).
- Config-only findings (SASTID, CodeReviewID, CIIBestPracticesID,
  BranchProtectionID) left as-is.

Closes #231

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 19, 2026 13:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Chris-Wolfgang added a commit that referenced this pull request Aug 21, 2026
…ty.yaml

I split zizmor.yml onto the wrong side in #235 — workflow-security.yaml
here now passes --config zizmor.yml, but without the file being present
on this branch the zizmor job fails "config file not found". Moves
zizmor.yml onto this branch so the two land together.

The dangerous-triggers waiver on pr.yaml is the whole reason both files
are needed at once — without the config, zizmor reports the finding
again and gates the job.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Chris-Wolfgang added a commit that referenced this pull request Aug 22, 2026
…-231-workflows

chore: workflow security tightening (protected-file split of #235)
Chris-Wolfgang and others added 2 commits August 22, 2026 16:43
…Contain (MA0002)

Stage 2 on #235 caught seven new MA0002 errors after #234 bumped
Meziantou.Analyzer 3.0.142 → 3.0.164. The newer analyzer flags xunit
Assert.Contains / DoesNotContain over IEnumerable<string> and
Assert.NotEqual over two strings as needing an explicit
IEqualityComparer<string>, per the rule's
`use-comparer-that-controls-equality` guidance.

Pass StringComparer.Ordinal to each flagged call — matches how xunit
already resolves the parameterless overload for strings (ordinal via
generic Equals), so no behavior change:

- AuditCaptureBranchTests.cs:93,94
- AuditingDbContextSaveChangesTests.cs:32
- PipeDelimitedEntityKeySerializerTests.cs:34,64,82,83

All 22 affected tests still pass on net10.0 locally.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@Chris-Wolfgang
Chris-Wolfgang merged commit 3abc939 into main Aug 22, 2026
15 checks passed
@Chris-Wolfgang
Chris-Wolfgang deleted the chore/close-code-scanning-231 branch August 22, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Maintenance] security: 248 open code-scanning alerts (3 tools)

2 participants