Repository navigation
The adopted count is the SET of adopted paths, not a sum of events (#2697) - #2716
Conversation
…2697) `bake consumption: the gate adopted 92 of 90 baked assembly(ies)` is an impossible sentence, and it appeared in the one line the operator is meant to trust. It was predicted in #2675 and reported from MeshWeaver.Crm PR #20. WHY IT COULD SAY THAT BakeSeedConsumer.Adopted is `Interlocked.Add(ref adopted, count)` over every SeedForTypes call — adoption EVENTS. Covered is a set of paths. The gate installs every package twice (the idempotence pin re-installs the unchanged snapshot) and a path can be covered under two packages, so the event sum drifts above the number of distinct assemblies and can exceed the denominator it is a fraction of. The verdict's NAMING was already right: DECLINED comes from `expected.Except( covered)`, a set difference. Only the numerator came from somewhere else. So the sentence contradicted itself — the count said 92 of 90 while the list said ten declined — and the set-based half was the correct one. THE FIX Derive the numerator from the same sets the DECLINED list comes from: AdoptedAmong(declared, requested, covered) = (declared ∩ requested ∩ covered).Count so `adopted + declined == expected` holds by construction. DescribeShortfall no longer TAKES an `adopted` parameter at all — the bug was passing a number in from elsewhere, so the fix is structural rather than arithmetic: there is no longer a way to report a count that disagrees with the naming. `Adopted` stays, because "how many times did the seeder act" is a real fact, but its doc now says in terms it cannot be missed that it must never be reported as a number of assemblies. THE GATE BECOMES EXACT .github/scripts/assert-bake-consumption.sh compared `-ge`, with a comment explaining that adopted counts events and a healthy run measured `adopted 32 of 28`. That weakness existed only to tolerate this bug. With a set count the numerator cannot exceed the denominator, so `-ge` had become a strictly weaker way of writing `-eq`; it now says what the check means — every baked assembly this run installed was judged from the bake's own bytes. TESTS 115/115 green, Release, -warnaserror clean. Two cases added to the existing pure-seam suite: a path seeded under two packages is counted once, and AdoptedAmong counts the intersection rather than the events. NOT FIXED HERE: the ten DECLINED assemblies that surfaced this. Those are a consumer-side compose gap (a gate declaring `registry-modules: AI` rather than Essentials, so the carved-out MeshWeaver.Markdown.Collaboration is "live absent"). The verdict was RIGHT to decline them; this changes only the sentence that reports it. Fixes #2697 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes bake-consumption reporting so the “adopted” numerator is computed from the same set of distinct covered paths used to determine the DECLINED list, preventing impossible summaries like “adopted 92 of 90” (Fixes #2697).
Changes:
- Introduces a set-derived adopted count (
AdoptedPaths/AdoptedAmong) and removes the ability forDescribeShortfallto accept an externally-provided event-sum “adopted” value. - Updates gate output and the CI assertion script to enforce exact equality between adopted distinct paths and expected declared∩requested paths.
- Extends the existing pure-seam test suite with cases that pin “count once” semantics and intersection-based counting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/MeshWeaver.PluginTester/PluginGateRunner.cs | Switches the gate’s “seed: adopted …” line to use the distinct-path adopted count. |
| tools/MeshWeaver.PluginTester/BakeSeed.cs | Adds AdoptedPaths/AdoptedAmong, removes DescribeShortfall’s adopted parameter, and derives the numerator from set intersections. |
| test/MeshWeaver.PluginTester.Test/BakeShortfallNamesTheTypeTest.cs | Updates existing tests for the new signature and adds new cases pinning set semantics. |
| .github/scripts/assert-bake-consumption.sh | Strengthens the postcondition from -ge to -eq now that adopted is set-based. |
Suppressed comments (1)
tools/MeshWeaver.PluginTester/BakeSeed.cs:248
- This XML doc comment still says the verdict is a pure function of "four facts"; after removing the
adoptedparameter, the verdict logic is now derived from the three sets (declared/requested/covered), withdirectoryonly used for message text. Updating the wording avoids implying there is still a fourth accounting input.
/// <see cref="Shortfall"/>'s verdict as a PURE function of the four facts it rests on, so the
/// accounting can be pinned without standing up a mesh (the same seam
/// <c>PrebuiltAssemblySeeder.DeclineReason</c> exposes for the same reason).
/// </summary>
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # 🚨 `=`, and it can be `=` since #2697. This was `>=` because the tester reported adoption EVENTS: | ||
| # the gate installs every package TWICE (the idempotence pin re-installs the unchanged snapshot), | ||
| # so the second install adopted the same assemblies again and a healthy run measured `adopted 32 of | ||
| # 28` on samples/Graph/Data. An equality test would have been red on every healthy run then — and a | ||
| # gate that cries wolf gets switched off. | ||
| # | ||
| # The tester now reports BakeSeedConsumer.AdoptedPaths: the size of the set of distinct paths that | ||
| # were declared AND requested AND backed — the exact complement of the DECLINED list in the same | ||
| # verdict. It cannot exceed `expected` however many times the seeder acted, so `>=` had become a | ||
| # strictly weaker way of writing `=`, and the reason for the weakness was a bug rather than a | ||
| # property of the run. Equality now says what this check actually means: every baked assembly this | ||
| # run installed was judged from the bake's own bytes. | ||
| [ "$adopted" -eq "$expected" ] \ |
Test Results (shard 3)693 tests 501 ✅ 4m 56s ⏱️ Results for commit 34a8d7e. |
Test Results (shard 2)941 tests 941 ✅ 5m 14s ⏱️ Results for commit 34a8d7e. |
Test Results (shard 5)1 530 tests 1 530 ✅ 6m 32s ⏱️ Results for commit 34a8d7e. ♻️ This comment has been updated with latest results. |
Test Results (shard 0)913 tests 913 ✅ 5m 35s ⏱️ Results for commit 34a8d7e. |
Test Results (shard 4) 10 files 10 suites 6m 2s ⏱️ Results for commit 34a8d7e. |
Test Results (shard 1)818 tests 814 ✅ 7m 31s ⏱️ Results for commit 34a8d7e. ♻️ This comment has been updated with latest results. |
Test Results 41 files 41 suites 35m 51s ⏱️ Results for commit 34a8d7e. ♻️ This comment has been updated with latest results. |
|
Both reds are unreachable from this diff — closure analysis, not a re-run. Failing tests (extracted from the shard trx artifacts; the check summaries carry only counts): This diff changes
The changed assembly cannot load in either process. The shell change runs in a gate job, not in a Re-running the failed jobs. To be explicit about why that is not circular: the closure is the Worth flagging for whoever owns the disposal work: |
Fixes #2697.
bake consumption: the gate adopted 92 of 90 baked assembly(ies)is an impossible sentence, and itappeared in the one line the operator is meant to trust. Predicted in #2675, reported from
MeshWeaver.Crm PR #20.
Why it could say that
BakeSeedConsumer.AdoptedisInterlocked.Add(ref adopted, count)over everySeedForTypescall —adoption events.
Coveredis a set of paths. The gate installs every package twice (theidempotence pin re-installs the unchanged snapshot), and a path can be covered under two packages,
so the event sum drifts above the number of distinct assemblies and can exceed the denominator it is
a fraction of.
The verdict's naming was already correct —
DECLINEDcomes fromexpected.Except(covered), a setdifference. Only the numerator came from somewhere else. So the sentence contradicted itself: the
count said 92 of 90 while the list said ten declined, and the set-based half was the right one.
The fix is structural, not arithmetic
DescribeShortfallno longer takes anadoptedparameter. The bug was passing a number in fromelsewhere, so removing the parameter removes the ability to report a count that disagrees with the
naming:
adopted + declined == expectednow holds by construction.Adoptedstays — "how many times did the seeder act" is a real fact — but its doc now says in termsthat cannot be missed that it must never be reported as a number of assemblies, and names #2697.
The gate becomes exact
.github/scripts/assert-bake-consumption.shcompared-ge, with a comment explaining that adoptedcounts events and that a healthy run measured
adopted 32 of 28onsamples/Graph/Data. Thatweakness existed only to tolerate this bug. With a set count the numerator cannot exceed the
denominator, so
-gehad become a strictly weaker way of writing-eq. It now says what the checkmeans: every baked assembly this run installed was judged from the bake's own bytes.
Worth noting the old comment also cited
BakeSeed.Shortfall: if (Adopted >= expected.Count) return null;— code that no longer exists (it ismissing.Count == 0now). The rationale had alreadydrifted from the implementation.
Tests
115/115green, Release,-warnaserrorclean. Two cases added to the existing pure-seam suite:A_path_seeded_under_two_packages_is_counted_ONCE— pins that the numerator cannot exceed thedenominator however many times the seeder acted, and that
adopted + declined == expectedwherethere is a shortfall.
AdoptedAmong_counts_the_intersection_not_the_events— pins the pure seam directly, includingthat a requested-but-undeclared path counts zero.
The five existing cases keep their assertions unchanged apart from dropping the removed argument.
Deliberately not fixed here
The ten
DECLINEDassemblies that surfaced this. They are a consumer-side compose gap — a gatedeclaring
registry-modules: AIrather thanEssentials, so the carved-outMeshWeaver.Markdown.Collaborationis "live absent". The verdict was right to decline them. Thischanges only the sentence that reports it.
(That compose gap is the same class as the one I hit on
MeshWeaver.Reinsurancetonight, whereregistry-modules: AI Collaborationnames a node-only package that carries no module at all — fixedseparately on that repo.)