Fix commitment discount eligibility fetch (Retail Prices API pagination)

[RolandKrummenacher]

Summary

The weekly Update Commitment Discount Eligibility workflow has been failing on every run. This fixes two independent root causes and regenerates the open-data file.

1. The fetch was broken and non-reproducible

Update-CommitmentDiscountEligibility.ps1 paged the Azure Retail Prices API with an undocumented multi-field $orderby and an explicit $top:

• The API now rejects multi-field $orderby with HTTP 400 (fails on page 1 — the recent ~30s failures).
• Following NextPageLink while sending $top is also broken: the API's NextPageLink decrements $top per page into negatives (1000 → 0 → -1000), returning HTTP 400.
• Even setting that aside, the documented NextPageLink/$skip pagination has no stable total order across page requests — a single long traversal silently drops a scattered ~2–24% of rows, so two identical runs disagreed (~76% Jaccard).

Fix: follow NextPageLink verbatim (no $orderby, no $top), shard each query by serviceFamily so traversals stay short, and repeat each shard unioning by meterId until the set is stable. A full run is now reproducible (two runs byte-identical) and more complete (captures meters the old walk dropped). Added a completeness guard (-MaxShrinkFraction, default 0.15) that aborts before writing if a run is materially incomplete, and switched to writing the freshly-fetched set so retired meters age out instead of accumulating forever.

2. The workflow tried to open a PR, which Actions can't do here

gh pr create failed every run (Actions aren't permitted to open PRs in this org — confirmed via run history: the Create PR step failed while the fetch succeeded). The workflow now pushes a dated branch and surfaces a one-click "create PR" link in the job summary for a maintainer to open (which then triggers Open Data CI and normal review).

Data change

Regenerated CommitmentDiscountEligibility.csv: 121,301 → 92,163 rows

• +6,630 meters the old broken pagination silently missed
• −35,768 retired meters no longer returned by the API (sampled 15/15 confirmed gone)
• 166 corrected eligibility flags

Test plan

• Two full runs produce byte-identical output (reproducibility)
• Completeness-guard arithmetic unit-tested (healthy → write, outage → abort, migration → abort unless overridden)
• Output verified: LF, no BOM, fully quoted, sorted by MeterId, correct columns
• Reviewer: scan the diff for unexpected changes; spot-check a few meter IDs against the Azure Pricing Calculator
• Confirm the first scheduled/dispatched run pushes a branch and posts the PR link (the Actions bot's branch-push permission can only be confirmed by a real run)

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR aims to make the weekly “Update Commitment Discount Eligibility” automation reliable again by fixing Azure Retail Prices API pagination handling in Update-CommitmentDiscountEligibility.ps1 and updating the GitHub Actions workflow to push an update branch + provide a manual PR creation link (since Actions cannot open PRs in this org).

Changes:

• Reworks the Retail Prices API fetch to follow NextPageLink verbatim, shard queries by serviceFamily, and repeat/union results until stable; adds a completeness guard to prevent overwriting open data with incomplete results.
• Updates the workflow to push a uniquely named branch and write a “Create pull request” compare-link to the job summary instead of calling gh pr create.
• Regenerates the open-data dataset (per PR description).

Reviewed changes

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

File	Description
src/scripts/Update-CommitmentDiscountEligibility.ps1	Implements sharded/repeated API traversal with convergence + completeness guards and writes a fresh (non-cached) dataset.
.github/workflows/opendata-commitment-eligibility.yml	Removes PR-creation permissions/step; pushes a branch and surfaces a PR link in the job summary.
src/open-data/CommitmentDiscountEligibility.csv	Regenerated eligibility data output (not shown in diff snippet, but described in PR).

[RolandKrummenacher]

Thanks @copilot-pull-request-reviewer. Addressed:

Retry/backoff null .Response (line 117) — fixed in 2573af3. On network/DNS/timeout errors the exception has no .Response, so …Response.Headers['Retry-After'] indexed $null and threw "Cannot index into a null array" inside the catch, bypassing the retry. Now guarded — a missing .Response is treated as a retryable "Network error".

$riResult.Keys / $spResult.Keys (lines 247, 260) — not a bug. For a PowerShell [hashtable], key access takes precedence over the .NET .Keys property, so $riResult.Keys returns the stored meter hashtable, not the member-name collection. Verified:

$r = @{ Keys = @{ a=1; b=2; c=3 }; NonConverged = @('x') }
$r.Keys.GetType().Name      # Hashtable
$r.Keys.Count               # 3   (not 2)
$r.Keys.ContainsKey('a')    # True

This also matches the end-to-end runs: RI-eligible meters: 69063 / SP-eligible meters: 82122 and a correct, reproducible CSV — if .Keys had returned the 2-member collection, every count would have been 2. Left as-is.

[flanakin]

🤖 [AI][Claude Opus 4.8] PR Review

Summary: Solid, well-diagnosed fix. The root-cause analysis (multi-field $orderby 400s, $top decrementing negative via NextPageLink, and unstable total order dropping a scattered fraction of rows) is accurate, and the shard-and-union-to-stable approach with a completeness guard is a sound workaround. The prior Copilot review was addressed correctly (null .Response guard is real; the .Keys defense is right — hashtable key access shadows the .Keys member). I verified the regenerated CSV: 92,163 rows, LF, no BOM, fully quoted, ordinally sorted by MeterId — matches the PR claims. PowerShell parses clean; the only PSScriptAnalyzer warnings are pre-existing Write-Host usage (consistent with the original script) plus one false positive noted below. No blockers.

⚠️ Should fix (2)

1. -le 1000 single-page heuristic hardcodes the API page size — if the server default changes, a paginated shard could be falsely treated as complete, silently re-introducing the partial-miss bug.
2. Completeness guard checks only the aggregate total, not per-shard health, so a systematically under-fetching shard can be masked by growth elsewhere.

💡 Suggestions (1)

3. PSReviewUnusedParameter false positive on $CollectKey (used only inside the nested -OnItem closure, which the analyzer can't trace). Resolvable with a suppression attribute if CI treats this rule as an error.

[RolandKrummenacher]

Thanks @flanakin. Addressed all three in 7037999:

1. -le 1000 hardcodes the page size — fixed. Get-RetailPriceSegment now returns @{ Items; Pages }, and the "single page = complete, skip repeats" shortcut keys off Pages -le 1 (i.e. the API returned no NextPageLink) instead of comparing the item count to a hardcoded 1000. A change to the server's default page size can no longer make a genuinely paginated shard look complete.

2. Completeness guard was aggregate-only — added a per-shard guard. The published CSV carries only MeterId + the two flags (no serviceFamily), so there was no per-shard baseline to compare against. Invoke-ShardedUnion now returns per-family ShardCounts, which are persisted in a sidecar CommitmentDiscountEligibility.shardcounts.json next to the CSV. Each run compares every shard against its own baseline using the same MaxShrinkFraction floor and aborts before writing if any family under-fetches — so a single sick shard can no longer be masked by growth elsewhere. The first run (no sidecar yet) skips the check, mirroring the aggregate guard's cachedTotal -gt 0 behavior. The workflow now pushes the sidecar alongside the CSV so the baseline survives each fresh CI checkout.

3. PSReviewUnusedParameter false positive on $CollectKey — suppressed. Added [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSReviewUnusedParameter', 'CollectKey', …)] on Invoke-ShardedUnion's param block (it's only invoked inside the -OnItem closure the analyzer can't trace). PSScriptAnalyzer now reports only the pre-existing Write-Host warnings.

Verified: script parses clean; Get-ShardShortfall + JSON round-trip unit-tested (first-run skip → pass, within-tolerance → pass, under-fetch → abort, shard-to-zero → abort, new shard → skipped), and space-containing family names (e.g. Azure Arc) survive serialization.

[flanakin]

🤖 [AI][Claude Opus 4.8] Follow-up review — $ShardList is a silent allowlist (live data loss today)

Digging into how $ShardList is managed surfaced a current correctness bug, not a hypothetical. I verified everything below against the live Retail Prices API.

The bug: because every shard is a positive serviceFamily eq '<x>' filter and the union of shards is the whole dataset, any family not in $ShardList is never queried. A full scan of priceType eq 'Reservation' / meterRegion='primary' today returns the family AI + Machine Learning with 168 meters — and it is not in $ShardList, so those meters are silently dropped right now. The -MaxShrinkFraction guard can't catch it: 168 is ~0.2% of the total, far under the 15% floor. Renames (these families get renamed periodically) fail the same silent way.

API constraints I confirmed while looking for a cheap canary:

• serviceFamily in (...) and not (serviceFamily in (...)) are both rejected (BadRequest) — the API supports only a small OData subset (eq, ne, and/or).
• A negative ne-AND canary caps at ~20 clauses (21+ → BadRequest), so it can't cover the 26-family list in one request.
• There is no hard result cap — I paged a single shard past $skip=49000 and NextPageLink was still being served; traversal ends only at NextPageLink=null. So a discovery scan that just collects the distinct family set is safe (the ordering instability drops scattered rows, never an entire family).

Recommended fix (also makes the job faster):
Fetches are latency-bound (~600ms/page; ~2.5× faster at 5 concurrent in my test), and the shards are independent — so run them with bounded parallelism (ForEach-Object -Parallel -ThrottleLimit 5-8; unbounded will trip the 429s the script already backs off on). Two viable shapes:

1. Discover then fan out: one quick scan for DISTINCT serviceFamily, then shard over the discovered set in parallel. Self-correcting — picks up AI + Machine Learning and future families automatically; the parallelism pays for the extra pass.
2. Keep the list + reconcile (smallest change): fan out the known shards in parallel and, riding along on the responses you're already pulling, collect every serviceFamily seen; abort if a seen family isn't in $ShardList. No extra requests — converts today's silent drop into a loud, actionable failure.

Either closes the silent-data-loss hole; #1 removes the maintenance burden entirely. Happy to expand on whichever direction you prefer.

[RolandKrummenacher]

Thanks @flanakin — great catch, and verified. Fixed in 9894db4 by going with option 1 (discover then shard).

What changed: the hardcoded $ShardList is gone. A new Get-ServiceFamily does one scan per price type to collect the distinct serviceFamily set, then Invoke-ShardedUnion shards over the discovered families. The discovery scan is a long traversal so it's exposed to the ordering drop, but — exactly as you found — that only loses scattered rows, never an entire family, so the family set is reliable; the per-family union-to-stable fetch still recovers the dropped rows. AI + Machine Learning (and any future/renamed family) is now picked up automatically, so the maintenance burden is gone too.

On option 2 (reconcile against the list): I don't think it can actually close the hole — every shard is a positive serviceFamily eq '<x>' filter, so the responses only ever contain families already in the list. An omitted family is never "seen", so there's nothing to abort on. That's why I went with discovery rather than the smaller change.

Belt-and-suspenders: I also hardened Get-ShardShortfall to iterate the baseline families instead of the current run's. Before, a family that vanished entirely from a run was skipped by the per-family guard (it only looked at families present this run). Now a disappeared family (discovery miss, rename) trips a 100% shortfall against its baseline.

On parallelism: I kept the fetch sequential for now. ForEach-Object -Parallel would need the Get-RetailPriceSegment/-OnItem closures and the shared union hashtable reworked across runspaces, which is real risk in a data-integrity script for a scheduled job where wall-clock isn't critical. Happy to add bounded -ThrottleLimit 5-8 parallelism as a follow-up if you'd like it to offset the extra discovery pass — your call.

Data refresh: I didn't regenerate the committed CSV here — the opendata-commitment-eligibility.yml workflow is the established path (it runs the script and surfaces a data-update PR). The next scheduled run (or a manual workflow_dispatch) will pick up AI + Machine Learning and produce that PR. Let me know if you'd rather I fold a fresh regen into this PR instead.

[Copilot]

Pull request overview

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

[RolandKrummenacher]

@flanakin heads-up on a follow-up since your last review — regenerating the data locally surfaced that the discovery rewrite alone didn't actually close the blocker, so I want to flag it explicitly.

After switching to dynamic discovery, AI + Machine Learning was discovered, but its sharded fetch still returned 0 meters. Root cause: the family name is interpolated into $filter, and a query-string + decodes server-side to a space — so serviceFamily eq 'AI + Machine Learning' matched nothing and your 168 meters were still being dropped. Verified against the live API:

• serviceFamily eq 'AI + Machine Learning' (raw +) → 0 rows
• serviceFamily eq 'AI %2B Machine Learning' (encoded) → 168 rows

Fixed in ff6892d by escaping the value with [uri]::EscapeDataString (+ → %2B, spaces → %20; the server decodes back to the literal before OData parsing). AI + Machine Learning is the only currently-discovered family containing +; space-only families were already working.

Regenerated the open data in 166d47b: +119 net meters (92,163 → 92,282) — AI + Machine Learning (82 RI-eligible) plus Blockchain, Microsoft 365 Copilot, Windows 365, and other families the old hardcoded allowlist never queried. Also added the per-shard baseline sidecar (CommitmentDiscountEligibility.shardcounts.json) that the completeness guard reads.

All four of your review threads are addressed/resolved. Ready for another look when you have a moment.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

.github/workflows/opendata-commitment-eligibility.yml:32

• The workflow’s “No changes detected” check only considers CommitmentDiscountEligibility.csv. If the shard baseline JSON changes but the CSV does not, the job will exit early and won’t push the updated shardcounts file (even though it is added/committed later). Include CommitmentDiscountEligibility.shardcounts.json in both the diff and untracked-file checks.

          # Check for changes (handles both new file and modified file)
          if git diff --quiet --exit-code -- src/open-data/CommitmentDiscountEligibility.csv 2>/dev/null && \
             ! git ls-files --others --exclude-standard | grep -q 'CommitmentDiscountEligibility.csv'; then
            echo "No changes detected"

[Copilot]

Pull request overview

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

[flanakin]

🤖 [AI][Claude Opus 4.8] PR Review (re-review)

Summary: The implementation has been substantially reworked and every prior finding is addressed — dynamic serviceFamily discovery unioned with the prior-run baseline (no more hardcoded allowlist; AI + Machine Learning is now captured, 82 RI meters), URL-encoding of the + in the shard filter, a per-shard completeness guard that iterates the baseline so a vanished/under-fetching family is caught, page-count-based single-page detection replacing the brittle -le 1000 heuristic, [ValidateRange(0,1)] on -MaxShrinkFraction, the verified empty-target lint suppression, and a new unit-test file. I verified locally: script and tests parse, PSScriptAnalyzer is clean (only pre-existing Write-Host), 8/8 Pester tests pass, the CSV is well-formed (LF, no BOM, fully quoted, ordinally sorted), and the sidecar round-trips. Nicely done. One should-fix below; no blockers.

⚠️ Should fix (1)

1. The new CommitmentDiscountEligibility.shardcounts.json is an internal guard-baseline artifact, but Package-Toolkit.ps1 packages src/open-data/*.json, so it would ship in the public release alongside real reference data. Recommend excluding it from packaging or relocating the sidecar.

[flanakin]

🤖 [AI][Claude Opus 4.8] ⚠️ Should fix

This sidecar is an internal operational artifact (per-shard baseline counts for the completeness guard), but it lands in src/open-data/ where Package-Toolkit.ps1's Copy-OpenDataFiles copies src/open-data/*.json verbatim into the public release package (next to legitimate reference data like ResourceTypes.json). So CommitmentDiscountEligibility.shardcounts.json would ship to customers as if it were reference data, which it isn't.

It also matches the src/open-data/*.json trigger in opendata-ci.yml, so each weekly auto-update fires Open Data CI — harmless (Build-OpenData only processes the CSV allowlist), but worth being aware of.

The packaging leak is the real one. A couple of options:

• Exclude this file in Copy-OpenDataFiles (e.g. -Exclude '*.shardcounts.json'), or
• Relocate the sidecar outside the packaged glob (e.g. a non-open-data path, or a dotfile/subfolder the packager skips) — though that complicates the workflow's checkout/commit step, so an exclude is likely simpler.

Not a blocker, but it ships an internal build-state file publicly until resolved.