Repository navigation
ci(#3349): refuse a PackageVersion removal nobody has checked against the satellites - #3368
Merged
Merged
Conversation
… the satellites MeshWeaver.Plugins' src/Directory.Packages.props IMPORTS this repo's, so a satellite project may carry a versionless PackageReference whose only version source is an entry here. Remove it — even as collateral — and the satellite stops restoring with NU1010 while BOTH repos stay green: nothing here consumes the package, and the satellite pins this repo, so its CI reads the old list until somebody moves the pin on a different day. main-cd is the first lane that notices, and by then no set seals. #3344 sealed nothing for ~80 minutes and reintroduced GHSA-2m69-gcr7-jv3q as NU1903. NOT THE CONTROL THE ISSUE PROPOSED, and the reason matters. #3349 specified checking out MeshWeaver.Plugins in core's PR lane and restoring it. That cannot be built: PlatformNeverDependsOnPluginsGuard.ThePullRequestGate_ReachesIntoNoPluginRepository asserts dotnet-test.yml holds ZERO actionable cross-repo hits, and its matcher counts both `repository: Systemorph/MeshWeaver.Plugins` and any line reading `plugins-repo`. It would have failed on its first run. The ban is the point, not an obstacle — the guard names the exact harm: "a gate on core's own pull requests whose verdict depends on another repository's moving HEAD makes the SAME diff go red or green with no change of its own", and "re-adding a checkout silently undoes" the clearing of that workflow's last external input. The repo had already weighed the trade the issue proposed and decided against it. So this does what the repo already does for the same SHAPE one category over. A change here that can silently break a satellite gets DECLARED, exactly as Pairs-with: declares a public-surface counterpart — a package pin is that shape without being C#. Core-only: no checkout, no API read, no credential, no ledger entry, nothing that can red on a sibling's HEAD, and it therefore runs on FORK pull requests too, where the credentialed gates cannot. Satellite-pins: <PackageId>[, <PackageId>…] — <what you checked> No blanket form, deliberately: "none of them matter" is the sentence that produced #3344. PROVEN AGAINST THE INCIDENT, not just fixtures. Replaying #3344's own commit c08fee1 against its parent with the three INTENDED withdrawals declared, the gate reds naming exactly the fourth: Undeclared removal(s): • SQLitePCLRaw.lib.e_sqlite3 Premise verified rather than assumed: MeshWeaver.Hosting.Sqlite.csproj consumes that package versionless, and core references it in no csproj at all. FAILS CLOSED. An undecidable read — a shallow fetch missing the merge base, a moved props file, a git failure — raises and exits RED naming why, never "nothing was removed"; and a starved list (<50 entries where main has 151) is refused as a broken matcher rather than trusted as a shrunken list. "Couldn't tell" and "clean" must never be one colour: that is how #3344 passed with every check green. Self-test proves both directions plus the shapes that decide whether the matcher is honest — fenced, commented, quoted, reasonless and mis-named declarations are all refused, a partial declaration names only what is still undeclared, and the undecidable paths raise rather than return clean. CrossRepoPairGate's "eighth shape" said "There is NO guard for this yet" and recommended the checkout; both are now false, and the section is rewritten to say which two controls are wrong and why. Verified: check-workflow-timeouts 66 jobs / 0 violations; check-workflow-shell clean; Documentation.Test guards 10/10 including PlatformNeverDependsOnPluginsGuard; DocumentationLinkIntegrityTest green; the gate run against its own diff reports 0 removed. Satellite-pins: none removed by this change — the gate's own run on this diff reports "0 removed", so there is nothing to declare Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RpJi9EZHtGYQCahUwWa7jH
Contributor
Test Results (shard 1)475 tests 475 ✅ 36s ⏱️ Results for commit 40adeeb. |
Contributor
Test Results (shard 0)251 tests 251 ✅ 2m 4s ⏱️ Results for commit 40adeeb. |
Contributor
Test Results (shard 3)962 tests 962 ✅ 1m 1s ⏱️ Results for commit 40adeeb. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new gate has a few confirmed correctness/robustness issues (case-sensitive removal detection, overly strict PR-body parsing vs existing gates, and a potentially insufficient base-branch fetch depth) that can cause false failures or incorrect “removal” reports.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new core PR gate to prevent silent breakage of satellite restores when a centrally-managed PackageVersion is removed from Directory.Packages.props. The workflow now fails unless each removed package ID is explicitly declared (with what was checked) in the PR body, and the documentation is updated to describe this “eighth shape” and why the previously-recommended “restore Plugins in core PR CI” approach is intentionally banned.
Changes:
- Introduce
scripts/check-package-pin-removal.pyto detect removed<PackageVersion Include="...">entries and require matchingSatellite-pins:declarations in the PR body. - Wire the new “Satellite package pins (removal declared)” job into
.github/workflows/dotnet-test.ymland make it block the requiredConsolidate test resultscheck. - Update
Doc/Architecture/CrossRepoPairGateto document the new control and the reasoning behind it.
File summaries
| File | Description |
|---|---|
src/MeshWeaver.Documentation/Data/Architecture/CrossRepoPairGate.md |
Updates the “eighth shape” guidance to describe the new declaration-based gate and why cross-repo restore-in-core-PR is banned. |
scripts/check-package-pin-removal.py |
New detector + PR-body parser + self-test for enforcing declared removals of central package pins. |
.github/workflows/dotnet-test.yml |
Adds the new gate job, captures PR body, computes merge-base, runs the detector, and makes the required check fail closed if the gate didn’t succeed. |
Review details
Suppressed comments (1)
scripts/check-package-pin-removal.py:238
- The self-test doesn’t currently cover the bullet/bold declaration format that GitHub PR bodies commonly use. If support for bulleted/bolded
Satellite-pinsis intended, add an explicit self-test case so the gate can’t silently regress back to rejecting it.
# Accepts a declaration, including several ids on one line and case differences.
body = "Satellite-pins: SQLitePCLRaw.lib.e_sqlite3, Other.Pkg — grepped both, no versionless hit"
check("declared passes", evaluate({"SQLitePCLRaw.lib.e_sqlite3", "Other.Pkg"}, body)[0], 0)
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - name: "Every removed PackageVersion is declared" | ||
| run: | | ||
| set -euo pipefail | ||
| git fetch --no-tags --depth=200 origin "${{ github.base_ref }}" |
Comment on lines
+71
to
+74
| DECLARATION_RE = re.compile( | ||
| r"^\s*Satellite-pins\s*:\s*(?P<ids>[^\r\n]*?)\s*(?:—|--|-)\s*(?P<reason>\S[^\r\n]*)$", | ||
| re.I, | ||
| ) |
Comment on lines
+164
to
+166
| before = ids_in(before_text) | ||
| after = ids_in(head_path.read_text(encoding="utf-8")) | ||
| return before - after, len(before), len(after) |
Comment on lines
+311
to
+316
| print( | ||
| f"::error::This gate could not read {PROPS} at the merge base, so it does not know " | ||
| f"whether a PackageVersion was removed: {e}. That is UNDECIDED, not clean — a shallow " | ||
| "fetch that misses the merge base, or a moved props file, would otherwise report a " | ||
| "clean tree forever. Fetch enough history (the lane fetches --depth=200) or repoint " | ||
| "the gate, then re-run.", |
Contributor
Test Results (shard 5) 5 files 5 suites 1m 41s ⏱️ Results for commit 40adeeb. |
Contributor
Test Results (shard 4)1 571 tests 1 377 ✅ 2m 5s ⏱️ Results for commit 40adeeb. |
Contributor
Test Results (shard 2)986 tests 986 ✅ 3m 27s ⏱️ Results for commit 40adeeb. |
Contributor
Test Results 16 files 16 suites 10m 56s ⏱️ Results for commit 40adeeb. |
This was referenced Sep 5, 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
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.
Closes #3349.
Not the control the issue specified, and the reason is the interesting part
#3349 proposed checking out
Systemorph/MeshWeaver.Pluginsin core's pull-request lane and runningdotnet restore. That cannot be built here.PlatformNeverDependsOnPluginsGuard.ThePullRequestGate_ReachesIntoNoPluginRepositoryassertsdotnet-test.ymlcontains zero actionable cross-repo hits, and its matcher counts exactly the twothings that control needs:
It would have failed on its first run. And the ban is the point rather than an obstacle — the guard
names the precise harm the proposal causes:
The repo had already weighed that trade and decided against it. (The issue author has verified this
independently and corrected #3349.)
What ships instead
The same instrument this repo already uses for the same shape one category over. A change here
that can silently break a satellite gets declared — exactly as
Pairs-with:declares apublic-surface counterpart. A package pin is that shape without being C#.
The gate fires only when the diff removes a
PackageVersion, and requires each removed id in thePR body:
There is deliberately no blanket form: "none of them matter" is the sentence that produced #3344.
Core-only — no checkout, no API read, no credential, no ledger entry, nothing that can go red on
somebody else's HEAD. A consequence worth having: it also runs on fork pull requests, where the
credentialed gates cannot.
What it does not do, stated plainly. It does not know whether a removed pin is load-bearing; it
makes a human find out and say what they found. That is weaker than deriving the answer, and it is
the honest price of staying inside the dependency direction. #3344 removed an entry, so it fires.
Proven against the incident, not against fixtures
Replaying #3344's own commit (
c08fee100) against its parent, with the three withdrawals itintended declared, the gate reds naming exactly the fourth:
That is the collateral removal that cost ~80 minutes of sealing nothing and reintroduced
GHSA-2m69-gcr7-jv3q. With an empty body it names all four.
Premise verified rather than assumed:
MeshWeaver.Hosting.Sqlite.csprojconsumes that packageversionless, and core references it in no csproj — the satellite-only case exactly.
Fails closed
An undecidable read raises and exits RED naming why — a shallow fetch missing the merge base, a
moved props file, a git failure — never "nothing was removed". A starved list (< 50 entries
where
mainhas 151) is refused as a broken matcher rather than trusted as a shrunken list.Couldn't tell and clean must never be one colour; that is how #3344 passed with every check green.
Non-vacuity
--self-testruns first and fails the job. It proves both directions plus the shapes that decidewhether the matcher is honest: fenced, commented, quoted, reasonless and mis-named declarations are
all refused; a partial declaration names only what is still undeclared; and the undecidable paths
raise rather than return clean. It also reads the real tree, so a matcher that stopped matching
fails the self-test rather than reporting a clean diff.
Docs
CrossRepoPairGate→ "An eighth shape" said "There is NO guard for this yet" and recommendedthe checkout. Both are now false; the section is rewritten to say which two controls are wrong and
why, and to carry the replay above.
Verification
check-workflow-timeouts.pycheck-workflow-shell.pyDocumentation.TestguardsPlatformNeverDependsOnPluginsGuardDocumentationLinkIntegrityTest0 removed — nothing to declareSatellite-pins: none removed by this change — the gate's own run on this diff reports "0 removed", so there is nothing to declare
🤖 Generated with Claude Code
https://claude.ai/code/session_01RpJi9EZHtGYQCahUwWa7jH