Repository navigation
fix: make the manifest validator agree with the store it validates - #84
Draft
openvoiceos-bot wants to merge 1 commit into
Draft
openvoiceos-bot wants to merge 1 commit into
openvoiceos-bot wants to merge 1 commit into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JarbasAl
force-pushed
the
fix/t5229-store-manifest-validation
branch
from
October 2, 2026 06:25
55c5651 to
b9c4d1c
Compare
…dates
Every one of the 36 committed entries failed validate_submission, and
nothing reported it: the workflow only checks the entries a pull request
touches, and validate_raw_jsons.py says so in its own docstring ("most of
them do not pass it").
Most of those failures were the validator's, not the data's.
icon was required to be a URL. The store renders icon as a react-icons
component name: SkillCard.tsx resolves it against react-icons and
IconSelector.tsx offers those names on the submission form, so every
committed entry holds "FaMicrophone", "MdWallpaper" and the like. The rule
rejected the convention the store itself uses, and would have rejected any
correct new submission. A react-icons name is now accepted, and a URL
still is, because entries predating the marketplace migration used one.
The skill_id pattern rejected underscores, but neon_homeassistant_skill.mikejgray
is a real committed entry and a skill id is often derived from a Python
package name. Widened rather than renaming a third party's skill id to fit
a pattern.
That left four real gaps the icon error had been hiding:
skill-spelling no examples added four spelling phrasings
skill-wallpapers empty tags wallpaper, background, pictures, reddit
skill-audio-recording empty tags recording, audio, voice memo, dictation
skill-easter-eggs empty tags easter eggs, fun, references
36 of 36 failing before, 1 of 36 after. The one left is
skill-homeassistant.json, which ships no examples: it is a third-party skill,
so the voice phrasings belong to its maintainer rather than being invented
here.
scripts/check_all_raw_jsons.py runs the same validator over the WHOLE
directory, so a rule that contradicts the corpus is one command away from
being visible instead of hiding behind a per-PR diff. Its KNOWN_FAILURES
holds that single entry and is asserted accurate in both directions: an
entry that starts failing cannot hide in it, and an entry that starts
passing must leave it.
Four entries predate the per-entry file-name rule, which requires a file's
name to equal its skill_id lowercased with dots replaced by dashes, and
failed CI on that rule rather than on their content. Renamed, content
untouched:
skill-audio-recording.json -> skill-ovos-audio-recording-openvoiceos.json
skill-easter-eggs.json -> skill-easter-eggs-jarbasai.json
skill-spelling.json -> ovos-skill-spelling-openvoiceos.json
skill-wallpapers.json -> skill-ovos-wallpapers-openvoiceos.json
The workflow now runs the whole-corpus check after the per-entry check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JarbasAl
force-pushed
the
fix/t5229-store-manifest-validation
branch
from
October 2, 2026 06:50
b9c4d1c to
9d3648a
Compare
This branch has not been deployed
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 changed
7bfec9diconandskill_idrules accept what the store itself writes; four real gaps filled;scripts/check_all_raw_jsons.pyvalidates the whole corpus, in CI55c5651skill_idgives; their contents are untouchedA reviewer has two things to weigh: the widened
iconrule, and the fourrenames.
The task's premise does not hold on this repository
The brief asked to rename
titletonamein two manifests, addnametofive, and make the validator check those fields. Measured on
master:nameANDdescription"title"has never been a key inraw_jsons/--git log -S '"title"'returns nothing, and the same search for"name"returns a commit, so the search works
nameanddescriptionare alreadyREQUIRED_FIELDS, with length boundsraw_jsons/*.json, soovos-skill-date-time.jsonis notmissed
The review this came from cites
src/components/SkillCard.astro. Thisrepository has a React frontend (
frontend/src/components/SkillCard.tsx) and no.astrofile in any branch or anywhere in its history. So no card renders ablank title for the reason given, and there was nothing to rename to
name.What is actually broken
Every one of the 36 entries fails
validate_submission, and nothing reportsit: the workflow only checks entries a pull request touches, and
validate_raw_jsons.pyadmits it in its docstring -- "most of them do notpass it".
Most of those failures are the validator's, not the data's.
iconwas required to be a URL. The store rendersiconas a react-iconscomponent name:
SkillCard.tsxresolves it against react-icons andIconSelector.tsxoffers those names on the submission form. Every committedentry holds
FaMicrophone,MdWallpaperand the like. The rule rejected thestore's own convention, and would have rejected any correct new submission.
The
skill_idpattern rejected underscores, butneon_homeassistant_skill.mikejgrayis a real committed entry, and a skill idis often derived from a Python package name. Widened, rather than renaming a
third party's skill id to fit a pattern.
With the icon error out of the way, four real gaps it had been hiding:
examplestagstagstags36 of 36 failing before, 1 of 36 after.
The remaining one is
skill-homeassistant.json, which ships noexamples. Itis a third-party skill, so its voice phrasings belong to its maintainer rather
than being invented here.
The four renames, commit
55c5651validate_raw_jsons.pyasks that a file be named asapply_submission.pywouldwrite it: the
skill_id, lowercased, with dots replaced by dashes. Four entriespredate that rule and failed it, so CI was red on the rule and not on the code.
Renamed, contents byte-for-byte unchanged:
skill-audio-recording.jsonskill-ovos-audio-recording-openvoiceos.jsonskill-easter-eggs.jsonskill-easter-eggs-jarbasai.jsonskill-spelling.jsonovos-skill-spelling-openvoiceos.jsonskill-wallpapers.jsonskill-ovos-wallpapers-openvoiceos.jsonNothing outside
scripts/reads these names:generate_store_data.pyglobs thedirectory and reads the contents, so the file name is not an identifier, and
git grepfinds no reference to any of the four old names outsideraw_jsons/itself. Measured: the four renamed entries passvalidate_raw_jsons.py, 4 of 4; the control, the oldskill-spelling.jsonname, fails with the rule's own sentence and exit 1.
The rename also closes a duplicate-submission hole for these four. The duplicate
check at
apply_submission.py:254tests whether the target file exists, and thetarget name is derived from the
skill_id. A second submission of the spellingskill is refused with "Skill already exists: ovos-skill-spelling.openvoiceos";
against the old file name the check saw nothing and would have written a second
file for the same skill. The same hole stays open for the 32 entries whose file
name still differs from their
skill_id, which is the follow-up below.The guard
scripts/check_all_raw_jsons.pyruns the same validator over the WHOLEdirectory, so a rule that contradicts the corpus is one command away from being
visible instead of hiding behind a per-PR diff.
KNOWN_FAILURESholds thesingle entry above and is asserted accurate in both directions: an entry that
starts failing cannot hide in it, and an entry that starts passing must leave
it.
Verified before committing:
The workflow runs it after the per-entry check.
Follow-up, not in this pull request
The guard calls
validate_submission(
scripts/check_all_raw_jsons.py:45). The per-pull-request validator callsvalidate_raw_jsons.check_entry, which adds two rules on top: the file namemust equal the
skill_idwith dots replaced by dashes, and no two entries mayshare a
skill_id. So the guard does not see the rule the rename commit existsto satisfy. Measured at this head:
check_entryover all 36 entries fails 32,every one of them on the file-name rule, while
validate_submissionfails 1.The consequence is bounded but real: a later pull request that touches any of
those 32 entries turns the per-entry step red for a reason unrelated to its own
change, and the duplicate check misses them. The debt is inherited from
masterand this change reduces it, so it is tracked separately: either the guard calls
check_entrywith the 32 listed as exemptions, or all 36 entries are renamed.Not done
No
namefield was added, because every entry carries one. If the blank cardsare real on the deployed site, they do not come from this repository's
raw_jsons/at the head measured, and the next step is to say which artifactthe cards were read from.