diff --git a/.claude/agents/requirements_reviewer.md b/.claude/agents/requirements_reviewer.md index 705a3342..1631b148 100644 --- a/.claude/agents/requirements_reviewer.md +++ b/.claude/agents/requirements_reviewer.md @@ -15,7 +15,7 @@ maxTurns: 30 # Requirements Reviewer Agent -You review the unsupervised-cli project to verify that its three-way traceability chain is maintained: +You review the project to verify that its three-way traceability chain is maintained: **Functionality → Requirements → Tests** @@ -52,10 +52,13 @@ For every piece of new or changed end-user functionality in the diff: ### 2. Test Coverage of Requirements +The direction of this check is FROM requirements TO tests. Every requirement must have a test, but not every test needs to reference a requirement — some tests are utility/edge-case tests that don't map to a specific requirement, and that's fine. + For every requirement (new or existing): - Verify there is at least one test that references the requirement ID - Check that the test actually validates the behavior described in the requirement - Flag any requirements that have no corresponding test +- Do NOT flag tests that lack a requirement reference — only flag requirements that lack tests ### 3. Test Stability @@ -66,9 +69,12 @@ For any modified test files: ### 4. Traceability Completeness -- Every test class or test function that validates a requirement must have the traceability comment +Only tests that DO validate a specific requirement need the traceability comment. Tests that are utility/edge-case tests without a requirement mapping do not need one — do not flag them. + +- Every test that validates a requirement must have the traceability comment - The comment must reference the correct requirement ID - The comment must include the "MUST NOT MODIFY ... UNLESS THE REQUIREMENT CHANGES" warning +- Do NOT flag tests that lack a requirement reference — only flag requirements whose tests are missing the traceability comment ## Output Format diff --git a/.deepreview b/.deepreview new file mode 100644 index 00000000..6fd79610 --- /dev/null +++ b/.deepreview @@ -0,0 +1,57 @@ +python_code_review: + description: "Check Python files for code quality, style, and best practices." + match: + include: + - "**/*.py" + review: + strategy: individual + instructions: + file: .deepwork/review/python_code_review.md + +python_lint: + description: "Run ruff (lint + format) and mypy on changed Python files." + match: + include: + - "**/*.py" + review: + strategy: matches_together + instructions: + file: .deepwork/review/python_lint.md + +requirements_traceability: + description: "Verify requirements traceability between specs, code, and tests." + match: + include: + - "**/*" + review: + strategy: all_changed_files + agent: + claude: requirements-reviewer + instructions: | + Review the changed files for requirements traceability. + + This project keeps formal requirements in `specs/` using the naming pattern + `REQ-NNN-.md` with individually numbered requirements (e.g. REQ-001.1). + Tests live in `tests/` and reference requirement IDs via docstrings and + traceability comments. + + For this review: + 1. Check that any new or changed end-user functionality has a corresponding + requirement in `specs/`. + 2. Check that every requirement touched by this change has at least one + automated test referencing it in `tests/`. + 3. Flag any test modifications where the underlying requirement did not + also change. + 4. Verify traceability comments are present, correctly reference + requirement IDs, and use the standard two-line format: + ``` + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-xxx.x.x). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + ``` + Both lines are required. They must appear inside the method body + (after `def`, before the docstring). If tests have REQ references + only in docstrings but are missing the formal comment block, flag + them. If the second line is missing, flag that too. + + Produce a structured review with Coverage Gaps, Test Stability Violations, + Traceability Issues, and a Summary with PASS/FAIL verdicts. diff --git a/.deepwork/review/python_code_review.md b/.deepwork/review/python_code_review.md new file mode 100644 index 00000000..4cb5c463 --- /dev/null +++ b/.deepwork/review/python_code_review.md @@ -0,0 +1,10 @@ +Review this Python file according to the project's code review standards defined in @doc/code_review_standards.md. + +Apply all review categories (General Issues, DRY, Naming Clarity, Test Coverage, Test Quality) and use the severity levels defined in that document. + +For each issue found, report: +1. File and line number +2. Severity level (Critical / High / Medium / Low) +3. Category +4. Description of the issue +5. Suggested fix diff --git a/.deepwork/review/python_lint.md b/.deepwork/review/python_lint.md new file mode 100644 index 00000000..c80c1d61 --- /dev/null +++ b/.deepwork/review/python_lint.md @@ -0,0 +1,16 @@ +Run ruff and mypy on the changed Python files listed below. Fix what you can automatically, then report only issues that remain unresolved. + +## Steps + +1. Run `uv run ruff check --fix` on each file listed under "Files to Review". +2. Run `uv run ruff format` on each file listed under "Files to Review". +3. Run `uv run mypy` on each file listed under "Files to Review". +4. Fix any remaining issues you can resolve (e.g., adding type annotations, renaming ambiguous variables). +5. Re-run all three checks to confirm your fixes are clean. + +## Output Format + +Only report issues that remain **after** your fixes. Do not report issues you already resolved. + +- PASS: All checks pass (no remaining issues). +- FAIL: Unfixable lint errors or type errors remain. List each with file, line, and details. diff --git a/.github/workflows/claude-code-test.yml b/.github/workflows/claude-code-test.yml index 1e591ffd..5244e8a7 100644 --- a/.github/workflows/claude-code-test.yml +++ b/.github/workflows/claude-code-test.yml @@ -45,6 +45,7 @@ jobs: uses: astral-sh/setup-uv@v4 with: version: "latest" + enable-cache: true - name: Set up Python if: github.event_name != 'pull_request' @@ -163,6 +164,7 @@ jobs: uses: astral-sh/setup-uv@v4 with: version: "latest" + enable-cache: true - name: Set up Python if: steps.check-key.outputs.has_key == 'true' diff --git a/.github/workflows/copilot-setup-steps.yml b/.github/workflows/copilot-setup-steps.yml index e97cd89f..b9077df5 100644 --- a/.github/workflows/copilot-setup-steps.yml +++ b/.github/workflows/copilot-setup-steps.yml @@ -27,22 +27,13 @@ jobs: uses: astral-sh/setup-uv@v4 with: version: "latest" + enable-cache: true - name: Set up Python uses: actions/setup-python@v5 with: python-version: "3.11" - - name: Cache UV dependencies - uses: actions/cache@v4 - with: - path: | - ~/.cache/uv - .venv - key: ${{ runner.os }}-uv-${{ hashFiles('uv.lock') }} - restore-keys: | - ${{ runner.os }}-uv- - - name: Install dependencies run: uv sync --extra dev diff --git a/.github/workflows/prepare-release.yml b/.github/workflows/prepare-release.yml index 9daa7045..1aa0eec7 100644 --- a/.github/workflows/prepare-release.yml +++ b/.github/workflows/prepare-release.yml @@ -115,6 +115,7 @@ jobs: uses: astral-sh/setup-uv@v4 with: version: "latest" + enable-cache: true - name: Set up Python uses: actions/setup-python@v5 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ba588c86..c4e61e23 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -18,6 +18,7 @@ jobs: uses: astral-sh/setup-uv@v4 with: version: "latest" + enable-cache: true - name: Set up Python uses: actions/setup-python@v5 diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 741add4e..91a64354 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -12,25 +12,54 @@ permissions: contents: read jobs: - tests: + lint: + name: Lint runs-on: ubuntu-latest - steps: - uses: actions/checkout@v4 - - name: Install Nix - uses: cachix/install-nix-action@v31 + - name: Install uv + uses: astral-sh/setup-uv@v4 + with: + version: "latest" + enable-cache: true - - name: Setup Nix development environment - uses: nicknovitski/nix-develop@v1 + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.11" - - name: Install Python dependencies + - name: Install dependencies run: uv sync --extra dev - - name: Check formatting (ruff) - run: | - ruff format --check src/ tests/ - ruff check src/ tests/ + - name: Check formatting + run: uv run ruff format --check src/ tests/ + + - name: Check linting + run: uv run ruff check src/ tests/ + + - name: Run type checking + run: uv run mypy src/ + + test: + name: Tests (pytest) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Install uv + uses: astral-sh/setup-uv@v4 + with: + version: "latest" + enable-cache: true + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.11" + + - name: Install dependencies + run: uv sync --extra dev - name: Run tests - run: pytest tests/ -v + run: uv run pytest tests/ -v diff --git a/.mcp.json b/.mcp.json deleted file mode 100644 index c72d476e..00000000 --- a/.mcp.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "mcpServers": { - "deepwork": { - "command": "deepwork", - "args": [ - "serve", - "--path", - ".", - "--external-runner", - "claude" - ] - } - } -} \ No newline at end of file diff --git a/README.md b/README.md index dc52983c..944a6060 100644 --- a/README.md +++ b/README.md @@ -128,6 +128,8 @@ Claude will follow the workflow step by step. **4. All work happens on Git branches** — Every change can be version-controlled and tracked. You can roll-back to prior versions of the skill or keep skills in-sync and up-to-date across your team. +**5. Automated change review** — Define `.deepreview` config files to set up review rules (patterns, strategies, instructions). Run `deepwork review --instructions-for claude` to generate parallel review tasks for your changed files. Works wonderfully for code reviews, but can review non-code files as well. + --- ## Supported Platforms diff --git a/README_REVIEWS.md b/README_REVIEWS.md new file mode 100644 index 00000000..99332fab --- /dev/null +++ b/README_REVIEWS.md @@ -0,0 +1,510 @@ +# DeepWork Reviews + +DeepWork Reviews lets you define automated code review policies using `.deepreview` config files placed anywhere in your project. When you run a review, it detects which files changed on your branch, matches them against your rules, and dispatches parallel review agents — each with focused instructions and only the files it needs to see. + +## How It Works + +1. You create `.deepreview` files that define review rules (what to match, how to review) +2. You run `deepwork review --instructions-for claude` +3. DeepWork Reviews diffs your branch, matches changed files to rules, and generates review instructions +4. Your CLI agent spawns parallel review tasks — one per rule match — each scoped to exactly the right files + +The outer agent never needs to load the full context of every review. Each sub-agent gets a self-contained instruction file with just its files and instructions. + +## What Files Are Considered "Changed"? + +When you run a review without explicit `--files`, DeepWork detects changed files using local git operations. Here's exactly what is and isn't included: + +### Included + +| Change type | Example | Why it's included | +|-------------|---------|-------------------| +| **Committed changes on your branch** | You committed a fix 3 commits ago | `git diff` against the merge-base with `main`/`master` catches all commits since the branch diverged | +| **Staged but not yet committed** | You ran `git add myfile.py` but haven't committed | A separate `git diff --cached` picks up index changes | +| **Unstaged modifications to tracked files** | You edited a file but haven't staged it | The working-tree diff against the merge-base includes these | + +### Not included + +| Change type | Why it's excluded | +|-------------|-------------------| +| **Untracked files** (new files never added to git) | `git diff` only operates on tracked files. Run `git add` first, or pass them explicitly with `--files`. | +| **Deleted files** | The `--diff-filter=ACMR` flag excludes deletions — there's nothing to review in a deleted file. | + +### Local-only detection + +All detection is local. DeepWork does not fetch from or communicate with any remote (GitHub, GitLab, etc.). This means: + +- **Commits pushed to the remote but not fetched locally** won't appear. Run `git fetch` first if your local branch is behind. +- **Commits that are local but not yet pushed** are fully included — pushing has no effect on what DeepWork sees. +- In practice, if your local branch is up to date, the changed files match what a GitHub PR would show. + +### Overriding detection + +You can bypass git diff entirely by providing files explicitly: + +```bash +# Explicit file arguments (highest priority) +deepwork review --instructions-for claude --files src/app.py --files src/lib.py + +# Piped from another command (second priority) +git diff --name-only HEAD~3 | deepwork review --instructions-for claude +``` + +When files are provided explicitly, `--base-ref` is ignored. + +## The `.deepreview` Config File + +A `.deepreview` file is YAML. It contains one or more named rules, each with a `match` section (what files to trigger on) and a `review` section (how to review them). + +### Placement in the File Hierarchy + +`.deepreview` files work like `.gitignore` — each file applies to the directory it lives in, and its glob patterns match relative to that directory. You can place them at any level: + +``` +my-project/ +├── .deepreview # Rules for the whole project +├── src/ +│ ├── .deepreview # Rules scoped to src/ +│ └── auth/ +│ └── .deepreview # Rules scoped to src/auth/ +└── infrastructure/ + └── .deepreview # Rules scoped to infrastructure/ +``` + +A `.deepreview` at the root with `include: ["**/*.py"]` matches all Python files in the project. The same pattern in `src/.deepreview` only matches Python files under `src/`. Rules from different files are completely independent — they don't override or interact with each other. + +This lets you keep review policies close to the code they govern. The security team can own `src/auth/.deepreview`, the platform team can own `infrastructure/.deepreview`, and so on. + +### Rule Structure + +```yaml +rule_name: + description: "Short description" # Required. Under 256 characters. + match: + include: + - "glob/pattern/**/*.ext" # Required. At least one pattern. + exclude: # Optional. + - "pattern/to/skip/**" + review: + strategy: individual # Required. How to group files. + instructions: | # Required. What to tell the reviewer. + Review this file for ... # Can be inline text (like this) or a file reference like: + # instructions: + # file: .deepwork/review/python_review.md + agent: # Optional. Use a specific agent persona. + claude: "security-expert" + additional_context: # Optional. Extra context for the reviewer. + all_changed_filenames: true + unchanged_matching_files: true +``` + +## Review Strategies + +The `strategy` field controls how matched files are grouped into review tasks. + +### `individual` — One review per file + +Each changed file that matches the rule gets its own review task. The reviewing agent sees only that one file. + +Best for: file-level linting, per-component checks, style reviews. + +```yaml +python_review: + description: "Review Python source files for code quality and best practices." + match: + include: + - "**/*.py" + exclude: + - "tests/**/*.py" + review: + strategy: individual + instructions: + file: .github/prompts/python_review.md +``` + +### `matches_together` — One review for all matched files + +All changed files matching the rule are grouped into a single review task. The reviewing agent sees all of them together. + +Best for: cross-file consistency checks, migration sequence validation, documentation link checks. + +```yaml +db_migration_safety: + description: "Check database migrations for conflicts, destructive ops, and ordering." + match: + include: + - "alembic/versions/*.py" + review: + strategy: matches_together + agent: + claude: "db-expert" + instructions: | + Review these database migrations together. + Ensure there are no conflicting locks, no destructive + drops without backups, and that the sequence IDs are ordered correctly. +``` + +### `all_changed_files` — Tripwire that reviews everything + +The match patterns act as a trigger. If *any* changed file matches, the review task gets *all* changed files in the entire changeset — not just the matched ones. + +Best for: security audits, broad impact analysis when sensitive areas are touched. + +```yaml +pr_security_review: + description: "Security audit triggered by auth or config changes." + match: + include: + - "src/auth/**/*.py" + - "config/*" + exclude: + - "config/*.dev.yaml" + review: + strategy: all_changed_files + agent: + claude: "security-expert" + instructions: | + A change was detected in the authentication module or core config. + Review all the changed files in this changeset for potential security + regressions, leaked secrets, or broken authorization logic. +``` + +## Additional Context + +The `additional_context` flags give the reviewer extra information beyond the matched files. + +### `all_changed_filenames` + +Includes a list of every file changed in the branch — even files that don't match this rule's patterns. The reviewer can use this to spot related changes (e.g., "a component's API changed but its consumers weren't updated"). + +```yaml +ui_component_review: + description: "Review React components for accessibility and prop-type safety." + match: + include: + - "src/components/**/*.tsx" + review: + strategy: individual + additional_context: + all_changed_filenames: true + instructions: | + Review this React component for accessibility and prop-type safety. + Check the changed filenames list to see if the consumer of this component + was also updated if you notice breaking API changes. +``` + +### `unchanged_matching_files` + +Includes files that match the `include` patterns but were *not* changed. This is useful when you need the reviewer to see the complete set of files, not just the changed ones — for example, verifying version numbers are in sync. + +```yaml +versions_in_sync: + description: "Ensure version numbers stay in sync across release files." + match: + include: + - "CHANGELOG.md" + - "pyproject.toml" + - "uv.lock" + review: + strategy: matches_together + additional_context: + unchanged_matching_files: true + instructions: "Make sure the version number is exactly the same across all three of these files." +``` + +If only `pyproject.toml` changed, the reviewer still sees all three files so it can verify they match. + +## Instructions + +The `instructions` field tells the reviewing agent what to look for. It can be an inline string or a reference to an external file. + +### Inline + +```yaml +review: + instructions: "Check for proper error handling and logging." +``` + +```yaml +review: + instructions: | + Review this file for: + 1. Proper error handling + 2. Consistent logging + 3. No hardcoded secrets +``` + +### File reference + +```yaml +review: + instructions: + file: .deepwork/review/python_review.md +``` + +These are snippets showing just the `review` block. In a full rule, the `description` field is also required at the rule level (see Rule Structure above). + +The file path is resolved relative to the directory containing the `.deepreview` file. This is useful for longer review guidelines that you want to maintain separately and reuse across rules. + +We recommend putting instruction files in `.deepwork/review/` so they're easy to find and reuse across multiple `.deepreview` configs. For example, a single `python_review.md` file can be referenced from both root and subdirectory `.deepreview` files. + +## Agent Personas + +The optional `agent` field maps platform names to agent persona strings. When the review runs on that platform, the specified persona is used instead of the default agent. + +```yaml +review: + agent: + claude: "security-expert" +``` + +This maps to Claude Code's agent system. If no agent is specified (or the platform key is missing), the default agent is used. + +## Running a Review + +```bash +deepwork review --instructions-for claude +``` + +This: +1. Finds all `.deepreview` files in your project +2. Runs `git diff` to detect changed files on your branch +3. Matches changed files against the rules +4. Generates instruction files in `.deepwork/tmp/review_instructions/` +5. Prints structured instructions for Claude Code to dispatch parallel review agents + +### Options + +| Flag | Default | Description | +|------|---------|-------------| +| `--instructions-for` | (required) | Target platform. Currently: `claude`. | +| `--base-ref` | auto-detect | Git ref to diff against. Auto-detects merge-base with `main` or `master`. | +| `--path` | `.` | Project root directory. | + +### What the output looks like + +``` +Invoke the following list of Tasks in parallel: + +Name: "python_review review of src/app.py" + Agent: Default + prompt: "@.deepwork/tmp/review_instructions/7142141.md" + +Name: "python_review review of src/lib.py" + Agent: Default + prompt: "@.deepwork/tmp/review_instructions/6316224.md" + +Name: "db_migration_safety review of 2 files" + Agent: db-expert + prompt: "@.deepwork/tmp/review_instructions/3847291.md" +``` + +Each sub-agent gets a self-contained instruction file. The `@` prefix tells Claude Code to read the file contents into the prompt. The outer agent stays lightweight — it just dispatches tasks. + +## Full Example + +Here is a `.deepreview` file that covers several common review scenarios: + +```yaml +# ===================================================================== +# Standard Python File Review +# Triggers on changes to any Python file outside of tests or docs. +# Every modified file is reviewed individually. +# ===================================================================== +python_file_best_practices: + description: "Review Python source files for best practices and code quality." + match: + include: + - "**/*.py" + exclude: + - "tests/**/*.py" + - "docs/**/*.py" + review: + strategy: individual + instructions: + file: .github/prompts/python_review.md + +# ===================================================================== +# UI Component Contextual Review +# Each component is reviewed individually, but the AI also gets a list +# of all other changed files to spot if a component's API changed +# without the consumers being updated. +# ===================================================================== +ui_component_review: + description: "Review React components for accessibility and prop-type safety." + match: + include: + - "src/components/**/*.tsx" + review: + strategy: individual + additional_context: + all_changed_filenames: true + instructions: | + Review this React component for accessibility and prop-type safety. + Check the changed filenames list to see if the consumer of this component + was also updated if you notice breaking API changes. + +# ===================================================================== +# Database Migration Batch Review +# All changed migration files are reviewed together so the agent can +# check for sequence conflicts and overall migration safety. +# ===================================================================== +db_migration_safety: + description: "Check database migrations for conflicts, destructive ops, and ordering." + match: + include: + - "alembic/versions/*.py" + review: + strategy: matches_together + agent: + claude: "db-expert" + instructions: | + Review these database migrations together. + Ensure there are no conflicting locks, no destructive + drops without backups, and that the sequence IDs are ordered correctly. + +# ===================================================================== +# Version Synchronization Check +# If any version file changes, all three are reviewed together — +# including unchanged ones — to verify version strings match. +# ===================================================================== +versions_in_sync: + description: "Ensure version numbers stay in sync across release files." + match: + include: + - "CHANGELOG.md" + - "pyproject.toml" + - "uv.lock" + review: + strategy: matches_together + additional_context: + unchanged_matching_files: true + instructions: "Make sure the version number is exactly the same across all three of these files." + +# ===================================================================== +# Global Security Audit +# Tripwire: if anything in auth or config is touched, ALL changed files +# in the branch get a security review. +# ===================================================================== +pr_security_review: + description: "Security audit triggered by auth or config changes." + match: + include: + - "src/auth/**/*.py" + - "config/*" + exclude: + - "config/*.dev.yaml" + review: + strategy: all_changed_files + agent: + claude: "security-expert" + instructions: | + A change was detected in the authentication module or core config. + Review all the changed files in this changeset for potential security + regressions, leaked secrets, or broken authorization logic. + +# ===================================================================== +# API Route Authorization Check +# Each route file is reviewed individually with a security persona. +# ===================================================================== +api_route_auth_check: + description: "Verify API routes have proper auth middleware and RBAC scopes." + match: + include: + - "src/routes/**/*.ts" + review: + strategy: individual + agent: + claude: "security-expert" + instructions: | + Verify that all exported API routes in this file are protected by + the `requireAuth` middleware unless explicitly decorated with `@Public`. + Check for proper role-based access control (RBAC) scopes. + +# ===================================================================== +# Dockerfile Optimization +# Each container config is reviewed individually for best practices. +# ===================================================================== +dockerfile_optimization: + description: "Check container configs for build optimization and security." + match: + include: + - "**/Dockerfile" + - "docker-compose*.yml" + review: + strategy: individual + agent: + claude: "devops-engineer" + instructions: | + Review this container configuration. Ensure multi-stage builds are used + where appropriate, layers are optimized for caching, dependencies are + pinned, and the default execution user is not root. + +# ===================================================================== +# CI/CD Pipeline Audit +# All matched workflow files are reviewed together for consistency. +# ===================================================================== +cicd_pipeline_audit: + description: "Audit CI/CD workflows for secret leaks and pinned actions." + match: + include: + - ".github/workflows/*.yml" + review: + strategy: matches_together + agent: + claude: "devops-engineer" + instructions: | + Review these CI/CD workflows. Verify that no secrets are being echoed + to the console, third-party actions are pinned to a specific commit SHA, + and deployment environments require manual approval. + +# ===================================================================== +# Documentation Consistency Check +# Matched docs are reviewed together for broken links and tone. +# ===================================================================== +docs_consistency_check: + description: "Check documentation for consistent tone, broken links, and syntax." + match: + include: + - "docs/**/*.md" + review: + strategy: matches_together + instructions: | + Review these documentation files. Ensure the tone is consistent, + check for any broken relative links between these documents, and + verify that code blocks have language tags for syntax highlighting. + +# ===================================================================== +# GraphQL Schema Evolution +# Schema files reviewed together, with a list of all other changed +# files so the agent can check if deprecated fields were removed +# before frontend clients stopped using them. +# ===================================================================== +graphql_schema_evolution: + description: "Flag breaking GraphQL schema changes and verify frontend updates." + match: + include: + - "schema/**/*.graphql" + review: + strategy: matches_together + additional_context: + all_changed_filenames: true + instructions: | + Review these GraphQL schema changes. Flag any breaking changes + (e.g., removing a field, changing a type). If there are breaking changes, + check the list of changed filenames to ensure the corresponding frontend + queries were also updated. +``` + +## Glob Pattern Reference + +Patterns follow standard glob syntax, evaluated relative to the `.deepreview` file's directory: + +| Pattern | Matches | Does not match | +|---------|---------|----------------| +| `**/*.py` | `app.py`, `src/lib.py`, `deep/nested/file.py` | `app.ts` | +| `*.py` | `app.py` | `src/app.py` (not recursive) | +| `src/components/**/*.tsx` | `src/components/Button.tsx`, `src/components/ui/Card.tsx` | `src/pages/Home.tsx` | +| `config/*` | `config/settings.yaml` | `config/deep/nested.yaml` | +| `**/Dockerfile` | `Dockerfile`, `services/api/Dockerfile` | `Dockerfile.dev` | +| `CHANGELOG.md` | `CHANGELOG.md` | `docs/CHANGELOG.md` | diff --git a/claude b/claude deleted file mode 100755 index 5f927c03..00000000 --- a/claude +++ /dev/null @@ -1,2 +0,0 @@ -#!/usr/bin/env bash -exec claude --plugin-dir "$(dirname "$0")/plugins/claude" --plugin-dir "$(dirname "$0")/learning_agents" "$@" diff --git a/claude.md b/claude.md index d0b6b20d..e5060729 100644 --- a/claude.md +++ b/claude.md @@ -71,13 +71,9 @@ deepwork/ ## Development Environment -This project uses Nix for reproducible development environments: +This project uses `uv` for package management: ```bash -# Enter development environment -nix-shell - -# Inside nix-shell, use uv for package management uv sync # Install dependencies uv run pytest # Run tests ``` diff --git a/doc/architecture.md b/doc/architecture.md index b0abeb38..baf9757e 100644 --- a/doc/architecture.md +++ b/doc/architecture.md @@ -58,7 +58,15 @@ deepwork/ # DeepWork tool repository │ │ └── gemini_hook.sh # Shell wrapper for Gemini CLI │ ├── standard_jobs/ # Built-in job definitions │ │ └── deepwork_jobs/ +│ ├── review/ # DeepWork Reviews system +│ │ ├── config.py # .deepreview config parsing + data models +│ │ ├── discovery.py # Find .deepreview files in project tree +│ │ ├── matcher.py # Git diff + glob matching + strategy grouping +│ │ ├── instructions.py # Generate review instruction files +│ │ ├── formatter.py # Format output for Claude Code +│ │ └── schema.py # JSON schema loader │ ├── schemas/ # Definition schemas +│ │ ├── deepreview_schema.json │ │ ├── job_schema.py │ │ └── doc_spec_schema.py │ └── utils/ @@ -85,7 +93,7 @@ deepwork/ # DeepWork tool repository ## DeepWork CLI Components -The CLI has been simplified to two commands: `serve` and `hook`. The old `install`, `sync`, adapters, detector, and generator have been replaced by the plugin system. +The CLI has three primary commands: `serve`, `hook`, and `review`. The old `install`, `sync`, adapters, detector, and generator have been replaced by the plugin system. ### 1. Serve Command (`serve.py`) @@ -108,7 +116,23 @@ Runs hook scripts by name, used by platform hook wrappers: deepwork hook my_hook ``` -### 3. Plugin System (replaces adapters/detector/generator) +### 3. Review Command (`review.py`) + +Generates review instructions for changed files based on `.deepreview` config files: + +```bash +deepwork review --instructions-for claude +``` + +The review command: +- Discovers `.deepreview` files throughout the project tree +- Detects changed files via `git diff` against the main branch +- Matches changed files against rules using include/exclude glob patterns +- Groups files by review strategy (`individual`, `matches_together`, `all_changed_files`) +- Generates per-task instruction files in `.deepwork/tmp/review_instructions/` +- Outputs structured text for Claude Code to dispatch parallel review agents + +### 4. Plugin System (replaces adapters/detector/generator) Platform-specific delivery is now handled by plugins in `plugins/`: @@ -948,12 +972,12 @@ DeepWork includes an MCP (Model Context Protocol) server that provides an altern The FastMCP server definition that: - Creates and configures the MCP server instance -- Registers the three workflow tools +- Registers the workflow tools and the `get_review_instructions` tool - Provides server instructions for agents ### Tools (`tools.py`) -Implements the three MCP tools: +Implements the workflow MCP tools: #### 1. `get_workflows` Lists all available workflows from `.deepwork/jobs/`. @@ -986,6 +1010,25 @@ Reports step completion and gets next instructions. - If `next_step`: next step instructions - If `workflow_complete`: summary of all outputs +#### 4. `abort_workflow` +Aborts the current workflow and returns to the parent (if nested). + +**Parameters**: +- `explanation: str` - Why the workflow is being aborted +- `session_id: str | None` - Target a specific workflow session + +**Returns**: Aborted workflow info, resumed parent info (if any), current stack + +#### 5. `get_review_instructions` +Runs the `.deepreview`-based code review pipeline. Registered directly in `server.py` (not in `tools.py`) since it operates outside the workflow lifecycle. + +**Parameters**: +- `files: list[str] | None` - Explicit files to review. When omitted, detects changes via git diff. + +**Returns**: Formatted review task list or informational message. + +The `--platform` CLI option on `serve` controls which formatter is used (defaults to `"claude"`). + ### State Management (`state.py`) Manages workflow session state persisted to `.deepwork/tmp/session_[id].json`: diff --git a/doc/mcp_interface.md b/doc/mcp_interface.md index 092fd05e..6ef843ca 100644 --- a/doc/mcp_interface.md +++ b/doc/mcp_interface.md @@ -10,7 +10,7 @@ This document describes the Model Context Protocol (MCP) tools exposed by the De ## Tools -DeepWork exposes four MCP tools: +DeepWork exposes six MCP tools: ### 1. `get_workflows` @@ -135,6 +135,50 @@ Abort the current workflow and return to the parent workflow (if nested). Use th --- +### 5. `get_review_instructions` + +Run a review of changed files based on `.deepreview` configuration files. Returns a list of review tasks to invoke in parallel. Each task has `name`, `description`, `subagent_type`, and `prompt` fields for the Task tool. + +This tool operates outside the workflow lifecycle — it can be called independently at any time. + +#### Parameters + +| Parameter | Type | Required | Description | +|-----------|------|----------|-------------| +| `files` | `string[] \| null` | No | Explicit file paths to review. When omitted, detects changes via git diff against the main branch. | + +#### Returns + +A plain string with one of: +- An informational message (no rules found, no changed files, no matches) +- Formatted review task list ready for parallel dispatch + +--- + +### 6. `get_configured_reviews` + +List all configured review rules from `.deepreview` files. Returns each rule's name, description, and defining file location. Optionally filters to rules matching specific files. + +This tool operates outside the workflow lifecycle — it can be called independently at any time. + +#### Parameters + +| Parameter | Type | Required | Description | +|-----------|------|----------|-------------| +| `only_rules_matching_files` | `string[] \| null` | No | File paths to filter by. When provided, only rules whose include/exclude patterns match at least one file are returned. When omitted, all rules are returned. | + +#### Returns + +```typescript +Array<{ + name: string; // Rule name from the .deepreview file + description: string; // Rule description + defining_file: string; // Relative path to .deepreview file with line number (e.g., ".deepreview:1") +}> +``` + +--- + ## Shared Types ```typescript @@ -302,10 +346,12 @@ details on how Claude CLI is invoked. deepwork serve [OPTIONS] Options: - --path PATH Project root directory (default: current directory) - --no-quality-gate Disable quality gate evaluation - --transport TYPE Transport type: stdio or sse (default: stdio) - --port PORT Port for SSE transport (default: 8000) + --path PATH Project root directory (default: current directory) + --no-quality-gate Disable quality gate evaluation + --transport TYPE Transport type: stdio or sse (default: stdio) + --port PORT Port for SSE transport (default: 8000) + --external-runner TYPE External runner for quality gate reviews (default: None) + --platform NAME Platform identifier (e.g., 'claude'). Used by the review tool to format output. ``` --- @@ -331,6 +377,8 @@ Add to your `.mcp.json`: | Version | Changes | |---------|---------| +| 1.6.0 | Added `get_configured_reviews` tool for listing configured review rules without running the full pipeline. Supports optional file-based filtering. | +| 1.5.0 | Added `get_review_instructions` tool (originally named `review`) for running `.deepreview`-based code reviews via MCP. Added `--platform` CLI option to `serve` command. | | 1.4.0 | Added optional `session_id` parameter to `finished_step` and `abort_workflow` for concurrent workflow safety. When multiple workflows are active on the stack, callers can pass the `session_id` (returned in `ActiveStepInfo`) to target the correct session. Fully backward compatible — omitting `session_id` preserves existing top-of-stack behavior. | | 1.3.0 | `step_expected_outputs` changed from `string[]` to `ExpectedOutput[]` — each entry includes `name`, `type`, `description`, and `syntax_for_finished_step_tool` so agents know exactly what format to use when calling `finished_step`. | | 1.2.0 | Quality gate now includes input files from prior steps in review payload with BEGIN INPUTS/END INPUTS and BEGIN OUTPUTS/END OUTPUTS section headers. Binary files (PDFs, etc.) get a placeholder instead of raw content. | diff --git a/flake.lock b/flake.lock index 06c23b58..97617209 100644 --- a/flake.lock +++ b/flake.lock @@ -6,11 +6,11 @@ "nixpkgs": "nixpkgs" }, "locked": { - "lastModified": 1771545934, - "narHash": "sha256-z2ZijehdcpfRnwu717pFirzHUoYfHHlo1Eu+Ajfme4U=", + "lastModified": 1771632347, + "narHash": "sha256-kNm0YX9RUwf7GZaWQu2F71ccm4OUMz0xFkXn6mGPfps=", "owner": "sadjow", "repo": "claude-code-nix", - "rev": "d09b5d8e0678cd5c87443d85d48983f866ef6f1c", + "rev": "ec90f84b2ea21f6d2272e00d1becbc13030d1895", "type": "github" }, "original": { diff --git a/flake.nix b/flake.nix index e1f4c14d..57c059cd 100644 --- a/flake.nix +++ b/flake.nix @@ -21,6 +21,17 @@ config.allowUnfree = true; }; in + let + claude-code = claude-code-nix.packages.${system}.default; + # Wrapper that auto-loads project plugin dirs. + # References the real binary by store path to avoid circular PATH lookup. + claude-wrapper = pkgs.writeShellScriptBin "claude" '' + exec ${claude-code}/bin/claude \ + --plugin-dir "$(git rev-parse --show-toplevel)/plugins/claude" \ + --plugin-dir "$(git rev-parse --show-toplevel)/learning_agents" \ + "$@" + ''; + in { default = pkgs.mkShell { packages = [ @@ -28,7 +39,7 @@ pkgs.uv pkgs.git pkgs.jq - claude-code-nix.packages.${system}.default + claude-wrapper pkgs.gh ]; diff --git a/plugins/claude/.mcp.json b/plugins/claude/.mcp.json index 9453a53a..278c6a9c 100644 --- a/plugins/claude/.mcp.json +++ b/plugins/claude/.mcp.json @@ -2,7 +2,7 @@ "mcpServers": { "deepwork": { "command": "uvx", - "args": ["deepwork", "serve", "--path", ".", "--external-runner", "claude"] + "args": ["deepwork", "serve", "--path", ".", "--external-runner", "claude", "--platform", "claude"] } } } diff --git a/plugins/claude/README_REVIEWS.md b/plugins/claude/README_REVIEWS.md new file mode 120000 index 00000000..6b25a48d --- /dev/null +++ b/plugins/claude/README_REVIEWS.md @@ -0,0 +1 @@ +../../README_REVIEWS.md \ No newline at end of file diff --git a/plugins/claude/hooks/hooks.json b/plugins/claude/hooks/hooks.json index deffac97..fd98afa7 100644 --- a/plugins/claude/hooks/hooks.json +++ b/plugins/claude/hooks/hooks.json @@ -1,3 +1,15 @@ { - "hooks": {} + "hooks": { + "PostToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "${CLAUDE_PLUGIN_ROOT}/hooks/post_commit_reminder.sh" + } + ] + } + ] + } } diff --git a/plugins/claude/hooks/post_commit_reminder.sh b/plugins/claude/hooks/post_commit_reminder.sh new file mode 100755 index 00000000..4095cfc1 --- /dev/null +++ b/plugins/claude/hooks/post_commit_reminder.sh @@ -0,0 +1,13 @@ +#!/bin/bash +# Post-commit reminder hook +# Triggers after Bash tool uses that contain "git commit" to remind +# the agent to run the review skill. + +INPUT=$(cat) +COMMAND=$(echo "$INPUT" | jq -r '.tool_input.command // empty') + +if echo "$COMMAND" | grep -q 'git commit'; then + cat << 'EOF' +{"hookSpecificOutput":{"hookEventName":"PostToolUse","additionalContext":"You **MUST** use AskUserQuestion tool to offer to the user to run the `review` skill to review the changes you just committed if you have not run a review recently."}} +EOF +fi diff --git a/plugins/claude/skills/configure_reviews/SKILL.md b/plugins/claude/skills/configure_reviews/SKILL.md new file mode 100644 index 00000000..a1f9563b --- /dev/null +++ b/plugins/claude/skills/configure_reviews/SKILL.md @@ -0,0 +1,25 @@ +--- +name: configure_reviews +description: "Set up DeepWork Reviews — automated code review rules using .deepreview config files" +--- + +# Configure DeepWork Reviews + +Help the user set up automated code review rules for their project. + +## How to Use + +1. Read the `README_REVIEWS.md` file in this plugin directory for the complete reference on DeepWork Reviews +2. Look at existing `.deepreview` files in the project that might have something related to what the user wants to do. Reuse existing rules / instructions if possible, including refactoring inline instructions that need to be reused into their own files in the `.deepwork/review` directory +3. Ask any clarifications needed using AskUserQuestion. Skip if you are clear on the need. +4. Create or update `.deepreview` YAML config files in the appropriate locations in their project +5. Test your change. Make a small change that should trigger the affected rule, then call `mcp__deepwork__get_review_instructions` and verify that the output includes text referencing your new rule. Be sure to revert the change. +6. Summarize your changes. +7. Ask the user if they'd like you to run a review now using the `/review` skill to try out the new configuration. + +## Important + +- Always read `README_REVIEWS.md` first (if you have not in this conversation already) — it contains the full configuration reference, examples, and glob pattern guide +- Help users think about WHERE to place `.deepreview` files (project root for broad rules, subdirectories for scoped rules) +- Write practical review instructions that will produce actionable feedback +- **Minimize reviewer count.** Each review rule spawns a separate sub-agent with material overhead (context setup, API round-trips, teardown). Combine rules into a single rule whenever the joint instructions would not overload the reviewer's context. For example, running ruff and mypy on Python files should be one `python_lint` rule — not two separate rules — because the combined instructions are short and the file set is identical. Only split into separate rules when the instructions are long enough or distinct enough that merging them would degrade review quality. diff --git a/plugins/claude/skills/review/SKILL.md b/plugins/claude/skills/review/SKILL.md new file mode 100644 index 00000000..e0dc1b6d --- /dev/null +++ b/plugins/claude/skills/review/SKILL.md @@ -0,0 +1,33 @@ +--- +name: review +description: "Run DeepWork Reviews on the current branch — review changed files using .deepreview rules" +--- + +# DeepWork Review + +Run automated code reviews on the current branch based on `.deepreview` config files. + +## Routing + +If the user is asking to **configure, create, or modify** review rules (e.g., "add a security review", "set up reviews for our API layer"), use the `configure_reviews` skill instead. This skill is for **running** reviews. + +## How to Run + +1. Call the `mcp__deepwork__get_review_instructions` tool: + - No arguments needed to review the current branch's changes (detects via git diff). + - To review specific files, pass `files`: `mcp__deepwork__get_review_instructions(files=["src/app.py", "src/lib.py"])` +2. The output will list review tasks to invoke in parallel. Each task has `name`, `description`, `subagent_type`, and `prompt` fields — these map directly to the Task tool parameters. Launch all of them as parallel Task agents. +3. Collect the results from all review agents. + +## Acting on Results + +For each finding from the review agents: + +- **Obviously good changes with no downsides** (e.g., fixing a typo, removing an unused import, adding a missing null check): make the change immediately without asking. +- **Everything else** (refactors, style changes, architectural suggestions, anything with trade-offs): use AskUserQuestion to ask the user about each finding **individually** — one question per finding. Do NOT batch unrelated findings into a single question. This lets the user make separate decisions on each item. For each question, provide several concrete fix approaches as options when there are reasonable alternatives (e.g., "Update the spec to match the code" vs "Update the code to match the spec" vs "Skip"). Be concise — quote the key finding, don't dump the full review. + +## Iterate + +After making any changes, run the review again by calling `mcp__deepwork__get_review_instructions` with no arguments. + +Repeat the full cycle (run → act on results → run again) until a clean run produces no further actionable findings. Note that you don't have to run EVERY task the above outputs after the first time - you can just run the ones that match up to ones that had feedback last time. If you made very large changes, it may be a good idea to run the full reviews set. diff --git a/specs/deepwork/review/REQ-001-deepreview-config.md b/specs/deepwork/review/REQ-001-deepreview-config.md new file mode 100644 index 00000000..831255ce --- /dev/null +++ b/specs/deepwork/review/REQ-001-deepreview-config.md @@ -0,0 +1,77 @@ +# REQ-001: DeepWork Reviews Configuration Format + +## Overview + +DeepWork Reviews uses `.deepreview` YAML configuration files to define review rules for automated code review. Each file contains one or more named rules that specify file matching patterns and review behavior. The configuration format is validated against a JSON Schema bundled with the DeepWork package. Rules support inline or file-referenced instructions, optional agent personas, and additional context flags. + +## Requirements + +### REQ-001.1: Configuration File Format + +1. A `.deepreview` file MUST be a valid YAML document. +2. The top-level structure MUST be a mapping of rule names to rule objects. +3. Rule names MUST match the pattern `^[a-zA-Z0-9_-]+$`. +4. Each rule object MUST contain exactly three required keys: `description`, `match`, and `review`. +5. Rule objects MUST NOT contain additional properties beyond `description`, `match`, and `review`. + +### REQ-001.2: Match Section + +1. The `match` section MUST be an object. +2. The `match` section MUST contain an `include` key with a non-empty array of glob pattern strings. +3. The `match` section MAY contain an `exclude` key with an array of glob pattern strings. +4. The `match` section MUST NOT contain additional properties beyond `include` and `exclude`. +5. Glob patterns MUST support standard glob syntax including `**` for recursive directory matching and `*` for filename matching. + +### REQ-001.3: Review Section + +1. The `review` section MUST be an object. +2. The `review` section MUST contain a `strategy` key with one of: `"individual"`, `"matches_together"`, or `"all_changed_files"`. +3. The `review` section MUST contain an `instructions` key. +4. The `review` section MAY contain an `agent` key. +5. The `review` section MAY contain an `additional_context` key. +6. The `review` section MUST NOT contain additional properties beyond `strategy`, `instructions`, `agent`, and `additional_context`. + +### REQ-001.4: Instructions + +1. The `instructions` field MUST be one of two forms: an inline string, or an object with a `file` key. +2. When `instructions` is a string, the string MUST be used directly as the review instruction text. +3. When `instructions` is an object, it MUST contain a `file` key whose value is a path to a markdown file. +4. The `file` path MUST be resolved relative to the directory containing the `.deepreview` file. +5. The system MUST raise an error if a referenced instructions file does not exist. +6. The instructions object MUST NOT contain additional properties beyond `file`. + +### REQ-001.5: Agent Configuration + +1. The `agent` field, when present, MUST be an object mapping CLI provider names to agent persona strings. +2. Provider names MUST match the pattern `^[a-zA-Z0-9_-]+$`. +3. Agent persona values MUST be strings (e.g., `"security-expert"`, `"devops-engineer"`). +4. The system MUST select the agent persona matching the target platform (e.g., the `claude` key when generating instructions for Claude Code). +5. When no matching agent key exists for the target platform, the system MUST use the default agent (no persona). + +### REQ-001.6: Additional Context + +1. The `additional_context` field, when present, MUST be an object. +2. The `additional_context` object MAY contain `all_changed_filenames` (boolean). +3. The `additional_context` object MAY contain `unchanged_matching_files` (boolean). +4. The `additional_context` object MUST NOT contain additional properties beyond these two. +5. When `all_changed_filenames` is `true`, the system MUST include a list of all files changed in the changeset (not just matched files) as context in the review instructions. +6. When `unchanged_matching_files` is `true`, the system MUST include the full contents of files that match the `include` patterns but were NOT modified in this changeset. + +### REQ-001.7: Schema Validation + +1. The JSON Schema for `.deepreview` files MUST be bundled with the DeepWork package at `src/deepwork/schemas/deepreview_schema.json`. +2. The schema MUST be loaded at module import time and stored as a module-level constant. +3. Every `.deepreview` file MUST be validated against the schema before parsing into data models. +4. The system MUST raise an error with a descriptive message if schema validation fails, including the validation path and error details. +5. Schema validation MUST use the `jsonschema` library (same as job definition validation, see REQ-002.2). + +### REQ-001.8: Data Model + +1. Each parsed rule MUST be represented as a `ReviewRule` dataclass. +2. The `ReviewRule` MUST contain: `name` (str), `description` (str), `include_patterns` (list[str]), `exclude_patterns` (list[str]), `strategy` (str), `instructions` (str — resolved text), `agent` (dict[str, str] | None), `all_changed_filenames` (bool), `unchanged_matching_files` (bool), `source_dir` (Path), `source_file` (Path), `source_line` (int). +3. The `source_dir` field MUST be set to the directory containing the `.deepreview` file (used for resolving relative glob patterns and file references). +4. The `source_file` field MUST be set to the path of the `.deepreview` file the rule was parsed from. +5. The `source_line` field MUST be set to the 1-based line number where the rule name appears in the `.deepreview` file. +6. When `exclude` is not specified in the YAML, `exclude_patterns` MUST default to an empty list. +7. When `additional_context` is not specified, both `all_changed_filenames` and `unchanged_matching_files` MUST default to `False`. +8. When `agent` is not specified, it MUST default to `None`. diff --git a/specs/deepwork/review/REQ-002-config-discovery.md b/specs/deepwork/review/REQ-002-config-discovery.md new file mode 100644 index 00000000..98d6f2eb --- /dev/null +++ b/specs/deepwork/review/REQ-002-config-discovery.md @@ -0,0 +1,36 @@ +# REQ-002: Configuration Discovery + +## Overview + +DeepWork Reviews configuration files (`.deepreview`) can be placed anywhere within a project directory tree, similar to `.gitignore` files. The discovery system walks the project directory to find all `.deepreview` files, parses each one, and produces a flat list of review rules with their source directories preserved. Each file's glob patterns apply relative to the directory containing that file. + +## Requirements + +### REQ-002.1: File Discovery + +1. The system MUST provide a `find_deepreview_files(project_root)` function that returns a list of `.deepreview` file paths. +2. The function MUST search `project_root` and all its subdirectories recursively. +3. The function MUST only return files named exactly `.deepreview` (no other extensions or prefixes). +4. The function MUST NOT traverse directories that are typically excluded from version control (e.g., `.git/`, `node_modules/`, `__pycache__/`). +5. The function MUST return paths sorted by depth (deepest files first), then alphabetically within the same depth. +6. If no `.deepreview` files are found, the function MUST return an empty list. + +### REQ-002.2: Scope Boundaries + +1. The discovery MUST be bounded to the `project_root` directory and its descendants. +2. The discovery MUST NOT traverse above `project_root`. +3. The `project_root` SHOULD be the git repository root, but the system MUST NOT require it to be a git repository for discovery purposes. + +### REQ-002.3: Rule Loading + +1. The system MUST provide a `load_all_rules(project_root)` function that discovers all `.deepreview` files and parses each into `ReviewRule` objects. +2. Each `.deepreview` file MUST be parsed and validated according to REQ-001. +3. The `source_dir` of each `ReviewRule` MUST be set to the parent directory of the `.deepreview` file it was parsed from. +4. If a `.deepreview` file fails to parse (invalid YAML, schema validation failure, or missing instructions file), the error MUST be reported but MUST NOT prevent other `.deepreview` files from being processed. +5. `load_all_rules()` MUST return both the successfully parsed rules and a list of errors (file path + error message). +6. Rules from different `.deepreview` files MUST be independent — a rule from `src/.deepreview` does not interact with or override a rule from the root `.deepreview`. + +### REQ-002.4: Symlinks and Special Files + +1. The discovery MUST follow the default behavior of the underlying directory traversal (do not follow symlinks by default). +2. The system MUST skip unreadable directories or files and continue discovery. diff --git a/specs/deepwork/review/REQ-003-changed-file-detection.md b/specs/deepwork/review/REQ-003-changed-file-detection.md new file mode 100644 index 00000000..a9435454 --- /dev/null +++ b/specs/deepwork/review/REQ-003-changed-file-detection.md @@ -0,0 +1,39 @@ +# REQ-003: Changed File Detection + +## Overview + +DeepWork Reviews needs to determine which files have changed in order to match them against review rules. Changed files are detected using git, comparing the current state against a base reference (typically the main branch or a specific commit). The detection uses `subprocess` to invoke git directly, since GitPython is a dev-only dependency. + +## Requirements + +### REQ-003.1: Changed File Retrieval + +1. The system MUST provide a `get_changed_files(project_root, base_ref)` function that returns a list of changed file paths relative to the repository root. +2. The function MUST use `subprocess.run()` to invoke `git diff --name-only` with appropriate flags. +3. The function MUST use `--diff-filter=ACMR` to include only Added, Copied, Modified, and Renamed files (excluding Deleted files, since deleted files cannot be reviewed). +4. The function MUST combine committed changes on the branch (since diverging from the base ref), unstaged modifications to tracked files, and staged-but-not-committed changes into a single deduplicated list. +5. The function MUST NOT include untracked files (files never added to git). Only files known to git are detected. +6. The function MUST operate entirely on local git state. It MUST NOT fetch from or communicate with any remote repository. +7. Changed file paths MUST be returned relative to the git repository root. +8. The returned list MUST be sorted alphabetically. +9. The function MUST raise an error if the `git` command fails (e.g., not a git repository, invalid base ref). + +### REQ-003.2: Base Reference + +1. The `base_ref` parameter MUST default to `None`. +2. When `base_ref` is `None`, the system MUST auto-detect the merge base by finding the common ancestor between HEAD and the main branch. +3. The system MUST detect the main branch by checking for `main` first, then `master`, using `git rev-parse --verify`. +4. If neither `main` nor `master` exists, the system MUST fall back to `HEAD` (uncommitted changes only). +5. When `base_ref` is provided explicitly (e.g., `"main"`, `"HEAD"`, a commit SHA), the system MUST use it directly as the comparison target. +6. The system MUST use `git merge-base` to find the common ancestor when comparing against a branch name, to avoid including changes from the target branch itself. + +### REQ-003.3: Working Directory + +1. All git commands MUST be executed with `cwd` set to `project_root`. +2. The function MUST NOT change the process working directory. + +### REQ-003.4: Error Handling + +1. If the project is not a git repository, the function MUST raise an error with a descriptive message. +2. If the specified `base_ref` does not exist, the function MUST raise an error with a descriptive message. +3. Git subprocess errors MUST be caught and re-raised with the stderr output included in the error message. diff --git a/specs/deepwork/review/REQ-004-rule-matching-and-strategies.md b/specs/deepwork/review/REQ-004-rule-matching-and-strategies.md new file mode 100644 index 00000000..f1211fbc --- /dev/null +++ b/specs/deepwork/review/REQ-004-rule-matching-and-strategies.md @@ -0,0 +1,67 @@ +# REQ-004: Rule Matching and Review Strategies + +## Overview + +After discovering review rules (REQ-002) and changed files (REQ-003), the system matches changed files against each rule's glob patterns, then groups the matched files according to the rule's review strategy. The result is a list of `ReviewTask` objects, each representing a discrete review to be performed by an agent. Glob patterns are always resolved relative to the directory containing the `.deepreview` file that defined the rule. + +## Requirements + +### REQ-004.1: Glob Pattern Matching + +1. Include patterns MUST be matched against file paths that are relative to the rule's `source_dir`. +2. A changed file MUST first be checked to determine whether it resides under the rule's `source_dir`. If the changed file is not under `source_dir`, it MUST NOT match any patterns in that rule. +3. To test a match, the system MUST compute the changed file's path relative to `source_dir` and match it against the glob pattern. +4. The system MUST support `**` for recursive directory matching (e.g., `**/*.py` matches Python files at any depth). +5. The system MUST support `*` for single-component matching (e.g., `*.py` matches Python files in the current directory only). +6. A file MUST match a rule if it matches ANY of the `include` patterns AND does NOT match ANY of the `exclude` patterns. +7. If `exclude` is empty, only `include` patterns determine the match. + +### REQ-004.2: Review Task Data Model + +1. Each review task MUST be represented as a `ReviewTask` dataclass. +2. The `ReviewTask` MUST contain: `rule_name` (str), `files_to_review` (list[str] — paths relative to repo root), `instructions` (str), `agent_name` (str | None), `source_location` (str — formatted as `"path:line"`), `additional_files` (list[str] — unchanged matching files, relative to repo root), `all_changed_filenames` (list[str] | None). +3. `files_to_review` MUST always contain at least one file path. +4. `source_location` MUST be formatted as `"{relative_path}:{line_number}"` where the path is relative to the project root (e.g., `"src/.deepreview:5"`). + +### REQ-004.3: Strategy — individual + +1. When a rule's strategy is `"individual"`, the system MUST create one `ReviewTask` per matched changed file. +2. Each task's `files_to_review` MUST contain exactly one file path. +3. The task's `rule_name` MUST be the rule name. +4. The task's `instructions` MUST be the rule's resolved instruction text. + +### REQ-004.4: Strategy — matches_together + +1. When a rule's strategy is `"matches_together"`, the system MUST create a single `ReviewTask` containing all matched changed files. +2. The task's `files_to_review` MUST contain all matched changed files. +3. When the rule has `unchanged_matching_files: true`, the system MUST find all files under `source_dir` that match the `include` patterns (and do not match `exclude` patterns) but are NOT in the changed files list. +4. These unchanged matching files MUST be included in the task's `additional_files` list. +5. To discover unchanged matching files, the system MUST scan the filesystem using the rule's glob patterns relative to `source_dir`. + +### REQ-004.5: Strategy — all_changed_files + +1. When a rule's strategy is `"all_changed_files"`, the system MUST create a single `ReviewTask` only if at least one changed file matches the rule's patterns. +2. If triggered, the task's `files_to_review` MUST contain ALL changed files from the changeset (not just the matched ones). +3. This strategy acts as a "tripwire": the match patterns determine IF the review triggers, but the review itself covers the entire changeset. + +### REQ-004.6: Additional Context — all_changed_filenames + +1. When a rule has `all_changed_filenames: true`, every `ReviewTask` generated from that rule MUST include the full list of all changed filenames in `all_changed_filenames`. +2. This list MUST include ALL changed files in the changeset, regardless of whether they matched the rule's patterns. + +### REQ-004.7: No-Match Behavior + +1. If no changed files match a rule's patterns, the system MUST NOT create any `ReviewTask` for that rule. +2. If no rules produce any tasks, the overall result MUST be an empty list. + +### REQ-004.8: Agent Resolution + +1. When generating a `ReviewTask`, the system MUST accept a `platform` parameter (e.g., `"claude"`). +2. If the rule defines an `agent` mapping and the mapping contains a key matching the platform, the corresponding value MUST be set as the task's `agent_name`. +3. If the rule defines no `agent` mapping, or the mapping does not contain the target platform key, `agent_name` MUST be `None`. + +### REQ-004.9: Orchestration + +1. The system MUST provide a `match_files_to_rules(changed_files, rules, project_root, platform="claude")` function that processes all rules and returns a list of `ReviewTask` objects. +2. Each rule MUST be processed independently — a file can match multiple rules and appear in multiple tasks. +3. The function MUST process rules in the order they were discovered. diff --git a/specs/deepwork/review/REQ-005-instruction-generation.md b/specs/deepwork/review/REQ-005-instruction-generation.md new file mode 100644 index 00000000..e9a11c50 --- /dev/null +++ b/specs/deepwork/review/REQ-005-instruction-generation.md @@ -0,0 +1,49 @@ +# REQ-005: Review Instruction Generation + +## Overview + +For each `ReviewTask`, the system generates a self-contained markdown instruction file that provides a review agent with everything it needs: the review instructions, the files to examine, and any additional context. These files are written to `.deepwork/tmp/review_instructions/` and referenced by path in the final CLI output. The instruction files use `@path` syntax so that Claude Code automatically reads the referenced files when the prompt is loaded. + +## Requirements + +### REQ-005.1: Instruction File Content + +1. Each instruction file MUST be a valid markdown document. +2. The file MUST begin with a heading identifying the review rule and scope (e.g., `# Review: python_file_best_practices — src/app.py`). +3. The file MUST contain a "Review Instructions" section with the rule's resolved instruction text. +4. The file MUST contain a "Files to Review" section listing the file paths to examine. +5. File paths in the "Files to Review" section MUST be relative to the repository root. +6. When the task has `additional_files` (unchanged matching files), the file MUST contain an "Unchanged Matching Files" section listing those file paths. +7. When the task has `all_changed_filenames`, the file MUST contain an "All Changed Files" section listing every changed filename for context. + +### REQ-005.2: File Path Formatting + +1. File paths in the "Files to Review" section MUST be prefixed with `@` to trigger Claude Code's file-reading behavior (e.g., `@src/app.py`). +2. File paths in the "Unchanged Matching Files" section MUST also be prefixed with `@`. +3. File paths in the "All Changed Files" section MUST NOT be prefixed with `@` (they are listed for informational context only, not for the agent to read in full). + +### REQ-005.3: File Writing + +1. Instruction files MUST be written to `.deepwork/tmp/review_instructions/` under the project root. +2. The directory MUST be created if it does not exist (with `parents=True, exist_ok=True`). +3. Each file MUST have a unique filename with a `.md` extension. +4. The filename MUST be a random numeric ID (e.g., `7142141.md`) to avoid collisions. +5. The system MUST use `fs.safe_write()` for writing instruction files. +6. The system MUST return a list of `(ReviewTask, Path)` tuples mapping each task to its generated instruction file path. + +### REQ-005.4: Instruction Resolution + +1. When the rule's instructions reference a file (`{file: "path"}`), the system MUST resolve the path relative to the rule's `source_dir` and read the file contents. +2. The resolved instruction text MUST be included verbatim in the instruction file. +3. If the referenced file cannot be read, the system MUST raise an error with the file path and reason. + +### REQ-005.5: Cleanup + +1. The system SHOULD clear the `.deepwork/tmp/review_instructions/` directory at the start of each `deepwork review` invocation to avoid stale instruction files from previous runs. + +### REQ-005.6: Policy Traceability + +1. Each instruction file MUST end with a traceability line linking back to the source `.deepreview` file and rule location. +2. The traceability line MUST be formatted as: `This review was requested by the policy at \`{source_location}\`.` where `source_location` is the relative file path and line number (e.g., `src/.deepreview:5`). +3. The traceability line MUST be preceded by a markdown horizontal rule (`---`). +4. When `source_location` is empty, the traceability section MUST be omitted. diff --git a/specs/deepwork/review/REQ-006-cli-review-command.md b/specs/deepwork/review/REQ-006-cli-review-command.md new file mode 100644 index 00000000..c19263be --- /dev/null +++ b/specs/deepwork/review/REQ-006-cli-review-command.md @@ -0,0 +1,61 @@ +# REQ-006: CLI Review Command + +## Overview + +The `deepwork review` CLI command orchestrates the full DeepWork Reviews pipeline: discovering `.deepreview` config files, detecting changed files via git, matching them against rules, generating review instruction files, and outputting structured instructions for the target AI agent platform (currently Claude Code). The command is designed to be invoked via `!bash` syntax from a skill, with its stdout captured and used as agent instructions. + +## Requirements + +### REQ-006.1: Command Definition + +1. The `review` command MUST be a Click command registered in the DeepWork CLI. +2. The command MUST be registered in `src/deepwork/cli/main.py` via `cli.add_command(review)`. +3. The command MUST accept a `--instructions-for` option with choices `["claude"]` (required). +4. The command MUST accept a `--base-ref` option (string, default: `None`) specifying the git ref to diff against. +5. The command MUST accept a `--path` option (default: `"."`, must exist, must be a directory) specifying the project root. +6. The command MUST accept a `--files` option that can be specified multiple times to provide explicit file paths to review. + +### REQ-006.2: Pipeline Orchestration + +1. The command MUST execute the following pipeline in order: + a. Discover all `.deepreview` files under the project root (REQ-002). + b. Determine changed files (see REQ-006.6). + c. Match changed files against discovered rules and group by strategy (REQ-004). + d. Generate review instruction files for each task (REQ-005). + e. Format and output the results for the target platform. +2. If any step in the pipeline fails with an error, the command MUST print a user-friendly error message to stderr and exit with code 1. + +### REQ-006.3: Output Format for Claude Code + +1. When `--instructions-for claude` is specified, the command MUST output to stdout a structured text block that Claude Code can use to dispatch parallel review agents. +2. The output MUST begin with a line instructing the agent to invoke tasks in parallel. +3. For each review task, the output MUST include fields matching the Claude Code `Task` tool parameters: + a. A `name` field formatted as `"{rule_name} review of {file_or_scope}"` — for `individual` strategy, this is the single filename; for grouped strategies, this is a summary (e.g., `"3 files"` or `"all changed files"`). + b. A `description` field with a short (3-5 word) summary for the task (e.g., `"Review {rule_name}"`). + c. A `subagent_type` field set to the agent persona name (from the rule's `agent.claude` value) or `"general-purpose"` if no persona is specified. + d. A `prompt` field referencing the instruction file path relative to the project root, prefixed with `@` (e.g., `@.deepwork/tmp/review_instructions/7142141.md`). +4. The instruction file paths MUST be relative to the project root. + +### REQ-006.4: Empty Results + +1. If no `.deepreview` files are found, the command MUST output a message indicating no review configuration was found. +2. If no changed files are detected, the command MUST output a message indicating no changes were found to review. +3. If changed files exist but no rules match, the command MUST output a message indicating no review rules matched the changed files. + +### REQ-006.5: Error Handling + +1. Configuration parse errors MUST be reported to stderr but MUST NOT prevent other `.deepreview` files from being processed (see REQ-002.3). +2. Git errors MUST be reported to stderr and the command MUST exit with code 1. +3. Instruction file write errors MUST be reported to stderr and the command MUST exit with code 1. + +### REQ-006.6: Changed File Sources + +The command supports three sources for the list of files to review, with the following priority: + +1. **`--files` arguments** (highest priority): When one or more `--files` options are provided, the command MUST use those file paths directly and MUST NOT invoke git diff or read stdin. +2. **stdin** (second priority): When no `--files` are provided and stdin is not a TTY (i.e., input is piped), the command MUST read file paths from stdin (one per line, blank lines ignored). This allows chaining with other commands (e.g., `git diff --name-only HEAD~3 | deepwork review ...` or `find src -name '*.py' | deepwork review ...`). +3. **git diff** (default): When neither `--files` nor piped stdin is present, the command MUST fall back to detecting changed files via git diff (REQ-003). + +In all cases: +4. The resulting file list MUST be sorted and deduplicated. +5. The `--base-ref` option only applies to the git diff source; it MUST be ignored when `--files` or stdin provides the file list. diff --git a/specs/deepwork/review/REQ-007-plugin-skills.md b/specs/deepwork/review/REQ-007-plugin-skills.md new file mode 100644 index 00000000..e415c691 --- /dev/null +++ b/specs/deepwork/review/REQ-007-plugin-skills.md @@ -0,0 +1,42 @@ +# REQ-007: Plugin Skills and Documentation + +## Overview + +DeepWork Reviews is delivered to users via the Claude Code plugin. The plugin ships two skills (`review` and `configure_reviews`) and a usage reference (`README_REVIEWS.md`). The `review` skill runs the review pipeline and acts on findings. The `configure_reviews` skill helps users create and modify `.deepreview` config files. The reference documentation is symlinked into the plugin directory so it is available at runtime. + +## Requirements + +### REQ-007.1: Review Skill + +1. The plugin MUST include a `review` skill at `plugins/claude/skills/review/SKILL.md`. +2. The skill's YAML frontmatter MUST contain a `name` field set to `"review"`. +3. The skill's YAML frontmatter MUST contain a `description` field. +4. The skill MUST instruct the agent to run `uvx deepwork review --instructions-for claude` to generate review tasks. +5. The skill MUST instruct the agent to launch the generated review tasks in parallel. +6. The skill MUST instruct the agent to automatically apply findings that are obviously correct with no downsides (e.g., typo fixes, unused import removal). +7. The skill MUST instruct the agent to use AskUserQuestion for findings that involve trade-offs or subjective judgment. +8. The skill MUST instruct the agent to re-run the review after making changes, repeating until no further actionable findings remain. +9. The skill MUST route configuration requests (creating or modifying `.deepreview` files) to the `configure_reviews` skill. + +### REQ-007.2: Configure Reviews Skill + +1. The plugin MUST include a `configure_reviews` skill at `plugins/claude/skills/configure_reviews/SKILL.md`. +2. The skill's YAML frontmatter MUST contain a `name` field set to `"configure_reviews"`. +3. The skill's YAML frontmatter MUST contain a `description` field. +4. The skill MUST instruct the agent to read `README_REVIEWS.md` for the full configuration reference. +5. The skill MUST instruct the agent to look at existing `.deepreview` files and reuse rules/instructions where possible. +6. The skill MUST instruct the agent to test changes by running `deepwork review --instructions-for claude` and verifying the new rule appears in the output. + +### REQ-007.3: Reference Documentation + +1. The plugin MUST include `README_REVIEWS.md` at `plugins/claude/README_REVIEWS.md`. +2. This file MUST be a symlink to `../../README_REVIEWS.md` (the project root copy). +3. The symlink target MUST exist and be readable. +4. The `README_REVIEWS.md` MUST document the `.deepreview` configuration format, all review strategies, instruction variants (inline and file reference), agent personas, and CLI usage. +5. The documentation MUST recommend `.deepwork/review/` as the location for reusable instruction files. + +### REQ-007.4: Skill Directory Conventions + +1. Each skill MUST be in its own directory under `plugins/claude/skills/`. +2. Each skill directory MUST contain a `SKILL.md` file. +3. The `name` field in the YAML frontmatter MUST match the skill's directory name. diff --git a/specs/deepwork/review/REQ-008-get-configured-reviews.md b/specs/deepwork/review/REQ-008-get-configured-reviews.md new file mode 100644 index 00000000..7d7d12a9 --- /dev/null +++ b/specs/deepwork/review/REQ-008-get-configured-reviews.md @@ -0,0 +1,29 @@ +# REQ-008: Get Configured Reviews MCP Tool + +## Overview + +The `get_configured_reviews` MCP tool allows agents to query which review rules are configured in a project without running the full review pipeline. It returns rule metadata (name, description, defining file) and optionally filters to rules matching a given file list. + +## Requirements + +### REQ-008.1: Tool Registration + +1. The tool MUST be registered as `get_configured_reviews` on the MCP server. +2. The tool MUST accept an optional `only_rules_matching_files` parameter (list of file paths). +3. The tool MUST return a list of rule summaries (see REQ-008.2). + +### REQ-008.2: Rule Summary + +1. Each entry in the returned list MUST include `name` (str) — the rule name from the `.deepreview` file. +2. Each entry MUST include `description` (str) — the rule's description field. +3. Each entry MUST include `defining_file` (str) — a relative path to the `.deepreview` file with line number (e.g., `.deepreview:1`). + +### REQ-008.3: File Filtering + +1. When `only_rules_matching_files` is provided, the tool MUST return only rules whose include/exclude patterns match at least one of the provided files. +2. When `only_rules_matching_files` is omitted (None), the tool MUST return all configured rules. + +### REQ-008.4: Error Handling + +1. The tool MUST handle discovery errors gracefully — if some `.deepreview` files are invalid, valid rules from other files MUST still be returned. +2. Discovery errors MUST NOT cause the tool to raise an exception. diff --git a/src/deepwork/cli/main.py b/src/deepwork/cli/main.py index e031381f..f3194c11 100644 --- a/src/deepwork/cli/main.py +++ b/src/deepwork/cli/main.py @@ -13,9 +13,11 @@ def cli() -> None: # Import commands from deepwork.cli.hook import hook # noqa: E402 from deepwork.cli.install import install, sync # noqa: E402 +from deepwork.cli.review import review # noqa: E402 from deepwork.cli.serve import serve # noqa: E402 cli.add_command(hook) +cli.add_command(review) cli.add_command(serve) # DEPRECATION NOTICE: Remove after June 1st, 2026; details in PR https://github.com/Unsupervisedcom/deepwork/pull/227 diff --git a/src/deepwork/cli/review.py b/src/deepwork/cli/review.py new file mode 100644 index 00000000..f08f9555 --- /dev/null +++ b/src/deepwork/cli/review.py @@ -0,0 +1,142 @@ +"""CLI command for running DeepWork Reviews.""" + +import sys +from pathlib import Path + +import click + +from deepwork.review.discovery import load_all_rules +from deepwork.review.formatter import format_for_claude +from deepwork.review.instructions import write_instruction_files +from deepwork.review.matcher import GitDiffError, get_changed_files, match_files_to_rules + + +@click.command() +@click.option( + "--instructions-for", + "instructions_for", + type=click.Choice(["claude"]), + required=True, + help="Target platform for review instructions.", +) +@click.option( + "--base-ref", + "base_ref", + default=None, + help="Git ref to diff against. Auto-detects merge-base with main/master if not specified.", +) +@click.option( + "--path", + type=click.Path(exists=True, file_okay=False, dir_okay=True), + default=".", + help="Project root directory.", +) +@click.option( + "--files", + "file_args", + multiple=True, + help="Explicit file paths to review (skips git diff). Can be repeated.", +) +def review( + instructions_for: str, + base_ref: str | None, + path: str, + file_args: tuple[str, ...], +) -> None: + """Generate review instructions for changed files based on .deepreview configs. + + By default, detects changed files via git diff. You can override this by + passing explicit file paths with --files or by piping a file list on stdin: + + \b + # Explicit files + deepwork review --instructions-for claude --files src/app.py --files src/lib.py + + \b + # Pipe from git + git diff --name-only HEAD~3 | deepwork review --instructions-for claude + + \b + # Glob (shell expands the pattern) + printf '%s\n' src/**/*.py | deepwork review --instructions-for claude + + \b + # find + find src -name '*.py' | deepwork review --instructions-for claude + """ + project_root = Path(path).resolve() + + # Step 1: Discover .deepreview files and parse rules + rules, discovery_errors = load_all_rules(project_root) + + for err in discovery_errors: + click.echo(f"Warning: {err.file_path}: {err.error}", err=True) + + if not rules: + click.echo("No .deepreview configuration files found.") + return + + # Step 2: Determine changed files + changed_files = _resolve_changed_files(file_args, project_root, base_ref) + if changed_files is None: + # Error already printed + sys.exit(1) + + if not changed_files: + click.echo("No changed files detected.") + return + + # Step 3: Match changed files against rules + tasks = match_files_to_rules(changed_files, rules, project_root, instructions_for) + + if not tasks: + click.echo("No review rules matched the changed files.") + return + + # Step 4: Generate instruction files + try: + task_files = write_instruction_files(tasks, project_root) + except OSError as e: + click.echo(f"Error writing instruction files: {e}", err=True) + sys.exit(1) + + # Step 5: Format and output + if instructions_for == "claude": + output = format_for_claude(task_files, project_root) + click.echo(output) + + +def _resolve_changed_files( + file_args: tuple[str, ...], + project_root: Path, + base_ref: str | None, +) -> list[str] | None: + """Determine the list of changed files from args, stdin, or git diff. + + Priority: + 1. --files arguments (if any provided) + 2. stdin (if piped, not a TTY) + 3. git diff (default) + + Returns: + Sorted deduplicated list of file paths, or None on error. + """ + if file_args: + return _normalize_file_list(list(file_args)) + + if not sys.stdin.isatty(): + lines = sys.stdin.read().strip().splitlines() + files = [line.strip() for line in lines if line.strip()] + if files: + return _normalize_file_list(files) + + try: + return get_changed_files(project_root, base_ref) + except GitDiffError as e: + click.echo(f"Error: {e}", err=True) + return None + + +def _normalize_file_list(files: list[str]) -> list[str]: + """Deduplicate and sort a list of file paths.""" + return sorted(set(files)) diff --git a/src/deepwork/cli/serve.py b/src/deepwork/cli/serve.py index 87e673ef..bd4817c7 100644 --- a/src/deepwork/cli/serve.py +++ b/src/deepwork/cli/serve.py @@ -42,12 +42,19 @@ class ServeError(Exception): default=None, help="External runner for quality gate reviews. 'claude' uses Claude CLI subprocess. Default: None (agent self-review).", ) +@click.option( + "--platform", + type=str, + default=None, + help="Platform identifier (e.g., 'claude'). Used by the review tool to format output.", +) def serve( path: Path, no_quality_gate: bool, transport: str, port: int, external_runner: str | None, + platform: str | None, ) -> None: """Start the DeepWork MCP server. @@ -76,7 +83,7 @@ def serve( deepwork serve --path /path/to/project """ try: - _serve_mcp(path, not no_quality_gate, transport, port, external_runner) + _serve_mcp(path, not no_quality_gate, transport, port, external_runner, platform) except ServeError as e: click.echo(f"Error: {e}", err=True) raise click.Abort() from e @@ -91,6 +98,7 @@ def _serve_mcp( transport: str, port: int, external_runner: str | None = None, + platform: str | None = None, ) -> None: """Start the MCP server. @@ -101,6 +109,7 @@ def _serve_mcp( port: Port for SSE transport external_runner: External runner for quality gate reviews. "claude" uses Claude CLI subprocess. None means agent self-review. + platform: Platform identifier for the review tool (e.g., "claude"). Raises: ServeError: If server fails to start @@ -116,6 +125,7 @@ def _serve_mcp( project_root=project_path, enable_quality_gate=enable_quality_gate, external_runner=external_runner, + platform=platform, ) if transport == "stdio": diff --git a/src/deepwork/jobs/mcp/server.py b/src/deepwork/jobs/mcp/server.py index db48d342..0785dc37 100644 --- a/src/deepwork/jobs/mcp/server.py +++ b/src/deepwork/jobs/mcp/server.py @@ -39,6 +39,7 @@ def create_server( quality_gate_timeout: int = 120, quality_gate_max_attempts: int = 3, external_runner: str | None = None, + platform: str | None = None, ) -> FastMCP: """Create and configure the MCP server. @@ -50,6 +51,8 @@ def create_server( external_runner: External runner for quality gate reviews. "claude" uses Claude CLI subprocess. None means agent self-review via instructions file. (default: None) + platform: Platform identifier for the review tool (e.g., "claude"). + Defaults to "claude" if not set. (default: None) Returns: Configured FastMCP server instance @@ -218,6 +221,53 @@ async def abort_workflow( response = await tools.abort_workflow(input_data) return response.model_dump() + # ---- Review tool (outside the workflow lifecycle) ---- + + from deepwork.review.mcp import ( + ReviewToolError, + run_review, + ) + from deepwork.review.mcp import ( + get_configured_reviews as get_configured_reviews_fn, + ) + + review_platform = platform or "claude" + + @mcp.tool( + description=( + "Run a review of changed files based on .deepreview configuration files. " + "Returns a list of review tasks to invoke in parallel. Each task has " + "name, description, subagent_type, and prompt fields for the Task tool. " + "Optional: files (list of file paths to review). When omitted, detects " + "changes via git diff against the main branch." + ) + ) + def get_review_instructions(files: list[str] | None = None) -> str: + """Run review pipeline on changed files.""" + _log_tool_call("get_review_instructions", {"files": files}) + try: + return run_review(project_path, review_platform, files) + except ReviewToolError as e: + return f"Review error: {e}" + + @mcp.tool( + description=( + "List all configured review rules from .deepreview files. " + "Returns each rule's name, description, and defining file. " + "Optional: only_rules_matching_files (list of file paths) to filter " + "to rules that would apply to those specific files." + ) + ) + def get_configured_reviews( + only_rules_matching_files: list[str] | None = None, + ) -> list[dict[str, str]]: + """List configured review rules, optionally filtered by file paths.""" + _log_tool_call( + "get_configured_reviews", + {"only_rules_matching_files": only_rules_matching_files}, + ) + return get_configured_reviews_fn(project_path, only_rules_matching_files) + return mcp diff --git a/src/deepwork/jobs/mcp/tools.py b/src/deepwork/jobs/mcp/tools.py index ad6c2c2c..580dff1d 100644 --- a/src/deepwork/jobs/mcp/tools.py +++ b/src/deepwork/jobs/mcp/tools.py @@ -287,11 +287,15 @@ def get_workflows(self) -> GetWorkflowsResponse: """ jobs, load_errors = self._load_all_jobs() job_infos = [self._job_to_info(job) for job in jobs] + repair_hint = ( + "\nThis project likely needs `/deepwork:repair` run to correct the issue" + " unless the offending file(s) were changed this session and the agent can fix it directly." + ) error_infos = [ JobLoadErrorInfo( job_name=e.job_name, job_dir=e.job_dir, - error=e.error, + error=e.error + repair_hint, ) for e in load_errors ] diff --git a/src/deepwork/review/__init__.py b/src/deepwork/review/__init__.py new file mode 100644 index 00000000..dea4fb56 --- /dev/null +++ b/src/deepwork/review/__init__.py @@ -0,0 +1 @@ +"""DeepWork Reviews - Automated code review based on .deepreview configuration files.""" diff --git a/src/deepwork/review/config.py b/src/deepwork/review/config.py new file mode 100644 index 00000000..72e99bfd --- /dev/null +++ b/src/deepwork/review/config.py @@ -0,0 +1,198 @@ +"""Configuration parsing for .deepreview files. + +Parses .deepreview YAML files into ReviewRule dataclasses, validating +against the JSON schema and resolving instruction file references. +""" + +import re +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any + +from deepwork.review.schema import DEEPREVIEW_SCHEMA +from deepwork.utils.validation import ValidationError, validate_against_schema +from deepwork.utils.yaml_utils import YAMLError, load_yaml + + +class ConfigError(Exception): + """Exception raised for .deepreview configuration errors.""" + + pass + + +@dataclass +class ReviewRule: + """A single named review rule from a .deepreview file.""" + + name: str + description: str + include_patterns: list[str] + exclude_patterns: list[str] + strategy: str # "individual" | "matches_together" | "all_changed_files" + instructions: str # Resolved instruction text + agent: dict[str, str] | None + all_changed_filenames: bool + unchanged_matching_files: bool + source_dir: Path # Directory containing the .deepreview file + source_file: Path # Path to the .deepreview file + source_line: int # Line number of the rule name in the .deepreview file + + +@dataclass +class ReviewTask: + """A single review task to be executed by an agent.""" + + rule_name: str + files_to_review: list[str] # Paths relative to repo root + instructions: str + agent_name: str | None # Agent persona for the target platform + source_location: str = "" # e.g. "src/.deepreview:5" + additional_files: list[str] = field(default_factory=list) # Unchanged matching files + all_changed_filenames: list[str] | None = None + + +def parse_deepreview_file(filepath: Path) -> list[ReviewRule]: + """Parse a .deepreview YAML file into ReviewRule objects. + + Args: + filepath: Path to the .deepreview file. + + Returns: + List of ReviewRule objects parsed from the file. + + Raises: + ConfigError: If the file cannot be parsed or fails validation. + """ + try: + data = load_yaml(filepath) + except YAMLError as e: + raise ConfigError(f"Failed to parse {filepath}: {e}") from e + + if data is None: + raise ConfigError(f"File not found: {filepath}") + + if not data: + return [] + + try: + validate_against_schema(data, DEEPREVIEW_SCHEMA) + except ValidationError as e: + raise ConfigError(f"Schema validation failed for {filepath}: {e}") from e + + source_dir = filepath.parent + line_numbers = _find_rule_line_numbers(filepath) + rules: list[ReviewRule] = [] + + for rule_name, rule_data in data.items(): + line = line_numbers.get(rule_name, 1) + rule = _parse_rule(rule_name, rule_data, source_dir, filepath, line) + rules.append(rule) + + return rules + + +def _parse_rule( + name: str, + data: dict[str, Any], + source_dir: Path, + source_file: Path, + source_line: int, +) -> ReviewRule: + """Parse a single rule from its YAML data. + + Args: + name: The rule name (YAML key). + data: The rule's YAML data. + source_dir: Directory containing the .deepreview file. + source_file: Path to the .deepreview file. + source_line: Line number of the rule name in the file. + + Returns: + A ReviewRule object. + + Raises: + ConfigError: If instruction file references cannot be resolved. + """ + description = data["description"] + match_data = data["match"] + review_data = data["review"] + + include_patterns = match_data["include"] + exclude_patterns = match_data.get("exclude", []) + + strategy = review_data["strategy"] + instructions = _resolve_instructions(review_data["instructions"], source_dir) + agent = review_data.get("agent") + + additional_context = review_data.get("additional_context", {}) + all_changed_filenames = additional_context.get("all_changed_filenames", False) + unchanged_matching_files = additional_context.get("unchanged_matching_files", False) + + return ReviewRule( + name=name, + description=description, + include_patterns=include_patterns, + exclude_patterns=exclude_patterns, + strategy=strategy, + instructions=instructions, + agent=agent, + all_changed_filenames=all_changed_filenames, + unchanged_matching_files=unchanged_matching_files, + source_dir=source_dir, + source_file=source_file, + source_line=source_line, + ) + + +def _resolve_instructions(instructions: str | dict[str, Any], source_dir: Path) -> str: + """Resolve instruction text — either inline string or file reference. + + Args: + instructions: Either a string (inline) or a dict with 'file' key. + source_dir: Directory for resolving relative file paths. + + Returns: + The resolved instruction text. + + Raises: + ConfigError: If the referenced file does not exist or cannot be read. + """ + if isinstance(instructions, str): + return instructions + + # Must be a dict with 'file' key (validated by schema) + file_ref: str = instructions["file"] + file_path = source_dir / file_ref + if not file_path.exists(): + raise ConfigError(f"Instructions file not found: {file_path}") + + try: + return file_path.read_text(encoding="utf-8") + except OSError as e: + raise ConfigError(f"Failed to read instructions file {file_path}: {e}") from e + + +# Matches a top-level YAML key (no leading whitespace, followed by colon) +_RULE_NAME_RE = re.compile(r"^([a-zA-Z0-9_-]+)\s*:", re.MULTILINE) + + +def _find_rule_line_numbers(filepath: Path) -> dict[str, int]: + """Scan a .deepreview file to find the line number of each top-level rule key. + + Args: + filepath: Path to the .deepreview file. + + Returns: + Dict mapping rule name to its 1-based line number. + """ + try: + text = filepath.read_text(encoding="utf-8") + except OSError: + return {} + + result: dict[str, int] = {} + for i, line in enumerate(text.splitlines(), start=1): + m = _RULE_NAME_RE.match(line) + if m: + result[m.group(1)] = i + return result diff --git a/src/deepwork/review/discovery.py b/src/deepwork/review/discovery.py new file mode 100644 index 00000000..8dc74507 --- /dev/null +++ b/src/deepwork/review/discovery.py @@ -0,0 +1,119 @@ +"""Discovery of .deepreview configuration files in the project tree. + +Walks the project directory to find all .deepreview files and parses +them into ReviewRule objects. +""" + +from dataclasses import dataclass +from pathlib import Path + +from deepwork.review.config import ConfigError, ReviewRule, parse_deepreview_file + +# Directories to skip during discovery +_SKIP_DIRS = { + ".git", + "node_modules", + "__pycache__", + ".venv", + "venv", + ".tox", + ".mypy_cache", + ".pytest_cache", + ".ruff_cache", + ".eggs", +} + +# Suffix patterns that can't be matched via set membership +_SKIP_SUFFIXES = (".egg-info",) + +DEEPREVIEW_FILENAME = ".deepreview" + + +@dataclass +class DiscoveryError: + """An error encountered while loading a .deepreview file.""" + + file_path: Path + error: str + + +def find_deepreview_files(project_root: Path) -> list[Path]: + """Find all .deepreview files in the project directory tree. + + Walks project_root and its subdirectories recursively, skipping + common non-source directories (.git, node_modules, etc.). + + Args: + project_root: Root directory to search. + + Returns: + List of .deepreview file paths, sorted by depth (deepest first), + then alphabetically within the same depth. + """ + results: list[Path] = [] + + for path in _walk_for_file(project_root, DEEPREVIEW_FILENAME): + results.append(path) + + # Sort by depth (deepest first), then alphabetically + root_depth = len(project_root.parts) + results.sort(key=lambda p: (-len(p.parts) + root_depth, str(p))) + + return results + + +def _walk_for_file(root: Path, filename: str) -> list[Path]: + """Walk directory tree looking for files with the given name. + + Skips directories in _SKIP_DIRS. + + Args: + root: Root directory to walk. + filename: Exact filename to look for. + + Returns: + List of matching file paths. + """ + results: list[Path] = [] + + try: + entries = sorted(root.iterdir()) + except PermissionError: + return results + + for entry in entries: + if entry.is_file() and entry.name == filename: + results.append(entry) + elif ( + entry.is_dir() + and entry.name not in _SKIP_DIRS + and not entry.name.endswith(_SKIP_SUFFIXES) + ): + results.extend(_walk_for_file(entry, filename)) + + return results + + +def load_all_rules( + project_root: Path, +) -> tuple[list[ReviewRule], list[DiscoveryError]]: + """Discover all .deepreview files and parse them into rules. + + Args: + project_root: Root directory to search. + + Returns: + Tuple of (successfully parsed rules, list of errors). + """ + files = find_deepreview_files(project_root) + all_rules: list[ReviewRule] = [] + errors: list[DiscoveryError] = [] + + for filepath in files: + try: + rules = parse_deepreview_file(filepath) + all_rules.extend(rules) + except ConfigError as e: + errors.append(DiscoveryError(file_path=filepath, error=str(e))) + + return all_rules, errors diff --git a/src/deepwork/review/formatter.py b/src/deepwork/review/formatter.py new file mode 100644 index 00000000..c338d80b --- /dev/null +++ b/src/deepwork/review/formatter.py @@ -0,0 +1,77 @@ +"""Output formatting for review task instructions. + +Formats ReviewTask results into structured text that AI agent platforms +(e.g., Claude Code) can use to dispatch parallel review agents. +""" + +from pathlib import Path + +from deepwork.review.config import ReviewTask + + +def format_for_claude( + task_files: list[tuple[ReviewTask, Path]], + project_root: Path, +) -> str: + """Format review tasks as Claude Code parallel task invocations. + + Produces structured text that Claude Code can parse to dispatch + multiple review agents in parallel. + + Args: + task_files: List of (ReviewTask, instruction_file_path) tuples. + project_root: Absolute path to the project root. + + Returns: + Formatted instruction text for Claude Code. + """ + if not task_files: + return "No review tasks to execute." + + lines: list[str] = [] + lines.append("Invoke the following list of Tasks in parallel:\n") + + for task, file_path in task_files: + # Make the instruction file path relative to project root + try: + rel_path = file_path.relative_to(project_root) + except ValueError: + rel_path = file_path + + name = _task_name(task) + description = _task_description(task) + subagent_type = task.agent_name or "general-purpose" + + lines.append(f'name: "{name}"') + lines.append(f"\tdescription: {description}") + lines.append(f"\tsubagent_type: {subagent_type}") + lines.append(f'\tprompt: "@{rel_path}"') + lines.append("") + + return "\n".join(lines) + + +def _task_description(task: ReviewTask) -> str: + """Generate a short description for a review task. + + Args: + task: The ReviewTask to describe. + + Returns: + Short (3-5 word) description string. + """ + return f"Review {task.rule_name}" + + +def _task_name(task: ReviewTask) -> str: + """Generate a descriptive name for a review task. + + Args: + task: The ReviewTask to name. + + Returns: + Task name string. + """ + if len(task.files_to_review) == 1: + return f"{task.rule_name} review of {task.files_to_review[0]}" + return f"{task.rule_name} review of {len(task.files_to_review)} files" diff --git a/src/deepwork/review/instructions.py b/src/deepwork/review/instructions.py new file mode 100644 index 00000000..6c3c11d8 --- /dev/null +++ b/src/deepwork/review/instructions.py @@ -0,0 +1,127 @@ +"""Review instruction file generation. + +Generates self-contained markdown instruction files for review agents. +Each file contains the review instructions, files to examine, and any +additional context. Written to .deepwork/tmp/review_instructions/. +""" + +import random +import shutil +from pathlib import Path + +from deepwork.review.config import ReviewTask +from deepwork.utils.fs import safe_write + +INSTRUCTIONS_DIR = ".deepwork/tmp/review_instructions" + + +def write_instruction_files( + tasks: list[ReviewTask], + project_root: Path, +) -> list[tuple[ReviewTask, Path]]: + """Write instruction files for all review tasks. + + Clears any existing instruction files, then generates a new .md file + for each task in .deepwork/tmp/review_instructions/. + + Args: + tasks: List of ReviewTask objects to generate files for. + project_root: Absolute path to the project root. + + Returns: + List of (ReviewTask, instruction_file_path) tuples. + """ + instructions_dir = project_root / INSTRUCTIONS_DIR + + # Clear previous instruction files + if instructions_dir.exists(): + shutil.rmtree(instructions_dir) + instructions_dir.mkdir(parents=True, exist_ok=True) + + results: list[tuple[ReviewTask, Path]] = [] + + for task in tasks: + content = build_instruction_file(task) + file_id = random.randint(1000000, 9999999) + file_path = instructions_dir / f"{file_id}.md" + + # Ensure unique filename + while file_path.exists(): + file_id = random.randint(1000000, 9999999) + file_path = instructions_dir / f"{file_id}.md" + + safe_write(file_path, content) + results.append((task, file_path)) + + return results + + +def build_instruction_file(task: ReviewTask) -> str: + """Build the markdown content for a single review instruction file. + + Args: + task: The ReviewTask to generate instructions for. + + Returns: + Markdown string containing the complete review instructions. + """ + parts: list[str] = [] + + # Header + scope = _describe_scope(task) + parts.append(f"# Review: {task.rule_name} — {scope}\n") + + # Review instructions + parts.append("## Review Instructions\n") + parts.append(task.instructions.strip()) + parts.append("") + + # Files to review + parts.append("## Files to Review\n") + for filepath in task.files_to_review: + parts.append(f"- @{filepath}") + parts.append("") + + # Additional context: unchanged matching files + if task.additional_files: + parts.append("## Unchanged Matching Files\n") + parts.append( + "These files match the review patterns but were not changed. " + "They are provided for context.\n" + ) + for filepath in task.additional_files: + parts.append(f"- @{filepath}") + parts.append("") + + # Additional context: all changed filenames + if task.all_changed_filenames: + parts.append("## All Changed Files\n") + parts.append( + "The following files were changed in this changeset " + "(listed for context, not all are subject to this review).\n" + ) + for filepath in task.all_changed_filenames: + parts.append(f"- {filepath}") + parts.append("") + + # Traceability: link back to the source policy + if task.source_location: + parts.append("---\n") + parts.append(f"This review was requested by the policy at `{task.source_location}`.") + parts.append("") + + return "\n".join(parts) + + +def _describe_scope(task: ReviewTask) -> str: + """Generate a human-readable scope description for the task. + + Args: + task: The ReviewTask to describe. + + Returns: + Scope description string. + """ + if len(task.files_to_review) == 1: + return task.files_to_review[0] + return f"{len(task.files_to_review)} files" diff --git a/src/deepwork/review/matcher.py b/src/deepwork/review/matcher.py new file mode 100644 index 00000000..b292389b --- /dev/null +++ b/src/deepwork/review/matcher.py @@ -0,0 +1,428 @@ +"""Changed file detection and rule matching. + +Detects changed files via git and matches them against ReviewRule glob +patterns, grouping results into ReviewTask objects according to the +rule's review strategy. +""" + +import re +import subprocess +from pathlib import Path + +from deepwork.review.config import ReviewRule, ReviewTask + + +class GitDiffError(Exception): + """Exception raised for git diff operation errors.""" + + pass + + +def get_changed_files(project_root: Path, base_ref: str | None = None) -> list[str]: + """Get list of changed files relative to the repository root. + + Uses git diff to detect Added, Copied, Modified, and Renamed files. + Combines unstaged and staged changes into a deduplicated list. + + Args: + project_root: Path to the project (must be in a git repo). + base_ref: Git ref to diff against. If None, auto-detects + merge-base with main/master branch. Falls back to HEAD. + + Returns: + Sorted list of changed file paths relative to repo root. + + Raises: + GitDiffError: If git operations fail. + """ + if base_ref is None: + base_ref = _detect_base_ref(project_root) + + # Get the merge-base to avoid including changes from the target branch + merge_base = _get_merge_base(project_root, base_ref) + + # Get changed files (unstaged + committed on branch) + diff_files = _git_diff_name_only(project_root, merge_base) + + # Also get staged changes (not yet committed) + staged_files = _git_diff_name_only(project_root, None, staged=True) + + return sorted(set(diff_files + staged_files)) + + +def _detect_base_ref(project_root: Path) -> str: + """Auto-detect the main branch to use as base ref. + + Checks for 'main' first, then 'master'. Falls back to 'HEAD'. + + Args: + project_root: Path to the project root. + + Returns: + Branch name to use as base ref. + """ + for branch in ("main", "master"): + try: + subprocess.run( + ["git", "rev-parse", "--verify", branch], + cwd=project_root, + capture_output=True, + text=True, + check=True, + ) + return branch + except subprocess.CalledProcessError: + continue + return "HEAD" + + +def _get_merge_base(project_root: Path, ref: str) -> str: + """Get the merge-base between HEAD and the given ref. + + If ref is HEAD, returns HEAD directly. + + Args: + project_root: Path to the project root. + ref: Git ref to find merge-base with. + + Returns: + The merge-base commit SHA, or the ref itself for HEAD. + + Raises: + GitDiffError: If the git command fails. + """ + if ref == "HEAD": + return "HEAD" + + try: + result = subprocess.run( + ["git", "merge-base", "HEAD", ref], + cwd=project_root, + capture_output=True, + text=True, + check=True, + ) + return result.stdout.strip() + except subprocess.CalledProcessError as e: + raise GitDiffError(f"Failed to find merge-base with '{ref}': {e.stderr.strip()}") from e + + +def _git_diff_name_only(project_root: Path, ref: str | None, *, staged: bool = False) -> list[str]: + """Run git diff --name-only and return the list of files. + + Args: + project_root: Path to the project root. + ref: Git ref to diff against. If None and staged=True, diffs staged changes. + staged: If True, include --cached flag for staged changes. + + Returns: + List of changed file paths. + + Raises: + GitDiffError: If the git command fails. + """ + cmd = ["git", "diff", "--name-only", "--diff-filter=ACMR"] + if staged: + cmd.append("--cached") + if ref is not None: + cmd.append(ref) + + try: + result = subprocess.run( + cmd, + cwd=project_root, + capture_output=True, + text=True, + check=True, + ) + return [f for f in result.stdout.strip().split("\n") if f] + except subprocess.CalledProcessError as e: + raise GitDiffError(f"git diff failed: {e.stderr.strip()}") from e + + +def match_files_to_rules( + changed_files: list[str], + rules: list[ReviewRule], + project_root: Path, + platform: str = "claude", +) -> list[ReviewTask]: + """Match changed files against rules and produce ReviewTask objects. + + Each rule is processed independently. A file can match multiple rules + and appear in multiple tasks. + + Args: + changed_files: List of changed file paths relative to repo root. + rules: List of ReviewRule objects to match against. + project_root: Absolute path to the project root. + platform: Target platform for agent resolution (e.g., "claude"). + + Returns: + List of ReviewTask objects. + """ + tasks: list[ReviewTask] = [] + + for rule in rules: + matched = match_rule(changed_files, rule, project_root) + if not matched: + continue + + agent_name = _resolve_agent(rule, platform) + all_filenames = changed_files if rule.all_changed_filenames else None + source_location = format_source_location(rule, project_root) + + if rule.strategy == "individual": + for filepath in matched: + tasks.append( + ReviewTask( + rule_name=rule.name, + files_to_review=[filepath], + instructions=rule.instructions, + agent_name=agent_name, + source_location=source_location, + all_changed_filenames=all_filenames, + ) + ) + + elif rule.strategy == "matches_together": + additional = [] + if rule.unchanged_matching_files: + additional = _find_unchanged_matching_files(changed_files, rule, project_root) + tasks.append( + ReviewTask( + rule_name=rule.name, + files_to_review=matched, + instructions=rule.instructions, + agent_name=agent_name, + source_location=source_location, + additional_files=additional, + all_changed_filenames=all_filenames, + ) + ) + + elif rule.strategy == "all_changed_files": + tasks.append( + ReviewTask( + rule_name=rule.name, + files_to_review=list(changed_files), + instructions=rule.instructions, + agent_name=agent_name, + source_location=source_location, + all_changed_filenames=all_filenames, + ) + ) + + return tasks + + +def match_rule(changed_files: list[str], rule: ReviewRule, project_root: Path) -> list[str]: + """Find changed files that match a rule's include/exclude patterns. + + Patterns are resolved relative to the rule's source_dir. + + Args: + changed_files: List of changed file paths relative to repo root. + rule: The ReviewRule to match against. + project_root: Absolute path to the project root. + + Returns: + List of matched file paths (relative to repo root). + """ + matched: list[str] = [] + source_dir = rule.source_dir + + # Compute source_dir relative to project_root for path comparison + try: + source_rel = source_dir.relative_to(project_root) + except ValueError: + # source_dir is not under project_root — skip this rule + return [] + + for filepath in changed_files: + # Check if file is under the rule's source directory + rel_to_source = _relative_to_dir(filepath, str(source_rel)) + if rel_to_source is None: + continue + + # Check include patterns + if not any(_glob_match(rel_to_source, pattern) for pattern in rule.include_patterns): + continue + + # Check exclude patterns + if any(_glob_match(rel_to_source, pattern) for pattern in rule.exclude_patterns): + continue + + matched.append(filepath) + + return matched + + +def _relative_to_dir(filepath: str, dir_path: str) -> str | None: + """Compute a filepath relative to a directory. + + Both paths should be relative to the same root. If the file is not + under the directory, returns None. + + Args: + filepath: File path (relative to some root). + dir_path: Directory path (relative to the same root). + + Returns: + The filepath relative to dir_path, or None if not under it. + """ + if dir_path == "" or dir_path == ".": + return filepath + + prefix = dir_path.rstrip("/") + "/" + if filepath.startswith(prefix): + return filepath[len(prefix) :] + + return None + + +def _glob_match(filepath: str, pattern: str) -> bool: + """Match a file path against a glob pattern. + + Uses glob semantics: ``*`` matches within a single path component + (does not cross ``/``), and ``**`` matches zero or more path components. + + Args: + filepath: File path to check (relative to rule source dir). + pattern: Glob pattern to match against. + + Returns: + True if the filepath matches the pattern. + """ + regex = _glob_to_regex(pattern) + return bool(re.match(regex, filepath)) + + +def _glob_to_regex(pattern: str) -> str: + """Convert a glob pattern to an anchored regex. + + - ``**`` matches zero or more path components (including separators). + - ``*`` matches any characters except ``/``. + - ``?`` matches any single character except ``/``. + - All other characters are escaped. + + Args: + pattern: Glob pattern string. + + Returns: + Regex pattern string anchored with ``^...$``. + """ + parts: list[str] = [] + i = 0 + n = len(pattern) + while i < n: + if i + 1 < n and pattern[i : i + 2] == "**": + # ** followed by / matches zero or more directories + if i + 2 < n and pattern[i + 2] == "/": + parts.append("(?:.+/)?") + i += 3 + else: + # ** at end matches everything remaining + parts.append(".*") + i += 2 + elif pattern[i] == "*": + parts.append("[^/]*") + i += 1 + elif pattern[i] == "?": + parts.append("[^/]") + i += 1 + else: + parts.append(re.escape(pattern[i])) + i += 1 + return "^" + "".join(parts) + "$" + + +def _resolve_agent(rule: ReviewRule, platform: str) -> str | None: + """Resolve the agent persona for the target platform. + + Args: + rule: The ReviewRule to check. + platform: Target platform (e.g., "claude"). + + Returns: + Agent persona name, or None if not specified. + """ + if rule.agent is None: + return None + return rule.agent.get(platform) + + +def format_source_location(rule: ReviewRule, project_root: Path) -> str: + """Format the source location of a rule as 'relative/path:line'. + + Args: + rule: The ReviewRule with source_file and source_line. + project_root: Project root for computing relative paths. + + Returns: + Source location string (e.g., "src/.deepreview:5"). + """ + try: + rel_path = rule.source_file.relative_to(project_root) + except ValueError: + rel_path = rule.source_file + return f"{rel_path}:{rule.source_line}" + + +def _find_unchanged_matching_files( + changed_files: list[str], + rule: ReviewRule, + project_root: Path, +) -> list[str]: + """Find files that match the rule's patterns but were not changed. + + Scans the filesystem under the rule's source_dir for files matching + the include patterns (minus exclude patterns), then removes any that + appear in the changed files list. + + Args: + changed_files: List of changed file paths relative to repo root. + rule: The ReviewRule with include/exclude patterns. + project_root: Absolute path to the project root. + + Returns: + List of unchanged matching file paths (relative to repo root). + """ + source_dir = rule.source_dir + changed_set = set(changed_files) + unchanged: list[str] = [] + + try: + source_rel = source_dir.relative_to(project_root) + except ValueError: + return [] + + # Walk the source directory for matching files. + # We intentionally follow symlinks here so that symlinked source trees + # are included in the unchanged-file search. + for include_pattern in rule.include_patterns: + for match_path in source_dir.glob(include_pattern): + if not match_path.is_file(): + continue + + # Get path relative to project root + try: + rel_path = str(match_path.relative_to(project_root)) + except ValueError: + continue + + # Skip if it's a changed file + if rel_path in changed_set: + continue + + # Get path relative to source dir for exclude check + rel_to_source = _relative_to_dir(rel_path, str(source_rel)) + if rel_to_source is None: + continue + + # Check exclude patterns + if any(_glob_match(rel_to_source, pattern) for pattern in rule.exclude_patterns): + continue + + unchanged.append(rel_path) + + return sorted(set(unchanged)) diff --git a/src/deepwork/review/mcp.py b/src/deepwork/review/mcp.py new file mode 100644 index 00000000..986c8207 --- /dev/null +++ b/src/deepwork/review/mcp.py @@ -0,0 +1,141 @@ +"""MCP adapter for the DeepWork review pipeline. + +Thin adapter that exposes the review pipeline as a single function +suitable for registration as an MCP tool. +""" + +from pathlib import Path + +from deepwork.review.discovery import load_all_rules +from deepwork.review.formatter import format_for_claude +from deepwork.review.instructions import write_instruction_files +from deepwork.review.matcher import ( + GitDiffError, + format_source_location, + get_changed_files, + match_files_to_rules, + match_rule, +) + +FORMATTERS = { + "claude": format_for_claude, +} + +SUPPORTED_PLATFORMS = set(FORMATTERS.keys()) + + +class ReviewToolError(Exception): + """Exception raised for review tool errors (git failures, write failures).""" + + pass + + +def run_review( + project_root: Path, + platform: str, + files: list[str] | None = None, +) -> str: + """Run the review pipeline and return formatted output. + + Args: + project_root: Absolute path to the project root. + platform: Target platform for formatting (e.g., "claude"). + files: Explicit file list. If None, detects changes via git diff. + + Returns: + Formatted review output string. + + Raises: + ReviewToolError: On git failures, write failures, or unsupported platform. + """ + if platform not in SUPPORTED_PLATFORMS: + raise ReviewToolError( + f"Unsupported platform: '{platform}'. " + f"Supported platforms: {', '.join(sorted(SUPPORTED_PLATFORMS))}" + ) + + # Step 1: Discover .deepreview files and parse rules + rules, discovery_errors = load_all_rules(project_root) + + if not rules: + if discovery_errors: + warnings = "\n".join(f" - {e.file_path}: {e.error}" for e in discovery_errors) + return f"No valid .deepreview rules found. Parse errors:\n{warnings}" + return "No .deepreview configuration files found." + + # Step 2: Determine changed files + if files is not None: + changed_files = sorted(set(files)) + else: + try: + changed_files = get_changed_files(project_root) + except GitDiffError as e: + raise ReviewToolError(f"Git error: {e}") from e + + if not changed_files: + return "No changed files detected." + + # Step 3: Match changed files against rules + tasks = match_files_to_rules(changed_files, rules, project_root, platform) + + if not tasks: + return "No review rules matched the changed files." + + # Step 4: Generate instruction files + try: + task_files = write_instruction_files(tasks, project_root) + except OSError as e: + raise ReviewToolError(f"Error writing instruction files: {e}") from e + + # Step 5: Format output + formatter = FORMATTERS[platform] + result = formatter(task_files, project_root) + + if discovery_errors: + warnings = "\n".join(f" - {e.file_path}: {e.error}" for e in discovery_errors) + result = f"Warning: Some .deepreview files could not be parsed:\n{warnings}\n\n{result}" + + return result + + +def get_configured_reviews( + project_root: Path, + only_rules_matching_files: list[str] | None = None, +) -> list[dict[str, str]]: + """Return metadata for configured review rules. + + Args: + project_root: Absolute path to the project root. + only_rules_matching_files: When provided, only return rules whose + include/exclude patterns match at least one of these files. + + Returns: + List of dicts with ``name``, ``description``, and ``defining_file``. + """ + rules, errors = load_all_rules(project_root) + + if only_rules_matching_files is not None: + rules = [ + rule for rule in rules if match_rule(only_rules_matching_files, rule, project_root) + ] + + result = [ + { + "name": rule.name, + "description": rule.description, + "defining_file": format_source_location(rule, project_root), + } + for rule in rules + ] + + if errors: + for err in errors: + result.append( + { + "name": f"PARSE_ERROR:{err.file_path}", + "description": err.error, + "defining_file": str(err.file_path), + } + ) + + return result diff --git a/src/deepwork/review/schema.py b/src/deepwork/review/schema.py new file mode 100644 index 00000000..5adab19a --- /dev/null +++ b/src/deepwork/review/schema.py @@ -0,0 +1,31 @@ +"""JSON Schema loader for .deepreview configuration files. + +This module loads the deepreview_schema.json file and provides it as a Python dict +for use with jsonschema validation. +""" + +import json +from pathlib import Path +from typing import Any + +_SCHEMA_FILE = Path(__file__).parent.parent / "schemas" / "deepreview_schema.json" + + +def _load_schema() -> dict[str, Any]: + """Load the JSON schema from file.""" + with open(_SCHEMA_FILE) as f: + result: dict[str, Any] = json.load(f) + return result + + +# Load the schema at module import time +DEEPREVIEW_SCHEMA: dict[str, Any] = _load_schema() + + +def get_schema_path() -> Path: + """Get the path to the JSON schema file. + + Returns: + Path to deepreview_schema.json + """ + return _SCHEMA_FILE diff --git a/src/deepwork/schemas/deepreview_schema.json b/src/deepwork/schemas/deepreview_schema.json new file mode 100644 index 00000000..f865e88f --- /dev/null +++ b/src/deepwork/schemas/deepreview_schema.json @@ -0,0 +1,122 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "DeepWork Reviews Configuration", + "description": "Configuration schema for .deepreview files used by the deepwork CLI.", + "type": "object", + "additionalProperties": false, + "patternProperties": { + "^[a-zA-Z0-9_-]+$": { + "type": "object", + "description": "A named review rule defining what to match and how to review it.", + "required": [ + "description", + "match", + "review" + ], + "additionalProperties": false, + "properties": { + "description": { + "type": "string", + "description": "A short description of what this review rule checks for.", + "maxLength": 256 + }, + "match": { + "type": "object", + "description": "Defines the file glob patterns that trigger this review rule.", + "required": [ + "include" + ], + "additionalProperties": false, + "properties": { + "include": { + "type": "array", + "description": "List of glob patterns to include in the trigger.", + "items": { + "type": "string", + "examples": [ + "**/*.py", + "src/components/**/*.tsx" + ] + }, + "minItems": 1 + }, + "exclude": { + "type": "array", + "description": "List of glob patterns to exclude from the trigger.", + "items": { + "type": "string" + } + } + } + }, + "review": { + "type": "object", + "description": "Defines how the matched files should be grouped and reviewed by the AI agent.", + "required": [ + "strategy", + "instructions" + ], + "additionalProperties": false, + "properties": { + "strategy": { + "type": "string", + "description": "How to batch the review tasks for the matched files.", + "enum": [ + "individual", + "matches_together", + "all_changed_files" + ] + }, + "agent": { + "type": "object", + "description": "Optional mapping of CLI providers to specific agent personas. If omitted, the default agent is used.", + "patternProperties": { + "^[a-zA-Z0-9_-]+$": { + "type": "string", + "description": "The specific persona or model name to use for this provider (e.g., 'security-expert')." + } + } + }, + "instructions": { + "description": "The prompt instructions for the review. Can be a literal string or a reference to an external file.", + "anyOf": [ + { + "type": "string", + "description": "Inline text instructions for the review." + }, + { + "type": "object", + "required": [ + "file" + ], + "additionalProperties": false, + "properties": { + "file": { + "type": "string", + "description": "Path to a markdown file containing the review instructions." + } + } + } + ] + }, + "additional_context": { + "type": "object", + "description": "Optional flags to include additional context in the AI's prompt.", + "additionalProperties": false, + "properties": { + "all_changed_filenames": { + "type": "boolean", + "description": "If true, includes a list of all files changed in the PR/commit." + }, + "unchanged_matching_files": { + "type": "boolean", + "description": "If true, pulls the full contents of files that match the include patterns even if they weren't modified in this diff." + } + } + } + } + } + } + } + } +} diff --git a/src/deepwork/standard_jobs/deepwork_jobs/job.yml b/src/deepwork/standard_jobs/deepwork_jobs/job.yml index 530a015f..508dad0e 100644 --- a/src/deepwork/standard_jobs/deepwork_jobs/job.yml +++ b/src/deepwork/standard_jobs/deepwork_jobs/job.yml @@ -248,3 +248,4 @@ steps: "Deepwork Skill Preserved": "The `deepwork` skill folder still exists in `.claude/skills/deepwork/`." "Rules Folder Removed": "`.deepwork/rules/` folder is gone." "Rules Job Removed": "`.deepwork/jobs/deepwork_rules/` is gone." + "MCP Server Entry Removed": "The `deepwork serve` entry is removed from `.mcp.json` (or the file is deleted if empty)." diff --git a/src/deepwork/standard_jobs/deepwork_jobs/steps/errata.md b/src/deepwork/standard_jobs/deepwork_jobs/steps/errata.md index e9e43605..bcbb37f1 100644 --- a/src/deepwork/standard_jobs/deepwork_jobs/steps/errata.md +++ b/src/deepwork/standard_jobs/deepwork_jobs/steps/errata.md @@ -120,7 +120,55 @@ platforms: Update if needed to match current schema expectations. -### Step 5: Remove Other Obsolete Files +### Step 5: Remove `deepwork serve` from `.mcp.json` + +Old DeepWork versions added a `deepwork serve` MCP server entry directly to the repo's `.mcp.json` file. This is now handled by the plugin system and must be removed. + +**Process:** + +1. Check if `.mcp.json` exists in the repo root: + ```bash + cat .mcp.json 2>/dev/null || echo "No .mcp.json found" + ``` + +2. If it exists, look for any entry whose `command` is `deepwork` with `serve` as an argument (e.g., `"command": "deepwork", "args": ["serve"]` or `"command": "uvx", "args": ["deepwork", "serve", ...]`). Remove that entire server entry. + +3. If `.mcp.json` becomes empty (no remaining server entries) after removal, delete the file entirely: + ```bash + rm .mcp.json + ``` + +4. If other MCP servers remain, keep the file with only the `deepwork serve` entry removed. + +**Example `.mcp.json` before cleanup:** +```json +{ + "mcpServers": { + "deepwork": { + "command": "uvx", + "args": ["deepwork", "serve"] + }, + "other-server": { + "command": "some-tool", + "args": ["serve"] + } + } +} +``` + +**After cleanup (other servers remain):** +```json +{ + "mcpServers": { + "other-server": { + "command": "some-tool", + "args": ["serve"] + } + } +} +``` + +### Step 6: Remove Other Obsolete Files Check for and remove other obsolete files: @@ -132,7 +180,7 @@ Check for and remove other obsolete files: | `.claude/commands/` | Generated commands | Keep (current system) | | `.claude/settings.local.json` | Local overrides | Keep (user settings) | -### Step 6: Re-install DeepWork +### Step 7: Re-install DeepWork After all cleanup is complete, re-run `deepwork install` to ensure configurations are current and consistent: @@ -148,7 +196,7 @@ deepwork install If any issues are found, fix them before proceeding. The goal is a clean, working DeepWork installation with no residual problems from the repair process. -### Step 7: Verify Git Status +### Step 8: Verify Git Status Check that the cleanup hasn't left untracked garbage: diff --git a/tests/integration/test_quality_gate_integration.py b/tests/integration/test_quality_gate_integration.py index 38efa0c0..7ce73d9f 100644 --- a/tests/integration/test_quality_gate_integration.py +++ b/tests/integration/test_quality_gate_integration.py @@ -34,8 +34,10 @@ # Skip marker for tests that require real Claude CLI # GitHub Actions sets CI=true, as do most other CI systems requires_claude_cli = pytest.mark.skipif( - os.environ.get("CI") == "true" or os.environ.get("GITHUB_ACTIONS") == "true", - reason="Integration tests require Claude CLI - skipped in CI", + os.environ.get("CI") == "true" + or os.environ.get("GITHUB_ACTIONS") == "true" + or os.environ.get("CLAUDECODE") is not None, + reason="Integration tests require Claude CLI - skipped in CI and nested Claude sessions", ) diff --git a/tests/unit/review/__init__.py b/tests/unit/review/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/review/test_config.py b/tests/unit/review/test_config.py new file mode 100644 index 00000000..03627c84 --- /dev/null +++ b/tests/unit/review/test_config.py @@ -0,0 +1,376 @@ +"""Tests for deepreview configuration parsing (deepwork.review.config) — validates REQ-001.""" + +from pathlib import Path + +import pytest + +from deepwork.review.config import ConfigError, parse_deepreview_file + + +def _write_deepreview(path: Path, content: str) -> Path: + """Write a .deepreview file and return its path.""" + filepath = path / ".deepreview" + filepath.write_text(content, encoding="utf-8") + return filepath + + +class TestParseDeepReviewFile: + """Tests for parse_deepreview_file.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.1.1, REQ-001.1.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_parses_valid_yaml_with_single_rule(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +python_review: + description: "Review Python files." + match: + include: + - "**/*.py" + review: + strategy: individual + instructions: "Review this Python file." +""", + ) + rules = parse_deepreview_file(filepath) + assert len(rules) == 1 + assert rules[0].name == "python_review" + assert rules[0].include_patterns == ["**/*.py"] + assert rules[0].strategy == "individual" + assert rules[0].instructions == "Review this Python file." + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.1.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_parses_multiple_rules(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +rule_a: + description: "Rule A description." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: "Rule A" +rule_b: + description: "Rule B description." + match: + include: ["**/*.ts"] + review: + strategy: matches_together + instructions: "Rule B" +""", + ) + rules = parse_deepreview_file(filepath) + assert len(rules) == 2 + names = {r.name for r in rules} + assert names == {"rule_a", "rule_b"} + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.2.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_parses_exclude_patterns(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + exclude: ["tests/**/*.py"] + review: + strategy: individual + instructions: "Review it." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].exclude_patterns == ["tests/**/*.py"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.8.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_exclude_defaults_to_empty(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: "Review it." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].exclude_patterns == [] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_parses_all_strategies(self, tmp_path: Path) -> None: + for strategy in ("individual", "matches_together", "all_changed_files"): + filepath = _write_deepreview( + tmp_path, + f""" +rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: {strategy} + instructions: "Review." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].strategy == strategy + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.4.1, REQ-001.4.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_inline_instructions(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: "Check for bugs." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].instructions == "Check for bugs." + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.4.1, REQ-001.4.3, REQ-001.4.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_file_reference_instructions(self, tmp_path: Path) -> None: + (tmp_path / "review_guide.md").write_text( + "# Review Guide\nCheck everything.", encoding="utf-8" + ) + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: + file: review_guide.md +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].instructions == "# Review Guide\nCheck everything." + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.4.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_file_reference_not_found_raises_error(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: + file: nonexistent.md +""", + ) + with pytest.raises(ConfigError, match="Instructions file not found"): + parse_deepreview_file(filepath) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.5.1, REQ-001.5.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_parses_agent_config(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + agent: + claude: security-expert + instructions: "Review." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].agent == {"claude": "security-expert"} + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.8.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_agent_defaults_to_none(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: "Review." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].agent is None + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.6.2, REQ-001.6.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_parses_additional_context(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + additional_context: + all_changed_filenames: true + unchanged_matching_files: true + instructions: "Review." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].all_changed_filenames is True + assert rules[0].unchanged_matching_files is True + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.8.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_additional_context_defaults_to_false(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: "Review." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].all_changed_filenames is False + assert rules[0].unchanged_matching_files is False + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.8.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_source_dir_set_to_parent(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: "Review." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].source_dir == tmp_path + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.7.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_invalid_yaml_raises_error(self, tmp_path: Path) -> None: + filepath = _write_deepreview(tmp_path, "invalid: [yaml") + with pytest.raises(ConfigError, match="Failed to parse"): + parse_deepreview_file(filepath) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.7.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_schema_validation_failure_raises_error(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +my_rule: + description: "Test rule." + match: + include: ["**/*.py"] + review: + strategy: invalid_strategy + instructions: "Review." +""", + ) + with pytest.raises(ConfigError, match="Schema validation failed"): + parse_deepreview_file(filepath) + + def test_empty_file_returns_empty_list(self, tmp_path: Path) -> None: + filepath = _write_deepreview(tmp_path, "") + rules = parse_deepreview_file(filepath) + assert rules == [] + + def test_nonexistent_file_raises_error(self, tmp_path: Path) -> None: + filepath = tmp_path / ".deepreview" + with pytest.raises(ConfigError, match="File not found"): + parse_deepreview_file(filepath) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-001.8.4, REQ-001.8.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_source_file_is_set(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """ +python_review: + description: "Test rule." + match: + include: + - "**/*.py" + review: + strategy: individual + instructions: "Review." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].source_file == filepath + + def test_source_line_for_single_rule(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """python_review: + description: "Test rule." + match: + include: + - "**/*.py" + review: + strategy: individual + instructions: "Review." +""", + ) + rules = parse_deepreview_file(filepath) + assert rules[0].source_line == 1 + + def test_source_line_for_multiple_rules(self, tmp_path: Path) -> None: + filepath = _write_deepreview( + tmp_path, + """rule_a: + description: "Rule A." + match: + include: + - "**/*.py" + review: + strategy: individual + instructions: "Review A." +rule_b: + description: "Rule B." + match: + include: + - "**/*.js" + review: + strategy: individual + instructions: "Review B." +""", + ) + rules = parse_deepreview_file(filepath) + rule_map = {r.name: r for r in rules} + assert rule_map["rule_a"].source_line == 1 + assert rule_map["rule_b"].source_line == 9 diff --git a/tests/unit/review/test_discovery.py b/tests/unit/review/test_discovery.py new file mode 100644 index 00000000..1aeebe58 --- /dev/null +++ b/tests/unit/review/test_discovery.py @@ -0,0 +1,136 @@ +"""Tests for deepreview configuration discovery (deepwork.review.discovery) — validates REQ-002.""" + +from pathlib import Path + +from deepwork.review.discovery import find_deepreview_files, load_all_rules + + +def _write_deepreview(path: Path, content: str) -> Path: + """Write a .deepreview file in the given directory.""" + path.mkdir(parents=True, exist_ok=True) + filepath = path / ".deepreview" + filepath.write_text(content, encoding="utf-8") + return filepath + + +VALID_CONFIG = """ +my_rule: + description: "Test rule for Python files." + match: + include: ["**/*.py"] + review: + strategy: individual + instructions: "Review it." +""" + + +class TestFindDeepReviewFiles: + """Tests for find_deepreview_files.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.1.1, REQ-002.1.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_finds_file_in_root(self, tmp_path: Path) -> None: + _write_deepreview(tmp_path, VALID_CONFIG) + files = find_deepreview_files(tmp_path) + assert len(files) == 1 + assert files[0] == tmp_path / ".deepreview" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.1.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_finds_files_in_subdirectories(self, tmp_path: Path) -> None: + _write_deepreview(tmp_path, VALID_CONFIG) + _write_deepreview(tmp_path / "src", VALID_CONFIG) + _write_deepreview(tmp_path / "src" / "lib", VALID_CONFIG) + files = find_deepreview_files(tmp_path) + assert len(files) == 3 + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.1.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_deepest_first_ordering(self, tmp_path: Path) -> None: + _write_deepreview(tmp_path, VALID_CONFIG) + _write_deepreview(tmp_path / "src", VALID_CONFIG) + _write_deepreview(tmp_path / "src" / "lib", VALID_CONFIG) + files = find_deepreview_files(tmp_path) + # Deepest first + assert files[0] == tmp_path / "src" / "lib" / ".deepreview" + assert files[1] == tmp_path / "src" / ".deepreview" + assert files[2] == tmp_path / ".deepreview" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.1.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_returns_empty_when_none_found(self, tmp_path: Path) -> None: + files = find_deepreview_files(tmp_path) + assert files == [] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.1.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_skips_git_directory(self, tmp_path: Path) -> None: + git_dir = tmp_path / ".git" + git_dir.mkdir() + (git_dir / ".deepreview").write_text(VALID_CONFIG) + files = find_deepreview_files(tmp_path) + assert files == [] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.1.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_skips_node_modules(self, tmp_path: Path) -> None: + nm_dir = tmp_path / "node_modules" + nm_dir.mkdir() + (nm_dir / ".deepreview").write_text(VALID_CONFIG) + _write_deepreview(tmp_path, VALID_CONFIG) + files = find_deepreview_files(tmp_path) + assert len(files) == 1 + assert files[0] == tmp_path / ".deepreview" + + +class TestLoadAllRules: + """Tests for load_all_rules.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.3.1, REQ-002.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_loads_rules_from_single_file(self, tmp_path: Path) -> None: + _write_deepreview(tmp_path, VALID_CONFIG) + rules, errors = load_all_rules(tmp_path) + assert len(rules) == 1 + assert rules[0].name == "my_rule" + assert errors == [] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.3.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_rules_from_multiple_files_are_independent(self, tmp_path: Path) -> None: + _write_deepreview(tmp_path, VALID_CONFIG) + _write_deepreview( + tmp_path / "src", + """ +src_rule: + description: "Test rule for TypeScript files." + match: + include: ["**/*.ts"] + review: + strategy: matches_together + instructions: "Review TS files." +""", + ) + rules, errors = load_all_rules(tmp_path) + assert len(rules) == 2 + names = {r.name for r in rules} + assert names == {"my_rule", "src_rule"} + assert errors == [] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.3.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_source_dir_set_correctly(self, tmp_path: Path) -> None: + _write_deepreview(tmp_path / "src", VALID_CONFIG) + rules, errors = load_all_rules(tmp_path) + assert len(rules) == 1 + assert rules[0].source_dir == tmp_path / "src" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-002.3.4, REQ-002.3.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_invalid_file_reports_error_without_blocking(self, tmp_path: Path) -> None: + _write_deepreview(tmp_path, VALID_CONFIG) + _write_deepreview(tmp_path / "bad", "invalid: [yaml") + rules, errors = load_all_rules(tmp_path) + assert len(rules) == 1 # Valid rule still loaded + assert len(errors) == 1 + assert "bad" in str(errors[0].file_path) diff --git a/tests/unit/review/test_formatter.py b/tests/unit/review/test_formatter.py new file mode 100644 index 00000000..4eb03044 --- /dev/null +++ b/tests/unit/review/test_formatter.py @@ -0,0 +1,99 @@ +"""Tests for output formatting (deepwork.review.formatter) — validates REQ-006.""" + +from pathlib import Path + +from deepwork.review.config import ReviewTask +from deepwork.review.formatter import format_for_claude + + +def _make_task( + rule_name: str = "test_rule", + files: list[str] | None = None, + agent_name: str | None = None, +) -> ReviewTask: + """Create a ReviewTask with sensible defaults.""" + return ReviewTask( + rule_name=rule_name, + files_to_review=files or ["src/app.py"], + instructions="Review it.", + agent_name=agent_name, + ) + + +class TestFormatForClaude: + """Tests for format_for_claude.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.3.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_empty_tasks_returns_no_tasks_message(self, tmp_path: Path) -> None: + result = format_for_claude([], tmp_path) + assert "No review tasks" in result + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_output_starts_with_invoke_line(self, tmp_path: Path) -> None: + task = _make_task() + file_path = tmp_path / ".deepwork" / "tmp" / "review_instructions" / "123.md" + file_path.parent.mkdir(parents=True) + file_path.write_text("content") + result = format_for_claude([(task, file_path)], tmp_path) + assert result.startswith("Invoke the following list of Tasks in parallel:") + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.3.3a). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_individual_task_name_includes_filename(self, tmp_path: Path) -> None: + task = _make_task(rule_name="py_review", files=["src/app.py"]) + file_path = tmp_path / "instructions.md" + result = format_for_claude([(task, file_path)], tmp_path) + assert 'name: "py_review review of src/app.py"' in result + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.3.3a). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_grouped_task_name_includes_file_count(self, tmp_path: Path) -> None: + task = _make_task(rule_name="py_review", files=["a.py", "b.py", "c.py"]) + file_path = tmp_path / "instructions.md" + result = format_for_claude([(task, file_path)], tmp_path) + assert 'name: "py_review review of 3 files"' in result + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.3.3b). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_default_subagent_type_when_no_agent(self, tmp_path: Path) -> None: + task = _make_task(agent_name=None) + file_path = tmp_path / "instructions.md" + result = format_for_claude([(task, file_path)], tmp_path) + assert "subagent_type: general-purpose" in result + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.3.3b). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_custom_subagent_type(self, tmp_path: Path) -> None: + task = _make_task(agent_name="security-expert") + file_path = tmp_path / "instructions.md" + result = format_for_claude([(task, file_path)], tmp_path) + assert "subagent_type: security-expert" in result + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.3.3c). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_description_field_present(self, tmp_path: Path) -> None: + task = _make_task(rule_name="py_review") + file_path = tmp_path / "instructions.md" + result = format_for_claude([(task, file_path)], tmp_path) + assert "description: Review py_review" in result + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.3.3c, REQ-006.3.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_prompt_references_instruction_file(self, tmp_path: Path) -> None: + task = _make_task() + file_path = tmp_path / ".deepwork" / "tmp" / "review_instructions" / "7142141.md" + file_path.parent.mkdir(parents=True) + file_path.write_text("content") + result = format_for_claude([(task, file_path)], tmp_path) + assert 'prompt: "@.deepwork/tmp/review_instructions/7142141.md"' in result + + def test_multiple_tasks(self, tmp_path: Path) -> None: + task_a = _make_task(rule_name="rule_a", files=["a.py"]) + task_b = _make_task(rule_name="rule_b", files=["b.py"]) + file_a = tmp_path / "a.md" + file_b = tmp_path / "b.md" + result = format_for_claude([(task_a, file_a), (task_b, file_b)], tmp_path) + assert 'name: "rule_a review of a.py"' in result + assert 'name: "rule_b review of b.py"' in result diff --git a/tests/unit/review/test_instructions.py b/tests/unit/review/test_instructions.py new file mode 100644 index 00000000..5dc5b608 --- /dev/null +++ b/tests/unit/review/test_instructions.py @@ -0,0 +1,187 @@ +"""Tests for review instruction file generation (deepwork.review.instructions) — validates REQ-005.""" + +from pathlib import Path + +from deepwork.review.config import ReviewTask +from deepwork.review.instructions import ( + build_instruction_file, + write_instruction_files, +) + + +def _make_task( + rule_name: str = "test_rule", + files: list[str] | None = None, + instructions: str = "Review this.", + agent_name: str | None = None, + source_location: str = ".deepreview:1", + additional_files: list[str] | None = None, + all_changed_filenames: list[str] | None = None, +) -> ReviewTask: + """Create a ReviewTask with sensible defaults.""" + return ReviewTask( + rule_name=rule_name, + files_to_review=files or ["src/app.py"], + instructions=instructions, + agent_name=agent_name, + source_location=source_location, + additional_files=additional_files or [], + all_changed_filenames=all_changed_filenames, + ) + + +class TestBuildInstructionFile: + """Tests for build_instruction_file.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.1.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_includes_rule_name_in_header(self) -> None: + task = _make_task(rule_name="python_review") + content = build_instruction_file(task) + assert "# Review: python_review" in content + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.1.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_includes_review_instructions(self) -> None: + task = _make_task(instructions="Check for security issues.") + content = build_instruction_file(task) + assert "## Review Instructions" in content + assert "Check for security issues." in content + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.1.4, REQ-005.2.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_includes_files_to_review_with_at_prefix(self) -> None: + task = _make_task(files=["src/app.py", "src/lib.py"]) + content = build_instruction_file(task) + assert "## Files to Review" in content + assert "- @src/app.py" in content + assert "- @src/lib.py" in content + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.1.6, REQ-005.2.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_includes_unchanged_matching_files(self) -> None: + task = _make_task(additional_files=["src/unchanged.py"]) + content = build_instruction_file(task) + assert "## Unchanged Matching Files" in content + assert "- @src/unchanged.py" in content + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.1.7, REQ-005.2.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_includes_all_changed_filenames_without_at_prefix(self) -> None: + task = _make_task(all_changed_filenames=["src/app.py", "tests/test.py", "README.md"]) + content = build_instruction_file(task) + assert "## All Changed Files" in content + assert "- src/app.py" in content + assert "- README.md" in content + # Should NOT have @ prefix for context-only files + lines = content.split("\n") + all_changed_section = False + for line in lines: + if "## All Changed Files" in line: + all_changed_section = True + elif line.startswith("##"): + all_changed_section = False + if all_changed_section and line.startswith("- "): + assert not line.startswith("- @"), ( + f"Context-only files should not have @ prefix: {line}" + ) + + def test_omits_unchanged_section_when_empty(self) -> None: + task = _make_task(additional_files=[]) + content = build_instruction_file(task) + assert "## Unchanged Matching Files" not in content + + def test_omits_all_changed_section_when_none(self) -> None: + task = _make_task(all_changed_filenames=None) + content = build_instruction_file(task) + assert "## All Changed Files" not in content + + def test_single_file_scope_description(self) -> None: + task = _make_task(files=["src/app.py"]) + content = build_instruction_file(task) + assert "src/app.py" in content.split("\n")[0] + + def test_multi_file_scope_description(self) -> None: + task = _make_task(files=["a.py", "b.py", "c.py"]) + content = build_instruction_file(task) + assert "3 files" in content.split("\n")[0] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.6.1, REQ-005.6.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_includes_traceability_blurb(self) -> None: + task = _make_task(source_location="src/.deepreview:5") + content = build_instruction_file(task) + assert "This review was requested by the policy at `src/.deepreview:5`." in content + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.6.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_traceability_preceded_by_horizontal_rule(self) -> None: + task = _make_task(source_location="src/.deepreview:5") + content = build_instruction_file(task) + lines = content.strip().split("\n") + # Find the traceability line + trace_idx = next(i for i, line in enumerate(lines) if "This review was requested" in line) + # The line two above should be the horizontal rule (blank line between) + assert "---" in lines[trace_idx - 2] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.6.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_omits_traceability_when_source_location_empty(self) -> None: + task = _make_task(source_location="") + content = build_instruction_file(task) + assert "This review was requested" not in content + + def test_traceability_is_at_end(self) -> None: + task = _make_task(source_location=".deepreview:1") + content = build_instruction_file(task) + last_nonblank = [line for line in content.strip().split("\n") if line.strip()][-1] + assert "This review was requested" in last_nonblank + + +class TestWriteInstructionFiles: + """Tests for write_instruction_files.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.3.1, REQ-005.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_creates_instruction_files(self, tmp_path: Path) -> None: + tasks = [_make_task(rule_name="rule_a"), _make_task(rule_name="rule_b")] + results = write_instruction_files(tasks, tmp_path) + assert len(results) == 2 + + for _task, file_path in results: + assert file_path.exists() + assert file_path.suffix == ".md" + assert ".deepwork/tmp/review_instructions" in str(file_path) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.3.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_unique_filenames(self, tmp_path: Path) -> None: + tasks = [_make_task() for _ in range(10)] + results = write_instruction_files(tasks, tmp_path) + paths = [r[1] for r in results] + assert len(set(paths)) == 10 # All unique + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.5.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_clears_previous_files(self, tmp_path: Path) -> None: + instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" + instructions_dir.mkdir(parents=True) + (instructions_dir / "old_file.md").write_text("stale") + + tasks = [_make_task()] + write_instruction_files(tasks, tmp_path) + + # Old file should be gone + assert not (instructions_dir / "old_file.md").exists() + # New file should exist + files = list(instructions_dir.iterdir()) + assert len(files) == 1 + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-005.3.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_returns_task_path_tuples(self, tmp_path: Path) -> None: + task = _make_task(rule_name="my_rule") + results = write_instruction_files([task], tmp_path) + assert len(results) == 1 + assert results[0][0] is task + assert results[0][1].exists() diff --git a/tests/unit/review/test_matcher.py b/tests/unit/review/test_matcher.py new file mode 100644 index 00000000..584a0d57 --- /dev/null +++ b/tests/unit/review/test_matcher.py @@ -0,0 +1,507 @@ +"""Tests for changed file detection and rule matching (deepwork.review.matcher) — validates REQ-003, REQ-004.""" + +import inspect +import os +import subprocess +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, call, patch + +import pytest + +from deepwork.review.config import ReviewRule +from deepwork.review.matcher import ( + GitDiffError, + _glob_match, + _relative_to_dir, + get_changed_files, + match_files_to_rules, + match_rule, +) + + +def _make_rule( + name: str = "test_rule", + include: list[str] | None = None, + exclude: list[str] | None = None, + strategy: str = "individual", + instructions: str = "Review it.", + agent: dict[str, str] | None = None, + all_changed_filenames: bool = False, + unchanged_matching_files: bool = False, + source_dir: Path = Path("/project"), + source_file: Path | None = None, + source_line: int = 1, +) -> ReviewRule: + """Create a ReviewRule with sensible defaults for testing.""" + return ReviewRule( + name=name, + description="Test rule description.", + include_patterns=include or ["**/*.py"], + exclude_patterns=exclude or [], + strategy=strategy, + instructions=instructions, + agent=agent, + all_changed_filenames=all_changed_filenames, + unchanged_matching_files=unchanged_matching_files, + source_dir=source_dir, + source_file=source_file or source_dir / ".deepreview", + source_line=source_line, + ) + + +class TestRelativeToDir: + """Tests for _relative_to_dir helper.""" + + def test_file_under_dir(self) -> None: + assert _relative_to_dir("src/app.py", "src") == "app.py" + + def test_file_under_nested_dir(self) -> None: + assert _relative_to_dir("src/lib/utils.py", "src") == "lib/utils.py" + + def test_file_not_under_dir(self) -> None: + assert _relative_to_dir("tests/test.py", "src") is None + + def test_root_dir(self) -> None: + assert _relative_to_dir("app.py", ".") == "app.py" + + def test_empty_dir(self) -> None: + assert _relative_to_dir("app.py", "") == "app.py" + + +class TestGlobMatch: + """Tests for _glob_match helper.""" + + def test_recursive_star_star(self) -> None: + assert _glob_match("lib/utils.py", "**/*.py") is True + + def test_recursive_star_star_no_match(self) -> None: + assert _glob_match("lib/utils.ts", "**/*.py") is False + + def test_single_star(self) -> None: + assert _glob_match("app.py", "*.py") is True + + def test_single_star_no_match_in_subdir(self) -> None: + # * should NOT match across / boundaries + assert _glob_match("lib/app.py", "*.py") is False + + def test_specific_path(self) -> None: + assert _glob_match("config/settings.yaml", "config/*") is True + + def test_specific_path_no_match(self) -> None: + assert _glob_match("other/settings.yaml", "config/*") is False + + +class TestMatchRule: + """Tests for match_rule.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.1.1, REQ-004.1.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_matches_include_pattern(self, tmp_path: Path) -> None: + rule = _make_rule(include=["**/*.py"], source_dir=tmp_path) + matched = match_rule(["src/app.py", "src/lib.ts"], rule, tmp_path) + assert matched == ["src/app.py"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.1.6, REQ-004.1.7). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_exclude_pattern_filters_out(self, tmp_path: Path) -> None: + rule = _make_rule( + include=["**/*.py"], + exclude=["tests/**/*.py"], + source_dir=tmp_path, + ) + matched = match_rule(["src/app.py", "tests/test_app.py"], rule, tmp_path) + assert matched == ["src/app.py"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.1.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_file_outside_source_dir_not_matched(self, tmp_path: Path) -> None: + source = tmp_path / "src" + source.mkdir() + rule = _make_rule(include=["**/*.py"], source_dir=source) + matched = match_rule(["tests/test.py", "src/app.py"], rule, tmp_path) + assert matched == ["src/app.py"] + + def test_no_matches(self, tmp_path: Path) -> None: + rule = _make_rule(include=["**/*.py"], source_dir=tmp_path) + matched = match_rule(["app.ts", "lib.js"], rule, tmp_path) + assert matched == [] + + +class TestMatchFilesToRules: + """Tests for match_files_to_rules.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.3.1, REQ-004.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_individual_strategy_creates_one_task_per_file(self, tmp_path: Path) -> None: + rule = _make_rule(strategy="individual", source_dir=tmp_path) + tasks = match_files_to_rules( + ["app.py", "lib.py", "main.ts"], + [rule], + tmp_path, + ) + assert len(tasks) == 2 # app.py and lib.py + assert tasks[0].files_to_review == ["app.py"] + assert tasks[1].files_to_review == ["lib.py"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.4.1, REQ-004.4.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_matches_together_strategy_creates_single_task(self, tmp_path: Path) -> None: + rule = _make_rule(strategy="matches_together", source_dir=tmp_path) + tasks = match_files_to_rules( + ["app.py", "lib.py", "main.ts"], + [rule], + tmp_path, + ) + assert len(tasks) == 1 + assert set(tasks[0].files_to_review) == {"app.py", "lib.py"} + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.5.1, REQ-004.5.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_all_changed_files_strategy_includes_all(self, tmp_path: Path) -> None: + rule = _make_rule(strategy="all_changed_files", source_dir=tmp_path) + tasks = match_files_to_rules( + ["app.py", "lib.py", "main.ts"], + [rule], + tmp_path, + ) + assert len(tasks) == 1 + assert set(tasks[0].files_to_review) == {"app.py", "lib.py", "main.ts"} + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.5.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_all_changed_files_not_triggered_without_match(self, tmp_path: Path) -> None: + rule = _make_rule( + include=["**/*.py"], + strategy="all_changed_files", + source_dir=tmp_path, + ) + tasks = match_files_to_rules( + ["main.ts", "lib.js"], + [rule], + tmp_path, + ) + assert tasks == [] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.7.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_no_match_produces_no_tasks(self, tmp_path: Path) -> None: + rule = _make_rule(include=["**/*.py"], source_dir=tmp_path) + tasks = match_files_to_rules( + ["app.ts"], + [rule], + tmp_path, + ) + assert tasks == [] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.6.1, REQ-004.6.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_all_changed_filenames_context(self, tmp_path: Path) -> None: + rule = _make_rule( + strategy="individual", + all_changed_filenames=True, + source_dir=tmp_path, + ) + changed = ["app.py", "main.ts"] + tasks = match_files_to_rules(changed, [rule], tmp_path) + assert len(tasks) == 1 # Only app.py matches + assert tasks[0].all_changed_filenames == ["app.py", "main.ts"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.8.2, REQ-004.8.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_agent_resolution(self, tmp_path: Path) -> None: + rule = _make_rule( + agent={"claude": "security-expert", "gemini": "sec-bot"}, + source_dir=tmp_path, + ) + tasks = match_files_to_rules(["app.py"], [rule], tmp_path, platform="claude") + assert tasks[0].agent_name == "security-expert" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.8.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_agent_resolution_missing_platform(self, tmp_path: Path) -> None: + rule = _make_rule( + agent={"gemini": "sec-bot"}, + source_dir=tmp_path, + ) + tasks = match_files_to_rules(["app.py"], [rule], tmp_path, platform="claude") + assert tasks[0].agent_name is None + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.2.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_source_location_set_on_tasks(self, tmp_path: Path) -> None: + rule = _make_rule( + source_dir=tmp_path, + source_file=tmp_path / ".deepreview", + source_line=3, + ) + tasks = match_files_to_rules(["app.py"], [rule], tmp_path) + assert tasks[0].source_location == ".deepreview:3" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.9.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_multiple_rules_processed_independently(self, tmp_path: Path) -> None: + rule_a = _make_rule(name="rule_a", include=["**/*.py"], source_dir=tmp_path) + rule_b = _make_rule(name="rule_b", include=["**/*.py"], source_dir=tmp_path) + tasks = match_files_to_rules(["app.py"], [rule_a, rule_b], tmp_path) + assert len(tasks) == 2 + assert tasks[0].rule_name == "rule_a" + assert tasks[1].rule_name == "rule_b" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-004.4.3, REQ-004.4.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_unchanged_matching_files(self, tmp_path: Path) -> None: + # Create files on disk for the glob scan + (tmp_path / "app.py").write_text("# app") + (tmp_path / "lib.py").write_text("# lib") + (tmp_path / "unchanged.py").write_text("# unchanged") + + rule = _make_rule( + strategy="matches_together", + unchanged_matching_files=True, + source_dir=tmp_path, + ) + tasks = match_files_to_rules( + ["app.py", "lib.py"], + [rule], + tmp_path, + ) + assert len(tasks) == 1 + assert "unchanged.py" in tasks[0].additional_files + + +class TestGetChangedFiles: + """Tests for get_changed_files.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.1.7). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_raises_on_non_git_repo(self, tmp_path: Path) -> None: + with pytest.raises(GitDiffError): + get_changed_files(tmp_path) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.1.2, REQ-003.1.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + @patch("deepwork.review.matcher._detect_base_ref", return_value="main") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + def test_returns_sorted_deduplicated_files( + self, mock_merge_base: Any, mock_detect: Any, mock_run: Any, tmp_path: Path + ) -> None: + mock_run.return_value.stdout = "b.py\na.py\n" + mock_run.return_value.returncode = 0 + files = get_changed_files(tmp_path) + assert files == ["a.py", "b.py"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.2.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher._git_diff_name_only") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + def test_uses_explicit_base_ref( + self, mock_merge_base: Any, mock_diff: Any, tmp_path: Path + ) -> None: + mock_diff.return_value = ["app.py"] + get_changed_files(tmp_path, base_ref="develop") + mock_merge_base.assert_called_once_with(tmp_path, "develop") + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.1.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher._git_diff_name_only") + @patch("deepwork.review.matcher._detect_base_ref", return_value="main") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + def test_returns_list_of_relative_paths( + self, mock_merge_base: Any, mock_detect: Any, mock_diff: Any, tmp_path: Path + ) -> None: + mock_diff.return_value = ["src/app.py", "tests/test_app.py"] + result = get_changed_files(tmp_path) + assert isinstance(result, list) + assert all(isinstance(p, str) for p in result) + # Paths should be relative (no leading /) + assert all(not p.startswith("/") for p in result) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.1.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + @patch("deepwork.review.matcher._detect_base_ref", return_value="main") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + def test_uses_diff_filter_acmr( + self, mock_merge_base: Any, mock_detect: Any, mock_run: Any, tmp_path: Path + ) -> None: + mock_run.return_value.stdout = "" + mock_run.return_value.returncode = 0 + get_changed_files(tmp_path) + # Every git diff call must include --diff-filter=ACMR + for c in mock_run.call_args_list: + cmd = c[0][0] if c[0] else c[1].get("args", []) + if "diff" in cmd: + assert "--diff-filter=ACMR" in cmd, ( + f"Expected --diff-filter=ACMR in git diff command: {cmd}" + ) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.1.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher._detect_base_ref", return_value="main") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + @patch("deepwork.review.matcher._git_diff_name_only") + def test_combines_unstaged_and_staged_changes( + self, mock_diff: Any, mock_merge_base: Any, mock_detect: Any, tmp_path: Path + ) -> None: + # First call is for diff against merge-base (unstaged/committed), + # second call is for staged changes + mock_diff.side_effect = [["src/app.py"], ["src/new.py"]] + result = get_changed_files(tmp_path) + assert "src/app.py" in result + assert "src/new.py" in result + # Verify two calls were made: one with merge_base ref, one with None+staged + assert mock_diff.call_count == 2 + first_call = mock_diff.call_args_list[0] + second_call = mock_diff.call_args_list[1] + # First call: diff against merge-base + assert first_call == call(tmp_path, "abc123") + # Second call: staged changes (ref=None, staged=True) + assert second_call == call(tmp_path, None, staged=True) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.1.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher._git_diff_name_only") + @patch("deepwork.review.matcher._detect_base_ref", return_value="main") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + def test_paths_relative_to_repo_root( + self, mock_merge_base: Any, mock_detect: Any, mock_diff: Any, tmp_path: Path + ) -> None: + mock_diff.return_value = ["src/lib/utils.py", "README.md"] + result = get_changed_files(tmp_path) + # All paths must be relative (no absolute paths) + for path in result: + assert not Path(path).is_absolute(), f"Path should be relative: {path}" + assert not path.startswith("/"), f"Path should not start with /: {path}" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.2.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_base_ref_defaults_to_none(self) -> None: + sig = inspect.signature(get_changed_files) + base_ref_param = sig.parameters["base_ref"] + assert base_ref_param.default is None + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.2.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher._git_diff_name_only") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + @patch("deepwork.review.matcher._detect_base_ref", return_value="main") + def test_auto_detects_merge_base_when_base_ref_none( + self, mock_detect: Any, mock_merge_base: Any, mock_diff: Any, tmp_path: Path + ) -> None: + mock_diff.return_value = [] + get_changed_files(tmp_path, base_ref=None) + mock_detect.assert_called_once_with(tmp_path) + mock_merge_base.assert_called_once_with(tmp_path, "main") + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.2.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + def test_detect_base_ref_prefers_main_over_master(self, mock_run: Any, tmp_path: Path) -> None: + from deepwork.review.matcher import _detect_base_ref + + # Simulate: 'main' exists (rev-parse succeeds) + mock_run.return_value.returncode = 0 + result = _detect_base_ref(tmp_path) + assert result == "main" + # Verify it checked 'main' first + first_call_cmd = mock_run.call_args_list[0][0][0] + assert "main" in first_call_cmd + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.2.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + def test_detect_base_ref_falls_back_to_master(self, mock_run: Any, tmp_path: Path) -> None: + from deepwork.review.matcher import _detect_base_ref + + def side_effect(cmd: Any, **kwargs: Any) -> Any: + if "main" in cmd: + raise subprocess.CalledProcessError(128, cmd) + result = MagicMock() + result.returncode = 0 + return result + + mock_run.side_effect = side_effect + result = _detect_base_ref(tmp_path) + assert result == "master" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.2.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + def test_detect_base_ref_falls_back_to_head(self, mock_run: Any, tmp_path: Path) -> None: + from deepwork.review.matcher import _detect_base_ref + + # Neither main nor master exists + mock_run.side_effect = subprocess.CalledProcessError(128, "git") + result = _detect_base_ref(tmp_path) + assert result == "HEAD" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.2.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + def test_uses_git_merge_base(self, mock_run: Any, tmp_path: Path) -> None: + from deepwork.review.matcher import _get_merge_base + + mock_run.return_value.stdout = "deadbeef\n" + mock_run.return_value.returncode = 0 + result = _get_merge_base(tmp_path, "main") + cmd = mock_run.call_args[0][0] + assert cmd[0:2] == ["git", "merge-base"] + assert "HEAD" in cmd + assert "main" in cmd + assert result == "deadbeef" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.3.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + @patch("deepwork.review.matcher._detect_base_ref", return_value="main") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + def test_git_commands_use_cwd_project_root( + self, mock_merge_base: Any, mock_detect: Any, mock_run: Any, tmp_path: Path + ) -> None: + mock_run.return_value.stdout = "app.py\n" + mock_run.return_value.returncode = 0 + get_changed_files(tmp_path) + for c in mock_run.call_args_list: + kwargs = c[1] if c[1] else {} + assert kwargs.get("cwd") == tmp_path, ( + f"Expected cwd={tmp_path}, got cwd={kwargs.get('cwd')} for command {c}" + ) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher._git_diff_name_only") + @patch("deepwork.review.matcher._detect_base_ref", return_value="main") + @patch("deepwork.review.matcher._get_merge_base", return_value="abc123") + def test_does_not_change_process_working_directory( + self, mock_merge_base: Any, mock_detect: Any, mock_diff: Any, tmp_path: Path + ) -> None: + mock_diff.return_value = ["app.py"] + cwd_before = os.getcwd() + get_changed_files(tmp_path) + cwd_after = os.getcwd() + assert cwd_before == cwd_after + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.4.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + def test_invalid_base_ref_raises_error(self, mock_run: Any, tmp_path: Path) -> None: + from deepwork.review.matcher import _get_merge_base + + mock_run.side_effect = subprocess.CalledProcessError( + 128, "git", stderr="fatal: Not a valid object name nonexistent_branch" + ) + with pytest.raises(GitDiffError, match="nonexistent_branch"): + _get_merge_base(tmp_path, "nonexistent_branch") + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-003.4.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.review.matcher.subprocess.run") + def test_stderr_included_in_error_message(self, mock_run: Any, tmp_path: Path) -> None: + from deepwork.review.matcher import _get_merge_base + + stderr_msg = "fatal: bad revision 'bad_ref'" + mock_run.side_effect = subprocess.CalledProcessError(128, "git", stderr=stderr_msg) + with pytest.raises(GitDiffError, match=stderr_msg): + _get_merge_base(tmp_path, "bad_ref") diff --git a/tests/unit/review/test_mcp.py b/tests/unit/review/test_mcp.py new file mode 100644 index 00000000..fa36246b --- /dev/null +++ b/tests/unit/review/test_mcp.py @@ -0,0 +1,284 @@ +"""Tests for the review MCP adapter (deepwork.review.mcp).""" + +from pathlib import Path +from typing import Any +from unittest.mock import patch + +import pytest + +from deepwork.review.config import ReviewRule, ReviewTask +from deepwork.review.mcp import ReviewToolError, get_configured_reviews, run_review + + +def _make_rule(tmp_path: Path) -> ReviewRule: + """Create a ReviewRule for testing.""" + return ReviewRule( + name="test_rule", + description="Test rule description.", + include_patterns=["**/*.py"], + exclude_patterns=[], + strategy="individual", + instructions="Review it.", + agent=None, + all_changed_filenames=False, + unchanged_matching_files=False, + source_dir=tmp_path, + source_file=tmp_path / ".deepreview", + source_line=1, + ) + + +class TestRunReview: + """Tests for the run_review adapter function.""" + + @patch("deepwork.review.mcp.load_all_rules") + def test_no_deepreview_files_returns_message(self, mock_load: Any, tmp_path: Path) -> None: + mock_load.return_value = ([], []) + result = run_review(tmp_path, "claude") + assert "No .deepreview configuration files found" in result + + @patch("deepwork.review.mcp.get_changed_files") + @patch("deepwork.review.mcp.load_all_rules") + def test_no_changed_files_git_diff( + self, mock_load: Any, mock_diff: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_diff.return_value = [] + result = run_review(tmp_path, "claude") + assert "No changed files detected" in result + + @patch("deepwork.review.mcp.load_all_rules") + def test_no_changed_files_explicit_empty_list(self, mock_load: Any, tmp_path: Path) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + result = run_review(tmp_path, "claude", files=[]) + assert "No changed files detected" in result + + @patch("deepwork.review.mcp.match_files_to_rules") + @patch("deepwork.review.mcp.get_changed_files") + @patch("deepwork.review.mcp.load_all_rules") + def test_no_rules_match( + self, mock_load: Any, mock_diff: Any, mock_match: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_diff.return_value = ["app.ts"] + mock_match.return_value = [] + result = run_review(tmp_path, "claude") + assert "No review rules matched" in result + + @patch("deepwork.review.mcp.get_changed_files") + @patch("deepwork.review.mcp.load_all_rules") + def test_git_diff_error_raises_review_tool_error( + self, mock_load: Any, mock_diff: Any, tmp_path: Path + ) -> None: + from deepwork.review.matcher import GitDiffError + + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_diff.side_effect = GitDiffError("not a git repo") + with pytest.raises(ReviewToolError, match="Git error"): + run_review(tmp_path, "claude") + + @patch("deepwork.review.mcp.match_files_to_rules") + @patch("deepwork.review.mcp.get_changed_files") + @patch("deepwork.review.mcp.load_all_rules") + def test_explicit_files_bypass_git_diff( + self, mock_load: Any, mock_diff: Any, mock_match: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_match.return_value = [] + run_review(tmp_path, "claude", files=["src/a.py", "src/b.py"]) + mock_diff.assert_not_called() + mock_match.assert_called_once() + called_files = mock_match.call_args[0][0] + assert called_files == ["src/a.py", "src/b.py"] + + @patch("deepwork.review.mcp.write_instruction_files") + @patch("deepwork.review.mcp.match_files_to_rules") + @patch("deepwork.review.mcp.get_changed_files") + @patch("deepwork.review.mcp.load_all_rules") + def test_full_pipeline_returns_formatted_output( + self, mock_load: Any, mock_diff: Any, mock_match: Any, mock_write: Any, tmp_path: Path + ) -> None: + rule = _make_rule(tmp_path) + task = ReviewTask( + rule_name="test_rule", + files_to_review=["app.py"], + instructions="Review it.", + agent_name=None, + ) + mock_load.return_value = ([rule], []) + mock_diff.return_value = ["app.py"] + mock_match.return_value = [task] + mock_write.return_value = [(task, tmp_path / "instr.md")] + + result = run_review(tmp_path, "claude") + assert "Invoke the following" in result + mock_write.assert_called_once() + + def test_unsupported_platform_raises_review_tool_error(self, tmp_path: Path) -> None: + with pytest.raises(ReviewToolError, match="Unsupported platform"): + run_review(tmp_path, "unsupported_platform") + + @patch("deepwork.review.mcp.match_files_to_rules") + @patch("deepwork.review.mcp.load_all_rules") + def test_files_are_deduplicated_and_sorted( + self, mock_load: Any, mock_match: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_match.return_value = [] + run_review(tmp_path, "claude", files=["z.py", "a.py", "z.py"]) + called_files = mock_match.call_args[0][0] + assert called_files == ["a.py", "z.py"] + + @patch("deepwork.review.mcp.write_instruction_files") + @patch("deepwork.review.mcp.match_files_to_rules") + @patch("deepwork.review.mcp.get_changed_files") + @patch("deepwork.review.mcp.load_all_rules") + def test_write_error_raises_review_tool_error( + self, mock_load: Any, mock_diff: Any, mock_match: Any, mock_write: Any, tmp_path: Path + ) -> None: + rule = _make_rule(tmp_path) + task = ReviewTask( + rule_name="test_rule", + files_to_review=["app.py"], + instructions="Review it.", + agent_name=None, + ) + mock_load.return_value = ([rule], []) + mock_diff.return_value = ["app.py"] + mock_match.return_value = [task] + mock_write.side_effect = OSError("Permission denied") + + with pytest.raises(ReviewToolError, match="Error writing instruction files"): + run_review(tmp_path, "claude") + + +class TestReviewToolRegistration: + """Test that the review tool is registered on the MCP server.""" + + def test_review_tool_is_registered(self, tmp_path: Path) -> None: + """Review tool is registered on the MCP server with or without explicit platform.""" + from deepwork.jobs.mcp.server import create_server + + # With explicit platform + server = create_server( + project_root=tmp_path, + enable_quality_gate=False, + platform="claude", + ) + assert "get_review_instructions" in server._tool_manager._tools + + # Without platform (defaults to claude) + server_default = create_server( + project_root=tmp_path, + enable_quality_gate=False, + ) + assert "get_review_instructions" in server_default._tool_manager._tools + + def test_get_configured_reviews_tool_is_registered(self, tmp_path: Path) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-008.1.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """get_configured_reviews is registered on the MCP server.""" + from deepwork.jobs.mcp.server import create_server + + server = create_server( + project_root=tmp_path, + enable_quality_gate=False, + ) + assert "get_configured_reviews" in server._tool_manager._tools + + +class TestGetConfiguredReviews: + """Tests for the get_configured_reviews adapter function — validates REQ-008.""" + + @patch("deepwork.review.mcp.load_all_rules") + def test_returns_all_rules(self, mock_load: Any, tmp_path: Path) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-008.1.3, REQ-008.2.1, REQ-008.2.2, REQ-008.2.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + rule1 = _make_rule(tmp_path) + rule2 = ReviewRule( + name="lint_rule", + description="Lint rule description.", + include_patterns=["**/*.ts"], + exclude_patterns=[], + strategy="matches_together", + instructions="Lint it.", + agent=None, + all_changed_filenames=False, + unchanged_matching_files=False, + source_dir=tmp_path, + source_file=tmp_path / ".deepreview", + source_line=10, + ) + mock_load.return_value = ([rule1, rule2], []) + + result = get_configured_reviews(tmp_path) + assert len(result) == 2 + assert result[0]["name"] == "test_rule" + assert result[0]["description"] == "Test rule description." + assert result[0]["defining_file"] == ".deepreview:1" + assert result[1]["name"] == "lint_rule" + assert result[1]["defining_file"] == ".deepreview:10" + + @patch("deepwork.review.mcp.load_all_rules") + def test_returns_empty_when_no_rules(self, mock_load: Any, tmp_path: Path) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-008.1.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + mock_load.return_value = ([], []) + result = get_configured_reviews(tmp_path) + assert result == [] + + @patch("deepwork.review.mcp.load_all_rules") + def test_filters_by_matching_files(self, mock_load: Any, tmp_path: Path) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-008.3.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + py_rule = _make_rule(tmp_path) # includes **/*.py + ts_rule = ReviewRule( + name="ts_rule", + description="TypeScript rule.", + include_patterns=["**/*.ts"], + exclude_patterns=[], + strategy="individual", + instructions="Review TS.", + agent=None, + all_changed_filenames=False, + unchanged_matching_files=False, + source_dir=tmp_path, + source_file=tmp_path / ".deepreview", + source_line=5, + ) + mock_load.return_value = ([py_rule, ts_rule], []) + + result = get_configured_reviews(tmp_path, only_rules_matching_files=["src/app.py"]) + assert len(result) == 1 + assert result[0]["name"] == "test_rule" + + @patch("deepwork.review.mcp.load_all_rules") + def test_no_filter_returns_all(self, mock_load: Any, tmp_path: Path) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-008.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + rule = _make_rule(tmp_path) + mock_load.return_value = ([rule], []) + + result = get_configured_reviews(tmp_path, only_rules_matching_files=None) + assert len(result) == 1 + assert result[0]["name"] == "test_rule" + + @patch("deepwork.review.mcp.load_all_rules") + def test_discovery_errors_still_return_valid_rules( + self, mock_load: Any, tmp_path: Path + ) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-008.4.1, REQ-008.4.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + from deepwork.review.discovery import DiscoveryError + + rule = _make_rule(tmp_path) + errors = [DiscoveryError(file_path=Path("/bad/.deepreview"), error="invalid YAML")] + mock_load.return_value = ([rule], errors) + + result = get_configured_reviews(tmp_path) + # Valid rules are returned, plus parse errors are surfaced + valid_rules = [r for r in result if not r["name"].startswith("PARSE_ERROR:")] + assert len(valid_rules) == 1 + assert valid_rules[0]["name"] == "test_rule" + error_entries = [r for r in result if r["name"].startswith("PARSE_ERROR:")] + assert len(error_entries) == 1 diff --git a/tests/unit/review/test_plugin_skills.py b/tests/unit/review/test_plugin_skills.py new file mode 100644 index 00000000..6b0f104d --- /dev/null +++ b/tests/unit/review/test_plugin_skills.py @@ -0,0 +1,246 @@ +"""Tests for DeepWork Reviews plugin skills and documentation — validates REQ-007.""" + +from pathlib import Path +from typing import Any + +import yaml + +# Project root — navigate up from this test file +PROJECT_ROOT = Path(__file__).resolve().parent.parent.parent.parent +PLUGIN_DIR = PROJECT_ROOT / "plugins" / "claude" +SKILLS_DIR = PLUGIN_DIR / "skills" + + +def _parse_frontmatter(skill_path: Path) -> dict[str, Any]: + """Parse YAML frontmatter from a SKILL.md file.""" + text = skill_path.read_text(encoding="utf-8") + assert text.startswith("---"), f"{skill_path} must start with YAML frontmatter" + end = text.index("---", 3) + result: dict[str, Any] = yaml.safe_load(text[3:end]) + return result + + +class TestReviewSkill: + """Tests for the review skill (REQ-007.1).""" + + skill_path = SKILLS_DIR / "review" / "SKILL.md" + + def test_skill_file_exists(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.1: review skill exists at expected path.""" + assert self.skill_path.exists() + + def test_frontmatter_name(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.2: name field is 'review'.""" + fm = _parse_frontmatter(self.skill_path) + assert fm["name"] == "review" + + def test_frontmatter_description(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.3: description field is present.""" + fm = _parse_frontmatter(self.skill_path) + assert "description" in fm + assert len(fm["description"]) > 0 + + def test_instructs_to_call_review_mcp_tool(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.4: skill tells agent to call the review MCP tool.""" + content = self.skill_path.read_text(encoding="utf-8") + assert "mcp__deepwork__get_review_instructions" in content + + def test_instructs_parallel_tasks(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.5: skill tells agent to launch tasks in parallel.""" + content = self.skill_path.read_text(encoding="utf-8") + assert "parallel" in content.lower() + + def test_instructs_auto_apply_obvious_fixes(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.6: skill tells agent to auto-apply no-downside changes.""" + content = self.skill_path.read_text(encoding="utf-8") + # Must mention making changes without asking for obvious fixes + assert "without asking" in content.lower() or "immediately" in content.lower() + + def test_instructs_ask_user_for_tradeoffs(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.7). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.7: skill tells agent to use AskUserQuestion for trade-offs.""" + content = self.skill_path.read_text(encoding="utf-8") + assert "AskUserQuestion" in content + + def test_instructs_iterate(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.8). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.8: skill tells agent to re-run after changes.""" + content = self.skill_path.read_text(encoding="utf-8") + # Must mention running again / repeating + assert "again" in content.lower() or "repeat" in content.lower() + + def test_routes_config_to_configure_reviews(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.1.9). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.1.9: skill routes configuration requests to configure_reviews.""" + content = self.skill_path.read_text(encoding="utf-8") + assert "configure_reviews" in content + + +class TestConfigureReviewsSkill: + """Tests for the configure_reviews skill (REQ-007.2).""" + + skill_path = SKILLS_DIR / "configure_reviews" / "SKILL.md" + + def test_skill_file_exists(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.2.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.2.1: configure_reviews skill exists at expected path.""" + assert self.skill_path.exists() + + def test_frontmatter_name(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.2.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.2.2: name field is 'configure_reviews'.""" + fm = _parse_frontmatter(self.skill_path) + assert fm["name"] == "configure_reviews" + + def test_frontmatter_description(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.2.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.2.3: description field is present.""" + fm = _parse_frontmatter(self.skill_path) + assert "description" in fm + assert len(fm["description"]) > 0 + + def test_instructs_to_read_readme(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.2.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.2.4: skill tells agent to read README_REVIEWS.md.""" + content = self.skill_path.read_text(encoding="utf-8") + assert "README_REVIEWS.md" in content + + def test_instructs_to_reuse_existing(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.2.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.2.5: skill tells agent to look at existing .deepreview files.""" + content = self.skill_path.read_text(encoding="utf-8") + assert ".deepreview" in content + assert "reuse" in content.lower() or "existing" in content.lower() + + def test_instructs_to_test_changes(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.2.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.2.6: skill tells agent to test by calling the review MCP tool.""" + content = self.skill_path.read_text(encoding="utf-8") + assert "mcp__deepwork__get_review_instructions" in content + + +class TestReferenceDocumentation: + """Tests for README_REVIEWS.md in the plugin (REQ-007.3).""" + + symlink_path = PLUGIN_DIR / "README_REVIEWS.md" + root_readme = PROJECT_ROOT / "README_REVIEWS.md" + + def test_symlink_exists(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.1: README_REVIEWS.md exists in plugin directory.""" + assert self.symlink_path.exists() + + def test_is_symlink(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.2: plugin copy is a symlink.""" + assert self.symlink_path.is_symlink() + + def test_symlink_target(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.2: symlink points to ../../README_REVIEWS.md.""" + target = self.symlink_path.readlink() + assert str(target) == "../../README_REVIEWS.md" + + def test_symlink_resolves(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.3: symlink target exists and is readable.""" + assert self.symlink_path.resolve().exists() + content = self.symlink_path.read_text(encoding="utf-8") + assert len(content) > 0 + + def test_root_readme_exists(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.3: root README_REVIEWS.md exists.""" + assert self.root_readme.exists() + + def test_documents_deepreview_format(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.4: documents .deepreview config format.""" + content = self.root_readme.read_text(encoding="utf-8") + assert ".deepreview" in content + assert "strategy" in content + assert "individual" in content + assert "matches_together" in content + assert "all_changed_files" in content + + def test_documents_instruction_variants(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.4: documents both inline and file reference instructions.""" + content = self.root_readme.read_text(encoding="utf-8") + assert "instructions:" in content + assert "file:" in content + + def test_documents_agent_personas(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.4: documents agent personas.""" + content = self.root_readme.read_text(encoding="utf-8") + assert "agent:" in content + assert "claude:" in content + + def test_recommends_deepwork_review_dir(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.3.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.3.5: recommends .deepwork/review/ for instruction files.""" + content = self.root_readme.read_text(encoding="utf-8") + assert ".deepwork/review/" in content + + +class TestSkillDirectoryConventions: + """Tests for skill directory structure (REQ-007.4).""" + + def test_each_skill_has_own_directory(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.4.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.4.1: each skill is in its own directory.""" + skill_dirs = [d for d in SKILLS_DIR.iterdir() if d.is_dir()] + assert len(skill_dirs) >= 3 # deepwork, review, configure_reviews + + def test_each_skill_has_skill_md(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.4.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.4.2: each skill directory contains SKILL.md.""" + skill_dirs = [d for d in SKILLS_DIR.iterdir() if d.is_dir()] + for skill_dir in skill_dirs: + assert (skill_dir / "SKILL.md").exists(), f"{skill_dir.name} is missing SKILL.md" + + def test_name_matches_directory(self) -> None: + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-007.4.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + """REQ-007.4.3: frontmatter name matches directory name.""" + skill_dirs = [d for d in SKILLS_DIR.iterdir() if d.is_dir()] + for skill_dir in skill_dirs: + skill_file = skill_dir / "SKILL.md" + if skill_file.exists(): + fm = _parse_frontmatter(skill_file) + assert fm["name"] == skill_dir.name, ( + f"Skill {skill_dir.name} has name={fm['name']} in frontmatter" + ) diff --git a/tests/unit/review/test_review_cli.py b/tests/unit/review/test_review_cli.py new file mode 100644 index 00000000..6eab8f74 --- /dev/null +++ b/tests/unit/review/test_review_cli.py @@ -0,0 +1,302 @@ +"""Tests for the deepwork review CLI command (deepwork.cli.review) — validates REQ-006.""" + +from pathlib import Path +from typing import Any +from unittest.mock import patch + +from click.testing import CliRunner + +from deepwork.cli.review import review +from deepwork.review.config import ReviewRule, ReviewTask + + +def _make_rule(tmp_path: Path) -> ReviewRule: + """Create a ReviewRule for testing.""" + return ReviewRule( + name="test_rule", + description="Test rule description.", + include_patterns=["**/*.py"], + exclude_patterns=[], + strategy="individual", + instructions="Review it.", + agent=None, + all_changed_filenames=False, + unchanged_matching_files=False, + source_dir=tmp_path, + source_file=tmp_path / ".deepreview", + source_line=1, + ) + + +class TestReviewCommand: + """Tests for the review CLI command.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.4.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.load_all_rules") + def test_no_config_files_found(self, mock_load: Any, tmp_path: Path) -> None: + mock_load.return_value = ([], []) + runner = CliRunner() + result = runner.invoke( + review, + ["--instructions-for", "claude", "--path", str(tmp_path)], + ) + assert result.exit_code == 0 + assert "No .deepreview configuration files found" in result.output + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.4.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_no_changed_files(self, mock_load: Any, mock_diff: Any, tmp_path: Path) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_diff.return_value = [] + runner = CliRunner() + result = runner.invoke( + review, + ["--instructions-for", "claude", "--path", str(tmp_path)], + ) + assert result.exit_code == 0 + assert "No changed files detected" in result.output + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.4.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.match_files_to_rules") + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_no_rules_match( + self, mock_load: Any, mock_diff: Any, mock_match: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_diff.return_value = ["app.ts"] + mock_match.return_value = [] + runner = CliRunner() + result = runner.invoke( + review, + ["--instructions-for", "claude", "--path", str(tmp_path)], + ) + assert result.exit_code == 0 + assert "No review rules matched" in result.output + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.5.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_git_error_exits_with_code_1( + self, mock_load: Any, mock_diff: Any, tmp_path: Path + ) -> None: + from deepwork.review.matcher import GitDiffError + + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_diff.side_effect = GitDiffError("not a git repo") + runner = CliRunner() + result = runner.invoke( + review, + ["--instructions-for", "claude", "--path", str(tmp_path)], + ) + assert result.exit_code == 1 + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.5.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.load_all_rules") + def test_discovery_errors_reported_but_continues(self, mock_load: Any, tmp_path: Path) -> None: + from deepwork.review.discovery import DiscoveryError + + mock_load.return_value = ( + [], + [DiscoveryError(file_path=Path("/bad/.deepreview"), error="parse error")], + ) + runner = CliRunner() + result = runner.invoke( + review, + ["--instructions-for", "claude", "--path", str(tmp_path)], + ) + # With no valid rules loaded, should report no config found + assert result.exit_code == 0 + assert "No .deepreview configuration files found" in result.output + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.2.1, REQ-006.3.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.format_for_claude") + @patch("deepwork.cli.review.write_instruction_files") + @patch("deepwork.cli.review.match_files_to_rules") + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_full_pipeline_produces_output( + self, + mock_load: Any, + mock_diff: Any, + mock_match: Any, + mock_write: Any, + mock_format: Any, + tmp_path: Path, + ) -> None: + rule = _make_rule(tmp_path) + task = ReviewTask( + rule_name="test_rule", + files_to_review=["app.py"], + instructions="Review it.", + agent_name=None, + ) + mock_load.return_value = ([rule], []) + mock_diff.return_value = ["app.py"] + mock_match.return_value = [task] + mock_write.return_value = [(task, tmp_path / "instr.md")] + mock_format.return_value = "Invoke the following..." + + runner = CliRunner() + result = runner.invoke( + review, + ["--instructions-for", "claude", "--path", str(tmp_path)], + ) + assert result.exit_code == 0 + assert "Invoke the following" in result.output + mock_format.assert_called_once() + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.1.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_instructions_for_is_required(self) -> None: + runner = CliRunner() + result = runner.invoke(review, []) + assert result.exit_code != 0 + assert "Missing option" in result.output or "required" in result.output.lower() + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.6.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.match_files_to_rules") + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_files_option_bypasses_git_diff( + self, mock_load: Any, mock_diff: Any, mock_match: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_match.return_value = [] + runner = CliRunner() + result = runner.invoke( + review, + [ + "--instructions-for", + "claude", + "--path", + str(tmp_path), + "--files", + "src/a.py", + "--files", + "src/b.py", + ], + ) + assert result.exit_code == 0 + # git diff should NOT have been called + mock_diff.assert_not_called() + # match should have been called with the provided files + mock_match.assert_called_once() + called_files = mock_match.call_args[0][0] + assert called_files == ["src/a.py", "src/b.py"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.6.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.match_files_to_rules") + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_stdin_provides_file_list( + self, mock_load: Any, mock_diff: Any, mock_match: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_match.return_value = [] + runner = CliRunner() + result = runner.invoke( + review, + ["--instructions-for", "claude", "--path", str(tmp_path)], + input="src/x.py\nsrc/y.py\n", + ) + assert result.exit_code == 0 + mock_diff.assert_not_called() + mock_match.assert_called_once() + called_files = mock_match.call_args[0][0] + assert called_files == ["src/x.py", "src/y.py"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.6.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.match_files_to_rules") + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_files_option_deduplicates_and_sorts( + self, mock_load: Any, mock_diff: Any, mock_match: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_match.return_value = [] + runner = CliRunner() + result = runner.invoke( + review, + [ + "--instructions-for", + "claude", + "--path", + str(tmp_path), + "--files", + "z.py", + "--files", + "a.py", + "--files", + "z.py", + ], + ) + assert result.exit_code == 0 + called_files = mock_match.call_args[0][0] + assert called_files == ["a.py", "z.py"] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.5.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.write_instruction_files") + @patch("deepwork.cli.review.match_files_to_rules") + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_instruction_write_error_exits_with_code_1( + self, mock_load: Any, mock_diff: Any, mock_match: Any, mock_write: Any, tmp_path: Path + ) -> None: + rule = _make_rule(tmp_path) + task = ReviewTask( + rule_name="test_rule", + files_to_review=["app.py"], + instructions="Review it.", + agent_name=None, + ) + mock_load.return_value = ([rule], []) + mock_diff.return_value = ["app.py"] + mock_match.return_value = [task] + mock_write.side_effect = OSError("Permission denied") + + runner = CliRunner() + result = runner.invoke( + review, + ["--instructions-for", "claude", "--path", str(tmp_path)], + ) + assert result.exit_code == 1 + assert "Error writing instruction files" in result.output + + # THIS TEST VALIDATES A HARD REQUIREMENT (REQ-006.6.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + @patch("deepwork.cli.review.match_files_to_rules") + @patch("deepwork.cli.review.get_changed_files") + @patch("deepwork.cli.review.load_all_rules") + def test_files_option_takes_priority_over_stdin( + self, mock_load: Any, mock_diff: Any, mock_match: Any, tmp_path: Path + ) -> None: + mock_load.return_value = ([_make_rule(tmp_path)], []) + mock_match.return_value = [] + runner = CliRunner() + result = runner.invoke( + review, + [ + "--instructions-for", + "claude", + "--path", + str(tmp_path), + "--files", + "explicit.py", + ], + input="from_stdin.py\n", + ) + assert result.exit_code == 0 + called_files = mock_match.call_args[0][0] + assert called_files == ["explicit.py"]