CI: actually run the kit in smoke tests, on oldest and newest SQL Server

[BrentOzar]

Closes #4046.

The problem

The smoke test installed only the changed sp_*.sql and ran it once, with @VersionCheckMode = 1 — an unconditional early RETURN at sp_Blitz.sql#L75-L78. No check body ever executed, so it proved the script parsed and nothing more.

#4045 is the evidence: two runtime breakages passed it green and had to be caught by human review. Neither was catchable at install time — one was a variable inside a string literal, the other left the outer T-SQL validly balanced.

What this does

Seeds a database, installs all 12 non-deprecated scripts, verifies each procedure actually exists, runs a 39-step matrix, and fails the build on any SQL error. On SQL Server 2017 and 2025.

• All 12 non-deprecated scripts, through .github/scripts/smoke-test-matrix.sql — the @Output* table paths, sp_BlitzIndex modes 0-4, sp_ineachdb's three-part-name rewrite, sp_BlitzPlanCompare against a real cached plan. Seeded from Documentation/Development/Test in Azure.sql, kept separate so CI can carry setup a hand-run script shouldn't (decision 2B). sp_BlitzUpdate is excluded: it overwrites the procs mid-run (decision 4).
• Each step runs on its own, split on --#STEP: markers, so a failure names one step and the rest still run — one CI round surfaces every problem, not just the first.
• sqlcmd -I, so procedures are created with QUOTED_IDENTIFIER ON. Without it sqlcmd's OFF default is captured at CREATE time and the sp_BlitzFirst/sp_BlitzLock XML paths die with Msg 1934 — a configuration no real client uses.
• Install is verified, not assumed: a script reduced to an empty file still exits sqlcmd 0, so every expected procedure is checked with OBJECT_ID.
• Real seed state — 2,000 users / 5,000 posts, a heap, duplicate and unused indexes, full and log backups, plan cache activity. An empty server makes most checks bind against empty tables and report nothing.
• Two engine versions — 2017 (oldest in support; 2016 left extended support 2026-07-14) and 2025, fail-fast: false.

What was removed, and why

An earlier revision of this PR also installed the base branch's copies, ran everything twice, and classified each failure as new or pre-existing — error signatures, step-body comparison, harness-file comparison, replay-on-mismatch. About 155 lines answering one question: is this failure the PR's fault?

It's gone. Three reasons:

1. It never took a decision. Every run logged harness differs from base; every step must pass on its own merits, because the harness files themselves keep changing. The machinery has not once been the thing that decided an outcome.
2. Grandfathering only matters when the baseline is dirty. The baseline is clean — 0 failing steps on both engines. When every step passes, "is this pre-existing?" has no instances.
3. It was where the defects lived. Nearly every problem found while reviewing this harness was in that code, several of them regressions introduced while fixing others.

The runner went from 792 lines to 281, and CI from two passes to one. Git history has a working implementation if a dirty baseline ever makes the question real.

What is deliberately not covered

Stated plainly, because a step whose name implies coverage it lacks is the same defect as the @VersionCheckMode call this replaces:

• sp_kill's kill loop. The matrix runs @ExecuteKills = 'Y' against a filter matching no session, so sp_kill sets @TotalKills to 0 and exits its nothing-to-kill branch; the cursor and EXEC(@KillSQL) at sp_kill.sql:793-840 are never reached. Parameter and filter handling are covered. Real coverage needs a second session held open — CI: sp_kill's actual kill loop is not covered by the smoke tests #4051.
• sp_DatabaseRestore's execute path. Stays at @Help until sp_DatabaseRestore: @MoveFiles = 1 fails on Linux with Msg 537 (hardcoded backslash path separator) #4049; its Ola Hallengren dependencies aren't installed either, since nothing could reach them.
• Windows-only paths. The containers are Linux, so @IsWindowsOperatingSystem = 0 and xp_regread, xp_cmdshell and cluster DMVs aren't exercised. Binding still happens, so both sp_Blitz: make it install and run on Azure SQL Database (#4040) #4045 bugs would have been caught.
• Azure SQL DB. Not possible with a container; deferred per decision 3A.

Bugs this found

All pre-existing on dev, all filed separately:

• sp_Blitz: divide by zero in CheckID 152 when the server has been up less than a minute #4048 — sp_Blitz divides by zero when the server has been up under a minute. @MsSinceWaitsCleared is DATEDIFF(MINUTE, ...), so it's 0, and the zero-guard sits inside a branch that can't be taken while the value is 0. Reproduced on both engine versions.
• sp_DatabaseRestore: @MoveFiles = 1 fails on Linux with Msg 537 (hardcoded backslash path separator) #4049 — sp_DatabaseRestore's @MoveFiles = 1 (the default) is broken on Linux: two hardcoded backslashes, one splitting the filename off PhysicalName (Msg 537), one joining the backup directory to the file name.
• sp_Blitz: CheckID 106 aborts the whole proc when the default trace is mid-write (Msg 568) #4050 — sp_Blitz aborts entirely when a read of the live default trace lands on a torn file (Msg 568), dropping every finding after CheckID 106.

CI works around #4048 (waits out the uptime window) and #4050 (skips CheckID 106 via @SkipChecksTable, which incidentally exercises that previously untested path). Both carry comments pointing at their issues and should come out when they're fixed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FMPBMMANLaFgSQ73GTgBW7

[Copilot]

Pull request overview

This PR strengthens the SQL Server smoke-test CI by running the full (non-deprecated) First Responder Kit against seeded, realistic state, across both the oldest supported and newest SQL Server versions, and comparing base vs head to only fail on newly introduced SQL errors.

Changes:

• Expands the GitHub Actions workflow to run a SQL Server version matrix (2017 + 2025) with fail-fast: false and longer timeouts.
• Adds CI SQL scripts to seed data and run a step-split execution matrix covering the kit’s main procedures and key parameter paths.
• Reworks the smoke-test runner to install and execute the kit for base and head revisions, diff results, and fail only on newly introduced SQL errors.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
.github/workflows/sql-server-smoke-tests.yml	Runs smoke tests across SQL Server 2017/2025 images with full git history for base vs head comparison.
.github/scripts/smoke-test-seed.sql	Seeds a realistic database, activity, and backup history for meaningful procedure execution.
.github/scripts/smoke-test-matrix.sql	Defines the 39-step execution matrix split into independently attributable steps.
.github/scripts/run-sql-server-smoke-tests.sh	Installs/runs base + head kits, splits and executes the matrix, captures findings, and enforces “new errors fail” logic.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[BrentOzar]

The expanded matrix is a strong direction, but the current successful workflow run still records seven failed head steps on each SQL Server version. I found five correctness gaps that currently leave several advertised paths untested or can mask regressions.

[BrentOzar]

The earlier five findings were addressed, and the new run confirms that -I, the plan-cache lookup, cleanup additions, and volatile-finding filter work. The refreshed CI also exposes five follow-up correctness gaps: the uptime guard is not actually waiting, the restore fixture still lacks a required dependency and cannot handle this Linux path, error signatures discard the diagnostic text, and a transient error in unchanged code is falsely attributed to the PR.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/scripts/run-sql-server-smoke-tests.sh:363

• This call reruns the same full sp_Blitz configuration that the matrix may just have recorded as a base failure. Because capture_findings uses sqlcmd -b and the script has set -e, such a pre-existing base error aborts the job here before the head pass and is never handled by the comparison logic. Guard the base capture, track whether findings are available, and skip only the informational findings diff when it fails.

if [[ -n "$BASE_REVISION" ]] && git -C "$REPO_ROOT" rev-parse --verify --quiet "$BASE_REVISION" >/dev/null; then

[BrentOzar]

The current head is clean on both engines: 38/38 base and head steps pass, and findings are identical. I found four future-regression holes below. The PR description also needs a refresh before merge: it still says 39 steps and implies sp_DatabaseRestore execute-path coverage, while the current matrix has 38 steps and deliberately runs only @Help; it also still says the SQL has not been verified even though both live jobs are now clean.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

[BrentOzar]

Reviewed the current head 734f71f. Both live jobs are clean: SQL Server 2017 and 2025 each passed all 39 base steps and all 39 head steps, with 49 and 43 findings respectively, identical to base. The latest push addresses the prior install, signature, harness, cleanup, retry-state, and findings-configuration comments. I found two remaining items below. The PR description also needs one final refresh: it still says 38 steps and that the Ola dependencies are installed, and its verification section is anchored to 8ddef6b rather than this 39-step run.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

.github/scripts/smoke-test-seed.sql:66

• CHECKSUM(NEWID()) can return -2147483648, and ABS on that INT raises arithmetic overflow. This makes the seed capable of randomly aborting the entire CI job before any smoke test runs; cast to BIGINT before applying ABS.

This issue also appears on line 72 of the same file.

       ABS(CHECKSUM(NEWID())) % 50000,
       DATEADD(DAY, -(ABS(CHECKSUM(NEWID())) % 3000), GETDATE())

.github/scripts/smoke-test-matrix.sql:202

• This does not exercise the advertised KILL construction/execution path. With a guaranteed-nonmatching application name, sp_kill sets @TotalKills to 0 and exits through the “Nothing to kill” branch; the cursor and EXEC(@KillSQL) path at sp_kill.sql:793-840 are never reached. Hold open a second sqlcmd session and target that session so regressions in the actual kill loop fail CI.

EXEC dbo.sp_kill @ExecuteKills = 'N';

.github/scripts/run-sql-server-smoke-tests.sh:455

• sqlcmd formats this variable-length expression before it reaches sed: by default it limits variable columns to 256 characters and uses an 80-character screen width. This serialized row can be about 340 characters (CheckID, a 128-character database name, and a 200-character finding), so output can be truncated or wrapped into multiple lines; sort/comm can then miss a changed suffix or compare fragments instead of findings. Request an untruncated, non-wrapping width for this machine-readable output.

  "$SQLCMD" "${SQLCMD_ARGS[@]}" -d FRKSmokeTest -h -1 -W -s '|' -Q "

.github/scripts/smoke-test-seed.sql:74

• These three random expressions have the same ABS(INT_MIN) failure mode: CHECKSUM(NEWID()) can produce -2147483648, which makes ABS raise arithmetic overflow and flakes the seed. Cast to BIGINT first.

       ABS(CHECKSUM(NEWID())) % 2000 + 1,
       ABS(CHECKSUM(NEWID())) % 100,
       DATEADD(DAY, -(ABS(CHECKSUM(NEWID())) % 1000), GETDATE()),

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

.github/scripts/smoke-test-matrix.sql:231

• This still says the dependencies are installed, but the workflow and the re-enabling instructions below explicitly say they are not. That contradiction can mislead maintainers about what the current help-only step covers; describe the dependencies as deliberately omitted.

Its dependencies are installed (CommandLog + CommandExecute, fetched by the
workflow), so the procedure runs -- but it cannot complete against a Linux

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/scripts/run-sql-server-smoke-tests.sh:510

• sp_BlitzLock also creates FRKSmokeTest.dbo.BlitzLockFindings on its table-output path (sp_BlitzLock.sql:869-893). Leaving that table behind means the head pass sees it from the base pass and skips its creation branch, so a regression in that DDL can pass. Include it in the table cleanup list.

WHERE name IN (N'BlitzOutput', N'BlitzCache', N'BlitzFirst', N'BlitzFirst_FileStats',
               N'BlitzFirst_PerfmonStats', N'BlitzFirst_WaitStats',
               N'BlitzFirst_WaitStats_Categories', N'BlitzWho',
               N'BlitzWho_Results', N'BlitzLock', N'BlitzIndex', N'BlitzFindings');

[BrentOzar]

Reviewed the current head 88f5e08. The earlier findings-readback and restore-guidance notes are fixed, as are the subsequent cleanup and empty-signature issues. Both live jobs passed all 39 base and 39 head steps with no SQL errors. SQL Server 2025 reported 43 findings identical to base; SQL Server 2017 showed one informational difference, CheckID 185 (Wait Stats Have Been Cleared), despite this PR changing no kit procedure. I found three remaining classifier/noise issues below.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

.github/scripts/smoke-test-matrix.sql:81

• This explanation describes the removed findings comparison, classification, and volatile-CheckID filter. The current runner only executes the SELECT; its purpose is now to make missing or renamed persisted columns fail the step.

/* The readback the comparison performs, so that renaming or removing CheckID,
   DatabaseName or Finding fails this classified step instead of silently
   downgrading the findings diff to "Skipped". The volatile-CheckID filter the
   runner applies is deliberately not reproduced here -- this exists to prove the
   columns the readback depends on, not to duplicate the filter. */

[BrentOzar]

Re-reviewed current head 87413ca.

The base-vs-head classifier and findings-diff subsystem that motivated my prior three comments has been removed. The remaining duplicate-label ambiguity is now explicitly rejected, and this latest commit cleans up the stale comments left by the one-pass simplification.

I found no additional actionable issues in the current runner, matrix, seed, or workflow. The live SQL Server 2017 and SQL Server 2025 jobs both passed all 39 matrix steps.