diff --git a/.factory-plugin/marketplace.json b/.factory-plugin/marketplace.json index 4eecc39..4282159 100644 --- a/.factory-plugin/marketplace.json +++ b/.factory-plugin/marketplace.json @@ -35,6 +35,24 @@ "description": "Autonomous experiment loop for optimization research. Try an idea, measure it, keep what works, discard what doesn't, repeat. Works standalone or as a mission worker.", "source": "./plugins/autoresearch", "category": "research" + }, + { + "name": "typescript", + "description": "Opinionated TypeScript and React patterns: ban `as` assertions, replace useEffect with derived state, and fix knip unused exports", + "source": "./plugins/typescript", + "category": "quality" + }, + { + "name": "debugging", + "description": "Inspect runtime behavior: HTTP interception, traffic capture, and wire-level debugging for CLIs and services", + "source": "./plugins/debugging", + "category": "productivity" + }, + { + "name": "code-review", + "description": "Pull request lifecycle skills: create PRs with consistent conventions and follow up on them until merge-ready", + "source": "./plugins/code-review", + "category": "productivity" } ] } diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..59e8245 --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +.factory/ diff --git a/README.md b/README.md index f28ccc9..a7a74d8 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,7 @@ Core skills for essential functionalities and integrations. Pre-installed by the **Skills:** - `review` - Review code changes and identify high-confidence, actionable bugs. Includes systematic analysis patterns for null safety, async/await, security, concurrency, API contracts, and more. Used by both the CLI `/review` command and the CI action. +- `simplify` - Parallel code review across reuse, quality, and efficiency axes ### droid-control @@ -44,7 +45,7 @@ See [plugins/droid-control/README.md](plugins/droid-control/README.md) for detai ### security-engineer -Security review, threat modeling, and vulnerability validation skills. +Security review, threat modeling, vulnerability validation, and patch generation skills. **Skills:** @@ -53,6 +54,33 @@ Security review, threat modeling, and vulnerability validation skills. - `commit-security-scan` - Scan commits/PRs for security vulnerabilities - `vulnerability-validation` - Validate and confirm security findings +### typescript + +Opinionated TypeScript and React patterns for safer, cleaner code. + +**Skills:** + +- `ban-type-assertions` - Ban `as` casts and replace them with compiler-verified alternatives (zod, control-flow narrowing) +- `no-use-effect` - Five replacement patterns for `useEffect` (derived state, query libraries, event handlers, `useMountEffect`, `key`) +- `fix-knip-unused-exports` - Fix every category of knip "Unused exports" violation + +### debugging + +Inspect runtime behavior: HTTP interception, traffic capture, and wire-level debugging for CLIs and services. + +**Skills:** + +- `http-toolkit-intercept` - Intercept and debug HTTP traffic from any CLI, service, or script via HTTP Toolkit (language/runtime agnostic) + +### code-review + +Pull request lifecycle skills: open, triage, and follow up on PRs with consistent conventions. + +**Skills:** + +- `create-pr` - Open a PR with Conventional Commits title, templated body, and local verification gates +- `follow-up-on-pr` - Rebase, address reviewer comments, fix CI, and push an existing PR to merge-ready state + ### droid-evolved Skills for continuous learning and improvement. diff --git a/plugins/code-review/.factory-plugin/plugin.json b/plugins/code-review/.factory-plugin/plugin.json new file mode 100644 index 0000000..6fee18c --- /dev/null +++ b/plugins/code-review/.factory-plugin/plugin.json @@ -0,0 +1,8 @@ +{ + "name": "code-review", + "description": "Pull request lifecycle skills: create PRs with consistent conventions and follow up on them until merge-ready", + "author": { + "name": "Factory", + "email": "support@factory.ai" + } +} diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md new file mode 100644 index 0000000..cab227d --- /dev/null +++ b/plugins/code-review/README.md @@ -0,0 +1,19 @@ +# code-review + +Pull request lifecycle skills: open, triage, and follow up on PRs with consistent conventions. + +## Skills + +### `create-pr` + +Open a PR with Conventional Commits title, a templated body, local verification (lint/typecheck/tests), and an optional linked ticket. Use when the user asks to "create a PR," "open a PR," or "put code up for review." + +### `follow-up-on-pr` + +Take over an existing PR: rebase on the base branch, address reviewer comments, fix CI failures, and push updates to a merge-ready state. Accepts a PR URL or number as input. + +## Install + +```bash +droid plugin install code-review@factory-plugins +``` diff --git a/plugins/code-review/skills/create-pr/SKILL.md b/plugins/code-review/skills/create-pr/SKILL.md new file mode 100644 index 0000000..5d34ca3 --- /dev/null +++ b/plugins/code-review/skills/create-pr/SKILL.md @@ -0,0 +1,179 @@ +--- +name: create-pr +description: Create a pull request with Conventional Commits formatting, a templated body, and local verification. Use when the user asks to create a PR, open a PR, submit changes for review, or put code up for review. +--- + +# Create Pull Request + +Create a PR with proper conventions: local verification, Conventional Commits title, a templated body, and an optional linked ticket. This skill is language- and framework-agnostic — substitute your project's actual build, lint, test, and format commands where examples are shown. + +## Prerequisites + +Before starting, verify: +1. Current branch has commits not on the base branch (`git log origin/..HEAD --oneline`) +2. Branch is pushed to remote (`git push -u origin HEAD` if not) +3. No uncommitted changes that should be included (`git status`) + +## Workflow + +### 1. Understand the Changes + +Run in parallel: +```bash +git log origin/..HEAD --oneline +git diff origin/..HEAD --stat +``` + +Determine: +- **What changed**: Which modules, packages, services, or directories were modified +- **Change type**: `feat`, `fix`, `docs`, `refactor`, `test`, `chore`, `perf`, `ci`, `build`, `revert` +- **Scope**: Primary module/package/service affected (use directory name or `monorepo` / `repo` for cross-cutting changes) +- **Is this a code change?**: If the PR modifies source code (not only docs, markdown, or config-only changes), run the local verification checklist in step 2 before creating the PR. + +### 2. Local Verification (for code changes) + +**Skip this step** if the PR only touches documentation, markdown files, or other non-code files. For any change that touches source files, run your project's verification commands locally before creating the PR. + +Discover the commands by reading the repo root (e.g. `Makefile`, `package.json`, `pyproject.toml`, `Cargo.toml`, `go.mod`, `build.gradle`, `mix.exs`, `Gemfile`, `composer.json`, `justfile`, `Taskfile.yml`, `README.md`, or the CI workflow config). Use filter/target flags where available (e.g. `turbo --filter`, `nx --projects`, `pnpm --filter`, `bazel //path/...`, `cargo -p `, `pytest `, `go test .//...`) to run only the affected portions — it is faster than running the whole repo. + +Common verification categories to run when applicable: + +#### Typecheck / Compile +Run the project's static type check or compile step if it has one. + +Examples across ecosystems (use whatever the repo defines): +- TypeScript: `npm run typecheck`, `pnpm -r typecheck`, `tsc --noEmit` +- Python (typed): `mypy .`, `pyright`, `ty check` +- Rust: `cargo check` +- Go: `go build ./...`, `go vet ./...` +- Java/Kotlin: `./gradlew compileJava`, `./mvnw compile` + +#### Lint / Format +Run the project's linter and formatter. Prefer an autofix target if one exists. + +Examples: +- JS/TS: `npm run fix`, `npm run lint`, `eslint .`, `prettier --check .` +- Python: `ruff check --fix .`, `ruff format .`, `black .`, `flake8` +- Rust: `cargo clippy --all-targets`, `cargo fmt --check` +- Go: `golangci-lint run`, `gofmt -l .` +- Shell: `shellcheck`, `shfmt -d .` + +#### Tests +Run the unit/integration tests for affected packages. + +Examples: +- JS/TS: `npm run test -- --filter=`, `pnpm -r test`, `vitest run `, `jest ` +- Python: `pytest `, `tox -e `, `python -m unittest` +- Rust: `cargo test -p ` +- Go: `go test .//...` +- Java/Kotlin: `./gradlew test`, `./mvnw test` +- Ruby: `bundle exec rspec `, `rake test` + +#### Additional checks (run when relevant) +- **Unused exports / dead code**: Run your project's dead-code check if it has one (e.g. `knip`, `ts-prune`, `vulture` for Python, `deadcode` / `unused` for Go, `cargo udeps` for Rust). +- **Dependency hygiene**: Run your project's dependency check if it has one (e.g. `depcheck`, `pip check`, `cargo audit`, `bundle audit`). +- **Lockfile in sync**: If you modified any dependency manifest (`package.json`, `requirements.txt`, `pyproject.toml`, `Cargo.toml`, `go.mod`, `Gemfile`, etc.), run the install command (`npm install`, `pnpm install`, `uv sync`, `poetry lock --no-update`, `cargo update -w`, `go mod tidy`, `bundle install`) and commit any lockfile changes. CI commonly fails if the lockfile is out of date. +- **Generated code / codegen**: If the repo has an OpenAPI spec, protobuf, GraphQL schema, SQL migrations, or any other generated artifacts, regenerate and commit any changes. +- **Style / asset linters**: Run stylesheet linters (`stylelint`, etc.) or asset linters if you changed those files. +- **Security scans**: Run any security/secret scanners configured in the repo (`trivy`, `semgrep`, `gitleaks`, etc.). + +### 3. Link to a Ticket (optional) + +If your org uses an issue tracker, ask the user whether to: +- **Create a new ticket**: Use the appropriate tool (Linear, Jira, GitHub Issues, etc.) +- **Link an existing ticket**: Ask for the identifier (e.g. `TEAM-1234`, `JIRA-567`, `#42`) +- **Skip**: Only if user explicitly says no ticket is needed + +Most CI systems can be configured to require the ticket identifier in the PR body. Follow your org's convention. + +### 4. Format PR Title + +Follow Conventional Commits: `type(scope): description` + +- `type`: `feat`, `fix`, `docs`, `refactor`, `test`, `chore`, `perf`, `ci`, `build`, `revert` +- `scope`: Name of the affected module/package/service/directory, or `monorepo` / `repo` for cross-cutting changes. Multiple scopes can be comma-separated: `fix(a, b, c): ...` + +Examples: +- `feat(web): add dark mode toggle` +- `fix(cli, daemon): load shell env at entrypoint` +- `fix(api): handle nil response from upstream` +- `chore(repo): bump dependencies` + +### 5. Generate PR Body + +Fill in all sections from your PR template. A typical template has four sections: + +```markdown +## Description + + + +## Related Issue + +Closes TEAM-XXXX + + +## Potential Risk & Impact + + + + +## How Has This Been Tested? + + +``` + +### 6. Create the PR + +```bash +gh pr create \ + --base \ + --head \ + --title "(): " \ + --body "" +``` + +If the body is long, write it to a temp file and use `--body-file`: +```bash +gh pr create --base --head --title "..." --body-file /tmp/pr-body.md +``` + +### 7. Report Result + +Return the PR URL to the user. + +## CI Checks Reference (template) + +These are typical check categories that run on every PR. Map them to your repo's actual commands when adapting this skill. + +### Always-run checks +| Category | What it does | How to find the local command | +|---|---|---| +| **Typecheck / compile** | Verifies the project compiles or passes static types | Check `package.json`, `Makefile`, `pyproject.toml`, `Cargo.toml`, `go.mod`, CI config | +| **Lint** | Enforces code style / correctness rules | Check for `lint`, `check`, or equivalent scripts in the repo root | +| **Format** | Enforces consistent formatting | Check for `format`, `fmt`, `prettier`, `black`, `gofmt`, `rustfmt`, etc. | +| **Tests** | Runs unit and integration tests | Check for `test` script / target | +| **Dead code / unused exports** | Flags unused code | Check for `knip`, `ts-prune`, `vulture`, `cargo udeps`, etc. | +| **Dependency check** | Flags unused / vulnerable dependencies | Check for `depcheck`, `audit`, `cargo audit`, etc. | +| **Lockfile in sync** | Fails if lockfile is stale relative to the manifest | Run your package manager's install command and commit the lockfile | +| **PR Conventions** | Validates branch name, semantic title, ticket presence | Follow the formatting rules above | + +### Conditional checks (run only when affected files change) +- **API / schema validation**: Triggered by API or schema changes. Regenerate locally. +- **Platform-specific builds**: Triggered when desktop/mobile/embedded targets are affected. +- **E2E tests**: Triggered when the consumer app or top-level binary is affected. + +### Typical PR conventions CI enforces +- **Branch name**: Max length, allowed characters (e.g. `[A-Za-z0-9/-]`). +- **Title**: Conventional Commits format with a valid scope. +- **Ticket reference**: PR body must contain a ticket identifier (often skipped for `chore:` and `revert:` types). + +## Common Mistakes to Avoid + +- **Wrong base branch**: Use the branch your org takes PRs into (e.g. `dev`, `main`, `develop`, `trunk`). +- **Missing scope**: PR title CI check often requires a valid scope. +- **Missing ticket reference**: Description must reference your ticket ID for CI to pass (except `chore:`/`revert:`). +- **Forgetting to push**: Branch must be on remote before `gh pr create`. +- **Lockfile drift**: Always run the install command and commit lockfile changes after dependency changes. +- **Skipping local checks on code PRs**: Typecheck/compile, lint, and tests should be run locally before sending out code changes to catch issues early and avoid CI round-trips. +- **Uncommitted generated artifacts**: After API/schema changes, regenerate and commit. diff --git a/plugins/code-review/skills/follow-up-on-pr/SKILL.md b/plugins/code-review/skills/follow-up-on-pr/SKILL.md new file mode 100644 index 0000000..44a8d1a --- /dev/null +++ b/plugins/code-review/skills/follow-up-on-pr/SKILL.md @@ -0,0 +1,144 @@ +--- +name: follow-up-on-pr +description: Follow up on an existing PR by rebasing on the base branch, addressing reviewer comments, fixing CI issues, and pushing updates. Use when the user provides a PR URL or number and wants to get it ready for merge. +--- + +# Follow Up on PR + +Take over an existing PR, bring it up to date, address all feedback, and push it to a merge-ready state. This skill is language- and framework-agnostic — substitute your project's actual build, lint, test, and format commands where examples are shown. + +## Inputs + +- **PR URL or number** (required): e.g. `https://github.com///pull/9996` or `#9996` +- **Branch name** (optional): If not provided, extract from PR metadata + +## Workflow + +### 1. Study the PR + +Fetch the PR via `FetchUrl` (or `gh pr view`) to get: +- File changes (diffs) +- Reviewer comments (inline and general) +- CI workflow status and logs +- PR description and any linked ticket + +Read all changed files in depth. Understand the intent and scope of the change. + +**If the PR fixes a specific issue** (e.g. error report, user-reported bug): investigate the root cause before reviewing the code. Confirm the fix addresses the actual problem. PRs are sometimes already superseded by other fixes. + +### 2. Fetch and Check Out the Branch + +```bash +git fetch origin +git checkout +``` + +If the branch doesn't exist locally, `git checkout` will create a tracking branch from `origin/`. + +### 3. Rebase on Latest Base Branch + +```bash +git pull origin --rebase +``` + +**If conflicts occur:** +1. Read each conflicted file to understand both sides +2. Resolve conflicts, preserving the PR's intent while incorporating base-branch changes +3. `git add ` +4. `git rebase --continue` + +**If rebase is clean:** Verify the state with `git log --oneline -5` and `git diff --stat origin/..HEAD`. + +### 4. Address Reviewer Comments + +Review comments were already fetched in step 1. For each unresolved comment: +1. Read the comment and understand what's being asked +2. Make the code change (or explain why it's not needed) +3. Add tests if requested +4. Commit the fix with a descriptive message + +**Already-addressed comments:** Check if a reply already exists (`in_reply_to_id` field). Skip comments that have been resolved. + +### 5. Run Local CI Checks + +Run the project's lint, format, typecheck/compile, and test commands for the affected areas. Discover them by reading the repo root (e.g. `Makefile`, `package.json`, `pyproject.toml`, `Cargo.toml`, `go.mod`, `build.gradle`, `mix.exs`, `Gemfile`, `composer.json`, `justfile`, `Taskfile.yml`, `README.md`, or the CI workflow config). + +Prefer filter / target flags to scope runs to the affected areas — it is faster than running the whole repo. Common patterns: + +- JS/TS monorepos: `npm run test -- --filter=`, `pnpm -r --filter test`, `nx test ` +- Python: `pytest `, `tox -e ` +- Rust: `cargo test -p `, `cargo clippy -p ` +- Go: `go test .//...`, `golangci-lint run .//...` +- Java/Kotlin: `./gradlew ::test`, `./mvnw -pl test` +- Bazel: `bazel test //path/...` + +See the `create-pr` skill's "CI Checks Reference" section for a broader template of local commands matching common CI checks. + +**Distinguishing pre-existing failures from PR issues:** +Some CI failures exist on the base branch and are unrelated to the PR. If a failure occurs in a file not touched by the PR, verify it exists on the base branch too before spending time fixing it. Common pre-existing issues include module / package resolution errors for recently-added dependencies. + +**E2E tests:** If the PR changes user-facing behavior (UI flow, defaults, keyboard handling, CLI output), E2E tests may break even if the code is correct. Read the failing test to understand what it expects, then update it to match the new behavior. Don't assume E2E failures are flaky — read them first. + +### 6. Commit and Push + +```bash +git add -A +git commit -m "(): " + +git push origin --force-with-lease +``` + +**If a commit-scanning bot blocks the push** (Droid-Shield, GitGuardian, TruffleHog, etc.): This happens when unrelated test fixtures contain strings that look like secrets. The agent cannot override these. Tell the user to push manually or temporarily disable the scanner per your org's docs. + +### 7. Reply to Reviewer Comments + +Reply to each addressed comment on the PR so reviewers know their feedback was handled. + +**For inline review comments** (the most common type): +```bash +# Reply to a specific inline comment thread +gh api repos///pulls//comments//replies \ + -X POST \ + -f body="" +``` + +**For general PR-level summary:** +```bash +gh pr comment --body "" +``` + +**To check thread resolution status** (optional): +```graphql +gh api graphql -f query='{ + repository(owner: "", name: "") { + pullRequest(number: ) { + reviewThreads(first: 20) { + nodes { + isResolved + comments(first: 3) { nodes { body author { login } } } + } + } + } + } +}' +``` + +### 8. Update PR Description + +If the changes made during follow-up are significant (new tests, architectural changes, additional scope), update the PR description: + +```bash +gh pr edit --body "" +``` + +Use your org's PR template format. Update the testing section to reflect the additional tests added. + +## Verification + +Before considering the task complete, confirm: +- [ ] Branch is rebased on the latest base branch +- [ ] All reviewer comments are addressed with code changes +- [ ] Local lint, typecheck/compile, and tests pass for affected packages +- [ ] Changes are pushed to remote +- [ ] All reviewer comments have replies explaining what was done +- [ ] PR description is up to date diff --git a/plugins/core/skills/simplify/SKILL.md b/plugins/core/skills/simplify/SKILL.md new file mode 100644 index 0000000..aa8ea43 --- /dev/null +++ b/plugins/core/skills/simplify/SKILL.md @@ -0,0 +1,53 @@ +--- +name: simplify +description: Review changed code for reuse, quality, and efficiency, then fix any issues found. +--- + +# Simplify: Code Review and Cleanup + +Review all changed files for reuse, quality, and efficiency. Fix any issues found. + +## Phase 1: Identify Changes + +Run `git diff` (or `git diff HEAD` if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation. + +## Phase 2: Launch Three Review Agents in Parallel + +Use the Task tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context. + +### Agent 1: Code Reuse Review + +For each change: + +1. **Search for existing utilities and helpers** that could replace newly written code. Look for similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones. +2. **Flag any new function that duplicates existing functionality.** Suggest the existing function to use instead. +3. **Flag any inline logic that could use an existing utility** — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates. + +### Agent 2: Code Quality Review + +Review the same changes for hacky patterns: + +1. **Redundant state**: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls +2. **Parameter sprawl**: adding new parameters to a function instead of generalizing or restructuring existing ones +3. **Copy-paste with slight variation**: near-duplicate code blocks that should be unified with a shared abstraction +4. **Leaky abstractions**: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries +5. **Stringly-typed code**: using raw strings where constants, enums (string unions), or branded types already exist in the codebase +6. **Unnecessary JSX nesting**: wrapper Boxes/elements that add no layout value — check if inner component props (flexShrink, alignItems, etc.) already provide the needed behavior + +### Agent 3: Efficiency Review + +Review the same changes for efficiency: + +1. **Unnecessary work**: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns +2. **Missed concurrency**: independent operations run sequentially when they could run in parallel +3. **Hot-path bloat**: new blocking work added to startup or per-request/per-render hot paths +4. **Recurring no-op updates**: state/store updates inside polling loops, intervals, or event handlers that fire unconditionally — add a change-detection guard so downstream consumers aren't notified when nothing changed. Also: if a wrapper function takes an updater/reducer callback, verify it honors same-reference returns (or whatever the "no change" signal is) — otherwise callers' early-return no-ops are silently defeated +5. **Unnecessary existence checks**: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error +6. **Memory**: unbounded data structures, missing cleanup, event listener leaks +7. **Overly broad operations**: reading entire files when only a portion is needed, loading all items when filtering for one + +## Phase 3: Fix Issues + +Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it. + +When done, briefly summarize what was fixed (or confirm the code was already clean). diff --git a/plugins/debugging/.factory-plugin/plugin.json b/plugins/debugging/.factory-plugin/plugin.json new file mode 100644 index 0000000..98c71a4 --- /dev/null +++ b/plugins/debugging/.factory-plugin/plugin.json @@ -0,0 +1,8 @@ +{ + "name": "debugging", + "description": "Inspect runtime behavior: HTTP interception, traffic capture, and wire-level debugging for CLIs and services", + "author": { + "name": "Factory", + "email": "support@factory.ai" + } +} diff --git a/plugins/debugging/README.md b/plugins/debugging/README.md new file mode 100644 index 0000000..8b80d68 --- /dev/null +++ b/plugins/debugging/README.md @@ -0,0 +1,15 @@ +# debugging + +Tools for inspecting what code actually does at runtime: HTTP traffic, wire-level behavior, and process-level traces. + +## Skills + +### `http-toolkit-intercept` + +Intercept and debug HTTP traffic from any CLI, service, or script using HTTP Toolkit. Works with any runtime (Node.js, Bun, Deno, Python, Go, Ruby, Java, .NET, Rust, shell, etc.) that respects a proxy. Pairs the outbound proxy log with a machine-readable session log so you can verify LLM API calls, backend requests, auth flows, and wire-level behavior. + +## Install + +```bash +droid plugin install debugging@factory-plugins +``` diff --git a/plugins/debugging/skills/http-toolkit-intercept/SKILL.md b/plugins/debugging/skills/http-toolkit-intercept/SKILL.md new file mode 100644 index 0000000..3c50b8d --- /dev/null +++ b/plugins/debugging/skills/http-toolkit-intercept/SKILL.md @@ -0,0 +1,179 @@ +--- +name: http-toolkit-intercept +description: Intercept and debug HTTP traffic from any CLI, service, or script using HTTP Toolkit. Use when you need to inspect LLM API calls, backend requests, auth flows, or debug network-level issues across any language or runtime. +--- + +# HTTP Toolkit Intercept + +Use this skill when you need authoritative evidence of what your program sent to a remote API and what it received back while verifying a code change. Works across any runtime — Node.js, Bun, Deno, Python, Go, Ruby, Java/JVM, .NET, PHP, Rust, shell scripts, etc. — as long as the process respects a proxy. + +The reliable pattern is: + +1. Start HTTP Toolkit correctly. +2. Run the program through the proxy in a mode that produces a machine-readable log (e.g. `--output-format json`, debug logging, or structured stdout). +3. Export outbound HTTP requests from HTTP Toolkit. +4. Pair the outbound HTTP export with the inbound program session log. + +Do not rely on TUI screenshots alone when the question is about request payloads, auth headers, or wire-level behavior. + +## Prerequisites / Known-Good Launch + +### Start HTTP Toolkit + +On Linux/headless environments, plain `httptoolkit` often fails due to sandbox/X11 issues. Prefer: + +```bash +xvfb-run --auto-servernum httptoolkit --no-sandbox +``` + +If a stale server is already running on ports `45456/45457`, stop it first: + +```bash +pkill -f "HTTP Toolkit Server|httptoolkit|xvfb-run --auto-servernum httptoolkit" || true +``` + +### Verify the proxy is reachable + +```bash +# HTTP Toolkit's admin API lives on port 45456/45457 by default. +# Treat 200/401/403 as "reachable"; only connection failure means the server is dead. +curl -s -o /dev/null -w "%{http_code}\n" http://127.0.0.1:45456/config +``` + +## Quick Start + +### 1. Launch your program with the proxy env vars set + +The canonical env vars most runtimes honor: + +```bash +HTTP_PROXY="http://127.0.0.1:8000" \ +HTTPS_PROXY="http://127.0.0.1:8000" \ +ALL_PROXY="http://127.0.0.1:8000" \ +NO_PROXY="" \ + +``` + +Some runtimes require an extra env var or flag — see the "Runtime proxy matrix" below. + +> If the HTTP Toolkit CA is not trusted by your runtime, TLS verification will fail. See "TLS Safety" below. Disabling TLS verification is appropriate only for controlled local debugging. + +### 2. Capture the inbound session log + +If your program supports a machine-readable output mode (e.g. `--output-format stream-json`, `--json`, `--log-level debug`, structured stdout, or writing to a file), pipe it to a file: + +```bash + exec --output-format stream-json "your input" \ + > /tmp/session-stdout.log 2> /tmp/session-stderr.log +``` + +### 3. Export outbound HTTP from HTTP Toolkit + +Either: + +- Use the HTTP Toolkit GUI export (File → Export → JSON / HAR), or +- Hit the admin API directly. The exact endpoint depends on your HTTP Toolkit version; inspect DevTools in the HTTP Toolkit UI to see the requests it makes. + +### 4. Cross-reference the two streams + +- **Outbound HTTP** (from HTTP Toolkit): authoritative for request bodies, headers, auth tokens, retry timing. +- **Inbound session log** (from the program): authoritative for how your code reacted to the responses. + +Together they answer: "what did we send?" and "what did we do with the response?" + +## What finally worked for payload verification + +The critical correct pathways that proved reliable were: + +1. **Use a non-interactive / exec mode, not the TUI, when verifying payloads** + - Interactive TUIs are slower and much harder to analyze. + - Use whatever your program has for scripting (`--output-format`, `--json`, `--headless`, `--batch`). + +2. **Treat outbound and inbound as separate evidence sources** + - HTTP Toolkit gives outbound HTTP requests. + - The program's session log gives inbound assistant/tool/application behavior. + - You need both to answer: "what did the remote return?" and "what did the program actually do with it?" + +3. **Set the proxy env vars your specific runtime honors** + - `HTTP_PROXY`/`HTTPS_PROXY` cover most runtimes, but some (e.g. Bun, Java) require extra vars or flags. + - See the "Runtime proxy matrix" below. + +4. **Disable TLS verification only for controlled local debugging when needed** + - If the HTTP Toolkit CA is not trusted locally, use the runtime-specific escape hatch to skip verification. + - Prefer trusting the CA in your OS / language trust store instead. + - Never disable TLS in production repros. + +5. **Keep runs bounded** + - Long network-heavy sessions can take time. + - If you only need to prove request shape, export after the relevant request is observed — you do not always need to wait for full completion. + +## Key Facts + +- **Proxy env vars are runtime-specific** — know which ones your runtime honors +- **HTTP Toolkit admin API is request-oriented** — outbound HTTP comes from HTTP Toolkit, inbound behavior comes from the program log +- **TLS is verified by default** — runtime-specific skip flags are local-dev escape hatches only + +## Runtime proxy matrix + +| Runtime | Proxy env vars / flags | TLS bypass (local dev only) | +|---------|------------------------|------------------------------| +| **Node.js** | `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` (most libraries); some HTTP clients need `--proxy` flags or explicit `agent:` option | `NODE_TLS_REJECT_UNAUTHORIZED=0` or `NODE_EXTRA_CA_CERTS=/path/to/ca.pem` | +| **Bun** | `BUN_CONFIG_PROXY` (mandatory — plain `HTTP_PROXY`/`HTTPS_PROXY` are silently ignored for Bun's own fetch) | `NODE_TLS_REJECT_UNAUTHORIZED=0` | +| **Deno** | `HTTP_PROXY`, `HTTPS_PROXY` | `--unsafely-ignore-certificate-errors` | +| **Python (`requests`, `httpx`)** | `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY` | `REQUESTS_CA_BUNDLE` / `SSL_CERT_FILE` pointing at the HTTP Toolkit CA, or `verify=False` in code | +| **Python (`urllib`)** | Same env vars | `SSL_CERT_FILE` or disable context verification | +| **Go (`net/http`)** | `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` (honored via `http.ProxyFromEnvironment`) | `SSL_CERT_FILE` or `InsecureSkipVerify: true` in the transport | +| **Ruby** | `HTTP_PROXY`, `HTTPS_PROXY` | `SSL_CERT_FILE`, or `OpenSSL::SSL::VERIFY_NONE` | +| **Java / JVM** | `-Dhttp.proxyHost=127.0.0.1 -Dhttp.proxyPort=8000 -Dhttps.proxyHost=127.0.0.1 -Dhttps.proxyPort=8000`; env vars are **not** honored by the JVM | Import CA into a truststore and pass `-Djavax.net.ssl.trustStore=...` | +| **.NET / C#** | `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY` (on recent runtimes) or configure `HttpClient` explicitly | Trust CA in OS store, or `HttpClientHandler.ServerCertificateCustomValidationCallback` | +| **PHP (curl / cli)** | `HTTP_PROXY`, `HTTPS_PROXY` (or per-call `CURLOPT_PROXY`) | `CURLOPT_SSL_VERIFYPEER => false` | +| **Rust (`reqwest`)** | `HTTP_PROXY`, `HTTPS_PROXY` | `danger_accept_invalid_certs(true)` on the client | +| **curl / shell** | `HTTP_PROXY`, `HTTPS_PROXY` env vars or `-x`/`--proxy` flag | `-k` / `--insecure` | +| **Docker containers** | Pass env vars through with `-e HTTP_PROXY=...`; use `host.docker.internal` (Mac/Win) or `--network=host` (Linux) so the container can reach the proxy | Mount the CA into the image and install it, or use runtime-specific bypass flags | + +Prefer trusting the HTTP Toolkit CA in your runtime/OS trust store over disabling verification. Bypass flags should be local-dev only. + +### TLS Safety Guardrails + +- Keep TLS verification enabled whenever possible. +- Prefer trusting the HTTP Toolkit CA in your local trust store instead of disabling verification. +- Use runtime-specific TLS-bypass flags only for controlled local debugging in development. +- Never disable TLS when intercepting production traffic. + +## Inspecting Captured Logs + +### Filter to relevant events only + +If your program emits newline-delimited JSON, use `jq`: + +```bash +jq -c 'select(.type == "tool_call" or .type == "message")' /tmp/session-stdout.log +``` + +Adjust the filter to match your program's event schema. For plain-text logs, use `rg`/`grep` patterns. + +### Match outbound HTTP to inbound events + +Sort both streams by timestamp, then interleave them. The sequence usually looks like: + +``` +outbound POST /api/endpoint (from HTTP Toolkit) +inbound event received (from program log) +inbound follow-up action (from program log) +outbound POST /api/endpoint (next request) +``` + +If a program log shows an outbound request that HTTP Toolkit didn't capture, that's a proxy-config bug. + +## Troubleshooting + +| Problem | Cause | Fix | +|---------|-------|-----| +| Program hangs, no events after startup | Proxy env vars not reaching the process, or TLS verification blocking | Re-run with the right env vars for your runtime (see matrix); if necessary, enable the runtime's TLS bypass in dev | +| `ECONNRESET` / `connection reset` on every request | Runtime-specific proxy env var missing (e.g. `BUN_CONFIG_PROXY` for Bun, JVM `-D` flags for Java) | Use the correct runtime-specific proxy config | +| TLS cert errors via proxy | MITM CA not trusted by this runtime | Trust HTTP Toolkit CA in the runtime / OS store, or enable the runtime's TLS bypass in dev only | +| HTTP Toolkit API 403s on `/config` | Auth-gated config endpoint | Treat 200/401/403 as reachable; only connection failure means the server is dead | +| Export has outbound data but no matching inbound events | Didn't capture the program log | Add `> /tmp/session.log` redirection to the launch | +| HTTP Toolkit misses the first request | Started capturing after the process launched | Start HTTP Toolkit first, THEN launch the program | +| Container / VM can't reach `127.0.0.1:8000` | Loopback is container-local | Use `host.docker.internal` (Docker Desktop) or `--network=host` (Linux) | +| Program ignores env vars entirely | Runtime doesn't honor env vars (e.g. JVM) | Use runtime-specific flags (`-Dhttp.proxyHost=...` for JVM, etc.) | diff --git a/plugins/security-engineer/.factory-plugin/plugin.json b/plugins/security-engineer/.factory-plugin/plugin.json index 9d984f2..cf2dd82 100644 --- a/plugins/security-engineer/.factory-plugin/plugin.json +++ b/plugins/security-engineer/.factory-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "security-engineer", - "description": "Security review, threat modeling, and vulnerability validation skills", + "description": "Security review, threat modeling, vulnerability validation, and patch generation skills", "author": { "name": "Factory", "email": "support@factory.ai" diff --git a/plugins/security-engineer/skills/security-review/SKILL.md b/plugins/security-engineer/skills/security-review/SKILL.md index 7d81191..3666ecd 100644 --- a/plugins/security-engineer/skills/security-review/SKILL.md +++ b/plugins/security-engineer/skills/security-review/SKILL.md @@ -7,7 +7,7 @@ tags: [security, vulnerability, STRIDE, CVE, audit, review] # Security Review -You are a senior security engineer conducting a focused security review using LLM-powered reasoning and STRIDE threat modeling. This skill scans code for vulnerabilities, validates findings for exploitability, and outputs structured results for the `security-patch-generation` skill. +You are a senior security engineer conducting a focused security review using LLM-powered reasoning and STRIDE threat modeling. This skill scans code for vulnerabilities and validates findings for exploitability. ## When to Use This Skill @@ -497,11 +497,6 @@ Include: - [ ] Results output in appropriate format (PR comments or report) - [ ] Severity actions applied -## Downstream Skills - -After this skill completes with CONFIRMED findings: -- **`security-patch-generation`** - Generate fixes, tests, and PR - ## Example Invocations **PR security review:** diff --git a/plugins/security-engineer/skills/vulnerability-validation/SKILL.md b/plugins/security-engineer/skills/vulnerability-validation/SKILL.md index 10a9139..bb2f914 100644 --- a/plugins/security-engineer/skills/vulnerability-validation/SKILL.md +++ b/plugins/security-engineer/skills/vulnerability-validation/SKILL.md @@ -309,4 +309,3 @@ Validate findings in security-findings.json using threat model at .factory/threa - OWASP Testing Guide: https://owasp.org/www-project-web-security-testing-guide/ - Examples: `validation-examples.md` (in this skill directory) - Upstream: `commit-security-scan` skill -- Downstream: `security-patch-generation` skill diff --git a/plugins/typescript/.factory-plugin/plugin.json b/plugins/typescript/.factory-plugin/plugin.json new file mode 100644 index 0000000..b0e7e5f --- /dev/null +++ b/plugins/typescript/.factory-plugin/plugin.json @@ -0,0 +1,8 @@ +{ + "name": "typescript", + "description": "Opinionated TypeScript and React patterns: ban `as` assertions, replace useEffect with derived state, and fix knip unused exports", + "author": { + "name": "Factory", + "email": "support@factory.ai" + } +} diff --git a/plugins/typescript/README.md b/plugins/typescript/README.md new file mode 100644 index 0000000..217e5a7 --- /dev/null +++ b/plugins/typescript/README.md @@ -0,0 +1,23 @@ +# typescript + +Opinionated TypeScript and React patterns for cleaner, safer code. + +## Skills + +### `ban-type-assertions` + +Enable `@typescript-eslint/consistent-type-assertions` with `assertionStyle: 'never'` and replace every `as X` cast with patterns the compiler can verify. Covers zod parsing for data boundaries, control-flow narrowing for unions, and the narrow cases where `eslint-disable` is acceptable. + +### `no-use-effect` + +Five replacement patterns for `useEffect`: derived state, query libraries, event handlers, `useMountEffect` for one-time external sync, and the `key` prop for state resets. Based on Alvin Sng's [tweet](https://x.com/alvinsng/status/2033969062834045089) and the React docs [You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect). + +### `fix-knip-unused-exports` + +Fix every category of knip "Unused exports" violation: extract test-only exports into new files, remove dead barrel re-exports, un-export internally-used symbols, and delete dead code. Explains how knip traces usage across a monorepo and how to handle cross-package test imports. + +## Install + +```bash +droid plugin install typescript@factory-plugins +``` diff --git a/plugins/typescript/skills/ban-type-assertions/SKILL.md b/plugins/typescript/skills/ban-type-assertions/SKILL.md new file mode 100644 index 0000000..bd01361 --- /dev/null +++ b/plugins/typescript/skills/ban-type-assertions/SKILL.md @@ -0,0 +1,208 @@ +--- +name: ban-type-assertions +description: Ban `as` type assertions in a package via the `@typescript-eslint/consistent-type-assertions` lint rule, replacing them with compiler-verified type-safe alternatives. Use when enabling the assertion ban in a new package or fixing violations in an existing one. +--- + +# Ban Type Assertions + +Enable `@typescript-eslint/consistent-type-assertions` with `assertionStyle: 'never'` in a package and replace all `as X` casts with patterns the compiler can verify. + +## Core Philosophy + +> Pick the strictly correct path, not the simpler one. + +Every `as` assertion is a spot where the developer told the compiler "trust me." The goal is to make the compiler *verify* instead. If you replace `as Foo` with a type guard that is equally unverified, you have not improved anything -- you have just moved the assertion. + +## Quick Reference + +- Rule: `@typescript-eslint/consistent-type-assertions` +- Config: `{ assertionStyle: 'never' }` +- Location: `packages//.eslintrc.js` + +## Workflow + +### 1. Enable the Rule + +Add to the package's `.eslintrc.js`: + +```js +rules: { + '@typescript-eslint/consistent-type-assertions': ['error', { assertionStyle: 'never' }], +} +``` + +### 2. Enumerate Violations + +```bash +cd packages/ && npm run lint 2>&1 | grep "consistent-type-assertions" +``` + +Group violations by file and pattern before fixing. + +### 3. Research Before Fixing + +Before writing any replacement code: + +1. **Check for existing zod schemas** -- grep for `Schema` alongside the type name in `@factory/common` and across the repo. +2. **Check if schemas exist but aren't exported** -- if so, export them rather than creating new ones. +3. **Check for duplicate types/interfaces** across packages -- consolidate into `@factory/common` if found. +4. **Understand the data flow** -- is this a parse boundary (external data), a narrowing site (union type), or a library type gap? + +### 4. Fix Violations Using the Pattern Hierarchy + +#### Tier 1: Zod Parsing (for external data boundaries) + +Use for any data entering the system from JSON, disk, network, IPC, etc. This gives **runtime validation**, not just a type annotation. + +```typescript +// BAD +const data = JSON.parse(raw) as MyType; + +// GOOD +const data = MySchema.parse(JSON.parse(raw)); +``` + +Use `safeParse` when you need to handle errors gracefully (e.g., returning an error response with context like a request id): + +```typescript +// BAD: throws before you can extract the request id +const request = RequestSchema.parse(JSON.parse(raw)); + +// GOOD: safeParse lets you return a proper error +const parsed = RequestSchema.safeParse(JSON.parse(raw)); +if (!parsed.success) { + return errorResponse(rawObj?.id ?? null, INVALID_PARAMS, parsed.error.message); +} +const request = parsed.data; +``` + +#### Tier 2: Control Flow Narrowing (for union types) + +Use `switch`, `in`, `instanceof`, or discriminated unions: + +```typescript +// BAD +(error as NodeJS.ErrnoException).code + +// GOOD +if (error instanceof Error && 'code' in error) { + const code = error.code; +} +``` + +```typescript +// BAD +if (METHODS.has(method as Method)) { ... } + +// GOOD: switch narrows exhaustively +switch (method) { + case 'foo': + case 'bar': + return handle(method); // narrowed +} +``` + +#### Tier 3: eslint-disable with Justification (last resort) + +Only for genuinely unavoidable cases (library type gaps, generic parameters that can't be inferred). Always explain *why*: + +```typescript +// eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- ws library types require generic parameter +ws.on('message', handler); +``` + +#### Anti-Pattern: Type Guards That Are Disguised Assertions + +```typescript +// NOT an improvement -- checks shape but not content +function isDaemonRequest(x: unknown): x is DaemonRequest { + return typeof x === 'object' && x !== null && 'method' in x; +} +``` + +A zod schema validates values. A type guard like this is an unverified assertion with extra steps. Only use type guards when the narrowing logic is truly sufficient. + +### 5. Use Strict Schemas, Not Permissive Ones + +When a schema exists (e.g., `SessionSettingsSchema`), use it strictly rather than `z.record(z.unknown())`. This ensures forward compatibility -- if fields are removed in a migration, stale data gets cleaned on read. + +```typescript +// BAD: accepts anything +const settings = z.record(z.unknown()).parse(raw); + +// GOOD: validates against the real shape +const settings = SessionSettingsSchema.parse(raw); +``` + +### 6. Promote Shared Schemas to `@factory/common` + +If you find duplicate interfaces, types, or schemas across packages, consolidate them: + +1. Create the schema in `@factory/common///schema.ts` +2. Put any enums in a sibling `enums.ts` (required by `factory/enum-file-organization`) +3. Export via a **subpath** (e.g., `@factory/common/session/summary`), **not** the barrel `index.ts` +4. Delete all local duplicates +5. Update all consumers to import from the common subpath +6. Run `npm run knip` at repo root to catch unused barrel re-exports + +### 7. Fix Test Mocks to Match Schemas + +Once you replace `as X` with `.parse()`, test mocks that relied on the assertion will fail validation. Fix the mocks -- do not disable the rule in tests. + +Create helper functions to centralize valid test fixtures: + +```typescript +function mockSessionSummary( + overrides?: Partial, +): SessionSummaryEvent { + return { + type: 'session_start', + id: 'test-id', + title: 'Test Session', + owner: 'test-owner', + ...overrides, + }; +} +``` + +### 8. Parse at the Boundary, Inside Error Handling + +Make sure parsing happens where failures produce proper error responses, not unhandled exceptions: + +```typescript +// BAD: parse outside try/catch -- if it throws, you lose context +const request = RequestSchema.parse(data); +try { handle(request); } catch { ... } + +// GOOD: safeParse before try, handle error with context +const parsed = RequestSchema.safeParse(data); +if (!parsed.success) { + return errorResponse(rawData?.id ?? null, INVALID_PARAMS, parsed.error.message); +} +try { handle(parsed.data); } catch { ... } +``` + +## Verification + +Run for **all affected packages** (a change in `@factory/common` can break downstream lint): + +```bash +# Lint (all affected packages) +cd packages/ && npm run lint + +# Typecheck +npm run typecheck + +# Tests +npm run test + +# Unused exports (repo root) +npm run knip +``` + +## Reminders + +- `factory/enum-file-organization` requires TypeScript enums to live in files named `enums.ts` +- `no-barrel-files` prevents re-exporting types from barrel files -- consumers must import from the subpath directly +- When promoting types to common, add a `package.json` exports entry for the new subpath if one doesn't exist +- Test overrides for the rule in `.eslintrc.js` may be needed if test files use assertion syntax in mock setup -- but prefer fixing mocks over disabling the rule diff --git a/plugins/typescript/skills/fix-knip-unused-exports/SKILL.md b/plugins/typescript/skills/fix-knip-unused-exports/SKILL.md new file mode 100644 index 0000000..755e9b6 --- /dev/null +++ b/plugins/typescript/skills/fix-knip-unused-exports/SKILL.md @@ -0,0 +1,259 @@ +--- +name: fix-knip-unused-exports +description: | + Fix knip "Unused exports" violations. Handles all violation categories: test-only exports + (extract to new file), dead barrel re-exports (remove from index.ts), and internally-only-used + exports (un-export). + Use when `npm run knip` reports unused exports. +--- + +# Fix Knip Unused Exports + +Fix knip "Unused exports" violations. There are several categories of violation, each with a different fix strategy. + +## When to Use + +- `npm run knip` reports "Unused exports" + +## When NOT to Use + +- The export is consumed by non-test production code in another file -- something else is wrong + +## Workflow + +### 1. Identify Violations + +```bash +npm run knip +``` + +Output looks like: +``` +Unused exports (3) +::error file=packages/foo/src/bar.ts,line=42,title=Unused exports::myFunction +``` + +### 2. Classify Each Violation + +For each flagged export, grep the **entire repository** (not just the package): + +```bash +rg "myFunction" +``` + +Determine which category it falls into: + +| Category | Callers | Fix | +|----------|---------|-----| +| **Test-only export** | Used in same file + test files only | Extract to new file | +| **Dead barrel re-export** | Re-exported from `index.ts`, but production code imports via relative paths or other subpaths instead | Remove the re-export from the barrel | +| **Internally-only-used export** | Used only within the same file, not by tests or other files | Remove the `export` keyword | +| **Dead code** | No callers anywhere | Delete the export | +| **Production consumer exists** | Used by non-test code in another file | Not a knip issue -- investigate further | + +**Important**: When grepping, exclude test files to identify production consumers: +```bash +rg "myFunction" --glob '!**/*.test.*' +``` + +## Fix: Test-Only Exports (Extract to New File) + +When a function is exported solely for test access but is also used internally in the same file. + +### Plan the Extraction + +Before writing code, answer these questions: + +**a) What moves to the new file?** +- The flagged export function/class/const +- All private helper functions it depends on +- All private constants/types it depends on + +**b) Are any helpers shared with functions staying behind?** +- If yes, the helper must be exported from the new file, and the original file imports it +- This means the new file will have 2+ exports (which is fine for any filename-match-export lint rule) + +**c) Will the new file have exactly one exported function?** +- If your project enforces a `filename-match-export` lint rule, the file MUST be named after that export: `myFunction.ts` +- If the file has 2+ function exports, the name is flexible + +**d) Does a test file with a matching name exist?** +- If `bar.ts` stays and `bar.test.ts` exists, the test must still import something from `./bar` (if your project enforces a `test-imports-source` rule) +- If `bar.ts` is deleted (everything moved out), that rule typically only applies when the matching source file exists + +**e) Any circular dependency risk?** +- Draw the import graph: new file -> original file -> new file is circular +- Fix: move the shared dependency to the new file or a third file + +**f) Does it export a constant?** +- If your project enforces a `constants-file-organization` lint rule, exported constants must live in a file named `constants.ts` +- If the extracted function depends on a constant that other functions in the original file also use, do NOT export the constant from the new file. Instead, call the function (e.g., replace `BUDGET[effort]` with `getBudget(effort)`) to avoid needing a separate `constants.ts` + +### Execute the Extraction + +Create the new file in the same directory: + +```typescript +// myFunction.ts (new file) +import { SomeType } from '../types'; + +function privateHelper(): void { /* ... */ } + +export function myFunction(): SomeType { + return privateHelper(); +} +``` + +Update the original file to import from the new file: + +```typescript +// bar.ts (original file, updated) +import { myFunction } from './myFunction'; + +function otherFunction() { + const result = myFunction(); // Now imports from new file +} +``` + +Update test files to import from the new file: + +```typescript +// bar.test.ts (updated) +import { myFunction } from './myFunction'; +// If bar.ts still exists, you may need to also import something from './bar' +// to satisfy any test-imports-source rule +``` + +### Watch for Chained Violations + +After extracting, run `npm run knip` again. If function A was extracted to a new file alongside function B that A calls, but B is also only consumed by tests externally, knip will flag B too. You need to extract B to its own file so that A's file creates a genuine production import of B. + +Example: suppose `throwMappedError` was first extracted alongside `mapResponseFailure` into `error-mappers.ts`. If `throwMappedError` is only called internally within that file (by `mapResponseFailure`), it will still be flagged. Fix: extract it to `throwMappedError.ts`, making the import from `error-mappers.ts` a genuine production consumer. + +## Fix: Dead Barrel Re-Exports (Remove from index.ts) + +When a barrel `index.ts` re-exports something, but no production code imports it through the barrel. This happens when: +- Production code within the same package uses relative imports (e.g., `import { x } from './source'`) instead of the barrel +- Production code in other packages imports directly from a subpath (e.g., `@scope/pkg/feature/handlers`) instead of the barrel +- The re-export was added speculatively but never consumed + +### How to Identify + +Grep excluding test files. If the only hits are: +- The barrel `index.ts` itself +- Source files using relative imports within the same package +- Test files + +Then the barrel re-export is unused. Simply remove it from `index.ts`. + +### Cross-Package Test Imports + +If a test in another package imports the symbol through the barrel (e.g., `import { x } from '@scope/pkg/feature'`), you need to provide an alternative import path after removing the barrel re-export: + +1. Add a **subpath export** in the source package's `package.json`: + ```json + { + "exports": { + "./feature": "./src/feature/index.ts", + "./feature/doSomething": "./src/feature/doSomething.ts" + } + } + ``` + +2. Update the test to import from the new subpath: + ```typescript + import { doSomething } from '@scope/pkg/feature/doSomething'; + ``` + +This pattern follows typical subpath-export conventions used in monorepos. + +## Fix: Internally-Only-Used Exports (Un-export) + +When an export is only used within the same file and not imported by anything else (not even tests), just remove the `export` keyword: + +```typescript +// Before +export const MySchema = z.object({ ... }); + +// After +const MySchema = z.object({ ... }); +``` + +This is common for Zod schemas that are only used as building blocks for other schemas in the same file. + +## Verify + +Run ALL of these checks on the affected packages: + +```bash +# Knip passes (the whole point) +npm run knip + +# Types still compile +npm run typecheck + +# Tests still pass +npm run test + +# Lint passes (catches filename-match-export, test-imports-source, constants-file-organization, etc.) +npm run lint +``` + +If cross-package imports exist, also verify the consuming package. + +## Interacting Lint Rules + +Many TypeScript monorepos layer additional custom lint rules on top of knip. Adapt the fixes below to whichever of these your project uses. + +### `filename-match-export` (or similar) + +If a file has exactly ONE exported function (not a React component), the filename must match the function name. + +- `export function loadConfig` in `loadConfig.ts` -- passes +- `export function loadConfig` in `helpers.ts` -- fails +- Two exports in `helpers.ts` -- rule does not apply (multiple exports) + +### `test-imports-source` (or similar) + +If `foo.test.ts` and `foo.ts` both exist, the test must import from `./foo`. + +- Imports like `import { x } from './foo'` satisfy the rule +- Typically also accepts importing from `'.'` or `'./index'` if `index.ts` re-exports from `foo.ts` +- If `foo.ts` is deleted, the rule does not apply + +### `constants-file-organization` (or similar) + +Exported constants must be defined in a file named `constants.ts`. + +- If you extract a function that depends on a shared constant, do NOT export the constant from the function's file +- Instead, replace direct constant access with function calls (e.g., `BUDGET[effort]` becomes `getBudget(effort)`) +- Or move the constant to a `constants.ts` file + +### How Knip Traces Exports + +- Knip ignores test files (`**/*.test.*`, `**/*.spec.*`) +- `ignoreIssues` in `knip.json` suppresses warnings ON the listed file, but does NOT make the source export "used" +- Barrel re-exports (`export { x } from './source'`) from an `index.ts` with `ignoreIssues` do NOT count as usage of the source export +- Only genuine imports from non-test, non-ignored project files count as usage +- `includeEntryExports: true` (if set) means exports from entry point files are checked too, so entry-point-style files (migrations, scripts) may need explicit `ignoreIssues` + +### Package Subpath Exports + +When removing barrel re-exports that cross-package tests relied on, add subpath exports to `package.json`: + +```json +{ + "exports": { + "./feature": "./src/feature/index.ts", + "./feature/doSomething": "./src/feature/doSomething.ts" + } +} +``` + +## What Not to Do + +- Do not add files to `ignoreIssues` in `knip.json` unless they are genuine entry point scripts (migrations, CLIs) +- Do not merge all functions into one file to reduce exports -- same-file usage of an export does not count as usage from knip's perspective +- Do not remove the `export` keyword if tests need it -- the tests would break +- Do not create circular imports between the new and original files +- Do not export constants from non-`constants.ts` files if your project enforces a `constants-file-organization` lint rule diff --git a/plugins/typescript/skills/no-use-effect/SKILL.md b/plugins/typescript/skills/no-use-effect/SKILL.md new file mode 100644 index 0000000..ffc16d0 --- /dev/null +++ b/plugins/typescript/skills/no-use-effect/SKILL.md @@ -0,0 +1,233 @@ +--- +name: no-use-effect +description: >- + Enforce the no-useEffect rule when writing or reviewing React code. + ACTIVATE when writing React components, refactoring existing useEffect calls, + reviewing PRs with useEffect, or when an agent adds useEffect "just in case." + Provides the five replacement patterns and the useMountEffect escape hatch. +--- + +# No useEffect + +Never call `useEffect` directly. Use derived state, event handlers, data-fetching libraries, or `useMountEffect` instead. + +## Quick Reference + +- Lint rule: `no-restricted-syntax` (configured to ban `useEffect`) +- React docs: [You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect) +- Origin: [https://x.com/alvinsng/status/2033969062834045089](https://x.com/alvinsng/status/2033969062834045089) + +| Instead of useEffect for... | Use | +| --- | --- | +| Deriving state from other state/props | Inline computation (Rule 1) | +| Fetching data | `useQuery` / data-fetching library (Rule 2) | +| Responding to user actions | Event handlers (Rule 3) | +| One-time external sync on mount | `useMountEffect` (Rule 4) | +| Resetting state when a prop changes | `key` prop on parent (Rule 5) | + +## When to Use This Skill + +- Writing new React components +- Refactoring existing `useEffect` calls +- Reviewing PRs that introduce `useEffect` +- An agent adds `useEffect` "just in case" + +## Workflow + +### 1. Identify the useEffect + +Determine what the effect is doing -- deriving state, fetching data, responding to an event, syncing with an external system, or resetting state. + +### 2. Apply the Correct Replacement Pattern + +Use the five rules below to pick the right replacement. + +### 3. Verify + +``` +npm run lint -- --filter= +npm run typecheck -- --filter= +npm run test -- --filter= +``` + +## The Escape Hatch: useMountEffect + +For the rare case where you need to sync with an external system on mount: + +The implementation wraps `useEffect` with an empty dependency array to make intent explicit: + +``` +export function useMountEffect(effect: () => void | (() => void)) { + /* eslint-disable no-restricted-syntax */ + useEffect(effect, []); +} +``` + +## Replacement Patterns + +### Rule 1: Derive state, do not sync it + +Most effects that set state from other state are unnecessary and add extra renders. + +``` +// BAD: Two render cycles - first stale, then filtered +function ProductList() { + const [products, setProducts] = useState([]); + const [filteredProducts, setFilteredProducts] = useState([]); + + useEffect(() => { + setFilteredProducts(products.filter((p) => p.inStock)); + }, [products]); +} + +// GOOD: Compute inline in one render +function ProductList() { + const [products, setProducts] = useState([]); + const filteredProducts = products.filter((p) => p.inStock); +} +``` + +**Smell test:** You are about to write `useEffect(() => setX(deriveFromY(y)), [y])`, or you have state that only mirrors other state or props. + +### Rule 2: Use data-fetching libraries + +Effect-based fetching creates race conditions and duplicated caching logic. + +``` +// BAD: Race condition risk +function ProductPage({ productId }) { + const [product, setProduct] = useState(null); + + useEffect(() => { + fetchProduct(productId).then(setProduct); + }, [productId]); +} + +// GOOD: Query library handles cancellation/caching/staleness +function ProductPage({ productId }) { + const { data: product } = useQuery(['product', productId], () => + fetchProduct(productId) + ); +} +``` + +**Smell test:** Your effect does `fetch(...)` and then `setState(...)`, or you are re-implementing caching, retries, cancellation, or stale handling. + +### Rule 3: Event handlers, not effects + +If a user clicks a button, do the work in the handler. + +``` +// BAD: Effect as an action relay +function LikeButton() { + const [liked, setLiked] = useState(false); + + useEffect(() => { + if (liked) { + postLike(); + setLiked(false); + } + }, [liked]); + + return ; +} + +// GOOD: Direct event-driven action +function LikeButton() { + return ; +} +``` + +**Smell test:** State is used as a flag so an effect can do the real action, or you are building "set flag -> effect runs -> reset flag" mechanics. + +### Rule 4: useMountEffect for one-time external sync + +Good uses: DOM integration (focus, scroll), third-party widget lifecycles, browser API subscriptions. + +``` +// BAD: Guard inside effect +function VideoPlayer({ isLoading }) { + useEffect(() => { + if (!isLoading) playVideo(); + }, [isLoading]); +} + +// GOOD: Mount only when preconditions are met +function VideoPlayerWrapper({ isLoading }) { + if (isLoading) return ; + return ; +} + +function VideoPlayer() { + useMountEffect(() => playVideo()); +} +``` + +Use `useMountEffect` for stable dependencies (singletons, refs, context values that never change): + +``` +// BAD: useEffect with dependency that never changes +useEffect(() => { + connectionManager.on('connected', handleConnect); + return () => connectionManager.off('connected', handleConnect); +}, [connectionManager]); // connectionManager is a singleton from context + +// GOOD: useMountEffect for stable dependencies + +useMountEffect(() => { + connectionManager.on('connected', handleConnect); + return () => connectionManager.off('connected', handleConnect); +}); +``` + +**Smell test:** You are synchronizing with an external system, and the behavior is naturally "setup on mount, cleanup on unmount." + +### Rule 5: Reset with key, not dependency choreography + +``` +// BAD: Effect attempts to emulate remount behavior +function VideoPlayer({ videoId }) { + useEffect(() => { + loadVideo(videoId); + }, [videoId]); +} + +// GOOD: key forces clean remount +function VideoPlayer({ videoId }) { + useMountEffect(() => { + loadVideo(videoId); + }); +} + +function VideoPlayerWrapper({ videoId }) { + return ; +} +``` + +**Smell test:** You are writing an effect whose only job is to reset local state when an ID/prop changes, or you want the component to behave like a brand-new instance for each entity. + +## Component Structure Convention + +Computed values come after hooks and local state, never via `useEffect`: + +``` +export function FeatureComponent({ featureId }: ComponentProps) { + // Hooks first + const { data, isLoading } = useQueryFeature(featureId); + + // Local state + const [isOpen, setIsOpen] = useState(false); + + // Computed values (NOT useEffect + setState) + const displayName = user?.name ?? 'Unknown'; + + // Event handlers + const handleClick = () => { setIsOpen(true); }; + + // Early returns + if (isLoading) return ; + + // Render + return ...; +} +``` diff --git a/skills/ban-type-assertions b/skills/ban-type-assertions new file mode 120000 index 0000000..1f6d015 --- /dev/null +++ b/skills/ban-type-assertions @@ -0,0 +1 @@ +../plugins/typescript/skills/ban-type-assertions \ No newline at end of file diff --git a/skills/create-pr b/skills/create-pr new file mode 120000 index 0000000..f779513 --- /dev/null +++ b/skills/create-pr @@ -0,0 +1 @@ +../plugins/code-review/skills/create-pr \ No newline at end of file diff --git a/skills/fix-knip-unused-exports b/skills/fix-knip-unused-exports new file mode 120000 index 0000000..2e7556c --- /dev/null +++ b/skills/fix-knip-unused-exports @@ -0,0 +1 @@ +../plugins/typescript/skills/fix-knip-unused-exports \ No newline at end of file diff --git a/skills/follow-up-on-pr b/skills/follow-up-on-pr new file mode 120000 index 0000000..5194af8 --- /dev/null +++ b/skills/follow-up-on-pr @@ -0,0 +1 @@ +../plugins/code-review/skills/follow-up-on-pr \ No newline at end of file diff --git a/skills/http-toolkit-intercept b/skills/http-toolkit-intercept new file mode 120000 index 0000000..0ea49fe --- /dev/null +++ b/skills/http-toolkit-intercept @@ -0,0 +1 @@ +../plugins/debugging/skills/http-toolkit-intercept \ No newline at end of file diff --git a/skills/no-use-effect b/skills/no-use-effect new file mode 120000 index 0000000..0670c88 --- /dev/null +++ b/skills/no-use-effect @@ -0,0 +1 @@ +../plugins/typescript/skills/no-use-effect \ No newline at end of file diff --git a/skills/simplify b/skills/simplify new file mode 120000 index 0000000..6e6a3a0 --- /dev/null +++ b/skills/simplify @@ -0,0 +1 @@ +../plugins/core/skills/simplify \ No newline at end of file