Repository navigation
PathResolution.Test drops MeshWeaver.AI — and the static-collections guard follows it - #2368
Merged
Merged
Conversation
…guard included Two thread cases move to MeshWeaver.AI.Test as ThreadAddressResolutionTest. Their fixture is NOT incidental: they build AI.Thread content under a _Thread satellite path and the shape being resolved IS that. Substituting a non-AI node type would have changed the scenario, so they travel with the engine. The resolution mechanism itself is core and its coverage stays in place — 136/136 there without the reference. 🚨 The ProjectReference was never a code dependency. NoStaticCollectionsTest enumerates MeshWeaver.*.dll from AppContext.BaseDirectory at RUNTIME, so it only ever covers the assemblies in the running test's bin — and the reference existed purely to put AI.dll where the guard would find it. Dropping it naively leaves the guard GREEN having stopped looking at AI, which is coverage lost with no signal. So the guard follows its subject: one linked <Compile> into MeshWeaver.AI.Test, no duplicated logic, and it travels with that project when the engine leaves. Linking it proved itself immediately, twice. It ran and FAILED — first because the four MeshWeaver.AI.* allow entries had been removed on the wrong assumption that AI would no longer be scanned anywhere (restored: they are legitimate CONST allowances needed wherever AI.dll is scanned), then because AI.Test's larger dependency closure brought six assemblies into scope for the FIRST time. A no-op link would have sat silently green. Those six are allow-listed, each verified rather than assumed: every one is a declaration-with-initializer whose only later uses are reads (.Contains, lookups) — the immutable read-only lookups NoStaticState.md permits. They were surfaced by WIDENED COVERAGE, not by new code, and the entries say so. Note this widens the guard repo-wide, so this PR classifies six statics unrelated to AI. The narrower option was to record a coverage gap the way #2293 did for the portal hosts' module declarations; coverage won because leaving AI unguarded is the failure this whole exercise exists to avoid, and the six were trivially verifiable. Verified: both projects build -c Release -warnaserror at 0 Error(s); PathResolution.Test 136/136; AI.Test 4/4 including the linked guard. No What's New entry: test-only refactor. Refs #2276 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes an indirect MeshWeaver.AI dependency from MeshWeaver.PathResolution.Test and preserves the “no static collections” architecture guard’s effective coverage by linking that guard into MeshWeaver.AI.Test instead.
Changes:
- Drop
MeshWeaver.AIas aProjectReferencefromMeshWeaver.PathResolution.Testand remove the remainingAddAI()-dependent thread path-resolution cases from that suite. - Add a new
ThreadAddressResolutionTestunderMeshWeaver.AI.Testto keep the thread-specific path-resolution scenarios covered with an AI-aware fixture. - Extend the static-collections guard allowlist with newly surfaced (read-only) static lookup fields discovered once
MeshWeaver.AI.Test’s larger dependency closure is scanned.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.PathResolution.Test/NoStaticCollectionsTest.cs | Updates the allowlist for the static-collections guard to account for newly scanned assemblies/statics. |
| test/MeshWeaver.PathResolution.Test/MeshWeaver.PathResolution.Test.csproj | Removes the MeshWeaver.AI ProjectReference. |
| test/MeshWeaver.PathResolution.Test/AddressResolutionTest.cs | Removes AddAI() and moves the thread-specific cases out of this test suite. |
| test/MeshWeaver.AI.Test/ThreadAddressResolutionTest.cs | Adds AI-fixture-backed thread path-resolution tests moved from PathResolution.Test. |
| test/MeshWeaver.AI.Test/MeshWeaver.AI.Test.csproj | Links NoStaticCollectionsTest.cs into MeshWeaver.AI.Test so the guard continues to scan AI’s bin closure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Test Results (shard 3) 9 files ± 0 9 suites ±0 5m 1s ⏱️ - 1m 16s Results for commit 6fe127b. ± Comparison against base commit d9b59b6. This pull request removes 54 and adds 115 tests. Note that renamed tests count towards both. |
Contributor
Test Results (shard 0)591 tests - 2 587 ✅ - 2 18m 5s ⏱️ + 3m 32s Results for commit 6fe127b. ± Comparison against base commit d9b59b6. This pull request removes 2 tests. |
Contributor
Contributor
Contributor
Contributor
Test Results (shard 4)2 039 tests - 57 2 036 ✅ - 59 14m 14s ⏱️ + 1m 42s Results for commit 6fe127b. ± Comparison against base commit d9b59b6. This pull request removes 111 and adds 54 tests. Note that renamed tests count towards both. |
Contributor
Test Results 53 files ±0 53 suites ±0 1h 2m 18s ⏱️ + 4m 19s Results for commit 6fe127b. ± Comparison against base commit d9b59b6. This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both. |
rbuergi
added a commit
that referenced
this pull request
Aug 26, 2026
…sees AI CI: "The type or namespace name 'AI' does not exist in the namespace 'MeshWeaver'" at SkillFileParserTest.cs:2-3. Not merge damage and not a missing reference to restore. Main deliberately removed MeshWeaver.AI from MeshWeaver.Content.Test as part of #2276 (AI leaves the platform) — the same direction as #2363 and #2368, which moved AI-dependent suites out rather than re-adding the reference. This branch added a NEW test to the project that had just been cut loose. SkillFileParser is AI code (src/MeshWeaver.AI/Persistence/SkillFileParser.cs), so its test belongs in MeshWeaver.AI.Test. Moved, namespace updated to match. Re-adding the ProjectReference would have been the smaller diff and the wrong one: it reverses deliberate carve-out work to accommodate a test that was in the wrong project to begin with. Verified: MeshWeaver.AI.Test and MeshWeaver.Content.Test both build clean under -c Release -warnaserror; SkillFileParserTest 14/14 green in its new home. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
MeshWeaver.PathResolution.Testloses itsMeshWeaver.AIreference — #2276.The reference was never a code dependency
NoStaticCollectionsTestenumeratesMeshWeaver.*.dllfromAppContext.BaseDirectoryat runtime. It only ever covers the assemblies in the running test's bin — and theProjectReferenceexisted purely to placeAI.dllwhere the guard would find it.🚨 Dropping it naively leaves the guard green, having stopped looking at AI. Coverage lost, no signal. So the guard follows its subject: one linked
<Compile>intoMeshWeaver.AI.Test— no duplicated logic, and it travels with that project when the engine leaves.Linking it proved itself immediately, twice
It ran and failed, which is the only way to know the link isn't decorative:
MeshWeaver.AI.*allow entries, assuming AI would no longer be scanned anywhere. Wrong — restored; they are legitimateCONSTallowances needed whereverAI.dllis scanned.AI.Test's larger dependency closure brought six assemblies into scope for the first time.A no-op link would have sat silently green.
The six, verified rather than assumed
NonfileRouteConstraint.ExcludedPrefixesstatic readonly string[], reads onlyPostgreSqlCrossSchemaQueryProvider.ExcludedSchemasstatic readonly HashSet,.ContainsonlyPostgreSqlSqlGenerator.AllowedSqlFunctions/NonTextColumns/PropertyMapPostgreSqlStorageAdapter.TransientPostgresSqlStatesEach is a declaration-with-initializer whose only later uses are reads — the immutable read-only lookups
NoStaticState.mdpermits. The entries record that they were surfaced by widened coverage, not by new code, because six new allow lines otherwise read as weakening a guard.The moved tests
The two thread cases go to
AI.TestasThreadAddressResolutionTest. Their fixture is not incidental — they buildAI.Threadcontent under a_Threadsatellite path and the shape being resolved is that. (I first wired them withAddThreadType(); isolating the failure showedThreadMessageunregistered, so they useAddAI()as the original fixture did.)Trade-off worth naming
This widens the guard repo-wide, so this PR classifies six statics unrelated to AI. The narrower option was to record a coverage gap, as #2293 did for the portal hosts' module declarations. Coverage won: leaving AI unguarded is the exact failure this exercise exists to avoid, and the six were trivially verifiable.
Verification
-c Release -p:CIRun=true -warnaserror→0 Error(s)PathResolution.Test: 136/136AI.Test: 4/4, including the linked guardRefs #2276
🤖 Generated with Claude Code