Repository navigation
fix(#4280): the @@-include closure is part of the completeness witness — an included node still landing is an arrival, not a move - #4304
Merged
rbuergi merged 3 commits intoSep 14, 2026
Conversation
…s — an included node still landing is an arrival, not a move #4293's deferral compares the bundle's query-resolved source paths (manifest sourceVersions) against the live snapshot, and neither side carries the @@-include closure: an include is matched by no source query, lands as its own node, and until it does the owner's include reader answers ABSENT — deliberately an answer, not a stall — so the live fold is shorter than the producer's, the fingerprints differ, and the judgement read an arrival as a move and dispatched the premature compile anyway. Copilot's first finding on #4293, verified. Both sides now carry the include paths as a separate fact: - NodeTypeSourceFingerprint.ComputeWithIncludes returns the resolved-present include paths beside the hash (Compute is its wrapper); IncludePathsOf sorts a closure's keys. - The bundle manifest gains per-assembly sourceIncludes (writer/reader init properties, so no constructor-signature change); all three bake sites write it from the closure the compile resolved (TreeBake/CascadeBuild: CompileInputs.ResolvedIncludes; BakeOutput: the same walk it already made for the fingerprint). - The seeder stamps NodeTypeDefinition.AdoptedSourceIncludes; the sources watcher publishes CurrentSourceIncludes in the same write as CurrentSourceFingerprint (null on an inconclusive emission, exactly as the fingerprint; part of the no-op equality so a pre-existing node acquires it on first activation). - CanJudgeAdoption waits on BOTH lists: every adopted path live AND every adopted include among the present ones; a bundle with no includes recorded, or no include list published yet, is handled as the docs say (legacy ⇒ path witness only; unpublished ⇒ not computed, defer). Both compile-state mirror and NodeTypeOperationalContent carry the two fields. Explicitly NOT done: adding include paths to sourceVersions. The live snapshot never contains includes, so that would defer every include-bearing type forever. Tests: three include cases beside the #4280 pure test (arrival deferred; unpublished list deferred / legacy judged; present-plus-extra judged StaleAdopted), BundleReader round-trip of sourceIncludes (sorted; null on legacy), ComputeWithIncludes names the transitive present set and omits the absent one. Compiler.Pipeline.Test 955/955, PluginTester.Test 362/362, Hosting and Graph seed/bundle/adoption/watcher slices, full solution -c Release -p:CIRun=true -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGsQEwX8ohS51nUpcsr4mi
Contributor
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Include metadata can remain stale on retry, and include arrivals do not trigger watcher recomputation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR tracks @@-resolved include paths in adoption completeness checks to prevent premature compilation.
Changes:
- Records include closures in fingerprints and bundle manifests.
- Propagates include metadata through seeding, watching, and operational state.
- Adds coverage for closure extraction, round-tripping, and deferral behavior.
File summaries
| File | Summary |
|---|---|
tools/MeshWeaver.PluginTester/TreeBake.cs |
Writes include paths during tree baking. |
tools/MeshWeaver.PluginTester/CascadeBuild.cs |
Writes include paths during cascade builds. |
tools/MeshWeaver.PluginTester/BakeOutput.cs |
Persists fingerprint and include metadata. |
test/MeshWeaver.Compiler.Pipeline.Test/SourceFingerprintIncludeClosureTest.cs |
Tests include closure path extraction. |
test/MeshWeaver.Compiler.Pipeline.Test/BundleReaderTest.cs |
Tests manifest include metadata reading. |
test/MeshWeaver.Compiler.Pipeline.Test/AdoptionIsNotJudgedFromAPartialSourceSetTest.cs |
Tests include-aware adoption judgment. |
src/MeshWeaver.PluginCatalog/PluginBundleClient.cs |
Passes include metadata during adoption. |
src/MeshWeaver.Plugin.Packaging/BundleWriter.cs |
Serializes include metadata. |
src/MeshWeaver.Plugin.Packaging/BundleReader.cs |
Reads include metadata. |
src/MeshWeaver.Mesh.Contract/NodeTypeOperationalContent.cs |
Extends operational content with include fields. |
src/MeshWeaver.Hosting/ShippedPrebuiltBundles.cs |
Passes shipped-bundle include metadata to seeding. |
src/MeshWeaver.Graph.Contract/NodeTypeDefinition.cs |
Adds adopted and current include state. |
src/MeshWeaver.Compiler/NodeTypeSourceFingerprint.cs |
Returns resolved include paths with fingerprints. |
src/MeshWeaver.Compiler.Pipeline/PrebuiltAssemblySeeder.cs |
Stamps and validates include provenance. |
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompileState.cs |
Mirrors include state. |
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompilationHelpers.cs |
Applies include-aware completeness judgment. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2007
to
+2009
| .ComputeWithIncludes(published.Nodes, hubPath, includeReader, logger) | ||
| .Select(computed => (published.Snapshot, Fingerprint: (string?)computed.Fingerprint, | ||
| Includes: (System.Collections.Immutable.ImmutableList<string>?)computed.Includes)) |
Contributor
Author
There was a problem hiding this comment.
Right, and measured: with the deferral in place and nothing waking the watcher, the include landed and the standing request never converged (72 s timeout). Fixed in 8e79c1b379dc88f753a3926c97b819b7c86bb410: AdoptedIncludeArrivals reads the adopted-but-absent includes off the hub's own record (AdoptedSourceIncludes − CurrentSourceIncludes, re-derived on every record change), watches each through a synced path: query (empty while absent, the node when it lands — never a point read on an absent path), and re-emits the current source set into the fingerprint stage when one lands; nothing missing ⇒ nothing subscribed. The integration test you asked for is AdoptionDeferredUntilIncludeLandsTest: one source present, the include absent, the bundle names both — the seed adopts, the request stands, nothing parks; the include lands with no change to the declared source set and the request converges to AdoptedVerified (12 s; A/B with the stage disabled: timeout).
Comment on lines
+2003
to
+2009
| // #4280 — and the include PATHS the closure resolved as present ride beside the | ||
| // fingerprint, published in the same write as CurrentSourceIncludes: the hash cannot | ||
| // say which includes it covered, and the owner's completeness witness needs to know. | ||
| .Select(published => NodeTypeSourceFingerprint | ||
| .Compute(published.Nodes, hubPath, includeReader, logger) | ||
| .Select(fingerprint => (published.Snapshot, Fingerprint: (string?)fingerprint)) | ||
| .ComputeWithIncludes(published.Nodes, hubPath, includeReader, logger) | ||
| .Select(computed => (published.Snapshot, Fingerprint: (string?)computed.Fingerprint, | ||
| Includes: (System.Collections.Immutable.ImmutableList<string>?)computed.Includes)) |
Contributor
Author
There was a problem hiding this comment.
Right — the retry write refreshed the snapshot alone and left fingerprint, include list and module version from the previous generation (the fingerprint half of that predates this PR, the include half is new). Fixed in 8e79c1b379dc88f753a3926c97b819b7c86bb410: the retry's refreshed record now carries published.Fingerprint, published.Includes and the module version with the same "inconclusive keeps the previous value" rule as the ordinary write below it.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
…arked-retry write publishes the whole generation Two review findings on #4304, both measured: 1. The sources watcher re-runs on the type's source QUERIES, and an @@-include is matched by none of them — it lands as its own node — so an adoption deferred on the include witness waited for an unrelated source edit or a restart (A/B: the include lands, the standing request never converges within 72 s). AdoptedIncludeArrivals now watches each adopted include the owner has not resolved as present through a synced `path:` query (empty while absent, the node when it lands — never a point read on an absent path) and re-emits the current source set into the fingerprint stage when one lands. Nothing missing ⇒ nothing subscribed. Pinned by AdoptionDeferredUntilIncludeLandsTest on a real mesh: one source present, the include absent, the bundle names both — the seed ADOPTS, the request stands, nothing parks; the include lands with NO change to the declared source set, and the request converges to AdoptedVerified (12 s with the trigger, timeout without). 2. The parked-retry write refreshed CurrentSourceVersions alone, leaving the fingerprint, the include list and the module version from the previous source generation on the record — a mixed snapshot a later judgement would read. It now publishes the same generation the ordinary write does, with the same "inconclusive keeps the previous value" rule. Compiler.Pipeline.Test 956/956 (both integration tests green), full solution -c Release -p:CIRun=true -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGsQEwX8ohS51nUpcsr4mi
…stem, not the identity-latch shape ImpersonationScopeSiteRatchetGuard refused the new site: Observable.Using(ImpersonateAsSystem, …) opens the AsyncLocal scope on the subscribing thread and disposes it wherever the work terminates, leaving the subscriber impersonated (#1790). AdoptedIncludeArrivals now takes the sanctioned RunAsSystem, which seals both ends inside the one Subscribe. Both integration tests and the ratchet guard green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGsQEwX8ohS51nUpcsr4mi
rbuergi
marked this pull request as ready for review
September 14, 2026 09:45
rbuergi
added this pull request to the merge queue
Sep 14, 2026
rbuergi
deleted the
fix/4280-include-closure-is-part-of-the-completeness-witness
branch
October 10, 2026 13:10
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.
Follow-up to #4293, taking the first Copilot finding there (the second is #4302). Refs #4280. Draft on purpose (#4299): waits for the ruleset's Copilot review before it is marked ready.
The finding
The deferral #4293 added compares the bundle's query-resolved paths (
sourceVersions) with the live snapshot. Neither side carries the@@-include closure — includes are matched by no query and land as their own nodes — andSourceFingerprintIncludeReadertreats a genuinely absent include as an answer (it contributes nothing; only a stalled read is inconclusive). So an included Code node still landing shortens the live fold, the fingerprints differ,CanJudgeAdoptionsees every recorded path present and says "judgeable", and the premature compile is dispatched anyway — the same install-order race, through the include door.The fix — the include paths become a fact both sides state
NodeTypeSourceFingerprint.ComputeWithIncludesreturns the resolved-present include paths beside the hash (Computewraps it);IncludePathsOfsorts a closure's keyssourceIncludesin the manifest (writer/reader init properties — no constructor change); all three bake sites write it from the closure the compile resolvedAdoptedSourceIncludes; the sources watcher publishesCurrentSourceIncludesin the same write as the fingerprint (null on an inconclusive emission, exactly like the fingerprint; part of the no-op equality so an older node acquires it on first activation)CanJudgeAdoptionwaits on both lists; the deferral log and the seeder's pre-write log name the missing includes. Legacy bundle (no includes recorded) ⇒ path witness only, as before. List not published yet ⇒ "not computed", defer — the fingerprint and the list are written together, so absence is never "none present"NodeTypeOperationalContentand the compile-state mirror carry both fields (the mirror guard test passes)Deliberately not done: adding include paths to
sourceVersions. The live snapshot never contains includes, so that obvious-looking fix would defer every include-bearing type forever.Tested
StaleAdoptedas before.BundleReaderTest:sourceIncludesround-trips sorted; null on a legacy bundle.SourceFingerprintIncludeClosureTest:ComputeWithIncludesyields the same hash asComputeand names the transitive present set, omitting the one that resolved to nothing.-c Release -p:CIRun=true -warnaserrorclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01HGsQEwX8ohS51nUpcsr4mi