diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 57b8eb0e1f..317ec89065 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -28,15 +28,29 @@ Do NOT leave comments about: Aim for fewer, higher-signal comments. A review with 2-3 important comments is better than 15 trivial ones. -## Instruction Files - -BEFORE editing or code-reviewing any file, you MUST read the `.github/instructions/` files whose `applyTo` patterns match the files you are about to edit. For example: -- Editing/code-reviewing `pyrit/**/*.py` → read `style-guide.instructions.md` and `user-custom.instructions.md` -- Editing/code-reviewing `pyrit/scenario/**` → also read `scenarios.instructions.md` -- Editing/code-reviewing `pyrit/prompt_converter/**` → also read `converters.instructions.md` -- Editing/code-reviewing `tests/**` → also read `test.instructions.md` -- Editing/code-reviewing `doc/**/*.py` or `doc/**/*.ipynb` → also read `docs.instructions.md` -- Editing/code-reviewing `frontend/**/*.{ts,tsx}` → also read `frontend-style-guide.instructions.md` -- Editing/code-reviewing `frontend/**/*.test.{ts,tsx}` → also read `frontend-test.instructions.md` - -Follow every rule in the applicable instruction files. Do not skip this step. +## Skills + +PyRIT ships subsystem-specific **skills** under `.github/skills/`. BEFORE you create, modify, or **review** code in one of these areas, invoke the matching skill and follow its contract: + +| Area (path) | Skill | +| --- | --- | +| `pyrit/prompt_converter/**` | `prompt-converter-development` | +| `pyrit/prompt_target/**` | `prompt-target-development` | +| `pyrit/score/**` | `scorer-development` | +| `pyrit/scenario/**` | `scenario-development` | +| `pyrit/executor/attack/**` | `attack-strategy-development` | +| `pyrit/models/**` | `models-guidelines` | +| `pyrit/output/**` | `output-module` | +| `pyrit/datasets/seed_datasets/**` | `seed-dataset-loaders` | +| `doc/**/*.{py,ipynb}` | `docs-sync` | +| `frontend/**/*.test.{ts,tsx}` | `frontend-testing` | + +Do not skip this step. If a skill's guidance and these repository instructions ever conflict, follow the skill for that subsystem. + +## Instructions + +These always-on instructions apply automatically by file path — no skill invocation needed: + +- `.github/instructions/style-guide.instructions.md` — all Python (`**/*.py`) +- `.github/instructions/frontend-style-guide.instructions.md` — frontend TypeScript/React (`frontend/**/*.{ts,tsx}`) +- `.github/instructions/test.instructions.md` — all tests (`**/tests/**`) diff --git a/.github/instructions/attacks.instructions.md b/.github/instructions/attacks.instructions.md deleted file mode 100644 index 2d5c4d7c96..0000000000 --- a/.github/instructions/attacks.instructions.md +++ /dev/null @@ -1,58 +0,0 @@ ---- -applyTo: "pyrit/executor/attack/**" ---- - -# PyRIT AttackStrategy Development Guidelines - -`AttackStrategy` subclasses (single-turn attacks like `PromptSendingAttack`, multi-turn attacks like `RedTeamingAttack`, etc.) are pluggable bricks orchestrated by `AttackExecutor` and the `Scenario` framework. Style rules from `style-guide.instructions.md` (async `_async` suffix, keyword-only args, type hints, enums-over-Literals) still apply and are not repeated here. - -## Constructor contract - -`AttackStrategy` subclasses MUST follow the keyword-only constructor shape: - -```python -class MyAttack(AttackStrategy[MyContext, MyResult]): - def __init__( - self, - *, - objective_target: PromptTarget, - custom_param: str = "", - **kwargs: Any, - ) -> None: - super().__init__( - objective_target=objective_target, - context_type=MyContext, - **kwargs, - ) -``` - -Requirements: - -- All parameters after ``self`` are keyword-only (insert ``*`` immediately - after ``self``). This is **enforced at class-definition time** by - `AttackStrategy.__init_subclass__` calling `enforce_keyword_only_init` - (see `pyrit/common/brick_contract.py`). Non-conforming subclasses - raise `TypeError` at import time. -- ``super().__init__(...)`` must be invoked with at minimum - ``objective_target`` and ``context_type``. -- Existing subclasses that cannot adopt the contract immediately may set - the class attribute ``_brick_legacy_init = True`` to opt into a - one-release grace period that downgrades the error to a - ``DeprecationWarning(removed_in="0.16.0")``. The opt-out is removed in - 0.16.0; classes that still violate the contract at that point will hard - fail. -- ``AttackTechniqueFactory`` already rejects ``**kwargs`` in attack - ``__init__`` at factory-registration time - (`pyrit/scenario/core/attack_technique_factory.py`); the new - ``__init_subclass__`` check is complementary — the factory check catches - scenarios-side wiring mistakes, the subclass check catches the - ``__init__`` shape at class-definition time. - -## Common pitfalls - -- Forgetting ``*`` after ``self`` — the new check will surface this at - import time with the exact list of positional parameters that need to be - made keyword-only. -- Calling ``super().__init__`` with positional arguments — the base - ``AttackStrategy.__init__`` is already keyword-only, so positional calls - raise ``TypeError`` at runtime. Always forward via kwargs. diff --git a/.github/instructions/docs.instructions.md b/.github/instructions/docs.instructions.md deleted file mode 100644 index 7bcc470491..0000000000 --- a/.github/instructions/docs.instructions.md +++ /dev/null @@ -1,96 +0,0 @@ ---- -applyTo: 'doc/**/*.{py,ipynb}' ---- - -# Documentation File Synchronization - -## CRITICAL: .ipynb and .py Files Are Linked - -All Jupyter notebooks (.ipynb) in the `doc/` directory have corresponding Python (.py) files that are **tightly synchronized**. These files MUST always match exactly in content. They represent the same documentation in different formats. - -**Locations:** `doc/**/*.ipynb` and `doc/**/*.py` - -## Editing Guidelines - -### Preferred Approach: Inline Updates to Both Files -For simple, straightforward changes (imports, variable names, paths, small code fixes): -- **UPDATE BOTH FILES INLINE** using search/replace operations -- This is the fastest and most reliable method for minor edits -- Ensures immediate synchronization without execution overhead -- **Exercise extreme caution**: Even small mismatches will break synchronization -- Also acceptable to just edit the .ipynb and regenerate the .py (this is fast) - -### Last Resort: Regenerate the ipynb with Jupytext -For complex or extensive changes where inline editing is error-prone: -1. Edit ONLY the .py file -2. Regenerate the .ipynb using: `jupytext --to ipynb --execute doc/path/to/your_notebook.py` -3. **WARNING**: This process takes several minutes to execute -4. Use this ONLY when inline updates are too risky or complex - -## Why This Matters -- Out-of-sync files create inconsistent documentation -- Users and CI/CD systems expect these files to match exactly -- Breaking synchronization causes maintenance headaches and confusion -- The .py files are managed by jupytext and must remain compatible - -## Verification Approach -When making changes: -1. **Think carefully** before editing - can this be done inline safely? -2. If editing inline, ensure BOTH .ipynb and .py receive identical logical changes -3. Pay special attention to: - - Code cell content must match exactly - - Imports and function calls - - File paths and constants - - Variable names and values -4. After editing, verify the changes are truly equivalent - - -## Jupytext Usage Reference - -### Critical pre-execution checklist - -Before running `jupytext --execute`, make sure the kernel will exercise *the code in this checkout*, not some stale install: - -1. **Use a kernel bound to a Python env that has this worktree installed editable.** - Reusing an existing `pyrit` kernel is fine *only if* it points at the current - checkout — otherwise it will resolve imports against an unrelated copy and - either pass on stale code or fail on missing new symbols. - - Quick check: `python -c "import pyrit, pathlib; print(pathlib.Path(pyrit.__file__).resolve())"` - - If it doesn't match this worktree, install editable here: `pip install -e .` - (this rebinds the existing kernel to this checkout, no new kernel needed). - - Only create a new kernel (`python -m ipykernel install --user --name `) - if you actually need an isolated env. -2. **Credentials must be pre-configured.** Most notebooks call live targets - (OpenAI, Azure, etc.) and load creds from `~/.pyrit/.env`. Make sure the - required keys are present before executing. - -### Keep the cell outputs - -**Do not strip cell outputs from notebooks under `doc/`.** Outputs are part of the -documentation — readers rely on seeing rendered tables, images, and printer output. -If a notebook can't execute end-to-end, that is exactly the regression we want -to surface in review; don't paper over it by committing an output-less notebook. -`nbstripout` is intentionally not run against `doc/` content for this reason. - -### Commands - -Generate .ipynb from .py (with execution — if it fails it means there are errors): -```bash -jupytext --to ipynb --execute doc/path/to/your_notebook.py -``` - -Generate .py from .ipynb: -```bash -jupytext --to py:percent doc/path/to/notebook.ipynb -``` - -If a `doc/**/*.py` notebook fails during `jupytext --execute` with errors that look like uninitialized state (missing env vars, undefined names, `initialize_pyrit_async` apparently not run, failing cell shows `Cell In[1]` despite earlier code), **check the `# %%` cell separators in the .py file first**. - -A missing `# %%` between a `# %% [markdown]` block and the following code causes jupytext to silently absorb the code into the markdown cell, so it never executes. Symptoms in isolation (small repro scripts) will work fine — the bug is purely in the cell structure of the .py file. Do not chase env-loading or runtime issues until cell markers are verified. - -## Summary -- **Default strategy**: Update both files inline for simple changes -- **Be cautious and deliberate**: Out-of-sync files are worse than slow regeneration -- **Last resort**: Edit .py only, then regenerate .ipynb (slow but safe) -- **Never** edit only one file without addressing the other -``` diff --git a/.github/instructions/frontend-test.instructions.md b/.github/instructions/frontend-test.instructions.md deleted file mode 100644 index d051329ae5..0000000000 --- a/.github/instructions/frontend-test.instructions.md +++ /dev/null @@ -1,472 +0,0 @@ ---- -applyTo: 'frontend/**/*.test.{ts,tsx}' ---- - -# PyRIT Frontend Test Instructions - -Consistent, fast, readable tests using Jest + React Testing Library + Fluent UI v9. - -## Test Stack - -| Tool | Purpose | -|---|---| -| **Jest** (`ts-jest`, `jsdom`) | Test runner and assertions | -| **React Testing Library** (`@testing-library/react`) | Component rendering and DOM queries | -| **`@testing-library/user-event`** | Simulating realistic user interactions | -| **`@testing-library/jest-dom`** | Extended DOM matchers (`toBeInTheDocument`, `toBeDisabled`, etc.) | -| **Playwright** (`@playwright/test`) | E2E browser tests (separate from unit tests) | - -## File Naming & Location - -- Test files are **co-located** next to the source file they test. -- Naming: `.test.tsx` or `.test.ts` - -``` -components/Chat/ - ChatWindow.tsx - ChatWindow.styles.ts - ChatWindow.test.tsx ← co-located test -hooks/ - useConnectionHealth.tsx - useConnectionHealth.test.tsx -utils/ - messageMapper.ts - messageMapper.test.ts ← for pure utility tests, use .test.ts (no JSX needed) -``` - -## Test Structure - -### Describe / It Blocks -- Group tests with `describe('', () => { ... })`. -- Use concise `it('should ...')` descriptions that state the expected behavior. - -```tsx -describe('ConnectionBanner', () => { - it('renders warning banner when degraded', () => { ... }) - it('renders error banner when disconnected', () => { ... }) -}) -``` - -### Setup & Teardown -- Use `beforeEach(() => jest.clearAllMocks())` to reset mocks between tests. -- Define `defaultProps` at the top of the `describe` block to avoid repetition. - -```tsx -describe('ChatInputArea', () => { - const defaultProps = { - onSend: jest.fn(), - disabled: false, - } - - beforeEach(() => { - jest.clearAllMocks() - }) - - it('should render input and send button', () => { - render() - expect(screen.getByRole('textbox')).toBeInTheDocument() - }) -}) -``` - -## Rendering Components - -### Fluent UI Provider Wrapper -Fluent UI v9 components require `FluentProvider` to be in the tree. Define a `TestWrapper` at the top of each test file for any test that renders Fluent UI components. - -```tsx -import { FluentProvider, webLightTheme } from '@fluentui/react-components' - -const TestWrapper: React.FC<{ children: React.ReactNode }> = ({ children }) => ( - {children} -) - -// Usage -render() -``` - -### Context Providers -For components that consume React Context (e.g., `useConnectionHealth`), wrap with the appropriate provider or mock the hook. - -```tsx -// Option A: Mock the hook -jest.mock('@/hooks/useConnectionHealth', () => ({ - useConnectionHealth: () => ({ status: 'connected', reconnectCount: 0 }), -})) - -// Option B: Wrap with the real provider -render( - - - -) -``` - -## Guiding Principle - -> "The more your tests resemble the way your software is used, the more confidence they can give you." — Kent C. Dodds - -Tests should interact with DOM nodes the way a real user would — not with component instances, internal state, or CSS class names. If a test breaks only because of a refactor (not a behavior change), the test is too tightly coupled. - -## Querying the DOM - -### Use `screen` for All Queries -Always import `screen` from `@testing-library/react` and query through it. Do not destructure queries from `render()` — `screen` is pre-bound to `document.body` and reads more clearly. - -```tsx -// CORRECT -import { render, screen } from '@testing-library/react' -render() -screen.getByRole('button', { name: /submit/i }) - -// INCORRECT — destructuring from render -const { getByRole } = render() -getByRole('button', { name: /submit/i }) -``` - -### Query Priority (follow Testing Library best practices) -Queries are ranked by how closely they mirror user experience. Prefer higher-priority queries: - -**1. Accessible to everyone** — reflect how visual users and assistive technology find elements: - -| Priority | Query | When to use | -|---|---|---| -| 1st | `getByRole` | Almost everything — buttons, textboxes, headings, dialogs, tabs. Use the `name` option to narrow: `getByRole('button', { name: /send/i })` | -| 2nd | `getByLabelText` | Form fields — emulates finding an input by its `