From 9458eebe37f84399bf0b30a3ed8b1912a9a04d12 Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Wed, 15 Apr 2026 09:16:11 +0200 Subject: [PATCH 1/4] fix PR 193 review findings --- .github/workflows/pr-orchestrator.yml | 3 +- .../workflows/sign-modules-on-approval.yml | 12 +- README.md | 2 +- docs/modules/code-review.md | 37 +++++- docs/reference/module-security.md | 10 +- .../specs/code-review-bug-finding/spec.md | 9 +- .../specs/contract-runner/spec.md | 10 +- .../specs/review-cli-contracts/spec.md | 14 +- .../specs/review-run-command/spec.md | 2 + .../specs/sidecar-route-extraction/spec.md | 2 + .../tasks.md | 2 +- .../specfact-code-review/.semgrep/bugs.yaml | 2 +- .../specfact-code-review/module-package.yaml | 5 +- .../src/specfact_code_review/_review_utils.py | 2 +- .../specfact_code_review/review/commands.py | 33 +++-- .../src/specfact_code_review/run/commands.py | 83 ++++++++---- .../tools/contract_runner.py | 57 +++++++-- .../tools/radon_runner.py | 20 ++- .../tools/semgrep_runner.py | 50 +++----- .../specfact-codebase/module-package.yaml | 5 +- .../validators/sidecar/frameworks/base.py | 17 ++- .../validators/sidecar/frameworks/fastapi.py | 121 +++++++----------- .../validators/sidecar/frameworks/flask.py | 8 ++ scripts/pre_commit_code_review.py | 25 ++-- .../specfact-code-review-run.scenarios.yaml | 41 +++++- .../scripts/test_pre_commit_code_review.py | 21 +++ .../tools/test_basedpyright_runner.py | 4 +- .../tools/test_contract_runner.py | 16 ++- .../tools/test_radon_runner.py | 43 +++++++ .../tools/test_semgrep_runner.py | 20 ++- .../test_sidecar_framework_extractors.py | 51 +++++++- ...git_branch_module_signature_flag_script.py | 18 +++ .../workflows/test_pr_orchestrator_signing.py | 4 +- .../test_sign_modules_on_approval.py | 2 + 34 files changed, 535 insertions(+), 216 deletions(-) diff --git a/.github/workflows/pr-orchestrator.yml b/.github/workflows/pr-orchestrator.yml index 2b38717d..80bbcbe3 100644 --- a/.github/workflows/pr-orchestrator.yml +++ b/.github/workflows/pr-orchestrator.yml @@ -92,8 +92,7 @@ jobs: VERIFY_CMD+=(--version-check-base "$BASE_REF") if [ "$TARGET_BRANCH" = "dev" ]; then VERIFY_CMD+=(--metadata-only) - elif [ "$TARGET_BRANCH" = "main" ] && \ - [ "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]; then + elif [ "$TARGET_BRANCH" = "main" ]; then VERIFY_CMD+=(--require-signature) fi else diff --git a/.github/workflows/sign-modules-on-approval.yml b/.github/workflows/sign-modules-on-approval.yml index 02ef3a2a..442618ed 100644 --- a/.github/workflows/sign-modules-on-approval.yml +++ b/.github/workflows/sign-modules-on-approval.yml @@ -30,6 +30,16 @@ jobs: echo "::notice::Skipping module signing: review state is not approved." exit 0 fi + author_association="${{ github.event.review.user.author_association }}" + case "$author_association" in + COLLABORATOR|MEMBER|OWNER) + ;; + *) + echo "sign=false" >> "$GITHUB_OUTPUT" + echo "::notice::Skipping module signing: reviewer association '${author_association}' is not trusted for signing." + exit 0 + ;; + esac base_ref="${{ github.event.pull_request.base.ref }}" if [ "$base_ref" != "dev" ] && [ "$base_ref" != "main" ]; then echo "sign=false" >> "$GITHUB_OUTPUT" @@ -44,7 +54,7 @@ jobs: exit 0 fi echo "sign=true" >> "$GITHUB_OUTPUT" - echo "Eligible for module signing (approved, same-repo PR to dev or main)." + echo "Eligible for module signing (approved by trusted reviewer, same-repo PR to dev or main)." - name: Guard signing secrets if: steps.gate.outputs.sign == 'true' diff --git a/README.md b/README.md index b32e7681..5838ce22 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,7 @@ pre-commit install pre-commit run --all-files ``` -**Code review gate (matches specfact-cli core):** runs in **Block 2** after the module verify hook and Block 1 quality hooks (`pre-commit-quality-checks.sh block2`, which calls `scripts/pre_commit_code_review.py`). Staged paths under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, and `openspec/changes/` are eligible; `openspec/changes/**/TDD_EVIDENCE.md` is excluded from the gate. Only staged **`.py` / `.pyi`** files are forwarded to SpecFact (YAML, registry tarballs, and similar are skipped). The hook blocks the commit when the JSON report contains **error**-severity findings; warning-only outcomes do not block. The helper runs `specfact code review run --json --out .specfact/code-review.json` on those Python paths and prints a short findings summary on stderr. Full CLI options (`--mode`, `--focus`, `--level`, `--bug-hunt`, etc.) are documented under [Code review module](./docs/modules/code-review.md). Block 1 is split into separate pre-commit hooks so output appears between stages instead of buffering until the end. Requires a local **specfact-cli** install (`hatch run dev-deps` resolves sibling `../specfact-cli` or `SPECFACT_CLI_REPO`). +**Code review gate (matches specfact-cli core):** runs in **Block 2** after the module verify hook and Block 1 quality hooks (`pre-commit-quality-checks.sh block2`, which calls `scripts/pre_commit_code_review.py`). Staged paths under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, and `openspec/changes/` are eligible; `openspec/changes/**/TDD_EVIDENCE.md` is excluded from the gate. Only staged **`.py` / `.pyi`** files are forwarded to SpecFact (YAML, registry tarballs, and similar are skipped). The hook blocks the commit when the JSON report contains **error**-severity findings; warning-only outcomes do not block. Non-blocking warnings reported by SpecFact still require remediation prior to merge unless a documented, approved exception exists. The helper runs `specfact code review run --json --out .specfact/code-review.json` on those Python paths and prints a short findings summary on stderr. Full CLI options (`--mode`, `--focus`, `--level`, `--bug-hunt`, etc.) are documented under [Code review module](./docs/modules/code-review.md). Block 1 is split into separate pre-commit hooks so output appears between stages instead of buffering until the end. Requires a local **specfact-cli** install (`hatch run dev-deps` resolves sibling `../specfact-cli` or `SPECFACT_CLI_REPO`). Scope notes: - Pre-commit runs `hatch run lint` in the **Block 1 — lint** hook when any staged path matches `*.py` / `*.pyi`, matching the CI quality job (Ruff alone does not run pylint). diff --git a/docs/modules/code-review.md b/docs/modules/code-review.md index cba6db81..2b42fc91 100644 --- a/docs/modules/code-review.md +++ b/docs/modules/code-review.md @@ -277,7 +277,7 @@ is skipped with no error. ### Contract runner -`specfact_code_review.tools.contract_runner.run_contract_check(files)` combines two +`specfact_code_review.tools.contract_runner.run_contract_check(files, *, bug_hunt=False)` combines two contract-oriented checks: 1. an AST scan for public functions missing `@require` / `@ensure` @@ -287,9 +287,10 @@ AST scan behavior: - only public module-level and class-level functions are checked - functions prefixed with `_` are treated as private and skipped -- the AST scan for `MISSING_ICONTRACT` runs **only when the file imports - `icontract`** (`from icontract …` or `import icontract`). Files that never - reference icontract skip the decorator scan and rely on CrossHair only +- the AST scan for `MISSING_ICONTRACT` runs only when a batch-level package/repo + scan root imports `icontract` (`from icontract …` or `import icontract`). + Reviewed files in a package that uses icontract are scanned even when the + changed file itself does not import icontract - missing icontract decorators become `contracts` findings with rule `MISSING_ICONTRACT` when the scan runs - unreadable or invalid Python files degrade to a single `tool_error` finding instead @@ -401,8 +402,15 @@ specfact code review rules update ## Review orchestration -`specfact_code_review.run.runner.run_review(files, no_tests=False)` orchestrates the -bundle runners in this order: +`specfact_code_review.run.runner.run_review( +files, +no_tests=False, +include_noise=False, +progress_callback=None, +bug_hunt=False, +review_level=None, +review_mode="enforce", +)` orchestrates the bundle runners in this order: 1. Ruff 2. Radon @@ -421,6 +429,23 @@ adding a new CLI flag. The merged findings are then scored into a governed `ReviewReport`. +Representative programmatic use: + +```python +from pathlib import Path + +from specfact_code_review.run.runner import run_review + +report = run_review( + [Path("src/app.py")], + no_tests=False, + include_noise=False, + bug_hunt=True, + review_level="error", + review_mode="shadow", +) +``` + ## Bundled policy pack The bundle now ships `specfact/clean-code-principles` as a resource payload at: diff --git a/docs/reference/module-security.md b/docs/reference/module-security.md index 1060aabd..89008203 100644 --- a/docs/reference/module-security.md +++ b/docs/reference/module-security.md @@ -46,11 +46,13 @@ Module packages carry **publisher** and **integrity** metadata so installation, - `SPECFACT_MODULE_PRIVATE_SIGN_KEY` - `SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE` - **Verification command** (`scripts/verify-modules-signature.py`): - - **Baseline (PR/CI and local hook)**: `--payload-from-filesystem --enforce-version-bump` — full payload checksum verification plus version-bump enforcement. This is the default integration path **without** `--require-signature` when the target branch is **`dev`** (pull requests to `dev`, or pushes to `dev`). - - **Strict mode**: add `--require-signature` so every manifest must include a verifiable `integrity.signature`. In `.github/workflows/pr-orchestrator.yml` this is appended for **pull requests whose base is `main`** and for **pushes to `main`**, in addition to the baseline flags. Locally, `scripts/pre-commit-verify-modules-signature.sh` adds `--require-signature` only when the checkout (or `GITHUB_BASE_REF` in Actions) is **`main`**; otherwise it runs the same baseline flags only. + - **Baseline (CI)**: `--payload-from-filesystem --enforce-version-bump` — full payload checksum verification plus version-bump enforcement. + - **Dev-target PR mode**: `.github/workflows/pr-orchestrator.yml` appends `--metadata-only` for pull requests targeting `dev` so branch work is not blocked before approval-time signing refreshes manifests. + - **Strict mode**: add `--require-signature` so every manifest must include a verifiable `integrity.signature`. In `.github/workflows/pr-orchestrator.yml` this is appended for **pull requests whose base is `main`** and for **pushes to `main`**, in addition to the baseline flags. Locally, `scripts/pre-commit-verify-modules-signature.sh` adds `--require-signature` only when the checkout (or `GITHUB_BASE_REF` in Actions) is **`main`**. + - **Local non-main hook mode**: `scripts/pre-commit-verify-modules-signature.sh` otherwise keeps the baseline command shape but adds `--metadata-only`, avoiding local checksum/signature enforcement on branches that are expected to be signed by CI. - **Pull request CI** also passes `--version-check-base ` (typically `origin/`) so version rules compare against the PR base. - - **CI uses the full verifier** (payload digest + rules above). It does **not** pass `--metadata-only`. The script still supports `--metadata-only` for optional tooling that only needs manifest shape and checksum format checks. -- **CI signing**: Approved same-repo PRs to `dev` or `main` may receive automated signing commits via `sign-modules-on-approval.yml` (repository secrets; merge-base scoped `--changed-only`). + - **CI uses the full verifier** for `main` and push checks (payload digest + rules above), while PRs targeting `dev` intentionally pass `--metadata-only`. The script still supports `--metadata-only` for optional tooling that only needs manifest shape and checksum format checks. +- **CI signing**: Approved same-repo PRs to `dev` or `main` from trusted reviewer associations may receive automated signing commits via `sign-modules-on-approval.yml` (repository secrets; merge-base scoped `--changed-only`). ## Public key and key rotation diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md index a91b7420..26f46c5f 100644 --- a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md @@ -1,3 +1,5 @@ +# Code Review Bug Finding + ## ADDED Requirements ### Requirement: Semgrep bug-finding rules pass @@ -10,14 +12,14 @@ silently skipped without error. #### Scenario: bugs.yaml present — security findings emitted - **WHEN** `.semgrep/bugs.yaml` exists in the bundle -- **AND** `run_semgrep` is called on Python files matching a bug rule -- **THEN** `ReviewFinding` records are returned with `category="security"` or `category="correctness"` +- **AND** `run_semgrep_bugs` is called on Python files matching a bug rule +- **THEN** `ReviewFinding` records are returned with `category="security"` or `category="clean_code"` - **AND** findings reference the matched rule id from `bugs.yaml` #### Scenario: bugs.yaml absent — pass is a no-op - **WHEN** no `.semgrep/bugs.yaml` file is discoverable -- **AND** `run_semgrep` is called +- **AND** `run_semgrep_bugs` is called - **THEN** no finding is returned for the missing bugs pass - **AND** no exception propagates to the caller @@ -25,6 +27,7 @@ silently skipped without error. - **WHEN** `specfact code review run --json` is executed on a file matching a bug rule - **THEN** the output JSON contains findings from both the clean-code and bug-finding passes +- **AND** `run_review` wires `run_semgrep_bugs` alongside the existing `run_semgrep` pass ### Requirement: CrossHair bug-hunt mode diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md index 30b57b66..5f8705d6 100644 --- a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md @@ -1,24 +1,26 @@ +# Contract Runner + ## MODIFIED Requirements ### Requirement: icontract Decorator AST Scan and CrossHair Fast Pass -The system SHALL AST-scan changed Python files for public functions missing +The system SHALL AST-scan reviewed Python files for public functions missing `@require` / `@ensure` decorators, and run CrossHair with a configurable per-path timeout for counterexample discovery. When no icontract usage is -detected in the reviewed files, `MISSING_ICONTRACT` findings SHALL be +detected in the reviewed files' package/repo scan roots, `MISSING_ICONTRACT` findings SHALL be suppressed entirely for that run. #### Scenario: Public function without icontract decorators produces a contracts finding when icontract is in use - **GIVEN** a Python file with a public function lacking icontract decorators -- **AND** at least one other reviewed file imports from `icontract` +- **AND** at least one file in the reviewed batch's package/repo scan roots imports from `icontract` - **WHEN** `run_contract_check(files=[...])` is called - **THEN** a `ReviewFinding` is returned with `category="contracts"` and `severity="warning"` #### Scenario: MISSING_ICONTRACT suppressed when no icontract usage detected -- **GIVEN** a set of reviewed Python files containing no `from icontract import` or +- **GIVEN** the package/repo scan roots for the reviewed Python files contain no `from icontract import` or `import icontract` statements - **WHEN** `run_contract_check(files=[...])` is called - **THEN** no `MISSING_ICONTRACT` findings are returned diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md index 583da50f..92256946 100644 --- a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md @@ -1,6 +1,8 @@ +# Review CLI Contracts + ## ADDED Requirements -### Requirement: Review-run CLI scenarios cover enforcement mode, focus facets, and severity level +### Requirement: Review-run CLI scenarios cover enforcement mode, bug-hunt, focus facets, and severity level The modules repository SHALL extend `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` so contract tests exercise the new `specfact code review run` flags together with existing scope and JSON output behaviour. @@ -11,20 +13,30 @@ The modules repository SHALL extend `tests/cli-contracts/specfact-code-review-ru - **THEN** it includes at least one scenario asserting `--mode shadow` yields process success (exit `0`) while JSON still reports a failing verdict when findings warrant it - **AND** it includes a control scenario showing `--mode enforce` (or default) preserves non-zero exit on blocking failures +#### Scenario: Scenarios cover --bug-hunt in shadow and enforce modes + +- **GIVEN** `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` +- **WHEN** it is validated after this change +- **THEN** it includes at least one scenario with `--bug-hunt --mode shadow` +- **AND** it includes at least one scenario with `--bug-hunt --mode enforce` + #### Scenario: Scenarios cover --focus facets - **GIVEN** the same scenario file - **WHEN** it is validated - **THEN** it includes coverage for `--focus` union behaviour (e.g. `source` + `docs`) and for `--focus tests` narrowing the file set +- **AND** it includes coverage for `--bug-hunt` composed with `--focus` #### Scenario: Scenarios cover --level filtering - **GIVEN** the same scenario file - **WHEN** it is validated - **THEN** it includes at least one scenario where `--level error` removes warnings from the JSON `findings` list +- **AND** it includes coverage for `--bug-hunt` composed with `--level error` #### Scenario: Scenarios cover invalid flag combinations - **GIVEN** the same scenario file - **WHEN** it is validated - **THEN** it includes an error-path scenario for `--focus` combined with `--include-tests` or `--exclude-tests` +- **AND** `--bug-hunt` remains composable with test-selection flags diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md index cd002811..c8cf4e13 100644 --- a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md @@ -1,3 +1,5 @@ +# Review Run Command Specification + ## ADDED Requirements ### Requirement: --bug-hunt flag on review run command diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md index d719aeb0..214dc426 100644 --- a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md @@ -1,3 +1,5 @@ +# Sidecar Route Extraction + ## ADDED Requirements ### Requirement: Framework extractors exclude .specfact from scan paths diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md index 61306429..4cdf8289 100644 --- a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md @@ -1,4 +1,4 @@ -## 1. Sidecar venv self-scan fix (specfact-codebase) +# 1. Sidecar venv self-scan fix (specfact-codebase) - [x] 1.1 Add `_EXCLUDED_DIR_NAMES` constant and `_iter_python_files(root)` generator to `BaseFrameworkExtractor` in `frameworks/base.py` that skips `.specfact`, `.git`, `__pycache__`, `node_modules` - [x] 1.2 Replace `search_path.rglob("*.py")` with `self._iter_python_files(search_path)` in `FastAPIExtractor.detect()` and `FastAPIExtractor.extract_routes()` diff --git a/packages/specfact-code-review/.semgrep/bugs.yaml b/packages/specfact-code-review/.semgrep/bugs.yaml index 536c3875..bb66f91b 100644 --- a/packages/specfact-code-review/.semgrep/bugs.yaml +++ b/packages/specfact-code-review/.semgrep/bugs.yaml @@ -35,7 +35,7 @@ rules: severity: WARNING patterns: - pattern: yaml.load(...) - - pattern-not-regex: yaml\.load\([^)]*Loader\s*= + - pattern-not-regex: (?s)yaml\.load\([\s\S]*?Loader\s*= metadata: specfact-category: security diff --git a/packages/specfact-code-review/module-package.yaml b/packages/specfact-code-review/module-package.yaml index fa5e9f0e..262f01a0 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.47.2 +version: 0.47.3 commands: - code tier: official @@ -23,5 +23,4 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:f57b0d5c239273df9c87eea31fabc07df630d44d89f3f552da20b1816dd316b5 - signature: uYll3YRRrGsNOHniavYpDE8Guq/bcwAT2vQ0GtmreHPi4lAHzdqv7ui8tlRh7uS/EFiqgIkBatULph2qXHoPBA== + checksum: sha256:582d07cb2941568056a35246db3dbb8ad612362a95bc84080d400f082bb6df80 diff --git a/packages/specfact-code-review/src/specfact_code_review/_review_utils.py b/packages/specfact-code-review/src/specfact_code_review/_review_utils.py index dd3cd3db..bbefd78a 100644 --- a/packages/specfact-code-review/src/specfact_code_review/_review_utils.py +++ b/packages/specfact-code-review/src/specfact_code_review/_review_utils.py @@ -39,7 +39,7 @@ def normalize_path_variants(path_value: str | Path) -> set[str]: @require(lambda files: all(isinstance(p, Path) for p in files)) @ensure(lambda result: isinstance(result, list)) def python_source_paths_for_tools(files: list[Path]) -> list[Path]: - """Paths Python linters and typecheckers should analyze (excludes YAML manifests, etc.).""" + """Python source and type stub paths linters/typecheckers should analyze.""" return [path for path in files if path.suffix in _PYTHON_LINTER_SUFFIXES] 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 3b6a66d1..891866b6 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 @@ -11,25 +11,34 @@ from specfact_code_review.ledger.commands import app as ledger_app from specfact_code_review.rules.commands import app as rules_app -from specfact_code_review.run.commands import run_command +from specfact_code_review.run.commands import ( + ConflictingScopeError, + FocusFacetConflictError, + InvalidOptionCombinationError, + MissingOutForJsonError, + NoReviewableFilesError, + RunCommandError, + run_command, +) app = typer.Typer(help="Code command extensions for structured review workflows.", no_args_is_help=True) review_app = typer.Typer(help="Governed code review workflows.", no_args_is_help=True) -def _friendly_run_command_error(exc: ValueError | ViolationError) -> str: - message = str(exc) - 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.", - "Cannot combine focus_facets with include_tests", - "No reviewable Python files matched the selected --focus facets.", +def _friendly_run_command_error(exc: RunCommandError | ValueError | ViolationError) -> str: + if isinstance( + exc, + ( + InvalidOptionCombinationError, + MissingOutForJsonError, + ConflictingScopeError, + FocusFacetConflictError, + NoReviewableFilesError, + ), ): - if expected in message: - return expected - return message + return str(exc) + return str(exc) def _resolve_include_tests(*, files: list[Path], include_tests: bool | None, interactive: bool) -> bool: 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 8b894997..f6e886c7 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 @@ -26,6 +26,32 @@ ReviewLevelFilter = Literal["error", "warning"] +class RunCommandError(ValueError): + """Structured validation error for review run command options.""" + + error_code = "run_command_error" + + +class InvalidOptionCombinationError(RunCommandError): + error_code = "invalid_option_combination" + + +class MissingOutForJsonError(RunCommandError): + error_code = "missing_out_for_json" + + +class ConflictingScopeError(RunCommandError): + error_code = "conflicting_scope" + + +class FocusFacetConflictError(RunCommandError): + error_code = "focus_facet_conflict" + + +class NoReviewableFilesError(RunCommandError): + error_code = "no_reviewable_files" + + @dataclass(frozen=True) class ReviewRunRequest: """Inputs needed to execute a governed review run.""" @@ -98,7 +124,7 @@ def _git_file_list(command: list[str], *, error_message: str) -> list[Path]: timeout=30, ) if result.returncode != 0: - raise ValueError(error_message) + raise RunCommandError(error_message) return [Path(line.strip()) for line in result.stdout.splitlines() if line.strip()] @@ -150,7 +176,7 @@ def _filtered_files(files: Iterable[Path], *, path_filters: list[Path]) -> list[ 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}") + raise RunCommandError(f"Path filters must be repo-relative: {path_filter}") return [ file_path for file_path in files @@ -170,14 +196,16 @@ def _raise_if_targeting_styles_conflict( 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.") + raise ConflictingScopeError("Choose positional files or auto-scope controls, not both.") def _resolve_positional_files(files: list[Path]) -> list[Path]: resolved = [file_path for file_path in files if not _is_ignored_review_path(file_path)] if resolved: return resolved - raise ValueError("No Python files to review were provided or detected from tracked or untracked changes.") + raise NoReviewableFilesError( + "No Python files to review were provided or detected from tracked or untracked changes." + ) def _resolve_auto_discovered_files( @@ -205,7 +233,7 @@ def _resolve_changed_scope_files(*, include_tests: bool, path_filters: list[Path 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( + raise NoReviewableFilesError( f"No reviewable files matched the selected auto-scope controls ({auto_scope_message}). " "Adjust --scope/--path or pass positional files." ) @@ -236,7 +264,7 @@ def _resolve_files( missing = [file_path for file_path in resolved if not file_path.is_file()] if missing: - raise ValueError(f"File not found: {missing[0]}") + raise NoReviewableFilesError(f"File not found: {missing[0]}") return resolved @@ -415,7 +443,7 @@ def _as_auto_scope(value: object) -> AutoScope | None: return None if isinstance(value, str) and value in {"changed", "full"}: return cast(AutoScope, value) - raise ValueError(f"Invalid scope value: {value!r}") + raise RunCommandError(f"Invalid scope value: {value!r}") def _as_path_filters(value: object) -> list[Path] | None: @@ -423,7 +451,7 @@ def _as_path_filters(value: object) -> list[Path] | None: return None if isinstance(value, list) and all(isinstance(path_filter, Path) for path_filter in value): return value - raise ValueError("Path filters must be a list of Path instances.") + raise RunCommandError("Path filters must be a list of Path instances.") def _as_optional_path(value: object) -> Path | None: @@ -431,7 +459,7 @@ def _as_optional_path(value: object) -> Path | None: return None if isinstance(value, Path): return value - raise ValueError("Output path must be a Path instance.") + raise RunCommandError("Output path must be a Path instance.") def _as_review_mode(value: object) -> ReviewRunMode: @@ -439,7 +467,7 @@ def _as_review_mode(value: object) -> ReviewRunMode: return "enforce" if value == "shadow": return "shadow" - raise ValueError(f"Invalid review mode: {value!r}") + raise RunCommandError(f"Invalid review mode: {value!r}") def _as_review_level(value: object) -> ReviewLevelFilter | None: @@ -447,7 +475,7 @@ def _as_review_level(value: object) -> ReviewLevelFilter | None: return None if value in ("error", "warning"): return cast(ReviewLevelFilter, value) - raise ValueError(f"Invalid review level: {value!r}") + raise RunCommandError(f"Invalid review level: {value!r}") def _as_focus_facets(value: object) -> tuple[str, ...]: @@ -456,9 +484,9 @@ def _as_focus_facets(value: object) -> tuple[str, ...]: if isinstance(value, (list, tuple)) and all(isinstance(item, str) for item in value): for item in value: if item not in ("source", "tests", "docs"): - raise ValueError(f"Invalid focus facet: {item!r}") + raise RunCommandError(f"Invalid focus facet: {item!r}") return tuple(value) - raise ValueError("focus_facets must be a list or tuple of strings") + raise RunCommandError("focus facets must be a list or tuple of strings") def _build_review_run_request( @@ -467,9 +495,9 @@ def _build_review_run_request( ) -> ReviewRunRequest: # Validate files is a list of Path instances if not isinstance(files, list): - raise ValueError(f"files must be a list, got {type(files).__name__}") + raise RunCommandError(f"files must be a list, got {type(files).__name__}") if not all(isinstance(file_path, Path) for file_path in files): - raise ValueError("files must contain only Path instances") + raise RunCommandError("files must contain only Path instances") request_kwargs = dict(kwargs) @@ -479,7 +507,7 @@ def _get_bool_param(name: str, default: bool = False) -> bool: if value is None: return default if not isinstance(value, bool): - raise ValueError(f"{name} must be a boolean, got {type(value).__name__}") + raise RunCommandError(f"{name} must be a boolean, got {type(value).__name__}") return value # Validate and extract known path/scope parameters @@ -494,7 +522,7 @@ def _get_optional_param(name: str, validator: Callable[[object], object], defaul include_tests = False # default value if include_tests_value is not None: if not isinstance(include_tests_value, bool): - raise ValueError(f"include_tests must be a boolean, got {type(include_tests_value).__name__}") + raise RunCommandError(f"include_tests must be a boolean, got {type(include_tests_value).__name__}") include_tests = include_tests_value # Get optional parameters with proper type casting @@ -509,7 +537,7 @@ def _get_optional_param(name: str, validator: Callable[[object], object], defaul focus_facets = cast(tuple[str, ...], _as_focus_facets(request_kwargs.pop("focus_facets", None))) if focus_facets and include_tests: - raise ValueError("Cannot combine focus_facets with include_tests; use --focus alone to scope files.") + raise FocusFacetConflictError("Cannot combine --focus with --include-tests or --exclude-tests") request = ReviewRunRequest( files=files, @@ -531,7 +559,7 @@ def _get_optional_param(name: str, validator: Callable[[object], object], defaul # Reject any unexpected keyword arguments if request_kwargs: unexpected = ", ".join(sorted(request_kwargs)) - raise ValueError(f"Unexpected keyword arguments: {unexpected}") + raise RunCommandError(f"Unexpected keyword arguments: {unexpected}") return request @@ -551,9 +579,9 @@ def _render_review_result(report: ReviewReport, request: ReviewRunRequest) -> tu def _validate_review_request(request: ReviewRunRequest) -> None: if request.json_output and request.score_only: - raise ValueError("Use either --json or --score-only, not both.") + raise InvalidOptionCombinationError("Use either --json or --score-only, not both.") if not request.json_output and request.out is not None: - raise ValueError("Use --out together with --json.") + raise MissingOutForJsonError("Use --out together with --json.") @beartype @@ -586,7 +614,7 @@ def run_command( ) resolved_files = _filter_files_by_focus(resolved_files, request.focus_facets) if not resolved_files: - raise ValueError( + raise NoReviewableFilesError( "No reviewable Python files matched the selected --focus facets." if request.focus_facets else "No Python files to review were provided or detected." @@ -604,4 +632,13 @@ def run_command( return _render_review_result(report, request) -__all__ = ["ReviewRunRequest", "run_command"] +__all__ = [ + "ConflictingScopeError", + "FocusFacetConflictError", + "InvalidOptionCombinationError", + "MissingOutForJsonError", + "NoReviewableFilesError", + "ReviewRunRequest", + "RunCommandError", + "run_command", +] diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py index d8062f75..b801e5fe 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py @@ -17,22 +17,55 @@ _CROSSHAIR_LINE_RE = re.compile(r"^(?P.+?):(?P\d+):\s*(?:error|warning|info):\s*(?P.+)$") _IGNORED_CROSSHAIR_PREFIXES = ("SideEffectDetected:",) +_ICONTRACT_SCAN_EXCLUDED_DIRS = frozenset( + {".git", ".mypy_cache", ".pytest_cache", ".ruff_cache", "__pycache__", "venv", ".venv"} +) -def _has_icontract_usage(files: list[Path]) -> bool: - """True when any reviewed file imports the icontract package.""" +def _icontract_usage_scan_roots(files: list[Path]) -> list[Path]: + roots: list[Path] = [] for file_path in files: - try: - tree = ast.parse(file_path.read_text(encoding="utf-8")) - except (OSError, UnicodeDecodeError, SyntaxError): - continue - for node in ast.walk(tree): - if isinstance(node, ast.ImportFrom) and node.module == "icontract": + parts = file_path.parts + if "packages" in parts: + package_index = parts.index("packages") + if len(parts) > package_index + 1: + roots.append(Path(*parts[: package_index + 2])) + continue + roots.append(file_path.parent) + return list(dict.fromkeys(roots)) + + +def _iter_icontract_usage_candidates(root: Path) -> list[Path]: + if not root.exists() or not root.is_dir(): + return [] + return [ + path + for path in root.rglob("*.py") + if path.is_file() and not any(part in _ICONTRACT_SCAN_EXCLUDED_DIRS for part in path.parts) + ] + + +def _has_icontract_usage(files: list[Path]) -> bool: + """True when any reviewed file's package/repo scan root imports the icontract package.""" + for root in _icontract_usage_scan_roots(files): + for file_path in _iter_icontract_usage_candidates(root): + if _file_imports_icontract(file_path): return True - if isinstance(node, ast.Import): - for alias in node.names: - if alias.name == "icontract": - return True + return False + + +def _file_imports_icontract(file_path: Path) -> bool: + try: + tree = ast.parse(file_path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, SyntaxError): + return False + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module == "icontract": + return True + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "icontract": + return True return False diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py index 44903000..8983e3f2 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py @@ -180,7 +180,25 @@ def _typer_cli_entrypoint_exempt(function_node: ast.FunctionDef | ast.AsyncFunct rendered = ast.unparse(ann) except AttributeError: return False - return rendered.endswith("Context") + return rendered.endswith("Context") and _has_typer_command_decorator(function_node) + + +def _decorator_name_parts(decorator: ast.expr) -> tuple[str, ...]: + if isinstance(decorator, ast.Call): + return _decorator_name_parts(decorator.func) + if isinstance(decorator, ast.Name): + return (decorator.id,) + if isinstance(decorator, ast.Attribute): + return (*_decorator_name_parts(decorator.value), decorator.attr) + return () + + +def _has_typer_command_decorator(function_node: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: + for decorator in function_node.decorator_list: + parts = _decorator_name_parts(decorator) + if parts == ("command",) or parts[-1:] == ("command",): + return True + return False def _kiss_parameter_findings( 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 52fd4083..67fdb70f 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 @@ -4,7 +4,6 @@ import json import os -import shutil import subprocess import tempfile from pathlib import Path @@ -238,7 +237,9 @@ def _category_for_rule(rule: str) -> SemgrepCategory | None: return None -def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> ReviewFinding | None: +def _extract_common_finding_fields( + item: dict[str, object], *, allowed_paths: set[str] +) -> tuple[str, str, int, str, dict[str, object]] | None: filename = item["path"] if not isinstance(filename, str): raise ValueError("semgrep filename must be a string") @@ -248,10 +249,6 @@ def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> 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): @@ -267,6 +264,19 @@ def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> if not isinstance(message, str): raise ValueError("semgrep message must be a string") + return filename, raw_rule, line, message, extra + + +def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> ReviewFinding | None: + extracted = _extract_common_finding_fields(item, allowed_paths=allowed_paths) + if extracted is None: + return None + filename, raw_rule, line, message, _extra = extracted + rule = _normalize_rule_id(raw_rule) + category = _category_for_rule(rule) + if category is None: + return None + return ReviewFinding( category=category, severity="warning", @@ -341,34 +351,15 @@ def _normalize_bug_rule_id(rule: str) -> str: def _finding_from_bug_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): + extracted = _extract_common_finding_fields(item, allowed_paths=allowed_paths) + if extracted is None: return None - - raw_rule = item["check_id"] - if not isinstance(raw_rule, str): - raise ValueError("semgrep rule must be a string") + filename, raw_rule, line, message, extra = extracted rule = _normalize_bug_rule_id(raw_rule) category = BUG_RULE_CATEGORY.get(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") - severity_raw = extra.get("severity", "WARNING") severity: Literal["error", "warning"] = ( "error" if isinstance(severity_raw, str) and severity_raw.upper() == "ERROR" else "warning" @@ -416,7 +407,8 @@ def run_semgrep_bugs(files: list[Path], *, bundle_root: Path | None = None) -> l if not files: return [] - if shutil.which("semgrep") is None: + skipped = skip_if_tool_missing("semgrep", files[0]) + if skipped: return [] config_path = find_semgrep_bugs_config(bundle_root=bundle_root) diff --git a/packages/specfact-codebase/module-package.yaml b/packages/specfact-codebase/module-package.yaml index dbb25435..43932374 100644 --- a/packages/specfact-codebase/module-package.yaml +++ b/packages/specfact-codebase/module-package.yaml @@ -1,5 +1,5 @@ name: nold-ai/specfact-codebase -version: 0.41.7 +version: 0.41.8 commands: - code tier: official @@ -24,5 +24,4 @@ description: Official SpecFact codebase bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:559af146c50a0971e8decf4ca14810e90f898b8c24a0051cd86dfd0190adc1c9 - signature: UPhlWxq4ikGSCWx2rfPLSEd/b5eGbqaJxUA7fHAQ4fsCmdeW4HZuPZ4hFgRQmBhg+Pk8WeFuMp9g/JdCGXJJDg== + checksum: sha256:4e9e9888eee7980bbf902c5b17bc2dba4b3e19debe23b08d2a4b5eee91a9f14e diff --git a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py index 96d7c6eb..d3abf46c 100644 --- a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py +++ b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py @@ -7,6 +7,7 @@ from __future__ import annotations +import os from abc import ABC, abstractmethod from collections.abc import Iterator from pathlib import Path @@ -38,19 +39,23 @@ class BaseFrameworkExtractor(ABC): ) @beartype - @staticmethod - def _path_touches_excluded_dir(path: Path) -> bool: + def _path_touches_excluded_dir(self, path: Path) -> bool: """True when any path component is an excluded dir (.specfact, venvs, VCS, caches, node_modules).""" - return any(part in BaseFrameworkExtractor._EXCLUDED_DIR_NAMES for part in path.parts) + return any(part in self._EXCLUDED_DIR_NAMES for part in path.parts) @beartype def _iter_python_files(self, root: Path) -> Iterator[Path]: """Yield ``*.py`` files under ``root``, skipping excluded directory subtrees by path.""" if not root.exists() or not root.is_dir(): return - for py_file in root.rglob("*.py"): - if not self._path_touches_excluded_dir(py_file): - yield py_file + for dirpath, dirnames, filenames in os.walk(root): + current_dir = Path(dirpath) + dirnames[:] = [ + dirname for dirname in dirnames if not self._path_touches_excluded_dir(current_dir / dirname) + ] + for filename in filenames: + if filename.endswith(".py"): + yield current_dir / filename @abstractmethod @beartype diff --git a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py index 46cc37fc..4e8e0392 100644 --- a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py +++ b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py @@ -20,38 +20,7 @@ _ROUTE_HTTP_METHODS = frozenset( {"get", "post", "put", "delete", "patch", "options", "head", "trace"}, ) - -_EXCLUDED_DIR_PARTS = frozenset( - { - ".specfact", - ".git", - "__pycache__", - "node_modules", - ".mypy_cache", - ".pytest_cache", - ".ruff_cache", - "venv", - ".venv", - }, -) - - -def _should_skip_path_for_fastapi_scan(path: Path, root: Path) -> bool: - """True when ``path`` lies under a directory we must not scan (venvs, caches, etc.).""" - try: - parts = path.resolve().relative_to(root.resolve()).parts - except ValueError: - return True - return any(part in _EXCLUDED_DIR_PARTS for part in parts) - - -def _iter_scan_python_files(search_path: Path): - """Yield ``*.py`` files under ``search_path``, skipping excluded directory trees.""" - root = search_path.resolve() - for path in search_path.rglob("*.py"): - if _should_skip_path_for_fastapi_scan(path, root): - continue - yield path +_FASTAPI_EXTRA_EXCLUDED_DIR_NAMES = frozenset({".mypy_cache", ".pytest_cache", ".ruff_cache"}) def _content_suggests_fastapi(content: str) -> bool: @@ -65,19 +34,15 @@ def _read_text_if_exists(path: Path) -> str | None: return None -def _scan_known_app_files(search_path: Path) -> bool: - for py_file in _iter_scan_python_files(search_path): - if py_file.name not in {"main.py", "app.py"}: - continue - content = _read_text_if_exists(py_file) - if content is not None and _content_suggests_fastapi(content): - return True - return False - - class FastAPIExtractor(BaseFrameworkExtractor): """FastAPI framework extractor.""" + @beartype + def _path_touches_excluded_dir(self, path: Path) -> bool: + return super()._path_touches_excluded_dir(path) or any( + part in _FASTAPI_EXTRA_EXCLUDED_DIR_NAMES for part in path.parts + ) + @beartype @require(lambda repo_path: repo_path.exists(), "Repository path must exist") @require(lambda repo_path: repo_path.is_dir(), "Repository path must be a directory") @@ -96,18 +61,28 @@ def detect(self, repo_path: Path) -> bool: file_path = repo_path / candidate_file if not file_path.exists(): continue - if _should_skip_path_for_fastapi_scan(file_path, repo_path.resolve()): + if self._path_touches_excluded_dir(file_path): continue content = _read_text_if_exists(file_path) if content is not None and _content_suggests_fastapi(content): return True for search_path in [repo_path, repo_path / "src", repo_path / "app", repo_path / "backend" / "app"]: - if search_path.exists() and _scan_known_app_files(search_path): + if search_path.exists() and self._scan_known_app_files(search_path): return True return False + @beartype + def _scan_known_app_files(self, search_path: Path) -> bool: + for py_file in self._iter_python_files(search_path): + if py_file.name not in {"main.py", "app.py"}: + continue + content = _read_text_if_exists(py_file) + if content is not None and _content_suggests_fastapi(content): + return True + return False + @beartype @require(lambda repo_path: repo_path.exists(), "Repository path must exist") @require(lambda repo_path: repo_path.is_dir(), "Repository path must be a directory") @@ -127,7 +102,7 @@ def extract_routes(self, repo_path: Path) -> list[RouteInfo]: for search_path in [repo_path, repo_path / "src", repo_path / "app", repo_path / "backend" / "app"]: if not search_path.exists(): continue - for py_file in _iter_scan_python_files(search_path): + for py_file in self._iter_python_files(search_path): try: routes = self._extract_routes_from_file(py_file) results.extend(routes) @@ -168,9 +143,7 @@ def _extract_routes_from_file(self, py_file: Path) -> list[RouteInfo]: for node in ast.walk(tree): if isinstance(node, ast.FunctionDef): - route_info = self._extract_route_from_function(node) - if route_info: - results.append(route_info) + results.extend(self._extract_routes_from_function(node)) return results @@ -200,12 +173,12 @@ def _path_method_from_route_call(self, decorator: ast.Call) -> tuple[str, str] | return None @beartype - def _path_method_from_api_route_call(self, decorator: ast.Call) -> tuple[str, str] | None: - """If ``decorator`` is ``@router.api_route(path, methods=[...])``, return first method + path.""" + def _path_methods_from_api_route_call(self, decorator: ast.Call) -> list[tuple[str, str]]: + """If ``decorator`` is ``@router.api_route(path, methods=[...])``, return all methods + path.""" if not isinstance(decorator.func, ast.Attribute): - return None + return [] if decorator.func.attr != "api_route": - return None + return [] path = "/" if decorator.args: path_arg = self._extract_string_literal(decorator.args[0]) @@ -221,39 +194,39 @@ def _path_method_from_api_route_call(self, decorator: ast.Call) -> tuple[str, st if lit: methods.append(lit.strip().upper()) if not methods: - return "GET", path - return methods[0], path + return [("GET", path)] + return [(method, path) for method in methods] @beartype - def _extract_route_from_function(self, func_node: ast.FunctionDef) -> RouteInfo | None: + def _extract_routes_from_function(self, func_node: ast.FunctionDef) -> list[RouteInfo]: """Extract route information from a function with FastAPI decorators.""" - matched = False - path = "/" - method = "GET" + matched_routes: list[tuple[str, str]] = [] for decorator in func_node.decorator_list: if not isinstance(decorator, ast.Call): continue got = self._path_method_from_route_call(decorator) - if got is None: - got = self._path_method_from_api_route_call(decorator) - if got is None: + if got is not None: + matched_routes.append(got) continue - matched = True - method, path = got + matched_routes.extend(self._path_methods_from_api_route_call(decorator)) - if not matched: - return None - - normalized_path, path_params = self._extract_path_parameters(path) + if not matched_routes: + return [] - return RouteInfo( - path=normalized_path, - method=method, - operation_id=func_node.name, - function=func_node.name, - path_params=path_params, - ) + results: list[RouteInfo] = [] + for method, path in matched_routes: + normalized_path, path_params = self._extract_path_parameters(path) + results.append( + RouteInfo( + path=normalized_path, + method=method, + operation_id=func_node.name, + function=func_node.name, + path_params=path_params, + ) + ) + return results @beartype def _extract_string_literal(self, node: ast.AST) -> str | None: diff --git a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py index cddb24c9..582dfd56 100644 --- a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py +++ b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py @@ -185,6 +185,7 @@ def _extract_route_from_function( """Extract route information from a function with Flask decorators.""" path = None methods = ["GET"] # Default method + _ = (imports, py_file) # Check decorators for route information for decorator in func_node.decorator_list: @@ -193,6 +194,7 @@ def _extract_route_from_function( isinstance(decorator, ast.Call) and isinstance(decorator.func, ast.Attribute) and decorator.func.attr == "route" + and self._is_owned_route_decorator(decorator.func, app_names, bp_names) ): # Extract path from first argument if decorator.args: @@ -226,6 +228,12 @@ def _extract_route_from_function( return results + @beartype + def _is_owned_route_decorator(self, func: ast.Attribute, app_names: set[str], bp_names: set[str]) -> bool: + if isinstance(func.value, ast.Name): + return func.value.id in app_names or func.value.id in bp_names + return False + @beartype def _extract_string_literal(self, node: ast.AST) -> str | None: """Extract string literal from AST node (Python 3.8+ uses ast.Constant).""" diff --git a/scripts/pre_commit_code_review.py b/scripts/pre_commit_code_review.py index aac752fc..20b909b3 100755 --- a/scripts/pre_commit_code_review.py +++ b/scripts/pre_commit_code_review.py @@ -208,33 +208,36 @@ def count_findings_by_severity(findings: list[object]) -> dict[str, int]: return buckets -def _print_review_findings_summary(repo_root: Path) -> tuple[bool, int | None]: - """Parse ``REVIEW_JSON_OUT``, print a one-line findings count, return ``(ok, error_count)``.""" +def _print_review_findings_summary(repo_root: Path) -> tuple[bool, int | None, int | None]: + """Parse ``REVIEW_JSON_OUT``, print counts, return ``(ok, error_count, ci_exit_code)``.""" report_path = _report_path(repo_root) if not report_path.is_file(): sys.stderr.write(f"Code review: no report file at {REVIEW_JSON_OUT} (could not print findings summary).\n") - return False, None + return False, None, None try: data = json.loads(report_path.read_text(encoding="utf-8")) except (OSError, UnicodeDecodeError) as exc: sys.stderr.write(f"Code review: could not read {REVIEW_JSON_OUT}: {exc}\n") - return False, None + return False, None, None except json.JSONDecodeError as exc: sys.stderr.write(f"Code review: invalid JSON in {REVIEW_JSON_OUT}: {exc}\n") - return False, None + return False, None, None if not isinstance(data, dict): sys.stderr.write(f"Code review: expected top-level JSON object in {REVIEW_JSON_OUT}.\n") - return False, None + return False, None, None findings_raw = data.get("findings") if not isinstance(findings_raw, list): sys.stderr.write(f"Code review: report has no findings list in {REVIEW_JSON_OUT}.\n") - return False, None + return False, None, None counts = count_findings_by_severity(findings_raw) total = len(findings_raw) verdict = data.get("overall_verdict", "?") + ci_exit_code = data.get("ci_exit_code") + if ci_exit_code not in {0, 1}: + ci_exit_code = 1 if verdict == "FAIL" else 0 parts = [ f"errors={counts['error']}", f"warnings={counts['warning']}", @@ -254,7 +257,7 @@ def _print_review_findings_summary(repo_root: Path) -> tuple[bool, int | None]: f" Read `{REVIEW_JSON_OUT}` and fix every finding (errors first), using file and line from each entry.\n" ) sys.stderr.write(f" @workspace Open `{REVIEW_JSON_OUT}` and remediate each item in `findings`.\n") - return True, counts["error"] + return True, counts["error"], ci_exit_code @ensure(lambda result: isinstance(result, tuple) and len(result) == 2) @@ -315,12 +318,12 @@ def main(argv: Sequence[str] | None = None) -> int: return _missing_report_exit_code(report_path, result) # Do not echo nested `specfact code review run` stdout/stderr (verbose tool banners); full report # is in REVIEW_JSON_OUT; we print a short summary on stderr below. - summary_ok, error_count = _print_review_findings_summary(repo_root) - if not summary_ok or error_count is None: + summary_ok, error_count, ci_exit_code = _print_review_findings_summary(repo_root) + if not summary_ok or error_count is None or ci_exit_code is None: return 1 if error_count == 0: return 0 - return result.returncode + return ci_exit_code if __name__ == "__main__": diff --git a/tests/cli-contracts/specfact-code-review-run.scenarios.yaml b/tests/cli-contracts/specfact-code-review-run.scenarios.yaml index 0b8c65ee..61777304 100644 --- a/tests/cli-contracts/specfact-code-review-run.scenarios.yaml +++ b/tests/cli-contracts/specfact-code-review-run.scenarios.yaml @@ -66,6 +66,18 @@ scenarios: exit_code: 0 stdout_contains: - review-report.json + - name: bug-hunt-shadow-dirty-exit-zero + type: pattern + argv: + - --json + - --bug-hunt + - --mode + - shadow + - tests/fixtures/review/dirty_module.py + expect: + exit_code: 0 + stdout_contains: + - review-report.json - name: mode-enforce-dirty-exit-nonzero type: pattern argv: @@ -77,6 +89,18 @@ scenarios: exit_code: 1 stdout_contains: - review-report.json + - name: bug-hunt-enforce-dirty-exit-nonzero + type: pattern + argv: + - --json + - --bug-hunt + - --mode + - enforce + - tests/fixtures/review/dirty_module.py + expect: + exit_code: 1 + stdout_contains: + - review-report.json - name: focus-source-and-docs-union type: pattern argv: @@ -84,6 +108,8 @@ scenarios: - full - --path - packages/specfact-code-review + - --path + - tests/unit/docs - --json - --focus - source @@ -92,32 +118,35 @@ scenarios: expect: exit_code: 0 stdout_contains: - - '"run_id":' + - packages/specfact-code-review/src/specfact_code_review + - tests/unit/docs - name: focus-tests-narrows-to-test-tree type: pattern argv: - --scope - full - --path - - packages/specfact-code-review + - tests/unit/specfact_code_review - --json + - --bug-hunt - --focus - tests expect: exit_code: 0 stdout_contains: - - '"run_id":' + - tests/unit/specfact_code_review - name: level-error-json-clean-module type: pattern argv: - --json + - --bug-hunt - --level - error - - tests/fixtures/review/clean_module.py + - tests/fixtures/review/dirty_module.py expect: - exit_code: 0 + exit_code: 1 stdout_contains: - - review-report.json + - '"severity":"error"' - name: focus-cannot-combine-with-include-tests type: anti-pattern argv: diff --git a/tests/unit/scripts/test_pre_commit_code_review.py b/tests/unit/scripts/test_pre_commit_code_review.py index a25ca5c5..dabb11d7 100644 --- a/tests/unit/scripts/test_pre_commit_code_review.py +++ b/tests/unit/scripts/test_pre_commit_code_review.py @@ -121,6 +121,7 @@ def test_main_warnings_only_does_not_block_despite_cli_fail_exit( repo_root = tmp_path payload: dict[str, object] = { "overall_verdict": "FAIL", + "ci_exit_code": 0, "findings": [{"severity": "warning", "rule": "w1"}], } _write_sample_review_report(repo_root, payload) @@ -188,6 +189,26 @@ def _fake_run(cmd: list[str], **_kwargs: object) -> subprocess.CompletedProcess[ assert "@workspace Open `.specfact/code-review.json`" in err +def test_main_uses_report_ci_exit_code_for_fixable_errors(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + module = _load_script_module() + repo_root = tmp_path + payload = { + "overall_verdict": "PASS_WITH_ADVISORY", + "ci_exit_code": 0, + "findings": [{"severity": "error", "rule": "fixable", "fixable": True}], + } + + def _fake_run(cmd: list[str], **_kwargs: object) -> subprocess.CompletedProcess[str]: + _write_sample_review_report(repo_root, payload) + return subprocess.CompletedProcess(cmd, 1, stdout=".specfact/code-review.json\n", stderr="") + + monkeypatch.setattr(module, "_repo_root", lambda: repo_root) + monkeypatch.setattr(module, "ensure_runtime_available", lambda: (True, None)) + monkeypatch.setattr(module.subprocess, "run", _fake_run) + + assert module.main(["tests/unit/test_app.py"]) == 0 + + def _write_sample_review_report(repo_root: Path, payload: dict[str, object]) -> None: spec_dir = repo_root / ".specfact" spec_dir.mkdir(parents=True, exist_ok=True) diff --git a/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py b/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py index db2e500f..22821079 100644 --- a/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py +++ b/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py @@ -12,7 +12,7 @@ def test_run_basedpyright_returns_empty_for_no_files() -> None: - assert not run_basedpyright([]) + assert run_basedpyright([]) == [] def test_run_basedpyright_skips_yaml_manifests(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: @@ -21,7 +21,7 @@ def test_run_basedpyright_skips_yaml_manifests(tmp_path: Path, monkeypatch: Monk run_mock = Mock() monkeypatch.setattr(subprocess, "run", run_mock) - assert not run_basedpyright([manifest]) + assert run_basedpyright([manifest]) == [] run_mock.assert_not_called() diff --git a/tests/unit/specfact_code_review/tools/test_contract_runner.py b/tests/unit/specfact_code_review/tools/test_contract_runner.py index 8c25bc9c..e407c042 100644 --- a/tests/unit/specfact_code_review/tools/test_contract_runner.py +++ b/tests/unit/specfact_code_review/tools/test_contract_runner.py @@ -28,8 +28,11 @@ def _which(name: str) -> str | None: monkeypatch.setattr("specfact_code_review.tools.tool_availability.shutil.which", _which) -def test_run_contract_check_skips_missing_icontract_when_package_unused(monkeypatch: MonkeyPatch) -> None: - file_path = FIXTURES_DIR / "public_without_contracts.py" +def test_run_contract_check_skips_missing_icontract_when_package_unused( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + file_path = tmp_path / "public_without_contracts.py" + file_path.write_text("def public_without_contracts(value: int) -> int:\n return value + 1\n", encoding="utf-8") run_mock = Mock(return_value=completed_process("crosshair", stdout="")) monkeypatch.setattr(subprocess, "run", run_mock) @@ -39,6 +42,15 @@ def test_run_contract_check_skips_missing_icontract_when_package_unused(monkeypa assert_tool_run(run_mock, ["crosshair", "check", "--per_path_timeout", "2", str(file_path)]) +def test_run_contract_check_uses_batch_level_icontract_detection(monkeypatch: MonkeyPatch) -> None: + file_path = FIXTURES_DIR / "public_without_contracts.py" + monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) + + findings = run_contract_check([file_path]) + + assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} + + def test_run_contract_check_skips_decorated_public_function(monkeypatch: MonkeyPatch) -> None: file_path = FIXTURES_DIR / "public_with_contracts.py" monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) diff --git a/tests/unit/specfact_code_review/tools/test_radon_runner.py b/tests/unit/specfact_code_review/tools/test_radon_runner.py index ce0c248b..5b1558b0 100644 --- a/tests/unit/specfact_code_review/tools/test_radon_runner.py +++ b/tests/unit/specfact_code_review/tools/test_radon_runner.py @@ -107,3 +107,46 @@ def test_run_radon_uses_dedicated_tool_identifier_for_kiss_findings(tmp_path: Pa kiss_findings = [finding for finding in findings if finding.rule.startswith("kiss.")] assert kiss_findings assert {finding.tool for finding in kiss_findings} == {"radon-kiss"} + + +def test_run_radon_requires_typer_decorator_for_context_parameter_exemption( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + file_path = tmp_path / "commands.py" + file_path.write_text( + """ +def callback(ctx: typer.Context, a: str, b: str, c: str, d: str, e: str) -> None: + return None +""", + encoding="utf-8", + ) + monkeypatch.setattr( + subprocess, + "run", + Mock(return_value=completed_process("radon", stdout=json.dumps({str(file_path): []}))), + ) + + findings = run_radon([file_path]) + + assert "kiss.parameter-count.warning" in {finding.rule for finding in findings} + + +def test_run_radon_exempts_typer_command_context_parameters(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "commands.py" + file_path.write_text( + """ +@app.command("run") +def callback(ctx: typer.Context, a: str, b: str, c: str, d: str, e: str) -> None: + return None +""", + encoding="utf-8", + ) + monkeypatch.setattr( + subprocess, + "run", + Mock(return_value=completed_process("radon", stdout=json.dumps({str(file_path): []}))), + ) + + findings = run_radon([file_path]) + + assert "kiss.parameter-count.warning" not in {finding.rule for finding in findings} 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 58c76a51..b2f12078 100644 --- a/tests/unit/specfact_code_review/tools/test_semgrep_runner.py +++ b/tests/unit/specfact_code_review/tools/test_semgrep_runner.py @@ -35,6 +35,18 @@ ] +@pytest.fixture(autouse=True) +def _stub_semgrep_on_path(monkeypatch: MonkeyPatch) -> None: # pyright: ignore[reportUnusedFunction] + real_which = shutil.which + + def _which(name: str) -> str | None: + if name == "semgrep": + return "/fake/semgrep" + return real_which(name) + + monkeypatch.setattr("specfact_code_review.tools.tool_availability.shutil.which", _which) + + def test_run_semgrep_maps_findings_to_review_finding(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" payload = { @@ -120,7 +132,8 @@ def test_run_semgrep_filters_findings_to_requested_files(tmp_path: Path, monkeyp def test_run_semgrep_returns_tool_error_when_semgrep_is_unavailable(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" - run_mock = Mock(side_effect=FileNotFoundError("semgrep not found")) + run_mock = Mock() + monkeypatch.setattr("specfact_code_review.tools.tool_availability.shutil.which", lambda _name: None) monkeypatch.setattr(subprocess, "run", run_mock) findings = run_semgrep([file_path]) @@ -128,7 +141,8 @@ def test_run_semgrep_returns_tool_error_when_semgrep_is_unavailable(tmp_path: Pa assert len(findings) == 1 assert findings[0].category == "tool_error" assert findings[0].tool == "semgrep" - assert findings[0].severity == "error" + assert findings[0].severity == "warning" + run_mock.assert_not_called() def test_run_semgrep_returns_empty_list_for_clean_file(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: @@ -218,7 +232,7 @@ def test_run_semgrep_bugs_returns_empty_when_semgrep_cli_missing(tmp_path: Path, (bundle / ".semgrep" / "bugs.yaml").write_text("rules: []\n", encoding="utf-8") target = tmp_path / "x.py" target.write_text("x = 1\n", encoding="utf-8") - monkeypatch.setattr(shutil, "which", lambda _name: None) + monkeypatch.setattr("specfact_code_review.tools.tool_availability.shutil.which", lambda _name: None) assert run_semgrep_bugs([target], bundle_root=bundle) == [] diff --git a/tests/unit/specfact_codebase/test_sidecar_framework_extractors.py b/tests/unit/specfact_codebase/test_sidecar_framework_extractors.py index 70f407bb..474b5b72 100644 --- a/tests/unit/specfact_codebase/test_sidecar_framework_extractors.py +++ b/tests/unit/specfact_codebase/test_sidecar_framework_extractors.py @@ -5,6 +5,7 @@ from pathlib import Path from specfact_codebase.validators.sidecar.frameworks.fastapi import FastAPIExtractor +from specfact_codebase.validators.sidecar.frameworks.flask import FlaskExtractor def _fake_fastapi_main() -> str: @@ -33,9 +34,26 @@ def multi(): ) extractor = FastAPIExtractor() routes = extractor.extract_routes(tmp_path) - match = next((r for r in routes if r.path == "/multi"), None) - assert match is not None - assert match.method == "GET" + methods = {route.method for route in routes if route.path == "/multi"} + assert methods == {"GET", "POST"} + + +def test_fastapi_extractor_preserves_multiple_route_decorators(tmp_path: Path) -> None: + (tmp_path / "routes.py").write_text( + """ +from fastapi import APIRouter +router = APIRouter() + +@router.get("/read") +@router.post("/write") +def multi(): + return {"ok": True} +""", + encoding="utf-8", + ) + extractor = FastAPIExtractor() + routes = extractor.extract_routes(tmp_path) + assert {(route.method, route.path) for route in routes} == {("GET", "/read"), ("POST", "/write")} def test_fastapi_extractor_ignores_non_http_decorators(tmp_path: Path) -> None: @@ -82,3 +100,30 @@ def ghost(): paths = {route.path for route in routes} assert "/real" in paths assert "/ghost-from-venv" not in paths + + +def test_flask_extractor_uses_registered_app_and_blueprint_symbols(tmp_path: Path) -> None: + (tmp_path / "app.py").write_text( + """ +from flask import Blueprint, Flask +app = Flask(__name__) +bp = Blueprint("api", __name__) + +@app.route("/app") +def app_route(): + return "ok" + +@bp.route("/bp", methods=["POST"]) +def bp_route(): + return "ok" + +@other.route("/ignored") +def unrelated(): + return "no" +""", + encoding="utf-8", + ) + + routes = FlaskExtractor().extract_routes(tmp_path) + + assert {(route.method, route.path) for route in routes} == {("GET", "/app"), ("POST", "/bp")} diff --git a/tests/unit/test_git_branch_module_signature_flag_script.py b/tests/unit/test_git_branch_module_signature_flag_script.py index 3b288e29..456bc9e2 100644 --- a/tests/unit/test_git_branch_module_signature_flag_script.py +++ b/tests/unit/test_git_branch_module_signature_flag_script.py @@ -1,5 +1,7 @@ from __future__ import annotations +import os +import subprocess from pathlib import Path @@ -13,3 +15,19 @@ def test_git_branch_module_signature_flag_script_documents_cli_parity() -> None: assert "GITHUB_BASE_REF" in text assert '"require"' in text assert "omit" in text + + +def test_git_branch_module_signature_flag_script_requires_for_main_base() -> None: + env = {**os.environ, "GITHUB_BASE_REF": "main"} + result = subprocess.run([SCRIPT_PATH], capture_output=True, text=True, check=False, env=env) + + assert result.returncode == 0 + assert result.stdout.strip() == "require" + + +def test_git_branch_module_signature_flag_script_omits_for_non_main_base() -> None: + env = {**os.environ, "GITHUB_BASE_REF": "feature/x"} + result = subprocess.run([SCRIPT_PATH], capture_output=True, text=True, check=False, env=env) + + assert result.returncode == 0 + assert result.stdout.strip() == "omit" diff --git a/tests/unit/workflows/test_pr_orchestrator_signing.py b/tests/unit/workflows/test_pr_orchestrator_signing.py index d3a00375..d4ae445f 100644 --- a/tests/unit/workflows/test_pr_orchestrator_signing.py +++ b/tests/unit/workflows/test_pr_orchestrator_signing.py @@ -26,7 +26,6 @@ def test_pr_orchestrator_verify_pr_to_dev_passes_integrity_shape_flag() -> None: workflow = _workflow_text() assert "--metadata-only" in workflow assert '[ "$TARGET_BRANCH" = "dev" ]' in workflow - assert "github.event.pull_request.head.repo.full_name" in workflow dev_guard = 'if [ "$TARGET_BRANCH" = "dev" ]; then' metadata_append = "VERIFY_CMD+=(--metadata-only)" assert dev_guard in workflow @@ -36,11 +35,14 @@ def test_pr_orchestrator_verify_pr_to_dev_passes_integrity_shape_flag() -> None: def test_pr_orchestrator_verify_require_signature_on_main_paths() -> None: workflow = _workflow_text() + main_pr_guard = 'elif [ "$TARGET_BRANCH" = "main" ]; then' main_ref_guard = '[ "${{ github.ref_name }}" = "main" ]; then' require_append = "VERIFY_CMD+=(--require-signature)" + assert main_pr_guard in workflow assert main_ref_guard in workflow assert require_append in workflow assert workflow.count(require_append) == 2 + assert "github.event.pull_request.head.repo.full_name" not in workflow push_require_block = ( 'if [ "${{ github.ref_name }}" = "main" ]; then\n VERIFY_CMD+=(--require-signature)' ) diff --git a/tests/unit/workflows/test_sign_modules_on_approval.py b/tests/unit/workflows/test_sign_modules_on_approval.py index bbd342e0..d7ea5c02 100644 --- a/tests/unit/workflows/test_sign_modules_on_approval.py +++ b/tests/unit/workflows/test_sign_modules_on_approval.py @@ -68,7 +68,9 @@ def _assert_eligibility_gate_step(doc: dict[Any, Any]) -> None: run = gate["run"] assert isinstance(run, str) assert "github.event.review.state" in run + assert "github.event.review.user.author_association" in run assert "approved" in run + assert "COLLABORATOR|MEMBER|OWNER" in run assert 'echo "sign=false"' in run assert 'echo "sign=true"' in run assert "github.event.pull_request.base.ref" in run From e971c8d4d222b7cfac914ea87ec897c2f5c0a15d Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Wed, 15 Apr 2026 09:58:11 +0200 Subject: [PATCH 2/4] fix: address PR 193 review follow-ups (icontract gate, CI hook, manifest) - Detect icontract usage across packages/ when reviewing a path under packages/ so MISSING_ICONTRACT is not suppressed when another bundle imports icontract. - Gate pr-orchestrator --require-signature on same-repo PRs to main; document in module-security reference; extend workflow contract test. - Pre-commit review hook exits strictly from report ci_exit_code; clarify summary helper docstring. - Bump specfact-code-review to 0.47.4 and refresh module checksum. Made-with: Cursor --- .github/workflows/pr-orchestrator.yml | 4 +- docs/reference/module-security.md | 2 +- .../specfact-code-review/module-package.yaml | 4 +- .../tools/contract_runner.py | 44 ++++++++++++++++--- scripts/pre_commit_code_review.py | 14 +++--- .../tools/test_contract_runner.py | 20 +++++++++ .../workflows/test_pr_orchestrator_signing.py | 5 ++- 7 files changed, 74 insertions(+), 19 deletions(-) diff --git a/.github/workflows/pr-orchestrator.yml b/.github/workflows/pr-orchestrator.yml index 80bbcbe3..f7c9331c 100644 --- a/.github/workflows/pr-orchestrator.yml +++ b/.github/workflows/pr-orchestrator.yml @@ -89,10 +89,12 @@ jobs: if [ "${{ github.event_name }}" = "pull_request" ]; then BASE_REF="origin/${{ github.event.pull_request.base.ref }}" TARGET_BRANCH="${{ github.event.pull_request.base.ref }}" + HEAD_REPO="${{ github.event.pull_request.head.repo.full_name }}" + THIS_REPO="${{ github.repository }}" VERIFY_CMD+=(--version-check-base "$BASE_REF") if [ "$TARGET_BRANCH" = "dev" ]; then VERIFY_CMD+=(--metadata-only) - elif [ "$TARGET_BRANCH" = "main" ]; then + elif [ "$TARGET_BRANCH" = "main" ] && [ "$HEAD_REPO" = "$THIS_REPO" ]; then VERIFY_CMD+=(--require-signature) fi else diff --git a/docs/reference/module-security.md b/docs/reference/module-security.md index 89008203..580df390 100644 --- a/docs/reference/module-security.md +++ b/docs/reference/module-security.md @@ -48,7 +48,7 @@ Module packages carry **publisher** and **integrity** metadata so installation, - **Verification command** (`scripts/verify-modules-signature.py`): - **Baseline (CI)**: `--payload-from-filesystem --enforce-version-bump` — full payload checksum verification plus version-bump enforcement. - **Dev-target PR mode**: `.github/workflows/pr-orchestrator.yml` appends `--metadata-only` for pull requests targeting `dev` so branch work is not blocked before approval-time signing refreshes manifests. - - **Strict mode**: add `--require-signature` so every manifest must include a verifiable `integrity.signature`. In `.github/workflows/pr-orchestrator.yml` this is appended for **pull requests whose base is `main`** and for **pushes to `main`**, in addition to the baseline flags. Locally, `scripts/pre-commit-verify-modules-signature.sh` adds `--require-signature` only when the checkout (or `GITHUB_BASE_REF` in Actions) is **`main`**. + - **Strict mode**: add `--require-signature` so every manifest must include a verifiable `integrity.signature`. In `.github/workflows/pr-orchestrator.yml` this is appended for **same-repository pull requests whose base is `main`** (fork heads skip strict signature enforcement here) and for **pushes to `main`**, in addition to the baseline flags. Locally, `scripts/pre-commit-verify-modules-signature.sh` adds `--require-signature` only when the checkout (or `GITHUB_BASE_REF` in Actions) is **`main`**. - **Local non-main hook mode**: `scripts/pre-commit-verify-modules-signature.sh` otherwise keeps the baseline command shape but adds `--metadata-only`, avoiding local checksum/signature enforcement on branches that are expected to be signed by CI. - **Pull request CI** also passes `--version-check-base ` (typically `origin/`) so version rules compare against the PR base. - **CI uses the full verifier** for `main` and push checks (payload digest + rules above), while PRs targeting `dev` intentionally pass `--metadata-only`. The script still supports `--metadata-only` for optional tooling that only needs manifest shape and checksum format checks. diff --git a/packages/specfact-code-review/module-package.yaml b/packages/specfact-code-review/module-package.yaml index 262f01a0..7aea49ef 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.47.3 +version: 0.47.4 commands: - code tier: official @@ -23,4 +23,4 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:582d07cb2941568056a35246db3dbb8ad612362a95bc84080d400f082bb6df80 + checksum: sha256:401e49529bd55941722117227b07ba0a992af2e8c66ded07b39fa0ef1f88e561 diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py index b801e5fe..2906fd6c 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py @@ -45,13 +45,43 @@ def _iter_icontract_usage_candidates(root: Path) -> list[Path]: ] -def _has_icontract_usage(files: list[Path]) -> bool: - """True when any reviewed file's package/repo scan root imports the icontract package.""" - for root in _icontract_usage_scan_roots(files): - for file_path in _iter_icontract_usage_candidates(root): - if _file_imports_icontract(file_path): - return True - return False +def _repo_root_from_review_paths(files: list[Path]) -> Path | None: + """Locate the git repository root from any reviewed path (``.git`` file or directory).""" + for file_path in files: + try: + resolved = file_path.resolve() + except OSError: + continue + for parent in [resolved, *resolved.parents]: + if (parent / ".git").exists(): + return parent + return None + + +def _root_imports_icontract(root: Path) -> bool: + """True when any Python file under ``root`` imports the ``icontract`` package.""" + return any(_file_imports_icontract(file_path) for file_path in _iter_icontract_usage_candidates(root)) + + +def _has_icontract_usage(py_files: list[Path]) -> bool: + """True when icontract is used in any per-path scan root or elsewhere under ``packages/``. + + Review runs often pass only the changed file under ``packages//…``. Icontract may live + in a sibling module under the same ``packages/`` tree; a per-bundle root scan alone would miss + that signal and incorrectly skip ``MISSING_ICONTRACT`` for the edited file. + """ + for root in dict.fromkeys(_icontract_usage_scan_roots(py_files)): + if _root_imports_icontract(root): + return True + repo_root = _repo_root_from_review_paths(py_files) + if repo_root is None: + return False + if not any("packages" in path.parts for path in py_files): + return False + bundled = repo_root / "packages" + if not bundled.is_dir(): + return False + return _root_imports_icontract(bundled) def _file_imports_icontract(file_path: Path) -> bool: diff --git a/scripts/pre_commit_code_review.py b/scripts/pre_commit_code_review.py index 20b909b3..0d69441e 100755 --- a/scripts/pre_commit_code_review.py +++ b/scripts/pre_commit_code_review.py @@ -209,7 +209,11 @@ def count_findings_by_severity(findings: list[object]) -> dict[str, int]: def _print_review_findings_summary(repo_root: Path) -> tuple[bool, int | None, int | None]: - """Parse ``REVIEW_JSON_OUT``, print counts, return ``(ok, error_count, ci_exit_code)``.""" + """Parse ``REVIEW_JSON_OUT``, print counts, return ``(ok, error_count, ci_exit_code)``. + + Callers should use ``ci_exit_code`` as the hook exit code; ``error_count`` is informational only + because fixable error-severity findings may still yield a passing ``ci_exit_code``. + """ report_path = _report_path(repo_root) if not report_path.is_file(): sys.stderr.write(f"Code review: no report file at {REVIEW_JSON_OUT} (could not print findings summary).\n") @@ -318,12 +322,10 @@ def main(argv: Sequence[str] | None = None) -> int: return _missing_report_exit_code(report_path, result) # Do not echo nested `specfact code review run` stdout/stderr (verbose tool banners); full report # is in REVIEW_JSON_OUT; we print a short summary on stderr below. - summary_ok, error_count, ci_exit_code = _print_review_findings_summary(repo_root) - if not summary_ok or error_count is None or ci_exit_code is None: + summary_ok, _error_count, ci_exit_code = _print_review_findings_summary(repo_root) + if not summary_ok or ci_exit_code is None: return 1 - if error_count == 0: - return 0 - return ci_exit_code + return int(ci_exit_code) if __name__ == "__main__": diff --git a/tests/unit/specfact_code_review/tools/test_contract_runner.py b/tests/unit/specfact_code_review/tools/test_contract_runner.py index e407c042..25bcbaf7 100644 --- a/tests/unit/specfact_code_review/tools/test_contract_runner.py +++ b/tests/unit/specfact_code_review/tools/test_contract_runner.py @@ -51,6 +51,26 @@ def test_run_contract_check_uses_batch_level_icontract_detection(monkeypatch: Mo assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} +def test_run_contract_check_detects_icontract_across_package_bundles(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + """MISSING_ICONTRACT must not depend only on the edited bundle: scan ``packages/`` when needed.""" + (tmp_path / ".git").mkdir() + pkg_a = tmp_path / "packages" / "pkg_a" + pkg_b = tmp_path / "packages" / "pkg_b" + pkg_a.mkdir(parents=True) + pkg_b.mkdir(parents=True) + (pkg_b / "icontract_anchor.py").write_text("import icontract\n", encoding="utf-8") + edited = pkg_a / "new_public_api.py" + edited.write_text( + "def public_no_contracts(value: int) -> int:\n return value + 1\n", + encoding="utf-8", + ) + monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) + + findings = run_contract_check([edited]) + + assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} + + def test_run_contract_check_skips_decorated_public_function(monkeypatch: MonkeyPatch) -> None: file_path = FIXTURES_DIR / "public_with_contracts.py" monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) diff --git a/tests/unit/workflows/test_pr_orchestrator_signing.py b/tests/unit/workflows/test_pr_orchestrator_signing.py index d4ae445f..ada52c8d 100644 --- a/tests/unit/workflows/test_pr_orchestrator_signing.py +++ b/tests/unit/workflows/test_pr_orchestrator_signing.py @@ -35,14 +35,15 @@ def test_pr_orchestrator_verify_pr_to_dev_passes_integrity_shape_flag() -> None: def test_pr_orchestrator_verify_require_signature_on_main_paths() -> None: workflow = _workflow_text() - main_pr_guard = 'elif [ "$TARGET_BRANCH" = "main" ]; then' + main_pr_guard = 'elif [ "$TARGET_BRANCH" = "main" ] && [ "$HEAD_REPO" = "$THIS_REPO" ]; then' main_ref_guard = '[ "${{ github.ref_name }}" = "main" ]; then' require_append = "VERIFY_CMD+=(--require-signature)" assert main_pr_guard in workflow assert main_ref_guard in workflow assert require_append in workflow assert workflow.count(require_append) == 2 - assert "github.event.pull_request.head.repo.full_name" not in workflow + assert 'HEAD_REPO="${{ github.event.pull_request.head.repo.full_name }}"' in workflow + assert 'THIS_REPO="${{ github.repository }}"' in workflow push_require_block = ( 'if [ "${{ github.ref_name }}" = "main" ]; then\n VERIFY_CMD+=(--require-signature)' ) From d06048a5f1d2f3857492091e86524d025b9f50f7 Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Wed, 15 Apr 2026 10:21:09 +0200 Subject: [PATCH 3/4] fix(code-review): repo-relative package roots and stub icontract scans - Resolve icontract scan bundle roots from paths relative to the git repo root so absolute path spellings cannot mis-detect a packages segment; mirror that logic for the monorepo packages/ fallback gate. - Include .pyi files when scanning for icontract imports. - Make batch-level MISSING_ICONTRACT test self-contained under tmp_path. - Bump specfact-code-review to 0.47.5 and refresh manifest checksum. Made-with: Cursor --- .../specfact-code-review/module-package.yaml | 4 +- .../tools/contract_runner.py | 111 ++++++++++++++---- .../tools/test_contract_runner.py | 15 ++- 3 files changed, 103 insertions(+), 27 deletions(-) diff --git a/packages/specfact-code-review/module-package.yaml b/packages/specfact-code-review/module-package.yaml index 7aea49ef..3ecb2ec0 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.47.4 +version: 0.47.5 commands: - code tier: official @@ -23,4 +23,4 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:401e49529bd55941722117227b07ba0a992af2e8c66ded07b39fa0ef1f88e561 + checksum: sha256:939e80621b256d1ecb16da1da717ba40444ff693218028af5638ee7d8bfa9fc2 diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py index 2906fd6c..eefa9d05 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py @@ -22,44 +22,111 @@ ) +def _repo_root_from_review_paths(files: list[Path]) -> Path | None: + """Locate the git repository root from any reviewed path (``.git`` file or directory).""" + for file_path in files: + try: + resolved = file_path.resolve() + except OSError: + continue + for parent in [resolved, *resolved.parents]: + if (parent / ".git").exists(): + return parent + return None + + def _icontract_usage_scan_roots(files: list[Path]) -> list[Path]: + """Bundle scan roots for icontract discovery, using paths relative to the repo root when known.""" roots: list[Path] = [] + repo_root = _repo_root_from_review_paths(files) + repo_resolved: Path | None = None + if repo_root is not None: + try: + repo_resolved = repo_root.resolve() + except OSError: + repo_resolved = None + for file_path in files: - parts = file_path.parts - if "packages" in parts: - package_index = parts.index("packages") - if len(parts) > package_index + 1: - roots.append(Path(*parts[: package_index + 2])) + rel_parts: tuple[str, ...] + if repo_resolved is not None: + try: + rel_parts = file_path.resolve().relative_to(repo_resolved).parts + except (OSError, ValueError): + try: + rel_parts = file_path.resolve().parts + except OSError: + rel_parts = file_path.parts + else: + try: + rel_parts = file_path.resolve().parts + except OSError: + rel_parts = file_path.parts + + if "packages" in rel_parts: + package_index = rel_parts.index("packages") + if len(rel_parts) > package_index + 1: + if repo_resolved is not None: + roots.append(repo_resolved / "packages" / rel_parts[package_index + 1]) + else: + roots.append(Path(*rel_parts[: package_index + 2])) continue - roots.append(file_path.parent) + try: + roots.append(file_path.resolve().parent) + except OSError: + roots.append(file_path.parent) + return list(dict.fromkeys(roots)) def _iter_icontract_usage_candidates(root: Path) -> list[Path]: if not root.exists() or not root.is_dir(): return [] - return [ - path - for path in root.rglob("*.py") - if path.is_file() and not any(part in _ICONTRACT_SCAN_EXCLUDED_DIRS for part in path.parts) - ] + collected: list[Path] = [] + for path in root.rglob("*"): + if not path.is_file(): + continue + if path.suffix not in {".py", ".pyi"}: + continue + if any(part in _ICONTRACT_SCAN_EXCLUDED_DIRS for part in path.parts): + continue + collected.append(path) + return sorted(collected, key=lambda candidate: candidate.as_posix()) -def _repo_root_from_review_paths(files: list[Path]) -> Path | None: - """Locate the git repository root from any reviewed path (``.git`` file or directory).""" - for file_path in files: +def _review_paths_include_repo_packages_tree(py_files: list[Path]) -> bool: + """True when any reviewed path maps under ``/packages/…`` using repo-relative segments.""" + repo_root = _repo_root_from_review_paths(py_files) + if repo_root is None: + for path in py_files: + try: + if "packages" in path.resolve().parts: + return True + except OSError: + if "packages" in path.parts: + return True + return False + try: + repo_resolved = repo_root.resolve() + except OSError: + for path in py_files: + try: + if "packages" in path.resolve().parts: + return True + except OSError: + continue + return False + for path in py_files: try: - resolved = file_path.resolve() - except OSError: + rel = path.resolve().relative_to(repo_resolved) + except (OSError, ValueError): continue - for parent in [resolved, *resolved.parents]: - if (parent / ".git").exists(): - return parent - return None + if "packages" in rel.parts: + return True + return False def _root_imports_icontract(root: Path) -> bool: - """True when any Python file under ``root`` imports the ``icontract`` package.""" + """True when any ``.py`` / ``.pyi`` file under ``root`` imports the ``icontract`` package.""" return any(_file_imports_icontract(file_path) for file_path in _iter_icontract_usage_candidates(root)) @@ -76,7 +143,7 @@ def _has_icontract_usage(py_files: list[Path]) -> bool: repo_root = _repo_root_from_review_paths(py_files) if repo_root is None: return False - if not any("packages" in path.parts for path in py_files): + if not _review_paths_include_repo_packages_tree(py_files): return False bundled = repo_root / "packages" if not bundled.is_dir(): diff --git a/tests/unit/specfact_code_review/tools/test_contract_runner.py b/tests/unit/specfact_code_review/tools/test_contract_runner.py index 25bcbaf7..6f45e2e1 100644 --- a/tests/unit/specfact_code_review/tools/test_contract_runner.py +++ b/tests/unit/specfact_code_review/tools/test_contract_runner.py @@ -42,11 +42,20 @@ def test_run_contract_check_skips_missing_icontract_when_package_unused( assert_tool_run(run_mock, ["crosshair", "check", "--per_path_timeout", "2", str(file_path)]) -def test_run_contract_check_uses_batch_level_icontract_detection(monkeypatch: MonkeyPatch) -> None: - file_path = FIXTURES_DIR / "public_without_contracts.py" +def test_run_contract_check_uses_batch_level_icontract_detection(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + """Icontract usage in a sibling module under the same bundle roots ``MISSING_ICONTRACT`` on the edited file.""" + (tmp_path / ".git").mkdir() + pkg = tmp_path / "packages" / "demo_pkg" + pkg.mkdir(parents=True) + (pkg / "uses_icontract.py").write_text("import icontract\n", encoding="utf-8") + tmp_file = pkg / "public_without_contracts.py" + tmp_file.write_text( + "def public_without_contracts(value: int) -> int:\n return value + 1\n", + encoding="utf-8", + ) monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) - findings = run_contract_check([file_path]) + findings = run_contract_check([tmp_file]) assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} From 71cabbe067f4f6e6b01cc70d7a3742f486db39f0 Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Wed, 15 Apr 2026 10:33:27 +0200 Subject: [PATCH 4/4] fix(code-review): strict repo-relative packages/ roots and .pyi anchor test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Treat bundle layout only when path is repo-relative packages//…, not when "packages" appears deeper in rel_parts; keep permissive fallback when no git root is found. - Add cross-bundle contract test with icontract_anchor.pyi. - Bump specfact-code-review to 0.47.6 and refresh manifest checksum. Made-with: Cursor --- .../specfact-code-review/module-package.yaml | 4 +-- .../tools/contract_runner.py | 31 +++++++++---------- .../tools/test_contract_runner.py | 22 +++++++++++++ 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/packages/specfact-code-review/module-package.yaml b/packages/specfact-code-review/module-package.yaml index 3ecb2ec0..9033f8d5 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.47.5 +version: 0.47.6 commands: - code tier: official @@ -23,4 +23,4 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:939e80621b256d1ecb16da1da717ba40444ff693218028af5638ee7d8bfa9fc2 + checksum: sha256:d48dfce318a75ea66d9a2bb2b69c7ed0c29e1228732b138719555a470e9fc47b diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py index eefa9d05..e6bed716 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py @@ -47,28 +47,21 @@ def _icontract_usage_scan_roots(files: list[Path]) -> list[Path]: repo_resolved = None for file_path in files: - rel_parts: tuple[str, ...] if repo_resolved is not None: try: rel_parts = file_path.resolve().relative_to(repo_resolved).parts except (OSError, ValueError): - try: - rel_parts = file_path.resolve().parts - except OSError: - rel_parts = file_path.parts + rel_parts = () + if rel_parts and rel_parts[0] == "packages" and len(rel_parts) > 1: + roots.append(repo_resolved / "packages" / rel_parts[1]) + continue else: try: - rel_parts = file_path.resolve().parts + abs_parts = file_path.resolve().parts except OSError: - rel_parts = file_path.parts - - if "packages" in rel_parts: - package_index = rel_parts.index("packages") - if len(rel_parts) > package_index + 1: - if repo_resolved is not None: - roots.append(repo_resolved / "packages" / rel_parts[package_index + 1]) - else: - roots.append(Path(*rel_parts[: package_index + 2])) + abs_parts = file_path.parts + if abs_parts and abs_parts[0] == "packages" and len(abs_parts) > 1: + roots.append(Path(*abs_parts[:2])) continue try: roots.append(file_path.resolve().parent) @@ -94,7 +87,11 @@ def _iter_icontract_usage_candidates(root: Path) -> list[Path]: def _review_paths_include_repo_packages_tree(py_files: list[Path]) -> bool: - """True when any reviewed path maps under ``/packages/…`` using repo-relative segments.""" + """True when any reviewed path lies under ``/packages/…`` (strict repo-relative prefix). + + Without a discoverable git root, fall back to ``\"packages\" in resolved.parts`` so callers + outside a normal repo layout still participate in the monorepo ``packages/`` scan when appropriate. + """ repo_root = _repo_root_from_review_paths(py_files) if repo_root is None: for path in py_files: @@ -120,7 +117,7 @@ def _review_paths_include_repo_packages_tree(py_files: list[Path]) -> bool: rel = path.resolve().relative_to(repo_resolved) except (OSError, ValueError): continue - if "packages" in rel.parts: + if rel.parts and rel.parts[0] == "packages": return True return False diff --git a/tests/unit/specfact_code_review/tools/test_contract_runner.py b/tests/unit/specfact_code_review/tools/test_contract_runner.py index 6f45e2e1..3cc55a4d 100644 --- a/tests/unit/specfact_code_review/tools/test_contract_runner.py +++ b/tests/unit/specfact_code_review/tools/test_contract_runner.py @@ -80,6 +80,28 @@ def test_run_contract_check_detects_icontract_across_package_bundles(tmp_path: P assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} +def test_run_contract_check_detects_icontract_anchor_in_stub_across_package_bundles( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """Cross-bundle icontract discovery must observe ``import icontract`` in a ``.pyi`` stub.""" + (tmp_path / ".git").mkdir() + pkg_a = tmp_path / "packages" / "pkg_a" + pkg_b = tmp_path / "packages" / "pkg_b" + pkg_a.mkdir(parents=True) + pkg_b.mkdir(parents=True) + (pkg_b / "icontract_anchor.pyi").write_text("import icontract\n", encoding="utf-8") + edited = pkg_a / "new_public_api.py" + edited.write_text( + "def public_no_contracts(value: int) -> int:\n return value + 1\n", + encoding="utf-8", + ) + monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) + + findings = run_contract_check([edited]) + + assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} + + def test_run_contract_check_skips_decorated_public_function(monkeypatch: MonkeyPatch) -> None: file_path = FIXTURES_DIR / "public_with_contracts.py" monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout="")))