feat: colon-safe moduleID resolution and scoped api.remove(moduleID, apiPath) - #306
Open
cldmv-bot[bot] wants to merge 13 commits into
Open
feat: colon-safe moduleID resolution and scoped api.remove(moduleID, apiPath)#306cldmv-bot[bot] wants to merge 13 commits into
cldmv-bot[bot] wants to merge 13 commits into
Conversation
…reload `:` is slothlet's internal composite "moduleID:apiPath" separator. A user (or internal) moduleID that itself contained a colon — e.g. a `vine:abc` namespaced convention — mounted and resolved fine and add() returned the exact id, but the id could not be removed or reloaded by that id: removeApiComponent split the argument on `:` and looked up only the first segment, and child wrappers of a multi-child mount were attributed to the truncated base, so remove() left the subtree behind. The mount was silently unmanageable. - removeApiComponent now resolves an exact registered moduleID verbatim before falling back to the split-based "<base>_<hash>" / composite heuristic, and the no-ownership branch no longer truncates the id on ":". - Base-id recovery no longer splits the composite: tagSystemMetadata stores the raw owning-module id as baseModuleID alongside the composite moduleID, and the two unified-wrapper recovery sites (impl:changed event, child attribution) read it verbatim. This also corrects internal `versionDispatcher:<path>` base ids, which were previously truncated to `versionDispatcher`. The composite stays the per-path user-metadata key; only the fragile colon-split recovery is removed. Adds colon-moduleID round-trip coverage (remove, leaves, reload, reload-then-remove, synthetic, multi-colon, hyphen control). Fixes #303
cldmv-bot
Bot
added
! fix → next
There was a problem hiding this comment.
Pull request overview
Fixes runtime add/remove/reload behavior when a user-supplied moduleID contains : (which also serves as Slothlet’s internal "moduleID:apiPath" composite separator), ensuring the module ID round-trips verbatim through lifecycle and ownership operations.
Changes:
- Adds a new regression test suite covering
remove(),leaves(), andreload()with colon-containing moduleIDs. - Stores the raw owning module ID separately as
baseModuleIDin system metadata and uses it in wrapper lifecycle/child attribution paths. - Updates
removeApiComponent()to prefer verbatim moduleID matching before applying any “base” heuristics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/vitests/suites/api-manager/api-manager-colon-module-id.test.vitest.mjs | Adds regression coverage for colon-containing moduleID round-tripping across add/leaves/remove/reload. |
| src/lib/handlers/unified-wrapper.mjs | Switches lifecycle/module attribution to use baseModuleID (colon-safe) rather than splitting composite IDs. |
| src/lib/handlers/metadata.mjs | Persists baseModuleID alongside composite moduleID to allow verbatim module ownership recovery. |
| src/lib/handlers/api-manager.mjs | Adjusts moduleID resolution in removeApiComponent() to avoid truncating colon-containing IDs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address Copilot review on #306. The verbatim-first match resolves an exact registered id, but the fallback still did `pathOrModuleId.split(":")[0]`, which truncated on ':': `remove("vine:abc")` with only a "vine" module registered matched "vine" and wrongly removed it (returned true, tore down the wrong mount). Match the auto-generated "<id>_<hash>" form of the WHOLE id instead of a ':'-truncated prefix, so a colon-containing id never collides with a shorter base. Adds a regression test asserting remove("vine:abc") leaves a registered "vine" mount untouched (and the real id still removes it).
remove(id) removes every path a module owns and remove(apiPath) removes that path's whole subtree regardless of which module owns each node, so neither can surgically remove a single module's node at a shared path — e.g. two modules mounted under the same namespace, or one moduleID reused across mounts. Add an optional second argument: remove(moduleID, apiPath) resolves the first argument strictly as a moduleID, verifies it owns apiPath, and removes only that node — sibling modules sharing the mount and the module's other mounts are left intact. Returns whether anything was removed; the single-argument forms are unchanged. Routes through the existing single-node removal path.
cldmv-bot
Bot
changed the title
Contributor
Author
|
Auto-normalized PR title: rewrote PR title to match the highest-priority commit type (
If this isn't what you want, edit the title — the normalizer won't re-fire as long as the title stays conventional. |
cldmv-bot
Bot
added
type: documentation
Shinrai
changed the title
Shinrai
previously approved these changes
Aug 24, 2026
…ng on ':'
The previous fallback dropped colon handling entirely to stop remove("vine:abc")
from truncating to a registered "vine". But a leaf's __metadata.moduleID is the
internal composite "<moduleID>:<apiPath-with-slashes>", and remove(meta.moduleID)
is a supported call — dropping the split broke removing a module by that
composite (metadata-collision-modes).
Restore composite resolution safely: match a registered module whose id is the
':'-delimited prefix ONLY when it actually owns the apiPath encoded in the
suffix. That ownership check distinguishes a real composite (base owns the path)
from a bare user id that merely contains ':' (a "vine:abc" whose "abc" no "vine"
owns still resolves to nothing, not to "vine"). Verbatim and "<id>_<hash>"
matches still run first.
…h composite Replace ':' as slothlet's internal composite separator with a reserved token (MODULE_ID_SEPARATOR = "__slothlet_sep__"). A ':' is common in real module ids — a `vine:abc` namespacing convention, the internal `versionDispatcher:<path>` id — so using it as the splitter made those ids un-round-trippable and forced fragile disambiguation. With a reserved token no module id may contain, the composite is always unambiguous to split and ':' (and every other character) is free. - metadata.mjs builds the composite with MODULE_ID_SEPARATOR (exported). - add() refuses a moduleID containing the separator (MODULE_ID_RESERVED_SEPARATOR, all 12 locales) — the only token a module id may not contain. - removeApiComponent recovers the base by splitting on the separator: a plain id (which cannot contain it) is preserved intact, a composite strips to its base. Drops the earlier ':'-specific verbatim/ownership-verified workarounds. - Update the metadata format tests (system-metadata, user-metadata) that pinned the old `prefix:apiPath` shape to the new separator; they now assert structure via the imported constant. __metadata.moduleID reads e.g. "modm__slothlet_sep__m/leaf". Fixes #303
…rator) Address Copilot review on #306. The doc block still described a Promise<void> return and an underscore-based moduleID heuristic. Document the boolean return, the options argument (scopedApiPath / recordHistory), and that the argument is resolved by splitting on the reserved composite separator — matching the current behavior.
…d separator Address Copilot review on #306. add() rejected a user-supplied moduleID containing MODULE_ID_SEPARATOR, but a default moduleID is derived from the apiPath (buildDefaultModuleId), so mounting at an apiPath whose segment carries the separator produced an auto-generated id containing it — breaking the "no moduleID contains the separator" invariant that remove()/metadata splitting rely on. Check the effective moduleID (user-supplied or auto-generated) once it is finalized and refuse it with MODULE_ID_RESERVED_SEPARATOR. Adds a regression test for the no-moduleID (apiPath-carries-separator) case.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/lib/handlers/unified-wrapper.mjs:2029
- This comment still describes splitting a composite "moduleID:apiPath" tag, but the composite metadata key is now joined with the reserved multi-character separator (MODULE_ID_SEPARATOR). Update the wording to avoid implying ':' is still the delimiter.
// The raw base id, stored verbatim (colon-safe) — no longer recovered by splitting the
// composite "moduleID:apiPath" tag, which truncated a base id that itself contained a
// colon (a user `vine:abc` convention, or an internal `versionDispatcher:<path>`) (#303).
…uleID reads Address a Copilot suppressed-comment finding on #306. Two comments near the child-attribution and impl:changed baseModuleID reads still described recovering the base by splitting a "moduleID:apiPath" tag on ':'. The composite now joins with the reserved MODULE_ID_SEPARATOR and the base is read verbatim from baseModuleID; reword to match. Comment-only.
Contributor
|
Addressed the suppressed comment from this review ( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/lib/handlers/api-manager.mjs:2474
- In the scoped two-arg removal path, the code only verifies ownership of the exact
scopedApiPathand then proceeds down the single-path removal branch. If the scoped path is a namespace/container that the module owns descendants under (OwnershipManager registers containers and leaf paths separately), removing just the container path can leave descendant ownership entries orphaned (still inmoduleToPath/pathToModulebut no longer reachable in the API tree), which can break later reload/remove behavior.
Consider either (a) implementing scoped removal as a prefix removal of all owned paths under scopedApiPath, or (b) explicitly rejecting scoped removals for non-leaf paths (paths that have owned descendants) to avoid corrupting ownership state.
const normalizedScoped = this.normalizeApiPath(scopedApiPath).apiPath;
const ownedPaths = this.slothlet.handlers.ownership?.moduleToPath?.get(moduleID);
if (!ownedPaths || !ownedPaths.has(normalizedScoped)) {
return false;
}
Address a Copilot suppressed-comment finding on #306. Scoped removal verified ownership of only the exact scopedApiPath, then removed that single node — so scoping to a container the module owns descendants under deleted the container's subtree from the api tree but left the descendants' ownership records orphaned in moduleToPath/pathToModule, breaking later reload/remove. Scoped removal now drops this module's ownership of every descendant path under the scoped node before removing the node itself (whose deletePath removes the subtree from the tree in one shot). A leaf scoped path has no descendants, so the single-node behavior is unchanged; a container removes the module's whole subtree there while a sibling module sharing the mount is left intact. Adds a container prefix-removal test asserting no orphaned ownership and sibling survival.
Contributor
|
Addressed the suppressed comment from this review ( |
…ot the path The previous attempt deleted the whole subtree at apiPath, which is wrong: the two-argument form must remove only THAT module's contribution across the path, never the entire path (that is what remove(apiPath) is for). Scoped removal now walks every path the module owns at or under apiPath, deepest-first, and for each either removes it (when this module was its sole owner) or reverts it to the module that owned it before (when the node is shared). A node still holding another module's descendant is left standing — a container is deleted only once nothing remains under it and it is unowned. So remove(modA, "svc.a") drops modA's leaf and keeps a sibling's svc.b; remove(modA, "shop") on a shared container reverts the container and keeps modB's shop.b; and a module that solely owns a subtree has the whole subtree removed with no orphaned ownership. Tests cover the leaf, shared-container, and sole-owned-subtree cases.
Contributor
|
Correction to my note above: |
…full-path) Address Copilot review on #306. The scoped remove(moduleID, apiPath) branch pushed {type:"remove", apiPath} to operationHistory, but reload replay re-runs a "remove" op as a single-argument removeApiComponent(apiPath) — a whole-path removal — which would over-delete other modules sharing the path. A module-scoped replay is not expressible either: replay regenerates moduleIDs, so the original id could never be matched. Recording a whole-path remove is worse than not recording, so drop the push (which also stops it from ignoring recordHistory). A scoped removal therefore does not persist across a reload — documented inline; a follow-up could add moduleID-stable replay if scoped removals need to survive reload.
A module's id survives a reload — the "add" op records options.moduleID and
addApiComponent reuses it on re-add (user-supplied and auto-generated
<path>_<hash> ids alike; the "replay_<hash>" param the replay passed was dead,
addApiComponent has no top-level moduleID param). So a scoped removal CAN be
replayed by id, contrary to the earlier assumption.
Record scoped removals as {type:"remove", apiPath, scopedModuleID} (honoring
recordHistory), and replay them via the two-argument
removeApiComponent(scopedModuleID, {scopedApiPath}) so only that module's nodes
go — not the whole path. Verified end-to-end: after a reload, remove(modA,"shop")
keeps modA's subtree gone while modB's survives (no over-delete). Also drop the
dead replay_<hash> param and correct the replay comment. Adds a reload-
persistence test.
…estore revert
Synthetic (in-memory) adds — an inline function, an export map, or a
`{ exports, ...options }` object — now record their original inline value as the
replay folderPath instead of the internal `synthetic:<path>` sentinel, which a
full-instance reload() would otherwise resolve as a filesystem path and fail on.
A synthetic add (and a scoped removal of one) now survives reload(), keeping its
moduleID so it stays removable afterward. restoreApiPath re-adds via the same
recorded folderPath, so ownership rollback of a synthetic mount is consistent too.
Also cover the scoped-removal restore branch that had no test: a shadowed-node
case drives the revert-to-previous-owner path, and the dead else fallback now
uses `/* v8 ignore else */` with an honest unreachability comment (a node whose
removePath returns "restore" always has a concrete current value, so the else
never runs) instead of a `never exercised in tests` justification.
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.
🚀 What's Changed
💥 Breaking Changes
No breaking changes
✨ Features
🐛 Bug Fixes
📦 Dependencies
No dependency updates
🔧 Other Changes
👥 Contributors