Repository navigation
fix(ai): bundle built-in AI content sources — Skill can't be silently un-imported - #135
Merged
Merged
Conversation
…ilently un-imported (Skill) The static-repo import sources for the built-in AI content (Agent / Provider / Harness / Skill) were registered one-by-one in the portal and gated on a hand-maintained per-partition allow-list (StaticRepoSync.Partitions). Forgetting a name — as happened with Skill — left that partition served from in-memory while the rest went to the DB: no import, silently. The user has hit this repeatedly. Treat AI content as ONE bundle, owned by MeshWeaver.AI: - AiContentSources.ContentPartitions — the canonical AI content partition set (Agent/Provider/ Harness/Skill), next to the sources. - AiContentSources.AddBuiltInAiContentSources() — registers EVERY built-in AI IStaticRepoSource. - StaticRepoSyncExtensions registers the whole bundle whenever any AI partition is served (no per-name list to forget); MemexConfiguration expands the served set to the whole bundle if it names any AI partition, so AddAI's per-type serve-from-DB gating stays consistent with the import. Ratchet: AiContentSourcesTest reflects over every IStaticRepoSource in MeshWeaver.AI and fails if any is not in the bundle — so a NEW AI content type can never again be silently left un-imported. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes built-in AI static-repo content (Agent/Provider/Harness/Skill) a single bundled unit owned by MeshWeaver.AI, so portal sync/import wiring can’t accidentally omit one partition (the recurring “Skill was never imported” failure mode).
Changes:
- Introduces
AiContentSourcesas the single source of truth for AI content partitions and registers all built-in AIIStaticRepoSources as one bundle. - Updates portal static-repo sync wiring to register the entire AI bundle whenever any AI partition is served.
- Adds tests that “ratchet” the bundle: fail if any AI
IStaticRepoSourceis not included or if the partition set drifts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/MeshWeaver.AI.Test/AiContentSourcesTest.cs | Adds ratchet tests ensuring all AI static-repo sources and partitions are bundled. |
| src/MeshWeaver.AI/AiContentSources.cs | Defines the canonical AI content partition set and the bundled registration helper. |
| memex/Memex.Portal.Shared/StaticRepoSyncExtensions.cs | Switches portal sync wiring to register AI static-repo sources as a single bundle. |
| memex/Memex.Portal.Shared/MemexConfiguration.cs | Expands config to serve all AI partitions if any AI partition is selected, keeping serving/import consistent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // there auto-joins the import; there is no per-partition allow-list here to forget. (The | ||
| // expand-to-the-whole-bundle also happens in MemexConfiguration so AddAI's serve-from-DB | ||
| // gating stays consistent with the import.) | ||
| if (serveFromPartition.Overlaps(AiContentSources.ContentPartitions)) |
Comment on lines
+24
to
+31
| public static readonly IReadOnlySet<string> ContentPartitions = | ||
| new HashSet<string>(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| "Agent", // AgentStaticRepoSource.Partition | ||
| ModelProviderNodeType.RootNamespace, // "Provider" | ||
| HarnessNodeType.RootNamespace, // "Harness" | ||
| SkillNodeType.RootNamespace, // "Skill" | ||
| }; |
…lections policy) Address Copilot: a static readonly HashSet is mutable; use ImmutableHashSet for the partition constant. (The Overlaps comment is moot — IReadOnlySet<T> defines Overlaps; the portal builds clean.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nk cards) The Agent/Provider/Skill PartitionRoot definitions never set an Icon, so on every distributed portal those space roots render as blank cards in search/catalogs. Set them (sparkle/database/sparkle), matching Doc (organization) and Harness (bot). The content change bumps the source fingerprint, so the next boot re-imports the root with the icon — no manual SQL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
Test Results (shard 1) 12 files ± 0 12 suites ±0 6m 2s ⏱️ +39s Results for commit 6d2badf. ± Comparison against base commit a669f20. This pull request removes 1 and adds 109 tests. Note that renamed tests count towards both. |
Contributor
Test Results 49 files ± 0 49 suites ±0 18m 37s ⏱️ +7s Results for commit 6d2badf. ± Comparison against base commit a669f20. This pull request removes 1 and adds 109 tests. Note that renamed tests count towards both. |
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.
The recurring "Skill was never imported" bug — made impossible
The static-repo import sources for built-in AI content (Agent / Provider / Harness / Skill) were registered one-by-one in the portal and gated on a hand-maintained per-partition allow-list (
StaticRepoSync.Partitions). Forgetting one name — as happened with Skill — left that partition served from in-memory while the rest went to the DB: no import, silently, no error. This has been hit repeatedly.Fix: AI content is ONE bundle, owned by
MeshWeaver.AIAiContentSources.ContentPartitions— the canonical AI content partition set (Agent/Provider/Harness/Skill), living next to the sources.AiContentSources.AddBuiltInAiContentSources()— registers every built-in AIIStaticRepoSource.StaticRepoSyncExtensionsregisters the whole bundle whenever any AI partition is served — no per-name list to forget.MemexConfigurationexpands the served set to the whole bundle if it names any AI partition, soAddAI's per-type serve-from-DB gating stays consistent with the import (you can't half-serve AI content).The ratchet (so it can't come back)
AiContentSourcesTestreflects over everyIStaticRepoSourceinMeshWeaver.AIand fails if any is not in the bundle. Add a fifth AI content type without bundling it → red test. The class of "forgot to wire up the new partition" bug is now caught at build time, not in production.Verified
AiContentSourcesTest— 2 green.Memex.Portal.Sharedbuilds clean (the wiring change).Note
This is the "bundle it / treat them all the same" half of the rant. Public-read for AI content is already covered at the node-type level (
AddSkillType/AddAgentTypedoConfigureNodeTypeAccess(WithPublicRead(...))), and the importer already provisions the schema first and writes underPostingIdentity.System. The companion storm cure (heartbeating a dead_Activity/import-*lock) is #134.🤖 Generated with Claude Code