feat: warn on the Az.Storage + EXO OData conflict (Test-DPLibraryConflict, #174 Phase 2)#231
Conversation
Data-driven knownConflicts (in dependency-policy.json, build-shipped as a small KnownConflicts.json subset), a Private detector, a public Test-DPLibraryConflict, and an Import-DPLibrary one-shot armed AssemblyLoad handler that warns when the Az.Storage + ExchangeOnlineManagement OData pair becomes co-loaded. Plus the OData evidence cross-ref, a known-limitation doc, the both-orders-fail #174 test, and a findings comment (issue kept open). OData stays block (unfixable by preload, per the Phase 1 runtime evidence). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
documentation
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| BestPractice | 3 minor |
| CodeStyle | 1 minor |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches. Re-running this action after a short time may resolve the issue. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
Adds a Phase 2 design spec for Issue #174 describing a data-driven knownConflicts model, how a shipped conflict subset would be consumed at runtime, and how DLLPickle would warn users when incompatible modules (Az.Storage ↔ ExchangeOnlineManagement) are co-loaded.
Changes:
- Introduces a new design spec outlining
knownConflictsJSON schema and build-shipped runtime subset (KnownConflicts.json). - Proposes a private conflict detector + public
Test-DPLibraryConflictcommand andImport-DPLibrarywarning integration (one-shotAssemblyLoadhandler). - Documents expected tests (unit + integration characterization for both import orders).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a09c8305d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8-task TDD plan: knownConflicts data + build-shipped KnownConflicts.json, private reader/detector/formatter, public Test-DPLibraryConflict, Import-DPLibrary armed one-shot AssemblyLoad warning, known-limitation doc, both-orders #174 test (shared OData-slot synthetic), and the #174 findings comment (keep open). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…evidence cross-ref
…mmediate + armed handler)
…rst fails on Az.Storage import)
SamErde
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50b8179c66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Warn only when the WHOLE pair is co-loaded: the AssemblyLoad handler now tracks per-module "seen" and warns/unregisters only once every watched module has loaded (was false-warning + consuming the one-shot when just one module loaded). - Watch ALL installed version base paths per not-yet-loaded module (not just the highest), so a version-pinned/by-path import is still detected. - Idempotent: a module-scoped $script:DPConflictHandled set warns/arms at most once per conflict per session (no stacked handlers / duplicate warnings on repeated Import-DPLibrary). - Perf: query Get-Module -ListAvailable only for the conflict's own module names. - Normalize paths (GetFullPath) before the prefix check; guard empty Location. - Workaround text: separate PROCESS (job / second pwsh), not a same-process runspace (process-wide ALC) - corrected in policy, Deep-Dive, and the spec. - Document the best-effort limits (failed-load raises no event; Get-Module-based) in the function help + spec; clarify the spec's extractor (TestDrive) sync test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e/CI hardening) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@claude[agent] review |
I've reviewed the PR implementation and it looks excellent. The Phase 2 conflict detection & warning feature has been implemented comprehensively and correctly addresses issue #174. Implementation Review✅ Architecture & Design - The data-driven approach using ✅ Core Functionality - All 11 unit tests pass successfully:
✅ Robustness - The Copilot review feedback on #231 has been properly addressed:
✅ Import-DPLibrary Integration - The conflict check is called at the end of ✅ Documentation - The Az.Storage + EXO limitation is clearly documented in ✅ Tests - The integration test's shared OData slot model (via ✅ Changelog - Version 2.2.0 entry (lines 13-32) accurately describes the new feature, CI hardening, and documentation changes. Minor ObservationsThe implementation correctly handles:
The workaround text in This is production-ready code with strong defensive design. The feature will provide valuable early warnings to users about the Az.Storage + EXO incompatibility that DLLPickle cannot fix. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…false positives) Normalize each watched module base to end with a directory separator before the AssemblyLoad handler's StartsWith check, so e.g. '...\Az.Storage\' does not match a sibling '...\Az.Storage.Custom\'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbb876772a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…resolves #231 P2) (#233) refactor(conflicts): ship KnownConflicts.json as a committed src file Resolves the unresolved Codex P2 on #231. Since #231 the build extracted knownConflicts from build/dependency-policy.json into the shipped KnownConflicts.json, so a build/** policy file affected the published bundle -- but build/** is excluded from the Release path filter, so a future conflict-data edit would not have auto-published (stale conflict warnings). Make the conflict data the single source of truth as a committed source file at src/DLLPickle/KnownConflicts.json, copied into the module verbatim by CopyModuleFiles. This restores the invariant "bundle inputs live under src/, policy/CI edits do not trigger a release": conflict-data edits now correctly publish, while policy-only edits do not. - Add src/DLLPickle/KnownConflicts.json (identical content to what 2.2.0 ships today; the runtime read path in Get-DPKnownConflict is unchanged). - Remove build/Export-DLLPickleKnownConflicts.ps1 and the ExportKnownConflicts build task (no extraction step needed). - Remove knownConflicts from build/dependency-policy.json; update the OData blockedPreloadAssemblies cross-references to point at the new location. - Rework the extractor sync test into direct validation of the committed source file (via the real Get-DPKnownConflict reader). 13/13 unit tests pass; Analyze/AnalyzeTests/AnalyzeTools clean; PrepareModuleOutput ships the file. - Add superseding notes to the #174 design (§3) and plan docs; update CHANGELOG. The drift fingerprint (conflictSurfaceFingerprint) is computed by New-DLLPickleConflictMatrix from the live upstream inventory, not from knownConflicts, so the baseline is unaffected. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Phase 2 resolution for #174 (the maintainer chose: document + warn + keep the issue open). Built on the Phase 1 runtime evidence — Az.Storage (OData 7.6.4) and ExchangeOnlineManagement (7.22.0) load incompatible strong-named OData into the default ALC; both import orders fail; not fixable by preloading (OData stays
block). This PR carries the design spec, the implementation plan, and the implementation.Feature
Test-DPLibraryConflict(new public): warns for any known-incompatible module pair currently loaded together (reason + separate-session workaround + issue link); returns the active conflicts.Import-DPLibrarynow warns automatically: immediately if both modules are already loaded, otherwise via a one-shot, self-unregisteringAssemblyLoadhandler armed only when both are installed (CLM-guarded; never throws).build/dependency-policy.jsonknownConflicts; the build ships a smallKnownConflicts.jsoninto the module (adding a future conflict is a data edit).Supporting
blockedPreloadAssembliesevidence cross-references theknownConflictsentry (staysblock).docs/Deep-Dive.md: known-limitation section + separate-session workaround.tests/Integration/DLLPickle.Issue174.OData.Tests.ps1: reworked to a shared single-slot OData model that asserts both import orders fail.Validation
Invoke-Build Analyze,Test— 0 errors (warnings are the conflict-warning tests exercising the feature);IssueReproTest— both orders fail as expected; new unit suite 11/11.KnownConflicts.jsonread OK;Test-DPLibraryConflictexported and silent when no conflict is loaded.🤖 Generated with Claude Code