Make Dependabot bumps mergeable without loosening the pins from #24 - #72
Conversation
Every open Dependabot pull request failed validation, and none of the four causes was the hash pinning itself - that gate worked exactly as intended. A bump has to move three files, and Dependabot writes one of them: Build/Dependencies/Dependencies.csproj Dependabot OmadaWeb.PS/DependencyLock.psd1 dependency-lock-sync.yml, by hand Build/Dependencies.psd1 (SBOM) nothing at all So: - The lock sync was workflow_dispatch-only and nobody ran it. It now sweeps every open Dependabot pull request weekly. To keep a write-scoped token away from code on the branch under review it checks out two trees and runs the script from the default branch against the pull request branch as data, through -RepositoryRoot. The manual single-branch run is still there. - -Refresh never updated the SBOM inventory, so the versions it reports drifted from the versions actually loaded and the unit test failed even once the hashes were right. It now refreshes both, and -Check reports the drift. - -Refresh reported the very drift it exists to resolve as a problem and then exited non-zero, so the workflow could never reach its commit step. Manifest and SBOM drift are now informational under -Refresh and fatal under -Check. - Dependabot kept proposing bumps that cannot be taken. #55, #58 and part of #70 moved members of the System.Text.Json closure on their own; the module loads those with Assembly.LoadFrom, which applies no binding redirects, so the versions loaded have to be the ones the pinned System.Text.Json resolves. They are now ignored for version updates, as Legacy/Selenium already was, and still raise advisories. -Check asserts each closure member has an ignore rule, so the lock and the policy cannot drift apart. The guarantee from #24 is unchanged: -Check still fails the build on any mismatch, and syncing does not revalidate a pull request - PR Validation is triggered by /validate, so a maintainer still validates and merges.
|
C:/Program Files/Git/validate |
|
/validate |
There was a problem hiding this comment.
Pull request overview
This PR makes Dependabot NuGet bumps for runtime-downloaded/pinned binaries arrive “mergeable” by automating lock/SBOM synchronization, while keeping the existing hash-pin integrity gate from #24 intact.
Changes:
- Extends
Build/Update-DependencyLock.ps1to keep the SBOM inventory (Build/Dependencies.psd1) in sync and to validate Dependabot ignore policy for the System.Text.Json closure. - Adds a scheduled + sweep-mode
dependency-lock-syncworkflow that syncs pins/SBOM across open Dependabot PR branches using a trusted-script / untrusted-tree model. - Adds/updates unit tests and documentation to cover the new gates and explain the 3-file bump flow.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/Unit/DependencyLock.Tests.ps1 | Adds sandboxed -Check tests plus ignore-policy assertions. |
| SECURITY.md | Documents the new 3-file bump flow and the automated sync trust model. |
| OmadaWeb.PS/DependencyLock.psd1 | Updates System.Text.Json-closure pin rationale to reflect ignore-policy constraints. |
| Build/Update-DependencyLock.ps1 | Adds SBOM drift checking/refresh and Dependabot ignore-policy enforcement; adjusts drift handling under -Refresh. |
| Build/Dependencies.psd1 | Updates closure components’ VersionStrategy text to match the new ignore policy. |
| .github/workflows/dependency-lock-sync.yml | Introduces weekly sweep + manual sync workflow with two-checkout trusted execution. |
| .github/dependabot.yml | Adds ignores for System.Text.Json closure members and clarifies behavior/expectations. |
Suppressed comments (1)
Build/Update-DependencyLock.ps1:426
- In -Refresh, the loop re-parses the same manifest file for every artefact (Get-ManifestVersion), even though the offline check phase already builds a per-manifest cache in $ManifestVersions. This adds unnecessary IO/XML parsing for each branch the workflow syncs; reusing the existing cache is faster and keeps error behavior consistent.
foreach ($Artifact in ($Artifacts | Where-Object { $_.Verification -eq "Sha256" })) {
$ManifestPath = Join-Path $RepositoryRoot $Artifact.Manifest
$Declared = Get-ManifestVersion -ManifestPath $ManifestPath
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $InventoryFile = Join-Path $Root -ChildPath 'Build\Dependencies.psd1' | ||
| (Get-Content -Path $InventoryFile -Raw).Replace('Version = "13.0.4"', 'Version = "13.0.3"') | | ||
| Set-Content -Path $InventoryFile -NoNewline | ||
|
|
There was a problem hiding this comment.
Agreed, fixed in c37a0aa. The versions now come from the lock at runtime via a Get-PinnedVersion helper rather than being written into the test, and Set-DoctoredVersion asserts the text it is replacing was actually present - so a renamed field fails loudly instead of leaving the test passing while exercising nothing. Applied to the manifest-drift test as well, which had the same problem.
Tests: the sandbox tests hard-coded Newtonsoft.Json 13.0.4, so a legitimate bump would have broken tests that are really about "these two files disagree". Versions now come from the lock at runtime, and the doctoring asserts the text it replaces was actually there - a renamed field would otherwise leave the test passing while exercising nothing. Workflow: pull.head.repo is null when a head repository has been deleted, which would have thrown and taken the whole sweep with it; guarded. And the branch name now reaches the commit step through the environment instead of being interpolated into the script, so a branch name cannot become code.
|
/validate |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
Build/Update-DependencyLock.ps1:167
- Get-IgnoredDependencyName only strips optional double quotes from the
directory:value. YAML commonly uses single quotes (or no quotes), and in that case this parser would capture the quotes as part of the directory and fail to match, causing a false build failure.
Consider accepting both quote styles (and resetting $InIgnoreList when a new directory is seen) so the check remains stable even if dependabot.yml formatting changes.
This issue also appears on line 178 of the same file.
if ($Line -match '^\s*directory\s*:\s*"?([^"]+?)"?\s*$') {
$InRequestedUpdate = ($Matches[1] -eq $Directory)
continue
Build/Update-DependencyLock.ps1:180
- Get-IgnoredDependencyName only strips optional double quotes from
dependency-name:values. If the YAML uses single quotes (or no quotes), the captured name can include quotes and then fail to match $Artifact.PackageId, producing a false failure.
Make the regex accept double quotes, single quotes, or bare values and normalize to the captured name.
if ($InIgnoreList -and $Line -match '^\s*-\s+dependency-name\s*:\s*"?([^"]+?)"?\s*$') {
$Names += $Matches[1]
}
…t cache Three findings from the second review pass, all in Get-IgnoredDependencyName and the -Refresh loop. The parser only stripped double quotes. YAML scalars may be double-quoted, single-quoted or bare and all three mean the same string, so reformatting dependabot.yml would have made the closure check read 'System.Buffers' including its quotes, not match the package id, and fail the build over nothing. Quotes are now stripped rather than matched, for both the directory and the dependency-name, and a test rewrites the whole file into the other two styles and asserts the check still passes. A directory line now also ends any ignore list already being read. The ordering in this file never produced that case, but scoping ignore rules to the right manifest is the whole point of the function. -Refresh re-parsed each manifest once per artefact although the offline checks had already built the per-manifest cache. Reuses it.
|
Picked up the three suppressed comments from the second review pass — all three were real, thanks. YAML quoting ( Resetting Re-parsing the manifest per artefact under Re-verified: 19/19 unit tests, |
|
/validate |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
Build/Update-DependencyLock.ps1:439
- In -Refresh mode the script unconditionally reads the SBOM inventory with Get-Content. If Build/Dependencies.psd1 is missing (or -InventoryPath points to a missing file), the script will throw immediately with a generic file-not-found error, bypassing the existing structured problem reporting. Consider failing early with a clear, consistent message before attempting to read the file.
if ($PSCmdlet.ParameterSetName -eq "Refresh") {
$Line = @(Get-Content -Path $LockPath)
$InventoryLine = @(Get-Content -Path $InventoryPath)
$Changed = 0
Tests/Unit/DependencyLock.Tests.ps1:239
- This policy test hard-codes double quotes around dependency-name values (e.g., dependency-name: "System.Memory"). The earlier unit test explicitly asserts that YAML scalars may be single-quoted or bare without breaking -Check, so this test will become a false failure if dependabot.yml is reformatted. Make the assertion quote-agnostic so it verifies the rule rather than the formatting.
This issue also appears on line 246 of the same file.
$Config = Get-Content -Path (Join-Path $Script:RepositoryRoot -ChildPath '.github\dependabot.yml') -Raw
foreach ($Artifact in $Closure) {
$Config | Should -BeLike "*dependency-name: `"$($Artifact.PackageId)`"*" -Because "Dependabot would otherwise propose bumping '$($Artifact.PackageId)' on its own"
}
Tests/Unit/DependencyLock.Tests.ps1:249
- This assertion only matches the Selenium ignore rule if it uses double quotes. Since other tests and Update-DependencyLock.ps1 treat single-quoted or bare scalars as equivalent YAML, this should also be quote-agnostic to avoid a formatting-only test failure.
$Config = Get-Content -Path (Join-Path $Script:RepositoryRoot -ChildPath '.github\dependabot.yml') -Raw
$Config | Should -BeLike '*dependency-name: "Selenium.WebDriver"*'
}
The previous commit taught -Check that YAML scalars may be double-quoted, single-quoted or bare, and then left the two policy tests matching a literal dependency-name: "X". So the tests contradicted the parser: a reformatting of dependabot.yml that -Check accepts would have failed them. Both now go through a Test-IgnoreRule helper that accepts any of the three quotings, which is what the rule actually means. -Refresh read the SBOM inventory unguarded. A missing file was already reported by the offline checks, but the bare Get-Content threw first and replaced that message with a file-not-found. Guarded, so the run ends with the message that explains it. And the "every pin already matches" line no longer prints when problems were reported - it sat directly above the failure and contradicted it.
|
Third pass, three more suppressed comments — all valid, all fixed in d897f5a. The policy tests hard-coded double quotes. This was the sharpest of the three, because it was a contradiction I introduced myself: the previous commit taught Unguarded SBOM read under While confirming that fix I noticed one more thing the review did not flag: with the inventory missing, the run still printed "No changes; every pin already matches its manifest, its published bytes and the SBOM" directly above the failure. Contradicting yourself in a CI log is its own bug, so that line is now suppressed when any problem was reported. Re-verified: 19/19 unit tests, |
|
/validate |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
Tests/Unit/DependencyLock.Tests.ps1:189
- This test removes the ignore rule by matching the exact text
dependency-name: "System.Memory", which makes it brittle to harmless YAML reformatting (single quotes / bare scalars) even though the script under test explicitly accepts those forms. Use a regex that matches any YAML scalar quoting forSystem.Memory.
This issue also appears on line 229 of the same file.
$Line = @(Get-Content -Path $ConfigFile | Where-Object { $_ -notmatch 'dependency-name: "System.Memory"' })
Tests/Unit/DependencyLock.Tests.ps1:229
- Same brittleness here: the ignore-rule removal matches only the double-quoted form (
dependency-name: "System.Memory"). If dependabot.yml is reformatted to single quotes or bare scalars (which the script supports), this test will stop exercising what it intends. Use the same quote-agnostic regex as above.
$Line = @(Get-Content -Path $ConfigFile | Where-Object { $_ -notmatch 'dependency-name: "System.Memory"' })
The two tests that strip a rule from a sandbox config to prove the check fires still matched the literal dependency-name: "System.Memory". That is the same contradiction the previous commit fixed on the asserting side: a reformatting of dependabot.yml that -Check accepts would have left these tests removing nothing, so they would have gone on to assert a failure that no longer had a cause - passing for the wrong reason, or failing for one. Both now go through Remove-IgnoreRule, which matches any of the three YAML quotings and asserts the rule was actually there before removing it. The matching pattern is shared with Test-IgnoreRule so the two cannot drift.
|
Fourth pass — both remaining comments were the same brittleness on the other side of the same coin, and both were right. Fixed in daee00b. The two sandbox tests that strip a rule to prove the check fires still matched the literal Both now go through a Separately, on the previous run: |
|
/validate |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 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.
Build/Update-DependencyLock.ps1:221
Get-InventoryComponentkeys SBOM components byLockIdbut silently overwrites earlier entries when the inventory contains the sameLockIdtwice. That can hide SBOM drift and make-Refreshupdate an arbitrary duplicate. Treat duplicateLockIdas a problem so-Checkfails closed.
foreach ($Component in $Inventory.Components) {
if ([string]::IsNullOrWhiteSpace($Component.LockId)) {
continue
}
$ByLockId[$Component.LockId] = $Component
|
|
||
| $Content = Get-Content -Path $Path -Raw | ||
| $Content | Should -BeLike "*$Find*" -Because "the test needs '$Find' present in '$Path' to doctor it" | ||
| $Content.Replace($Find, $Replace) | Set-Content -Path $Path -NoNewline |
Get-InventoryComponent keys components by LockId and silently kept the last of any duplicates, so a second component mirroring the same artefact would have been dropped from the drift check while -Refresh still rewrote both. A stale version in the shadowed component could then ship unnoticed - a hole in a file whose whole purpose is failing closed. The lock already refuses ambiguous ids. The SBOM now does too.
|
Fifth pass, one comment — and this one was a genuine hole in the shipped logic rather than in the tests, so it is the most worthwhile finding of the review so far. Fixed in 0ee9c5e.
The lock has refused ambiguous ids since it was written; the SBOM now does too, with the same phrasing. Covered by a new test that inserts a duplicate component and asserts Verified: 20/20 unit tests, |
|
/validate |
All four open Dependabot pull requests (#55, #58, #69, #70) fail validation. None of the causes is the
hash pinning from #24 — that gate works exactly as designed, and this PR does not loosen it.
What was actually wrong
A bump has to move three files in step, and Dependabot writes one of them:
Build/Dependencies/Dependencies.csprojOmadaWeb.PS/DependencyLock.psd1(SHA-256)dependency-lock-sync.ymlworkflow_dispatchonly — nobody ran itBuild/Dependencies.psd1(SBOM version)Confirmed on the failing run for #55:
Four distinct defects, fixed here:
The sync was manual. It is now a weekly sweep over every open Dependabot PR, so a bump arrives
already synced. The manual single-branch run stays for use after a rebase.
-Refreshnever touched the SBOM inventory.Build/Dependencies.psd1reports the versions themodule actually loads, and nothing kept it current — so
Tests/Unit/DependencyLock.Tests.ps1failedindependently even once the hashes were right.
-Refreshnow updates it;-Checknow reports thedrift at the same gate that blocks the PR.
-Refreshexited non-zero on exactly the bumps it was invoked to fix. It reported manifestdrift as a problem and then errored, so the workflow's commit step could never run — the manual
path was broken too. Drift is now informational under
-Refreshand still fatal under-Check.Dependabot proposed bumps that cannot be taken. deps: Bump System.Buffers from 4.5.1 to 4.6.1 #55, deps: Bump System.ValueTuple from 4.5.0 to 4.6.2 #58 and part of deps: Bump Selenium.WebDriver and 2 others #70 move members of the
System.Text.Json closure on their own. The module loads those with
Assembly.LoadFrom, whichapplies no binding redirects, so the versions loaded have to be the ones the pinned
System.Text.Json resolves — the lock file already documents this. Automating the hash refresh
without addressing it would have turned a red PR into a silently wrong one. They are now ignored
for version updates, exactly as
Legacy/Selenium.WebDriveralready was, and still raiseadvisories.
Keeping it from rotting
-Checknow asserts that everyGroup = "SystemTextJson"artefact has an ignore rule in.github/dependabot.yml. Adding a closure member to the lock without ignoring it fails the build,instead of producing another perpetually-red PR months later.
Trust model of the sweep
The sweep pushes to a branch it did not write, so it must not execute anything from it. It checks out
two trees — the default branch and the PR branch — and runs the script from the default branch against
the other one as data via
-RepositoryRoot. Nothing from the branch under review runs with thewrite-scoped token, and no
pull_request_targetis involved. The Dependabot config is read from thetrusted tree because that is the copy Dependabot itself obeys (it only reads
dependabot.ymlfrom thedefault branch).
The previous rationale for keeping this manual — "workflows triggered by Dependabot get a read-only
token" — is true for
pull_requestevents but does not apply to a scheduled run, which executes fromthe default branch with a normal token.
The
#24guarantee is unchanged.-Checkstill fails PR validation and the release on anymismatch. Syncing does not revalidate a PR — PR Validation is
/validate-triggered — so a maintainerstill validates and merges deliberately. Only the mechanical part is automated.
Verification
Update-DependencyLock.ps1 -Check -SkipDownloadclean on pwsh 7 and Windows PowerShell 5.1.rewrites version/URL/hash in the lock and the version in the SBOM, two lines per file, comments
and alignment byte-identical;
-Checkclean afterwards; exit code 0.ignore rules — it syncs and exits 0, so the commit step is reachable.
ignore rule, and an ignore rule placed under the wrong manifest directory.
Tests/Unit/DependencyLock.Tests.ps1: 18 passed, 0 failed (7 new cases).New-Sbom,Invoke-DownloadFile,BundledWebView2unaffected.Follow-up (not in this PR)
Merging #69/#70 themselves. After this lands they will need
@dependabot rebase(PR Validationrefuses branches behind main), then a sweep run. #55 and #58 need closing by hand — an ignore rule
does not retroactively withdraw an open PR.