Skip to content

test: add skills catalog loading and validation tests#666

Open
Ayush7614 wants to merge 2 commits into
usestrix:mainfrom
Ayush7614:test/skills-catalog
Open

test: add skills catalog loading and validation tests#666
Ayush7614 wants to merge 2 commits into
usestrix:mainfrom
Ayush7614:test/skills-catalog

Conversation

@Ayush7614

@Ayush7614 Ayush7614 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds unit tests for strix.skills — previously untested despite growing community skill contributions.

Covers:

  • Internal categories (scan_modes, coordination) excluded from user-selectable skills
  • Regression guard for merged community skills (oauth, aws, django, prototype_pollution, insecure_deserialization)
  • validate_requested_skills() success and error paths
  • load_skills() frontmatter stripping, category-prefixed names, missing-skill handling
  • Catalog integrity: every user-selectable skill loads non-empty content

Why

Verification

uv run pytest tests/test_skills.py -v

Test plan

  • pytest tests/test_skills.py — 11 passed locally
  • ruff check tests/test_skills.py — clean

Cover validate_requested_skills, load_skills, internal category
exclusion, and regression checks for merged community skills.
@greptile-apps

greptile-apps Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds tests for the skills catalog loader and validator. The main changes are:

  • Coverage for excluding internal skill categories.
  • Regression checks for merged community skills.
  • Validation tests for accepted, too many, and unknown skill names.
  • Loader tests for frontmatter stripping, category prefixes, missing skills, and catalog content.

Confidence Score: 5/5

The changed tests look safe to merge after a small cleanup to the catalog content assertion.

  • The imports and API expectations match the current skills loader.
  • The new checks cover the main catalog and validation paths.
  • The only issue found is an arbitrary content-length threshold that can reject valid short skills.

tests/test_skills.py

Important Files Changed

Filename Overview
tests/test_skills.py Adds skills catalog tests; one catalog integrity assertion is stricter than the loader contract it is checking.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
tests/test_skills.py:87
**Short Skills Fail Catalog Test**

When a valid skill has non-empty content of 100 characters or less after frontmatter stripping, this catalog test fails even though the loader contract is satisfied. That can block lightweight skill additions for test policy rather than a broken catalog entry.

```suggestion
        assert content[name].strip(), f"{name}: loaded body is empty"
```

Reviews (1): Last reviewed commit: "test: add skills catalog loading and val..." | Re-trigger Greptile

Comment thread tests/test_skills.py Outdated
for name in sorted(get_all_skill_names()):
content = load_skills([name])
assert name in content, f"{name}: load_skills did not return content"
assert len(content[name].strip()) > 100, f"{name}: loaded body too short"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Short Skills Fail Catalog Test

When a valid skill has non-empty content of 100 characters or less after frontmatter stripping, this catalog test fails even though the loader contract is satisfied. That can block lightweight skill additions for test policy rather than a broken catalog entry.

Suggested change
assert len(content[name].strip()) > 100, f"{name}: loaded body too short"
assert content[name].strip(), f"{name}: loaded body is empty"
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_skills.py
Line: 87

Comment:
**Short Skills Fail Catalog Test**

When a valid skill has non-empty content of 100 characters or less after frontmatter stripping, this catalog test fails even though the loader contract is satisfied. That can block lightweight skill additions for test policy rather than a broken catalog entry.

```suggestion
        assert content[name].strip(), f"{name}: loaded body is empty"
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Assert non-empty loaded content instead of a 100-char minimum.
@Ayush7614

Ayush7614 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

cc: @rajpratham1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant