diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 980454d..b1613a7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,30 +1,26 @@ # Contributing to AutoSkills -First off, thank you for considering contributing to AutoSkills! It's people like you that make open source such a great community. +Thank you for contributing to AutoSkills. This guide covers the full workflow, from finding an issue to opening a clean PR. -## 1. Where do I go from here? +## 1. Find or open an issue first -If you've noticed a bug or have a feature request, please [open an issue](../../issues) on GitHub. It's the best way to get things started. +Before writing code, [open an issue](../../issues) or find an existing one. -## 2. Fork & create a branch +Describe what you want to add or fix and why. Maintainers use issues to coordinate work, avoid duplicates, and give early feedback before you invest time in a PR. -If this is something you think you can fix, then [fork AutoSkills](../../fork) and create a branch with a descriptive name. +## 2. Fork and create a branch -A good branch name would be (where issue #325 is the ticket you're working on): +[Fork AutoSkills](../../fork), then create a descriptive branch: ```sh -git checkout -b 325-add-new-feature +git checkout -b 325-add-tailwind-detection +git checkout -b fix/ruby-gems-empty-array +git checkout -b docs/improve-contributing-guide ``` -Or for a general fix: +## 3. Make your changes -```sh -git checkout -b fix/typo-in-readme -``` - -## 3. Implement your fix or feature - -At this point, you're ready to make your changes! Feel free to ask for help on your PR if you get stuck. +Make the smallest change that fully solves the problem. Ask for help in the PR — you don't need to wait until it's perfect. ## 4. Install Git hooks @@ -36,36 +32,265 @@ pnpm run hooks:install This configures Git to use the versioned hooks in `.githooks/`. The pre-commit hook formats staged files and adds the formatted result to the commit automatically. -## 5. Get the style right - -Your patch should follow the same coding conventions and style as the rest of the project. Please ensure: +## 5. Follow the style -- Your code is properly linted and formatted before pushing. -- All new features and bug fixes include relevant tests. -- Existing tests pass locally before committing. +- Lint and format before pushing. +- Add or update tests for every behavior change. +- Make sure all tests pass locally before opening a PR. -## 6. Make a Pull Request +## 6. Open a pull request -At this point, you should switch back to your master branch and make sure it's up to date with the main repository: +Sync with main before pushing: ```sh git remote add upstream https://github.com/midudev/autoskills.git git checkout main git pull upstream main +git checkout your-branch +git rebase main +git push --set-upstream origin your-branch ``` -Then update your feature branch from your local copy of main, and push it! +Keep the PR title short and in the imperative mood: `feat: add Bun detection`, `fix: empty gems array in Ruby entry`, `test: validate skills map entries`. + +## 7. Keep your PR up to date + +When a maintainer asks you to rebase, the base branch has moved. Update your branch: ```sh -git checkout 325-add-new-feature -git rebase main -git push --set-upstream origin 325-add-new-feature +git fetch upstream +git rebase upstream/main +git push --force-with-lease +``` + +## 8. AutoSkills-specific contributions + +Most AutoSkills changes fall into one of these: + +- Add detection for a new technology +- Fix a detection edge case +- Add or update skills for an existing technology +- Add a combo that provides value beyond individual detections + +**Keep each PR to one concern.** One technology, one fix, one combo. Mixing unrelated changes slows review. + +--- + +### Adding a technology + +Add one entry to `SKILLS_MAP` in `packages/autoskills/skills-map.ts`: + +```ts +{ + id: "bun", + name: "Bun", + detect: { + configFiles: ["bun.lockb", "bunfig.toml"], + }, + skills: [ + "owner/repo/bun-best-practices", + ], +} +``` + +Every entry requires these four fields: + +| Field | Type | Description | +| -------- | ---------- | ------------------------------------------------ | +| `id` | `string` | Stable, unique, lowercase. Use `-` as separator. | +| `name` | `string` | User-facing display name. | +| `detect` | `object` | At least one detection signal (see below). | +| `skills` | `string[]` | One or more skill references. | + +#### Detection signals — choose the simplest one that works + +Use signals in this order of preference: + +**1. `packages`** — match npm/pip/cargo package names + +```ts +detect: { + packages: ["react", "react-dom"], +} +``` + +**2. `configFiles`** — match specific filenames in the project root + +```ts +detect: { + configFiles: ["next.config.js", "next.config.mjs", "next.config.ts"], +} +``` + +**3. `gems`** — match Ruby gem names + +```ts +detect: { + gems: ["rails", "sinatra"], +} +``` + +**4. `packagePatterns`** — match packages by RegExp when a prefix covers multiple packages + +```ts +detect: { + packagePatterns: [/^@aws-sdk\//, /^aws-cdk/], +} +``` + +**5. `configFileContent`** — search file contents when names and packages are not enough + +```ts +// Standard form — search inside specific files +detect: { + configFileContent: { + files: ["wrangler.json", "wrangler.toml"], + patterns: ["durable_objects"], + }, +} + +// Gradle form — let the scanner find build.gradle files automatically +detect: { + configFileContent: { + scanGradleLayout: true, + patterns: ["com.android.application", "com.android.library"], + }, +} +``` + +You can combine signals — the entry matches when any signal is found: + +```ts +detect: { + packages: ["tailwindcss"], + configFiles: ["tailwind.config.js", "tailwind.config.ts"], +} +``` + +#### Skill reference format + +``` +owner/repo/skill-name # GitHub-hosted skill +https://example.com/skill # URL-based skill +``` + +Real example: `"vercel-labs/agent-skills/vercel-react-best-practices"` + +Verify that the referenced skill actually exists before opening a PR. + +#### What the validator catches + +Run the tests and you'll see exactly what needs fixing: + +``` +✗ [my-tech] detect.packages is empty — add at least one package name (e.g. ["react", "react-dom"]) +✗ [my-tech] skill "my-best-practices" is not a valid reference — use "owner/repo/skill-name" or an https:// URL +✗ [my-tech] detect has no signals — define at least one of: packages, configFiles, gems, packagePatterns, or configFileContent +``` + +Common mistakes: + +```ts +// ✗ packages is empty +detect: { + packages: []; +} + +// ✗ skill ref is missing owner/repo +skills: ["react-best-practices"]; + +// ✗ packagePattern is a string, not a RegExp +detect: { + packagePatterns: ["^@aws-sdk/"]; +} + +// ✓ correct +detect: { + packages: ["react"]; +} +skills: ["vercel-labs/agent-skills/vercel-react-best-practices"]; +detect: { + packagePatterns: [/^@aws-sdk\//]; +} +``` + +--- + +### Adding a combo + +Add one entry to `COMBO_SKILLS_MAP` in `packages/autoskills/skills-map.ts`: + +```ts +{ + id: "nextjs-supabase", + name: "Next.js + Supabase", + requires: ["nextjs", "supabase"], + skills: [ + "supabase/agent-skills/supabase-postgres-best-practices", + ], +} ``` -Finally, go to GitHub and create a Pull Request on the main repository. +Add a combo only when it provides skills that are not already covered by the individual technology entries. A combo that just duplicates standalone skills adds noise. + +| Field | Requirement | +| ---------- | ----------------------------------------------- | +| `id` | Unique, lowercase, hyphen-separated. | +| `name` | User-facing, e.g. `"Next.js + Supabase"`. | +| `requires` | Array of two or more existing `SKILLS_MAP` ids. | +| `skills` | At least one valid skill reference. | + +If you reference an id that doesn't exist in `SKILLS_MAP`, the test fails: + +``` +✗ [nextjs-supabase] requires "supbase" which is not in SKILLS_MAP — fix the typo or add the technology first +``` + +--- + +### Run the tests + +```sh +cd packages/autoskills +node --test 'tests/*.test.ts' +``` + +Expected output when everything is correct: + +``` +✔ SKILLS_MAP validation › has unique technology ids +✔ SKILLS_MAP validation › validates every technology entry shape +✔ COMBO_SKILLS_MAP validation › has unique combo ids +✔ COMBO_SKILLS_MAP validation › validates every combo entry shape +``` + +--- + +### PR checklists + +#### Adding a technology + +- [ ] One entry added to `SKILLS_MAP` in `packages/autoskills/skills-map.ts` +- [ ] Detection uses the simplest reliable signal +- [ ] All skill refs follow the `owner/repo/skill-name` or `https://` format +- [ ] Tests pass locally (`node --test 'tests/*.test.ts'`) +- [ ] PR title uses the imperative mood: `feat: add Bun detection` + +#### Adding a combo + +- [ ] One entry added to `COMBO_SKILLS_MAP` in `packages/autoskills/skills-map.ts` +- [ ] `requires` contains at least two ids that exist in `SKILLS_MAP` +- [ ] The combo adds skills not already covered by the individual entries +- [ ] Tests pass locally +- [ ] PR title uses the imperative mood: `feat: add Next.js + Supabase combo` + +#### Fixing detection -## 7. Keeping your Pull Request updated +- [ ] Existing entry updated in `packages/autoskills/skills-map.ts` +- [ ] A test in `packages/autoskills/tests/detect.test.ts` covers the fixed case +- [ ] Tests pass locally +- [ ] PR title describes the fix: `fix: detect Tailwind from @tailwindcss/vite` -If an maintainer asks you to rebase, they're saying that a lot of code has changed, and that you need to update your branch to easily merge it into the main project. +--- Thank you for contributing! diff --git a/packages/autoskills/skills-map.ts b/packages/autoskills/skills-map.ts index d39c041..c18fbf4 100644 --- a/packages/autoskills/skills-map.ts +++ b/packages/autoskills/skills-map.ts @@ -891,14 +891,6 @@ export const SKILLS_MAP: Technology[] = [ }, skills: [], }, - { - id: "python", - name: "Python", - detect: { - configFiles: ["pyproject.toml", "requirements.txt", "setup.py", "setup.cfg", "Pipfile"], - }, - skills: [], - }, { id: "sorbet", name: "Sorbet", @@ -919,18 +911,6 @@ export const SKILLS_MAP: Technology[] = [ }, skills: [], }, - { - id: "django", - name: "Django", - detect: { - configFiles: ["manage.py"], - configFileContent: { - files: ["pyproject.toml", "requirements.txt", "setup.py", "setup.cfg", "Pipfile"], - patterns: ["django", "Django"], - }, - }, - skills: [], - }, { id: "devise", name: "Devise", @@ -939,17 +919,6 @@ export const SKILLS_MAP: Technology[] = [ }, skills: [], }, - { - id: "fastapi", - name: "FastAPI", - detect: { - configFileContent: { - files: ["pyproject.toml", "requirements.txt", "setup.py", "setup.cfg", "Pipfile"], - patterns: ["fastapi", "FastAPI"], - }, - }, - skills: [], - }, { id: "sidekiq", name: "Sidekiq", @@ -1007,7 +976,7 @@ export const SKILLS_MAP: Technology[] = [ id: "python", name: "Python", detect: { - configFiles: ["pyproject.toml", "requirements.txt", "setup.py", "Pipfile"], + configFiles: ["pyproject.toml", "requirements.txt", "setup.py", "setup.cfg", "Pipfile"], }, skills: ["inferen-sh/skills/python-executor", "wshobson/agents/python-testing-patterns"], }, @@ -1016,7 +985,7 @@ export const SKILLS_MAP: Technology[] = [ name: "FastAPI", detect: { configFileContent: { - files: ["pyproject.toml", "requirements.txt", "setup.py", "Pipfile"], + files: ["pyproject.toml", "requirements.txt", "setup.py", "setup.cfg", "Pipfile"], patterns: ["fastapi", "FastAPI"], }, }, @@ -1026,8 +995,9 @@ export const SKILLS_MAP: Technology[] = [ id: "django", name: "Django", detect: { + configFiles: ["manage.py"], configFileContent: { - files: ["pyproject.toml", "requirements.txt", "setup.py", "Pipfile"], + files: ["pyproject.toml", "requirements.txt", "setup.py", "setup.cfg", "Pipfile"], patterns: ["django", "Django"], }, }, diff --git a/packages/autoskills/tests/skills-map.validation.test.ts b/packages/autoskills/tests/skills-map.validation.test.ts new file mode 100644 index 0000000..e2e4e1f --- /dev/null +++ b/packages/autoskills/tests/skills-map.validation.test.ts @@ -0,0 +1,376 @@ +import { describe, it } from "node:test"; +import { ok, strictEqual, throws } from "node:assert/strict"; +import { SKILLS_MAP, COMBO_SKILLS_MAP } from "../skills-map.ts"; +import type { Technology, ComboSkill, DetectConfig } from "../skills-map.ts"; + +// ── Helpers ─────────────────────────────────────────────────── + +function findDuplicates(values: string[]): string[] { + const counts = new Map(); + for (const value of values) { + counts.set(value, (counts.get(value) ?? 0) + 1); + } + return [...counts.entries()] + .filter(([, count]) => count > 1) + .map(([value]) => value) + .sort(); +} + +function isNonEmptyString(value: unknown): value is string { + return typeof value === "string" && value.trim().length > 0; +} + +function isValidSkillRef(skill: string): boolean { + if (/^https?:\/\/\S+$/u.test(skill)) return true; + const parts = skill.split("/"); + if (parts.length < 3) return false; + const [owner, repo, ...rest] = parts; + return ( + isNonEmptyString(owner) && + isNonEmptyString(repo) && + isNonEmptyString(rest.join("/")) + ); +} + +function hasAtLeastOneDetectSignal(detect: DetectConfig): boolean { + return Boolean( + detect.packages?.length || + detect.packagePatterns?.length || + detect.configFiles?.length || + detect.gems?.length || + detect.configFileContent, + ); +} + +function toConfigBlocks( + value: DetectConfig["configFileContent"], +): Array> { + if (!value) return []; + return Array.isArray(value) ? value : [value]; +} + +// ── Per-entry validators ────────────────────────────────────── + +function validateDetectConfig(tech: Technology): void { + const { detect } = tech; + + ok(detect && typeof detect === "object", `[${tech.id}] detect must exist`); + ok( + hasAtLeastOneDetectSignal(detect), + `[${tech.id}] detect has no signals — define at least one of: packages, configFiles, gems, packagePatterns, or configFileContent`, + ); + + if (detect.packages !== undefined) { + ok(Array.isArray(detect.packages), `[${tech.id}] detect.packages must be an array`); + ok( + detect.packages.length > 0, + `[${tech.id}] detect.packages is empty — add at least one package name (e.g. ["react", "react-dom"])`, + ); + for (const pkg of detect.packages) { + ok( + isNonEmptyString(pkg), + `[${tech.id}] detect.packages contains an empty or non-string value`, + ); + } + } + + if (detect.packagePatterns !== undefined) { + ok( + Array.isArray(detect.packagePatterns), + `[${tech.id}] detect.packagePatterns must be an array`, + ); + ok( + detect.packagePatterns.length > 0, + `[${tech.id}] detect.packagePatterns is empty — add at least one RegExp (e.g. [/^@aws-sdk\\//])`, + ); + for (const pattern of detect.packagePatterns) { + ok( + pattern instanceof RegExp, + `[${tech.id}] detect.packagePatterns contains a non-RegExp value — use /pattern/ syntax, not a string`, + ); + } + } + + if (detect.configFiles !== undefined) { + ok(Array.isArray(detect.configFiles), `[${tech.id}] detect.configFiles must be an array`); + ok( + detect.configFiles.length > 0, + `[${tech.id}] detect.configFiles is empty — add at least one filename (e.g. ["next.config.js", "next.config.ts"])`, + ); + for (const file of detect.configFiles) { + ok( + isNonEmptyString(file), + `[${tech.id}] detect.configFiles contains an empty or non-string value`, + ); + } + } + + if (detect.gems !== undefined) { + ok(Array.isArray(detect.gems), `[${tech.id}] detect.gems must be an array`); + ok( + detect.gems.length > 0, + `[${tech.id}] detect.gems is empty — add at least one gem name (e.g. ["rails", "sinatra"])`, + ); + for (const gem of detect.gems) { + ok(isNonEmptyString(gem), `[${tech.id}] detect.gems contains an empty or non-string value`); + } + } + + for (const block of toConfigBlocks(detect.configFileContent)) { + ok(block && typeof block === "object", `[${tech.id}] configFileContent block must be an object`); + ok(Array.isArray(block.patterns), `[${tech.id}] configFileContent.patterns must be an array`); + ok( + block.patterns.length > 0, + `[${tech.id}] configFileContent.patterns is empty — add at least one string pattern to search for`, + ); + for (const pattern of block.patterns) { + ok( + isNonEmptyString(pattern), + `[${tech.id}] configFileContent.patterns contains an empty or non-string value`, + ); + } + + if (!block.scanGradleLayout && !block.scanDotNetLayout) { + ok( + Array.isArray(block.files), + `[${tech.id}] configFileContent.files must be an array (or set scanGradleLayout: true for Gradle projects, scanDotNetLayout: true for .NET projects)`, + ); + ok( + block.files.length > 0, + `[${tech.id}] configFileContent.files is empty — add at least one filename to search in`, + ); + for (const file of block.files) { + ok( + isNonEmptyString(file), + `[${tech.id}] configFileContent.files contains an empty or non-string value`, + ); + } + } + } +} + +function validateSkillList(ownerId: string, skills: string[]): void { + ok(Array.isArray(skills), `[${ownerId}] skills must be an array`); + + for (const skill of skills) { + ok(isNonEmptyString(skill), `[${ownerId}] skills contains an empty or non-string value`); + ok( + isValidSkillRef(skill), + `[${ownerId}] skill "${skill}" is not a valid reference — use "owner/repo/skill-name" (e.g. "vercel-labs/agent-skills/vercel-react-best-practices") or an https:// URL`, + ); + } + + const duplicateSkills = findDuplicates(skills); + strictEqual( + duplicateSkills.length, + 0, + `[${ownerId}] duplicate skill references: ${duplicateSkills.join(", ")} — each skill can appear only once per entry`, + ); +} + +function validateTechnologyEntry(tech: Technology): void { + ok( + isNonEmptyString(tech.id), + `technology.id is missing or empty — every SKILLS_MAP entry needs a stable unique id (e.g. "react", "nextjs", "prisma")`, + ); + ok( + isNonEmptyString(tech.name), + `[${tech.id ?? "?"}] name is missing or empty — add a user-facing display name (e.g. "React", "Next.js")`, + ); + validateDetectConfig(tech); + validateSkillList(tech.id, tech.skills); +} + +function validateComboEntry(combo: ComboSkill, technologyIds: Set): void { + ok( + isNonEmptyString(combo.id), + `combo.id is missing or empty — every COMBO_SKILLS_MAP entry needs a stable unique id (e.g. "nextjs-supabase")`, + ); + ok( + isNonEmptyString(combo.name), + `[${combo.id ?? "?"}] name is missing or empty — add a user-facing name (e.g. "Next.js + Supabase")`, + ); + + ok(Array.isArray(combo.requires), `[${combo.id}] requires must be an array`); + ok( + combo.requires.length >= 2, + `[${combo.id}] requires must list at least two technology ids — combos need a minimum of two technologies`, + ); + + for (const requiredId of combo.requires) { + ok(isNonEmptyString(requiredId), `[${combo.id}] requires contains an empty or non-string id`); + ok( + technologyIds.has(requiredId), + `[${combo.id}] requires "${requiredId}" which is not in SKILLS_MAP — fix the typo or add the technology first`, + ); + } + + const duplicateRequires = findDuplicates(combo.requires); + strictEqual( + duplicateRequires.length, + 0, + `[${combo.id}] duplicate ids in requires: ${duplicateRequires.join(", ")} — list each technology id once`, + ); + + validateSkillList(combo.id, combo.skills); +} + +// ── SKILLS_MAP ──────────────────────────────────────────────── + +describe("SKILLS_MAP validation", () => { + it("has unique technology ids", () => { + const duplicates = findDuplicates(SKILLS_MAP.map((tech) => tech.id)); + strictEqual(duplicates.length, 0, `duplicate technology ids: ${duplicates.join(", ")}`); + }); + + it("validates every technology entry shape", () => { + for (const tech of SKILLS_MAP) { + validateTechnologyEntry(tech); + } + }); +}); + +// ── COMBO_SKILLS_MAP ────────────────────────────────────────── + +describe("COMBO_SKILLS_MAP validation", () => { + const technologyIds = new Set(SKILLS_MAP.map((tech) => tech.id)); + + it("has unique combo ids", () => { + const duplicates = findDuplicates(COMBO_SKILLS_MAP.map((combo) => combo.id)); + strictEqual(duplicates.length, 0, `duplicate combo ids: ${duplicates.join(", ")}`); + }); + + it("validates every combo entry shape", () => { + for (const combo of COMBO_SKILLS_MAP) { + validateComboEntry(combo, technologyIds); + } + }); +}); + +// ── Feedback quality: error messages must be actionable ─────── +// +// TDD contract: when a contributor submits a malformed entry, +// the assertion message must tell them exactly what to fix. + +describe("validation gives actionable feedback on bad entries", () => { + const VALID_SKILL = "owner/repo/skill-name"; + const VALID_DETECT: DetectConfig = { packages: ["some-package"] }; + + function expectsError(fn: () => void, pattern: RegExp): void { + throws(fn, (err: unknown) => { + const message = err instanceof Error ? err.message : String(err); + ok( + pattern.test(message), + `\nExpected error message matching: ${pattern}\nGot: "${message}"`, + ); + return true; + }); + } + + it("reports missing technology id with a usage hint", () => { + expectsError( + () => + validateTechnologyEntry({ + name: "My Tech", + detect: VALID_DETECT, + skills: [VALID_SKILL], + } as Technology), + /e\.g\.|stable.*id|unique.*id/i, + ); + }); + + it("reports empty detect by listing all available signal types", () => { + expectsError( + () => + validateTechnologyEntry({ + id: "my-tech", + name: "My Tech", + detect: {}, + skills: [VALID_SKILL], + }), + /packages.*configFiles.*gems.*packagePatterns.*configFileContent/, + ); + }); + + it("reports empty detect.packages with an example array", () => { + expectsError( + () => + validateTechnologyEntry({ + id: "my-tech", + name: "My Tech", + detect: { packages: [] }, + skills: [VALID_SKILL], + }), + /empty|at least one/i, + ); + }); + + it("reports non-RegExp in packagePatterns with usage hint", () => { + expectsError( + () => + validateTechnologyEntry({ + id: "my-tech", + name: "My Tech", + detect: { packagePatterns: ["not-a-regexp"] as unknown as RegExp[] }, + skills: [VALID_SKILL], + }), + /RegExp|\/pattern\//, + ); + }); + + it("reports invalid skill reference with expected format and example", () => { + expectsError( + () => + validateTechnologyEntry({ + id: "my-tech", + name: "My Tech", + detect: VALID_DETECT, + skills: ["not-valid"], + }), + /owner\/repo\/skill-name/, + ); + }); + + it("reports duplicate skills within an entry", () => { + expectsError( + () => + validateTechnologyEntry({ + id: "my-tech", + name: "My Tech", + detect: VALID_DETECT, + skills: [VALID_SKILL, VALID_SKILL], + }), + /duplicate/i, + ); + }); + + it("reports unknown combo requires id with actionable message", () => { + const technologyIds = new Set(["react", "nextjs"]); + expectsError( + () => + validateComboEntry( + { + id: "my-combo", + name: "My Combo", + requires: ["react", "nonexistent-tech"], + skills: [VALID_SKILL], + }, + technologyIds, + ), + /"nonexistent-tech".*SKILLS_MAP|SKILLS_MAP.*"nonexistent-tech"/, + ); + }); + + it("reports missing configFileContent.files with scanGradleLayout hint", () => { + expectsError( + () => + validateTechnologyEntry({ + id: "my-tech", + name: "My Tech", + detect: { configFileContent: { patterns: ["some-pattern"] } }, + skills: [VALID_SKILL], + }), + /files.*array|scanGradleLayout/, + ); + }); +});