Tests assert the contract, not a snapshot — and a broken skill file no longer vanishes silently by rbuergi · Pull Request #1225 · Systemorph/MeshWeaver

rbuergi · 2026-08-11T16:02:31Z

MeshWeaver ships dual-licensed Apache-2.0 / MIT. A copyleft or pay-to-use dependency is therefore a licensing defect, and nothing in the build checked for one: a licence is added by a single line in Directory.Packages.props, the compiler is happy, and every test still passes.

This audits all 432 packages in the dependency graph — direct and transitive — deletes the one free win, and adds a gate so it cannot regress silently.

What the audit found

	Package(s)	Licence	Verdict
🔴	`itext7` 9.6.0	AGPL-3.0 or paid commercial	Deleted in this PR
🔴	`QuestPDF` 2026.7.2	dual: Community free only below a revenue threshold, else paid	Tracked exception → #1230
🟠	`JsonPatch.Net`, `JsonPath.Net`, `Json.More.Net`, `JsonPointer.Net`	Open Source Maintenance Fee EULA	Needs a decision → #1231
🟡	`UglyToad.PdfPig` (+4 sub-packages)	no `<license>` element at all	Documented exception (Apache-2.0 upstream)
🟢	everything else	MIT ×290, Apache-2.0 ×75, BSD ×10, PostgreSQL ×3, MS-PL, public domain	fine

Spot-checked and clean, as expected: Radzen.Blazor (MIT — the component library, not the commercial IDE), DocSharp.Markdown, BlazorMonaco, ClaudeAgentSdk, CSharpVitamins.ShortGuid, CommunityToolkit.Maui, akgul.Maui.DataGrid, LiveChartsCore (all MIT); Appium.WebDriver, Castle.Core, ModelContextProtocol*, SQLitePCLRaw managed (Apache-2.0); SQLitePCLRaw.lib.e_sqlite3 (SQLite, public domain).

`itext7` was dead configuration

It is AGPL-3.0-or-commercial — the more serious of the two, since AGPL is viral for a network-served product. It turned out to have no PackageReference anywhere: not in any .csproj/.props/.targets, no using iText, no #r "nuget:" directive, and nothing in the in-mesh C# that dotnet build cannot see (node JSON, .csx templates, configuration lambdas). Decisively, no project.assets.json contains an itext7/9.6.0 library entry — it appears only inside centralPackageVersions, a verbatim echo of the declaration. It was never resolved, never downloaded, never shipped.

So deleting it was free. But nothing would have told us either way, which is the actual problem this PR fixes.

`QuestPDF` — removal is real work, not a line deletion

Per the user's decision it is being dereferenced (not licensed, not swapped for another restrictive package); PDF rendering moves to the browser via the existing MIT/Playwright Pixel/HeadlessChromiumPdfRenderer. Its footprint is small — one renderer (Pdf/PdfDocumentRenderer.cs, 368 lines), one licence-acceptance line, one PackageReference — but it cannot be deleted in isolation today: ExportPdf.csx:193-196 falls back to QuestPDF for every non-Deck node, and the Chromium path has no cover page, TOC, running header/footer or page-break rules (PixelFaithfulExport.md:70).

Deleting it now would be a functional regression, not a cleanup, so it lands with the document-export migration — #1230, coordinated with the agent doing that work. This PR touches no file under src/MeshWeaver.Markdown.Export.

`json-everything` — a decision, not a bug

Four packages carry OSMFEULA.txt. This is not the QuestPDF shape and the difference matters: the source is MIT, the EULA says explicitly "The Fee is not a license fee", and "the OSI License shall govern" on conflict. It is a maintenance-fee expectation on the published binary for revenue-generating users ≥ US$10,000/yr. JsonPatch.Net also implements the RFC 7396 merge-patch path behind every cross-hub stream.Update, so replacing it touches the core mutation API. Raised as #1231 rather than decided unilaterally.

The gate

.github/scripts/check-licenses.py, run by the new Dependency licences job.

Reads the restored graph (project.assets.json), so transitive dependencies are covered — a copyleft library that arrives indirectly binds us just as hard. It caught the json-everything transitives (Json.More.Net, JsonPointer.Net) that a declared-packages-only check would have missed.
Resolves each licence from the package's own .nuspec — SPDX expression, else by classifying the licence file. Deny patterns are tested first: a file granting MIT-style permissions that also mentions the Affero GPL classifies as AGPL. That ordering is the difference between catching a relicensed package and waving it through.
A violation is only something that actually ships. A <PackageVersion> no project references restores nothing and is redistributed nowhere — dead config, reported but not failed on. There are 40 such entries (this is what itext7 was).

Gate shape (AGENTS.md)

No continue-on-error, no input-shaped if:. It needs no secret, so it has no legitimate skip and no fork exemption — it runs on fork PRs too.
Fails closed on absent evidence: no dependency graph, or an unresolvable licence, is a failure.
Wired into collect-results (the only required check) with an explicit fail step, because needs: alone does not fail an always() job.
Exceptions carry a written reason and a tracking issue, and a passing run still prints them, so tracked debt cannot quietly become permanent.

Verified the gate actually fails — a gate that cannot fail is decorative. With EXCEPTIONS cleared it exits 1 and names all 5 violations; classification was checked against real AGPL/GPL/LGPL/OSMF/dual-commercial/MIT/Apache texts, including the MIT-text-inside-an-AGPL-file trap.

Verification

dotnet restore clean after the itext7 removal (a missing PackageVersion for a referenced package is a restore error, so this is the real proof nothing referenced it).
dotnet build src/MeshWeaver.Documentation -c Release -warnaserror clean.
DocumentationLinkIntegrityTest + WhatsNewEntryIntegrityTest pass.
Gate green on this tree; proven red without its exceptions.

Docs: Doc/Architecture/DependencyLicensing (policy, allowlist, exception rules). What's New entry included (Category: Fix).

Follow-ups: #1230 (remove QuestPDF), #1231 (decide json-everything). Each closes by deleting its EXCEPTIONS entry, at which point the gate enforces it permanently.

…ing it silently A skill whose YAML front matter cannot be parsed was SKIPPED — correctly, since a throw during mesh startup takes the host down — but skipped SILENTLY: no log, no error, the catalog one entry shorter. The author's only symptom was "my skill doesn't appear and nothing is red". The agent half of content/ai was hardened this way after the 2026-08-07 incident; the skill half never was. Product: SkillMarkdown.TryParse now returns an author-actionable reason, and BuiltInSkillProvider records every file it drops (AiContentLoadFailure) and names it on stderr — the same sink and shape BuiltInAgentProvider already uses. BuiltInAgentProvider gains the matching TryParseEmbeddedNode so its own guard test can assert the file was REPORTED, not merely that null came back. Tests — assert the CONTRACT and the INVARIANTS, not a snapshot of today's data: - The shipped skill set was pinned as a hardcoded 19-element literal duplicated in two files. It is DERIVED FROM content/ai/Skill/*.md, so that was brittle (a legitimate new skill reds CI in N places) and blind (a malformed NEW file leaves the shipped set unchanged, so the literal still matched — green while broken). Replaced by file-derived set equality plus a no-skipped-file guard that fails RED naming the file and the reason. Proven by negative control in both directions. - AiContentSectionTest asserted floors (Count >= 13 / >= 15). Now asserts the embedded fallback ships EXACTLY the authored files — a divergence there is invisible in dev (which reads the on-disk section) and only shows up as missing content in a shipped offline build. - Five stream-count assertions in the observable-query suites were tautologies: after Match(acc.Count >= N), Scan guarantees the snapshot has exactly N, so HaveCount(N) could never fail. Two tests had that as their ONLY assertion and therefore asserted nothing. Worse, the Count >= N gate they rode on was satisfied by an unrelated framework `_Access` satellite emission — ScopeSubtree was declaring success without ever observing the descendant. Now gated and asserted on emission SHAPE, per AGENTS.md's ban on "exactly N change events". - MeshPluginTest's HaveCount(5)/(13) were environment-dependent (RunTests registers only inside a repo checkout) and named nothing on failure. Replaced with the read-only allowlist as a subset assertion plus uniqueness. - DataCubesDocBlocksTest pinned the count of executable blocks in a shipped doc page; a sixth demo would have redded CI editorially. New shared helper AiContentSection carries the section invariants so the next content/ai/<Section> gets them for free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR hardens AI-content loading and modernizes several tests to assert stable behavioral contracts/invariants rather than brittle or tautological snapshot counts, while ensuring malformed built-in skill files are reported instead of disappearing silently.

Changes:

Add actionable parse diagnostics for skill markdown (front matter) and record/log skipped built-in skill files during catalog load.
Introduce shared AI content section invariants in tests and replace multiple count-based assertions with shape/contract assertions.
Relax documentation demo-block tests to avoid editorial breakage while still asserting key invariants (e.g., unique block ids).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
test/MeshWeaver.Query.Test/ObservableQueryTests.cs	Replaces a tautological emission-count assertion with a path-shape contract.
test/MeshWeaver.Query.Test/ObservableQueryIntegrationTests.cs	Reworks multiple observable-query tests to assert on emitted content shape rather than counts/racy sampling.
test/MeshWeaver.Persistence.Test/DataCubesDocBlocksTest.cs	Removes brittle exact-count pin; asserts non-empty and uniqueness invariant for executable block ids.
test/MeshWeaver.AI.Test/SkillNodeTypeTest.cs	Stops hardcoding built-in skill membership; keeps per-skill action contract assertions.
test/MeshWeaver.AI.Test/SkillHarnessImportSourceTest.cs	Asserts imported skill ids match the provider-derived catalog, avoiding duplicated literals.
test/MeshWeaver.AI.Test/MeshPluginTest.cs	Converts tool-surface tests from count assertions to explicit allowlist/subset invariants.
test/MeshWeaver.AI.Test/BuiltInSkillCatalogTest.cs	New catalog guard: file-set equality, no-skipped-file invariant, and negative controls for reporting.
test/MeshWeaver.AI.Test/BuiltInAgentContentTest.cs	Updates malformed-front-matter guard to assert an actionable reported error (not just null).
test/MeshWeaver.AI.Test/AiContentSectionTest.cs	Asserts embedded fallback content matches authored on-disk section exactly (set equality).
test/MeshWeaver.AI.Test/AiContentSection.cs	New shared helper encoding AI content section invariants used by multiple tests.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-11-broken-skill-file-no-longer-vanishes-silently.md	What’s New entry describing the new “broken skill file is reported” behavior.
src/MeshWeaver.AI/SkillMarkdown.cs	Adds `TryParse` with actionable error output while keeping `Parse` API.
src/MeshWeaver.AI/BuiltInSkillProvider.cs	Records skipped files as `AiContentLoadFailure` and logs them to stderr; exposes catalog failures for tests.
src/MeshWeaver.AI/BuiltInAgentProvider.cs	Adds `TryParseEmbeddedNode` returning an error reason for guard tests; keeps skip-on-error behavior.
src/MeshWeaver.AI/AiContentLoadFailure.cs	New shared failure record type for reporting skipped AI content files.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

        var yamlBlock = document.Descendants<YamlFrontMatterBlock>().FirstOrDefault();
-        if (yamlBlock == null) return null;
+        if (yamlBlock == null)
+        {
+            error = "no YAML front matter — the file must open with a '---' fenced block";
+            return null;
+        }


        foreach (var resourceName in assembly.GetManifestResourceNames()
                     .Where(n => n.StartsWith(skillPrefix, StringComparison.Ordinal)
                                 && n.EndsWith(".md", StringComparison.OrdinalIgnoreCase))
                     .Order())
        {


github-actions · 2026-08-11T16:17:35Z