Enterprise quality bar: build hardening, CI/CD gates, security tooling, integration tests#4
Merged
Merged
Conversation
…rail Lift project-wide MSBuild settings into Directory.Build.props so the quality bar is enforced uniformly: TreatWarningsAsErrors, EnableNETAnalyzers with latest-recommended, Deterministic=true, ContinuousIntegrationBuild on CI, RestorePackagesWithLockFile, source link via Microsoft.SourceLink .GitHub. Add packages.lock.json files for both projects so package versions can no longer drift silently across restores. Pin SDK via global.json (10.0.100, rollForward latestFeature) so local and CI use the same toolchain. Add .editorconfig for consistent formatting and .gitattributes for line-ending normalization. CI workflow runs build (with /warnaserror) and tests with coverage on every PR to main. Sanitization workflow greps every committed file for forbidden tokens (prior client refs, personal email patterns, common credential signatures) and fails the workflow on any hit — last-line defense if a future doc edit slips a leak past human review. NoWarn list is documented inline with WHY for each suppression. CA1848/ CA1873 are deliberate (LoggerMessage perf delegates would be overkill at our log volume); CA1310/CA1822 are transitional pending follow-up audits; CS8602 is transitional pending a one-site source fix on this branch; CA1707 is permanent (xUnit naming convention). quality-bar.md is the durable PR checklist authors should read before opening a PR. Includes the Git Bash /warnaserror MSYS gotcha.
Two complementary defenses for a public repo: Dependabot (v2 syntax) covers nuget at /, github-actions at /, runs weekly Monday 06:00 UTC, opens at most 5 PRs per ecosystem, groups security updates separately. Commit-message prefix follows the team's conventional format as closely as Dependabot allows. CodeQL (security-and-quality query set) on push to main, every PR to main, and weekly. Restores in --locked-mode against the lock files added in the build-hardening commit, then builds Release. Permissions scoped to the minimum (security-events:write, actions:read, contents:read). Note: a structured-logging audit of the codebase was performed as part of this work and found no offending sites — all 27 _logger.Log* calls already use proper message templates with PascalCase placeholders. No source changes required.
…tion Add IntegrationTests.cs that exercises the full host-builder graph the way Program.cs.CreateHost does. Six tests: Host_BuildsWithoutErrors Host_AllSourceScrapersAreRegistered Host_CatalogBuilderAndDependenciesResolve Host_HttpClientForFileDownloader_HasResiliencePipeline Host_HttpClientForManualsScraper_HasResiliencePipeline Host_BuildCatalogAsync_ProducesValidCatalogJson The resilience-pipeline tests use the behaviour pattern (fake primary handler returns 503 then 200, assert retry occurred) rather than walking the message-handler chain, since the resilience handler's concrete type is internal to the package and matching by type-name string would be brittle. Behaviour tests fail naturally if retry stops working for any reason without coupling to package internals. The BuildCatalogAsync test is a regression guard for the "preserve LastDownloadedAt on missing-file" fix: it seeds a catalog with a missing-on-disk file, runs reconciliation, then asserts the JSON is valid, File is cleared, AND LastDownloadedAt is preserved (so a missing file is distinguishable from a never-downloaded one). No new NuGet packages were needed; all required types are transitively available via the existing project reference. CONTRIBUTING.md captures local setup, branch naming, commit format, quality bar pointer, and PR conventions. README.md gains three badges (CI, CodeQL, .NET version) and a CONTRIBUTING pointer; the badges will go green once this PR merges and the workflows run on main. Test count: 68 -> 74. Build clean, tests pass.
Companion to docs/quality-bar.md: the lighter quality-bar is a 30-second PR checklist; ENGINEERING_STANDARDS.md is the deep reference covering guiding principles, C#/.NET conventions, project structure, error handling, logging, configuration, testing, security, observability, build/CI, dependencies, documentation, performance, and explicit non-goals. Sanitized to remove client-name references (replaced with generic "any other project" / "standalone Azure infrastructure" framing) — the sanitization guardrail added in the build-hardening commit would have caught these on first CI run.
GameRecord.Source is GameSourceInfo? (optional), but MergeGameRecord was unconditionally writing existing.Source.ScrapedAt — caught by CS8602 once the build moved to latest-recommended analyzers, and previously suppressed at project level pending a real fix. Prefer the freshly-scraped Source when the incoming GameRecord has one (it already carries the current ScrapedAt baked in by the scraper); otherwise stamp the existing Source if non-null; otherwise leave null rather than synthesize a value with no truthful ScrapedFrom URL. Removes the CS8602 entry from project NoWarn — a band-aid replaced by a real fix at the site.
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+30
to
+32
| _tempDir = Path.Combine( | ||
| Path.GetTempPath(), | ||
| "pinballwizard-integration-" + Guid.NewGuid().ToString("N")); |
Comment on lines
+43
to
+46
| catch | ||
| { | ||
| // best-effort cleanup | ||
| } |
This was referenced May 2, 2026
Merged
jkeeley2073
added a commit
that referenced
this pull request
May 8, 2026
Replaces the README placeholder ("No license file yet; reach out before
reusing.") with an actual MIT license file. MIT is the most permissive
common option and matches typical portfolio-project convention; explicit
licensing also unblocks any future code-reuse questions.
Copyright holder is the GitHub org owner: Early Bird Solutions LLC.
README's License section now points to the LICENSE file.
A license-status badge can be added alongside the CI/CodeQL/.NET badges
in a small followup once PR #4 merges (PR #4 currently owns the badge
area; deferred to avoid a merge-conflict-prone change here).
jkeeley2073
added a commit
that referenced
this pull request
May 8, 2026
Replaces the README placeholder ("No license file yet; reach out before
reusing.") with an actual MIT license file. MIT is the most permissive
common option and matches typical portfolio-project convention; explicit
licensing also unblocks any future code-reuse questions.
Copyright holder is the GitHub org owner: Early Bird Solutions LLC.
README's License section now points to the LICENSE file.
A license-status badge can be added alongside the CI/CodeQL/.NET badges
in a small followup once PR #4 merges (PR #4 currently owns the badge
area; deferred to avoid a merge-conflict-prone change here).
This was referenced May 8, 2026
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
Five focused commits that raise PinballWizard to a portfolio-grade quality bar across build, test, security, and documentation. Each commit builds and tests independently green (bisect-safe).
Guardrails this PR delivers
TreatWarningsAsErrors=true+latest-recommendedanalyzersDirectory.Build.propsDirectory.Build.propsDirectory.Build.props+packages.lock.jsonfiles.github/workflows/ci.yml.github/workflows/sanitization.yml.github/dependabot.yml.github/workflows/codeql.ymltests/.../IntegrationTests.csdocs/quality-bar.mddocs/ENGINEERING_STANDARDS.mdCONTRIBUTING.mdNotable findings during this work
_logger.Log*call sites found zero offending interpolations — the team already uses message templates with PascalCase placeholders. No source changes needed.JungleTechreferences slipped intodocs/ENGINEERING_STANDARDS.mdand were scrubbed before commit; the workflow would have caught them on first CI run.CS8602was a band-aid replaced by a real fix. Initially suppressed at project level to keep the build green; final commit fixes the one site (CatalogBuilder.MergeGameRecordaccessing optionalSource) and removes the suppression. The fix correctly merges fresh-scraped Source with existing.NoWarn list (documented in
Directory.Build.props)CA1848/CA1873CA1310/CA1822CA1707Method_Scenario_ExpectedResultnaming convention requires underscoresDown from 6 suppressions to 5 —
CS8602was retired by this PR.Test plan
dotnet build PinballWizard.slnx /warnaserrorclean (0 warnings, 0 errors)dotnet test --no-build→ 74/74 pass (was 68; +6 integration tests)dotnet restore --locked-modesucceeds against committed lockfilesmainafter merge. README badges will go from "no status" to live status at that point.Out of scope (deferred)
main— needs to be enabled via repo settings AFTER this PR merges (the new CI check needs to run onmainonce before it can be set as a required check). Will surface as a separate ask.LoggerMessage.Definemigration — deliberate non-goal; suppressed via<NoWarn>with documented rationale.