chore(skills): drop the legacy untyped skill metadata parsing - #331
Merged
Conversation
`parseSkillMetadataContent` accepted `.oo-metadata.json` files that
predate the typed format, reading `{version}` as bundled metadata and
`{packageName, version}` as registry metadata. The TODO guarding it
assumed existing installs had since migrated, so this removes the
fallback along with `parseLegacyBundledSkillMetadata`; only metadata
carrying `schemaVersion` and a valid `kind` is recognized now.
That assumption holds for bundled skills, which startup auto-sync
re-stamped on the first run after upgrading, but not for registry
skills: the canonical directory is written only by install and update,
so a registry skill installed before the typed format landed still
carries legacy metadata. Those directories now classify as unmanaged,
which makes them invisible to the inventory and its consumers —
notably `oo skills sync upload`, whose full-replace `PUT` would drop
them from the account manifest.
Test fixtures that seeded managed skills with untyped literals move to
the typed constructors. The deleted blank-identity cases are restored
against typed input, since they were the only coverage of the
`toNonBlankString` wiring — swapping it for a bare `typeof` check
otherwise passed the whole suite.
Signed-off-by: Kevin Cui <bh@bugs.cc>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
Summary by CodeRabbit
WalkthroughSkill metadata parsing now accepts only supported typed metadata, trims typed identity fields, rejects blank required values, and removes legacy bundled and registry fallbacks. Tests classify schema-less legacy metadata as unmanaged and use metadata factory helpers for fixtures. English and Chinese documentation now describe valid metadata requirements and the updated legacy compatibility rule. Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
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
1 participant
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.
parseSkillMetadataContentstill accepted.oo-metadata.jsonfiles that predate the typed format, reading{version}as bundled metadata and{packageName, version}as registry metadata. The TODO guarding that fallback assumed existing installs had migrated by now, so this removes it along withparseLegacyBundledSkillMetadata; only metadata carryingschemaVersionand a validkindis recognized from here on.Worth a careful look before approving: that "already migrated" assumption holds for bundled skills but not for registry ones. Startup auto-sync re-stamps a bundled skill whenever the installed version differs from the CLI version, so bundled installs migrated themselves on the first run after upgrading. Nothing ever did the equivalent for registry skills —
listCanonicalRegistrySkillsonly reads the canonical directory, which is written solely by install and update — so a registry skill installed before the typed format landed and never updated since still carries legacy metadata today.Those directories now classify as
unmanaged, which drops them out ofreadInstalledSkillsand everything built on it. The consequence I'd most want a second opinion on isoo skills sync upload: it is a full-replacePUT /v1/skills, so a user whose registry skills are all legacy would silently push a shorter list — or[]— and wipe their account manifest, with a success message and exit code 0. Two smaller ones:oo skills share <id>falls back to treating the skill id as a package name (isSkillDirectoryAbsentcovers onlymissing/not-directory, so a legacy directory still yields a share target with nopackageName), andoo uninstall --purgenow retains legacy bundled directories instead of removing them. All three are left unmitigated here so the removal stays reviewable on its own — happy to add a guard on the upload path in a follow-up if we think legacy metadata is still out in the wild.The rest is fixture work. Tests that seeded managed skills with untyped literals move to the typed constructors. The blank-identity cases in
skill-metadata.test.tsare restored against typed input rather than deleted — they were the only coverage of thetoNonBlankStringwiring, and swapping it for a baretypeofcheck otherwise passed the entire suite. Docs in both languages drop the "legacy metadata remains readable" promise, and the--forcegloss changes from "no readable.oo-metadata.json" to "no validoometadata", which a legacy file now fails despite being perfectly readable.