Handle empty public API baseline snapshots

[thomhurst]

Summary

• make public API baseline merge tolerate empty inputs without null dereferences
• preserve deterministic ordering and idempotent writes using explicit typed collections
• cover fully empty shipped, unshipped, snapshot, and confirmed-removal files

Validation

• Test-MergePublicApiBaselineSnapshot.ps1
• Test-SyncPublicApiBaselines.ps1

Closes #4608

Summary by CodeRabbit

• Bug Fixes

  • Improved public API baseline merging by ignoring blank lines and producing consistent, unique, sorted output.
  • Corrected handling of empty baseline inputs so resulting outputs remain empty when appropriate.

• Tests

  • Added coverage for empty baseline files.
  • Improved line-by-line validation for more reliable comparison results.

[coderabbitai]

Warning

Review limit reached

Next included review available in 27 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 09d39e9f-069f-4842-b045-22b5337343ae

📥 Commits

Reviewing files that changed from the base of the PR and between f4bfc2e and 4f20e80.

📒 Files selected for processing (2)

• tools/ModularPipelines.OptionsGenerator/scripts/Merge-PublicApiBaselineSnapshot.ps1
• tools/ModularPipelines.OptionsGenerator/scripts/Test-MergePublicApiBaselineSnapshot.ps1

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 892f0223-1b69-45c2-951e-e88162cd0d0e

📥 Commits

Reviewing files that changed from the base of the PR and between f592a96 and e71500c.

📒 Files selected for processing (2)

• tools/ModularPipelines.OptionsGenerator/scripts/Merge-PublicApiBaselineSnapshot.ps1
• tools/ModularPipelines.OptionsGenerator/scripts/Test-MergePublicApiBaselineSnapshot.ps1

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

The merge script now filters empty baseline lines and uses typed collections for deduplication, sorting, comparison, and output. Tests now validate case-sensitive line matching and empty input baselines.

Changes

Baseline merge handling

Layer / File(s)	Summary
Baseline parsing and output assembly tools/ModularPipelines.OptionsGenerator/scripts/Merge-PublicApiBaselineSnapshot.ps1	Get-Headers and Get-ApiEntries exclude null or whitespace lines. Write-Baseline deduplicates, sorts, filters, compares, and writes lines through HashSet and List collections.
Empty-input regression coverage tools/ModularPipelines.OptionsGenerator/scripts/Test-MergePublicApiBaselineSnapshot.ps1	Assert-Lines uses case-sensitive comparisons. The test validates empty output after all four input baselines are truncated.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to b0bc9

Baseline snapshot merging now treats empty valid input files as empty collections and verifies empty outputs without changing the intended populated-file behavior. The change is ready to merge with no identified current risk.

Poem

A rabbit checks each empty line,
And sorts the records into line.
The baselines breathe, the tests agree,
No null hops through the API.
With tidy lists and output bright,
The merge now ends in empty white.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and concisely describes the main change: handling empty public API baseline snapshots.
Linked Issues check	✅ Passed	The changes handle empty shipped, unshipped, snapshot, and confirmed-removal files as empty collections. The test covers empty inputs, including the confirmed-removal file, and verifies empty outputs.…
Out of Scope Changes check	✅ Passed	All changes support empty baseline handling, deterministic output, or regression-test coverage for issue #4608. No unrelated code changes are identified.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-4608-empty-baseline

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Code review

Reviewed the changes to Merge-PublicApiBaselineSnapshot.ps1 and its test script for issue #4608 (empty public API baseline handling).

Verification performed:

• Checked out both the base commit (f592a963) and the PR head (8ab0fbdd) and ran the merge script directly against four empty input files.
  • Base commit: reproduces the exact crash from #4608 — Where-Object: You cannot call a method on a null-valued expression at the StartsWith call in Get-ApiEntries. This confirms the classic PowerShell gotcha where a function that return @(pipeline)s zero elements collapses to $null for the caller, so $Lines becomes $null instead of an empty array, and piping $null invokes the Where-Object predicate once with $_ = $null.
  • PR head: no longer crashes. The -not [string]::IsNullOrEmpty($_) -and ... guard added to Get-Headers/Get-ApiEntries correctly short-circuits before StartsWith is reached when $_ is $null.
• Ran Test-MergePublicApiBaselineSnapshot.ps1 from the PR branch: passes, including the new fully-empty-inputs case, and the pre-existing additive/removal/idempotency assertions still pass unchanged, so populated-file behavior is preserved.
• Traced the Write-Baseline rewrite: switching from @($Headers) + $sortedEntries / @(if(...) {Get-Content...}) to explicit foreach loops with IsNullOrEmpty filtering avoids the same null/array-collapse hazard that previously existed there too (e.g., @($null) producing a one-element array containing $null), and the HashSet is now built by iterating rather than passed a possibly-null collection to the constructor, avoiding a potential ArgumentNullException.

Assessment: the fix directly addresses the root cause described in #4608 rather than papering over a symptom, ordering/idempotency (via the existing-vs-lines comparison before writing) is preserved, and the added test meaningfully exercises the regression (all four empty inputs, verified against actual empty output). No CLAUDE.md concerns apply to this change (PowerShell tooling scripts, no dotnet build/test pipeline involved). No actionable issues found.

[github-actions]

Code review

This PR was previously reviewed at commit 8ab0fbdd and marked CLEAR (verified by actually running the merge script against empty inputs on base vs. head, confirming the crash from #4608 was fixed without regressing populated-file behavior). One additional commit (e71500cc, "fix: avoid assigning PowerShell automatic variable in baseline test") has been pushed since then.

What changed in the new commit: in Test-MergePublicApiBaselineSnapshot.ps1, the local variable previously named similarly to PowerShell's automatic $Matches variable was renamed to $isMatch in Assert-Lines. $Matches is populated/reset by the -match/-cmatch operators, so reusing that name as a plain boolean accumulator is a latent footgun (and a common PSScriptAnalyzer AvoidAssignmentToAutomaticVariable-style complaint) even though it was scoped locally and didn't cause an observable bug here. This is a pure defensive rename — the comparison logic ($actual.Count -eq $expectedLines.Count, then a per-index -ceq loop) is unchanged from the previously-reviewed version.

Re-verified for this delta:

• Traced all call sites of Get-Headers/Get-ApiEntries/Read-Baseline in Merge-PublicApiBaselineSnapshot.ps1 (unchanged since the prior review) to confirm the null-collapse footgun (PowerShell functions that return @(pipeline) collapse to $null at the call site when zero items are emitted, unless the caller itself wraps the call in @()) is handled safely everywhere it matters: every consumer of a possibly-null result uses either foreach (which PowerShell special-cases to iterate zero times over $null, unlike piping $null through Where-Object) or an explicit @() wrap at the call site (e.g. -Entries @($shippedApis)). No path reaches a .StartsWith()/method call on a potentially-null pipeline item.
• Write-Baseline's rewrite to explicit List<string>/HashSet<string> construction with manual foreach population avoids ever passing a possibly-null collection into a constructor or SequenceEqual, which is the same class of hazard as the original bug — this is a real improvement over the old @($Headers) + $sortedEntries / @(if(...) {...}) pattern.

No CLAUDE.md concerns apply (PowerShell tooling scripts, not part of the dotnet build/test pipeline or generated-options code). No actionable issues found on this new commit.

[thomhurst]

Synced current origin/main at exact head b0bc902f6d (conflict-free; this PR's own diff is unchanged). The failed Ubuntu run at e71500cc broke on two timing-sensitive core unit tests (Flush_RenderGateTimeout_WritesBufferedOutputDir, Timeout_Claims_Tokenless_Cooperative_Cancellation) in RunCoreUnitTestsModule, unrelated to these PowerShell scripts; the branch predated #4588, which is the only main change to those tests since this branch forked. Local validation on the merged head: Test-MergePublicApiBaselineSnapshot.ps1 and Test-SyncPublicApiBaselines.ps1 both pass.

[github-actions]

Review: #4609 — Handle empty public API baseline snapshots

This PR hardens Merge-PublicApiBaselineSnapshot.ps1 for empty inputs: Get-Headers/Get-ApiEntries now guard against null/empty lines, Write-Baseline switched to explicit List<string>/HashSet<string> construction instead of array-cast constructors, and a new "empty inputs, idempotency" case was added to Test-MergePublicApiBaselineSnapshot.ps1. The dedup/sort/idempotent-write logic itself looks correct and I traced it against the Sync-PublicApiBaselines.ps1 call site.

Finding: the new "empty inputs" test doesn't exercise the case it claims to cover

Write-Baseline (Merge-PublicApiBaselineSnapshot.ps1:57-66) only calls WriteAllLines when the computed $lines differ from what's already on disk:

$existing = [System.Collections.Generic.List[string]]::new()
if (Test-Path -LiteralPath $Path -PathType Leaf) {
    foreach ($line in Get-Content -LiteralPath $Path) { $existing.Add($line) }
}
if ([System.Linq.Enumerable]::SequenceEqual($existing, $lines, $comparer)) {
    return   # <-- also hit when $Path doesn't exist yet and $lines is empty
}

If $Path doesn't exist yet, $existing stays an empty list. When all four inputs are empty, $lines is also empty, SequenceEqual is true, and the function returns without ever writing the file — even though the script reports "Synchronized 0 current public API entries." as if it succeeded.

The PR's summary says it now covers "fully empty shipped, unshipped, snapshot, and confirmed-removal files," and Test-MergePublicApiBaselineSnapshot.ps1:114-131 adds a case for that — but $shippedOutput/$unshippedOutput were already populated by the earlier merge (line 73-79) and idempotency re-run (line 101-107) in the same test, so the SequenceEqual comparison there is always "non-empty vs empty" → always unequal → always writes (truncates to empty). The test therefore verifies "an existing baseline gets truncated to empty," not "a brand-new baseline file gets created empty," which is the scenario where the early-return bug above actually bites.

In practice this is currently unreachable: the sole production caller, Sync-PublicApiBaselines.ps1:22-28, asserts PublicAPI.Shipped.txt/PublicAPI.Unshipped.txt already exist before ever invoking the merge script, so the output paths are always pre-existing files. But Merge-PublicApiBaselineSnapshot.ps1 is written as a general-purpose, independently-invocable script (it validates its own inputs, has its own test harness), so relying on an un-enforced caller invariant for correctness is fragile — any future caller (or manual invocation while bootstrapping a new package's baselines) that points -ShippedOutputPath/-UnshippedOutputPath at files that don't exist yet, with empty inputs, will silently get no output file at all.

Suggested fix: in Write-Baseline, only take the early-return fast path when the file already exists (Test-Path true) and content matches; otherwise always write, so a not-yet-existing target is created (even as an empty file). Then add a genuine test case using fresh, never-written output paths with all-empty inputs to actually cover the claim in the PR description.

Not a regression introduced by this PR (the same early-return-on-SequenceEqual structure existed in the prior version too) and not currently reachable in the real pipeline, so this is a robustness/test-coverage gap rather than a live bug — but worth closing given the PR explicitly calls out empty-file handling as its purpose.

[thomhurst]

Addressed the review finding at exact head d922dc62b1: Write-Baseline now takes the no-op fast path only when the target file already exists with identical content, so a not-yet-existing baseline is created even when there is nothing to list. Test-MergePublicApiBaselineSnapshot.ps1 gained a case that runs the merge with all-empty inputs into fresh, never-written output paths and asserts both files exist and are empty; it fails on the previous head and passes now. Test-SyncPublicApiBaselines.ps1 still passes. @claude please re-review exact head d922dc6.

[thomhurst]

Synced current origin/main at exact head 4f20e80db2 (conflict-free; the script changes are unchanged). The run at d922dc62 failed only on the timing-sensitive core test DistributedMasterMetricsTimeoutWhenCoordinatorIs… in RunCoreUnitTestsModule, unrelated to these PowerShell scripts; main now carries #4659's environment isolation, and a fresh push restarts CI because a rerun cannot recover the expired options-generator-test-results artifact. Both baseline script tests pass on the merged head.

[github-actions]

Review: #4609 — Handle empty public API baseline snapshots

This is a follow-up revision after two prior review rounds on this PR. Both previously flagged issues have been addressed:

1. Null-collapse footgun (original bug, Handle empty public API baseline snapshots #4608): `Get-Headers`/`Get-ApiEntries` now guard against null/empty lines, and `Write-Baseline` was rewritten to build `List`/`HashSet` via explicit `foreach` population instead of array-cast constructors (`@($Headers) + $sortedEntries`, `[string[]] @($existing)`), which is what previously produced a `$null` at the call site for zero-length collections. Traced every consumer of `Get-Headers`/`Get-ApiEntries`/`Read-Baseline` — all either `foreach` (safe over `$null`) or wrap in `@()` at the call site, so no path reaches a method call on a possibly-null item.

2. Early-return-on-`SequenceEqual` bug (flagged in the prior review round): `Write-Baseline` previously could return without writing when `-Path` didn't exist yet and the computed lines were empty (empty `$existing` == empty `$lines`). This is now fixed exactly as suggested — the `SequenceEqual` short-circuit is nested inside `if (Test-Path -LiteralPath $Path -PathType Leaf)` (`Merge-PublicApiBaselineSnapshot.ps1:59-68`), so a not-yet-existing target is now always created. `Test-MergePublicApiBaselineSnapshot.ps1:134-150` adds the genuine regression case this needed: fresh, never-written `-ShippedOutputPath`/`-UnshippedOutputPath` paths with all-empty inputs, asserting the files are created and empty. This closes the gap the earlier "empty inputs" test left (it only exercised truncating an already-populated output, not creating a brand-new one).

3. The `$Matches`-shadowing rename (`$isMatch` in `Assert-Lines`) from the intermediate commit is a reasonable, low-risk defensive cleanup and doesn't change comparison semantics (still per-index `-ceq` after a count check).

I traced the dedup/sort/promotion logic in the main script body (shipped ↔ unshipped promotion, `REMOVED` handling, reintroduction) against the new empty-input test case and the pre-existing populated-file test case — both check out, and this logic itself is unchanged by this PR (only the header/entry filtering and the write helper were touched).

No actionable issues found. Nice, tight fix scoped to the actual defect, and the new test now genuinely covers the create-vs-truncate distinction that was missing before.