Repository navigation
fix(#4367): a dropped native or RID-specific managed asset REFUSES the pack — unless something named carries it - #4444
Merged
Conversation
…e pack — unless something named carries it The last stage of #4126. `module-pack --deps-closure` used to print two findings as `warning:` and pack anyway, leaving the module on the host-supplies-it fallback: a RID-specific MANAGED assembly (the flat closure has one slot per name and no way to choose a RID) and a native at a layout the loader never probes. It now exits 2, writes nothing, and names the package, the path and the exact value that carries it. Armed on a measurement (the #3240 shape): the full module-pack wave of both publishing repos printed zero of either finding — MeshWeaver.Plugins run 34939850754 (41 bundles, 3 on this --deps-closure path) and MeshWeaver.SocialMedia run 34941532528 (1 bundle, on it). The refusal is SATISFIABLE by its own advice. deps.json keeps declaring the asset after the author follows the advice, so a refusal keyed on the findings alone would refuse the fix too. A finding is refused only when nothing the caller named carries it: * `--with <file name>` (named, and in the bundle as written) — for either shape. A RID-agnostic copy that merely rides by derivation does NOT count: that is the silent "which RID's copy" choice the refusal stops; * for a native, a payload at exactly runtimes/<its rid>/native/<its file name> — from --with-native or from the derivation — the slot the loader probes for that RID and name. DepsClosure.Result gains `Uncarried` (an init property — no constructor change) carrying the findings as data; `Warnings` is now exactly their `Describe()` text. A RID fallback reads the key's second segment only under `runtimes/`, so a ridless native is never advised an invented slot. Not armed on the container lane, and why: its derivation (NativeContributions.DeclaredBy) drops both shapes WITHOUT A LINE, so no measurement of it exists — arming there would be a refusal on faith. Recorded in Modules.md with the lane's missing per-module --with hook. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical native path matching issue remains unresolved, and the required What's New entry is missing.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens module-pack --deps-closure against silently dropped managed and native assets.
Changes:
- Adds structured uncarried-asset detection and carrier-aware refusals.
- Adds refusal, remediation, and derivation tests.
- Documents behavior and remaining scope.
File summaries
| File | Summary |
|---|---|
test/MeshWeaver.Graph.Test/ModulePackCommandTest.cs |
Tests refusal and remediation scenarios. |
test/MeshWeaver.Graph.Test/DepsClosureTest.cs |
Tests structured findings and guidance. |
src/MeshWeaver.Plugin.Build/ModulePackCommand.cs |
Implements carrier-aware refusal logic. Critical (3 votes): native lookup is case-insensitive despite ordinal path matching, potentially bypassing refusal for differently cased entries. |
src/MeshWeaver.Plugin.Build/DepsClosure.cs |
Detects and describes uncarried assets. |
src/MeshWeaver.Documentation/Data/Architecture/Modules.md |
Documents packaging behavior. Nit (1 vote): add the required What's New Fix entry. |
Review details
Suppressed comments (1)
src/MeshWeaver.Documentation/Data/Architecture/Modules.md:764
- This changes the externally consumed
module-packCLI from warning-and-success to an exit-2 refusal, so module authors can directly observe the fix even if portal end users cannot. Please add the required What's NewFixentry for this user-visible behavior rather than treating the absence of a portal UI change as an exemption.
**What is refused.** `module-pack --deps-closure` exits 2, writes nothing, and prints one `error:`
line per finding — naming the package, the declared path and the value that carries it — for each
asset the module's own closure DECLARES and the derivation cannot carry: a native at a layout the
loader does not probe, or a RID-specific MANAGED assembly. Both used to print `warning:` and pack,
leaving the module on the "the host happens to supply it" fallback.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
… the container lane NAMES both shapes Review of #4444: the refusal prescribed --with / --with-native, but node-repo-module-pack.yml's sdk path passes only --deps-closure — so the first CI-built module to trip it would meet a message naming a step nobody can take in the only lane that runs it. 1. A per-module remedy the shared lane applies. A module declares its carrier in its OWN csproj: <MeshWeaverPackWith Include="<file>" /> -> --with <file> <MeshWeaverPackWithNative Include="runtimes/<rid>/native/<file>" /> -> --with-native <path> The sdk path reads them with `dotnet msbuild -getItem` (JSON on SDK >= 8; the lane runs 10.0.x) under the build's properties and appends one flag per item; a failed evaluation or an answer that is not the JSON shape is RED, never "declares none". The refusal now prints the csproj lines to paste per finding — a Copy after Publish from the key the portable publish lays the file out at, plus the item — while still naming --with / --with-native for a local run. Measured end to end on SDK 10.0.400: System.IO.Ports 9.0.9 (a real RID-specific managed asset) plus a native at runtimes/linux-x64/nativeassets/ — refused, lines pasted verbatim, republished, the lane's own block run with real -getItem, packed (bytes = the unix copy; native at the slot). Executed in CI by .github/scripts/test-module-pack-with.py (wired into dotnet-test.yml). 2. The container lane NAMES both shapes as warnings, never refuses. NativeContributions.UncarriedBy names what DeclaredBy filtered out silently (image `native` section, shelf runtimeTargets) and the shelf's RID-specific managed assets; PrivateClosure.Result.Uncarried carries them; the builder prints one `warning:` line each. The finding wording is ONE spelling shared by both lanes (MeshWeaver.Plugin.Packaging.UncarriedAssetFindings), so one grep measures both. Arming a refusal there is #4445, on the same criterion #4367 was armed on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… spelling reaches the bundle CarrierOf matched the --with name with the kind-specific comparer (ordinal for a native) but looked it up in the closure ignoring case. `--with` joins the closure only when no case-insensitive match is already there, so a native `LIBODD.DLL` named beside a managed `libodd.dll` never enters it — yet the ignore-case lookup found the managed file and read the native as carried: the refusal bypassed for bytes that never reach the bundle (Copilot, thread 4018652584). Both lookups now use the same comparer. Ordinal cannot miss a native that IS carried: one that entered via --with sits in the closure under exactly the spelling it was named with. Pinned by DepsClosure_ANativeNamedWithWith_DifferingOnlyInCaseFromAManagedFileInTheClosure_IsRefused, which asserts the refusal's own words (so it cannot pass by way of "--with file not found" on a case-sensitive filesystem); red with the ignore-case lookup put back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi
marked this pull request as ready for review
September 15, 2026 18:26
rbuergi
added this pull request to the merge queue
Sep 15, 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 #4367 — the last stage of #4126. Follow-up: #4445 (arming the container-lane refusal). Draft on purpose: held for review before undrafting (undrafting arms auto-merge).
What changes
module-pack --deps-closureREFUSES (exit 2, nothing written) the two shapes a bundle cannot carry by derivation, unless something the module NAMES carries them. The two shapes are a RID-specific MANAGED assembly and a native at a layout the loader does not probe.node-repo-module-pack.yml's sdk path reads it and appends the flag.warning:lines in the same wording, and does not refuse. Arming it is Arming the container-lane refusal is blocked on the FINDING, not on a wave: a RID-specific asset for a RID the bundle does not target is not uncarried (#4367 follow-up) #4445.Why
Closes #4367#4367 asked to flip
ModulePackCommand's two warnings to a non-zero exit, armed on a measured wave, with a test and a negative control per shape. That is done, and the prescribed remedy is now a step the only lane that runs it can take. The container lane was explicitly "not part of this" in #4367. It no longer drops silently: it names both shapes in the same words, so its own wave can be measured. Its refusal is #4445, on #4367's criterion.🚨 The refusal is satisfiable by its own advice
deps.json keeps declaring the asset after the fix, so a refusal keyed on the findings alone would refuse the fix too. A finding is refused only when nothing the module named carries it (
ModulePackCommand.CarrierOf):…/<file>--with <file>, named and in the bundle. The RID-agnostic copy that merely rides does not count<MeshWeaverPackWith Include="<file>" /><rid>--with-native runtimes/<rid>/native/<file>, or--with <file>(flattened, the loader's last probe)<MeshWeaverPackWithNative Include="runtimes/<rid>/native/<file>" />, or<MeshWeaverPackWith Include="<file>" />Comparisons are ordinal for natives and ignore case for assemblies. A native whose RID cannot form the probed layout is offered only the flat carrier; no slot is ever invented.
The refusal prints the csproj lines to paste, per finding: a
CopyafterPublishplus the item. A portable publish (what the lane packs) lays everyruntimeTargetsasset out at its declared key, so no package-cache path is needed:The lane's read (sdk path, right after
--deps-closure) runsdotnet msbuild <csproj> -getItem:MeshWeaverPackWith -getItem:MeshWeaverPackWithNative, under the build's properties.-getItemhas existed since SDK 8. The pack job installs10.0.x.--with/--with-nativein$packargs.Measured end to end on the real SDK (10.0.400). The module references
System.IO.Ports9.0.9, which has a real RID-specific managed asset, plus a local package declaringruntimes/linux-x64/nativeassets/net10.0/libodd.so.-getItem. It produced--with System.IO.Ports.dll --with-native runtimes/linux-x64/native/libodd.so.System.IO.Ports.dllis byte-identical to the unix copy, andlibodd.sosits at the probed slot.Measurement the SDK refusal was armed on
--deps-closure(the refusal's population)Container lane: NAMED, not refused
Until now its derivation dropped both shapes without a line:
NativeContributions.DeclaredByfiltered an unprobed native out of the image'snativesection and the shelf'sruntimeTargets;Now:
NativeContributions.UncarriedBynames both shapes;PrivateClosure.Result.Uncarriedcarries them;ProjectBuild.ReportUncarriedprints onewarning:line each.The finding sentence is one spelling shared by both lanes (
MeshWeaver.Plugin.Packaging.UncarriedAssetFindings), so one grep measures both. The remedy it offers is one this lane can take: build on the sdk path and declare the csproj item. The image cannot drop a RID-specific managed asset, because as a RID-specific publish it has already resolved that RID's copy intoruntime.Red/green
NC1 through NC4 each rebuilt the test project. NC-C and NC-D mutate the workflow and re-run the harness.
DepsClosure_ARidSpecificManagedAsset_NothingNamesIt_RefusesThePackDepsClosure_AnUnprobedNative_NothingCarriesIt_RefusesThePackDepsClosure_AnUnprobedNative_IsNotCarriedBySomethingNamedThatIsNotIt×2DepsClosure_ARidSpecificManagedAsset_NamedWithWith_PacksDepsClosure_AnUnprobedNative_LaidOutAtTheProbedSlotWithWithNative_PacksDepsClosure_AnUnprobedNative_FlattenedAndNamedWithWith_PacksDepsClosure_TheSharedLaneCsprojLinesInTheRefusal_WhenApplied_LiftIt×2DepsClosureTest.ANativeWhoseRIDIsUnknown_IsOfferedOnlyTheFlatCarrier_NeverAnInventedSlotNativeClosureTest:APORTABLERecordsUncarriedShapesAreNAMED…,ARIDSpecificImagesNativeAtAnUnprobedKey_IsNamedToo,EveryNamedFindingIsOneWARNINGLine,BothLanesNameTheSameFinding_inTheSameWordstest-module-pack-with.py(8 cases)My first NC2 attempt didn't compile and ran against a stale binary. I discarded it and re-ran.
Builds, runs, guards
dotnet build -c Release -warnaserror, one project per invocation, gave 0 Warning(s) and 0 Error(s) for each of:MeshWeaver.Plugin.Packaging,MeshWeaver.Plugin.Build,MeshWeaver.PluginTester,MeshWeaver.Documentation,MeshWeaver.Graph.Test,MeshWeaver.PluginTester.Test,MeshWeaver.Compiler.Pipeline.Test.Workflow guards all green:
check-workflow-timeouts(87 jobs, 0 violations),check-workflow-yaml-keys,check-workflow-permission-pairing(both forms),check-pr-secret-preflight,check-workflow-shell(0 live),test-module-pack-test-runner.py, andtest-module-asset-landing.py.check-record-signatureswas also green: nothing public was removed, and records only gainedinitproperties.No What's New entry: a portal user cannot notice this. It is a pack-tool refusal with zero current victims plus a build-lane log line, and module authors read the refusal itself and
Doc/Architecture/Modules.🤖 Generated with Claude Code