feat: add scripts.json schema validation#53
Merged
Conversation
Adds tools/validate_scripts.py (stdlib-only, all errors in one pass) and .github/workflows/validate-scripts-json.yml to validate scripts.json on push/PR. Step 1 of the issue-form submission pipeline. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new Python validator script and a GitHub Actions workflow are added. The script loads Changesscripts.json Validation
Sequence DiagramsequenceDiagram
participant GitHubActions
participant Runner
participant UV as uv
participant Validator as tools/validate_scripts.py
GitHubActions->>Runner: triggered (push / pull_request with path filters)
Runner->>Runner: checkout code
Runner->>UV: setup uv
Runner->>Validator: uv run tools/validate_scripts.py --scripts-json scripts.json
Validator->>Validator: load_and_parse(scripts.json)
Validator->>Validator: validate(data) -> (errors, warnings)
Validator->>Runner: exit code and stdout/stderr
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/validate-scripts-json.yml (1)
20-21: ⚡ Quick winPin third-party actions to commit SHAs instead of version tags for stronger supply-chain integrity.
Lines 20–21 use version tags (
@v6,@v8.1.0). Consider pinning to commit SHAs to prevent tag-move attacks:- uses: actions/checkout@<commit_sha> - uses: astral-sh/setup-uv@<commit_sha>Look up the commit SHA for each tag in the action's repository and replace the version tag.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/validate-scripts-json.yml around lines 20 - 21, Replace the third-party GitHub Action version tags with the corresponding commit SHAs to pin them; specifically update the uses entries for actions/checkout@v6 and astral-sh/setup-uv@v8.1.0 to use the commit SHAs for those tags (e.g., actions/checkout@<commit_sha>, astral-sh/setup-uv@<commit_sha>), by looking up the tag in each action's GitHub repo Releases/Tags page and copying the full commit SHA for the tag, then replacing the tag strings in the workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/validate_scripts.py`:
- Around line 75-79: The current validation for entry["images"] and
entry["tags"] only checks type but allows empty lists; update the checks in the
validate function (where entry and prefix are used) to require non-empty lists
by verifying isinstance(..., list) and len(...) > 0, and append a clear error
like "{prefix}: 'images' must be a non-empty list" or "{prefix}: 'tags' must be
a non-empty list" when the check fails.
- Around line 31-36: The current try/except only handles json.JSONDecodeError
when loading the file at path, so filesystem read errors (e.g., permission
denied, file not found) still raise tracebacks; update the error handling around
the open/json.load block in tools/validate_scripts.py to also catch OSError (or
more generally IOError) from open/read and handle it like the JSON error: print
a clear error to stderr including the path and the OS error message and call
sys.exit(1); ensure the existing json.JSONDecodeError handler remains to
distinguish parse errors from file read errors and reference the open(path,
encoding="utf-8") call and the json.load(f) usage when making the change.
---
Nitpick comments:
In @.github/workflows/validate-scripts-json.yml:
- Around line 20-21: Replace the third-party GitHub Action version tags with the
corresponding commit SHAs to pin them; specifically update the uses entries for
actions/checkout@v6 and astral-sh/setup-uv@v8.1.0 to use the commit SHAs for
those tags (e.g., actions/checkout@<commit_sha>,
astral-sh/setup-uv@<commit_sha>), by looking up the tag in each action's GitHub
repo Releases/Tags page and copying the full commit SHA for the tag, then
replacing the tag strings in the workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba5b388e-46ff-4d72-add2-cf91482c86c0
📒 Files selected for processing (2)
.github/workflows/validate-scripts-json.ymltools/validate_scripts.py
Comment on lines
+75
to
+79
| if "images" in entry and not isinstance(entry["images"], list): | ||
| errors.append(f"{prefix}: 'images' must be a list") | ||
|
|
||
| if "tags" in entry and not isinstance(entry["tags"], list): | ||
| errors.append(f"{prefix}: 'tags' must be a list") |
Contributor
There was a problem hiding this comment.
Enforce non-empty images and tags lists.
Line 75 and Line 78 validate only list type, but required fields are supposed to be non-empty as well. Empty lists currently pass.
💡 Proposed fix
- if "images" in entry and not isinstance(entry["images"], list):
- errors.append(f"{prefix}: 'images' must be a list")
+ if "images" in entry:
+ if not isinstance(entry["images"], list) or not entry["images"]:
+ errors.append(f"{prefix}: 'images' must be a non-empty list")
- if "tags" in entry and not isinstance(entry["tags"], list):
- errors.append(f"{prefix}: 'tags' must be a list")
+ if "tags" in entry:
+ if not isinstance(entry["tags"], list) or not entry["tags"]:
+ errors.append(f"{prefix}: 'tags' must be a non-empty list")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if "images" in entry and not isinstance(entry["images"], list): | |
| errors.append(f"{prefix}: 'images' must be a list") | |
| if "tags" in entry and not isinstance(entry["tags"], list): | |
| errors.append(f"{prefix}: 'tags' must be a list") | |
| if "images" in entry: | |
| if not isinstance(entry["images"], list) or not entry["images"]: | |
| errors.append(f"{prefix}: 'images' must be a non-empty list") | |
| if "tags" in entry: | |
| if not isinstance(entry["tags"], list) or not entry["tags"]: | |
| errors.append(f"{prefix}: 'tags' must be a non-empty list") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/validate_scripts.py` around lines 75 - 79, The current validation for
entry["images"] and entry["tags"] only checks type but allows empty lists;
update the checks in the validate function (where entry and prefix are used) to
require non-empty lists by verifying isinstance(..., list) and len(...) > 0, and
append a clear error like "{prefix}: 'images' must be a non-empty list" or
"{prefix}: 'tags' must be a non-empty list" when the check fails.
Member
Author
There was a problem hiding this comment.
while not recommended, images list can be empty. tags should not be though.
Contributor
There was a problem hiding this comment.
@pfeerick, thanks for the clarification! In that case, only tags needs the non-empty enforcement. Here's the revised fix:
- if "images" in entry and not isinstance(entry["images"], list):
- errors.append(f"{prefix}: 'images' must be a list")
+ if "images" in entry and not isinstance(entry["images"], list):
+ errors.append(f"{prefix}: 'images' must be a list") # empty list is allowed
- if "tags" in entry and not isinstance(entry["tags"], list):
- errors.append(f"{prefix}: 'tags' must be a list")
+ if "tags" in entry:
+ if not isinstance(entry["tags"], list) or not entry["tags"]:
+ errors.append(f"{prefix}: 'tags' must be a non-empty list")This keeps images as a type-only check (empty list passes) while requiring tags to be a non-empty list.
ʕ •ᴥ•ʔ 🐇✏️
✏️ Learnings added
Learnt from: pfeerick
Repo: EdgeTX/lua-scripts PR: 53
File: tools/validate_scripts.py:0-0
Timestamp: 2026-05-13T01:50:18.150Z
Learning: In `tools/validate_scripts.py` (EdgeTX/lua-scripts repo), the `images` field in `scripts.json` entries is allowed to be an empty list (while not recommended), so only a type check (`isinstance(entry["images"], list)`) is needed. The `tags` field, however, must be a non-empty list and should be validated with both `isinstance(..., list)` and a non-empty check (`not entry["tags"]`).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/validate_scripts.py (1)
124-126: ⚡ Quick winWrite validation errors to stderr for consistency
Warnings are sent to stderr, but error lines here are sent to stdout. Sending both to stderr improves CLI/CI behavior and log handling.
Proposed fix
- print(f"Found {len(errors)} error(s) in {args.scripts_json}:") + print(f"Found {len(errors)} error(s) in {args.scripts_json}:", file=sys.stderr) for error in errors: - print(f" - {error}") + print(f" - {error}", file=sys.stderr)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/validate_scripts.py` around lines 124 - 126, The error printing currently uses print() which writes to stdout; change the two lines that output validation errors (the f"Found {len(errors)}..." and the loop printing each error) to write to stderr by passing file=sys.stderr (e.g. print(..., file=sys.stderr)); ensure sys is imported at top if not already. This keeps warnings and errors consistent by sending validation errors to stderr while preserving the same message text and formatting for the variables errors and args.scripts_json.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/validate_scripts.py`:
- Line 23: The REQUIRED_FIELDS array currently includes "images" which causes a
hard error while the later warning block also flags missing images; remove
"images" from REQUIRED_FIELDS so missing images only triggers the existing
warning path, and keep the warning generation code (the block that checks for
missing images and appends the warning) intact; update any tests or comments
referencing REQUIRED_FIELDS to reflect that "images" is now optional and ensure
no other validation code treats "images" as required.
---
Nitpick comments:
In `@tools/validate_scripts.py`:
- Around line 124-126: The error printing currently uses print() which writes to
stdout; change the two lines that output validation errors (the f"Found
{len(errors)}..." and the loop printing each error) to write to stderr by
passing file=sys.stderr (e.g. print(..., file=sys.stderr)); ensure sys is
imported at top if not already. This keeps warnings and errors consistent by
sending validation errors to stderr while preserving the same message text and
formatting for the variables errors and args.scripts_json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90b4f0dc-2127-465f-bebb-db99d022f146
📒 Files selected for processing (1)
tools/validate_scripts.py
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.
Summary
tools/validate_scripts.py— stdlib-only Python validator that checks every entry inscripts.jsonin a single pass and reports all errors before exiting.github/workflows/validate-scripts-json.yml— CI workflow that runs the validator on push tomainand on PRs whenscripts.jsonor the validator itself changesValidation rules
name,category,description,infourl,images,tagscategorymatches one of the 7 known valuesinfourlstarts withhttp://orhttps://imagesandtagsare listsnamevalues (case-insensitive)Notes
This is step 1 of the issue-form submission pipeline —
validate_scripts.pyis designed to be called by that pipeline later as well.Test plan
uv run tools/validate_scripts.py --scripts-json scripts.jsonpasses against currentscripts.json(53 entries, exit 0)Validate scripts.jsonworkflow appears and passes in GitHub Actions on this PR🤖 Generated with Claude Code
Summary by CodeRabbit