diff --git a/CHANGELOG.md b/CHANGELOG.md index 77aadd3..177052a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ All notable changes to this repository will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project follows SemVer for bundle versions. +## [0.44.0] - 2026-03-17 + +### Added + +- Add `--scope changed|full` and repeatable repo-relative `--path` filters to + `specfact code review run` for deterministic changed-only, full-repository, + and subtree-limited review selection. + +### Changed + +- Keep changed-only auto-discovery as the default, allow explicit test subtrees + to opt matching tests back into scope, and extend the review-run docs plus + cli-contract scenarios to cover the new targeting controls. + ## [0.43.0] - 2026-03-16 ### Added diff --git a/docs/modules/code-review.md b/docs/modules/code-review.md index 25d1c85..b91715a 100644 --- a/docs/modules/code-review.md +++ b/docs/modules/code-review.md @@ -17,6 +17,10 @@ Options: - `--score-only`: print only the integer `reward_delta` - `--fix`: apply Ruff autofixes and re-run the review before printing results - `--no-tests`: skip the targeted TDD gate +- `--scope changed|full`: choose changed-only or full-repository auto-discovery + when positional files are omitted +- `--path PATH`: repeatable repo-relative subtree filter for auto-discovered + review targets - `--include-tests/--exclude-tests`: include or exclude changed test files when review scope is auto-detected from `git diff` - `--include-noise/--suppress-noise`: include or suppress known low-signal @@ -34,7 +38,27 @@ git ls-files --others --exclude-standard Only existing Python files from tracked or untracked workspace changes are reviewed. Test files under `tests/` are excluded by default for auto-detected review scope unless you pass `--include-tests` or answer yes in -`--interactive` mode. +`--interactive` mode. Explicit `--path tests/...` filters count as intentional +targeting and keep matching tests in scope even with the default test +exclusion. + +Use `--scope full` to review the governed repository file set instead of just +current changes: + +```bash +specfact code review run --scope full +``` + +Use repeatable `--path` filters to limit either scope to one package or a +focused source-plus-test slice: + +```bash +specfact code review run --scope full --path packages/specfact-code-review +specfact code review run --scope changed --path packages/specfact-code-review --path tests/unit/specfact_code_review +``` + +Positional `FILES...` cannot be mixed with `--scope` or `--path`. Choose one +targeting style per invocation. With default noise suppression, the review also hides known low-signal test findings such as: diff --git a/openspec/changes/code-review-10-review-scope-modes/.openspec.yaml b/openspec/changes/code-review-10-review-scope-modes/.openspec.yaml new file mode 100644 index 0000000..bea0667 --- /dev/null +++ b/openspec/changes/code-review-10-review-scope-modes/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-03-17 diff --git a/openspec/changes/code-review-10-review-scope-modes/CHANGE_VALIDATION.md b/openspec/changes/code-review-10-review-scope-modes/CHANGE_VALIDATION.md new file mode 100644 index 0000000..3fc4d42 --- /dev/null +++ b/openspec/changes/code-review-10-review-scope-modes/CHANGE_VALIDATION.md @@ -0,0 +1,54 @@ +## Verification Report: code-review-10-review-scope-modes + +### Summary + +| Dimension | Status | +| --- | --- | +| Completeness | 4/4 apply-ready artifacts present (`proposal`, `design`, `specs`, `tasks`) | +| Correctness | Scope-mode and path-filter requirements align with upstream `code-review-10-review-scope-modes` | +| Coherence | Proposal, design, and tasks consistently keep changed-only as the default auto-scope | + +### Validation Checks + +- Reviewed upstream `specfact-cli` proposal, design, and spec delta for `code-review-10-review-scope-modes`. +- Reviewed local artifacts: `proposal.md`, `design.md`, `tasks.md`, and delta specs under `specs/`. +- Checked current modules-repo review-run capabilities from `code-review-08-review-run-integration` to ensure this is a requirement change on an existing command, not a new command surface. + +### CRITICAL + +None. + +### WARNING + +- The derived change assumes the bundle command can identify a stable governed + repository file set for `--scope full`. If existing runtime logic only + supports changed-file discovery, implementation may need a new helper to keep + full-review selection deterministic across unit and e2e tests. + Recommendation: verify current target-resolution boundaries before coding and + centralize scope resolution rather than duplicating it across command tests + and runtime entry points. + +### SUGGESTION + +- Keep cli-val scenarios narrow and representative. One changed-only example, + one full-review example, one subtree-filter example, and one invalid + invocation are enough to prove the contract without making scenario YAML noisy. + +### Dependency Analysis + +- Modified capabilities: `review-run-command`, `review-cli-contracts` +- Primary implementation touchpoint: `packages/specfact-code-review/src/specfact_code_review/run/` +- Primary regression surface: `tests/unit/`, `tests/e2e/`, and `tests/cli-contracts/` +- Upstream source dependency: `nold-ai/specfact-cli` change `code-review-10-review-scope-modes` + +### Final Assessment + +No critical issues found. The derived change is internally consistent, matches +the upstream intent, and is ready for implementation after strict OpenSpec +validation passes. + +### OpenSpec Validation + +- **Status**: Pass +- **Command**: `openspec validate code-review-10-review-scope-modes --strict` +- **Issues Found/Fixed**: 0 diff --git a/openspec/changes/code-review-10-review-scope-modes/TDD_EVIDENCE.md b/openspec/changes/code-review-10-review-scope-modes/TDD_EVIDENCE.md new file mode 100644 index 0000000..c25637c --- /dev/null +++ b/openspec/changes/code-review-10-review-scope-modes/TDD_EVIDENCE.md @@ -0,0 +1,106 @@ +# TDD Evidence: code-review-10 review scope modes + +## Failing + +### 2026-03-17 `tests/unit/specfact_code_review/run/test_commands.py` + +Command: + +```bash +HATCH_DATA_DIR=/tmp/hatch-data \ +HATCH_CACHE_DIR=/tmp/hatch-cache \ +VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \ +hatch run pytest tests/unit/specfact_code_review/run/test_commands.py -q +``` + +Result: `5 failed, 14 passed` + +Key failures before implementation: + +- `test_run_command_supports_full_scope_and_path_filters` + - exit code was `2` because `--scope` was not supported yet +- `test_run_command_supports_changed_scope_with_repeatable_path_filters` + - exit code was `2` because `--scope`/`--path` were not supported yet +- `test_run_command_rejects_scope_mixed_with_positional_files` + - CLI returned the generic option error instead of the governed mixed-targeting message +- `test_run_command_rejects_path_mixed_with_positional_files` + - CLI returned the generic option error instead of the governed mixed-targeting message +- `test_run_command_fails_when_scope_and_paths_match_no_files` + - CLI returned the generic option error instead of an actionable empty-scope failure + +## Passing + +### 2026-03-17 Focused scope-mode tests + +Commands: + +```bash +HATCH_DATA_DIR=/tmp/hatch-data \ +HATCH_CACHE_DIR=/tmp/hatch-cache \ +VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \ +hatch run pytest tests/unit/specfact_code_review/run/test_commands.py tests/unit/specfact_code_review/run/test_runner.py -q + +HATCH_DATA_DIR=/tmp/hatch-data \ +HATCH_CACHE_DIR=/tmp/hatch-cache \ +VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \ +hatch run pytest tests/e2e/specfact_code_review/test_review_run_e2e.py -q + +HATCH_DATA_DIR=/tmp/hatch-data \ +HATCH_CACHE_DIR=/tmp/hatch-cache \ +VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \ +hatch run validate-cli-contracts +``` + +Results: + +- `tests/unit/specfact_code_review/run/test_commands.py` and `tests/unit/specfact_code_review/run/test_runner.py`: `36 passed` +- `tests/e2e/specfact_code_review/test_review_run_e2e.py`: `2 passed` +- CLI contract validation: `Validated 3 CLI contract scenario files.` + +### 2026-03-17 SpecFact dogfood review + +Command: + +```bash +SPECFACT_ALLOW_UNSIGNED=1 \ +HATCH_DATA_DIR=/tmp/hatch-data \ +HATCH_CACHE_DIR=/tmp/hatch-cache \ +VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \ +hatch run specfact code review run \ + --scope changed \ + --path packages/specfact-code-review \ + --path tests/unit/specfact_code_review \ + --json \ + --out /tmp/code-review-10-report.json +``` + +Result: + +- verdict: `PASS` +- score: `115` +- findings: `0` + +### 2026-03-17 Worktree quality gates + +Commands completed successfully: + +```bash +HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run format +HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run type-check +HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run lint +HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run check-bundle-imports +HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run smart-test +HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run contract-test +``` + +Results: + +- `type-check`: `0 errors, 0 warnings, 0 notes` +- `lint`: `10.00/10` +- `smart-test`: `383 passed` +- `contract-test`: `384 passed` + +Pending: + +- `verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump` + - blocked until the final `packages/specfact-code-review/module-package.yaml` changes in this worktree are re-signed diff --git a/openspec/changes/code-review-10-review-scope-modes/design.md b/openspec/changes/code-review-10-review-scope-modes/design.md new file mode 100644 index 0000000..3f8ba3d --- /dev/null +++ b/openspec/changes/code-review-10-review-scope-modes/design.md @@ -0,0 +1,71 @@ +# Design: code-review scope modes for modules repo + +## Context + +`code-review-08-review-run-integration` gave the modules repo a working +`specfact code review run` command, but its implicit changed-file discovery is +too narrow for stepwise review debt cleanup. The upstream `specfact-cli` +contract now defines two auto-discovery modes, `changed` and `full`, plus +repo-relative path filters that can recursively narrow either mode. + +This repository owns the runtime command implementation, test fixtures, cli-val +scenario YAML, and bundle docs. The design therefore focuses on deterministic +file selection inside the bundle rather than on higher-level GitHub or +automation wiring. + +## Goals / Non-Goals + +**Goals:** +- Keep default no-argument behavior aligned with existing changed-file review +- Add `--scope changed|full` and repeatable `--path` filters at the bundle + command boundary +- Make subtree filtering recursive so users can review one package or test area + at a time +- Cover the new behavior with unit, e2e, and cli-val fixtures + +**Non-Goals:** +- Changing scoring, verdict mapping, or ledger integration +- Adding globbing or exclusion syntax beyond repo-relative path prefixes +- Reworking the internal runner ordering established in `code-review-08` + +## Decisions + +### Decision: Resolve scope before filtering by path + +The bundle should first compute the candidate file set from the selected scope, +then filter that set by any `--path` prefixes. That keeps the behavior stable +between tests and runtime, and makes it easy to explain to users. + +### Decision: Reject positional files mixed with scope controls + +Positional files already provide an explicit review target. Mixing them with +`--scope` or `--path` would create ambiguous precedence rules, so the command +should fail fast and tell the user to choose one targeting style. + +### Decision: Treat `--path` values as repo-relative recursive path segments + +Users asked for folder and subfolder limiting. Prefix-based matching is enough +for that need, but matching must happen on normalized path boundaries rather +than raw strings. That keeps `--path packages/specfact-code-review` scoped to +that package and its descendants instead of accidentally including sibling +paths such as `packages/specfact-code-review-old`, while still avoiding new +pattern syntax in the CLI contract. + +### Decision: Explicit path filters may opt matched tests back in + +The existing changed-only workflow excludes tests by default unless users pass +`--include-tests`. For scope-mode filtering, explicit `--path tests/...` +selection should count as intentional targeting, so matching test files remain +reviewable even when the global include-tests toggle stays at its default. + +## Risks / Trade-offs + +- [Risk] Full-review mode may surface far more findings than users expect. + Mitigation: keep it opt-in and document subtree filtering prominently. +- [Risk] Scope/path selection may vary between unit tests and e2e execution if + repo root detection is inconsistent. + Mitigation: centralize target resolution in one helper and exercise it + through both direct tests and installed-command tests. +- [Risk] cli-val scenarios could lag behind actual command behavior. + Mitigation: update review-run scenario YAML in the same change as the command + implementation and validate them together. diff --git a/openspec/changes/code-review-10-review-scope-modes/proposal.md b/openspec/changes/code-review-10-review-scope-modes/proposal.md new file mode 100644 index 0000000..7277cf2 --- /dev/null +++ b/openspec/changes/code-review-10-review-scope-modes/proposal.md @@ -0,0 +1,57 @@ +# Change: Reapply code-review-10 review scope modes in modules repo + +## Why + +The upstream `specfact-cli` change `code-review-10-review-scope-modes` defines +the governed contract for explicit changed-only vs. full review modes plus +repo-relative path filtering. The modules repository needs a matching active +change because the bundle-owned `specfact code review run` implementation, +runtime fixtures, and cli-val scenarios live here. + +Without this derived change, the upstream contract would exist only on paper: +the installed `specfact-code-review` bundle would still lack the runtime +behavior needed to review a full repo or a limited subtree in a controlled way. + +## What Changes + +- Extend the `specfact-code-review` bundle command to support `--scope changed` + and `--scope full`. +- Add repeatable `--path` filtering for repo-relative files and subfolders in + auto-discovery mode. +- Keep changed-only auto-discovery as the default behavior when users do not + pass positional files or an explicit scope. +- Treat explicit `--path tests/...` filters as an intentional opt-in for those + matched test files without changing the default auto-scope exclusion of tests. +- Add targeted tests, fixtures, and cli-val scenarios for changed-only reviews, + full reviews, filtered subtree reviews, and invalid scope combinations. +- Update bundle docs, changelog, and signed package metadata to describe the + new review-scope controls. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `review-run-command`: add explicit review-scope selection and repo-relative + path filtering to the bundle command +- `review-cli-contracts`: extend review-run scenario coverage for changed-only, + full-review, and subtree-limited invocations + +## Impact + +- Affects `packages/specfact-code-review/src/specfact_code_review/run/` command + parsing and target-file resolution +- Expands runtime validation in `tests/unit/`, `tests/e2e/`, and + `tests/cli-contracts/` +- Requires documentation, changelog, and module metadata updates for the + `specfact-code-review` bundle + +## Source Tracking + + +- **Source Change**: `code-review-10-review-scope-modes` +- **Repository**: nold-ai/specfact-cli +- **Last Synced Status**: re-applied in modules repo diff --git a/openspec/changes/code-review-10-review-scope-modes/specs/review-cli-contracts/spec.md b/openspec/changes/code-review-10-review-scope-modes/specs/review-cli-contracts/spec.md new file mode 100644 index 0000000..dc63780 --- /dev/null +++ b/openspec/changes/code-review-10-review-scope-modes/specs/review-cli-contracts/spec.md @@ -0,0 +1,21 @@ +## MODIFIED Requirements + +### Requirement: cli-val scenarios exist for review command groups +The modules repository SHALL define cli-val-compatible scenario YAML files for +the `specfact code review run`, `ledger`, and `rules` command groups, including +scope-mode and path-filter coverage for the review run command. + +#### Scenario: review-run scenarios cover success, scope selection, and error paths +- **GIVEN** `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` +- **WHEN** it is validated +- **THEN** it includes success coverage, changed-only and full-review scope examples, subtree-filtered examples, and an error or anti-pattern scenario + +#### Scenario: ledger scenarios cover update, status, and reset guardrails +- **GIVEN** `tests/cli-contracts/specfact-code-review-ledger.scenarios.yaml` +- **WHEN** it is validated +- **THEN** it includes `update`, `status`, and reset guardrail coverage + +#### Scenario: rules scenarios cover init, show, and update +- **GIVEN** `tests/cli-contracts/specfact-code-review-rules.scenarios.yaml` +- **WHEN** it is validated +- **THEN** it includes the supported rules subcommands diff --git a/openspec/changes/code-review-10-review-scope-modes/specs/review-run-command/spec.md b/openspec/changes/code-review-10-review-scope-modes/specs/review-run-command/spec.md new file mode 100644 index 0000000..52e4b40 --- /dev/null +++ b/openspec/changes/code-review-10-review-scope-modes/specs/review-run-command/spec.md @@ -0,0 +1,59 @@ +## MODIFIED Requirements + +### Requirement: End-to-End `specfact code review run` in modules repo +The `specfact-code-review` bundle SHALL provide a fully wired +`specfact code review run` command that orchestrates the existing tool runners, +supports explicit `changed` and `full` auto-discovery modes plus repo-relative +path filtering, and emits a governed `ReviewReport` with correct exit codes. + +#### Scenario: Clean review fixture returns PASS +- **GIVEN** `tests/fixtures/review/clean_module.py` and its matching tests +- **WHEN** `specfact code review run tests/fixtures/review/clean_module.py` is executed +- **THEN** the report verdict is `PASS` and the process exits `0` + +#### Scenario: Dirty review fixture returns FAIL +- **GIVEN** `tests/fixtures/review/dirty_module.py` with blocking findings +- **WHEN** `specfact code review run tests/fixtures/review/dirty_module.py` is executed +- **THEN** the report verdict is `FAIL` and the process exits `1` + +#### Scenario: JSON output writes a valid ReviewReport to a file +- **GIVEN** a review run over one or more files +- **WHEN** `specfact code review run --json` is executed +- **THEN** the command writes a valid `ReviewReport` JSON payload to the selected output path +- **AND** stdout reports that output path instead of printing the full JSON payload + +#### Scenario: Score-only output emits only reward delta +- **GIVEN** a review run over one or more files +- **WHEN** `specfact code review run --score-only` is executed +- **THEN** stdout contains only the integer reward delta and a trailing newline + +#### Scenario: Default auto-discovery reviews changed files +- **GIVEN** no positional file arguments and no explicit `--scope` +- **WHEN** `specfact code review run` is executed +- **THEN** the command reviews files from the changed-file set for the current repo + +#### Scenario: Full scope reviews the governed repository file set +- **GIVEN** no positional file arguments +- **WHEN** `specfact code review run --scope full` is executed +- **THEN** the command reviews the governed repository Python file set instead of only changed files +- **AND** test files remain excluded by default unless users opt in with `--include-tests` or explicitly target test subtrees with `--path tests/...` + +#### Scenario: Full scope can be limited to a package subtree +- **GIVEN** no positional file arguments +- **WHEN** `specfact code review run --scope full --path packages/specfact-code-review` is executed +- **THEN** only reviewable files under that package path and its subfolders are included + +#### Scenario: Changed scope can be limited to test and source subtrees +- **GIVEN** changed files exist in multiple repository areas +- **WHEN** `specfact code review run --scope changed --path packages/specfact-code-review --path tests/unit/specfact_code_review` is executed +- **THEN** only changed files under those repo-relative prefixes are reviewed + +#### Scenario: Empty filtered scope fails fast +- **GIVEN** the selected scope and path filters match no reviewable files +- **WHEN** `specfact code review run` is executed +- **THEN** the command exits non-zero with an actionable empty-scope message + +#### Scenario: Positional files cannot be mixed with auto-scope controls +- **GIVEN** one or more positional file arguments +- **WHEN** `specfact code review run src/example.py --scope full --path src` is executed +- **THEN** the command rejects the invocation and instructs the user to choose positional files or auto-scope controls diff --git a/openspec/changes/code-review-10-review-scope-modes/tasks.md b/openspec/changes/code-review-10-review-scope-modes/tasks.md new file mode 100644 index 0000000..7d3d8dc --- /dev/null +++ b/openspec/changes/code-review-10-review-scope-modes/tasks.md @@ -0,0 +1,30 @@ +# Tasks: code-review-10 review scope modes + +## 1. Rehydrate scope and confirm prerequisites + +- [x] 1.1 Reapply upstream `code-review-10-review-scope-modes` in this modules repository +- [x] 1.2 Confirm `code-review-08-review-run-integration` behavior is the baseline for the bundle command +- [x] 1.3 Confirm the upstream default remains changed-only auto-discovery for automation compatibility + +## 2. Add failing tests and fixtures first + +- [x] 2.1 Add or extend unit tests for `--scope changed` and `--scope full` +- [x] 2.2 Add failing tests for repeatable `--path` subtree filtering in both scope modes +- [x] 2.3 Add failing tests for empty-scope and invalid targeting combinations +- [x] 2.4 Update cli-val `specfact-code-review-run` scenarios for changed-only, full-review, and subtree-filtered runs +- [x] 2.5 Run the new tests before implementation and record failing evidence in `TDD_EVIDENCE.md` + +## 3. Implement bundle scope selection + +- [x] 3.1 Extend `packages/specfact-code-review/src/specfact_code_review/run/commands.py` with `--scope` and repeatable `--path` +- [x] 3.2 Add or update helper logic for deterministic candidate-file selection and recursive subtree filtering +- [x] 3.3 Reject invocations that mix positional files with auto-scope controls +- [x] 3.4 Emit actionable failures when selected filters leave no reviewable files + +## 4. Validate and document the change + +- [x] 4.1 Record passing evidence for unit, e2e, and cli-val checks in `TDD_EVIDENCE.md` +- [x] 4.2 Update `docs/modules/code-review.md` with changed-only, full-review, and subtree-review examples +- [x] 4.3 Bump the `specfact-code-review` bundle minor version and update `CHANGELOG.md` +- [x] 4.4 Re-sign and verify the updated module package metadata +- [x] 4.5 Run format, type-check, lint, yaml-lint, contract-test, smart-test, targeted tests, runtime/docs validation, signed-manifest verification, and the final `test` gate in the worktree diff --git a/packages/specfact-code-review/module-package.yaml b/packages/specfact-code-review/module-package.yaml index 7f1b1e8..b8d331a 100644 --- a/packages/specfact-code-review/module-package.yaml +++ b/packages/specfact-code-review/module-package.yaml @@ -1,5 +1,5 @@ name: nold-ai/specfact-code-review -version: 0.43.1 +version: 0.44.0 commands: - code tier: official @@ -22,5 +22,5 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:584e3ed7743470072dff872bff6b0f453baf70ee2ba31ad6c71fb724ca9e5c78 - signature: 47win2dUSo6BCLqRgtB7Z/dEQuvUJOdj78GzRj/kKroUV2VdIFLQX+PSIAgjhxW72hn+GI2hbnUmWuciSJ+uCg== + checksum: sha256:4821d747f0341fb8d7a4843619c0485e2a9b96ea9476c980ce9e3f2aba6a3e31 + signature: fCpAGDYn06PnRUz/LVMmxaVcgnffGUXFc+f7gti4imXQrwerPFg7IfvkFRRropzI1LmmKgh/8YSo64+bSSWXAQ== diff --git a/packages/specfact-code-review/src/specfact_code_review/review/commands.py b/packages/specfact-code-review/src/specfact_code_review/review/commands.py index bb7ef90..ff73483 100644 --- a/packages/specfact-code-review/src/specfact_code_review/review/commands.py +++ b/packages/specfact-code-review/src/specfact_code_review/review/commands.py @@ -3,6 +3,7 @@ from __future__ import annotations from pathlib import Path +from typing import Literal import typer from icontract.errors import ViolationError @@ -21,6 +22,7 @@ def _friendly_run_command_error(exc: ValueError | ViolationError) -> str: for expected in ( "Use either --json or --score-only, not both.", "Use --out together with --json.", + "Choose positional files or auto-scope controls, not both.", ): if expected in message: return expected @@ -40,6 +42,17 @@ def _resolve_include_tests(*, files: list[Path], include_tests: bool | None, int @review_app.command("run") def _run( files: list[Path] = typer.Argument(None, metavar="FILES..."), + *, + scope: Literal["changed", "full"] | None = typer.Option( + None, + "--scope", + help="Auto-discovery scope when positional files are omitted: changed or full.", + ), + path_filters: list[Path] | None = typer.Option( + None, + "--path", + help="Repeatable repo-relative path prefix used to limit auto-discovered review files.", + ), include_tests: bool | None = typer.Option( None, "--include-tests/--exclude-tests", @@ -75,6 +88,8 @@ def _run( exit_code, output = run_command( files, include_tests=resolved_include_tests, + scope=scope, + path_filters=path_filters, include_noise=include_noise, json_output=json_output, out=out, diff --git a/packages/specfact-code-review/src/specfact_code_review/run/commands.py b/packages/specfact-code-review/src/specfact_code_review/run/commands.py index 07230d0..b083bdf 100644 --- a/packages/specfact-code-review/src/specfact_code_review/run/commands.py +++ b/packages/specfact-code-review/src/specfact_code_review/run/commands.py @@ -5,7 +5,9 @@ import subprocess import sys from collections import defaultdict +from collections.abc import Iterable from pathlib import Path +from typing import Literal from beartype import beartype from icontract import ensure, require @@ -18,6 +20,7 @@ console = Console() progress_console = Console(stderr=True) +AutoScope = Literal["changed", "full"] def _is_test_file(file_path: Path) -> bool: @@ -58,10 +61,114 @@ def _changed_files_from_git_diff(*, include_tests: bool) -> list[Path]: return [file_path for file_path in deduped_python_files if not _is_test_file(file_path)] -def _resolve_files(files: list[Path], *, include_tests: bool) -> list[Path]: - resolved = files or _changed_files_from_git_diff(include_tests=include_tests) +def _all_python_files_from_git() -> list[Path]: + tracked_files = _git_file_list( + ["git", "ls-files", "--cached"], + error_message="Unable to determine tracked repository files from `git ls-files --cached`.", + ) + untracked_files = _git_file_list( + ["git", "ls-files", "--others", "--exclude-standard"], + error_message="Unable to determine untracked files from `git ls-files --others --exclude-standard`.", + ) + python_files = [ + file_path + for file_path in [*tracked_files, *untracked_files] + if file_path.suffix == ".py" and file_path.is_file() + ] + return list(dict.fromkeys(python_files)) + + +def _path_filter_matches(file_path: Path, path_filter: Path) -> bool: + return file_path == path_filter or path_filter in file_path.parents + + +def _filtered_files(files: Iterable[Path], *, path_filters: list[Path]) -> list[Path]: + if not path_filters: + return list(files) + normalized_filters = [path_filter for path_filter in path_filters if str(path_filter).strip()] + for path_filter in normalized_filters: + if path_filter.is_absolute(): + raise ValueError(f"Path filters must be repo-relative: {path_filter}") + return [ + file_path + for file_path in files + if any(_path_filter_matches(file_path, path_filter) for path_filter in normalized_filters) + ] + + +def _auto_scope_message(*, scope: AutoScope, path_filters: list[Path]) -> str: + parts = [f"--scope {scope}", *(f"--path {path_filter}" for path_filter in path_filters)] + return " ".join(parts) + + +def _raise_if_targeting_styles_conflict( + files: list[Path], + *, + scope: AutoScope | None, + path_filters: list[Path], +) -> None: + if files and (scope is not None or path_filters): + raise ValueError("Choose positional files or auto-scope controls, not both.") + + +def _resolve_positional_files(files: list[Path]) -> list[Path]: + if files: + return files + raise ValueError("No Python files to review were provided or detected from tracked or untracked changes.") + + +def _resolve_auto_discovered_files( + *, + include_tests: bool, + scope: AutoScope, + path_filters: list[Path], +) -> list[Path]: + if scope == "full": + return _resolve_full_scope_files(include_tests=include_tests, path_filters=path_filters) + return _resolve_changed_scope_files(include_tests=include_tests, path_filters=path_filters) + + +def _resolve_full_scope_files(*, include_tests: bool, path_filters: list[Path]) -> list[Path]: + resolved = _all_python_files_from_git() + if not include_tests and not path_filters: + return [file_path for file_path in resolved if not _is_test_file(file_path)] + return resolved + + +def _resolve_changed_scope_files(*, include_tests: bool, path_filters: list[Path]) -> list[Path]: + changed_include_tests = include_tests or bool(path_filters) + return _changed_files_from_git_diff(include_tests=changed_include_tests) + + +def _raise_for_empty_auto_scope(*, scope: AutoScope, path_filters: list[Path]) -> None: + auto_scope_message = _auto_scope_message(scope=scope, path_filters=path_filters) + raise ValueError( + f"No reviewable files matched the selected auto-scope controls ({auto_scope_message}). " + "Adjust --scope/--path or pass positional files." + ) + + +def _resolve_files( + files: list[Path], + *, + include_tests: bool, + scope: AutoScope | None, + path_filters: list[Path], +) -> list[Path]: + _raise_if_targeting_styles_conflict(files, scope=scope, path_filters=path_filters) + if files: + resolved = _resolve_positional_files(files) + else: + selected_scope: AutoScope = scope or "changed" + resolved = _resolve_auto_discovered_files( + include_tests=include_tests, + scope=selected_scope, + path_filters=path_filters, + ) + resolved = _filtered_files(resolved, path_filters=path_filters) + if not resolved: - raise ValueError("No Python files to review were provided or detected from tracked or untracked changes.") + _raise_for_empty_auto_scope(scope=scope or "changed", path_filters=path_filters) missing = [file_path for file_path in resolved if not file_path.is_file()] if missing: @@ -195,6 +302,8 @@ def run_command( files: list[Path] | None = None, *, include_tests: bool = False, + scope: AutoScope | None = None, + path_filters: list[Path] | None = None, include_noise: bool = False, json_output: bool = False, out: Path | None = None, @@ -203,7 +312,12 @@ def run_command( fix: bool = False, ) -> tuple[int, str | None]: """Execute a governed review run over the provided files.""" - resolved_files = _resolve_files(files or [], include_tests=include_tests) + resolved_files = _resolve_files( + files or [], + include_tests=include_tests, + scope=scope, + path_filters=path_filters or [], + ) report = _run_review_with_progress( resolved_files, no_tests=no_tests, diff --git a/packages/specfact-code-review/src/specfact_code_review/run/runner.py b/packages/specfact-code-review/src/specfact_code_review/run/runner.py index 639e12e..ca1d90f 100644 --- a/packages/specfact-code-review/src/specfact_code_review/run/runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/run/runner.py @@ -122,7 +122,7 @@ def _pytest_targets(test_files: list[Path]) -> list[Path]: if len(test_files) <= 1: return test_files common_root = Path(os.path.commonpath([str(test_file) for test_file in test_files])) - if common_root.is_dir() and common_root.parts[:2] == ("tests", "unit"): + if common_root.is_dir() and common_root.parts[:2] == ("tests", "unit") and len(common_root.parts) > 3: return [common_root] return test_files @@ -147,7 +147,7 @@ def _run_pytest_with_coverage(test_files: list[Path]) -> tuple[subprocess.Comple capture_output=True, text=True, check=False, - timeout=30, + timeout=120, env=_pytest_env(), ) return result, coverage_path diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py index 539bba6..3d1af20 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py @@ -7,6 +7,7 @@ import subprocess import tempfile from pathlib import Path +from typing import Literal, cast from beartype import beartype from icontract import ensure, require @@ -22,6 +23,8 @@ "print-in-src": "architecture", } SEMGREP_TIMEOUT_SECONDS = 90 +SEMGREP_RETRY_ATTEMPTS = 2 +SemgrepCategory = Literal["clean_code", "architecture"] def _normalize_path_variants(path_value: str | Path) -> set[str]: @@ -76,6 +79,101 @@ def _resolve_config_path() -> Path: raise FileNotFoundError(f"Semgrep config not found at {config_path}") +def _run_semgrep_command(files: list[Path]) -> subprocess.CompletedProcess[str]: + with tempfile.TemporaryDirectory(prefix="semgrep-home-") as temp_home: + semgrep_home = Path(temp_home) + semgrep_log_dir = semgrep_home / ".semgrep" + semgrep_log_dir.mkdir(parents=True, exist_ok=True) + env = os.environ.copy() + env["HOME"] = str(semgrep_home) + env["XDG_CONFIG_HOME"] = str(semgrep_home / ".config") + env["XDG_CACHE_HOME"] = str(semgrep_home / ".cache") + env["SEMGREP_SETTINGS_FILE"] = str(semgrep_log_dir / "settings.yml") + env.setdefault("SEMGREP_SEND_METRICS", "off") + return subprocess.run( + [ + "semgrep", + "--disable-version-check", + "--config", + str(_resolve_config_path()), + "--json", + *(str(file_path) for file_path in files), + ], + capture_output=True, + text=True, + check=False, + timeout=SEMGREP_TIMEOUT_SECONDS, + env=env, + ) + + +def _load_semgrep_results(files: list[Path]) -> list[object]: + last_error: Exception | None = None + for _attempt in range(SEMGREP_RETRY_ATTEMPTS): + try: + result = _run_semgrep_command(files) + payload = json.loads(result.stdout) + if not isinstance(payload, dict): + raise ValueError("semgrep output must be an object") + raw_results = payload.get("results", []) + if not isinstance(raw_results, list): + raise ValueError("semgrep results must be a list") + return raw_results + except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: + last_error = exc + if last_error is None: + raise ValueError("semgrep returned no usable results") + raise last_error + + +def _category_for_rule(rule: str) -> SemgrepCategory | None: + category = SEMGREP_RULE_CATEGORY.get(rule) + if category in {"clean_code", "architecture"}: + return cast(SemgrepCategory, category) + return None + + +def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> ReviewFinding | None: + filename = item["path"] + if not isinstance(filename, str): + raise ValueError("semgrep filename must be a string") + if _normalize_path_variants(filename).isdisjoint(allowed_paths): + return None + + raw_rule = item["check_id"] + if not isinstance(raw_rule, str): + raise ValueError("semgrep rule must be a string") + rule = _normalize_rule_id(raw_rule) + category = _category_for_rule(rule) + if category is None: + return None + + start = item["start"] + if not isinstance(start, dict): + raise ValueError("semgrep start location must be an object") + line = start["line"] + if not isinstance(line, int): + raise ValueError("semgrep line must be an integer") + + extra = item["extra"] + if not isinstance(extra, dict): + raise ValueError("semgrep extra payload must be an object") + message = extra["message"] + if not isinstance(message, str): + raise ValueError("semgrep message must be a string") + + return ReviewFinding( + category=category, + severity="warning", + tool="semgrep", + rule=rule, + file=filename, + line=line, + message=message, + fixable=False, + ) + + @beartype @require(lambda files: isinstance(files, list), "files must be a list") @require(lambda files: all(isinstance(file_path, Path) for file_path in files), "files must contain Path instances") @@ -90,37 +188,7 @@ def run_semgrep(files: list[Path]) -> list[ReviewFinding]: return [] try: - with tempfile.TemporaryDirectory(prefix="semgrep-home-") as temp_home: - semgrep_home = Path(temp_home) - semgrep_log_dir = semgrep_home / ".semgrep" - semgrep_log_dir.mkdir(parents=True, exist_ok=True) - env = os.environ.copy() - env["HOME"] = str(semgrep_home) - env["XDG_CONFIG_HOME"] = str(semgrep_home / ".config") - env["XDG_CACHE_HOME"] = str(semgrep_home / ".cache") - env["SEMGREP_SETTINGS_FILE"] = str(semgrep_log_dir / "settings.yml") - env.setdefault("SEMGREP_SEND_METRICS", "off") - result = subprocess.run( - [ - "semgrep", - "--disable-version-check", - "--config", - str(_resolve_config_path()), - "--json", - *(str(file_path) for file_path in files), - ], - capture_output=True, - text=True, - check=False, - timeout=SEMGREP_TIMEOUT_SECONDS, - env=env, - ) - payload = json.loads(result.stdout) - if not isinstance(payload, dict): - raise ValueError("semgrep output must be an object") - raw_results = payload.get("results", []) - if not isinstance(raw_results, list): - raise ValueError("semgrep results must be a list") + raw_results = _load_semgrep_results(files) except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: return _tool_error(files[0], f"Unable to parse Semgrep output: {exc}") @@ -130,46 +198,9 @@ def run_semgrep(files: list[Path]) -> list[ReviewFinding]: for item in raw_results: if not isinstance(item, dict): raise ValueError("semgrep finding must be an object") - filename = item["path"] - if not isinstance(filename, str): - raise ValueError("semgrep filename must be a string") - if _normalize_path_variants(filename).isdisjoint(allowed_paths): - continue - - raw_rule = item["check_id"] - if not isinstance(raw_rule, str): - raise ValueError("semgrep rule must be a string") - rule = _normalize_rule_id(raw_rule) - category = SEMGREP_RULE_CATEGORY.get(rule) - if category is None: - continue - - start = item["start"] - if not isinstance(start, dict): - raise ValueError("semgrep start location must be an object") - line = start["line"] - if not isinstance(line, int): - raise ValueError("semgrep line must be an integer") - - extra = item["extra"] - if not isinstance(extra, dict): - raise ValueError("semgrep extra payload must be an object") - message = extra["message"] - if not isinstance(message, str): - raise ValueError("semgrep message must be a string") - - findings.append( - ReviewFinding( - category=category, - severity="warning", - tool="semgrep", - rule=rule, - file=filename, - line=line, - message=message, - fixable=False, - ) - ) + finding = _finding_from_result(item, allowed_paths=allowed_paths) + if finding is not None: + findings.append(finding) except (KeyError, TypeError, ValueError) as exc: return _tool_error(files[0], f"Unable to parse Semgrep finding payload: {exc}") diff --git a/registry/index.json b/registry/index.json index b18d1e5..fec36af 100644 --- a/registry/index.json +++ b/registry/index.json @@ -73,9 +73,9 @@ }, { "id": "nold-ai/specfact-code-review", - "latest_version": "0.43.1", - "download_url": "modules/specfact-code-review-0.43.1.tar.gz", - "checksum_sha256": "678e79fa4004b3293f9f673e0ac74be55fcb2c1036af3982138e6b09685a3176", + "latest_version": "0.44.0", + "download_url": "modules/specfact-code-review-0.44.0.tar.gz", + "checksum_sha256": "6b0f48495c45c9fe2f0127ce5a76e4cdd60915f9080bfe68d224169718373643", "tier": "official", "publisher": { "name": "nold-ai", diff --git a/registry/modules/specfact-code-review-0.44.0.tar.gz b/registry/modules/specfact-code-review-0.44.0.tar.gz new file mode 100644 index 0000000..3fb2ea8 Binary files /dev/null and b/registry/modules/specfact-code-review-0.44.0.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.44.0.tar.gz.sha256 b/registry/modules/specfact-code-review-0.44.0.tar.gz.sha256 new file mode 100644 index 0000000..3e1a00e --- /dev/null +++ b/registry/modules/specfact-code-review-0.44.0.tar.gz.sha256 @@ -0,0 +1 @@ +6b0f48495c45c9fe2f0127ce5a76e4cdd60915f9080bfe68d224169718373643 diff --git a/registry/signatures/specfact-code-review-0.44.0.tar.sig b/registry/signatures/specfact-code-review-0.44.0.tar.sig new file mode 100644 index 0000000..068e9a4 --- /dev/null +++ b/registry/signatures/specfact-code-review-0.44.0.tar.sig @@ -0,0 +1 @@ +fCpAGDYn06PnRUz/LVMmxaVcgnffGUXFc+f7gti4imXQrwerPFg7IfvkFRRropzI1LmmKgh/8YSo64+bSSWXAQ== diff --git a/tests/cli-contracts/specfact-code-review-run.scenarios.yaml b/tests/cli-contracts/specfact-code-review-run.scenarios.yaml index e815dee..7be31de 100644 --- a/tests/cli-contracts/specfact-code-review-run.scenarios.yaml +++ b/tests/cli-contracts/specfact-code-review-run.scenarios.yaml @@ -9,6 +9,32 @@ scenarios: exit_code: 0 stdout_contains: - '"overall_verdict":"PASS"' + - name: full-scope-package-subtree + type: pattern + argv: + - --scope + - full + - --path + - packages/specfact-code-review + - --json + expect: + exit_code: 0 + stdout_contains: + - '"run_id":' + - name: changed-scope-subtree + type: pattern + argv: + - --scope + - changed + - --path + - packages/specfact-code-review + - --path + - tests/unit/specfact_code_review + - --json + expect: + exit_code: 0 + stdout_contains: + - '"run_id":' - name: missing-file-errors type: anti-pattern argv: @@ -17,3 +43,15 @@ scenarios: exit_code: 2 stderr_contains: - not found + - name: positional-files-cannot-mix-with-scope + type: anti-pattern + argv: + - tests/fixtures/review/clean_module.py + - --scope + - full + - --path + - tests/fixtures/review + expect: + exit_code: 2 + stderr_contains: + - choose positional files or auto-scope controls diff --git a/tests/unit/specfact_code_review/run/test_commands.py b/tests/unit/specfact_code_review/run/test_commands.py index 92dd259..432b71d 100644 --- a/tests/unit/specfact_code_review/run/test_commands.py +++ b/tests/unit/specfact_code_review/run/test_commands.py @@ -28,6 +28,13 @@ def _report(*, score: int = 85) -> ReviewReport: ) +def _write_repo_file(repo_root: Path, relative_path: str, *, content: str = "VALUE = 1\n") -> Path: + file_path = repo_root / relative_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text(content, encoding="utf-8") + return Path(relative_path) + + def test_run_command_json_output_uses_review_report(monkeypatch: Any, tmp_path: Path) -> None: monkeypatch.setattr( "specfact_code_review.run.commands.run_review", @@ -105,6 +112,96 @@ def fake_run_review(files: list[Path], **_kwargs: Any) -> ReviewReport: assert out.exists() +def test_run_command_supports_full_scope_and_path_filters(monkeypatch: Any, tmp_path: Path) -> None: + package_file = _write_repo_file( + tmp_path, + "packages/specfact-code-review/src/specfact_code_review/run/commands.py", + ) + _write_repo_file(tmp_path, "packages/specfact-backlog/src/specfact_backlog/commands.py") + monkeypatch.chdir(tmp_path) + + recorded: dict[str, list[Path]] = {} + monkeypatch.setattr( + "specfact_code_review.run.commands._all_python_files_from_git", + lambda: [package_file, Path("packages/specfact-backlog/src/specfact_backlog/commands.py")], + raising=False, + ) + + def fake_run_review(files: list[Path], **_kwargs: Any) -> ReviewReport: + recorded["files"] = files + return _report() + + monkeypatch.setattr("specfact_code_review.run.commands.run_review", fake_run_review) + + result = runner.invoke( + app, + [ + "review", + "run", + "--scope", + "full", + "--path", + "packages/specfact-code-review", + "--json", + "--out", + "review-report.json", + ], + ) + + assert result.exit_code == 0 + assert recorded["files"] == [package_file] + + +def test_run_command_supports_changed_scope_with_repeatable_path_filters(monkeypatch: Any, tmp_path: Path) -> None: + package_file = _write_repo_file( + tmp_path, + "packages/specfact-code-review/src/specfact_code_review/run/commands.py", + ) + test_file = _write_repo_file( + tmp_path, + "tests/unit/specfact_code_review/run/test_commands.py", + content="def test_scope_paths() -> None:\n assert True\n", + ) + _write_repo_file(tmp_path, "packages/specfact-backlog/src/specfact_backlog/commands.py") + monkeypatch.chdir(tmp_path) + + recorded: dict[str, list[Path]] = {} + monkeypatch.setattr( + "specfact_code_review.run.commands._changed_files_from_git_diff", + lambda *, include_tests: [ + package_file, + test_file, + Path("packages/specfact-backlog/src/specfact_backlog/commands.py"), + ], + ) + + def fake_run_review(files: list[Path], **_kwargs: Any) -> ReviewReport: + recorded["files"] = files + return _report() + + monkeypatch.setattr("specfact_code_review.run.commands.run_review", fake_run_review) + + result = runner.invoke( + app, + [ + "review", + "run", + "--scope", + "changed", + "--path", + "packages/specfact-code-review", + "--path", + "tests/unit/specfact_code_review", + "--json", + "--out", + "review-report.json", + ], + ) + + assert result.exit_code == 0 + assert recorded["files"] == [package_file, test_file] + + def test_run_command_rejects_out_without_json(tmp_path: Path) -> None: out = tmp_path / "review-report.json" result = runner.invoke(app, ["review", "run", "--out", str(out), "tests/fixtures/review/clean_module.py"]) @@ -141,6 +238,38 @@ def test_run_command_rejects_json_and_score_only_together() -> None: assert "not both" in result.output +def test_run_command_rejects_scope_mixed_with_positional_files() -> None: + result = runner.invoke( + app, + [ + "review", + "run", + "tests/fixtures/review/clean_module.py", + "--scope", + "full", + ], + ) + + assert result.exit_code == 2 + assert "choose positional files or auto-scope controls" in result.output.lower() + + +def test_run_command_rejects_path_mixed_with_positional_files() -> None: + result = runner.invoke( + app, + [ + "review", + "run", + "tests/fixtures/review/clean_module.py", + "--path", + "tests/fixtures/review", + ], + ) + + assert result.exit_code == 2 + assert "choose positional files or auto-scope controls" in result.output.lower() + + def test_run_command_fix_mode_applies_fixes_before_second_run(monkeypatch: Any) -> None: calls: list[str] = [] @@ -188,6 +317,36 @@ def test_run_command_default_output_renders_findings(monkeypatch: Any) -> None: assert "Rendered output report." in result.output +def test_run_command_fails_when_scope_and_paths_match_no_files(monkeypatch: Any, tmp_path: Path) -> None: + package_file = _write_repo_file( + tmp_path, + "packages/specfact-code-review/src/specfact_code_review/run/commands.py", + ) + monkeypatch.chdir(tmp_path) + monkeypatch.setattr( + "specfact_code_review.run.commands._all_python_files_from_git", + lambda: [package_file], + raising=False, + ) + + result = runner.invoke( + app, + [ + "review", + "run", + "--scope", + "full", + "--path", + "packages/specfact-backlog", + ], + ) + + assert result.exit_code == 2 + assert "no reviewable files" in result.output.lower() + assert "scope" in result.output.lower() + assert "full" in result.output.lower() + + def test_changed_files_from_git_diff_filters_python_files(monkeypatch: Any, tmp_path: Path) -> None: python_file = tmp_path / "example.py" python_file.write_text("VALUE = 1\n", encoding="utf-8") diff --git a/tests/unit/specfact_code_review/run/test_runner.py b/tests/unit/specfact_code_review/run/test_runner.py index a82f5fa..cd902f3 100644 --- a/tests/unit/specfact_code_review/run/test_runner.py +++ b/tests/unit/specfact_code_review/run/test_runner.py @@ -260,6 +260,35 @@ def test_run_review_suppresses_global_duplicate_code_noise_by_default(monkeypatc assert report.findings == [] +def test_pytest_targets_collapses_only_specific_subdirectories(tmp_path: Path) -> None: + run_tests = tmp_path / "tests/unit/specfact_code_review/run" + run_tests.mkdir(parents=True) + first = run_tests / "test_commands.py" + second = run_tests / "test_runner.py" + first.write_text("def test_one():\n assert True\n", encoding="utf-8") + second.write_text("def test_two():\n assert True\n", encoding="utf-8") + + assert _pytest_targets([first.relative_to(tmp_path), second.relative_to(tmp_path)]) == [ + Path("tests/unit/specfact_code_review/run") + ] + + +def test_pytest_targets_keeps_files_when_common_root_is_too_broad(tmp_path: Path) -> None: + run_tests = tmp_path / "tests/unit/specfact_code_review/run" + review_tests = tmp_path / "tests/unit/specfact_code_review/review" + run_tests.mkdir(parents=True) + review_tests.mkdir(parents=True) + first = run_tests / "test_commands.py" + second = review_tests / "test_commands.py" + first.write_text("def test_one():\n assert True\n", encoding="utf-8") + second.write_text("def test_two():\n assert True\n", encoding="utf-8") + + assert _pytest_targets([first.relative_to(tmp_path), second.relative_to(tmp_path)]) == [ + Path("tests/unit/specfact_code_review/run/test_commands.py"), + Path("tests/unit/specfact_code_review/review/test_commands.py"), + ] + + def test_run_review_can_include_global_duplicate_code_noise(monkeypatch: MonkeyPatch) -> None: duplicate_code_finding = ReviewFinding( category="style", diff --git a/tests/unit/specfact_code_review/tools/test_semgrep_runner.py b/tests/unit/specfact_code_review/tools/test_semgrep_runner.py index bae4ca5..70f0de1 100644 --- a/tests/unit/specfact_code_review/tools/test_semgrep_runner.py +++ b/tests/unit/specfact_code_review/tools/test_semgrep_runner.py @@ -111,7 +111,57 @@ def test_run_semgrep_returns_empty_list_for_clean_file(tmp_path: Path, monkeypat findings = run_semgrep([file_path]) - assert findings == [] + assert not findings + + +def test_run_semgrep_ignores_unsupported_rules(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "target.py" + payload = { + "results": [ + { + "check_id": "unsupported-rule", + "path": str(file_path), + "start": {"line": 3}, + "extra": {"message": "Ignored rule."}, + } + ] + } + monkeypatch.setattr( + subprocess, + "run", + Mock(return_value=completed_process("semgrep", stdout=json.dumps(payload), returncode=1)), + ) + + findings = run_semgrep([file_path]) + + assert not findings + + +def test_run_semgrep_retries_after_transient_parse_failure(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "target.py" + payload = { + "results": [ + { + "check_id": "unguarded-nested-access", + "path": str(file_path), + "start": {"line": 2}, + "extra": {"message": "Nested attribute access can fail."}, + } + ] + } + run_mock = Mock( + side_effect=[ + completed_process("semgrep", stdout="", returncode=2), + completed_process("semgrep", stdout=json.dumps(payload), returncode=1), + ] + ) + monkeypatch.setattr(subprocess, "run", run_mock) + + findings = run_semgrep([file_path]) + + assert len(findings) == 1 + assert findings[0].rule == "unguarded-nested-access" + assert run_mock.call_count == 2 @pytest.mark.parametrize(("fixture_name", "expected_rule"), list(BAD_FIXTURE_RULES.items())) @@ -132,4 +182,4 @@ def test_each_good_fixture_triggers_no_findings(fixture_name: str) -> None: findings = run_semgrep([FIXTURE_ROOT / fixture_name]) - assert findings == [] + assert not findings