Repository navigation
Restore the 8-parameter constructor of LanguageModelCatalogSource - #2333
Conversation
There was a problem hiding this comment.
Pull request overview
Restores binary compatibility for LanguageModelCatalogSource so separately-compiled AI provider modules (landed under /data/modules) can continue binding to the shipped 8-parameter primary constructor at runtime, preventing MissingMethodException crash loops during rollout.
Changes:
- Moves
Descriptionoff theLanguageModelCatalogSourceprimary constructor and into aninitproperty to keep the shipped constructor signature stable. - Adds a reflection-based contract test that pins the 8-parameter constructor shape (with a negative control) and verifies
Descriptionremains settable viainit/with. - Adds explicit XML documentation warning that the record’s primary constructor is a shipped binary contract.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/MeshWeaver.AI.Test/LanguageModelCatalogSourceBinaryContractTest.cs | Adds a reflection-based binary contract test for the shipped constructor and validates Description remains available via init. |
| src/MeshWeaver.AI/BuiltInLanguageModelProvider.cs | Restores the 8-parameter primary constructor and reintroduces Description as an init property with a prominent binary-compatibility warning. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`Description` was added as a ninth primary-constructor parameter (987da5f). It had a default, so every source-level gate stayed green — but a record's primary constructor signature is REPLACED, not extended, and a call site that omits optional arguments still emits a call to the full constructor as it stood at ITS compile time. The AI provider modules are compiled separately, live in another repository, and land at runtime under /data/modules, where — per ModuleLandingService — they "bind by simple name, and their contract is API compatibility, not build identity". So the six landed providers kept calling the 8-argument constructor that no longer existed, and the first type initializer to run aborted the host: System.TypeInitializationException: 'MeshWeaver.AI.Anthropic.AnthropicExtensions' ---> System.MissingMethodException: Method not found: 'Void MeshWeaver.AI.LanguageModelCatalogSource..ctor(String, String, Int32, String, String, ImmutableArray`1<String>, Boolean, ProviderKind)' That crash-looped systemorph for 100 minutes across 24 restarts on the roll to rc8.ci.5567. It read as a CD failure the whole way: the old pods kept serving, so every health probe stayed green, and helm simply waited on a pod that could never become ready. The repair is ADDITIVE. My first cut moved Description to an init property, which restored the 8-parameter form by REMOVING the 9-parameter one — the same mistake pointing the other way, and check-record-signatures.py (added 11 hours after the break it would have caught) correctly refused it. So the ninth parameter stays and the eight-parameter form is restored beside it: modules compiled before the change and after it both bind. The overload has no source-level caller by design — its callers are separately compiled assemblies that bind by name at runtime — so it is pinned by LanguageModelCatalogSourceBinaryContractTest rather than by use. That test holds both arities, agreement between them, and a negative control so it cannot pass vacuously; verified by mutation in both directions. Verified: MeshWeaver.AI builds with 0 warnings; MeshWeaver.AI.Test 1305/1305; check-record-signatures.py --base origin/main clean; all six provider modules in MeshWeaver.Plugins compile against this core. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
272504f to
1375b02
Compare
rbuergi
changed the title
Test Results (shard 3) 9 files ± 0 9 suites ±0 5m 35s ⏱️ +17s Results for commit 1375b02. ± Comparison against base commit 187f910. This pull request removes 346 and adds 460 tests. Note that renamed tests count towards both. |
Why the existing gate didn't catch thisWorth recording, because the repo already had a gate aimed squarely at this failure and it passed the break without a murmur.
It had to. It builds the modules from source against the PR's core, and the change was source-compatible: But production never runs that compile. It loads
Both gates are needed and neither subsumes the other:
The natural experiment on the fleet says the same thing from the other side: Which is also why the repair here is additive rather than a revert. Removing the ninth parameter would fix every module built before the change and break every one built after it — and on a fleet where instances re-land modules at different times, both populations are live simultaneously. memex-cloud is running the new one right now. |
Test Results (shard 4)2 123 tests - 109 1 823 ✅ - 111 13m 34s ⏱️ -16s Results for commit 1375b02. ± Comparison against base commit 187f910. This pull request removes 460 and adds 351 tests. Note that renamed tests count towards both. |
What this fixes
systemorphcould not roll for ~2 hours. It did not look like a code problem: the old pods kept servingaade75d00, every health probe returned 200, and helm just sat there. The new ReplicaSet was inCrashLoopBackOffthe whole time — 24 restarts — on:Cause
987da5f9eaddedDescriptionas a ninth primary-constructor parameter. It has a default, so it is source-compatible and every gate stayed green — but a record's primary constructor signature is replaced, not extended, and a call site that omits the optional arguments still emits a call to the full constructor as it stood at its own compile time.The AI provider modules are compiled separately, live in another repository, and land at runtime under
/data/modules. PerModuleLandingServicethey "bind by simple name, and their contract is API compatibility, not build identity" — build identity is deliberately diagnostic-only there. So all six landed providers went on calling the 8-argument constructor, which no longer existed.Landed Aug 25 01:17–01:19, all before the break:
MeshWeaver.AI.Anthropic@c2916b6cMeshWeaver.AI.AzureFoundry@8f60c8c4MeshWeaver.AI.OpenAI@661a3510MeshWeaver.AI.WebSearch@8412db3fModules whose own source changed were rebuilt and re-landed at Aug 25 20:3x. These four were untouched — only the framework under them moved — so nothing rebuilt them and, by design, nothing rejected them.
The fix is additive — and the CI gate is why
My first cut moved
Descriptionto aninitproperty. That restores the 8-parameter form by removing the 9-parameter one: the same mistake pointing the other way, repairing every module compiled before the change and breaking every one compiled after it.scripts/check-record-signatures.pyrefused it, exactly as it should:That gate landed in
f60dc19dfat 2026-08-26 00:21 — eleven hours after the break it would have caught. This PR is one of its first real customers, and it caught me committing the inverse of the bug I was fixing.So: the ninth parameter stays, and the eight-parameter form is restored beside it. Both populations bind. No call site changes;
Descriptionkeeps working positionally and throughwith { … }.The overload has no source-level caller by design — its callers are separately-compiled assemblies that bind by name at runtime. It is therefore pinned by a test rather than by use, and carries a comment saying so, because the obvious next cleanup is to delete it as dead code.
Verification
MeshWeaver.AIbuilds clean, 0 warningsMeshWeaver.AI.Test— 1305 passed, 0 failedcheck-record-signatures.py --base origin/main— clean (and--self-testpasses)MeshWeaver.Pluginscompile against this coreLanguageModelCatalogSourceBinaryContractTestverified by mutation in both directions: deleting the 8-param overload turns it red (2 failures); adding a 10th parameter turns it redThe guard
No compile-time gate inside this solution can catch this class of break, because nothing here compiles against the binaries that make the call. The gate above catches it at the diff; this test holds the same line from inside the assembly, where it still applies to a consumer that never sees the diff. It pins both arities, asserts the two forms produce equal values, and carries a negative control so it cannot pass vacuously.
Its doc comment says explicitly: if it goes red, do not update it to match the new signature. That one "fix" converts a caught break into a shipped one.
🤖 Generated with Claude Code