diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index e772db2..089834a 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -69,6 +69,62 @@ "description": "Audit CLAUDE.md files against Anthropic best practices", "version": "1.1.0", "keywords": ["audit", "claude-md", "best-practices"] + }, + { + "name": "devflow-typescript", + "source": "./plugins/devflow-typescript", + "description": "TypeScript language patterns (type safety, generics, utility types)", + "version": "1.1.0", + "keywords": ["typescript", "types", "generics"] + }, + { + "name": "devflow-react", + "source": "./plugins/devflow-react", + "description": "React framework patterns (hooks, state, composition, performance)", + "version": "1.1.0", + "keywords": ["react", "hooks", "components"] + }, + { + "name": "devflow-accessibility", + "source": "./plugins/devflow-accessibility", + "description": "Web accessibility patterns (WCAG, ARIA, keyboard navigation)", + "version": "1.1.0", + "keywords": ["accessibility", "wcag", "aria", "a11y"] + }, + { + "name": "devflow-frontend-design", + "source": "./plugins/devflow-frontend-design", + "description": "Frontend design patterns (typography, color, spacing, motion)", + "version": "1.1.0", + "keywords": ["design", "typography", "css"] + }, + { + "name": "devflow-go", + "source": "./plugins/devflow-go", + "description": "Go language patterns (error handling, interfaces, concurrency)", + "version": "1.1.0", + "keywords": ["go", "golang", "concurrency"] + }, + { + "name": "devflow-python", + "source": "./plugins/devflow-python", + "description": "Python language patterns (type hints, protocols, data modeling)", + "version": "1.1.0", + "keywords": ["python", "type-hints", "dataclasses"] + }, + { + "name": "devflow-java", + "source": "./plugins/devflow-java", + "description": "Java language patterns (records, sealed classes, composition)", + "version": "1.1.0", + "keywords": ["java", "records", "composition"] + }, + { + "name": "devflow-rust", + "source": "./plugins/devflow-rust", + "description": "Rust language patterns (ownership, error handling, type system)", + "version": "1.1.0", + "keywords": ["rust", "ownership", "type-safety"] } ] } diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c33c0a..a274040 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,40 @@ All notable changes to DevFlow will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added +- **Polyglot language skills** — Go, Java, Python, and Rust skill plugins with comprehensive patterns + - Go: error handling, interfaces, concurrency (errgroup, worker pools, fan-out/fan-in) + - Java: records, sealed classes, streams, composition over inheritance + - Python: type hints, protocols, dataclasses, async patterns + - Rust: ownership, error handling (`thiserror`/`anyhow`), type system, concurrency + - Skills: 26 → 30, Plugins: 9 → 17 +- **Optional plugin architecture** — Language/ecosystem plugins (`optional: true`) not installed by default + - Install selectively: `devflow init --plugin=go --plugin=python` + - Existing skills (typescript, react, accessibility, frontend-design) moved to optional plugins + - `devflow-core-skills` no longer bundles language-specific skills +- **Conditional language reviews** in `/code-review` command + - Spawns language-specific Reviewer agents when matching files are in the diff + - Skill availability check: skips review if optional plugin not installed +- **Dynamic skill loading in Coder agent** — Reads language skills at runtime based on DOMAIN hint instead of static frontmatter dependencies + +### Changed +- **`devflow-core-skills`** no longer includes typescript, react, accessibility, or frontend-design skills (moved to optional plugins) +- **Coder agent** frontmatter trimmed from 14 skills to 6 core skills; language skills loaded dynamically + +### Fixed +- **Deprecated `grpc.WithInsecure()`** in Go concurrency examples → replaced with `grpc.WithTransportCredentials(insecure.NewCredentials())` +- **Deprecated `datetime.utcnow`** in Python dataclass example → replaced with `datetime.now(timezone.utc)` +- **SQL injection** in Python async streaming example → replaced raw query with parameterized query +- **Deprecated ``** in React examples → replaced with `` (React 19+) +- **Deprecated `useRef()`** without argument in React patterns → replaced with `useRef(undefined)` (React 19+) +- **Non-portable `NodeJS.Timeout`** in TypeScript debounce/throttle → replaced with `ReturnType` +- **Unsafe `Function` type** in TypeScript type guard → replaced with `(...args: unknown[]) => unknown` +- **Go test file exclusion** removed from go skill activation (test files are valid Go code) + +--- + ## [1.1.0] - 2026-03-04 ### Added diff --git a/CLAUDE.md b/CLAUDE.md index c443d00..797a28b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,7 +12,7 @@ DevFlow enhances Claude Code with intelligent development workflows. Modificatio ## Architecture Overview -Plugin marketplace with 9 self-contained plugins, each following the Claude plugins format (`.claude-plugin/plugin.json`, `commands/`, `agents/`, `skills/`). +Plugin marketplace with 17 plugins (9 core + 8 optional language/ecosystem), each following the Claude plugins format (`.claude-plugin/plugin.json`, `commands/`, `agents/`, `skills/`). | Plugin | Purpose | Teams Variant | |--------|---------|---------------| @@ -25,6 +25,14 @@ Plugin marketplace with 9 self-contained plugins, each following the Claude plug | `devflow-ambient` | Ambient mode — auto-loads relevant skills based on each prompt | No | | `devflow-core-skills` | Auto-activating quality enforcement | No | | `devflow-audit-claude` | Audit CLAUDE.md files (optional) | No | +| `devflow-typescript` | TypeScript language patterns (optional) | No | +| `devflow-react` | React framework patterns (optional) | No | +| `devflow-accessibility` | Web accessibility patterns (optional) | No | +| `devflow-frontend-design` | Frontend design patterns (optional) | No | +| `devflow-go` | Go language patterns (optional) | No | +| `devflow-python` | Python language patterns (optional) | No | +| `devflow-java` | Java language patterns (optional) | No | +| `devflow-rust` | Rust language patterns (optional) | No | Commands with Teams Variant ship as `{name}.md` (parallel subagents) and `{name}-teams.md` (Agent Teams with debate). The installer copies the chosen variant based on `--teams`/`--no-teams` flag. @@ -36,9 +44,9 @@ Commands with Teams Variant ship as `{name}.md` (parallel subagents) and `{name} ``` devflow/ -├── shared/skills/ # 26 skills (single source of truth) +├── shared/skills/ # 30 skills (single source of truth) ├── shared/agents/ # 10 shared agents (single source of truth) -├── plugins/devflow-*/ # 9 self-contained plugins +├── plugins/devflow-*/ # 17 plugins (9 core + 8 optional language/ecosystem) ├── docs/reference/ # Detailed reference documentation ├── scripts/ # Helper scripts (statusline, docs-helpers) │ └── hooks/ # Working Memory + ambient hooks (stop, session-start, pre-compact, ambient-prompt) diff --git a/README.md b/README.md index 99725df..2ba97d2 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ DevFlow adds structured commands that handle the full lifecycle: specify feature - **Full-lifecycle implementation** — spec, explore, plan, code, validate, refine in one command - **Automatic session memory** — survives restarts, `/clear`, and context compaction - **Parallel debugging** — competing hypotheses investigated simultaneously -- **26 quality skills** — 12 auto-activating, plus specialized review and agent skills +- **30 quality skills** — 8 auto-activating core, 8 optional language/ecosystem, plus specialized review and agent skills ## Quick Start @@ -81,7 +81,7 @@ Creates a PR when complete. Multi-perspective code review with specialized reviewers: - **Core**: Security, Architecture, Performance, Quality -- **Conditional** (activated when relevant): TypeScript, React, Accessibility, Database, Dependencies, Documentation +- **Conditional** (activated when relevant): TypeScript, React, Accessibility, Go, Python, Java, Rust, Database, Dependencies, Documentation - Findings classified as must-fix, should-fix, or nit with severity and confidence levels Provides actionable feedback with specific file locations and suggested fixes. @@ -120,10 +120,27 @@ The `devflow-core-skills` plugin provides quality enforcement skills that activa | `test-driven-development` | Implementing new features (RED-GREEN-REFACTOR) | | `test-patterns` | Writing or modifying tests | | `input-validation` | Creating API endpoints | -| `typescript` | Working in TypeScript codebases | -| `react` | Working with React components | -| `accessibility` | Creating UI components, forms, interactive elements | -| `frontend-design` | Working with CSS, styling, visual design | + +## Language & Ecosystem Plugins + +Optional plugins for language-specific patterns. Install only what you need: + +| Plugin | Skill | Triggers When | +|--------|-------|---------------| +| `devflow-typescript` | `typescript` | Working in TypeScript codebases | +| `devflow-react` | `react` | Working with React components | +| `devflow-accessibility` | `accessibility` | Creating UI components, forms | +| `devflow-frontend-design` | `frontend-design` | Working with CSS, styling | +| `devflow-go` | `go` | Working in Go codebases | +| `devflow-python` | `python` | Working in Python codebases | +| `devflow-java` | `java` | Working in Java codebases | +| `devflow-rust` | `rust` | Working in Rust codebases | + +```bash +# Install specific language plugins +npx devflow-kit init --plugin=typescript,react +npx devflow-kit init --plugin=go +``` ## Requirements diff --git a/docs/reference/skills-architecture.md b/docs/reference/skills-architecture.md index 9be777b..f960609 100644 --- a/docs/reference/skills-architecture.md +++ b/docs/reference/skills-architecture.md @@ -57,6 +57,10 @@ Language and framework patterns. Referenced by agents via frontmatter and condit | `react` | Components, hooks, state management, performance | React codebases | | `accessibility` | Keyboard, ARIA, focus, color contrast | Frontend codebases | | `frontend-design` | Typography, color, spacing, visual design | Frontend codebases | +| `go` | Error handling, interfaces, concurrency, package design | Go codebases | +| `python` | Type hints, protocols, dataclasses, async patterns | Python codebases | +| `java` | Records, sealed classes, composition, modern Java | Java codebases | +| `rust` | Ownership, borrowing, error handling, type-driven design | Rust codebases | ## How Skills Activate @@ -184,6 +188,10 @@ activation: | `typescript` | `**/*.ts`, `**/*.tsx` | `node_modules/**`, `**/*.d.ts` | | `accessibility` | `**/*.tsx`, `**/*.jsx`, `**/*.css` | `node_modules/**` | | `frontend-design` | `**/*.tsx`, `**/*.jsx`, `**/*.css`, `**/*.scss` | `node_modules/**` | +| `go` | `**/*.go` | `vendor/**` | +| `python` | `**/*.py` | `venv/**`, `.venv/**`, `**/__pycache__/**` | +| `java` | `**/*.java` | `**/build/**`, `**/target/**` | +| `rust` | `**/*.rs` | `**/target/**` | | `test-patterns` | `**/*.test.*`, `**/*.spec.*`, `**/test/**` | `node_modules/**` | **Note:** Glob patterns are metadata hints for documentation. Claude Code does not currently read glob patterns to trigger skills — activation happens through agent frontmatter and Reviewer dynamic read (see "How Skills Activate" above). diff --git a/plugins/devflow-accessibility/.claude-plugin/plugin.json b/plugins/devflow-accessibility/.claude-plugin/plugin.json new file mode 100644 index 0000000..b79379c --- /dev/null +++ b/plugins/devflow-accessibility/.claude-plugin/plugin.json @@ -0,0 +1,15 @@ +{ + "name": "devflow-accessibility", + "description": "Web accessibility patterns - WCAG compliance, ARIA roles, keyboard navigation, focus management", + "author": { + "name": "DevFlow Contributors", + "email": "dean@keren.dev" + }, + "version": "1.1.0", + "homepage": "https://github.com/dean0x/devflow", + "repository": "https://github.com/dean0x/devflow", + "license": "MIT", + "keywords": ["accessibility", "wcag", "aria", "a11y"], + "agents": [], + "skills": ["accessibility"] +} diff --git a/plugins/devflow-code-review/.claude-plugin/plugin.json b/plugins/devflow-code-review/.claude-plugin/plugin.json index 5984416..ce6885f 100644 --- a/plugins/devflow-code-review/.claude-plugin/plugin.json +++ b/plugins/devflow-code-review/.claude-plugin/plugin.json @@ -12,7 +12,6 @@ "keywords": ["review", "code-quality", "security", "architecture", "pr-review"], "agents": ["git", "reviewer", "synthesizer"], "skills": [ - "accessibility", "agent-teams", "architecture-patterns", "complexity-patterns", @@ -20,9 +19,7 @@ "database-patterns", "dependencies-patterns", "documentation-patterns", - "frontend-design", "performance-patterns", - "react", "regression-patterns", "review-methodology", "security-patterns", diff --git a/plugins/devflow-code-review/commands/code-review-teams.md b/plugins/devflow-code-review/commands/code-review-teams.md index da1c22e..c8627fa 100644 --- a/plugins/devflow-code-review/commands/code-review-teams.md +++ b/plugins/devflow-code-review/commands/code-review-teams.md @@ -42,10 +42,16 @@ Detect file types in diff to determine conditional reviews: | .tsx/.jsx files | react | | .tsx/.jsx files | accessibility | | .tsx/.jsx/.css/.scss files | frontend-design | +| .go files | go | +| .java files | java | +| .py files | python | +| .rs files | rust | | DB/migration files | database | | Dependency files changed | dependencies | | Docs or significant code | documentation | +**Skill availability check**: Language/ecosystem reviews (typescript, react, accessibility, frontend-design, go, java, python, rust) require their optional skill plugin to be installed. Before adding a conditional perspective, check if `~/.claude/skills/{focus}/SKILL.md` exists (use Glob). If the skill file doesn't exist, **skip that perspective** — the language plugin isn't installed. Non-language reviews (database, dependencies, documentation) use skills bundled with this plugin and are always available. + ### Phase 2: Spawn Review Team Create an agent team for adversarial review. Always include 4 core perspectives; conditionally add more based on Phase 1 analysis. @@ -61,6 +67,10 @@ Create an agent team for adversarial review. Always include 4 core perspectives; - **React**: hooks, state, rendering, composition (if .tsx/.jsx changed) - **Accessibility**: ARIA, keyboard nav, focus management (if .tsx/.jsx changed) - **Frontend Design**: visual consistency, spacing, typography (if .tsx/.jsx/.css changed) +- **Go**: error handling, interfaces, concurrency (if .go changed) +- **Java**: records, sealed classes, composition (if .java changed) +- **Python**: type hints, protocols, data modeling (if .py changed) +- **Rust**: ownership, error handling, type system (if .rs changed) - **Database**: schema, queries, migrations, indexes (if DB files changed) - **Dependencies**: CVEs, versions, licenses, supply chain (if package files changed) - **Documentation**: doc drift, missing docs, stale comments (if docs or significant code changed) @@ -238,7 +248,7 @@ Display results: │ ├─ Architecture Reviewer (teammate) │ ├─ Performance Reviewer (teammate) │ ├─ Quality Reviewer (teammate) -│ └─ [Conditional: TypeScript, React, A11y, Design, DB, Deps, Docs] +│ └─ [Conditional: TypeScript, React, A11y, Design, Go, Java, Python, Rust, DB, Deps, Docs] │ ├─ Phase 3: Debate round │ └─ Reviewers challenge each other (max 2 rounds) diff --git a/plugins/devflow-code-review/commands/code-review.md b/plugins/devflow-code-review/commands/code-review.md index 5f9a826..d2c3df2 100644 --- a/plugins/devflow-code-review/commands/code-review.md +++ b/plugins/devflow-code-review/commands/code-review.md @@ -42,13 +42,19 @@ Detect file types in diff to determine conditional reviews: | .tsx/.jsx files | react | | .tsx/.jsx files | accessibility | | .tsx/.jsx/.css/.scss files | frontend-design | +| .go files | go | +| .java files | java | +| .py files | python | +| .rs files | rust | | DB/migration files | database | | Dependency files changed | dependencies | | Docs or significant code | documentation | +**Skill availability check**: Language/ecosystem reviews (typescript, react, accessibility, frontend-design, go, java, python, rust) require their optional skill plugin to be installed. Before spawning a conditional Reviewer for these focuses, check if `~/.claude/skills/{focus}/SKILL.md` exists (use Glob). If the skill file doesn't exist, **skip that review** — the language plugin isn't installed. Non-language reviews (database, dependencies, documentation) use skills bundled with this plugin and are always available. + ### Phase 2: Run Reviews (Parallel) -Spawn Reviewer agents **in a single message**. Always run 7 core reviews; conditionally add up to 4 more: +Spawn Reviewer agents **in a single message**. Always run 7 core reviews; conditionally add more based on changed file types: | Focus | Always | Pattern Skill | |-------|--------|---------------| @@ -63,6 +69,10 @@ Spawn Reviewer agents **in a single message**. Always run 7 core reviews; condit | react | conditional | react | | accessibility | conditional | accessibility | | frontend-design | conditional | frontend-design | +| go | conditional | go | +| java | conditional | java | +| python | conditional | python | +| rust | conditional | rust | | database | conditional | database-patterns | | dependencies | conditional | dependencies-patterns | | documentation | conditional | documentation-patterns | @@ -123,7 +133,7 @@ Display results from all agents: │ ├─ Reviewer: consistency │ ├─ Reviewer: regression │ ├─ Reviewer: tests -│ └─ Reviewer: [conditional: typescript, react, a11y, design, database, deps, docs] +│ └─ Reviewer: [conditional: typescript, react, a11y, design, go, java, python, rust, database, deps, docs] │ ├─ Phase 3: Synthesis (PARALLEL) │ ├─ Git agent (comment-pr) diff --git a/plugins/devflow-core-skills/.claude-plugin/plugin.json b/plugins/devflow-core-skills/.claude-plugin/plugin.json index 1650cd9..c06feb9 100644 --- a/plugins/devflow-core-skills/.claude-plugin/plugin.json +++ b/plugins/devflow-core-skills/.claude-plugin/plugin.json @@ -12,17 +12,13 @@ "keywords": ["skills", "quality", "patterns", "auto-activate", "enforcement", "foundation"], "agents": [], "skills": [ - "accessibility", "core-patterns", "docs-framework", - "frontend-design", "git-safety", "git-workflow", "github-patterns", "input-validation", - "react", "test-driven-development", - "test-patterns", - "typescript" + "test-patterns" ] } diff --git a/plugins/devflow-frontend-design/.claude-plugin/plugin.json b/plugins/devflow-frontend-design/.claude-plugin/plugin.json new file mode 100644 index 0000000..d189b71 --- /dev/null +++ b/plugins/devflow-frontend-design/.claude-plugin/plugin.json @@ -0,0 +1,15 @@ +{ + "name": "devflow-frontend-design", + "description": "Frontend design patterns - typography, color systems, spacing, motion, responsive design", + "author": { + "name": "DevFlow Contributors", + "email": "dean@keren.dev" + }, + "version": "1.1.0", + "homepage": "https://github.com/dean0x/devflow", + "repository": "https://github.com/dean0x/devflow", + "license": "MIT", + "keywords": ["design", "typography", "color", "css"], + "agents": [], + "skills": ["frontend-design"] +} diff --git a/plugins/devflow-go/.claude-plugin/plugin.json b/plugins/devflow-go/.claude-plugin/plugin.json new file mode 100644 index 0000000..13b8f38 --- /dev/null +++ b/plugins/devflow-go/.claude-plugin/plugin.json @@ -0,0 +1,15 @@ +{ + "name": "devflow-go", + "description": "Go language patterns - error handling, interfaces, concurrency, package design", + "author": { + "name": "DevFlow Contributors", + "email": "dean@keren.dev" + }, + "version": "1.1.0", + "homepage": "https://github.com/dean0x/devflow", + "repository": "https://github.com/dean0x/devflow", + "license": "MIT", + "keywords": ["go", "golang", "concurrency", "errors"], + "agents": [], + "skills": ["go"] +} diff --git a/plugins/devflow-implement/.claude-plugin/plugin.json b/plugins/devflow-implement/.claude-plugin/plugin.json index d3347ca..dcf7f68 100644 --- a/plugins/devflow-implement/.claude-plugin/plugin.json +++ b/plugins/devflow-implement/.claude-plugin/plugin.json @@ -12,9 +12,7 @@ "keywords": ["implementation", "coding", "workflow", "agents", "agentic", "pr"], "agents": ["git", "skimmer", "synthesizer", "coder", "simplifier", "scrutinizer", "shepherd", "validator"], "skills": [ - "accessibility", "agent-teams", - "frontend-design", "implementation-patterns", "self-review" ] diff --git a/plugins/devflow-java/.claude-plugin/plugin.json b/plugins/devflow-java/.claude-plugin/plugin.json new file mode 100644 index 0000000..dbabddc --- /dev/null +++ b/plugins/devflow-java/.claude-plugin/plugin.json @@ -0,0 +1,15 @@ +{ + "name": "devflow-java", + "description": "Java language patterns - records, sealed classes, composition, modern Java features", + "author": { + "name": "DevFlow Contributors", + "email": "dean@keren.dev" + }, + "version": "1.1.0", + "homepage": "https://github.com/dean0x/devflow", + "repository": "https://github.com/dean0x/devflow", + "license": "MIT", + "keywords": ["java", "records", "sealed-classes", "composition"], + "agents": [], + "skills": ["java"] +} diff --git a/plugins/devflow-python/.claude-plugin/plugin.json b/plugins/devflow-python/.claude-plugin/plugin.json new file mode 100644 index 0000000..20a8179 --- /dev/null +++ b/plugins/devflow-python/.claude-plugin/plugin.json @@ -0,0 +1,15 @@ +{ + "name": "devflow-python", + "description": "Python language patterns - type hints, protocols, dataclasses, async programming", + "author": { + "name": "DevFlow Contributors", + "email": "dean@keren.dev" + }, + "version": "1.1.0", + "homepage": "https://github.com/dean0x/devflow", + "repository": "https://github.com/dean0x/devflow", + "license": "MIT", + "keywords": ["python", "type-hints", "dataclasses", "async"], + "agents": [], + "skills": ["python"] +} diff --git a/plugins/devflow-react/.claude-plugin/plugin.json b/plugins/devflow-react/.claude-plugin/plugin.json new file mode 100644 index 0000000..50a37cc --- /dev/null +++ b/plugins/devflow-react/.claude-plugin/plugin.json @@ -0,0 +1,15 @@ +{ + "name": "devflow-react", + "description": "React framework patterns - hooks, state management, composition, performance", + "author": { + "name": "DevFlow Contributors", + "email": "dean@keren.dev" + }, + "version": "1.1.0", + "homepage": "https://github.com/dean0x/devflow", + "repository": "https://github.com/dean0x/devflow", + "license": "MIT", + "keywords": ["react", "hooks", "state", "components"], + "agents": [], + "skills": ["react"] +} diff --git a/plugins/devflow-rust/.claude-plugin/plugin.json b/plugins/devflow-rust/.claude-plugin/plugin.json new file mode 100644 index 0000000..c16821d --- /dev/null +++ b/plugins/devflow-rust/.claude-plugin/plugin.json @@ -0,0 +1,15 @@ +{ + "name": "devflow-rust", + "description": "Rust language patterns - ownership, borrowing, error handling, type-driven design", + "author": { + "name": "DevFlow Contributors", + "email": "dean@keren.dev" + }, + "version": "1.1.0", + "homepage": "https://github.com/dean0x/devflow", + "repository": "https://github.com/dean0x/devflow", + "license": "MIT", + "keywords": ["rust", "ownership", "borrowing", "type-safety"], + "agents": [], + "skills": ["rust"] +} diff --git a/plugins/devflow-typescript/.claude-plugin/plugin.json b/plugins/devflow-typescript/.claude-plugin/plugin.json new file mode 100644 index 0000000..879d08b --- /dev/null +++ b/plugins/devflow-typescript/.claude-plugin/plugin.json @@ -0,0 +1,15 @@ +{ + "name": "devflow-typescript", + "description": "TypeScript language patterns - type safety, generics, utility types, type guards", + "author": { + "name": "DevFlow Contributors", + "email": "dean@keren.dev" + }, + "version": "1.1.0", + "homepage": "https://github.com/dean0x/devflow", + "repository": "https://github.com/dean0x/devflow", + "license": "MIT", + "keywords": ["typescript", "types", "generics", "type-safety"], + "agents": [], + "skills": ["typescript"] +} diff --git a/shared/agents/coder.md b/shared/agents/coder.md index 9cc1380..10c7b0a 100644 --- a/shared/agents/coder.md +++ b/shared/agents/coder.md @@ -2,7 +2,7 @@ name: Coder description: Autonomous task implementation on feature branch. Implements, tests, and commits. model: inherit -skills: core-patterns, git-safety, implementation-patterns, git-workflow, typescript, react, test-patterns, input-validation, accessibility, frontend-design +skills: core-patterns, git-safety, implementation-patterns, git-workflow, test-patterns, input-validation --- # Coder Agent @@ -33,11 +33,16 @@ You receive from orchestrator: 2. **Reference handoff** (if PRIOR_PHASE_SUMMARY provided): Use summary to validate your understanding of prior work, not as the sole source of truth. The actual code is authoritative. -3. **Load domain skills**: Based on DOMAIN hint, apply relevant patterns: - - `backend`: typescript, implementation-patterns, input-validation - - `frontend`: react, typescript, accessibility, frontend-design - - `tests`: test-patterns, typescript - - `fullstack`: all of the above +3. **Load domain skills**: Based on DOMAIN hint and files in scope, dynamically load relevant language/ecosystem skills by reading their SKILL.md. Only load skills that are installed: + - `backend` (TypeScript): Read `~/.claude/skills/typescript/SKILL.md`, `~/.claude/skills/input-validation/SKILL.md` + - `backend` (Go): Read `~/.claude/skills/go/SKILL.md` + - `backend` (Java): Read `~/.claude/skills/java/SKILL.md` + - `backend` (Python): Read `~/.claude/skills/python/SKILL.md` + - `backend` (Rust): Read `~/.claude/skills/rust/SKILL.md` + - `frontend`: Read `~/.claude/skills/react/SKILL.md`, `~/.claude/skills/typescript/SKILL.md`, `~/.claude/skills/accessibility/SKILL.md`, `~/.claude/skills/frontend-design/SKILL.md` + - `tests`: Read `~/.claude/skills/test-patterns/SKILL.md`, `~/.claude/skills/typescript/SKILL.md` + - `fullstack`: Combine backend + frontend skills + - If a Read fails (skill not installed), skip it silently and continue. 4. **Implement the plan**: Work through execution steps systematically, creating and modifying files. Follow existing patterns. Type everything. Use Result types if codebase uses them. diff --git a/shared/agents/reviewer.md b/shared/agents/reviewer.md index 96661fb..f66fce1 100644 --- a/shared/agents/reviewer.md +++ b/shared/agents/reviewer.md @@ -34,6 +34,10 @@ The orchestrator provides: | `react` | `~/.claude/skills/react/SKILL.md` | | `accessibility` | `~/.claude/skills/accessibility/SKILL.md` | | `frontend-design` | `~/.claude/skills/frontend-design/SKILL.md` | +| `go` | `~/.claude/skills/go/SKILL.md` | +| `java` | `~/.claude/skills/java/SKILL.md` | +| `python` | `~/.claude/skills/python/SKILL.md` | +| `rust` | `~/.claude/skills/rust/SKILL.md` | ## Responsibilities @@ -117,3 +121,7 @@ Report format for `{output_path}`: | react | If .tsx/.jsx files changed | | accessibility | If .tsx/.jsx files changed | | frontend-design | If .tsx/.jsx/.css/.scss files changed | +| go | If .go files changed | +| java | If .java files changed | +| python | If .py files changed | +| rust | If .rs files changed | diff --git a/shared/skills/ambient-router/SKILL.md b/shared/skills/ambient-router/SKILL.md index 84dae3d..b3835e5 100644 --- a/shared/skills/ambient-router/SKILL.md +++ b/shared/skills/ambient-router/SKILL.md @@ -54,7 +54,7 @@ Based on classified intent, read the following skills to inform your response. | Intent | Primary Skills | Secondary (if file type matches) | |--------|---------------|----------------------------------| -| **BUILD** | test-driven-development, implementation-patterns | typescript (.ts), react (.tsx/.jsx), frontend-design (CSS/UI), input-validation (forms/API), security-patterns (auth/crypto) | +| **BUILD** | test-driven-development, implementation-patterns | typescript (.ts), react (.tsx/.jsx), go (.go), java (.java), python (.py), rust (.rs), frontend-design (CSS/UI), input-validation (forms/API), security-patterns (auth/crypto) | | **DEBUG** | test-patterns, core-patterns | git-safety (if git operations involved) | | **REVIEW** | self-review, core-patterns | test-patterns | | **PLAN** | implementation-patterns | core-patterns | diff --git a/shared/skills/ambient-router/references/skill-catalog.md b/shared/skills/ambient-router/references/skill-catalog.md index b46ab74..baba7b9 100644 --- a/shared/skills/ambient-router/references/skill-catalog.md +++ b/shared/skills/ambient-router/references/skill-catalog.md @@ -16,6 +16,10 @@ These skills may be loaded during STANDARD-depth ambient routing. | react | React components in scope | `*.tsx`, `*.jsx` | | frontend-design | UI/styling work | `*.css`, `*.scss`, `*.tsx` with styling keywords | | input-validation | Forms, APIs, user input | Files with form/input/validation keywords | +| go | Go files in scope | `*.go` | +| java | Java files in scope | `*.java` | +| python | Python files in scope | `*.py` | +| rust | Rust files in scope | `*.rs` | | security-patterns | Auth, crypto, secrets | Files with auth/token/crypto/password keywords | ### DEBUG Intent diff --git a/shared/skills/go/SKILL.md b/shared/skills/go/SKILL.md new file mode 100644 index 0000000..41c53c9 --- /dev/null +++ b/shared/skills/go/SKILL.md @@ -0,0 +1,187 @@ +--- +name: go +description: This skill should be used when the user works with Go files (.go), asks about "error handling", "interfaces", "goroutines", "channels", "packages", or discusses Go idioms and concurrency. Provides patterns for error handling, interface design, concurrency, and package organization. +user-invocable: false +allowed-tools: Read, Grep, Glob +activation: + file-patterns: + - "**/*.go" + exclude: + - "vendor/**" +--- + +# Go Patterns + +Reference for Go-specific patterns, idioms, and best practices. + +## Iron Law + +> **ERRORS ARE VALUES** +> +> Never ignore errors. `if err != nil` is correctness, not boilerplate. Every error +> return must be checked, wrapped with context, or explicitly documented as intentionally +> ignored with `_ = fn()`. Silent error swallowing causes cascading failures. + +## When This Skill Activates + +- Working with Go codebases +- Designing interfaces and packages +- Implementing concurrent code +- Handling errors +- Structuring Go projects + +--- + +## Error Handling + +### Wrap Errors with Context + +```go +// BAD: return err +// GOOD: return fmt.Errorf("reading config %s: %w", path, err) +``` + +### Sentinel Errors for Expected Conditions + +```go +var ErrNotFound = errors.New("not found") + +func FindUser(id string) (*User, error) { + u, err := db.Get(id) + if err != nil { + return nil, fmt.Errorf("finding user %s: %w", id, err) + } + if u == nil { + return nil, ErrNotFound + } + return u, nil +} +// Caller: if errors.Is(err, ErrNotFound) { ... } +``` + +--- + +## Interface Design + +### Accept Interfaces, Return Structs + +```go +// BAD: func NewService(repo *PostgresRepo) *Service +// GOOD: func NewService(repo Repository) *Service + +type Repository interface { + FindByID(ctx context.Context, id string) (*Entity, error) + Save(ctx context.Context, entity *Entity) error +} +``` + +### Keep Interfaces Small + +```go +// BAD: 10-method interface +// GOOD: single-method interfaces composed as needed +type Reader interface { Read(p []byte) (n int, err error) } +type Writer interface { Write(p []byte) (n int, err error) } +type ReadWriter interface { Reader; Writer } +``` + +--- + +## Concurrency + +### Use Context for Cancellation + +```go +func Process(ctx context.Context, items []Item) error { + g, ctx := errgroup.WithContext(ctx) + for _, item := range items { + g.Go(func() error { + return processItem(ctx, item) + }) + } + return g.Wait() +} +``` + +### Channel Direction + +```go +// Declare direction in function signatures +func producer(ch chan<- int) { ch <- 42 } +func consumer(ch <-chan int) { v := <-ch; _ = v } +``` + +--- + +## Package Design + +### Organize by Domain, Not by Type + +```go +// BAD: models/, controllers/, services/ +// GOOD: user/, order/, payment/ +``` + +### Export Only What's Needed + +```go +// Internal helpers stay unexported (lowercase) +func (s *Service) validate(u *User) error { ... } + +// Public API is exported (uppercase) +func (s *Service) CreateUser(ctx context.Context, req CreateUserReq) (*User, error) { ... } +``` + +--- + +## Zero Values + +```go +// Use zero values as valid defaults +var mu sync.Mutex // Ready to use +var buf bytes.Buffer // Ready to use +var wg sync.WaitGroup // Ready to use + +// Design types with useful zero values +type Config struct { + Timeout time.Duration // zero = no timeout + Retries int // zero = no retries +} +``` + +--- + +## Anti-Patterns + +| Pattern | Bad | Good | +|---------|-----|------| +| Ignoring error | `val, _ := fn()` | `val, err := fn(); if err != nil { ... }` | +| Naked return | `return` in named returns | Explicit `return val, err` | +| init() abuse | Complex `init()` functions | Explicit initialization in `main()` or constructors | +| Interface pollution | Defining interfaces before use | Define interfaces at the consumer site | +| Goroutine leak | `go fn()` without lifecycle | Use context, errgroup, or done channels | + +--- + +## Extended References + +For additional patterns and examples: +- `references/violations.md` - Common Go violations +- `references/patterns.md` - Extended Go patterns +- `references/detection.md` - Detection patterns for Go issues +- `references/concurrency.md` - Advanced concurrency patterns + +--- + +## Checklist + +- [ ] All errors checked or explicitly ignored with `_ =` +- [ ] Errors wrapped with `fmt.Errorf("context: %w", err)` +- [ ] Interfaces defined at consumer, not producer +- [ ] Interfaces kept small (1-3 methods) +- [ ] Context passed as first parameter +- [ ] Goroutines have clear lifecycle/cancellation +- [ ] Channel direction specified in signatures +- [ ] Zero values are useful defaults +- [ ] Packages organized by domain +- [ ] No `init()` with side effects diff --git a/shared/skills/go/references/concurrency.md b/shared/skills/go/references/concurrency.md new file mode 100644 index 0000000..c420057 --- /dev/null +++ b/shared/skills/go/references/concurrency.md @@ -0,0 +1,312 @@ +# Go Concurrency Patterns + +Advanced concurrency patterns for Go. Reference from main SKILL.md. + +## errgroup for Structured Concurrency + +```go +import "golang.org/x/sync/errgroup" + +func FetchAll(ctx context.Context, urls []string) ([]Response, error) { + g, ctx := errgroup.WithContext(ctx) + responses := make([]Response, len(urls)) + + for i, url := range urls { + g.Go(func() error { + resp, err := fetch(ctx, url) + if err != nil { + return fmt.Errorf("fetching %s: %w", url, err) + } + responses[i] = resp // Safe: each goroutine writes to unique index + return nil + }) + } + + if err := g.Wait(); err != nil { + return nil, err + } + return responses, nil +} +``` + +### errgroup with Concurrency Limit + +```go +func ProcessItems(ctx context.Context, items []Item) error { + g, ctx := errgroup.WithContext(ctx) + g.SetLimit(10) // Maximum 10 concurrent goroutines + + for _, item := range items { + g.Go(func() error { + return processItem(ctx, item) + }) + } + + return g.Wait() +} +``` + +--- + +## Worker Pool + +```go +func WorkerPool(ctx context.Context, jobs <-chan Job, workers int) <-chan Result { + results := make(chan Result, workers) + + var wg sync.WaitGroup + for range workers { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case job, ok := <-jobs: + if !ok { + return + } + results <- process(ctx, job) + case <-ctx.Done(): + return + } + } + }() + } + + go func() { + wg.Wait() + close(results) + }() + + return results +} + +// Usage +jobs := make(chan Job, 100) +results := WorkerPool(ctx, jobs, 5) + +// Send jobs +go func() { + defer close(jobs) + for _, j := range allJobs { + jobs <- j + } +}() + +// Collect results +for r := range results { + fmt.Println(r) +} +``` + +--- + +## Fan-Out / Fan-In + +```go +// Fan-out: one source, multiple workers +func fanOut(ctx context.Context, input <-chan int, workers int) []<-chan int { + channels := make([]<-chan int, workers) + for i := range workers { + channels[i] = worker(ctx, input) + } + return channels +} + +func worker(ctx context.Context, input <-chan int) <-chan int { + out := make(chan int) + go func() { + defer close(out) + for n := range input { + select { + case out <- n * n: + case <-ctx.Done(): + return + } + } + }() + return out +} + +// Fan-in: multiple sources, one destination +func fanIn(ctx context.Context, channels ...<-chan int) <-chan int { + merged := make(chan int) + var wg sync.WaitGroup + + for _, ch := range channels { + wg.Add(1) + go func() { + defer wg.Done() + for val := range ch { + select { + case merged <- val: + case <-ctx.Done(): + return + } + } + }() + } + + go func() { + wg.Wait() + close(merged) + }() + + return merged +} +``` + +--- + +## Select with Timeout + +```go +func fetchWithTimeout(ctx context.Context, url string) ([]byte, error) { + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + + ch := make(chan result, 1) + go func() { + data, err := doFetch(ctx, url) + ch <- result{data, err} + }() + + select { + case r := <-ch: + return r.data, r.err + case <-ctx.Done(): + return nil, fmt.Errorf("fetch %s: %w", url, ctx.Err()) + } +} + +type result struct { + data []byte + err error +} +``` + +### Select for Multiple Sources + +```go +func merge(ctx context.Context, primary, fallback <-chan Event) <-chan Event { + out := make(chan Event) + go func() { + defer close(out) + for { + select { + case e, ok := <-primary: + if !ok { + return + } + out <- e + case e, ok := <-fallback: + if !ok { + return + } + out <- e + case <-ctx.Done(): + return + } + } + }() + return out +} +``` + +--- + +## Mutex vs Channels + +### Use Mutex When + +```go +// Protecting shared state with simple read/write +type SafeCounter struct { + mu sync.RWMutex + v map[string]int +} + +func (c *SafeCounter) Inc(key string) { + c.mu.Lock() + defer c.mu.Unlock() + c.v[key]++ +} + +func (c *SafeCounter) Get(key string) int { + c.mu.RLock() + defer c.mu.RUnlock() + return c.v[key] +} +``` + +### Use Channels When + +```go +// Communicating between goroutines / coordinating work +func pipeline(ctx context.Context, input []int) <-chan int { + out := make(chan int) + go func() { + defer close(out) + for _, n := range input { + select { + case out <- transform(n): + case <-ctx.Done(): + return + } + } + }() + return out +} +``` + +### Decision Guide + +| Scenario | Use | +|----------|-----| +| Guarding shared state | `sync.Mutex` or `sync.RWMutex` | +| Passing ownership of data | Channels | +| Coordinating goroutine lifecycle | `context.Context` + channels | +| Waiting for N goroutines | `sync.WaitGroup` or `errgroup` | +| One-time initialization | `sync.Once` | +| Concurrent map access | `sync.Map` (high read, low write) | + +--- + +## sync.Once for Initialization + +```go +type Client struct { + once sync.Once + conn *grpc.ClientConn + err error +} + +func (c *Client) connection() (*grpc.ClientConn, error) { + c.once.Do(func() { + // requires: "google.golang.org/grpc/credentials/insecure" + c.conn, c.err = grpc.Dial("localhost:50051", + grpc.WithTransportCredentials(insecure.NewCredentials())) + }) + return c.conn, c.err +} +``` + +--- + +## Rate Limiting + +```go +func rateLimited(ctx context.Context, items []Item, rps int) error { + limiter := rate.NewLimiter(rate.Limit(rps), 1) + + for _, item := range items { + if err := limiter.Wait(ctx); err != nil { + return fmt.Errorf("rate limiter: %w", err) + } + if err := process(ctx, item); err != nil { + return fmt.Errorf("processing item %s: %w", item.ID, err) + } + } + return nil +} +``` diff --git a/shared/skills/go/references/detection.md b/shared/skills/go/references/detection.md new file mode 100644 index 0000000..6499fd5 --- /dev/null +++ b/shared/skills/go/references/detection.md @@ -0,0 +1,129 @@ +# Go Issue Detection + +Grep and regex patterns for finding common Go issues. Reference from main SKILL.md. + +## Error Handling Detection + +### Unchecked Errors + +```bash +# Find ignored error returns (blank identifier) +grep -rn ', _ := ' --include='*.go' . +grep -rn ', _ = ' --include='*.go' . + +# Find bare error returns without wrapping +grep -rn 'return.*err$' --include='*.go' . | grep -v 'fmt.Errorf' | grep -v ':= err' + +# Find deferred calls that return errors (Close, Flush, Sync) +grep -rn 'defer.*\.Close()' --include='*.go' . +grep -rn 'defer.*\.Flush()' --include='*.go' . +``` + +### Missing Error Context + +```bash +# Find raw error returns (no wrapping) +grep -rn 'return nil, err$' --include='*.go' . +grep -rn 'return err$' --include='*.go' . +``` + +--- + +## Concurrency Detection + +### Goroutine Without Context + +```bash +# Find goroutines that don't reference ctx +grep -rn 'go func()' --include='*.go' . + +# Find goroutines without done/cancel/ctx +grep -B5 -A10 'go func' --include='*.go' . | grep -L 'ctx\|done\|cancel\|errgroup' +``` + +### Potential Goroutine Leaks + +```bash +# Find unbuffered channel creation followed by goroutine +grep -rn 'make(chan ' --include='*.go' . | grep -v ', [0-9]' + +# Find goroutines with infinite loops +grep -A5 'go func' --include='*.go' . | grep 'for {' +``` + +--- + +## Interface Detection + +### Empty Interface Usage + +```bash +# Find empty interface (pre-1.18 style) +grep -rn 'interface{}' --include='*.go' . + +# Find any type (1.18+ style) - may be intentional, review context +grep -rn '\bany\b' --include='*.go' . | grep -v '_test.go' | grep -v 'vendor/' +``` + +### Large Interfaces + +```bash +# Find interface declarations and count methods (interfaces > 5 methods) +grep -B1 -A20 'type.*interface {' --include='*.go' . +``` + +--- + +## Panic Detection + +### Panic in Library Code + +```bash +# Find panic calls outside main/test +grep -rn 'panic(' --include='*.go' . | grep -v '_test.go' | grep -v 'main.go' + +# Find log.Fatal (calls os.Exit) in library code +grep -rn 'log.Fatal' --include='*.go' . | grep -v 'main.go' | grep -v '_test.go' + +# Find os.Exit in library code +grep -rn 'os.Exit' --include='*.go' . | grep -v 'main.go' | grep -v '_test.go' +``` + +--- + +## init() Detection + +```bash +# Find all init functions +grep -rn '^func init()' --include='*.go' . + +# Find init functions with side effects (network, file, global state) +grep -A10 '^func init()' --include='*.go' . | grep -E 'http\.|sql\.|os\.|Open|Dial|Connect' +``` + +--- + +## Mutex and Race Detection + +```bash +# Find mutex passed by value (function params with Mutex, not pointer) +grep -rn 'func.*sync\.Mutex[^*]' --include='*.go' . + +# Find Lock without corresponding Unlock +grep -rn '\.Lock()' --include='*.go' . | grep -v 'defer.*Unlock' + +# Find RLock without RUnlock +grep -rn '\.RLock()' --include='*.go' . | grep -v 'defer.*RUnlock' +``` + +--- + +## Map and Slice Safety + +```bash +# Find uninitialized map declarations (potential nil map panic) +grep -rn 'var.*map\[' --include='*.go' . | grep -v 'make\|:=' + +# Find slice append without pre-allocation hint +grep -rn 'append(' --include='*.go' . | head -20 +``` diff --git a/shared/skills/go/references/patterns.md b/shared/skills/go/references/patterns.md new file mode 100644 index 0000000..91587eb --- /dev/null +++ b/shared/skills/go/references/patterns.md @@ -0,0 +1,232 @@ +# Go Correct Patterns + +Extended correct patterns for Go development. Reference from main SKILL.md. + +## Table-Driven Tests + +```go +func TestAdd(t *testing.T) { + tests := []struct { + name string + a, b int + expected int + }{ + {"positive numbers", 2, 3, 5}, + {"negative numbers", -1, -2, -3}, + {"zero", 0, 0, 0}, + {"mixed signs", -1, 3, 2}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := Add(tt.a, tt.b) + if result != tt.expected { + t.Errorf("Add(%d, %d) = %d, want %d", tt.a, tt.b, result, tt.expected) + } + }) + } +} +``` + +--- + +## Functional Options + +```go +type Server struct { + addr string + timeout time.Duration + logger *slog.Logger +} + +type Option func(*Server) + +func WithAddr(addr string) Option { + return func(s *Server) { s.addr = addr } +} + +func WithTimeout(d time.Duration) Option { + return func(s *Server) { s.timeout = d } +} + +func WithLogger(l *slog.Logger) Option { + return func(s *Server) { s.logger = l } +} + +func NewServer(opts ...Option) *Server { + s := &Server{ + addr: ":8080", // sensible default + timeout: 30 * time.Second, // sensible default + logger: slog.Default(), + } + for _, opt := range opts { + opt(s) + } + return s +} + +// Usage +srv := NewServer( + WithAddr(":9090"), + WithTimeout(60 * time.Second), +) +``` + +--- + +## Custom Error Types + +```go +type ValidationError struct { + Field string + Message string +} + +func (e *ValidationError) Error() string { + return fmt.Sprintf("validation failed on %s: %s", e.Field, e.Message) +} + +type NotFoundError struct { + Resource string + ID string +} + +func (e *NotFoundError) Error() string { + return fmt.Sprintf("%s %s not found", e.Resource, e.ID) +} + +// Usage with errors.As +func handleErr(err error) { + var validErr *ValidationError + if errors.As(err, &validErr) { + log.Printf("bad input: field=%s msg=%s", validErr.Field, validErr.Message) + return + } + var notFound *NotFoundError + if errors.As(err, ¬Found) { + log.Printf("missing: %s/%s", notFound.Resource, notFound.ID) + return + } + log.Printf("unexpected: %v", err) +} +``` + +--- + +## Middleware Pattern + +```go +type Middleware func(http.Handler) http.Handler + +func Logging(logger *slog.Logger) Middleware { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + start := time.Now() + next.ServeHTTP(w, r) + logger.Info("request", + "method", r.Method, + "path", r.URL.Path, + "duration", time.Since(start), + ) + }) + } +} + +func Recovery() Middleware { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer func() { + if err := recover(); err != nil { + w.WriteHeader(http.StatusInternalServerError) + slog.Error("panic recovered", "error", err) + } + }() + next.ServeHTTP(w, r) + }) + } +} + +// Chain composes middleware in order +func Chain(handler http.Handler, middlewares ...Middleware) http.Handler { + for i := len(middlewares) - 1; i >= 0; i-- { + handler = middlewares[i](handler) + } + return handler +} + +// Usage +mux := http.NewServeMux() +mux.HandleFunc("/api/users", handleUsers) +handler := Chain(mux, Recovery(), Logging(logger)) +``` + +--- + +## Graceful Shutdown + +```go +func main() { + srv := &http.Server{Addr: ":8080", Handler: mux} + + go func() { + if err := srv.ListenAndServe(); err != http.ErrServerClosed { + slog.Error("server error", "error", err) + } + }() + + quit := make(chan os.Signal, 1) + signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM) + <-quit + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + if err := srv.Shutdown(ctx); err != nil { + slog.Error("shutdown error", "error", err) + } + slog.Info("server stopped") +} +``` + +--- + +## Constructor Pattern + +```go +// Validate at construction - enforce invariants +func NewUser(name, email string) (*User, error) { + if name == "" { + return nil, &ValidationError{Field: "name", Message: "required"} + } + if !strings.Contains(email, "@") { + return nil, &ValidationError{Field: "email", Message: "invalid format"} + } + return &User{ + ID: uuid.New().String(), + Name: name, + Email: email, + CreatedAt: time.Now(), + }, nil +} +``` + +--- + +## Structured Logging with slog + +```go +func ProcessOrder(ctx context.Context, orderID string) error { + logger := slog.With("order_id", orderID, "trace_id", traceID(ctx)) + + logger.Info("processing order") + + items, err := fetchItems(ctx, orderID) + if err != nil { + logger.Error("failed to fetch items", "error", err) + return fmt.Errorf("fetching items for order %s: %w", orderID, err) + } + + logger.Info("items fetched", "count", len(items)) + return nil +} +``` diff --git a/shared/skills/go/references/violations.md b/shared/skills/go/references/violations.md new file mode 100644 index 0000000..d6aab68 --- /dev/null +++ b/shared/skills/go/references/violations.md @@ -0,0 +1,205 @@ +# Go Violation Examples + +Extended violation patterns for Go reviews. Reference from main SKILL.md. + +## Error Handling Violations + +### Ignored Errors + +```go +// VIOLATION: Silently discarding error +data, _ := json.Marshal(user) +w.Write(data) + +// VIOLATION: Error ignored in deferred call +defer file.Close() // Close() returns error + +// VIOLATION: Swallowing error with log +if err != nil { + log.Println("something failed") // Error details lost + return nil +} +``` + +### Unwrapped Errors + +```go +// VIOLATION: No context on error +func LoadConfig(path string) (*Config, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, err // Caller has no idea what failed + } + var cfg Config + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, err // Which unmarshal? What file? + } + return &cfg, nil +} +``` + +--- + +## Goroutine Leak Violations + +### Fire-and-Forget Goroutine + +```go +// VIOLATION: No way to stop or wait for this goroutine +func StartPoller() { + go func() { + for { + poll() + time.Sleep(10 * time.Second) + } + }() +} + +// VIOLATION: Goroutine blocks forever on channel nobody reads +func process(items []Item) { + ch := make(chan Result) + for _, item := range items { + go func(i Item) { + ch <- compute(i) // Blocks if nobody reads + }(item) + } + // Only reads first result - rest leak + result := <-ch + _ = result +} +``` + +### Missing Context Cancellation + +```go +// VIOLATION: Goroutine ignores context +func Fetch(ctx context.Context, url string) ([]byte, error) { + ch := make(chan []byte, 1) + go func() { + resp, _ := http.Get(url) // Ignores ctx cancellation + body, _ := io.ReadAll(resp.Body) + ch <- body + }() + return <-ch, nil +} +``` + +--- + +## Interface Pollution Violations + +### Premature Interface Definition + +```go +// VIOLATION: Interface defined at producer, not consumer +package user + +type UserStore interface { // Only one implementation exists + Get(id string) (*User, error) + Save(u *User) error + Delete(id string) error + List() ([]*User, error) + Count() (int, error) +} + +type PostgresStore struct{ db *sql.DB } + +func (s *PostgresStore) Get(id string) (*User, error) { ... } +// ... implements all 5 methods +``` + +### God Interface + +```go +// VIOLATION: Interface too large - impossible to mock cleanly +type Service interface { + CreateUser(ctx context.Context, u *User) error + GetUser(ctx context.Context, id string) (*User, error) + UpdateUser(ctx context.Context, u *User) error + DeleteUser(ctx context.Context, id string) error + ListUsers(ctx context.Context) ([]*User, error) + SendEmail(ctx context.Context, to string, body string) error + GenerateReport(ctx context.Context) ([]byte, error) + ProcessPayment(ctx context.Context, amt int) error +} +``` + +--- + +## Naked Return Violations + +```go +// VIOLATION: Naked returns obscure what's being returned +func divide(a, b float64) (result float64, err error) { + if b == 0 { + err = errors.New("division by zero") + return // What is result here? Zero - but not obvious + } + result = a / b + return // Have to trace back to find return values +} +``` + +--- + +## init() Abuse Violations + +```go +// VIOLATION: Side effects in init - runs on import +func init() { + db, err := sql.Open("postgres", os.Getenv("DATABASE_URL")) + if err != nil { + log.Fatal(err) // Crashes on import + } + globalDB = db +} + +// VIOLATION: Registration magic in init +func init() { + http.HandleFunc("/health", healthCheck) // Hidden route registration + prometheus.MustRegister(requestCounter) // Panics if called twice +} +``` + +--- + +## Mutex Violations + +```go +// VIOLATION: Copying mutex (passes by value) +type Cache struct { + mu sync.Mutex + data map[string]string +} + +func process(c Cache) { // c is a COPY - mutex is copied too + c.mu.Lock() + defer c.mu.Unlock() + c.data["key"] = "val" +} + +// VIOLATION: Forgetting to unlock +func (c *Cache) Get(key string) string { + c.mu.Lock() + if val, ok := c.data[key]; ok { + return val // Mutex never unlocked! + } + c.mu.Unlock() + return "" +} +``` + +--- + +## Slice and Map Violations + +```go +// VIOLATION: Nil map write (runtime panic) +var m map[string]int +m["key"] = 1 // panic: assignment to entry in nil map + +// VIOLATION: Sharing slice backing array +func getFirstThree(s []int) []int { + return s[:3] // Shares backing array - mutations leak +} +``` diff --git a/shared/skills/java/SKILL.md b/shared/skills/java/SKILL.md new file mode 100644 index 0000000..a4a63b4 --- /dev/null +++ b/shared/skills/java/SKILL.md @@ -0,0 +1,183 @@ +--- +name: java +description: This skill should be used when the user works with Java files (.java), asks about "records", "sealed classes", "Optional", "streams", "composition over inheritance", or discusses modern Java patterns and API design. Provides patterns for type system usage, error handling, immutability, and concurrency. +user-invocable: false +allowed-tools: Read, Grep, Glob +activation: + file-patterns: + - "**/*.java" + exclude: + - "**/build/**" + - "**/target/**" +--- + +# Java Patterns + +Reference for modern Java patterns, type system, and best practices. + +## Iron Law + +> **FAVOR COMPOSITION OVER INHERITANCE** +> +> Delegation and interfaces over class hierarchies. Inheritance creates tight coupling, +> breaks encapsulation, and makes refactoring dangerous. Use interfaces for polymorphism, +> records for data, and sealed classes for restricted hierarchies. Extend only when the +> "is-a" relationship is genuinely invariant. + +## When This Skill Activates + +- Working with Java codebases +- Designing APIs with modern Java features +- Using records, sealed classes, Optional +- Implementing concurrent code +- Structuring Java packages + +--- + +## Type System (Modern Java) + +### Records for Data + +```java +// BAD: Mutable POJO with getters/setters +// GOOD: Immutable record +public record User(String name, String email, Instant createdAt) { + public User { + Objects.requireNonNull(name, "name must not be null"); + Objects.requireNonNull(email, "email must not be null"); + } +} +``` + +### Sealed Classes for Restricted Hierarchies + +```java +public sealed interface Result permits Success, Failure { + record Success(T value) implements Result {} + record Failure(String error) implements Result {} +} + +// Exhaustive pattern matching (Java 21+) +switch (result) { + case Success s -> handleSuccess(s.value()); + case Failure f -> handleError(f.error()); +} +``` + +### Optional for Absent Values + +```java +// BAD: return null; +// GOOD: +public Optional findById(String id) { + return Optional.ofNullable(userMap.get(id)); +} + +// BAD: if (optional.isPresent()) optional.get() +// GOOD: +optional.map(User::name).orElse("Anonymous"); +``` + +--- + +## Error Handling + +### Custom Exceptions with Context + +```java +public class EntityNotFoundException extends RuntimeException { + private final String entityType; + private final String entityId; + + public EntityNotFoundException(String entityType, String entityId) { + super("%s with id %s not found".formatted(entityType, entityId)); + this.entityType = entityType; + this.entityId = entityId; + } +} +``` + +### Try-with-Resources + +```java +// Always use try-with-resources for AutoCloseable +try (var conn = dataSource.getConnection(); + var stmt = conn.prepareStatement(sql)) { + stmt.setString(1, id); + return mapResult(stmt.executeQuery()); +} +``` + +--- + +## Immutability + +```java +// Prefer unmodifiable collections +List names = List.of("Alice", "Bob"); +Map scores = Map.of("Alice", 100, "Bob", 95); + +// Defensive copies in constructors +public final class Team { + private final List members; + public Team(List members) { + this.members = List.copyOf(members); + } + public List members() { return members; } +} +``` + +--- + +## Composition Over Inheritance + +```java +// BAD: class UserService extends BaseService extends AbstractDAO +// GOOD: compose via constructor injection +public class UserService { + private final UserRepository repository; + private final EventPublisher events; + + public UserService(UserRepository repository, EventPublisher events) { + this.repository = repository; + this.events = events; + } +} +``` + +--- + +## Anti-Patterns + +| Pattern | Bad | Good | +|---------|-----|------| +| Returning null | `return null` | `return Optional.empty()` | +| Checked exception abuse | `throws Exception` | Specific exceptions or unchecked | +| Raw types | `List list` | `List list` | +| Deep inheritance | 4+ level hierarchy | Interfaces + composition | +| Mutable data objects | `setName()/getName()` | Records or immutable classes | + +--- + +## Extended References + +For additional patterns and examples: +- `references/violations.md` - Common Java violations +- `references/patterns.md` - Extended Java patterns +- `references/detection.md` - Detection patterns for Java issues +- `references/modern-java.md` - Modern Java features (17-21+) + +--- + +## Checklist + +- [ ] Records for pure data types +- [ ] Sealed interfaces for type hierarchies +- [ ] Optional instead of null returns +- [ ] Composition over inheritance +- [ ] Try-with-resources for all AutoCloseable +- [ ] Immutable collections (List.of, Map.of) +- [ ] No raw generic types +- [ ] Custom exceptions with context +- [ ] Streams for collection transforms +- [ ] Constructor injection for dependencies diff --git a/shared/skills/java/references/detection.md b/shared/skills/java/references/detection.md new file mode 100644 index 0000000..c272d73 --- /dev/null +++ b/shared/skills/java/references/detection.md @@ -0,0 +1,120 @@ +# Detection Patterns for Java Issues + +Grep and regex patterns for automated detection of Java violations. Reference from main SKILL.md. + +--- + +## Null Returns + +```bash +# Methods returning null instead of Optional +rg 'return\s+null\s*;' --type java + +# Nullable method parameters without validation +rg 'public\s+\w+\s+\w+\((?!.*@NonNull).*\)' --type java +``` + +--- + +## Raw Types + +```bash +# Raw List, Map, Set, Collection usage +rg '\b(List|Map|Set|Collection|Iterator|Iterable)\b(?!\s*<)' --type java + +# Raw Comparable implementation +rg 'implements\s+Comparable\b(?!\s*<)' --type java + +# Unsafe casts from Object +rg '\(\s*(String|Integer|Long|Double)\s*\)\s*\w+\.get' --type java +``` + +--- + +## Deep Inheritance + +```bash +# Classes extending non-Object/non-interface classes (potential deep hierarchy) +rg 'class\s+\w+\s+extends\s+(?!Object\b)\w+' --type java + +# Abstract class chains (look for multiple levels) +rg 'abstract\s+class\s+\w+\s+extends\s+Abstract' --type java + +# Count inheritance depth per file +rg 'extends\s+\w+' --type java --count +``` + +--- + +## Missing Try-with-Resources + +```bash +# Manual close() calls (should use try-with-resources) +rg '\.close\(\)\s*;' --type java + +# InputStream/OutputStream/Connection created outside try-with-resources +rg 'new\s+(FileInputStream|FileOutputStream|BufferedReader|BufferedWriter|Socket)\(' --type java + +# getConnection() without try-with-resources context +rg '\.getConnection\(\)' --type java +``` + +--- + +## Checked Exception Abuse + +```bash +# Broad throws declarations +rg 'throws\s+Exception\b' --type java + +# Empty catch blocks +rg -U 'catch\s*\([^)]+\)\s*\{\s*\}' --type java --multiline + +# Catch-and-ignore (catch with only a comment or log) +rg -U 'catch\s*\([^)]+\)\s*\{\s*(//|/\*|logger\.(debug|trace))' --type java --multiline +``` + +--- + +## Mutable Data Objects + +```bash +# Setter methods (JavaBean anti-pattern for data objects) +rg 'public\s+void\s+set[A-Z]\w*\(' --type java + +# Mutable collections returned from getters +rg 'return\s+(this\.)?(list|map|set|collection)\s*;' --type java -i + +# Missing defensive copies in constructors +rg 'this\.\w+\s*=\s*\w+\s*;' --type java +``` + +--- + +## Concurrency Issues + +```bash +# HashMap in concurrent context (should be ConcurrentHashMap) +rg 'new\s+HashMap\b' --type java + +# Missing volatile on shared fields +rg 'private\s+(?!volatile\s)(?!final\s)\w+\s+\w+\s*=' --type java + +# Synchronized on non-private lock object +rg 'synchronized\s*\(\s*this\s*\)' --type java +``` + +--- + +## Streams Anti-Patterns + +```bash +# Stream.forEach with side effects (should use for-loop or collect) +rg '\.stream\(\).*\.forEach\(' --type java + +# Nested streams (performance concern) +rg '\.stream\(\).*\.stream\(\)' --type java + +# collect(Collectors.toList()) instead of .toList() (Java 16+) +rg 'collect\(Collectors\.toList\(\)\)' --type java +``` diff --git a/shared/skills/java/references/modern-java.md b/shared/skills/java/references/modern-java.md new file mode 100644 index 0000000..58e395b --- /dev/null +++ b/shared/skills/java/references/modern-java.md @@ -0,0 +1,270 @@ +# Modern Java Features (17-21+) + +Deep-dive on modern Java language features. Reference from main SKILL.md. + +--- + +## Records (Java 16+) + +### Basic Record + +```java +// Immutable data carrier with auto-generated equals, hashCode, toString +public record Point(double x, double y) {} + +// Usage +var p = new Point(3.0, 4.0); +double x = p.x(); // Accessor method, not getX() +``` + +### Compact Constructor (Validation) + +```java +public record Email(String value) { + public Email { + Objects.requireNonNull(value, "email must not be null"); + if (!value.contains("@")) { + throw new IllegalArgumentException("Invalid email: " + value); + } + value = value.toLowerCase().strip(); // Reassign before final assignment + } +} +``` + +### Record with Custom Methods + +```java +public record Range(int start, int end) { + public Range { + if (start > end) throw new IllegalArgumentException("start must be <= end"); + } + + public int length() { return end - start; } + public boolean contains(int value) { return value >= start && value <= end; } + public Range overlap(Range other) { + int newStart = Math.max(start, other.start); + int newEnd = Math.min(end, other.end); + return newStart <= newEnd ? new Range(newStart, newEnd) : null; + } +} +``` + +### Records as Local Classes + +```java +public List processOrders(List orders) { + // Local record for intermediate computation + record OrderTotal(String orderId, Money total) {} + + return orders.stream() + .map(o -> new OrderTotal(o.id(), o.calculateTotal())) + .filter(ot -> ot.total().isGreaterThan(Money.of(100))) + .map(OrderTotal::orderId) + .toList(); +} +``` + +--- + +## Sealed Classes (Java 17+) + +### Sealed Interface + +```java +public sealed interface Shape permits Circle, Rectangle, Triangle { + double area(); +} + +public record Circle(double radius) implements Shape { + public double area() { return Math.PI * radius * radius; } +} + +public record Rectangle(double width, double height) implements Shape { + public double area() { return width * height; } +} + +public record Triangle(double base, double height) implements Shape { + public double area() { return 0.5 * base * height; } +} +``` + +### Sealed Class with Abstract Methods + +```java +public sealed abstract class Payment permits CreditCard, BankTransfer, Crypto { + abstract Money amount(); + abstract String reference(); +} + +public final class CreditCard extends Payment { + private final String cardLast4; + private final Money amount; + // ... +} + +public final class BankTransfer extends Payment { /* ... */ } +public final class Crypto extends Payment { /* ... */ } +``` + +--- + +## Pattern Matching (Java 21+) + +### Switch with Patterns + +```java +// Exhaustive pattern matching on sealed types +public String describe(Shape shape) { + return switch (shape) { + case Circle c -> "Circle with radius %.2f".formatted(c.radius()); + case Rectangle r -> "Rectangle %s x %s".formatted(r.width(), r.height()); + case Triangle t -> "Triangle with base %.2f".formatted(t.base()); + }; +} +``` + +### Guarded Patterns + +```java +public String classifyTemperature(Object obj) { + return switch (obj) { + case Integer i when i < 0 -> "Freezing"; + case Integer i when i < 15 -> "Cold"; + case Integer i when i < 25 -> "Comfortable"; + case Integer i -> "Hot"; + case Double d when d < 0.0 -> "Freezing"; + case Double d -> "Warm-ish (%.1f)".formatted(d); + case String s -> "Not a temperature: " + s; + case null -> "No reading"; + default -> "Unknown type"; + }; +} +``` + +### Record Patterns (Destructuring) + +```java +// Nested destructuring +record Address(String city, String country) {} +record Person(String name, Address address) {} + +public String greet(Object obj) { + return switch (obj) { + case Person(var name, Address(var city, _)) -> + "Hello %s from %s".formatted(name, city); + default -> "Hello stranger"; + }; +} +``` + +--- + +## Text Blocks (Java 15+) + +```java +// Multi-line strings with proper indentation +String json = """ + { + "name": "%s", + "email": "%s", + "active": true + } + """.formatted(user.name(), user.email()); + +String sql = """ + SELECT u.id, u.name, u.email + FROM users u + JOIN orders o ON o.user_id = u.id + WHERE u.active = true + AND o.created_at > ? + ORDER BY u.name + """; +``` + +--- + +## Virtual Threads (Java 21+) + +### Basic Virtual Thread + +```java +// Lightweight thread - does not pin platform thread during I/O +Thread.startVirtualThread(() -> { + var result = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); + process(result.body()); +}); +``` + +### Structured Concurrency (Preview) + +```java +// All subtasks complete or cancel together +try (var scope = new StructuredTaskScope.ShutdownOnFailure()) { + Subtask userTask = scope.fork(() -> fetchUser(userId)); + Subtask> ordersTask = scope.fork(() -> fetchOrders(userId)); + + scope.join().throwIfFailed(); + + return new UserDashboard(userTask.get(), ordersTask.get()); +} +``` + +### ExecutorService with Virtual Threads + +```java +// Process thousands of concurrent I/O tasks +try (var executor = Executors.newVirtualThreadPerTaskExecutor()) { + List> futures = urls.stream() + .map(url -> executor.submit(() -> httpClient.send( + HttpRequest.newBuilder(URI.create(url)).build(), + HttpResponse.BodyHandlers.ofString() + ))) + .toList(); + + List responses = futures.stream() + .map(f -> { + try { return f.get(); } + catch (Exception e) { throw new RuntimeException(e); } + }) + .toList(); +} +``` + +--- + +## Other Modern Features + +### Enhanced instanceof (Java 16+) + +```java +// Pattern variable binding eliminates explicit cast +if (obj instanceof String s && s.length() > 5) { + System.out.println(s.toUpperCase()); +} + +// Works with negation +if (!(obj instanceof String s)) { + throw new IllegalArgumentException("Expected String"); +} +// s is in scope here +process(s); +``` + +### Helpful NullPointerExceptions (Java 14+) + +```java +// JVM now tells you exactly which reference was null: +// java.lang.NullPointerException: Cannot invoke "String.length()" +// because the return value of "User.name()" is null +// Enable with: -XX:+ShowCodeDetailsInExceptionMessages (default since Java 17) +``` + +### Stream Gatherers (Java 22+ Preview) + +```java +// Custom intermediate stream operations +var windowedAverages = temperatures.stream() + .gather(Gatherers.windowSliding(5)) + .map(window -> window.stream().mapToDouble(d -> d).average().orElse(0)) + .toList(); +``` diff --git a/shared/skills/java/references/patterns.md b/shared/skills/java/references/patterns.md new file mode 100644 index 0000000..049bec6 --- /dev/null +++ b/shared/skills/java/references/patterns.md @@ -0,0 +1,235 @@ +# Extended Java Patterns + +Correct patterns for Java development. Reference from main SKILL.md. + +--- + +## Builder Pattern (Type-Safe) + +```java +// Compile-time enforcement of required fields +public record EmailMessage(String to, String subject, String body, List cc) { + + public static Builder builder() { return new Builder(); } + + public static class Builder { + private String to; + private String subject; + private String body; + private List cc = List.of(); + + public Builder to(String to) { this.to = Objects.requireNonNull(to); return this; } + public Builder subject(String subject) { this.subject = Objects.requireNonNull(subject); return this; } + public Builder body(String body) { this.body = Objects.requireNonNull(body); return this; } + public Builder cc(List cc) { this.cc = List.copyOf(cc); return this; } + + public EmailMessage build() { + Objects.requireNonNull(to, "to is required"); + Objects.requireNonNull(subject, "subject is required"); + Objects.requireNonNull(body, "body is required"); + return new EmailMessage(to, subject, body, cc); + } + } +} + +// Usage +var email = EmailMessage.builder() + .to("user@example.com") + .subject("Welcome") + .body("Hello!") + .build(); +``` + +--- + +## Strategy via Interfaces + +```java +// Define strategy as functional interface +@FunctionalInterface +public interface PricingStrategy { + Money calculatePrice(Order order); +} + +// Implementations as lambdas or classes +PricingStrategy standard = order -> order.subtotal(); +PricingStrategy discounted = order -> order.subtotal().multiply(0.9); +PricingStrategy tiered = order -> { + if (order.itemCount() > 10) return order.subtotal().multiply(0.8); + if (order.itemCount() > 5) return order.subtotal().multiply(0.9); + return order.subtotal(); +}; + +// Inject strategy +public class OrderService { + private final PricingStrategy pricing; + + public OrderService(PricingStrategy pricing) { + this.pricing = pricing; + } + + public Money calculateTotal(Order order) { + return pricing.calculatePrice(order); + } +} +``` + +--- + +## Repository Pattern + +```java +public interface UserRepository { + Optional findById(String id); + List findByEmail(String email); + User save(User user); + void deleteById(String id); + boolean existsById(String id); +} + +// Implementation with constructor injection +public class JpaUserRepository implements UserRepository { + private final EntityManager em; + + public JpaUserRepository(EntityManager em) { + this.em = em; + } + + @Override + public Optional findById(String id) { + return Optional.ofNullable(em.find(User.class, id)); + } + + @Override + public User save(User user) { + return em.merge(user); + } +} +``` + +--- + +## Value Objects + +```java +// Immutable value object with validation +public record Money(BigDecimal amount, Currency currency) { + public Money { + Objects.requireNonNull(amount, "amount must not be null"); + Objects.requireNonNull(currency, "currency must not be null"); + if (amount.scale() > currency.getDefaultFractionDigits()) { + throw new IllegalArgumentException("Scale exceeds currency precision"); + } + } + + public Money add(Money other) { + requireSameCurrency(other); + return new Money(amount.add(other.amount), currency); + } + + public Money multiply(double factor) { + return new Money( + amount.multiply(BigDecimal.valueOf(factor)) + .setScale(currency.getDefaultFractionDigits(), RoundingMode.HALF_UP), + currency + ); + } + + private void requireSameCurrency(Money other) { + if (!currency.equals(other.currency)) { + throw new IllegalArgumentException( + "Cannot combine %s and %s".formatted(currency, other.currency) + ); + } + } +} +``` + +--- + +## Event-Driven Pattern + +```java +// Domain event as sealed interface +public sealed interface OrderEvent permits OrderCreated, OrderShipped, OrderCancelled { + String orderId(); + Instant occurredAt(); +} + +public record OrderCreated(String orderId, Instant occurredAt, List items) + implements OrderEvent {} + +public record OrderShipped(String orderId, Instant occurredAt, String trackingNumber) + implements OrderEvent {} + +public record OrderCancelled(String orderId, Instant occurredAt, String reason) + implements OrderEvent {} + +// Event handler with exhaustive matching +public class OrderEventHandler { + public void handle(OrderEvent event) { + switch (event) { + case OrderCreated e -> notifyWarehouse(e.items()); + case OrderShipped e -> sendTrackingEmail(e.trackingNumber()); + case OrderCancelled e -> processRefund(e.orderId(), e.reason()); + } + } +} +``` + +--- + +## Stream Pipelines + +```java +// Declarative collection processing +public List getActiveUserEmails(List users) { + return users.stream() + .filter(User::isActive) + .map(User::email) + .filter(email -> email.contains("@")) + .sorted() + .toList(); // Unmodifiable list (Java 16+) +} + +// Grouping and aggregation +public Map countByDepartment(List employees) { + return employees.stream() + .collect(Collectors.groupingBy(Employee::department, Collectors.counting())); +} + +// Reducing to summary +public record OrderSummary(int count, Money total) {} + +public OrderSummary summarize(List orders) { + return orders.stream() + .reduce( + new OrderSummary(0, Money.ZERO), + (summary, order) -> new OrderSummary( + summary.count() + 1, + summary.total().add(order.total()) + ), + (a, b) -> new OrderSummary(a.count() + b.count(), a.total().add(b.total())) + ); +} +``` + +--- + +## Dependency Injection (Manual) + +```java +// Composition root wires everything together +public class Application { + public static void main(String[] args) { + var config = Config.load(); + var dataSource = createDataSource(config); + var userRepo = new JpaUserRepository(dataSource); + var eventBus = new InMemoryEventBus(); + var userService = new UserService(userRepo, eventBus); + var userController = new UserController(userService); + + startServer(config.port(), userController); + } +} +``` diff --git a/shared/skills/java/references/violations.md b/shared/skills/java/references/violations.md new file mode 100644 index 0000000..1779523 --- /dev/null +++ b/shared/skills/java/references/violations.md @@ -0,0 +1,213 @@ +# Common Java Violations + +Extended violation patterns for Java reviews. Reference from main SKILL.md. + +--- + +## Null Returns + +### Returning null Instead of Optional + +```java +// VIOLATION: Null return forces callers to null-check +public User findById(String id) { + return userMap.get(id); // Returns null if absent +} + +// Callers must remember to check: +User user = findById(id); +user.getName(); // NullPointerException if not found +``` + +### Nullable Collections + +```java +// VIOLATION: Returning null instead of empty collection +public List getOrders(String userId) { + List orders = orderMap.get(userId); + return orders; // null if user has no orders +} + +// Callers iterate without checking: +for (Order o : getOrders(userId)) { // NullPointerException + process(o); +} +``` + +--- + +## Raw Types + +### Unparameterized Generics + +```java +// VIOLATION: Raw List - no type safety +List users = new ArrayList(); +users.add("not a user"); // No compile error +users.add(42); // No compile error +User first = (User) users.get(0); // ClassCastException at runtime + +// VIOLATION: Raw Map +Map cache = new HashMap(); +cache.put(123, "value"); +String val = (String) cache.get(123); // Unsafe cast +``` + +### Raw Type in Method Signatures + +```java +// VIOLATION: Raw Comparable +public class Price implements Comparable { + public int compareTo(Object other) { + return Double.compare(this.amount, ((Price) other).amount); // Unsafe cast + } +} + +// VIOLATION: Raw Iterator +Iterator it = collection.iterator(); +while (it.hasNext()) { + String s = (String) it.next(); // Unsafe cast +} +``` + +--- + +## Checked Exception Abuse + +### Broad Throws Declarations + +```java +// VIOLATION: throws Exception - hides what can actually go wrong +public User createUser(String name) throws Exception { + // Callers must catch Exception, can't handle specific failures +} + +// VIOLATION: Wrapping everything in checked exceptions +public void process(String data) throws ProcessingException { + try { + Integer.parseInt(data); + } catch (NumberFormatException e) { + throw new ProcessingException("Failed", e); // Unnecessary wrapping + } +} +``` + +### Swallowed Exceptions + +```java +// VIOLATION: Empty catch block +try { + connection.close(); +} catch (SQLException e) { + // silently ignored - resource leak hidden +} + +// VIOLATION: Catching and logging only +try { + processPayment(order); +} catch (PaymentException e) { + logger.error("Payment failed", e); + // Continues as if nothing happened - order in inconsistent state +} +``` + +--- + +## Mutable Data Objects + +### JavaBean-Style Mutability + +```java +// VIOLATION: Mutable DTO with setters +public class UserDTO { + private String name; + private String email; + + public void setName(String name) { this.name = name; } + public void setEmail(String email) { this.email = email; } + public String getName() { return name; } + public String getEmail() { return email; } +} + +// Anyone can modify at any time: +dto.setName("changed"); // No control over when/where mutation happens +``` + +### Exposing Internal Mutable State + +```java +// VIOLATION: Getter returns mutable internal list +public class Team { + private List members = new ArrayList<>(); + + public List getMembers() { + return members; // Caller can modify internal state + } +} + +// External code breaks encapsulation: +team.getMembers().clear(); // Empties the team's internal list +``` + +--- + +## Deep Inheritance + +### Fragile Base Class + +```java +// VIOLATION: Deep hierarchy creates tight coupling +public abstract class AbstractEntity { ... } +public abstract class AbstractAuditableEntity extends AbstractEntity { ... } +public abstract class AbstractVersionedEntity extends AbstractAuditableEntity { ... } +public class User extends AbstractVersionedEntity { ... } + +// Changing any base class ripples through all descendants +// Testing requires understanding 4 levels of behavior +``` + +### Inheritance for Code Reuse + +```java +// VIOLATION: Extending just to reuse utility methods +public class OrderService extends BaseService { + // Only extends BaseService to get logAndAudit() method + public void processOrder(Order order) { + logAndAudit("processing", order.getId()); // Inherited utility + } +} +// Should be: inject a LogAuditService instead +``` + +--- + +## Concurrency Violations + +### Shared Mutable State Without Synchronization + +```java +// VIOLATION: HashMap accessed from multiple threads +private Map sessions = new HashMap<>(); + +public void addSession(String id, Session s) { + sessions.put(id, s); // Not thread-safe +} +``` + +### Double-Checked Locking Done Wrong + +```java +// VIOLATION: Missing volatile on lazily-initialized field +private ExpensiveObject instance; + +public ExpensiveObject getInstance() { + if (instance == null) { + synchronized (this) { + if (instance == null) { + instance = new ExpensiveObject(); // Partially constructed object visible + } + } + } + return instance; +} +``` diff --git a/shared/skills/python/SKILL.md b/shared/skills/python/SKILL.md new file mode 100644 index 0000000..3e3183b --- /dev/null +++ b/shared/skills/python/SKILL.md @@ -0,0 +1,188 @@ +--- +name: python +description: This skill should be used when the user works with Python files (.py), asks about "type hints", "protocols", "dataclasses", "async/await", "decorators", or discusses Pythonic patterns and data modeling. Provides patterns for type safety, error handling, data modeling, and async programming. +user-invocable: false +allowed-tools: Read, Grep, Glob +activation: + file-patterns: + - "**/*.py" + exclude: + - "venv/**" + - ".venv/**" + - "**/__pycache__/**" +--- + +# Python Patterns + +Reference for Python-specific patterns, type safety, and idioms. + +## Iron Law + +> **EXPLICIT IS BETTER THAN IMPLICIT** +> +> Type-hint every function signature. Name every exception. Use dataclasses over raw dicts. +> Python's flexibility is a strength only when boundaries are explicit. Implicit behavior +> causes debugging nightmares and makes codebases hostile to newcomers. + +## When This Skill Activates + +- Working with Python codebases +- Designing typed APIs with type hints +- Modeling data with dataclasses or Pydantic +- Implementing async code +- Structuring Python packages + +--- + +## Type Safety + +### Type Hint Everything + +```python +# BAD: def process(data, config): ... +# GOOD: +def process(data: list[dict[str, Any]], config: AppConfig) -> ProcessResult: + ... +``` + +### Use Protocols for Structural Typing + +```python +from typing import Protocol + +class Repository(Protocol): + def find_by_id(self, id: str) -> User | None: ... + def save(self, entity: User) -> User: ... + +# Any class with these methods satisfies Repository — no inheritance needed +``` + +### Strict Optional Handling + +```python +# BAD: def get_name(user): return user.name +# GOOD: +def get_name(user: User | None) -> str: + if user is None: + return "Anonymous" + return user.name +``` + +--- + +## Error Handling + +### Custom Exception Hierarchies + +```python +class AppError(Exception): + """Base application error.""" + +class NotFoundError(AppError): + def __init__(self, entity: str, id: str) -> None: + super().__init__(f"{entity} {id} not found") + self.entity = entity + self.id = id + +class ValidationError(AppError): + def __init__(self, field: str, message: str) -> None: + super().__init__(f"Validation failed for {field}: {message}") +``` + +### Context Managers for Resources + +```python +from contextlib import contextmanager + +@contextmanager +def database_transaction(conn: Connection): + try: + yield conn + conn.commit() + except Exception: + conn.rollback() + raise +``` + +--- + +## Data Modeling + +### Dataclasses Over Raw Dicts + +```python +# BAD: user = {"name": "Alice", "email": "alice@example.com"} +# GOOD: +@dataclass(frozen=True) +class User: + name: str + email: str + created_at: datetime = field(default_factory=lambda: datetime.now(timezone.utc)) +``` + +### Pydantic for Validation at Boundaries + +```python +from pydantic import BaseModel, EmailStr + +class CreateUserRequest(BaseModel): + name: str + email: EmailStr + age: int = Field(ge=0, le=150) +``` + +--- + +## Pythonic Patterns + +```python +# Comprehensions over loops for transforms +names = [user.name for user in users if user.active] + +# Enumerate over manual index tracking +for i, item in enumerate(items): + process(i, item) + +# EAFP: Easier to Ask Forgiveness than Permission +try: + value = mapping[key] +except KeyError: + value = default +``` + +--- + +## Anti-Patterns + +| Pattern | Bad | Good | +|---------|-----|------| +| Bare except | `except:` | `except (ValueError, KeyError):` | +| Mutable default | `def fn(items=[])` | `def fn(items: list | None = None)` | +| No type hints | `def process(data)` | `def process(data: DataFrame) -> Result` | +| String typing | `x: "MyClass"` (without reason) | `from __future__ import annotations` | +| God class | `class App` with 50 methods | Compose smaller focused classes | + +--- + +## Extended References + +For additional patterns and examples: +- `references/violations.md` - Common Python violations +- `references/patterns.md` - Extended Python patterns +- `references/detection.md` - Detection patterns for Python issues +- `references/async.md` - Async Python patterns + +--- + +## Checklist + +- [ ] All functions have type hints (params + return) +- [ ] Custom exceptions with meaningful messages +- [ ] Dataclasses or Pydantic for structured data +- [ ] No bare `except:` clauses +- [ ] No mutable default arguments +- [ ] Context managers for resource management +- [ ] `from __future__ import annotations` for forward refs +- [ ] Protocols for structural typing (not ABC unless needed) +- [ ] Comprehensions for simple transforms +- [ ] Tests use pytest with fixtures diff --git a/shared/skills/python/references/async.md b/shared/skills/python/references/async.md new file mode 100644 index 0000000..2178155 --- /dev/null +++ b/shared/skills/python/references/async.md @@ -0,0 +1,220 @@ +# Async Python Patterns + +Deep-dive on async Python programming. Reference from main SKILL.md. + +## Core asyncio Patterns + +### Basic Async Function + +```python +import asyncio +from typing import Any + +async def fetch_user(user_id: str) -> User: + async with aiohttp.ClientSession() as session: + async with session.get(f"/api/users/{user_id}") as response: + data = await response.json() + return User.model_validate(data) +``` + +### Concurrent Execution with gather + +```python +async def load_dashboard(user_id: str) -> Dashboard: + # Independent fetches run concurrently + user, orders, preferences = await asyncio.gather( + fetch_user(user_id), + fetch_orders(user_id), + fetch_preferences(user_id), + ) + return Dashboard(user=user, orders=orders, preferences=preferences) +``` + +## Structured Concurrency with TaskGroup + +### TaskGroup for Safe Concurrency (Python 3.11+) + +```python +async def process_batch(items: list[Item]) -> list[Result]: + results: list[Result] = [] + + async with asyncio.TaskGroup() as tg: + for item in items: + tg.create_task(process_and_collect(item, results)) + + return results + +async def process_and_collect(item: Item, results: list[Result]) -> None: + result = await process_item(item) + results.append(result) +``` + +### Error Handling with TaskGroup + +```python +async def resilient_batch(items: list[Item]) -> tuple[list[Result], list[Error]]: + results: list[Result] = [] + errors: list[Error] = [] + + # TaskGroup cancels all tasks if one raises — wrap individual tasks + async def safe_process(item: Item) -> None: + try: + result = await process_item(item) + results.append(result) + except ProcessingError as e: + errors.append(Error(item_id=item.id, message=str(e))) + + async with asyncio.TaskGroup() as tg: + for item in items: + tg.create_task(safe_process(item)) + + return results, errors +``` + +## Async Generators + +### Streaming Results + +```python +from typing import AsyncGenerator + +async def stream_results(query: str, params: tuple = ()) -> AsyncGenerator[Record, None]: + async with get_connection() as conn: + cursor = await conn.execute(query, params) + async for row in cursor: + yield Record.from_row(row) + +# Usage +async for record in stream_results("SELECT * FROM events WHERE type = ?", ("click",)): + await process(record) +``` + +### Async Generator with Cleanup + +```python +async def paginated_fetch(url: str, page_size: int = 100) -> AsyncGenerator[Item, None]: + page = 0 + while True: + response = await fetch_page(url, page=page, size=page_size) + if not response.items: + break + for item in response.items: + yield item + page += 1 +``` + +## Semaphore for Rate Limiting + +### Bounded Concurrency + +```python +async def fetch_all(urls: list[str], max_concurrent: int = 10) -> list[Response]: + semaphore = asyncio.Semaphore(max_concurrent) + + async def bounded_fetch(url: str) -> Response: + async with semaphore: + return await fetch(url) + + return await asyncio.gather(*[bounded_fetch(url) for url in urls]) +``` + +## aiohttp Patterns + +### Client Session Management + +```python +from contextlib import asynccontextmanager +from typing import AsyncGenerator + +@asynccontextmanager +async def api_client(base_url: str) -> AsyncGenerator[aiohttp.ClientSession, None]: + timeout = aiohttp.ClientTimeout(total=30) + async with aiohttp.ClientSession(base_url, timeout=timeout) as session: + yield session + +# Usage — session reused for multiple requests +async def sync_users(user_ids: list[str]) -> list[User]: + async with api_client("https://api.example.com") as client: + tasks = [fetch_user_with_session(client, uid) for uid in user_ids] + return await asyncio.gather(*tasks) + +async def fetch_user_with_session( + client: aiohttp.ClientSession, user_id: str +) -> User: + async with client.get(f"/users/{user_id}") as response: + response.raise_for_status() + data = await response.json() + return User.model_validate(data) +``` + +## Timeout Patterns + +### Per-Operation Timeout + +```python +async def fetch_with_timeout(url: str, timeout_seconds: float = 5.0) -> dict[str, Any]: + try: + async with asyncio.timeout(timeout_seconds): + return await fetch(url) + except TimeoutError: + raise OperationTimeout(f"Request to {url} timed out after {timeout_seconds}s") +``` + +## Anti-Patterns + +### Blocking Calls in Async Code + +```python +# VIOLATION: Blocks the event loop +async def bad_fetch(url: str) -> str: + import requests + return requests.get(url).text # Blocks entire event loop! + +# CORRECT: Use async library or run in executor +async def good_fetch(url: str) -> str: + async with aiohttp.ClientSession() as session: + async with session.get(url) as response: + return await response.text() + +# CORRECT: Offload blocking call to thread pool +async def run_blocking(func: Callable[..., R], *args: Any) -> R: + loop = asyncio.get_running_loop() + return await loop.run_in_executor(None, func, *args) +``` + +### Fire-and-Forget Without Error Handling + +```python +# VIOLATION: Exception silently lost +async def bad_handler(event: Event) -> None: + asyncio.create_task(send_notification(event)) # Error vanishes + +# CORRECT: Track the task and handle errors +async def good_handler(event: Event) -> None: + task = asyncio.create_task(send_notification(event)) + task.add_done_callback(handle_task_exception) + +def handle_task_exception(task: asyncio.Task[Any]) -> None: + if not task.cancelled() and task.exception() is not None: + logger.error("Background task failed", exc_info=task.exception()) +``` + +### Sequential When Parallel Is Safe + +```python +# VIOLATION: Unnecessarily sequential — 3x slower +async def slow_load(user_id: str) -> Dashboard: + user = await fetch_user(user_id) + orders = await fetch_orders(user_id) + prefs = await fetch_preferences(user_id) + return Dashboard(user=user, orders=orders, preferences=prefs) + +# CORRECT: Concurrent — ~1x latency +async def fast_load(user_id: str) -> Dashboard: + user, orders, prefs = await asyncio.gather( + fetch_user(user_id), + fetch_orders(user_id), + fetch_preferences(user_id), + ) + return Dashboard(user=user, orders=orders, preferences=prefs) +``` diff --git a/shared/skills/python/references/detection.md b/shared/skills/python/references/detection.md new file mode 100644 index 0000000..7f76a6a --- /dev/null +++ b/shared/skills/python/references/detection.md @@ -0,0 +1,128 @@ +# Python Issue Detection + +Grep patterns for finding common Python issues. Reference from main SKILL.md. + +## Type Safety Detection + +### Missing Type Hints + +```bash +# Functions without return type annotation +grep -rn "def .*):$" --include="*.py" | grep -v "->.*:" + +# Functions without any annotations +grep -rn "def .*([a-z_][a-z_0-9]*\s*," --include="*.py" | grep -v ":" +``` + +### Bare Except Clauses + +```bash +# Bare except (catches everything) +grep -rn "except:" --include="*.py" + +# Overly broad except Exception +grep -rn "except Exception:" --include="*.py" | grep -v "# noqa" +``` + +## Data Modeling Detection + +### Mutable Default Arguments + +```bash +# List defaults +grep -rn "def .*=\s*\[\]" --include="*.py" + +# Dict defaults +grep -rn "def .*=\s*{}" --include="*.py" + +# Set defaults +grep -rn "def .*=\s*set()" --include="*.py" +``` + +### Raw Dict Usage Instead of Dataclasses + +```bash +# Dict literals assigned to typed variables +grep -rn ': dict\[' --include="*.py" | grep "= {" + +# Nested dict access patterns (fragile) +grep -rn '\["[a-z].*\]\["' --include="*.py" +``` + +## Anti-Pattern Detection + +### Assert in Production Code + +```bash +# Assert statements outside test files +grep -rn "^ assert " --include="*.py" | grep -v "test_\|_test\.py\|tests/" +``` + +### Global Mutable State + +```bash +# Global variable assignment +grep -rn "^[a-z_].*= \[\]\|^[a-z_].*= {}\|^[a-z_].*= set()" --include="*.py" + +# Global keyword usage +grep -rn "global " --include="*.py" +``` + +### Old-Style String Formatting + +```bash +# Percent formatting +grep -rn '"%.*" %' --include="*.py" +grep -rn "'%.*' %" --include="*.py" + +# .format() where f-strings would be cleaner +grep -rn '\.format(' --include="*.py" +``` + +### Wildcard Imports + +```bash +# Star imports +grep -rn "from .* import \*" --include="*.py" +``` + +## Async Detection + +### Missing Await + +```bash +# Coroutine called without await (common mistake) +grep -rn "async def" --include="*.py" -l | xargs grep -n "[^await ]fetch\|[^await ]save\|[^await ]send" +``` + +### Blocking Calls in Async Code + +```bash +# time.sleep in async context +grep -rn "time\.sleep" --include="*.py" + +# requests library in async files (should use aiohttp/httpx) +grep -rn "import requests\|from requests" --include="*.py" +``` + +## Security Detection + +### Unsafe Deserialization + +```bash +# pickle usage (arbitrary code execution risk) +grep -rn "pickle\.loads\|pickle\.load(" --include="*.py" + +# yaml.load without SafeLoader +grep -rn "yaml\.load(" --include="*.py" | grep -v "SafeLoader\|safe_load" +``` + +### Shell Injection + +```bash +# subprocess with shell=True +grep -rn "shell=True" --include="*.py" + +# os.system calls +grep -rn "os\.system(" --include="*.py" +``` diff --git a/shared/skills/python/references/patterns.md b/shared/skills/python/references/patterns.md new file mode 100644 index 0000000..f65e882 --- /dev/null +++ b/shared/skills/python/references/patterns.md @@ -0,0 +1,226 @@ +# Python Correct Patterns + +Extended correct patterns for Python development. Reference from main SKILL.md. + +## Dependency Injection + +### Constructor Injection + +```python +from typing import Protocol + +class EmailSender(Protocol): + def send(self, to: str, subject: str, body: str) -> None: ... + +class UserService: + def __init__(self, repo: UserRepository, emailer: EmailSender) -> None: + self._repo = repo + self._emailer = emailer + + def create_user(self, request: CreateUserRequest) -> User: + user = User(name=request.name, email=request.email) + saved = self._repo.save(user) + self._emailer.send(user.email, "Welcome", f"Hello {user.name}") + return saved + +# Easy to test — inject fakes +service = UserService(repo=FakeUserRepo(), emailer=FakeEmailSender()) +``` + +### Factory Functions + +```python +def create_app(config: AppConfig) -> Flask: + app = Flask(__name__) + db = Database(config.database_url) + cache = RedisCache(config.redis_url) + user_service = UserService(repo=SqlUserRepo(db), emailer=SmtpSender(config.smtp)) + register_routes(app, user_service) + return app +``` + +## Decorator Patterns + +### Retry Decorator + +```python +import functools +import time +from typing import TypeVar, Callable, ParamSpec + +P = ParamSpec("P") +R = TypeVar("R") + +def retry( + max_attempts: int = 3, + delay: float = 1.0, + exceptions: tuple[type[Exception], ...] = (Exception,), +) -> Callable[[Callable[P, R]], Callable[P, R]]: + def decorator(func: Callable[P, R]) -> Callable[P, R]: + @functools.wraps(func) + def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: + last_error: Exception | None = None + for attempt in range(max_attempts): + try: + return func(*args, **kwargs) + except exceptions as e: + last_error = e + if attempt < max_attempts - 1: + time.sleep(delay * (2 ** attempt)) + raise last_error # type: ignore[misc] + return wrapper + return decorator + +@retry(max_attempts=3, delay=0.5, exceptions=(ConnectionError, TimeoutError)) +def fetch_data(url: str) -> dict[str, Any]: + ... +``` + +### Validation Decorator + +```python +def validate_input(schema: type[BaseModel]): + def decorator(func: Callable[P, R]) -> Callable[P, R]: + @functools.wraps(func) + def wrapper(*args: P.args, **kwargs: P.kwargs) -> R: + # Validate first positional arg after self/cls + data = args[1] if len(args) > 1 else args[0] + schema.model_validate(data) + return func(*args, **kwargs) + return wrapper + return decorator +``` + +## Context Manager Patterns + +### Database Transaction + +```python +from contextlib import contextmanager +from typing import Generator + +@contextmanager +def transaction(session: Session) -> Generator[Session, None, None]: + try: + yield session + session.commit() + except Exception: + session.rollback() + raise + finally: + session.close() + +# Usage +with transaction(db.session()) as session: + session.add(user) + session.add(audit_log) +``` + +### Temporary Directory + +```python +from contextlib import contextmanager +from pathlib import Path +import tempfile +import shutil + +@contextmanager +def temp_workspace() -> Generator[Path, None, None]: + path = Path(tempfile.mkdtemp()) + try: + yield path + finally: + shutil.rmtree(path, ignore_errors=True) +``` + +## Pytest Fixture Patterns + +### Service Fixtures with DI + +```python +import pytest + +@pytest.fixture +def fake_repo() -> FakeUserRepo: + return FakeUserRepo() + +@pytest.fixture +def fake_emailer() -> FakeEmailSender: + return FakeEmailSender() + +@pytest.fixture +def user_service(fake_repo: FakeUserRepo, fake_emailer: FakeEmailSender) -> UserService: + return UserService(repo=fake_repo, emailer=fake_emailer) + +def test_create_user_sends_welcome_email( + user_service: UserService, + fake_emailer: FakeEmailSender, +) -> None: + user_service.create_user(CreateUserRequest(name="Alice", email="a@b.com")) + assert fake_emailer.sent_count == 1 + assert fake_emailer.last_to == "a@b.com" +``` + +### Parametrized Tests + +```python +@pytest.mark.parametrize( + "input_age, expected_valid", + [ + (25, True), + (0, True), + (150, True), + (-1, False), + (151, False), + (None, False), + ], +) +def test_age_validation(input_age: int | None, expected_valid: bool) -> None: + result = validate_age(input_age) + assert result.is_valid == expected_valid +``` + +## Structured Logging + +```python +import structlog + +logger = structlog.get_logger() + +def process_order(order: Order) -> OrderResult: + log = logger.bind(order_id=order.id, user_id=order.user_id) + log.info("processing_order", item_count=len(order.items)) + + try: + result = fulfill(order) + log.info("order_fulfilled", total=result.total) + return result + except InsufficientStockError as e: + log.warning("order_failed_stock", item_id=e.item_id) + raise +``` + +## Enum Patterns + +```python +from enum import Enum, auto + +class OrderStatus(Enum): + PENDING = auto() + PROCESSING = auto() + SHIPPED = auto() + DELIVERED = auto() + CANCELLED = auto() + + @property + def is_terminal(self) -> bool: + return self in (OrderStatus.DELIVERED, OrderStatus.CANCELLED) + + def can_transition_to(self, target: "OrderStatus") -> bool: + valid = { + OrderStatus.PENDING: {OrderStatus.PROCESSING, OrderStatus.CANCELLED}, + OrderStatus.PROCESSING: {OrderStatus.SHIPPED, OrderStatus.CANCELLED}, + OrderStatus.SHIPPED: {OrderStatus.DELIVERED}, + } + return target in valid.get(self, set()) +``` diff --git a/shared/skills/python/references/violations.md b/shared/skills/python/references/violations.md new file mode 100644 index 0000000..1e9c7c2 --- /dev/null +++ b/shared/skills/python/references/violations.md @@ -0,0 +1,204 @@ +# Python Violation Examples + +Extended violation patterns for Python reviews. Reference from main SKILL.md. + +## Type Safety Violations + +### Missing Type Hints + +```python +# VIOLATION: No type annotations +def process(data, config): + result = transform(data) + return result + +# VIOLATION: Partial annotations (inconsistent) +def save(user: User, db): # db missing type + return db.insert(user) + +# VIOLATION: Missing return type +def calculate_total(items: list[Item]): + return sum(item.price for item in items) +``` + +### Bare Except Clauses + +```python +# VIOLATION: Catches everything including SystemExit and KeyboardInterrupt +try: + process_data() +except: + pass + +# VIOLATION: Catches too broadly and silences +try: + user = fetch_user(user_id) +except Exception: + user = None # Hides real errors (network, auth, parsing) + +# VIOLATION: Bare except with logging but no re-raise +try: + critical_operation() +except: + logger.error("Something failed") + # Swallows the error — caller never knows +``` + +## Data Modeling Violations + +### Raw Dicts Instead of Dataclasses + +```python +# VIOLATION: Untyped dict — no IDE support, no validation +user = { + "name": "Alice", + "email": "alice@example.com", + "age": 30, +} + +# VIOLATION: Accessing dict keys without safety +def get_display_name(user: dict) -> str: + return f"{user['first_name']} {user['last_name']}" # KeyError risk + +# VIOLATION: Nested untyped dicts +config = { + "database": { + "host": "localhost", + "port": 5432, + }, + "cache": { + "ttl": 300, + }, +} +``` + +### Mutable Default Arguments + +```python +# VIOLATION: Mutable default shared across calls +def add_item(item: str, items: list[str] = []) -> list[str]: + items.append(item) + return items + +# VIOLATION: Dict default mutated in place +def register(name: str, registry: dict[str, bool] = {}) -> None: + registry[name] = True + +# VIOLATION: Set default +def collect(value: int, seen: set[int] = set()) -> set[int]: + seen.add(value) + return seen +``` + +## String Formatting Violations + +### Percent Formatting + +```python +# VIOLATION: Old-style % formatting +message = "Hello %s, you have %d messages" % (name, count) + +# VIOLATION: % formatting with dict — hard to read, error-prone +log = "%(timestamp)s - %(level)s - %(message)s" % log_data +``` + +### String Concatenation in Loops + +```python +# VIOLATION: O(n^2) string building +result = "" +for line in lines: + result += line + "\n" + +# CORRECT: Use join +result = "\n".join(lines) +``` + +## Global State Violations + +### Module-Level Mutable State + +```python +# VIOLATION: Global mutable state +_cache = {} +_connections = [] + +def get_cached(key: str) -> Any: + return _cache.get(key) + +def add_connection(conn: Connection) -> None: + _connections.append(conn) + +# VIOLATION: Global variable modified by functions +current_user = None + +def login(username: str) -> None: + global current_user + current_user = authenticate(username) +``` + +### Singleton via Module Import + +```python +# VIOLATION: Hard-to-test singleton +# db.py +connection = create_connection(os.environ["DATABASE_URL"]) + +# service.py +from db import connection # Cannot mock or swap for tests +``` + +## Import Violations + +### Wildcard Imports + +```python +# VIOLATION: Pollutes namespace, hides dependencies +from os.path import * +from utils import * + +# VIOLATION: Star import in __init__.py +# mypackage/__init__.py +from .models import * +from .services import * +``` + +### Circular Imports + +```python +# VIOLATION: Circular dependency +# models.py +from services import UserService # services imports models too + +# services.py +from models import User # Circular! +``` + +## Testing Violations + +### Assert in Production Code + +```python +# VIOLATION: assert is stripped with -O flag +def withdraw(account: Account, amount: float) -> None: + assert amount > 0, "Amount must be positive" # Disabled in production! + assert account.balance >= amount, "Insufficient funds" + account.balance -= amount +``` + +### No pytest Fixtures + +```python +# VIOLATION: Repeated setup in every test +def test_create_user(): + db = Database(":memory:") + db.create_tables() + service = UserService(db) + # ... test logic + +def test_update_user(): + db = Database(":memory:") # Duplicated setup + db.create_tables() + service = UserService(db) + # ... test logic +``` diff --git a/shared/skills/react/SKILL.md b/shared/skills/react/SKILL.md index 0c88fb7..ed5635a 100644 --- a/shared/skills/react/SKILL.md +++ b/shared/skills/react/SKILL.md @@ -95,7 +95,7 @@ export function AuthProvider({ children }: { children: React.ReactNode }) { const [user, setUser] = useState(null); const login = async (creds: Credentials) => setUser(await authApi.login(creds)); const logout = () => { authApi.logout(); setUser(null); }; - return {children}; + return {children}; } export function useAuth() { diff --git a/shared/skills/react/references/patterns.md b/shared/skills/react/references/patterns.md index 669447c..12ea9e0 100644 --- a/shared/skills/react/references/patterns.md +++ b/shared/skills/react/references/patterns.md @@ -429,9 +429,9 @@ export function AuthProvider({ children }: { children: React.ReactNode }) { }; return ( - + {children} - + ); } @@ -590,7 +590,7 @@ function SearchInput() { ```tsx // CORRECT: Track previous value for comparisons function usePrevious(value: T): T | undefined { - const ref = useRef(); + const ref = useRef(undefined); useEffect(() => { ref.current = value; diff --git a/shared/skills/rust/SKILL.md b/shared/skills/rust/SKILL.md new file mode 100644 index 0000000..1198ccc --- /dev/null +++ b/shared/skills/rust/SKILL.md @@ -0,0 +1,193 @@ +--- +name: rust +description: This skill should be used when the user works with Rust files (.rs), asks about "ownership", "borrowing", "lifetimes", "Result/Option", "traits", or discusses memory safety and type-driven design. Provides patterns for ownership, error handling, type system usage, and safe concurrency. +user-invocable: false +allowed-tools: Read, Grep, Glob +activation: + file-patterns: + - "**/*.rs" + exclude: + - "**/target/**" +--- + +# Rust Patterns + +Reference for Rust-specific patterns, ownership model, and type-driven design. + +## Iron Law + +> **MAKE ILLEGAL STATES UNREPRESENTABLE** +> +> Encode invariants in the type system. If a function can fail, return `Result`. If a value +> might be absent, return `Option`. If a state transition is invalid, make it uncompilable. +> Runtime checks are a fallback, not a strategy. + +## When This Skill Activates + +- Working with Rust codebases +- Designing type-safe APIs +- Managing ownership and borrowing +- Implementing error handling +- Writing concurrent code + +--- + +## Ownership & Borrowing + +### Prefer Borrowing Over Cloning + +```rust +// BAD: fn process(data: String) — takes ownership unnecessarily +// GOOD: fn process(data: &str) — borrows, caller keeps ownership + +fn process(data: &str) -> usize { + data.len() +} +``` + +### Lifetime Annotations When Needed + +```rust +// Return reference tied to input lifetime +fn first_word(s: &str) -> &str { + s.split_whitespace().next().unwrap_or("") +} + +// Explicit when compiler can't infer +struct Excerpt<'a> { + text: &'a str, +} +``` + +--- + +## Error Handling + +### Use Result and the ? Operator + +```rust +use std::fs; +use std::io; + +fn read_config(path: &str) -> Result { + let content = fs::read_to_string(path) + .map_err(|e| AppError::Io { path: path.into(), source: e })?; + let config: Config = toml::from_str(&content) + .map_err(|e| AppError::Parse { source: e })?; + Ok(config) +} +``` + +### Custom Error Types with thiserror + +```rust +use thiserror::Error; + +#[derive(Error, Debug)] +pub enum AppError { + #[error("IO error reading {path}")] + Io { path: String, #[source] source: io::Error }, + #[error("parse error")] + Parse { #[from] source: toml::de::Error }, + #[error("{entity} with id {id} not found")] + NotFound { entity: String, id: String }, +} +``` + +--- + +## Type System + +### Newtype Pattern + +```rust +// Prevent mixing up IDs +struct UserId(String); +struct OrderId(String); + +fn get_order(user_id: &UserId, order_id: &OrderId) -> Result { + // Can't accidentally swap parameters + todo!() +} +``` + +### Enums for State Machines + +```rust +enum Connection { + Disconnected, + Connecting { attempt: u32 }, + Connected { session: Session }, +} + +// Each state carries only its relevant data +// Invalid transitions are uncompilable +``` + +--- + +## Patterns + +### Builder Pattern + +```rust +pub struct ServerBuilder { + port: u16, + host: String, +} + +impl ServerBuilder { + pub fn new() -> Self { Self { port: 8080, host: "localhost".into() } } + pub fn port(mut self, port: u16) -> Self { self.port = port; self } + pub fn host(mut self, host: impl Into) -> Self { self.host = host.into(); self } + pub fn build(self) -> Server { Server { port: self.port, host: self.host } } +} +``` + +### Iterator Chains Over Loops + +```rust +// BAD: manual loop with push +// GOOD: +let active_names: Vec<&str> = users.iter() + .filter(|u| u.is_active) + .map(|u| u.name.as_str()) + .collect(); +``` + +--- + +## Anti-Patterns + +| Pattern | Bad | Good | +|---------|-----|------| +| Unwrap in library | `.unwrap()` | `?` operator or `.ok_or()` | +| Clone to satisfy borrow checker | `.clone()` everywhere | Restructure ownership | +| String for everything | `HashMap` | Typed structs and enums | +| Ignoring Result | `let _ = write(...)` | Handle or propagate error | +| Mutex for message passing | Shared mutable state | Channels (`mpsc`) | + +--- + +## Extended References + +For additional patterns and examples: +- `references/violations.md` - Common Rust violations +- `references/patterns.md` - Extended Rust patterns +- `references/detection.md` - Detection patterns for Rust issues +- `references/ownership.md` - Advanced ownership and lifetime patterns + +--- + +## Checklist + +- [ ] No `.unwrap()` in library/application code (ok in tests) +- [ ] Custom error types with `thiserror` +- [ ] `?` operator for error propagation +- [ ] Borrow instead of clone where possible +- [ ] Newtype pattern for type-safe IDs +- [ ] Enums for state machines +- [ ] Iterator chains over manual loops +- [ ] `#[must_use]` on Result-returning functions +- [ ] No `unsafe` without safety comment +- [ ] Clippy clean (`cargo clippy -- -D warnings`) diff --git a/shared/skills/rust/references/detection.md b/shared/skills/rust/references/detection.md new file mode 100644 index 0000000..5f85e89 --- /dev/null +++ b/shared/skills/rust/references/detection.md @@ -0,0 +1,131 @@ +# Rust Detection Patterns + +Grep and regex patterns for finding common Rust issues. Use with `Grep` tool. + +## Unwrap and Expect + +```bash +# Find .unwrap() calls (exclude tests) +grep -rn '\.unwrap()' --include='*.rs' --exclude-dir=tests --exclude='*_test.rs' + +# Find .expect() without descriptive message +grep -rn '\.expect("")' --include='*.rs' + +# Find unwrap_or_default hiding errors +grep -rn '\.unwrap_or_default()' --include='*.rs' +``` + +**Pattern**: `\.unwrap\(\)` — matches any `.unwrap()` call +**Pattern**: `\.expect\("` — matches `.expect("` to review message quality + +--- + +## Clone Abuse + +```bash +# Find .clone() calls — review each for necessity +grep -rn '\.clone()' --include='*.rs' + +# Find clone in loop bodies (likely hot-path waste) +grep -rn -A2 'for.*in' --include='*.rs' | grep '\.clone()' + +# Find to_string() where &str would work +grep -rn '\.to_string()' --include='*.rs' +``` + +**Pattern**: `\.clone\(\)` — all clone calls for manual review +**Pattern**: `\.to_owned\(\)` — ownership transfer that may be unnecessary + +--- + +## Unsafe Blocks + +```bash +# Find all unsafe blocks +grep -rn 'unsafe\s*{' --include='*.rs' + +# Find unsafe without SAFETY comment +grep -rn -B2 'unsafe\s*{' --include='*.rs' | grep -v 'SAFETY' + +# Find unsafe functions +grep -rn 'unsafe fn' --include='*.rs' +``` + +**Pattern**: `unsafe\s*\{` — unsafe blocks +**Pattern**: `unsafe fn` — unsafe function declarations + +--- + +## Incomplete Code + +```bash +# Find todo! and unimplemented! macros +grep -rn 'todo!\|unimplemented!' --include='*.rs' + +# Find unreachable! that may hide bugs +grep -rn 'unreachable!' --include='*.rs' + +# Find panic! in non-test code +grep -rn 'panic!' --include='*.rs' --exclude-dir=tests --exclude='*_test.rs' +``` + +**Pattern**: `todo!\(\)` — placeholder code +**Pattern**: `unimplemented!\(\)` — unfinished implementations +**Pattern**: `panic!\(` — explicit panics outside tests + +--- + +## Error Handling Issues + +```bash +# Find ignored Results (let _ = expr that returns Result) +grep -rn 'let _ =' --include='*.rs' + +# Find empty match arms that may swallow errors +grep -rn '=> {}' --include='*.rs' + +# Find catch-all match arms hiding missing cases +grep -rn '_ =>' --include='*.rs' +``` + +**Pattern**: `let _ =` — potentially ignored Result or important value +**Pattern**: `=> \{\}` — empty match arm (may swallow error) + +--- + +## Concurrency Red Flags + +```bash +# Find static mut (almost always wrong) +grep -rn 'static mut' --include='*.rs' + +# Find blocking calls in async functions +grep -rn 'std::fs::' --include='*.rs' | grep -v 'test' +grep -rn 'std::thread::sleep' --include='*.rs' + +# Find Mutex without Arc in multi-threaded context +grep -rn 'Mutex::new' --include='*.rs' +``` + +**Pattern**: `static mut` — mutable global state (data race risk) +**Pattern**: `std::fs::` — blocking I/O that may appear in async context +**Pattern**: `std::thread::sleep` — blocking sleep (use `tokio::time::sleep` in async) + +--- + +## Clippy Lints + +Run Clippy for automated detection of many patterns above: + +```bash +cargo clippy -- -D warnings +cargo clippy -- -W clippy::pedantic +cargo clippy -- -W clippy::nursery +``` + +Key Clippy lints that catch issues: +- `clippy::unwrap_used` — flags unwrap calls +- `clippy::clone_on_ref_ptr` — unnecessary Arc/Rc clone +- `clippy::needless_pass_by_value` — should borrow instead +- `clippy::missing_errors_doc` — public Result fn without doc +- `clippy::wildcard_enum_match_arm` — catch-all hiding cases diff --git a/shared/skills/rust/references/ownership.md b/shared/skills/rust/references/ownership.md new file mode 100644 index 0000000..82ec6fb --- /dev/null +++ b/shared/skills/rust/references/ownership.md @@ -0,0 +1,242 @@ +# Rust Ownership Deep Dive + +Advanced ownership patterns, lifetime elision, interior mutability, and pinning. + +## Lifetime Elision Rules + +The compiler applies three rules to infer lifetimes. When they don't resolve, annotate manually. + +### Rule 1: Each Reference Parameter Gets Its Own Lifetime + +```rust +// Compiler sees: fn first(s: &str) -> &str +// Compiler infers: fn first<'a>(s: &'a str) -> &'a str +fn first(s: &str) -> &str { + &s[..1] +} +``` + +### Rule 2: Single Input Lifetime Applies to All Outputs + +```rust +// One input reference — output borrows from it +fn trim(s: &str) -> &str { + s.trim() +} +``` + +### Rule 3: &self Lifetime Applies to All Outputs in Methods + +```rust +impl Config { + // &self lifetime flows to return + fn database_url(&self) -> &str { + &self.db_url + } +} +``` + +### When Elision Fails + +```rust +// Two input lifetimes — compiler can't decide which output borrows from +// Must annotate: output borrows from `a`, not `b` +fn longest<'a>(a: &'a str, b: &str) -> &'a str { + if a.len() >= b.len() { a } else { a } +} +``` + +--- + +## Interior Mutability + +Mutate data behind a shared reference when ownership rules are too strict. + +### Cell — Copy Types Only + +```rust +use std::cell::Cell; + +struct Counter { + count: Cell, // Mutate through &self +} + +impl Counter { + fn increment(&self) { + self.count.set(self.count.get() + 1); + } +} +``` + +### RefCell — Runtime Borrow Checking + +```rust +use std::cell::RefCell; + +struct Cache { + data: RefCell>, +} + +impl Cache { + fn get_or_insert(&self, key: &str, value: &str) -> String { + let mut data = self.data.borrow_mut(); // Panics if already borrowed + data.entry(key.to_string()) + .or_insert_with(|| value.to_string()) + .clone() + } +} +``` + +### Mutex — Thread-Safe Interior Mutability + +```rust +use std::sync::Mutex; + +struct SharedState { + data: Mutex>, +} + +impl SharedState { + fn push(&self, item: String) -> Result<(), AppError> { + let mut data = self.data.lock() + .map_err(|_| AppError::LockPoisoned)?; + data.push(item); + Ok(()) + } +} +``` + +### Decision Guide + +| Type | Thread-Safe | Cost | Use Case | +|------|-------------|------|----------| +| `Cell` | No | Zero | Copy types, single-threaded | +| `RefCell` | No | Runtime borrow check | Non-Copy, single-threaded | +| `Mutex` | Yes | Lock overhead | Multi-threaded mutation | +| `RwLock` | Yes | Lock overhead | Multi-threaded, read-heavy | +| `Atomic*` | Yes | Hardware atomic | Counters, flags | + +--- + +## Cow — Clone on Write + +Defer cloning until mutation is actually needed. + +```rust +use std::borrow::Cow; + +// Returns borrowed if no processing needed, owned if modified +fn normalize_path(path: &str) -> Cow<'_, str> { + if path.contains("//") { + Cow::Owned(path.replace("//", "/")) + } else { + Cow::Borrowed(path) + } +} + +// Function accepts both owned and borrowed transparently +fn process(input: Cow<'_, str>) { + println!("{}", input); // No allocation if already borrowed +} +``` + +### Cow in APIs + +```rust +// Accept Cow for flexible ownership — caller decides allocation +pub fn log_message(msg: Cow<'_, str>) { + eprintln!("[LOG] {}", msg); +} + +// Caller with borrowed data — zero-copy +log_message(Cow::Borrowed("static message")); + +// Caller with owned data — no extra clone +log_message(Cow::Owned(format!("dynamic: {}", value))); +``` + +--- + +## Pin for Async and Self-Referential Types + +### Why Pin Exists + +Self-referential structs break if moved in memory. `Pin` guarantees the value won't move. + +```rust +use std::pin::Pin; +use std::future::Future; + +// Async functions return self-referential futures +// Pin ensures the future stays in place while polled +fn fetch_data(url: &str) -> Pin> + '_>> { + Box::pin(async move { + let response = reqwest::get(url).await?; + let data = response.json::().await?; + Ok(data) + }) +} +``` + +### Pin in Practice + +```rust +use tokio::pin; + +async fn process_stream(stream: impl Stream) { + // pin! macro pins the stream to the stack + pin!(stream); + + while let Some(item) = stream.next().await { + handle(item).await; + } +} +``` + +### When You Need Pin + +| Scenario | Need Pin? | +|----------|-----------| +| Returning `async` blocks as trait objects | Yes | +| Implementing `Future` manually | Yes | +| Using `tokio::select!` on futures | Yes (automatically handled) | +| Normal async/await | No (compiler handles it) | +| Storing futures in collections | Yes (`Pin>`) | + +--- + +## Ownership Transfer Patterns + +### Take Pattern — Move Out of Option + +```rust +struct Connection { + session: Option, +} + +impl Connection { + fn close(&mut self) -> Option { + self.session.take() // Moves out, leaves None + } +} +``` + +### Swap Pattern — Replace In Place + +```rust +use std::mem; + +fn rotate_buffer(current: &mut Vec, new_data: Vec) -> Vec { + mem::replace(current, new_data) // Returns old, installs new +} +``` + +### Entry Pattern — Conditional Insertion + +```rust +use std::collections::HashMap; + +fn get_or_create(map: &mut HashMap>, key: &str) -> &mut Vec { + map.entry(key.to_string()).or_insert_with(Vec::new) +} +``` diff --git a/shared/skills/rust/references/patterns.md b/shared/skills/rust/references/patterns.md new file mode 100644 index 0000000..32b3ee4 --- /dev/null +++ b/shared/skills/rust/references/patterns.md @@ -0,0 +1,210 @@ +# Rust Extended Patterns + +Extended correct patterns for Rust. Reference from main SKILL.md. + +## Typestate Pattern + +Encode valid state transitions in the type system so invalid sequences don't compile. + +```rust +// States are zero-sized types — no runtime cost +struct Draft; +struct Published; +struct Archived; + +struct Article { + title: String, + body: String, + _state: std::marker::PhantomData, +} + +impl Article { + pub fn new(title: String, body: String) -> Self { + Article { title, body, _state: std::marker::PhantomData } + } + + pub fn publish(self) -> Article { + Article { title: self.title, body: self.body, _state: std::marker::PhantomData } + } +} + +impl Article { + pub fn archive(self) -> Article { + Article { title: self.title, body: self.body, _state: std::marker::PhantomData } + } +} + +// article.archive() on Draft won't compile — transition enforced at compile time +``` + +--- + +## Error Handling Hierarchy + +Layer errors from specific to general using `thiserror` for libraries and `anyhow` for applications. + +```rust +// Library: precise, typed errors +#[derive(thiserror::Error, Debug)] +pub enum RepoError { + #[error("entity {entity} with id {id} not found")] + NotFound { entity: &'static str, id: String }, + #[error("duplicate key: {0}")] + Duplicate(String), + #[error("connection failed")] + Connection(#[from] sqlx::Error), +} + +// Application: ergonomic error propagation +use anyhow::{Context, Result}; + +fn run() -> Result<()> { + let config = load_config() + .context("failed to load configuration")?; + let db = connect_db(&config.database_url) + .context("failed to connect to database")?; + serve(db, config.port) + .context("server exited with error") +} +``` + +--- + +## Trait Objects vs Generics + +### Use Generics for Performance (Monomorphization) + +```rust +fn largest(list: &[T]) -> Option<&T> { + list.iter().reduce(|a, b| if a >= b { a } else { b }) +} +``` + +### Use Trait Objects for Heterogeneous Collections + +```rust +trait Handler: Send + Sync { + fn handle(&self, request: &Request) -> Response; +} + +struct Router { + routes: Vec>, // Different concrete types in one Vec +} +``` + +### Decision Guide + +| Criteria | Generics | Trait Objects | +|----------|----------|--------------| +| Known types at compile time | Yes | No | +| Heterogeneous collection | No | Yes | +| Performance-critical | Yes | Acceptable overhead | +| Binary size concern | Increases | Minimal | + +--- + +## Smart Pointers + +### Box — Heap Allocation + +```rust +// Recursive types require indirection +enum List { + Cons(T, Box>), + Nil, +} +``` + +### Rc/Arc — Shared Ownership + +```rust +use std::sync::Arc; + +// Shared read-only config across threads +let config = Arc::new(load_config()?); +let config_clone = Arc::clone(&config); +tokio::spawn(async move { + use_config(&config_clone).await; +}); +``` + +### When to Use Each + +| Pointer | Use Case | +|---------|----------| +| `Box` | Single owner, heap allocation, recursive types | +| `Rc` | Multiple owners, single-threaded | +| `Arc` | Multiple owners, multi-threaded | +| `Cow<'a, T>` | Clone-on-write, flexible borrowing | + +--- + +## From/Into Conversions + +```rust +// Implement From for automatic Into +impl From for User { + fn from(req: CreateUserRequest) -> Self { + User { + id: Uuid::new_v4(), + name: req.name, + email: req.email, + created_at: Utc::now(), + } + } +} + +// Callers get Into for free +fn save_user(user: impl Into) -> Result<(), DbError> { + let user: User = user.into(); + // ... + Ok(()) +} +``` + +--- + +## Derive and Trait Best Practices + +```rust +// Derive the standard set for data types +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct UserId(String); + +// Derive serde for serialization boundaries +#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] +pub struct ApiResponse { + pub data: T, + pub metadata: Metadata, +} +``` + +### Trait Implementation Order Convention + +1. Standard library traits (`Debug`, `Display`, `Clone`, `PartialEq`) +2. Conversion traits (`From`, `Into`, `TryFrom`) +3. Iterator traits (`Iterator`, `IntoIterator`) +4. Serde traits (`Serialize`, `Deserialize`) +5. Custom traits (domain-specific) + +--- + +## Module Organization + +``` +src/ +├── lib.rs # Public API re-exports +├── error.rs # Crate-level error types +├── domain/ +│ ├── mod.rs # Domain re-exports +│ ├── user.rs # User entity and logic +│ └── order.rs # Order entity and logic +├── repo/ +│ ├── mod.rs # Repository trait definitions +│ └── postgres.rs # Concrete implementation +└── api/ + ├── mod.rs # Route registration + └── handlers.rs # HTTP handlers +``` + +Keep `lib.rs` thin — re-export only the public API. Internal modules use `pub(crate)`. diff --git a/shared/skills/rust/references/violations.md b/shared/skills/rust/references/violations.md new file mode 100644 index 0000000..85bb9fc --- /dev/null +++ b/shared/skills/rust/references/violations.md @@ -0,0 +1,191 @@ +# Rust Violation Examples + +Extended violation patterns for Rust reviews. Reference from main SKILL.md. + +## Unwrap Abuse + +### Unwrap in Library Code + +```rust +// VIOLATION: Panics on None — caller has no way to handle failure +pub fn get_username(users: &HashMap, id: u64) -> &str { + users.get(&id).unwrap() // Panics if id not found +} + +// VIOLATION: Unwrap on parse without context +let port: u16 = std::env::var("PORT").unwrap().parse().unwrap(); +``` + +### Expect Without Useful Message + +```rust +// VIOLATION: Message doesn't help diagnose the problem +let config = load_config().expect("failed"); + +// CORRECT: Actionable message +let config = load_config().expect("failed to load config from config.toml — does file exist?"); +``` + +--- + +## Unnecessary Cloning + +### Clone to Satisfy Borrow Checker + +```rust +// VIOLATION: Cloning to work around borrow issues +fn process_items(items: &Vec) { + let cloned = items.clone(); // Entire Vec cloned + for item in &cloned { + println!("{}", item.name); + } +} + +// CORRECT: Just borrow +fn process_items(items: &[Item]) { + for item in items { + println!("{}", item.name); + } +} +``` + +### Clone in Hot Loop + +```rust +// VIOLATION: Allocating on every iteration +for record in &records { + let key = record.id.clone(); // String allocation per iteration + map.insert(key, record); +} + +// CORRECT: Borrow or use references +for record in &records { + map.insert(&record.id, record); +} +``` + +--- + +## Stringly-Typed APIs + +### String Where Enum Belongs + +```rust +// VIOLATION: Any typo compiles and fails at runtime +fn set_status(status: &str) { + match status { + "active" => { /* ... */ } + "inactive" => { /* ... */ } + _ => panic!("unknown status"), // Runtime failure + } +} + +// CORRECT: Compiler enforces valid values +enum Status { Active, Inactive } + +fn set_status(status: Status) { + match status { + Status::Active => { /* ... */ } + Status::Inactive => { /* ... */ } + } // Exhaustive — no default needed +} +``` + +--- + +## Unsafe Without Justification + +### Bare Unsafe Block + +```rust +// VIOLATION: No safety comment explaining invariants +unsafe { + let ptr = data.as_ptr(); + std::ptr::copy_nonoverlapping(ptr, dest, len); +} + +// CORRECT: Document why this is safe +// SAFETY: `data` is guaranteed to be valid for `len` bytes because +// it was allocated by `Vec::with_capacity(len)` and filled by `read_exact`. +// `dest` is a valid pointer from `alloc::alloc(layout)` with matching size. +unsafe { + std::ptr::copy_nonoverlapping(data.as_ptr(), dest, len); +} +``` + +### Unnecessary Unsafe + +```rust +// VIOLATION: Using unsafe when safe alternative exists +unsafe fn get_element(slice: &[u8], index: usize) -> u8 { + *slice.get_unchecked(index) +} + +// CORRECT: Safe indexing with bounds check +fn get_element(slice: &[u8], index: usize) -> Option { + slice.get(index).copied() +} +``` + +--- + +## Ignoring Results + +### Discarding Write Errors + +```rust +// VIOLATION: Write failure silently ignored +let _ = file.write_all(data); +let _ = file.flush(); + +// CORRECT: Propagate errors +file.write_all(data)?; +file.flush()?; +``` + +### Ignoring Lock Poisoning + +```rust +// VIOLATION: Silently ignoring poisoned mutex +let guard = mutex.lock().unwrap_or_else(|e| e.into_inner()); + +// CORRECT: Handle or propagate the poison +let guard = mutex.lock().map_err(|_| AppError::LockPoisoned)?; +``` + +--- + +## Concurrency Violations + +### Shared Mutable State Without Synchronization + +```rust +// VIOLATION: Data race potential — no synchronization +static mut COUNTER: u64 = 0; + +fn increment() { + unsafe { COUNTER += 1; } // Undefined behavior under concurrency +} + +// CORRECT: Use atomic or mutex +use std::sync::atomic::{AtomicU64, Ordering}; +static COUNTER: AtomicU64 = AtomicU64::new(0); + +fn increment() { + COUNTER.fetch_add(1, Ordering::Relaxed); +} +``` + +### Blocking in Async Context + +```rust +// VIOLATION: Blocks the async runtime thread +async fn read_file(path: &str) -> Result { + std::fs::read_to_string(path) // Blocking call in async fn +} + +// CORRECT: Use async file I/O or spawn_blocking +async fn read_file(path: &str) -> Result { + tokio::fs::read_to_string(path).await +} +``` diff --git a/shared/skills/typescript/references/patterns.md b/shared/skills/typescript/references/patterns.md index 62536ce..132ceec 100644 --- a/shared/skills/typescript/references/patterns.md +++ b/shared/skills/typescript/references/patterns.md @@ -137,7 +137,7 @@ const isUndefined = (value: unknown): value is undefined => const isNullish = (value: unknown): value is null | undefined => value === null || value === undefined; -const isFunction = (value: unknown): value is Function => +const isFunction = (value: unknown): value is (...args: unknown[]) => unknown => typeof value === 'function'; const isObject = (value: unknown): value is object => @@ -760,7 +760,7 @@ function debounce any>( fn: T, delayMs: number ): (...args: Parameters) => void { - let timeoutId: NodeJS.Timeout | null = null; + let timeoutId: ReturnType | null = null; return (...args: Parameters) => { if (timeoutId) clearTimeout(timeoutId); @@ -774,7 +774,7 @@ function throttle any>( limitMs: number ): (...args: Parameters) => void { let lastRun = 0; - let timeoutId: NodeJS.Timeout | null = null; + let timeoutId: ReturnType | null = null; return (...args: Parameters) => { const now = Date.now(); diff --git a/src/cli/plugins.ts b/src/cli/plugins.ts index dc09850..c87199d 100644 --- a/src/cli/plugins.ts +++ b/src/cli/plugins.ts @@ -24,7 +24,7 @@ export const DEVFLOW_PLUGINS: PluginDefinition[] = [ description: 'Auto-activating quality enforcement (foundation layer)', commands: [], agents: [], - skills: ['accessibility', 'core-patterns', 'docs-framework', 'frontend-design', 'git-safety', 'git-workflow', 'github-patterns', 'input-validation', 'react', 'test-driven-development', 'test-patterns', 'typescript'], + skills: ['core-patterns', 'docs-framework', 'git-safety', 'git-workflow', 'github-patterns', 'input-validation', 'test-driven-development', 'test-patterns'], }, { name: 'devflow-specify', @@ -38,14 +38,14 @@ export const DEVFLOW_PLUGINS: PluginDefinition[] = [ description: 'Complete task implementation workflow', commands: ['/implement'], agents: ['git', 'skimmer', 'synthesizer', 'coder', 'simplifier', 'scrutinizer', 'shepherd', 'validator'], - skills: ['accessibility', 'agent-teams', 'frontend-design', 'implementation-patterns', 'self-review'], + skills: ['agent-teams', 'implementation-patterns', 'self-review'], }, { name: 'devflow-code-review', description: 'Comprehensive code review', commands: ['/code-review'], agents: ['git', 'reviewer', 'synthesizer'], - skills: ['accessibility', 'agent-teams', 'architecture-patterns', 'complexity-patterns', 'consistency-patterns', 'database-patterns', 'dependencies-patterns', 'documentation-patterns', 'frontend-design', 'performance-patterns', 'react', 'regression-patterns', 'review-methodology', 'security-patterns', 'test-patterns'], + skills: ['agent-teams', 'architecture-patterns', 'complexity-patterns', 'consistency-patterns', 'database-patterns', 'dependencies-patterns', 'documentation-patterns', 'performance-patterns', 'regression-patterns', 'review-methodology', 'security-patterns', 'test-patterns'], }, { name: 'devflow-resolve', @@ -83,6 +83,70 @@ export const DEVFLOW_PLUGINS: PluginDefinition[] = [ skills: [], optional: true, }, + { + name: 'devflow-typescript', + description: 'TypeScript language patterns (type safety, generics, utility types)', + commands: [], + agents: [], + skills: ['typescript'], + optional: true, + }, + { + name: 'devflow-react', + description: 'React framework patterns (hooks, state, composition, performance)', + commands: [], + agents: [], + skills: ['react'], + optional: true, + }, + { + name: 'devflow-accessibility', + description: 'Web accessibility patterns (WCAG, ARIA, keyboard navigation)', + commands: [], + agents: [], + skills: ['accessibility'], + optional: true, + }, + { + name: 'devflow-frontend-design', + description: 'Frontend design patterns (typography, color, spacing, motion)', + commands: [], + agents: [], + skills: ['frontend-design'], + optional: true, + }, + { + name: 'devflow-go', + description: 'Go language patterns (error handling, interfaces, concurrency)', + commands: [], + agents: [], + skills: ['go'], + optional: true, + }, + { + name: 'devflow-java', + description: 'Java language patterns (records, sealed classes, composition)', + commands: [], + agents: [], + skills: ['java'], + optional: true, + }, + { + name: 'devflow-python', + description: 'Python language patterns (type hints, protocols, data modeling)', + commands: [], + agents: [], + skills: ['python'], + optional: true, + }, + { + name: 'devflow-rust', + description: 'Rust language patterns (ownership, error handling, type system)', + commands: [], + agents: [], + skills: ['rust'], + optional: true, + }, ]; /** diff --git a/tests/plugins.test.ts b/tests/plugins.test.ts index 8287def..2a99e42 100644 --- a/tests/plugins.test.ts +++ b/tests/plugins.test.ts @@ -16,7 +16,7 @@ describe('getAllSkillNames', () => { it('includes skills from multiple plugins', () => { const skills = getAllSkillNames(); - // 'accessibility' appears in core-skills, implement, and code-review + // 'accessibility' appears in devflow-accessibility (optional plugin) expect(skills).toContain('accessibility'); // 'agent-teams' appears in multiple plugins expect(skills).toContain('agent-teams'); @@ -42,8 +42,8 @@ describe('buildAssetMaps', () => { it('assigns each asset to the first plugin that declares it', () => { const { skillsMap, agentsMap } = buildAssetMaps(DEVFLOW_PLUGINS); - // 'accessibility' first appears in devflow-core-skills - expect(skillsMap.get('accessibility')).toBe('devflow-core-skills'); + // 'accessibility' first appears in devflow-accessibility (optional plugin) + expect(skillsMap.get('accessibility')).toBe('devflow-accessibility'); // 'git' first appears in devflow-implement expect(agentsMap.get('git')).toBe('devflow-implement'); @@ -121,3 +121,49 @@ describe('DEVFLOW_PLUGINS integrity', () => { expect(DEVFLOW_PLUGINS.length).toBeGreaterThanOrEqual(8); }); }); + +describe('optional plugin flag', () => { + const languagePluginNames = [ + 'devflow-typescript', + 'devflow-react', + 'devflow-accessibility', + 'devflow-frontend-design', + 'devflow-go', + 'devflow-java', + 'devflow-python', + 'devflow-rust', + ]; + + it('all language/ecosystem plugins have optional: true', () => { + for (const name of languagePluginNames) { + const plugin = DEVFLOW_PLUGINS.find(p => p.name === name); + expect(plugin, `${name} should exist`).toBeDefined(); + expect(plugin!.optional, `${name} should be optional`).toBe(true); + } + }); + + it('non-language plugins do not have optional: true (except audit-claude)', () => { + const allowedOptional = new Set([...languagePluginNames, 'devflow-audit-claude']); + for (const plugin of DEVFLOW_PLUGINS) { + if (!allowedOptional.has(plugin.name)) { + expect(plugin.optional, `${plugin.name} should not be optional`).toBeFalsy(); + } + } + }); + + it('new language skills exist in the registry', () => { + const skills = getAllSkillNames(); + for (const lang of ['go', 'java', 'python', 'rust']) { + expect(skills, `skill '${lang}' should exist`).toContain(lang); + } + }); + + it('devflow-core-skills does not contain language/ecosystem skills', () => { + const coreSkills = DEVFLOW_PLUGINS.find(p => p.name === 'devflow-core-skills'); + expect(coreSkills).toBeDefined(); + const movedSkills = ['typescript', 'react', 'accessibility', 'frontend-design']; + for (const skill of movedSkills) { + expect(coreSkills!.skills, `core-skills should not contain '${skill}'`).not.toContain(skill); + } + }); +});