Repository navigation
Follow-up to #3187: pre-size the module stream; state what the availability observation reads - #3188
Conversation
… reads SealedModuleMvidOf now sizes its MemoryStream from the zip entry's length instead of growing a default one, and ArtifactsForIdentity's summary states the real cost — every bundle's manifest, plus each sealed module bundle's entry assembly inflated into memory for its PE metadata — rather than "one PE header, never assembly bytes". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4xPnGYEbdu8AVvR5jytyj
There was a problem hiding this comment.
🟡 Changes recommended
The new checked((int)entry.Length) can throw OverflowException (not handled by the per-module unreadable catch), potentially failing the entire identity observation instead of recording a scoped refusal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Follow-up to #3187 that refines PublishedBundleCatalogue’s availability observation by (1) documenting the true IO/memory cost of the observation and (2) pre-sizing the in-memory buffer used when inflating a sealed module’s entry assembly to read PE metadata (MVID).
Changes:
- Update
ArtifactsForIdentityXML summary to describe that it reads every bundle manifest and inflates each sealed module bundle’s entry assembly to read its MVID. - Pre-size the
MemoryStreamused bySealedModuleMvidOffrom the zip entry’s uncompressedLength.
File summaries
| File | Description |
|---|---|
| src/MeshWeaver.PluginCatalog/PublishedBundleCatalogue.cs | Clarifies observation cost in docs and pre-sizes module-assembly buffering when extracting MVID from sealed module bundles. |
Review details
- Files reviewed: 1/1 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.
…an overflow checked((int)entry.Length) could throw OverflowException on corrupt zip metadata, which ArtifactsForIdentity's per-module handling does not catch — the whole identity would read as unreadable instead of one source recording a named refusal. Guard the length and throw InvalidOperationException, which that handler already turns into the source's Refusal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4xPnGYEbdu8AVvR5jytyj
Test Results (shard 3)341 tests 339 ✅ 43s ⏱️ Results for commit fd2dd16. |
Test Results (shard 0)243 tests 243 ✅ 2m 11s ⏱️ Results for commit fd2dd16. |
Test Results (shard 1)448 tests 448 ✅ 1m 1s ⏱️ Results for commit fd2dd16. |
Test Results (shard 5) 4 files 4 suites 1m 36s ⏱️ Results for commit fd2dd16. |
Test Results (shard 4)2 188 tests 2 188 ✅ 2m 33s ⏱️ Results for commit fd2dd16. |
Test Results (shard 2)578 tests 386 ✅ 2m 39s ⏱️ Results for commit fd2dd16. |
Test Results 15 files 15 suites 10m 45s ⏱️ Results for commit fd2dd16. |
Follow-up to #3187: pre-size the module stream; state what the observation reads
Two review threads on #3187 landed after its branch was locked in the merge queue (the protected-branch hook rejected the push), so they are addressed here:
PublishedBundleCatalogue.SealedModuleMvidOfsizes itsMemoryStreamfrom the zip entry'sLengthinstead of growing a default one — the availability poll inflates one module assembly per sealed module bundle, and that should not reallocate its way up on a large module.ArtifactsForIdentity's summary said "one PE header per module; never a bundle's assembly bytes". It now states the real cost: every bundle's MANIFEST, plus each sealed module bundle's entry assembly inflated into memory for its PE metadata — never a content bundle's NodeType assembly bytes.No behaviour change;
Memex.Portal.Shared.Test622/622 andMeshWeaver.PluginCatalogbuild-c Release -warnaserrorclean. Pure-internal — no What's New entry.Refs #3175.
🤖 Generated with Claude Code
Claude-Session: https://claude.ai/code/session_01G4xPnGYEbdu8AVvR5jytyj