diff --git a/.github/workflows/pr-orchestrator.yml b/.github/workflows/pr-orchestrator.yml index a8638d9..2b38717 100644 --- a/.github/workflows/pr-orchestrator.yml +++ b/.github/workflows/pr-orchestrator.yml @@ -80,29 +80,28 @@ jobs: - name: Verify bundled module signatures and version bumps run: | set -euo pipefail - TARGET_BRANCH="" - if [ "${{ github.event_name }}" = "pull_request" ]; then - TARGET_BRANCH="${{ github.event.pull_request.base.ref }}" - else - TARGET_BRANCH="${GITHUB_REF#refs/heads/}" - fi - - BASE_REF="" - if [ "${{ github.event_name }}" = "pull_request" ]; then - BASE_REF="origin/${{ github.event.pull_request.base.ref }}" - fi - if [ -z "${SPECFACT_MODULE_PUBLIC_SIGN_KEY:-}" ] && [ -z "${SPECFACT_MODULE_SIGNING_PUBLIC_KEY_PEM:-}" ]; then echo "warning: no public signing key secret set; verifier must resolve key from repo/default path" fi VERIFY_CMD=(python scripts/verify-modules-signature.py --payload-from-filesystem --enforce-version-bump) - if [ "$TARGET_BRANCH" = "main" ]; then - VERIFY_CMD+=(--require-signature) - fi - if [ -n "$BASE_REF" ]; then + + if [ "${{ github.event_name }}" = "pull_request" ]; then + BASE_REF="origin/${{ github.event.pull_request.base.ref }}" + TARGET_BRANCH="${{ github.event.pull_request.base.ref }}" 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 + VERIFY_CMD+=(--require-signature) + fi + else + if [ "${{ github.ref_name }}" = "main" ]; then + VERIFY_CMD+=(--require-signature) + fi fi + "${VERIFY_CMD[@]}" quality: diff --git a/.github/workflows/sign-modules-on-approval.yml b/.github/workflows/sign-modules-on-approval.yml index c6d71bb..02ef3a2 100644 --- a/.github/workflows/sign-modules-on-approval.yml +++ b/.github/workflows/sign-modules-on-approval.yml @@ -14,10 +14,6 @@ permissions: jobs: sign-modules: - if: >- - github.event.review.state == 'approved' && - (github.event.pull_request.base.ref == 'dev' || github.event.pull_request.base.ref == 'main') && - github.event.pull_request.head.repo.full_name == github.repository runs-on: ubuntu-latest env: SPECFACT_MODULE_PRIVATE_SIGN_KEY: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY }} @@ -25,7 +21,33 @@ jobs: PR_BASE_REF: ${{ github.event.pull_request.base.ref }} PR_HEAD_REF: ${{ github.event.pull_request.head.ref }} steps: + - name: Eligibility gate (required status check) + id: gate + run: | + set -euo pipefail + if [ "${{ github.event.review.state }}" != "approved" ]; then + echo "sign=false" >> "$GITHUB_OUTPUT" + echo "::notice::Skipping module signing: review state is not approved." + exit 0 + fi + base_ref="${{ github.event.pull_request.base.ref }}" + if [ "$base_ref" != "dev" ] && [ "$base_ref" != "main" ]; then + echo "sign=false" >> "$GITHUB_OUTPUT" + echo "::notice::Skipping module signing: base branch is not dev or main." + exit 0 + fi + head_repo="${{ github.event.pull_request.head.repo.full_name }}" + this_repo="${{ github.repository }}" + if [ "$head_repo" != "$this_repo" ]; then + echo "sign=false" >> "$GITHUB_OUTPUT" + echo "::notice::Skipping module signing: fork PR (head repo differs from target repo)." + exit 0 + fi + echo "sign=true" >> "$GITHUB_OUTPUT" + echo "Eligible for module signing (approved, same-repo PR to dev or main)." + - name: Guard signing secrets + if: steps.gate.outputs.sign == 'true' run: | set -euo pipefail if [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ]; then @@ -38,21 +60,25 @@ jobs: fi - uses: actions/checkout@v4 + if: steps.gate.outputs.sign == 'true' with: ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 0 - name: Set up Python 3.12 + if: steps.gate.outputs.sign == 'true' uses: actions/setup-python@v5 with: python-version: "3.12" - name: Install signing dependencies + if: steps.gate.outputs.sign == 'true' run: | python -m pip install --upgrade pip python -m pip install pyyaml beartype icontract cryptography cffi - name: Discover module manifests + if: steps.gate.outputs.sign == 'true' id: discover run: | set -euo pipefail @@ -61,6 +87,7 @@ jobs: echo "Discovered ${#MANIFESTS[@]} module-package.yaml file(s) under packages/" - name: Sign changed module manifests + if: steps.gate.outputs.sign == 'true' id: sign run: | set -euo pipefail @@ -73,6 +100,7 @@ jobs: --payload-from-filesystem - name: Commit and push signed manifests + if: steps.gate.outputs.sign == 'true' id: commit run: | set -euo pipefail @@ -94,15 +122,20 @@ jobs: - name: Write job summary if: always() env: - COMMIT_CHANGED: ${{ steps.commit.outputs.changed }} - MANIFESTS_COUNT: ${{ steps.discover.outputs.manifests_count }} + GATE_SIGN: ${{ steps.gate.outputs.sign }} + COMMIT_CHANGED: ${{ steps.commit.outputs.changed || '' }} + MANIFESTS_COUNT: ${{ steps.discover.outputs.manifests_count || '' }} run: | { echo "### Module signing (CI approval)" - echo "Manifests discovered under \`packages/\`: ${MANIFESTS_COUNT:-unknown}" - if [ "${COMMIT_CHANGED}" = "true" ]; then - echo "Committed signed manifest updates to ${PR_HEAD_REF}." + if [ "${GATE_SIGN}" != "true" ]; then + echo "Signing skipped (eligibility gate: not approved, wrong base branch, or fork PR)." else - echo "No changes detected (manifests already signed or no module changes on this PR vs merge-base)." + echo "Manifests discovered under \`packages/\`: ${MANIFESTS_COUNT:-unknown}" + if [ "${COMMIT_CHANGED}" = "true" ]; then + echo "Committed signed manifest updates to ${PR_HEAD_REF}." + else + echo "No changes detected (manifests already signed or no module changes on this PR vs merge-base)." + fi fi } >> "$GITHUB_STEP_SUMMARY" diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9281007..3fee305 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -19,10 +19,12 @@ repos: pass_filenames: false always_run: true verbose: true + # pass_filenames: false — same chunking issue as lint; script runs repo-wide yaml-lint once. - id: modules-block1-yaml name: "Block 1 — stage 2/4 — yaml-lint (when YAML staged)" entry: ./scripts/pre-commit-quality-checks.sh block1-yaml language: system + pass_filenames: false files: \.(yaml|yml)$ verbose: true - id: modules-block1-bundle @@ -32,10 +34,13 @@ repos: pass_filenames: false always_run: true verbose: true + # pass_filenames: false — otherwise pre-commit re-invokes this hook per filename chunk (ARG_MAX), + # and each run still executes full-repo `hatch run lint` (wasteful duplicate output). - id: modules-block1-lint name: "Block 1 — stage 4/4 — lint (when Python staged)" entry: ./scripts/pre-commit-quality-checks.sh block1-lint language: system + pass_filenames: false files: \.(py|pyi)$ verbose: true - id: modules-block2 diff --git a/CHANGELOG.md b/CHANGELOG.md index f461b5a..c527f84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,13 @@ and this project follows SemVer for bundle versions. - Refresh the canonical `specfact-code-review` house-rules skill to a compact clean-code charter and bump the bundle metadata for the signed 0.45.1 release. +- Document CI module verification: **`pr-orchestrator`** PR checks run + `verify-modules-signature` with **`--payload-from-filesystem --enforce-version-bump`** + and omit **`--require-signature` by default**; **`--require-signature`** is enforced + when the target is **`main`** (including pushes to **`main`**). **`sign-modules.py`** + in approval workflows continues to use **`--payload-from-filesystem`**. Sign bundled + manifests before merging release PRs or address post-merge verification failures by + re-signing and bumping versions as required. ## [0.44.0] - 2026-03-17 diff --git a/README.md b/README.md index ed4e049..b32e768 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ hatch run test hatch run specfact code review run --json --out .specfact/code-review.json ``` -**Module signatures:** `pr-orchestrator` enforces `--require-signature` only for events targeting **`main`**; for **`dev`** (and feature branches) CI checks checksums and version bumps without requiring a cryptographic signature yet. Add `--require-signature` to the `verify-modules-signature` command when you want the same bar as **`main`** (for example before merging to `main`). Pre-commit runs `scripts/pre-commit-verify-modules-signature.sh`, which mirrors that policy (signatures required on branch `main`, or when `GITHUB_BASE_REF=main` in Actions). +**Module signatures:** For pull request verification, `pr-orchestrator` runs `verify-modules-signature` with **`--payload-from-filesystem --enforce-version-bump`** and does **not** pass **`--require-signature` by default** (checksum + version bump only). **Strict `--require-signature`** applies when the integration target is **`main`** (pushes to `main` and PRs whose base is `main`). Add `--require-signature` locally when you want the same bar as **`main`** before promotion. Approval-time **`sign-modules-on-approval`** signs with `scripts/sign-modules.py` using **`--payload-from-filesystem`** among other flags; if verification fails after merge, re-sign affected **`module-package.yaml`** files and bump versions as needed. Pre-commit runs `scripts/pre-commit-verify-modules-signature.sh`, mirroring CI (**`--require-signature`** on branch **`main`** or when **`GITHUB_BASE_REF=main`** in Actions). **CI signing:** Approved PRs to `dev` or `main` from **this repository** (not forks) run `.github/workflows/sign-modules-on-approval.yml`, which can commit signed manifests using repository secrets. See [Module signing](./docs/authoring/module-signing.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. OpenSpec Markdown other than evidence files is not passed to SpecFact (the review CLI treats paths as Python). The helper runs `specfact code review run --json --out .specfact/code-review.json` on the remaining paths and prints only a short findings summary and copy-paste prompts on stderr. 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. 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 d5c188f..cba6db8 100644 --- a/docs/modules/code-review.md +++ b/docs/modules/code-review.md @@ -37,6 +37,22 @@ Options: findings such as test-scope contract noise - `--interactive`: ask whether changed test files should be included before auto-detected review runs +- `--bug-hunt`: use longer CrossHair budgets (`--per_path_timeout 10`, subprocess + timeout 120s) for deeper counterexample search; other tools keep normal speed +- `--mode shadow|enforce`: **enforce** (default) keeps today’s non-zero process + exit when the governed report says the run failed; **shadow** still runs the + full toolchain and preserves `overall_verdict` in JSON, but forces + `ci_exit_code` and the process exit code to `0` so CI or hooks can log signal + without blocking +- `--focus`: repeatable facet filter applied after scope resolution; values are + `source` (non-test, non-docs Python), `tests` (paths with a `tests/` segment), + and `docs` (Python under a `docs/` directory segment). Multiple `--focus` + values **union** their file sets, then intersect with the resolved scope. When + any `--focus` is set, **`--include-tests` and `--exclude-tests` are rejected** + (use focus alone to express test intent) +- `--level error|warning`: drop findings below the chosen severity **before** + scoring and report construction so JSON, tables, score, verdict, and + `ci_exit_code` all match the filtered list. Omit to keep all severities When `FILES` is omitted, the command falls back to: @@ -80,8 +96,10 @@ findings such as: ### Exit codes -- `0`: `PASS` or `PASS_WITH_ADVISORY` -- `1`: `FAIL` +- `0`: `PASS` or `PASS_WITH_ADVISORY`, or any outcome under **`--mode shadow`** + (shadow forces success at the process level even when `overall_verdict` is + `FAIL`) +- `1`: `FAIL` under default **enforce** semantics - `2`: invalid CLI usage, such as a missing file path or incompatible options ### Output modes @@ -249,6 +267,14 @@ Additional behavior: - semgrep rule IDs emitted with path prefixes are normalized back to the governed rule IDs above - malformed output, a missing `results` list, or a missing Semgrep executable yields a single `tool_error` finding +### Semgrep bug-rules pass + +After the clean-code Semgrep pass, the orchestrator runs +`specfact_code_review.tools.semgrep_runner.run_semgrep_bugs(files)`, which uses +`packages/specfact-code-review/.semgrep/bugs.yaml` when present. Findings are +mapped to `security` or `correctness`. If the config file is missing, the pass +is skipped with no error. + ### Contract runner `specfact_code_review.tools.contract_runner.run_contract_check(files)` combines two @@ -261,20 +287,26 @@ 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 - missing icontract decorators become `contracts` findings with rule - `MISSING_ICONTRACT` + `MISSING_ICONTRACT` when the scan runs - unreadable or invalid Python files degrade to a single `tool_error` finding instead of raising CrossHair behavior: ```bash -crosshair check --per_path_timeout 2 +crosshair check --per_path_timeout 2 # default +crosshair check --per_path_timeout 10 # with CLI --bug-hunt ``` - CrossHair counterexamples map to `contracts` warnings with tool `crosshair` - timeouts are skipped so the AST scan can still complete - missing CrossHair binaries degrade to a single `tool_error` finding +- with **`--bug-hunt`**, the per-path timeout is **10** seconds and the + subprocess budget is **120** seconds instead of **2** / **30** Operational note: @@ -374,12 +406,13 @@ bundle runners in this order: 1. Ruff 2. Radon -3. Semgrep -4. AST clean-code checks -5. basedpyright -6. pylint -7. contract runner -8. TDD gate, unless `no_tests=True` +3. Semgrep (clean-code ruleset) +4. Semgrep bug rules (`.semgrep/bugs.yaml`, skipped if absent) +5. AST clean-code checks +6. basedpyright +7. pylint +8. contract runner (AST + CrossHair; optional bug-hunt timeouts) +9. TDD gate, unless `no_tests=True` When `SPECFACT_CODE_REVIEW_PR_MODE=1` is present, the runner also evaluates a bundle-local advisory PR checklist from `SPECFACT_CODE_REVIEW_PR_TITLE`, diff --git a/docs/reference/module-security.md b/docs/reference/module-security.md index 5edfb5e..1060aab 100644 --- a/docs/reference/module-security.md +++ b/docs/reference/module-security.md @@ -45,10 +45,11 @@ Module packages carry **publisher** and **integrity** metadata so installation, - **CI secrets**: - `SPECFACT_MODULE_PRIVATE_SIGN_KEY` - `SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE` -- **Verification command**: - - Default strict local / **main** check: `scripts/verify-modules-signature.py --require-signature --payload-from-filesystem --enforce-version-bump` - - **Dev / feature parity with CI** (checksum + version bump, signature optional): omit `--require-signature` (see `pr-orchestrator` and `scripts/pre-commit-verify-modules-signature.sh`). - - `--version-check-base ` is used for PR comparisons in CI. +- **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. + - **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`). ## Public key and key rotation diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.md new file mode 100644 index 0000000..5902d26 --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.md @@ -0,0 +1,31 @@ +# TDD evidence — code-review-bug-finding-and-sidecar-venv-fix + +## Timestamp + +2026-04-14 (worktree `feature/code-review-bug-finding-and-sidecar-impl`) + +## Tests + +- `hatch run test` — **566 passed** (after contract-runner fixture updates and new tests for `tool_availability` / `run` package exports). + +## Quality gates (touched scope) + +- `hatch run format` — clean +- `hatch run type-check` — clean +- `hatch run lint` — clean +- `hatch run yaml-lint` — clean +- `hatch run validate-cli-contracts` — clean +- `hatch run check-bundle-imports` — clean +- `hatch run verify-modules-signature --payload-from-filesystem --enforce-version-bump` — clean (manifests re-signed with `--allow-unsigned` where no release key; registry `index.json` + `registry/modules` tarballs updated for `specfact-codebase` **0.41.5** and `specfact-code-review` **0.47.0**) + +## SpecFact code review + +- For KISS/radon changes in the editable module to be exercised, link the dev module before CLI review: + - `hatch run python scripts/link_dev_module.py specfact-code-review --force` +- Full-repo JSON report: `hatch run specfact code review run --json --out .specfact/code-review.json` + - After dev link: **0 error-severity** findings; remaining items are **warnings** (historical KISS/complexity across the repo). The pre-commit / quality gate exit policy is **error-severity only**: **warnings do not block**—only **error**-severity findings affect the CI exit code. +- Scoped check on primary touched sources (Typer `run`, `radon_runner`, `run/commands`, FastAPI/Flask extractors): `PASS_WITH_ADVISORY`, **`ci_exit_code` 0**, report at `.specfact/code-review-touch.json`. + +## Registry + +- `registry/index.json` updated for new module versions and tarball SHA-256 digests; sidecar `.sha256` files written next to published `.tar.gz` artifacts under `registry/modules/`. 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 c53d934..6130642 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,57 +1,57 @@ ## 1. Sidecar venv self-scan fix (specfact-codebase) -- [ ] 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` -- [ ] 1.2 Replace `search_path.rglob("*.py")` with `self._iter_python_files(search_path)` in `FastAPIExtractor.detect()` and `FastAPIExtractor.extract_routes()` -- [ ] 1.3 Replace `rglob("*.py")` with `self._iter_python_files(...)` in `FlaskExtractor` -- [ ] 1.4 Replace `rglob("*.py")` with `self._iter_python_files(...)` in `DjangoExtractor` and `DRFExtractor` -- [ ] 1.5 Write tests: repo fixture with `.specfact/venv/` containing fake routes; assert extractor returns 0 routes from venv; assert real routes still found -- [ ] 1.6 Bump `specfact-codebase` patch version in `module-package.yaml` +- [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()` +- [x] 1.3 Replace `rglob("*.py")` with `self._iter_python_files(...)` in `FlaskExtractor` +- [x] 1.4 Replace `rglob("*.py")` with `self._iter_python_files(...)` in `DjangoExtractor` and `DRFExtractor` +- [x] 1.5 Write tests: repo fixture with `.specfact/venv/` containing fake routes; assert extractor returns 0 routes from venv; assert real routes still found +- [x] 1.6 Bump `specfact-codebase` patch version in `module-package.yaml` ## 2. MISSING_ICONTRACT auto-suppression (specfact-code-review) -- [ ] 2.1 Add `_has_icontract_usage(files: list[Path]) -> bool` to `contract_runner.py` — scan file ASTs for `from icontract import` or `import icontract` -- [ ] 2.2 In `run_contract_check`, call `_has_icontract_usage`; when `False`, skip `_scan_file` loop and return only CrossHair findings -- [ ] 2.3 Write tests: files with no icontract import → no `MISSING_ICONTRACT` findings; files with icontract import → findings emitted as before +- [x] 2.1 Add `_has_icontract_usage(files: list[Path]) -> bool` to `contract_runner.py` — scan file ASTs for `from icontract import` or `import icontract` +- [x] 2.2 In `run_contract_check`, call `_has_icontract_usage`; when `False`, skip `_scan_file` loop and return only CrossHair findings +- [x] 2.3 Write tests: files with no icontract import → no `MISSING_ICONTRACT` findings; files with icontract import → findings emitted as before ## 3. CrossHair bug-hunt mode (specfact-code-review) -- [ ] 3.1 Add `bug_hunt: bool = False` parameter to `run_contract_check` and `_run_crosshair`; when `True` use `--per_path_timeout 10` and subprocess timeout 120s -- [ ] 3.2 Thread `bug_hunt` through `run_review` in `runner.py` -- [ ] 3.3 Add `bug_hunt: bool = False` field to `ReviewRunRequest` in `commands.py` -- [ ] 3.4 Add `--bug-hunt` Typer option to the `run` command; wire into `ReviewRunRequest` -- [ ] 3.5 Write tests: `ReviewRunRequest(bug_hunt=True)` propagates to CrossHair invocation with extended timeouts; default is unchanged +- [x] 3.1 Add `bug_hunt: bool = False` parameter to `run_contract_check` and `_run_crosshair`; when `True` use `--per_path_timeout 10` and subprocess timeout 120s +- [x] 3.2 Thread `bug_hunt` through `run_review` in `runner.py` +- [x] 3.3 Add `bug_hunt: bool = False` field to `ReviewRunRequest` in `commands.py` +- [x] 3.4 Add `--bug-hunt` Typer option to the `run` command; wire into `ReviewRunRequest` +- [x] 3.5 Write tests: `ReviewRunRequest(bug_hunt=True)` propagates to CrossHair invocation with extended timeouts; default is unchanged ## 4. Semgrep bugs.yaml pass (specfact-code-review) -- [ ] 4.1 Create `packages/specfact-code-review/.semgrep/bugs.yaml` with an initial set of Python bug/security rules (≤10 high-confidence rules: dangerous system calls, useless equality checks, hardcoded passwords, swallowed broad exceptions with re-raise, unsafe `eval`/`exec`) -- [ ] 4.2 Add `find_semgrep_bugs_config()` to `semgrep_runner.py` — mirrors `find_semgrep_config` but returns `None` instead of raising when absent -- [ ] 4.3 Add `run_semgrep_bugs(files, *, bundle_root)` that calls `find_semgrep_bugs_config` and skips gracefully when `None`; map findings to `category="security"` or `category="correctness"` -- [ ] 4.4 Add `run_semgrep_bugs` to the `_tool_steps()` list in `runner.py` after the existing semgrep step -- [ ] 4.5 Write tests: file matching a `bugs.yaml` rule returns a finding; missing `bugs.yaml` returns no findings and no exception +- [x] 4.1 Create `packages/specfact-code-review/.semgrep/bugs.yaml` with an initial set of Python bug/security rules (≤10 high-confidence rules: dangerous system calls, useless equality checks, hardcoded passwords, swallowed broad exceptions with re-raise, unsafe `eval`/`exec`) +- [x] 4.2 Add `find_semgrep_bugs_config()` to `semgrep_runner.py` — mirrors `find_semgrep_config` but returns `None` instead of raising when absent +- [x] 4.3 Add `run_semgrep_bugs(files, *, bundle_root)` that calls `find_semgrep_bugs_config` and skips gracefully when `None`; map findings to `category="security"` or `category="correctness"` +- [x] 4.4 Add `run_semgrep_bugs` to the `_tool_steps()` list in `runner.py` after the existing semgrep step +- [x] 4.5 Write tests: file matching a `bugs.yaml` rule returns a finding; missing `bugs.yaml` returns no findings and no exception ## 5. Enforcement mode, focus facets, and severity level (specfact-code-review) -- [ ] 5.1 Add `ReviewRunMode = Literal["shadow", "enforce"]`, `ReviewFocusFacet = Literal["source", "tests", "docs"]`, and `ReviewLevel = Literal["error", "warning"] | None` (or equivalent) on `ReviewRunRequest`; default `mode="enforce"`, `focus=()`, `level=None` -- [ ] 5.2 Implement `_filter_files_by_focus(files, facets)` in `run/commands.py` (or a small helper module) using the facet rules from design D6; apply after `_resolve_files` -- [ ] 5.3 Add Typer options on `review/commands.py`: `--mode`, repeatable `--focus`, `--level`; reject `--focus` together with `--include-tests` / `--exclude-tests` with `typer.BadParameter` -- [ ] 5.4 Thread `mode` and `level` through `run_command` → `_run_review_with_progress` → `run_review`: inside `run_review` (or a single post-orchestration helper), apply `--level` filtering to the collected findings before `score_review` / `ReviewReport` construction so JSON, tables, score, verdict, and `ci_exit_code` all match the filtered list -- [ ] 5.5 When `mode == "shadow"`, set `ci_exit_code` to `0` on the final `ReviewReport` after scoring while preserving `overall_verdict` -- [ ] 5.6 Unit tests: focus union and exclusions; level filtering; shadow exit override; Typer conflict errors -- [ ] 5.7 Extend `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` per delta spec `review-cli-contracts` +- [x] 5.1 Add `ReviewRunMode = Literal["shadow", "enforce"]`, `ReviewFocusFacet = Literal["source", "tests", "docs"]`, and `ReviewLevel = Literal["error", "warning"] | None` (or equivalent) on `ReviewRunRequest`; default `mode="enforce"`, `focus=()`, `level=None` +- [x] 5.2 Implement `_filter_files_by_focus(files, facets)` in `run/commands.py` (or a small helper module) using the facet rules from design D6; apply after `_resolve_files` +- [x] 5.3 Add Typer options on `review/commands.py`: `--mode`, repeatable `--focus`, `--level`; reject `--focus` together with `--include-tests` / `--exclude-tests` with `typer.BadParameter` +- [x] 5.4 Thread `mode` and `level` through `run_command` → `_run_review_with_progress` → `run_review`: inside `run_review` (or a single post-orchestration helper), apply `--level` filtering to the collected findings before `score_review` / `ReviewReport` construction so JSON, tables, score, verdict, and `ci_exit_code` all match the filtered list +- [x] 5.5 When `mode == "shadow"`, set `ci_exit_code` to `0` on the final `ReviewReport` after scoring while preserving `overall_verdict` +- [x] 5.6 Unit tests: focus union and exclusions; level filtering; shadow exit override; Typer conflict errors +- [x] 5.7 Extend `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` per delta spec `review-cli-contracts` ## 6. Tool dependencies and runtime availability (specfact-code-review) -- [ ] 6.1 Add a canonical `REVIEW_TOOL_PIP_PACKAGES` (or equivalent) map: tool id → executable name on PATH (if any) → pip distribution name(s); document AST pass as in-process only -- [ ] 6.2 Audit `packages/specfact-code-review/module-package.yaml` `pip_dependencies` against the map; add any missing entries (including for new runners such as `bugs.yaml` semgrep pass — still `semgrep`) -- [ ] 6.3 Implement `specfact_code_review.tools.tool_availability` (or similar) with `skip_if_tool_missing(tool_id, file_path) -> list[ReviewFinding]` returning one standardized `tool_error` per missing tool (message shape per design D11) -- [ ] 6.4 Call the helper at the start of `run_ruff`, `run_radon`, `run_semgrep`, `run_basedpyright`, `run_pylint`, and `run_contract_check` (before CrossHair only; AST scan always runs); extend `run_semgrep_bugs` when implemented -- [ ] 6.5 In `runner.py`, handle pytest / pytest-cov absence for the TDD gate with the same skip messaging pattern (no misleading “coverage read failed” when pytest never ran) -- [ ] 6.6 Add a unit test that loads `module-package.yaml` and asserts every pip package from the canonical map is listed under `pip_dependencies` -- [ ] 6.7 Add unit tests with `PATH` / env patched so at least one tool is missing → exactly one skip finding, no subprocess invoked +- [x] 6.1 Add a canonical `REVIEW_TOOL_PIP_PACKAGES` (or equivalent) map: tool id → executable name on PATH (if any) → pip distribution name(s); document AST pass as in-process only +- [x] 6.2 Audit `packages/specfact-code-review/module-package.yaml` `pip_dependencies` against the map; add any missing entries (including for new runners such as `bugs.yaml` semgrep pass — still `semgrep`) +- [x] 6.3 Implement `specfact_code_review.tools.tool_availability` (or similar) with `skip_if_tool_missing(tool_id, file_path) -> list[ReviewFinding]` returning one standardized `tool_error` per missing tool (message shape per design D11) +- [x] 6.4 Call the helper at the start of `run_ruff`, `run_radon`, `run_semgrep`, `run_basedpyright`, `run_pylint`, and `run_contract_check` (before CrossHair only; AST scan always runs); extend `run_semgrep_bugs` when implemented +- [x] 6.5 In `runner.py`, handle pytest / pytest-cov absence for the TDD gate with the same skip messaging pattern (no misleading “coverage read failed” when pytest never ran) +- [x] 6.6 Add a unit test that loads `module-package.yaml` and asserts every pip package from the canonical map is listed under `pip_dependencies` +- [x] 6.7 Add unit tests with `PATH` / env patched so at least one tool is missing → exactly one skip finding, no subprocess invoked ## 7. TDD evidence and quality gates -- [ ] 7.1 Run `hatch run test` — all new and existing tests pass -- [ ] 7.2 Run `hatch run format && hatch run type-check && hatch run lint` — clean -- [ ] 7.3 Run `specfact code review run --json --out .specfact/code-review.json` — resolve any findings -- [ ] 7.4 Record passing test output in `TDD_EVIDENCE.md` +- [x] 7.1 Run `hatch run test` — all new and existing tests pass +- [x] 7.2 Run `hatch run format && hatch run type-check && hatch run lint` — clean +- [x] 7.3 Run `specfact code review run --json --out .specfact/code-review.json` — resolve any findings +- [x] 7.4 Record passing test output in `TDD_EVIDENCE.md` diff --git a/packages/specfact-code-review/.semgrep/bugs.yaml b/packages/specfact-code-review/.semgrep/bugs.yaml new file mode 100644 index 0000000..536c387 --- /dev/null +++ b/packages/specfact-code-review/.semgrep/bugs.yaml @@ -0,0 +1,56 @@ +# Bundled high-confidence bug / security patterns for specfact-code-review second pass. +# Additions should ship with tests (see openspec change code-review-bug-finding-and-sidecar-venv-fix). +rules: + - id: specfact-bugs-eval-exec + languages: [python] + message: Avoid eval() and exec(); they enable arbitrary code execution. + severity: ERROR + pattern-either: + - pattern: eval(...) + - pattern: exec(...) + metadata: + specfact-category: security + + - id: specfact-bugs-os-system + languages: [python] + message: Avoid os.system(); prefer subprocess with a fixed argument list. + severity: WARNING + pattern: os.system(...) + metadata: + specfact-category: security + + - id: specfact-bugs-pickle-loads + languages: [python] + message: pickle.loads on untrusted data is unsafe; validate inputs or use a safe format. + severity: WARNING + pattern: pickle.loads(...) + metadata: + specfact-category: security + + - id: specfact-bugs-yaml-unsafe + languages: [python] + message: > + yaml.load(...) without an explicit Loader= uses unsafe defaults; use yaml.safe_load() or pass + Loader= explicitly (e.g. SafeLoader/FullLoader). + severity: WARNING + patterns: + - pattern: yaml.load(...) + - pattern-not-regex: yaml\.load\([^)]*Loader\s*= + metadata: + specfact-category: security + + - id: specfact-bugs-hardcoded-password + languages: [python] + message: Possible hardcoded password assignment; use configuration or secrets management. + severity: WARNING + pattern-regex: (?i)(password|passwd|secret)\s*=\s*['"][^'"]+['"] + metadata: + specfact-category: security + + - id: specfact-bugs-useless-comparison + languages: [python] + message: Comparison may be always True or always False (same variable on both sides). + severity: WARNING + pattern: $X == $X + metadata: + specfact-category: clean_code diff --git a/packages/specfact-code-review/module-package.yaml b/packages/specfact-code-review/module-package.yaml index 68caac3..05fefe6 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.46.4 +version: 0.47.1 commands: - code tier: official @@ -23,5 +23,4 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:ee7d224ac2a7894cc67d3e052764137b034e089624d5007989cab818839e5449 - signature: V+GNklTfgmdYKDWgp53SDw4s1R5GE1UF/745CVnXFJ0v3WSCWLY8APqyuabetBU6/Z1UQ1lKfEiRiZOFJw7WBg== + checksum: sha256:aab4cba70012af43ef8451412b7d45fa70e5bd460eec02fc0523392b45d06b48 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 33323ad..2da0395 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 @@ -31,6 +31,15 @@ def normalize_path_variants(path_value: str | Path) -> set[str]: return variants +@beartype +@require(lambda files: isinstance(files, list)) +@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.).""" + return [path for path in files if path.suffix == ".py"] + + @beartype @require(lambda tool: isinstance(tool, str) and bool(tool.strip())) @require(lambda file_path: isinstance(file_path, Path)) 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 3021d66..3b6a66d 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 @@ -24,6 +24,8 @@ def _friendly_run_command_error(exc: ValueError | ViolationError) -> str: "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.", ): if expected in message: return expected @@ -40,6 +42,40 @@ def _resolve_include_tests(*, files: list[Path], include_tests: bool | None, int return typer.confirm("Include changed and untracked test files in this review?", default=False) +def _resolve_review_run_flags( + *, + files: list[Path] | None, + include_tests: bool | None, + exclude_tests: bool | None, + focus: list[str] | None, + include_noise: bool, + suppress_noise: bool, + interactive: bool, +) -> tuple[list[str], bool, bool]: + if include_tests is not None and exclude_tests is not None: + raise typer.BadParameter("Cannot use both --include-tests and --exclude-tests") + + focus_list = list(focus) if focus else [] + if focus_list: + if include_tests is not None or exclude_tests is not None: + raise typer.BadParameter("Cannot combine --focus with --include-tests or --exclude-tests") + unknown = [facet for facet in focus_list if facet not in {"source", "tests", "docs"}] + if unknown: + raise typer.BadParameter(f"Invalid --focus value(s): {unknown!r}; use source, tests, or docs.") + resolved_include_tests = True + else: + resolved_include_tests = _resolve_include_tests( + files=files or [], + include_tests=include_tests, + interactive=interactive, + ) + if exclude_tests is True: + resolved_include_tests = False + + resolved_include_noise = include_noise and not suppress_noise + return focus_list, resolved_include_tests, resolved_include_noise + + @review_app.command("run") @require(lambda ctx: True, "run command validation") @ensure(lambda result: result is None, "run command does not return") @@ -50,6 +86,10 @@ def run( path: list[Path] = typer.Option(None, "--path"), include_tests: bool = typer.Option(None, "--include-tests"), exclude_tests: bool = typer.Option(None, "--exclude-tests"), + focus: list[str] | None = typer.Option(None, "--focus", help="Limit to source, tests, and/or docs (repeatable)."), + mode: Literal["shadow", "enforce"] = typer.Option("enforce", "--mode"), + level: Literal["error", "warning"] | None = typer.Option(None, "--level"), + bug_hunt: bool = typer.Option(False, "--bug-hunt"), include_noise: bool = typer.Option(False, "--include-noise"), suppress_noise: bool = typer.Option(False, "--suppress-noise"), json_output: bool = typer.Option(False, "--json"), @@ -60,25 +100,26 @@ def run( interactive: bool = typer.Option(False, "--interactive"), ) -> None: """Run the full code review workflow.""" - # Resolve mutually exclusive test inclusion options - if include_tests is not None and exclude_tests is not None: - raise typer.BadParameter("Cannot use both --include-tests and --exclude-tests") - - resolved_include_tests = _resolve_include_tests( - files=files or [], + focus_list, resolved_include_tests, resolved_include_noise = _resolve_review_run_flags( + files=files, include_tests=include_tests, + exclude_tests=exclude_tests, + focus=focus, + include_noise=include_noise, + suppress_noise=suppress_noise, interactive=interactive, ) - # Resolve noise inclusion (suppress-noise takes precedence) - resolved_include_noise = include_noise and not suppress_noise - try: exit_code, output = run_command( files or [], include_tests=resolved_include_tests, scope=scope, path_filters=path, + focus_facets=tuple(focus_list), + review_mode=mode, + review_level=level, + bug_hunt=bug_hunt, include_noise=resolved_include_noise, json_output=json_output, out=out, diff --git a/packages/specfact-code-review/src/specfact_code_review/run/__init__.py b/packages/specfact-code-review/src/specfact_code_review/run/__init__.py index b570b0d..dd37c65 100644 --- a/packages/specfact-code-review/src/specfact_code_review/run/__init__.py +++ b/packages/specfact-code-review/src/specfact_code_review/run/__init__.py @@ -5,6 +5,7 @@ from collections.abc import Callable from importlib import import_module from pathlib import Path +from typing import Literal from beartype import beartype from icontract import ensure, require @@ -23,6 +24,9 @@ def run_review( no_tests: bool = False, include_noise: bool = False, progress_callback: Callable[[str], None] | None = None, + bug_hunt: bool = False, + review_level: Literal["error", "warning"] | None = None, + review_mode: Literal["shadow", "enforce"] = "enforce", ) -> ReviewReport: """Lazily import the orchestrator to avoid package import cycles.""" run_review_impl = import_module("specfact_code_review.run.runner").run_review @@ -31,6 +35,9 @@ def run_review( no_tests=no_tests, include_noise=include_noise, progress_callback=progress_callback, + bug_hunt=bug_hunt, + review_level=review_level, + review_mode=review_mode, ) 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 9be7a20..8b89499 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 @@ -22,6 +22,8 @@ console = Console() progress_console = Console(stderr=True) AutoScope = Literal["changed", "full"] +ReviewRunMode = Literal["shadow", "enforce"] +ReviewLevelFilter = Literal["error", "warning"] @dataclass(frozen=True) @@ -38,12 +40,50 @@ class ReviewRunRequest: score_only: bool = False no_tests: bool = False fix: bool = False + bug_hunt: bool = False + review_mode: ReviewRunMode = "enforce" + review_level: ReviewLevelFilter | None = None + focus_facets: tuple[str, ...] = () + + +@dataclass(frozen=True) +class _ReviewLoopFlags: + no_tests: bool + include_noise: bool + fix: bool + progress_callback: Callable[[str], None] | None + bug_hunt: bool + review_mode: ReviewRunMode + review_level: ReviewLevelFilter | None def _is_test_file(file_path: Path) -> bool: return "tests" in file_path.parts +def _is_docs_tree_file(file_path: Path) -> bool: + return "docs" in file_path.parts + + +def _filter_files_by_focus(files: list[Path], facets: tuple[str, ...]) -> list[Path]: + """Restrict files to the union of facet selections (Python files only).""" + if not facets: + return files + + def _matches_focus(file_path: Path, facet: str) -> bool: + if file_path.suffix not in (".py", ".pyi"): + return False + if facet == "tests": + return _is_test_file(file_path) + if facet == "docs": + return _is_docs_tree_file(file_path) + if facet == "source": + return not _is_test_file(file_path) and not _is_docs_tree_file(file_path) + return False + + return [file_path for file_path in files if any(_matches_focus(file_path, f) for f in facets)] + + def _is_ignored_review_path(file_path: Path) -> bool: parent_parts = file_path.parts[:-1] return any(part.startswith(".") and len(part) > 1 for part in parent_parts) @@ -75,7 +115,7 @@ def _changed_files_from_git_diff(*, include_tests: bool) -> list[Path]: python_files = [ file_path for file_path in [*tracked_files, *untracked_files] - if file_path.suffix == ".py" and file_path.is_file() and not _is_ignored_review_path(file_path) + if file_path.suffix in (".py", ".pyi") and file_path.is_file() and not _is_ignored_review_path(file_path) ] deduped_python_files = list(dict.fromkeys(python_files)) if include_tests: @@ -95,7 +135,7 @@ def _all_python_files_from_git() -> list[Path]: python_files = [ file_path for file_path in [*tracked_files, *untracked_files] - if file_path.suffix == ".py" and file_path.is_file() and not _is_ignored_review_path(file_path) + if file_path.suffix in (".py", ".pyi") and file_path.is_file() and not _is_ignored_review_path(file_path) ] return list(dict.fromkeys(python_files)) @@ -277,19 +317,35 @@ def _run_review_with_progress( no_tests: bool, include_noise: bool, fix: bool, + bug_hunt: bool, + review_mode: ReviewRunMode, + review_level: ReviewLevelFilter | None, ) -> ReviewReport: if _is_interactive_terminal(): - return _run_review_with_status(files, no_tests=no_tests, include_noise=include_noise, fix=fix) + return _run_review_with_status( + files, + no_tests=no_tests, + include_noise=include_noise, + fix=fix, + bug_hunt=bug_hunt, + review_mode=review_mode, + review_level=review_level, + ) def _emit_progress(description: str) -> None: progress_console.print(f"[dim]{description}[/dim]") return _run_review_once( files, - no_tests=no_tests, - include_noise=include_noise, - fix=fix, - progress_callback=_emit_progress, + _ReviewLoopFlags( + no_tests=no_tests, + include_noise=include_noise, + fix=fix, + progress_callback=_emit_progress, + bug_hunt=bug_hunt, + review_mode=review_mode, + review_level=review_level, + ), ) @@ -299,58 +355,57 @@ def _run_review_with_status( no_tests: bool, include_noise: bool, fix: bool, + bug_hunt: bool, + review_mode: ReviewRunMode, + review_level: ReviewLevelFilter | None, ) -> ReviewReport: with progress_console.status("Preparing code review...") as status: - report = _run_review_once( - files, + base = _ReviewLoopFlags( no_tests=no_tests, include_noise=include_noise, fix=False, progress_callback=status.update, + bug_hunt=bug_hunt, + review_mode=review_mode, + review_level=review_level, ) + report = _run_review_once(files, base) if fix: status.update("Applying Ruff autofixes...") _apply_fixes(files) status.update("Re-running review after autofixes...") - report = _run_review_once( - files, - no_tests=no_tests, - include_noise=include_noise, - fix=False, - progress_callback=status.update, - ) + report = _run_review_once(files, base) return report -def _run_review_once( - files: list[Path], - *, - no_tests: bool, - include_noise: bool, - fix: bool, - progress_callback: Callable[[str], None] | None, -) -> ReviewReport: +def _run_review_once(files: list[Path], flags: _ReviewLoopFlags) -> ReviewReport: report = run_review( files, - no_tests=no_tests, - include_noise=include_noise, - progress_callback=progress_callback, + no_tests=flags.no_tests, + include_noise=flags.include_noise, + progress_callback=flags.progress_callback, + bug_hunt=flags.bug_hunt, + review_mode=flags.review_mode, + review_level=flags.review_level, ) - if fix: - if progress_callback is not None: - progress_callback("Applying Ruff autofixes...") + if flags.fix: + if flags.progress_callback is not None: + flags.progress_callback("Applying Ruff autofixes...") else: progress_console.print("[dim]Applying Ruff autofixes...[/dim]") _apply_fixes(files) - if progress_callback is not None: - progress_callback("Re-running review after autofixes...") + if flags.progress_callback is not None: + flags.progress_callback("Re-running review after autofixes...") else: progress_console.print("[dim]Re-running review after autofixes...[/dim]") report = run_review( files, - no_tests=no_tests, - include_noise=include_noise, - progress_callback=progress_callback, + no_tests=flags.no_tests, + include_noise=flags.include_noise, + progress_callback=flags.progress_callback, + bug_hunt=flags.bug_hunt, + review_mode=flags.review_mode, + review_level=flags.review_level, ) return report @@ -379,6 +434,33 @@ def _as_optional_path(value: object) -> Path | None: raise ValueError("Output path must be a Path instance.") +def _as_review_mode(value: object) -> ReviewRunMode: + if value is None or value == "enforce": + return "enforce" + if value == "shadow": + return "shadow" + raise ValueError(f"Invalid review mode: {value!r}") + + +def _as_review_level(value: object) -> ReviewLevelFilter | None: + if value is None: + return None + if value in ("error", "warning"): + return cast(ReviewLevelFilter, value) + raise ValueError(f"Invalid review level: {value!r}") + + +def _as_focus_facets(value: object) -> tuple[str, ...]: + if value is None: + return () + 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}") + return tuple(value) + raise ValueError("focus_facets must be a list or tuple of strings") + + def _build_review_run_request( files: list[Path], kwargs: dict[str, object], @@ -425,6 +507,10 @@ def _get_optional_param(name: str, validator: Callable[[object], object], defaul path_filters = cast(list[Path] | None, path_filters_value) out = cast(Path | None, out_value) + 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.") + request = ReviewRunRequest( files=files, include_tests=include_tests, @@ -436,6 +522,10 @@ def _get_optional_param(name: str, validator: Callable[[object], object], defaul score_only=_get_bool_param("score_only"), no_tests=_get_bool_param("no_tests"), fix=_get_bool_param("fix"), + bug_hunt=_get_bool_param("bug_hunt"), + review_mode=_as_review_mode(request_kwargs.pop("review_mode", "enforce")), + review_level=_as_review_level(request_kwargs.pop("review_level", None)), + focus_facets=focus_facets, ) # Reject any unexpected keyword arguments @@ -487,17 +577,29 @@ def run_command( ) _validate_review_request(request) + include_for_resolve = request.include_tests or ("tests" in request.focus_facets) resolved_files = _resolve_files( request.files, - include_tests=request.include_tests, + include_tests=include_for_resolve, scope=request.scope, path_filters=request.path_filters or [], ) + resolved_files = _filter_files_by_focus(resolved_files, request.focus_facets) + if not resolved_files: + raise ValueError( + "No reviewable Python files matched the selected --focus facets." + if request.focus_facets + else "No Python files to review were provided or detected." + ) + report = _run_review_with_progress( resolved_files, no_tests=request.no_tests, include_noise=request.include_noise, fix=request.fix, + bug_hunt=request.bug_hunt, + review_mode=request.review_mode, + review_level=request.review_level, ) return _render_review_result(report, request) diff --git a/packages/specfact-code-review/src/specfact_code_review/run/runner.py b/packages/specfact-code-review/src/specfact_code_review/run/runner.py index 53426cc..fab4d2f 100644 --- a/packages/specfact-code-review/src/specfact_code_review/run/runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/run/runner.py @@ -10,7 +10,9 @@ import tempfile from collections.abc import Callable, Iterable from contextlib import suppress +from functools import partial from pathlib import Path +from typing import Literal from uuid import uuid4 from beartype import beartype @@ -25,7 +27,8 @@ from specfact_code_review.tools.pylint_runner import run_pylint from specfact_code_review.tools.radon_runner import run_radon from specfact_code_review.tools.ruff_runner import run_ruff -from specfact_code_review.tools.semgrep_runner import run_semgrep +from specfact_code_review.tools.semgrep_runner import run_semgrep, run_semgrep_bugs +from specfact_code_review.tools.tool_availability import skip_if_pytest_unavailable _SOURCE_ROOT = Path("packages/specfact-code-review/src") @@ -243,18 +246,30 @@ def _checklist_findings() -> list[ReviewFinding]: ] -def _tool_steps() -> list[tuple[str, Callable[[list[Path]], list[ReviewFinding]]]]: +def _tool_steps(*, bug_hunt: bool) -> list[tuple[str, Callable[[list[Path]], list[ReviewFinding]]]]: return [ ("Running Ruff checks...", run_ruff), ("Running Radon complexity checks...", run_radon), ("Running Semgrep rules...", run_semgrep), + ("Running Semgrep bug rules...", run_semgrep_bugs), ("Running AST clean-code checks...", run_ast_clean_code), ("Running basedpyright type checks...", run_basedpyright), ("Running pylint checks...", run_pylint), - ("Running contract checks...", run_contract_check), + ("Running contract checks...", partial(run_contract_check, bug_hunt=bug_hunt)), ] +def _filter_findings_by_review_level( + findings: list[ReviewFinding], + level: Literal["error", "warning"] | None, +) -> list[ReviewFinding]: + if level is None: + return findings + if level == "error": + return [finding for finding in findings if finding.severity == "error"] + return [finding for finding in findings if finding.severity in {"error", "warning"}] + + def _collect_tdd_inputs(files: list[Path]) -> tuple[list[Path], list[Path], list[ReviewFinding]]: source_files = [file_path for file_path in files if _expected_test_path(file_path) is not None] findings: list[ReviewFinding] = [] @@ -366,6 +381,10 @@ def _evaluate_tdd_gate(files: list[Path]) -> tuple[list[ReviewFinding], dict[str if findings: return findings, None + pytest_skip = skip_if_pytest_unavailable(source_files[0]) + if pytest_skip: + return pytest_skip, None + try: test_result, coverage_path = _run_pytest_with_coverage(test_files) except (FileNotFoundError, OSError, subprocess.TimeoutExpired) as exc: @@ -442,10 +461,13 @@ def run_review( no_tests: bool = False, include_noise: bool = False, progress_callback: Callable[[str], None] | None = None, + bug_hunt: bool = False, + review_level: Literal["error", "warning"] | None = None, + review_mode: Literal["shadow", "enforce"] = "enforce", ) -> ReviewReport: """Run all configured review runners and build the governed report.""" findings: list[ReviewFinding] = [] - for description, runner in _tool_steps(): + for description, runner in _tool_steps(bug_hunt=bug_hunt): if progress_callback is not None: progress_callback(description) findings.extend(runner(files)) @@ -463,6 +485,8 @@ def run_review( if not include_noise: findings = _suppress_known_noise(findings) + findings = _filter_findings_by_review_level(findings, review_level) + score = score_review( findings=findings, zero_loc_violations=not any(finding.tool == "ruff" and finding.rule == "E501" for finding in findings), @@ -471,9 +495,12 @@ def run_review( coverage_90_plus=coverage_90_plus, no_new_suppressions=_has_no_suppressions(files), ) - return ReviewReport( + report = ReviewReport( run_id=f"review-{uuid4()}", score=score.score, findings=findings, summary=_summary_for_findings(findings), ) + if review_mode == "shadow": + return report.model_copy(update={"ci_exit_code": 0}) + return report diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py index 7b122be..ab5842f 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py @@ -10,7 +10,7 @@ from beartype import beartype from icontract import ensure, require -from specfact_code_review._review_utils import tool_error +from specfact_code_review._review_utils import python_source_paths_for_tools, tool_error from specfact_code_review.run.findings import ReviewFinding @@ -187,7 +187,7 @@ def _solid_findings(file_path: Path, tree: ast.Module) -> list[ReviewFinding]: def run_ast_clean_code(files: list[Path]) -> list[ReviewFinding]: """Run Python-native AST checks for SOLID, YAGNI, and DRY findings.""" findings: list[ReviewFinding] = [] - for file_path in files: + for file_path in python_source_paths_for_tools(files): try: tree = ast.parse(file_path.read_text(encoding="utf-8"), filename=str(file_path)) except (OSError, SyntaxError) as exc: diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py index 17a253d..3746b53 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py @@ -10,8 +10,9 @@ from beartype import beartype from icontract import require -from specfact_code_review._review_utils import normalize_path_variants, tool_error +from specfact_code_review._review_utils import normalize_path_variants, python_source_paths_for_tools, tool_error from specfact_code_review.run.findings import ReviewFinding +from specfact_code_review.tools.tool_availability import skip_if_tool_missing def _allowed_paths(files: list[Path]) -> set[str]: @@ -88,9 +89,14 @@ def _findings_from_diagnostics(diagnostics: list[object], *, allowed_paths: set[ @require(lambda files: all(isinstance(file_path, Path) for file_path in files), "files must contain Path instances") def run_basedpyright(files: list[Path]) -> list[ReviewFinding]: """Run basedpyright and map diagnostics into ReviewFinding records.""" + files = python_source_paths_for_tools(files) if not files: return [] + skipped = skip_if_tool_missing("basedpyright", files[0]) + if skipped: + return skipped + try: result = subprocess.run( ["basedpyright", "--outputjson", "--project", ".", *[str(file_path) for file_path in files]], @@ -100,7 +106,7 @@ def run_basedpyright(files: list[Path]) -> list[ReviewFinding]: timeout=30, ) diagnostics = _diagnostics_from_output(result.stdout) - except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, KeyError, subprocess.TimeoutExpired) as exc: + except (OSError, ValueError, json.JSONDecodeError, KeyError, subprocess.TimeoutExpired) as exc: return [ tool_error( tool="basedpyright", 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 fd04bf0..d8062f7 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 @@ -10,12 +10,32 @@ from beartype import beartype from icontract import ensure, require -from specfact_code_review._review_utils import normalize_path_variants, tool_error +from specfact_code_review._review_utils import normalize_path_variants, python_source_paths_for_tools, tool_error from specfact_code_review.run.findings import ReviewFinding +from specfact_code_review.tools.tool_availability import skip_if_tool_missing _CROSSHAIR_LINE_RE = re.compile(r"^(?P.+?):(?P\d+):\s*(?:error|warning|info):\s*(?P.+)$") _IGNORED_CROSSHAIR_PREFIXES = ("SideEffectDetected:",) + + +def _has_icontract_usage(files: list[Path]) -> bool: + """True when any reviewed file imports the icontract package.""" + 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": + return True + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "icontract": + return True + return False + + _SYNC_RUNTIME_ICONTRACT_ENTRYPOINTS = { "bridge_probe.py", "bridge_sync.py", @@ -104,17 +124,23 @@ def _scan_file(file_path: Path) -> list[ReviewFinding]: return findings -def _run_crosshair(files: list[Path]) -> list[ReviewFinding]: +def _run_crosshair(files: list[Path], *, bug_hunt: bool) -> list[ReviewFinding]: if not files: return [] + skipped = skip_if_tool_missing("crosshair", files[0]) + if skipped: + return skipped + + per_path_timeout = "10" if bug_hunt else "2" + proc_timeout = 120 if bug_hunt else 30 try: result = subprocess.run( - ["crosshair", "check", "--per_path_timeout", "2", *(str(file_path) for file_path in files)], + ["crosshair", "check", "--per_path_timeout", per_path_timeout, *(str(file_path) for file_path in files)], capture_output=True, text=True, check=False, - timeout=30, + timeout=proc_timeout, ) except subprocess.TimeoutExpired: return [] @@ -163,13 +189,15 @@ def _run_crosshair(files: list[Path]) -> list[ReviewFinding]: lambda result: all(isinstance(finding, ReviewFinding) for finding in result), "result must contain ReviewFinding instances", ) -def run_contract_check(files: list[Path]) -> list[ReviewFinding]: +def run_contract_check(files: list[Path], *, bug_hunt: bool = False) -> list[ReviewFinding]: """Run AST-based contract checks and a CrossHair fast pass for the provided files.""" - if not files: + py_files = python_source_paths_for_tools(files) + if not py_files: return [] findings: list[ReviewFinding] = [] - for file_path in files: - findings.extend(_scan_file(file_path)) - findings.extend(_run_crosshair(files)) + if _has_icontract_usage(py_files): + for file_path in py_files: + findings.extend(_scan_file(file_path)) + findings.extend(_run_crosshair(py_files, bug_hunt=bug_hunt)) return findings diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py index e194f90..af333ff 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py @@ -10,8 +10,9 @@ from beartype import beartype from icontract import ensure, require -from specfact_code_review._review_utils import normalize_path_variants, tool_error +from specfact_code_review._review_utils import normalize_path_variants, python_source_paths_for_tools, tool_error from specfact_code_review.run.findings import ReviewFinding +from specfact_code_review.tools.tool_availability import skip_if_tool_missing PYLINT_CATEGORY_MAP: dict[str, Literal["architecture"]] = { @@ -102,9 +103,14 @@ def _result_is_review_findings(result: list[ReviewFinding]) -> bool: @ensure(_result_is_review_findings, "result must contain ReviewFinding instances") def run_pylint(files: list[Path]) -> list[ReviewFinding]: """Run pylint and map message IDs into ReviewFinding records.""" + files = python_source_paths_for_tools(files) if not files: return [] + skipped = skip_if_tool_missing("pylint", files[0]) + if skipped: + return skipped + try: result = subprocess.run( ["pylint", "--output-format", "json", *[str(file_path) for file_path in files]], @@ -114,7 +120,7 @@ def run_pylint(files: list[Path]) -> list[ReviewFinding]: timeout=30, ) payload = _payload_from_output(result.stdout) - except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: + except (OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: return [tool_error(tool="pylint", file_path=files[0], message=f"Unable to parse pylint output: {exc}")] allowed_paths = _allowed_paths(files) 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 7e31382..4490300 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 @@ -13,7 +13,9 @@ from beartype import beartype from icontract import ensure, require +from specfact_code_review._review_utils import python_source_paths_for_tools from specfact_code_review.run.findings import ReviewFinding +from specfact_code_review.tools.tool_availability import skip_if_tool_missing _KISS_LOC_WARNING = 80 @@ -163,10 +165,30 @@ def _kiss_nesting_findings( return findings +def _typer_cli_entrypoint_exempt(function_node: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: + """Typer command callbacks legitimately take many injected options; skip parameter-count KISS on them.""" + args0 = function_node.args.args + if not args0: + return False + first = args0[0] + if first.arg != "ctx": + return False + ann = first.annotation + if ann is None: + return False + try: + rendered = ast.unparse(ann) + except AttributeError: + return False + return rendered.endswith("Context") + + def _kiss_parameter_findings( function_node: ast.FunctionDef | ast.AsyncFunctionDef, file_path: Path ) -> list[ReviewFinding]: findings: list[ReviewFinding] = [] + if _typer_cli_entrypoint_exempt(function_node): + return findings parameter_count = len(function_node.args.posonlyargs) parameter_count += len(function_node.args.args) parameter_count += len(function_node.args.kwonlyargs) @@ -270,9 +292,14 @@ def _ensure_review_findings(result: list[ReviewFinding]) -> bool: ) def run_radon(files: list[Path]) -> list[ReviewFinding]: """Run Radon for the provided files and map complexity findings into ReviewFinding records.""" + files = python_source_paths_for_tools(files) if not files: return [] + skipped = skip_if_tool_missing("radon", files[0]) + if skipped: + return skipped + payload = _run_radon_command(files) findings: list[ReviewFinding] = [] if payload is None: diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py index 93f1a9d..c4da5ca 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py @@ -10,8 +10,9 @@ from beartype import beartype from icontract import ensure, require -from specfact_code_review._review_utils import normalize_path_variants, tool_error +from specfact_code_review._review_utils import normalize_path_variants, python_source_paths_for_tools, tool_error from specfact_code_review.run.findings import ReviewFinding +from specfact_code_review.tools.tool_availability import skip_if_tool_missing def _allowed_paths(files: list[Path]) -> set[str]: @@ -96,9 +97,14 @@ def _result_is_review_findings(result: list[ReviewFinding]) -> bool: @ensure(_result_is_review_findings, "result must contain ReviewFinding instances") def run_ruff(files: list[Path]) -> list[ReviewFinding]: """Run Ruff for the provided files and map findings into ReviewFinding records.""" + files = python_source_paths_for_tools(files) if not files: return [] + skipped = skip_if_tool_missing("ruff", files[0]) + if skipped: + return skipped + try: result = subprocess.run( ["ruff", "check", "--output-format", "json", *[str(file_path) for file_path in files]], @@ -108,7 +114,7 @@ def run_ruff(files: list[Path]) -> list[ReviewFinding]: timeout=30, ) payload = _payload_from_output(result.stdout) - except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: + except (OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: return [tool_error(tool="ruff", file_path=files[0], message=f"Unable to parse Ruff output: {exc}")] allowed_paths = _allowed_paths(files) 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 7118cd7..0c5a9ab 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 @@ -13,6 +13,7 @@ from icontract import ensure, require from specfact_code_review.run.findings import ReviewFinding +from specfact_code_review.tools.tool_availability import skip_if_tool_missing SEMGREP_RULE_CATEGORY = { @@ -29,6 +30,16 @@ _SEMGREP_STDERR_SNIP_MAX = 4000 _MAX_CONFIG_PARENT_WALK = 32 SemgrepCategory = Literal["clean_code", "architecture", "naming"] +BugSemgrepCategory = Literal["security", "clean_code"] + +BUG_RULE_CATEGORY: dict[str, BugSemgrepCategory] = { + "specfact-bugs-eval-exec": "security", + "specfact-bugs-os-system": "security", + "specfact-bugs-pickle-loads": "security", + "specfact-bugs-yaml-unsafe": "security", + "specfact-bugs-hardcoded-password": "security", + "specfact-bugs-useless-comparison": "clean_code", +} def _normalize_path_variants(path_value: str | Path) -> set[str]: @@ -121,7 +132,40 @@ def find_semgrep_config( raise FileNotFoundError(f"Semgrep config not found (no .semgrep/clean_code.yaml under bundle for {here})") -def _run_semgrep_command(files: list[Path], *, bundle_root: Path | None) -> subprocess.CompletedProcess[str]: +@beartype +@require(lambda bundle_root, module_file: bundle_root is None or isinstance(bundle_root, Path)) +@require(lambda bundle_root, module_file: module_file is None or isinstance(module_file, Path)) +@ensure(lambda result: result is None or isinstance(result, Path)) +def find_semgrep_bugs_config( + *, + bundle_root: Path | None = None, + module_file: Path | None = None, +) -> Path | None: + """Locate ``.semgrep/bugs.yaml`` for this package or bundle root; return ``None`` if absent.""" + if bundle_root is not None: + br = bundle_root.resolve() + candidate = br / ".semgrep" / "bugs.yaml" + return candidate if candidate.is_file() else None + + here = (module_file if module_file is not None else Path(__file__)).resolve() + for depth, parent in enumerate([here.parent, *here.parents]): + if depth > _MAX_CONFIG_PARENT_WALK: + break + if parent == parent.parent: + break + candidate = parent / ".semgrep" / "bugs.yaml" + if candidate.is_file(): + return candidate + if _is_bundle_boundary(parent): + break + if parent.name == "site-packages": + break + return None + + +def _run_semgrep_command( + files: list[Path], *, bundle_root: Path | None, config_file: Path +) -> subprocess.CompletedProcess[str]: with tempfile.TemporaryDirectory(prefix="semgrep-home-") as temp_home: semgrep_home = Path(temp_home) semgrep_log_dir = semgrep_home / ".semgrep" @@ -138,7 +182,7 @@ def _run_semgrep_command(files: list[Path], *, bundle_root: Path | None) -> subp "--disable-version-check", "--quiet", "--config", - str(find_semgrep_config(bundle_root=bundle_root)), + str(config_file), "--json", *(str(file_path) for file_path in files), ], @@ -158,11 +202,11 @@ def _snip_stderr_tail(stderr: str) -> str: return "…" + err_raw[-_SEMGREP_STDERR_SNIP_MAX:] -def _load_semgrep_results(files: list[Path], *, bundle_root: Path | None) -> list[object]: +def _load_semgrep_results(files: list[Path], *, bundle_root: Path | None, config_file: Path) -> list[object]: last_error: Exception | None = None for _attempt in range(SEMGREP_RETRY_ATTEMPTS): try: - result = _run_semgrep_command(files, bundle_root=bundle_root) + result = _run_semgrep_command(files, bundle_root=bundle_root, config_file=config_file) raw_out = result.stdout.strip() if not raw_out: err_tail = _snip_stderr_tail(result.stderr or "") @@ -254,8 +298,13 @@ def run_semgrep(files: list[Path], *, bundle_root: Path | None = None) -> list[R if not files: return [] + skipped = skip_if_tool_missing("semgrep", files[0]) + if skipped: + return skipped + try: - raw_results = _load_semgrep_results(files, bundle_root=bundle_root) + config_path = find_semgrep_config(bundle_root=bundle_root) + raw_results = _load_semgrep_results(files, bundle_root=bundle_root, config_file=config_path) except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: return _tool_error(files[0], f"Unable to parse Semgrep output: {exc}") @@ -281,3 +330,106 @@ def _append_semgrep_finding( finding = _finding_from_result(item, allowed_paths=allowed_paths) if finding is not None: findings.append(finding) + + +def _normalize_bug_rule_id(rule: str) -> str: + for rule_id in BUG_RULE_CATEGORY: + if rule == rule_id or rule.endswith(f".{rule_id}"): + return rule_id + return rule.rsplit(".", 1)[-1] + + +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): + return None + + raw_rule = item["check_id"] + if not isinstance(raw_rule, str): + raise ValueError("semgrep rule must be a string") + 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" + ) + + return ReviewFinding( + category=category, + severity=severity, + tool="semgrep", + rule=rule, + file=filename, + line=line, + message=message, + fixable=False, + ) + + +def _append_semgrep_bug_finding( + findings: list[ReviewFinding], + item: object, + *, + allowed_paths: set[str], +) -> None: + if not isinstance(item, dict): + raise ValueError("semgrep finding must be an object") + finding = _finding_from_bug_result(item, allowed_paths=allowed_paths) + if finding is not None: + findings.append(finding) + + +@beartype +@require(lambda files: isinstance(files, list), "files must be a list") +@require(lambda files: all(isinstance(file_path, Path) for file_path in files), "files must contain Path instances") +@ensure(lambda result: isinstance(result, list), "result must be a list") +@ensure( + lambda result: all(isinstance(finding, ReviewFinding) for finding in result), + "result must contain ReviewFinding instances", +) +def run_semgrep_bugs(files: list[Path], *, bundle_root: Path | None = None) -> list[ReviewFinding]: + """Second Semgrep pass using ``.semgrep/bugs.yaml`` when present; no-op if config is absent.""" + if not files: + return [] + + config_path = find_semgrep_bugs_config(bundle_root=bundle_root) + if config_path is None: + return [] + + skipped = skip_if_tool_missing("semgrep", files[0]) + if skipped: + return skipped + + try: + raw_results = _load_semgrep_results(files, bundle_root=bundle_root, config_file=config_path) + except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: + return _tool_error(files[0], f"Unable to parse Semgrep bugs pass output: {exc}") + + allowed_paths = _allowed_paths(files) + findings: list[ReviewFinding] = [] + try: + for item in raw_results: + _append_semgrep_bug_finding(findings, item, allowed_paths=allowed_paths) + except (KeyError, TypeError, ValueError) as exc: + return _tool_error(files[0], f"Unable to parse Semgrep bugs finding payload: {exc}") + + return findings diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py b/packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py new file mode 100644 index 0000000..a42b2fc --- /dev/null +++ b/packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py @@ -0,0 +1,108 @@ +"""Resolve external review-tool executables and emit skip findings when missing.""" + +from __future__ import annotations + +import importlib.util +import shutil +from pathlib import Path +from typing import Literal + +from beartype import beartype +from icontract import ensure, require + +from specfact_code_review._review_utils import tool_error +from specfact_code_review.run.findings import ReviewFinding + + +ReviewToolId = Literal[ + "ruff", + "radon", + "semgrep", + "basedpyright", + "pylint", + "crosshair", + "pytest", +] + +# tool_id -> pip distribution name(s) declared on module-package.yaml +REVIEW_TOOL_PIP_PACKAGES: dict[ReviewToolId, str] = { + "ruff": "ruff", + "radon": "radon", + "semgrep": "semgrep", + "basedpyright": "basedpyright", + "pylint": "pylint", + "crosshair": "crosshair-tool", + "pytest": "pytest", +} + +# Pytest is listed in REVIEW_TOOL_PIP_PACKAGES for documentation parity with module-package.yaml, but it is +# intentionally omitted here: skip_if_tool_missing() only consults _EXECUTABLE_ON_PATH and would treat a +# stray `pytest` script on PATH as “tool present” even when the review interpreter cannot import pytest. +# TDD coverage instead uses skip_if_pytest_unavailable(), which probes importlib.util.find_spec("pytest") +# and importlib.util.find_spec("pytest_cov") for the active Python environment. +_EXECUTABLE_ON_PATH: dict[ReviewToolId, str] = { + "ruff": "ruff", + "radon": "radon", + "semgrep": "semgrep", + "basedpyright": "basedpyright", + "pylint": "pylint", + "crosshair": "crosshair", +} + + +def _skip_message(tool_id: ReviewToolId) -> str: + pip_name = REVIEW_TOOL_PIP_PACKAGES[tool_id] + return ( + f"Review checks for {tool_id} were skipped: executable not found on PATH. " + f"Install the `{pip_name}` package (declared on the code-review module) so the tool is available." + ) + + +@beartype +@require(lambda tool_id: tool_id in REVIEW_TOOL_PIP_PACKAGES) +@ensure(lambda result: isinstance(result, list)) +def skip_if_tool_missing(tool_id: ReviewToolId, file_path: Path) -> list[ReviewFinding]: + """Return a single tool_error when the CLI is absent; otherwise return an empty list.""" + exe = _EXECUTABLE_ON_PATH.get(tool_id) + if exe is not None and shutil.which(exe) is None: + return [ + tool_error( + tool=tool_id, + file_path=file_path, + message=_skip_message(tool_id), + severity="warning", + ) + ] + return [] + + +@beartype +@require(lambda file_path: isinstance(file_path, Path)) +@ensure(lambda result: isinstance(result, list)) +def skip_if_pytest_unavailable(file_path: Path) -> list[ReviewFinding]: + """Skip TDD gate when pytest cannot be imported in the current interpreter.""" + if importlib.util.find_spec("pytest") is None: + return [ + tool_error( + tool="pytest", + file_path=file_path, + message=( + "Review checks for pytest were skipped: pytest is not importable in this environment. " + "Install `pytest` and `pytest-cov` (declared on the code-review module)." + ), + severity="warning", + ) + ] + if importlib.util.find_spec("pytest_cov") is None: + return [ + tool_error( + tool="pytest", + file_path=file_path, + message=( + "Review checks for pytest coverage were skipped: pytest-cov is not importable. " + "Install `pytest-cov` (declared on the code-review module)." + ), + severity="warning", + ) + ] + return [] diff --git a/packages/specfact-codebase/module-package.yaml b/packages/specfact-codebase/module-package.yaml index 9776e9d..5552a70 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.4 +version: 0.41.6 commands: - code tier: official @@ -24,5 +24,4 @@ description: Official SpecFact codebase bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:436e9e3d0d56a6eae861c4a6bef0a8f805f00b8b5bd8b0e037c19dede4972117 - signature: FQOsqabH5ATcyLfjTVrVJPIC4KiuwwFbCQZL+BZAu6dFoOz/DLjk91NZAyc7z6oq5dpVfwMHFsKEYqzW4wlDDA== + checksum: sha256:a1508cf26a2519eefae9106ad991db5406cdbbb46ba0eefd56068aa32e754f83 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 f97dadb..96d7c6e 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 @@ -8,6 +8,8 @@ from __future__ import annotations from abc import ABC, abstractmethod +from collections.abc import Iterator +from pathlib import Path from typing import Any from beartype import beartype @@ -31,6 +33,25 @@ class RouteInfo(BaseModel): class BaseFrameworkExtractor(ABC): """Abstract base class for framework-specific route and schema extractors.""" + _EXCLUDED_DIR_NAMES: frozenset[str] = frozenset( + {".specfact", ".git", "__pycache__", "node_modules", "venv", ".venv"} + ) + + @beartype + @staticmethod + def _path_touches_excluded_dir(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) + + @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 + @abstractmethod @beartype @require(lambda repo_path: repo_path.exists(), "Repository path must exist") diff --git a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.py b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.py index e7093c7..913f6f1 100644 --- a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.py +++ b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.py @@ -38,7 +38,7 @@ def detect(self, repo_path: Path) -> bool: if manage_py.exists(): return True - urls_files = list(repo_path.rglob("urls.py")) + urls_files = [path for path in self._iter_python_files(repo_path) if path.name == "urls.py"] return len(urls_files) > 0 @beartype @@ -98,7 +98,7 @@ def _find_urls_file(self, repo_path: Path) -> Path | None: if candidate.exists(): return candidate - urls_files = list(repo_path.rglob("urls.py")) + urls_files = [path for path in self._iter_python_files(repo_path) if path.name == "urls.py"] return urls_files[0] if urls_files else None @beartype diff --git a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.py b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.py index 66d89ad..9af097a 100644 --- a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.py +++ b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.py @@ -38,7 +38,7 @@ def detect(self, repo_path: Path) -> bool: True if DRF is detected """ # Check for rest_framework imports - for py_file in repo_path.rglob("*.py"): + for py_file in self._iter_python_files(repo_path): try: content = py_file.read_text(encoding="utf-8") if "rest_framework" in content or "from rest_framework" in content: 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 34a353f..46cc37f 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 @@ -17,6 +17,64 @@ from specfact_codebase.validators.sidecar.frameworks.base import BaseFrameworkExtractor, RouteInfo +_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 + + +def _content_suggests_fastapi(content: str) -> bool: + return "from fastapi import" in content or "FastAPI(" in content + + +def _read_text_if_exists(path: Path) -> str | None: + try: + return path.read_text(encoding="utf-8") + except (UnicodeDecodeError, PermissionError): + 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.""" @@ -26,36 +84,27 @@ class FastAPIExtractor(BaseFrameworkExtractor): @ensure(lambda result: isinstance(result, bool), "Must return bool") def detect(self, repo_path: Path) -> bool: """ - Detect if FastAPI is used in the repository. + Detect if this framework is used in the repository. Args: repo_path: Path to repository root Returns: - True if FastAPI is detected + True if this framework is detected """ for candidate_file in ["main.py", "app.py"]: file_path = repo_path / candidate_file - if file_path.exists(): - try: - content = file_path.read_text(encoding="utf-8") - if "from fastapi import" in content or "FastAPI(" in content: - return True - except (UnicodeDecodeError, PermissionError): - continue + if not file_path.exists(): + continue + if _should_skip_path_for_fastapi_scan(file_path, repo_path.resolve()): + continue + content = _read_text_if_exists(file_path) + if content is not None and _content_suggests_fastapi(content): + return True - # Check in common locations 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 search_path.rglob("*.py"): - if py_file.name in ["main.py", "app.py"]: - try: - content = py_file.read_text(encoding="utf-8") - if "from fastapi import" in content or "FastAPI(" in content: - return True - except (UnicodeDecodeError, PermissionError): - continue + if search_path.exists() and _scan_known_app_files(search_path): + return True return False @@ -65,21 +114,20 @@ def detect(self, repo_path: Path) -> bool: @ensure(lambda result: isinstance(result, list), "Must return list") def extract_routes(self, repo_path: Path) -> list[RouteInfo]: """ - Extract routes from FastAPI route files. + Extract route information from framework-specific patterns. Args: repo_path: Path to repository root Returns: - List of RouteInfo objects + List of RouteInfo objects with extracted routes """ results: list[RouteInfo] = [] - # Find FastAPI app files 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 search_path.rglob("*.py"): + for py_file in _iter_scan_python_files(search_path): try: routes = self._extract_routes_from_file(py_file) results.extend(routes) @@ -94,17 +142,17 @@ def extract_routes(self, repo_path: Path) -> list[RouteInfo]: @ensure(lambda result: isinstance(result, dict), "Must return dict") def extract_schemas(self, repo_path: Path, routes: list[RouteInfo]) -> dict[str, dict[str, Any]]: """ - Extract schemas from Pydantic models for routes. + Extract request/response schemas from framework-specific patterns. Args: - repo_path: Path to repository root - routes: List of extracted routes + repo_path: Path to repository root (reserved for future schema mining) + routes: List of extracted routes (reserved for future schema mining) Returns: Dictionary mapping route identifiers to schema dictionaries """ - # Simplified schema extraction - full implementation would parse Pydantic models - # For now, return empty dict - can be enhanced later + _ = (repo_path, routes) + # Placeholder until Pydantic schema mining is implemented. return {} @beartype @@ -116,66 +164,93 @@ def _extract_routes_from_file(self, py_file: Path) -> list[RouteInfo]: except (SyntaxError, UnicodeDecodeError, PermissionError): return [] - imports = self._extract_imports(tree) results: list[RouteInfo] = [] for node in ast.walk(tree): if isinstance(node, ast.FunctionDef): - route_info = self._extract_route_from_function(node, imports, py_file) + route_info = self._extract_route_from_function(node) if route_info: results.append(route_info) return results @beartype - def _extract_imports(self, tree: ast.AST) -> dict[str, str]: - """Extract import statements from AST.""" - imports: dict[str, str] = {} - for node in ast.walk(tree): - if isinstance(node, ast.ImportFrom): - module = node.module or "" - for alias in node.names: - alias_name = alias.asname or alias.name - imports[alias_name] = f"{module}.{alias.name}" - elif isinstance(node, ast.Import): - for alias in node.names: - alias_name = alias.asname or alias.name - imports[alias_name] = alias.name - return imports + def _path_method_from_route_call(self, decorator: ast.Call) -> tuple[str, str] | None: + """If ``decorator`` is ``@app.get`` / ``@router.post`` / …, return ``(METHOD, path)``.""" + if isinstance(decorator.func, ast.Attribute): + attr = decorator.func.attr.lower() + if attr not in _ROUTE_HTTP_METHODS: + return None + path = "/" + if decorator.args: + path_arg = self._extract_string_literal(decorator.args[0]) + if path_arg: + path = path_arg + return attr.upper(), path + if isinstance(decorator.func, ast.Name): + name = decorator.func.id.lower() + if name not in _ROUTE_HTTP_METHODS: + return None + path = "/" + if decorator.args: + path_arg = self._extract_string_literal(decorator.args[0]) + if path_arg: + path = path_arg + return name.upper(), path + 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.""" + if not isinstance(decorator.func, ast.Attribute): + return None + if decorator.func.attr != "api_route": + return None + path = "/" + if decorator.args: + path_arg = self._extract_string_literal(decorator.args[0]) + if path_arg: + path = path_arg + methods: list[str] = [] + for kw in decorator.keywords: + if kw.arg != "methods": + continue + if isinstance(kw.value, (ast.List, ast.Tuple)): + for elt in kw.value.elts: + lit = self._extract_string_literal(elt) + if lit: + methods.append(lit.strip().upper()) + if not methods: + return "GET", path + return methods[0], path @beartype - def _extract_route_from_function( - self, func_node: ast.FunctionDef, imports: dict[str, str], py_file: Path - ) -> RouteInfo | None: + def _extract_route_from_function(self, func_node: ast.FunctionDef) -> RouteInfo | None: """Extract route information from a function with FastAPI decorators.""" + matched = False path = "/" method = "GET" - operation_id = func_node.name - # Check decorators for route information for decorator in func_node.decorator_list: - if isinstance(decorator, ast.Call): - if isinstance(decorator.func, ast.Attribute): - # @app.get(), @app.post(), etc. - method = decorator.func.attr.upper() - if decorator.args: - path_arg = self._extract_string_literal(decorator.args[0]) - if path_arg: - path = path_arg - elif isinstance(decorator.func, ast.Name): - # @get(), @post(), etc. - method = decorator.func.id.upper() - if decorator.args: - path_arg = self._extract_string_literal(decorator.args[0]) - if path_arg: - path = path_arg + 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: + continue + matched = True + method, path = got + + if not matched: + return None normalized_path, path_params = self._extract_path_parameters(path) return RouteInfo( path=normalized_path, method=method, - operation_id=operation_id, + operation_id=func_node.name, function=func_node.name, path_params=path_params, ) @@ -193,7 +268,6 @@ def _extract_path_parameters(self, path: str) -> tuple[str, list[dict[str, Any]] path_params: list[dict[str, Any]] = [] normalized_path = path - # FastAPI path parameter pattern: {param_name} or {param_name:type} pattern = r"\{([^}:]+)(?::([^}]+))?\}" matches = list(re.finditer(pattern, path)) 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 c046231..cddb24c 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 @@ -48,7 +48,7 @@ def detect(self, repo_path: Path) -> bool: 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 search_path.rglob("*.py"): + for py_file in self._iter_python_files(search_path): if py_file.name in ["app.py", "main.py", "__init__.py"]: try: content = py_file.read_text(encoding="utf-8") @@ -79,7 +79,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 search_path.rglob("*.py"): + for py_file in self._iter_python_files(search_path): try: routes = self._extract_routes_from_file(py_file) results.extend(routes) @@ -107,6 +107,24 @@ def extract_schemas(self, repo_path: Path, routes: list[RouteInfo]) -> dict[str, # For now, return empty dict return {} + @beartype + def _register_flask_assign_symbols( + self, target: ast.expr, value: ast.expr, app_names: set[str], bp_names: set[str] + ) -> None: + if not isinstance(target, ast.Name) or not isinstance(value, ast.Call): + return + func = value.func + if isinstance(func, ast.Name) and func.id == "Flask": + app_names.add(target.id) + return + if isinstance(func, ast.Attribute) and func.attr == "Flask": + app_names.add(target.id) + return + if (isinstance(func, ast.Name) and func.id == "Blueprint") or ( + isinstance(func, ast.Attribute) and func.attr == "Blueprint" + ): + bp_names.add(target.id) + @beartype def _extract_routes_from_file(self, py_file: Path) -> list[RouteInfo]: """Extract routes from a Python file.""" @@ -125,22 +143,10 @@ def _extract_routes_from_file(self, py_file: Path) -> list[RouteInfo]: # First pass: Find Flask app and Blueprint instances for node in ast.walk(tree): - if isinstance(node, ast.Assign): - for target in node.targets: - if isinstance(target, ast.Name): - if isinstance(node.value, ast.Call): - if isinstance(node.value.func, ast.Name): - func_name = node.value.func.id - if func_name == "Flask": - app_names.add(target.id) - elif isinstance(node.value.func, ast.Attribute): - if node.value.func.attr == "Flask": - app_names.add(target.id) - elif isinstance(node.value, ast.Call) and ( - (isinstance(node.value.func, ast.Name) and node.value.func.id == "Blueprint") - or (isinstance(node.value.func, ast.Attribute) and node.value.func.attr == "Blueprint") - ): - bp_names.add(target.id) + if not isinstance(node, ast.Assign): + continue + for target in node.targets: + self._register_flask_assign_symbols(target, node.value, app_names, bp_names) # Second pass: Extract routes from functions with decorators for node in ast.walk(tree): diff --git a/registry/index.json b/registry/index.json index 935d33a..e7dae2f 100644 --- a/registry/index.json +++ b/registry/index.json @@ -30,9 +30,9 @@ }, { "id": "nold-ai/specfact-codebase", - "latest_version": "0.41.4", - "download_url": "modules/specfact-codebase-0.41.4.tar.gz", - "checksum_sha256": "18534ed0fa07e711f57c9a473db01ab83b5b0ebefba0039b969997919907e049", + "latest_version": "0.41.5", + "download_url": "modules/specfact-codebase-0.41.5.tar.gz", + "checksum_sha256": "fe8f95c325f21eb80209aa067f6a4f2055f1f5feed4e818a1c9d3061320c2270", "tier": "official", "publisher": { "name": "nold-ai", @@ -78,9 +78,9 @@ }, { "id": "nold-ai/specfact-code-review", - "latest_version": "0.46.4", - "download_url": "modules/specfact-code-review-0.46.4.tar.gz", - "checksum_sha256": "caecd26d6e6308ed88047385e0a9579c5336665d46e8118c3ae9caf4cbd786c8", + "latest_version": "0.47.0", + "download_url": "modules/specfact-code-review-0.47.0.tar.gz", + "checksum_sha256": "7bda277c0c8fb137750ee6b88090e0df929e6e699bf5c1c048d18679890bb347", "core_compatibility": ">=0.44.0,<1.0.0", "tier": "official", "publisher": { diff --git a/registry/modules/specfact-code-review-0.47.0.tar.gz b/registry/modules/specfact-code-review-0.47.0.tar.gz new file mode 100644 index 0000000..7f59324 Binary files /dev/null and b/registry/modules/specfact-code-review-0.47.0.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.47.0.tar.gz.sha256 b/registry/modules/specfact-code-review-0.47.0.tar.gz.sha256 new file mode 100644 index 0000000..369986c --- /dev/null +++ b/registry/modules/specfact-code-review-0.47.0.tar.gz.sha256 @@ -0,0 +1 @@ +7bda277c0c8fb137750ee6b88090e0df929e6e699bf5c1c048d18679890bb347 diff --git a/registry/modules/specfact-codebase-0.41.5.tar.gz b/registry/modules/specfact-codebase-0.41.5.tar.gz new file mode 100644 index 0000000..c705667 Binary files /dev/null and b/registry/modules/specfact-codebase-0.41.5.tar.gz differ diff --git a/registry/modules/specfact-codebase-0.41.5.tar.gz.sha256 b/registry/modules/specfact-codebase-0.41.5.tar.gz.sha256 new file mode 100644 index 0000000..227d9a9 --- /dev/null +++ b/registry/modules/specfact-codebase-0.41.5.tar.gz.sha256 @@ -0,0 +1 @@ +fe8f95c325f21eb80209aa067f6a4f2055f1f5feed4e818a1c9d3061320c2270 diff --git a/scripts/git-branch-module-signature-flag.sh b/scripts/git-branch-module-signature-flag.sh new file mode 100755 index 0000000..99bcdf1 --- /dev/null +++ b/scripts/git-branch-module-signature-flag.sh @@ -0,0 +1,27 @@ +#!/usr/bin/env bash +# Emit module signature policy for the current context (consumed by pre-commit-verify-modules-signature.sh). +# +# Contract matches specfact-cli `scripts/git-branch-module-signature-flag.sh`: print a single token +# "require" on `main`, "omit" elsewhere. This repo additionally treats GITHUB_BASE_REF=main (PRs +# targeting main) as "require" so pre-commit matches integration-target semantics. +set -euo pipefail + +if [[ -n "${GITHUB_BASE_REF:-}" ]]; then + if [[ "${GITHUB_BASE_REF}" == "main" ]]; then + printf '%s\n' "require" + exit 0 + fi + printf '%s\n' "omit" + exit 0 +fi + +branch="" +branch=$(git branch --show-current 2>/dev/null || true) +if [[ -z "${branch}" || "${branch}" == "HEAD" ]]; then + branch=$(git rev-parse --abbrev-ref HEAD 2>/dev/null || true) +fi +if [[ "${branch}" == "main" ]]; then + printf '%s\n' "require" +else + printf '%s\n' "omit" +fi diff --git a/scripts/pre-commit-verify-modules-signature.sh b/scripts/pre-commit-verify-modules-signature.sh index 2fcd3b9..8e5481e 100755 --- a/scripts/pre-commit-verify-modules-signature.sh +++ b/scripts/pre-commit-verify-modules-signature.sh @@ -1,24 +1,39 @@ #!/usr/bin/env bash -# Mirror pr-orchestrator verify-module-signatures policy: require cryptographic signatures only when -# the integration target is `main`. Locally that is branch `main`; in GitHub Actions pull_request* -# contexts use GITHUB_BASE_REF (PR base / target), not GITHUB_REF_NAME (head). +# Pre-commit entry: branch-aware module verify (same policy shape as specfact-cli +# `scripts/pre-commit-verify-modules.sh`, adapted for this repository). +# +# Uses `scripts/git-branch-module-signature-flag.sh` (require | omit). When policy is `require` +# (checkout or PR target is `main`), run full payload + signature verification. When `omit`, +# run `verify-modules-signature.py --metadata-only` so local commits are not blocked by checksum +# drift before CI / approval-time signing refreshes manifests (specfact-cli `omit` still runs full +# checksum verification against bundled modules under modules/). set -euo pipefail + _repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" cd "$_repo_root" -_branch="$(git rev-parse --abbrev-ref HEAD 2>/dev/null || true)" -_require_signature=false -if [[ -n "${GITHUB_BASE_REF:-}" ]]; then - if [[ "${GITHUB_BASE_REF}" == "main" ]]; then - _require_signature=true - fi -elif [[ "$_branch" == "main" ]]; then - _require_signature=true +_flag_script="${_repo_root}/scripts/git-branch-module-signature-flag.sh" +if [[ ! -f "${_flag_script}" ]]; then + echo "❌ Missing ${_flag_script}" >&2 + exit 1 fi +sig_policy=$(bash "${_flag_script}") +sig_policy="${sig_policy//$'\r'/}" +sig_policy="${sig_policy//$'\n'/}" _base=(hatch run ./scripts/verify-modules-signature.py --payload-from-filesystem --enforce-version-bump) -if [[ "$_require_signature" == true ]]; then - exec "${_base[@]}" --require-signature -else - exec "${_base[@]}" -fi + +case "${sig_policy}" in + require) + echo "🔐 Verifying module manifests (strict: --require-signature, --enforce-version-bump, --payload-from-filesystem)" >&2 + exec "${_base[@]}" --require-signature + ;; + omit) + echo "🔐 Verifying module manifests (metadata-only for local commits; full verify runs in CI — see docs/reference/module-security.md)" >&2 + exec "${_base[@]}" --metadata-only + ;; + *) + echo "❌ Invalid module signature policy from ${_flag_script}: '${sig_policy}' (expected require or omit)" >&2 + exit 1 + ;; +esac diff --git a/scripts/pre_commit_code_review.py b/scripts/pre_commit_code_review.py index 6256967..aac752f 100755 --- a/scripts/pre_commit_code_review.py +++ b/scripts/pre_commit_code_review.py @@ -53,6 +53,8 @@ def _is_review_gate_path(path: str) -> bool: normalized = path.replace("\\", "/").strip() if not normalized: return False + if normalized.endswith("module-package.yaml"): + return False if normalized.startswith("openspec/changes/") and Path(normalized).name.casefold() == "tdd_evidence.md": return False prefixes = ( @@ -83,12 +85,14 @@ def filter_review_gate_paths(paths: Sequence[str]) -> list[str]: def _specfact_review_paths(paths: Sequence[str]) -> list[str]: - """Paths to pass to SpecFact ``code review run`` (it treats inputs as Python; skip OpenSpec Markdown).""" + """Paths to pass to SpecFact ``code review run`` (Python sources only; skip Markdown and non-.py/.pyi).""" result: list[str] = [] for raw in paths: normalized = raw.replace("\\", "/").strip() if normalized.startswith("openspec/changes/") and normalized.lower().endswith(".md"): continue + if not normalized.endswith((".py", ".pyi")): + continue result.append(raw) return result @@ -204,25 +208,29 @@ def count_findings_by_severity(findings: list[object]) -> dict[str, int]: return buckets -def _print_review_findings_summary(repo_root: Path) -> bool: - """Parse ``REVIEW_JSON_OUT`` and print a one-line findings count (errors / warnings / etc.).""" +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)``.""" 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 + return False, 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 + return False, None except json.JSONDecodeError as exc: sys.stderr.write(f"Code review: invalid JSON in {REVIEW_JSON_OUT}: {exc}\n") - return False + return False, 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 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 + return False, None counts = count_findings_by_severity(findings_raw) total = len(findings_raw) @@ -246,7 +254,7 @@ def _print_review_findings_summary(repo_root: Path) -> bool: 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 + return True, counts["error"] @ensure(lambda result: isinstance(result, tuple) and len(result) == 2) @@ -288,7 +296,7 @@ def main(argv: Sequence[str] | None = None) -> int: if len(specfact_files) == 0: sys.stdout.write( "Staged review paths are only OpenSpec Markdown under openspec/changes/; " - "skipping SpecFact code review (those files are not Python review targets).\n" + "skipping SpecFact code review (no staged .py/.pyi targets; Markdown is not passed to SpecFact).\n" ) return 0 @@ -307,8 +315,11 @@ 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. - if not _print_review_findings_summary(repo_root): + summary_ok, error_count = _print_review_findings_summary(repo_root) + if not summary_ok or error_count is None: return 1 + if error_count == 0: + return 0 return result.returncode diff --git a/scripts/validate_agent_rule_applies_when.py b/scripts/validate_agent_rule_applies_when.py old mode 100644 new mode 100755 diff --git a/scripts/verify-modules-signature.py b/scripts/verify-modules-signature.py index 8a75a72..565bc20 100755 --- a/scripts/verify-modules-signature.py +++ b/scripts/verify-modules-signature.py @@ -391,8 +391,32 @@ def verify_manifest( @beartype -@ensure(lambda result: result in {0, 1}, "main must return a process exit code") -def main() -> int: +@require(lambda manifest_path: manifest_path.exists(), "manifest_path must exist") +@ensure(lambda result: result is None, "integrity-shape-only verification raises or returns None") +def verify_manifest_integrity_shape_only( + manifest_path: Path, + *, + require_signature: bool, +) -> None: + raw = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + if not isinstance(raw, dict): + raise ValueError("manifest YAML must be object") + integrity = raw.get("integrity") + if not isinstance(integrity, dict): + raise ValueError("missing integrity metadata") + + checksum = str(integrity.get("checksum", "")).strip() + if not checksum: + raise ValueError("missing integrity.checksum") + _parse_checksum(checksum) + signature = str(integrity.get("signature", "")).strip() + if require_signature and not signature: + raise ValueError("missing integrity.signature") + if signature and len(signature) < 32: + raise ValueError("integrity.signature is present but implausibly short") + + +def _parse_verify_cli_args() -> argparse.Namespace: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument( "--require-signature", action="store_true", help="Require integrity.signature for every manifest" @@ -422,39 +446,69 @@ def main() -> int: "--payload-from-filesystem." ), ) - args = parser.parse_args() + parser.add_argument( + "--metadata-only", + action="store_true", + help=( + "Only validate module-package.yaml structure (integrity.checksum format; " + "integrity.signature required when --require-signature). Skips payload digest and " + "cryptographic checks so PRs to dev can pass before approval-time signing updates " + "manifests; push to main and fork PRs to main still use the full verifier in CI." + ), + ) + return parser.parse_args() - public_key_pem = _resolve_public_key(args) - manifests = _iter_manifests() - if not manifests: - _emit_line("No module-package.yaml manifests found.") - return 0 +def _verify_manifests_for_cli(args: argparse.Namespace, public_key_pem: str, manifests: list[Path]) -> list[str]: failures: list[str] = [] for manifest in manifests: try: - verification_mode = verify_manifest( - manifest, - require_signature=args.require_signature, - public_key_pem=public_key_pem, - payload_from_filesystem=args.payload_from_filesystem, - ) - suffix = ( - " (filesystem payload)" - if verification_mode == "filesystem" and not args.payload_from_filesystem - else "" - ) - _emit_line(f"OK {manifest}{suffix}") + if args.metadata_only: + verify_manifest_integrity_shape_only( + manifest, + require_signature=args.require_signature, + ) + _emit_line(f"OK {manifest} (metadata-only)") + else: + verification_mode = verify_manifest( + manifest, + require_signature=args.require_signature, + public_key_pem=public_key_pem, + payload_from_filesystem=args.payload_from_filesystem, + ) + suffix = ( + " (filesystem payload)" + if verification_mode == "filesystem" and not args.payload_from_filesystem + else "" + ) + _emit_line(f"OK {manifest}{suffix}") except ValueError as exc: failures.append(f"FAIL {manifest}: {exc}") + return failures - version_failures: list[str] = [] - if args.enforce_version_bump: - base_ref = _resolve_version_check_base(args.version_check_base) - try: - version_failures = _verify_version_bumps(base_ref) - except ValueError as exc: - version_failures.append(f"FAIL version-check: {exc}") + +def _version_bump_failures_for_cli(args: argparse.Namespace) -> list[str]: + if not args.enforce_version_bump: + return [] + base_ref = _resolve_version_check_base(args.version_check_base) + try: + return _verify_version_bumps(base_ref) + except ValueError as exc: + return [f"FAIL version-check: {exc}"] + + +@beartype +@ensure(lambda result: result in {0, 1}, "main must return a CLI exit code") +def main() -> int: + args = _parse_verify_cli_args() + public_key_pem = "" if args.metadata_only else _resolve_public_key(args) + manifests = _iter_manifests() + if not manifests: + _emit_line("No module-package.yaml manifests found.") + return 0 + + failures = _verify_manifests_for_cli(args, public_key_pem, manifests) + version_failures = _version_bump_failures_for_cli(args) if failures or version_failures: if failures: diff --git a/tests/cli-contracts/specfact-code-review-run.scenarios.yaml b/tests/cli-contracts/specfact-code-review-run.scenarios.yaml index 7be31de..0b8c65e 100644 --- a/tests/cli-contracts/specfact-code-review-run.scenarios.yaml +++ b/tests/cli-contracts/specfact-code-review-run.scenarios.yaml @@ -55,3 +55,77 @@ scenarios: exit_code: 2 stderr_contains: - choose positional files or auto-scope controls + - name: mode-shadow-dirty-exit-zero + type: pattern + argv: + - --json + - --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: + - --json + - --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: + - --scope + - full + - --path + - packages/specfact-code-review + - --json + - --focus + - source + - --focus + - docs + expect: + exit_code: 0 + stdout_contains: + - '"run_id":' + - name: focus-tests-narrows-to-test-tree + type: pattern + argv: + - --scope + - full + - --path + - packages/specfact-code-review + - --json + - --focus + - tests + expect: + exit_code: 0 + stdout_contains: + - '"run_id":' + - name: level-error-json-clean-module + type: pattern + argv: + - --json + - --level + - error + - tests/fixtures/review/clean_module.py + expect: + exit_code: 0 + stdout_contains: + - review-report.json + - name: focus-cannot-combine-with-include-tests + type: anti-pattern + argv: + - tests/fixtures/review/clean_module.py + - --focus + - source + - --include-tests + expect: + exit_code: 2 + stderr_contains: + - Cannot combine --focus with --include-tests or --exclude-tests diff --git a/tests/unit/scripts/test_pre_commit_code_review.py b/tests/unit/scripts/test_pre_commit_code_review.py index dc43362..a25ca5c 100644 --- a/tests/unit/scripts/test_pre_commit_code_review.py +++ b/tests/unit/scripts/test_pre_commit_code_review.py @@ -34,11 +34,26 @@ def _load_script_module() -> Any: } -def test_specfact_review_paths_skips_openspec_markdown() -> None: +def test_specfact_review_paths_keeps_only_python_sources() -> None: module = _load_script_module() assert module._specfact_review_paths( - ["tests/test_app.py", "openspec/changes/foo/tasks.md", "openspec/changes/foo/proposal.md"] - ) == ["tests/test_app.py"] + [ + "tests/test_app.py", + "openspec/changes/foo/tasks.md", + "openspec/changes/foo/proposal.md", + "registry/modules/specfact-code-review-0.47.0.tar.gz", + "registry/index.json", + "packages/specfact-code-review/module-package.yaml", + "src/pkg/stub.pyi", + ] + ) == ["tests/test_app.py", "src/pkg/stub.pyi"] + + +def test_filter_review_gate_paths_excludes_module_package_manifest() -> None: + """module-package.yaml is not Python; it must not trigger the code-review gate.""" + module = _load_script_module() + + assert module.filter_review_gate_paths(["packages/specfact-code-review/module-package.yaml"]) == [] def test_filter_review_gate_paths_keeps_contract_relevant_trees() -> None: @@ -95,6 +110,39 @@ def test_main_skips_specfact_when_only_openspec_markdown(capsys: pytest.CaptureF assert exit_code == 0 out = capsys.readouterr().out assert "skipping SpecFact code review" in out + assert ".py/.pyi" in out + + +def test_main_warnings_only_does_not_block_despite_cli_fail_exit( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Pre-commit gate blocks on error findings only; warning-only FAIL verdict is advisory.""" + module = _load_script_module() + repo_root = tmp_path + payload: dict[str, object] = { + "overall_verdict": "FAIL", + "findings": [{"severity": "warning", "rule": "w1"}], + } + _write_sample_review_report(repo_root, payload) + + def _fake_root() -> Path: + return repo_root + + def _fake_ensure() -> tuple[bool, str | None]: + return True, None + + def _fake_run(cmd: list[str], **_kwargs: object) -> subprocess.CompletedProcess[str]: + _write_sample_review_report(repo_root, payload) + return subprocess.CompletedProcess(cmd, 1, stdout="", stderr="") + + monkeypatch.setattr(module, "_repo_root", _fake_root) + monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) + monkeypatch.setattr(module.subprocess, "run", _fake_run) + + exit_code = module.main(["tests/unit/test_app.py"]) + err = capsys.readouterr().err + assert exit_code == 0 + assert "warnings=1" in err def test_main_propagates_review_gate_exit_code( @@ -111,11 +159,11 @@ def _fake_root() -> Path: def _fake_ensure() -> tuple[bool, str | None]: return True, None - def _fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + def _fake_run(cmd: list[str], **_kwargs: object) -> subprocess.CompletedProcess[str]: assert "--json" in cmd assert module.REVIEW_JSON_OUT in cmd - assert kwargs.get("cwd") == str(repo_root) - assert kwargs.get("timeout") == 300 + assert _kwargs.get("cwd") == str(repo_root) + assert _kwargs.get("timeout") == 300 _write_sample_review_report(repo_root, SAMPLE_FAIL_REVIEW_REPORT) return subprocess.CompletedProcess(cmd, 1, stdout=".specfact/code-review.json\n", stderr="") @@ -198,9 +246,9 @@ def test_main_timeout_fails_hook(monkeypatch: pytest.MonkeyPatch, capsys: pytest def _fake_ensure() -> tuple[bool, str | None]: return True, None - def _fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: - assert kwargs.get("cwd") == str(repo_root) - assert kwargs.get("timeout") == 300 + def _fake_run(cmd: list[str], **_kwargs: object) -> subprocess.CompletedProcess[str]: + assert _kwargs.get("cwd") == str(repo_root) + assert _kwargs.get("timeout") == 300 raise subprocess.TimeoutExpired(cmd, 300) monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) diff --git a/tests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.py b/tests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.py new file mode 100644 index 0000000..a73114a --- /dev/null +++ b/tests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.py @@ -0,0 +1,5 @@ +import icontract as _icontract_presence_only # noqa: F401 # pyright: ignore[reportUnusedImport] + + +def public_without_contracts(value: int) -> int: + return value + 1 diff --git a/tests/unit/specfact_code_review/run/test___init__.py b/tests/unit/specfact_code_review/run/test___init__.py new file mode 100644 index 0000000..69f96ad --- /dev/null +++ b/tests/unit/specfact_code_review/run/test___init__.py @@ -0,0 +1,14 @@ +"""Smoke tests for lazy `specfact_code_review.run` exports.""" + +from __future__ import annotations + +import specfact_code_review.run as run_pkg + + +def test_run_package_exports_run_review() -> None: + assert callable(run_pkg.run_review) + + +def test_all_exports_are_defined() -> None: + for name in run_pkg.__all__: + assert hasattr(run_pkg, name) diff --git a/tests/unit/specfact_code_review/run/test_runner.py b/tests/unit/specfact_code_review/run/test_runner.py index 2bb18fb..c776939 100644 --- a/tests/unit/specfact_code_review/run/test_runner.py +++ b/tests/unit/specfact_code_review/run/test_runner.py @@ -63,10 +63,14 @@ def _record(name: str) -> list[ReviewFinding]: monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: _record("ruff")) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: _record("radon")) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: _record("semgrep")) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: _record("semgrep_bugs")) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: _record("ast")) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: _record("basedpyright")) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: _record("pylint")) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: _record("contracts")) + monkeypatch.setattr( + "specfact_code_review.run.runner.run_contract_check", + lambda files, **_: _record("contracts"), + ) monkeypatch.setattr( "specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: ( @@ -78,7 +82,17 @@ def _record(name: str) -> list[ReviewFinding]: report = run_review([Path("packages/specfact-code-review/src/specfact_code_review/run/scorer.py")]) assert isinstance(report, ReviewReport) - assert calls == ["ruff", "radon", "semgrep", "ast", "basedpyright", "pylint", "contracts", "testing"] + assert calls == [ + "ruff", + "radon", + "semgrep", + "semgrep_bugs", + "ast", + "basedpyright", + "pylint", + "contracts", + "testing", + ] def test_run_review_merges_findings_from_all_runners(monkeypatch: MonkeyPatch) -> None: @@ -90,6 +104,10 @@ def test_run_review_merges_findings_from_all_runners(monkeypatch: MonkeyPatch) - "specfact_code_review.run.runner.run_semgrep", lambda files: [_finding(tool="semgrep", rule="cross-layer-call", category="architecture")], ) + monkeypatch.setattr( + "specfact_code_review.run.runner.run_semgrep_bugs", + lambda files: [_finding(tool="semgrep", rule="specfact-bugs-eval-exec", category="security")], + ) monkeypatch.setattr( "specfact_code_review.run.runner.run_ast_clean_code", lambda files: [_finding(tool="ast", rule="dry.duplicate-function-shape", category="dry")], @@ -104,7 +122,7 @@ def test_run_review_merges_findings_from_all_runners(monkeypatch: MonkeyPatch) - ) monkeypatch.setattr( "specfact_code_review.run.runner.run_contract_check", - lambda files: [_finding(tool="contract_runner", rule="MISSING_ICONTRACT", category="contracts")], + lambda files, **_: [_finding(tool="contract_runner", rule="MISSING_ICONTRACT", category="contracts")], ) monkeypatch.setattr( "specfact_code_review.run.runner._evaluate_tdd_gate", @@ -120,6 +138,7 @@ def test_run_review_merges_findings_from_all_runners(monkeypatch: MonkeyPatch) - "ruff", "radon", "semgrep", + "semgrep", "ast", "basedpyright", "pylint", @@ -141,10 +160,11 @@ def test_run_review_skips_tdd_gate_when_no_tests_is_true(monkeypatch: MonkeyPatc monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: []) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files, **_: []) monkeypatch.setattr( "specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: (_ for _ in ()).throw(AssertionError("_evaluate_tdd_gate should not be called")), @@ -162,10 +182,11 @@ def test_run_review_returns_review_report(monkeypatch: MonkeyPatch) -> None: monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: []) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files, **_: []) monkeypatch.setattr( "specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: ([], {"packages/specfact-code-review/src/specfact_code_review/run/scorer.py": 95.0}), @@ -213,10 +234,14 @@ def test_run_review_suppresses_known_test_noise_by_default(monkeypatch: MonkeyPa monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: noisy_findings[2:]) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: noisy_findings[1:2]) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: noisy_findings[:1]) + monkeypatch.setattr( + "specfact_code_review.run.runner.run_contract_check", + lambda files, **_: noisy_findings[:1], + ) monkeypatch.setattr("specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: ([], None)) report = run_review([Path("tests/unit/specfact_code_review/run/test_commands.py")], no_tests=True) @@ -250,10 +275,14 @@ def test_run_review_can_include_known_test_noise(monkeypatch: MonkeyPatch) -> No monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: noisy_findings[1:]) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: noisy_findings[:1]) + monkeypatch.setattr( + "specfact_code_review.run.runner.run_contract_check", + lambda files, **_: noisy_findings[:1], + ) monkeypatch.setattr("specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: ([], None)) report = run_review( @@ -269,10 +298,11 @@ def test_run_review_emits_advisory_checklist_finding_in_pr_mode(monkeypatch: Mon monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: []) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files, **_: []) monkeypatch.setattr("specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: ([], None)) monkeypatch.setenv("SPECFACT_CODE_REVIEW_PR_MODE", "true") monkeypatch.setenv("SPECFACT_CODE_REVIEW_PR_TITLE", "Expand code review coverage") @@ -291,10 +321,11 @@ def test_run_review_requires_explicit_pr_mode_token_for_clean_code_reasoning(mon monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: []) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files, **_: []) monkeypatch.setattr("specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: ([], None)) monkeypatch.setenv("SPECFACT_CODE_REVIEW_PR_MODE", "true") monkeypatch.setenv("SPECFACT_CODE_REVIEW_PR_TITLE", "Expand code review coverage") @@ -320,10 +351,11 @@ def test_run_review_suppresses_global_duplicate_code_noise_by_default(monkeypatc monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: [duplicate_code_finding]) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files, **_: []) monkeypatch.setattr("specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: ([], None)) report = run_review([Path("scripts/link_dev_module.py")], no_tests=True) @@ -374,10 +406,11 @@ def test_run_review_can_include_global_duplicate_code_noise(monkeypatch: MonkeyP monkeypatch.setattr("specfact_code_review.run.runner.run_ruff", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_radon", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_semgrep_bugs", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_ast_clean_code", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_basedpyright", lambda files: []) monkeypatch.setattr("specfact_code_review.run.runner.run_pylint", lambda files: [duplicate_code_finding]) - monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files: []) + monkeypatch.setattr("specfact_code_review.run.runner.run_contract_check", lambda files, **_: []) monkeypatch.setattr("specfact_code_review.run.runner._evaluate_tdd_gate", lambda files: ([], None)) report = run_review([Path("scripts/link_dev_module.py")], no_tests=True, include_noise=True) diff --git a/tests/unit/specfact_code_review/test__review_utils.py b/tests/unit/specfact_code_review/test__review_utils.py index d886eb4..989c618 100644 --- a/tests/unit/specfact_code_review/test__review_utils.py +++ b/tests/unit/specfact_code_review/test__review_utils.py @@ -2,7 +2,7 @@ from pathlib import Path -from specfact_code_review._review_utils import normalize_path_variants, tool_error +from specfact_code_review._review_utils import normalize_path_variants, python_source_paths_for_tools, tool_error def test_normalize_path_variants_includes_relative_and_resolved_paths(tmp_path: Path) -> None: @@ -16,6 +16,15 @@ def test_normalize_path_variants_includes_relative_and_resolved_paths(tmp_path: assert file_path.resolve().as_posix() in variants +def test_python_source_paths_for_tools_keeps_only_py_suffix(tmp_path: Path) -> None: + py_file = tmp_path / "a.py" + yaml_file = tmp_path / "module-package.yaml" + py_file.write_text("x = 1\n", encoding="utf-8") + yaml_file.write_text("name: t\n", encoding="utf-8") + + assert python_source_paths_for_tools([py_file, yaml_file]) == [py_file] + + def test_tool_error_returns_review_finding_defaults(tmp_path: Path) -> None: file_path = tmp_path / "example.py" file_path.write_text("VALUE = 1\n", encoding="utf-8") diff --git a/tests/unit/specfact_code_review/test_review_tool_pip_manifest.py b/tests/unit/specfact_code_review/test_review_tool_pip_manifest.py new file mode 100644 index 0000000..abca519 --- /dev/null +++ b/tests/unit/specfact_code_review/test_review_tool_pip_manifest.py @@ -0,0 +1,21 @@ +"""Guard: code-review pip_dependencies cover all external review tools.""" + +from __future__ import annotations + +from pathlib import Path + +import yaml + +from specfact_code_review.tools.tool_availability import REVIEW_TOOL_PIP_PACKAGES + + +REPO_ROOT = Path(__file__).resolve().parents[3] +MODULE_PACKAGE = REPO_ROOT / "packages" / "specfact-code-review" / "module-package.yaml" + + +def test_module_package_lists_all_review_tool_pip_packages() -> None: + data = yaml.safe_load(MODULE_PACKAGE.read_text(encoding="utf-8")) + pip_deps: list[str] = data["pip_dependencies"] + declared = set(pip_deps) + for _tool_id, pip_name in REVIEW_TOOL_PIP_PACKAGES.items(): + assert pip_name in declared, f"Add {pip_name!r} to specfact-code-review module-package.yaml pip_dependencies" 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 52ac4e6..db2e500 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,18 @@ def test_run_basedpyright_returns_empty_for_no_files() -> None: - assert run_basedpyright([]) == [] + assert not run_basedpyright([]) + + +def test_run_basedpyright_skips_yaml_manifests(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + manifest = tmp_path / "module-package.yaml" + manifest.write_text("name: example\nversion: 1\n", encoding="utf-8") + run_mock = Mock() + monkeypatch.setattr(subprocess, "run", run_mock) + + assert not run_basedpyright([manifest]) + + run_mock.assert_not_called() def test_run_basedpyright_maps_error_diagnostic_to_type_safety(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: 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 cccf1fd..8c25bc9 100644 --- a/tests/unit/specfact_code_review/tools/test_contract_runner.py +++ b/tests/unit/specfact_code_review/tools/test_contract_runner.py @@ -1,9 +1,11 @@ from __future__ import annotations +import shutil import subprocess from pathlib import Path from unittest.mock import Mock +import pytest from pytest import MonkeyPatch from specfact_code_review.tools.contract_runner import _skip_icontract_ast_scan, run_contract_check @@ -13,18 +15,27 @@ FIXTURES_DIR = Path(__file__).resolve().parent.parent / "fixtures" / "contract_runner" -def test_run_contract_check_reports_public_function_without_contracts(monkeypatch: MonkeyPatch) -> None: +@pytest.fixture(autouse=True) +def _stub_crosshair_on_path(monkeypatch: MonkeyPatch) -> None: # pyright: ignore[reportUnusedFunction] + """So skip_if_tool_missing does not short-circuit before mocked subprocess.run.""" + real_which = shutil.which + + def _which(name: str) -> str | None: + if name == "crosshair": + return "/fake/crosshair" + return real_which(name) + + 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" run_mock = Mock(return_value=completed_process("crosshair", stdout="")) monkeypatch.setattr(subprocess, "run", run_mock) findings = run_contract_check([file_path]) - assert len(findings) == 1 - assert findings[0].category == "contracts" - assert findings[0].rule == "MISSING_ICONTRACT" - assert findings[0].file == str(file_path) - assert findings[0].line == 1 + assert not findings assert_tool_run(run_mock, ["crosshair", "check", "--per_path_timeout", "2", str(file_path)]) @@ -88,7 +99,7 @@ def test_run_contract_check_ignores_crosshair_timeout(monkeypatch: MonkeyPatch) def test_run_contract_check_reports_unavailable_crosshair_but_keeps_ast_findings(monkeypatch: MonkeyPatch) -> None: - file_path = FIXTURES_DIR / "public_without_contracts.py" + file_path = FIXTURES_DIR / "public_missing_contract_but_icontract_imported.py" monkeypatch.setattr(subprocess, "run", Mock(side_effect=FileNotFoundError("crosshair not found"))) findings = run_contract_check([file_path]) 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 6a70133..ce0c248 100644 --- a/tests/unit/specfact_code_review/tools/test_radon_runner.py +++ b/tests/unit/specfact_code_review/tools/test_radon_runner.py @@ -11,6 +11,17 @@ from tests.unit.specfact_code_review.tools.helpers import assert_tool_run, completed_process, create_noisy_file +def test_run_radon_returns_empty_when_only_non_python_paths(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + manifest = tmp_path / "module-package.yaml" + manifest.write_text("name: example\n", encoding="utf-8") + run_mock = Mock() + monkeypatch.setattr(subprocess, "run", run_mock) + + assert not run_radon([manifest]) + + run_mock.assert_not_called() + + def test_run_radon_maps_complexity_thresholds_and_filters_files(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" other_path = tmp_path / "other.py" diff --git a/tests/unit/specfact_code_review/tools/test_tool_availability.py b/tests/unit/specfact_code_review/tools/test_tool_availability.py new file mode 100644 index 0000000..d047410 --- /dev/null +++ b/tests/unit/specfact_code_review/tools/test_tool_availability.py @@ -0,0 +1,44 @@ +"""Unit tests for review tool PATH / import skip helpers.""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import patch + +from specfact_code_review.tools.tool_availability import ( + REVIEW_TOOL_PIP_PACKAGES, + skip_if_pytest_unavailable, + skip_if_tool_missing, +) + + +def test_skip_if_tool_missing_empty_when_executable_present() -> None: + with patch("specfact_code_review.tools.tool_availability.shutil.which", return_value="/usr/bin/ruff"): + assert skip_if_tool_missing("ruff", Path("x.py")) == [] + + +def test_skip_if_tool_missing_finds_single_finding_when_absent() -> None: + with patch("specfact_code_review.tools.tool_availability.shutil.which", return_value=None): + findings = skip_if_tool_missing("ruff", Path("src/m.py")) + assert len(findings) == 1 + assert findings[0].tool == "ruff" + assert "ruff" in findings[0].message.lower() + + +def test_skip_if_pytest_unavailable_when_pytest_missing() -> None: + with patch("importlib.util.find_spec", return_value=None): + findings = skip_if_pytest_unavailable(Path("tests/test_x.py")) + assert len(findings) == 1 + assert findings[0].tool == "pytest" + + +def test_review_tool_pip_packages_covers_each_tool_id() -> None: + assert set(REVIEW_TOOL_PIP_PACKAGES) == { + "ruff", + "radon", + "semgrep", + "basedpyright", + "pylint", + "crosshair", + "pytest", + } diff --git a/tests/unit/specfact_codebase/test_sidecar_framework_extractors.py b/tests/unit/specfact_codebase/test_sidecar_framework_extractors.py new file mode 100644 index 0000000..70f407b --- /dev/null +++ b/tests/unit/specfact_codebase/test_sidecar_framework_extractors.py @@ -0,0 +1,84 @@ +"""Tests for sidecar framework extractors (path exclusions).""" + +from __future__ import annotations + +from pathlib import Path + +from specfact_codebase.validators.sidecar.frameworks.fastapi import FastAPIExtractor + + +def _fake_fastapi_main() -> str: + return """ +from fastapi import FastAPI +app = FastAPI() + +@app.get("/real") +def real(): + return {"ok": True} +""" + + +def test_fastapi_extractor_resolves_api_route_methods(tmp_path: Path) -> None: + """api_route(methods=[...]) should yield a canonical HTTP verb, not the decorator name.""" + (tmp_path / "routes.py").write_text( + """ +from fastapi import APIRouter +router = APIRouter() + +@router.api_route("/multi", methods=["GET", "POST"]) +def multi(): + return {"ok": True} +""", + encoding="utf-8", + ) + 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" + + +def test_fastapi_extractor_ignores_non_http_decorators(tmp_path: Path) -> None: + """Middleware-style decorators must not overwrite method with bogus verb names.""" + (tmp_path / "app.py").write_text( + """ +from fastapi import FastAPI +app = FastAPI() + +@app.middleware("http") +@app.get("/ok") +def ok(): + return {"ok": True} +""", + encoding="utf-8", + ) + extractor = FastAPIExtractor() + routes = extractor.extract_routes(tmp_path) + match = next((r for r in routes if r.path == "/ok"), None) + assert match is not None + assert match.method == "GET" + + +def test_fastapi_extractor_ignores_specfact_venv_routes(tmp_path: Path) -> None: + """Routes under .specfact/venv must not be counted (sidecar installs deps there).""" + (tmp_path / "main.py").write_text(_fake_fastapi_main(), encoding="utf-8") + + venv_app = tmp_path / ".specfact" / "venv" / "lib" / "site-packages" / "fastapi_app" + venv_app.mkdir(parents=True) + (venv_app / "noise.py").write_text( + """ +from fastapi import FastAPI +app = FastAPI() + +@app.get("/ghost-from-venv") +def ghost(): + return {} +""", + encoding="utf-8", + ) + + extractor = FastAPIExtractor() + routes = extractor.extract_routes(tmp_path) + paths = {route.path for route in routes} + assert "/real" in paths + assert "/ghost-from-venv" not in paths diff --git a/tests/unit/test_git_branch_module_signature_flag_script.py b/tests/unit/test_git_branch_module_signature_flag_script.py new file mode 100644 index 0000000..3b288e2 --- /dev/null +++ b/tests/unit/test_git_branch_module_signature_flag_script.py @@ -0,0 +1,15 @@ +from __future__ import annotations + +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT_PATH = REPO_ROOT / "scripts" / "git-branch-module-signature-flag.sh" + + +def test_git_branch_module_signature_flag_script_documents_cli_parity() -> None: + text = SCRIPT_PATH.read_text(encoding="utf-8") + assert "specfact-cli" in text + assert "GITHUB_BASE_REF" in text + assert '"require"' in text + assert "omit" in text diff --git a/tests/unit/test_pre_commit_verify_modules_signature_script.py b/tests/unit/test_pre_commit_verify_modules_signature_script.py index a2a31d7..8263d8b 100644 --- a/tests/unit/test_pre_commit_verify_modules_signature_script.py +++ b/tests/unit/test_pre_commit_verify_modules_signature_script.py @@ -6,22 +6,23 @@ REPO_ROOT = Path(__file__).resolve().parents[2] -def test_pre_commit_verify_modules_signature_script_matches_ci_branch_policy() -> None: - text = (REPO_ROOT / "scripts" / "pre-commit-verify-modules-signature.sh").read_text(encoding="utf-8") - assert "git rev-parse --abbrev-ref HEAD" in text - assert "GITHUB_BASE_REF" in text - assert "_branch" in text - assert "_require_signature" in text - assert '== "main"' in text +def test_pre_commit_verify_modules_signature_script_matches_cli_shape() -> None: + text = (REPO_ROOT / "scripts/pre-commit-verify-modules-signature.sh").read_text(encoding="utf-8") + assert "git-branch-module-signature-flag.sh" in text + assert 'case "${sig_policy}" in' in text + assert "require)" in text + assert "omit)" in text assert "--payload-from-filesystem" in text assert "--enforce-version-bump" in text assert "verify-modules-signature.py" in text + assert "--metadata-only" in text - marker = 'if [[ "$_require_signature" == true ]]; then' + marker = 'case "${sig_policy}" in' assert marker in text _head, tail = text.split(marker, 1) assert "--require-signature" not in _head - true_part, from_else = tail.split("else", 1) - false_part = from_else.split("fi", 1)[0] - assert "--require-signature" in true_part - assert "--require-signature" not in false_part + require_block = tail.split("omit)", 1)[0] + assert "--require-signature" in require_block + omit_block = tail.split("omit)", 1)[1].split("*)", 1)[0] + assert "--require-signature" not in omit_block + assert "--metadata-only" in omit_block diff --git a/tests/unit/test_verify_modules_signature_script.py b/tests/unit/test_verify_modules_signature_script.py index 683b042..eb25349 100644 --- a/tests/unit/test_verify_modules_signature_script.py +++ b/tests/unit/test_verify_modules_signature_script.py @@ -68,3 +68,72 @@ def test_verify_manifest_falls_back_to_filesystem_payload_when_checksum_matches( ) assert verification_mode == "filesystem" + + +def test_verify_manifest_integrity_shape_only_accepts_checksum_only_manifest(tmp_path: Path) -> None: + verify_script = _load_verify_script() + module_dir = tmp_path / "packages" / "specfact-example" + module_dir.mkdir(parents=True) + manifest_path = module_dir / "module-package.yaml" + manifest_path.write_text( + yaml.safe_dump( + { + "name": "nold-ai/specfact-example", + "version": "0.1.0", + "integrity": { + "checksum": "sha256:" + "a" * 64, + }, + }, + sort_keys=False, + ), + encoding="utf-8", + ) + verify_script.verify_manifest_integrity_shape_only(manifest_path, require_signature=False) + + +def test_verify_manifest_integrity_shape_only_rejects_bad_checksum_format(tmp_path: Path) -> None: + verify_script = _load_verify_script() + module_dir = tmp_path / "packages" / "specfact-example" + module_dir.mkdir(parents=True) + manifest_path = module_dir / "module-package.yaml" + manifest_path.write_text( + yaml.safe_dump( + { + "name": "nold-ai/specfact-example", + "version": "0.1.0", + "integrity": {"checksum": "not-a-valid-checksum"}, + }, + sort_keys=False, + ), + encoding="utf-8", + ) + try: + verify_script.verify_manifest_integrity_shape_only(manifest_path, require_signature=False) + except ValueError as exc: + assert "checksum" in str(exc).lower() + else: + raise AssertionError("expected ValueError") + + +def test_verify_manifest_integrity_shape_only_enforces_signature_when_requested(tmp_path: Path) -> None: + verify_script = _load_verify_script() + module_dir = tmp_path / "packages" / "specfact-example" + module_dir.mkdir(parents=True) + manifest_path = module_dir / "module-package.yaml" + manifest_path.write_text( + yaml.safe_dump( + { + "name": "nold-ai/specfact-example", + "version": "0.1.0", + "integrity": {"checksum": "sha256:" + "b" * 64}, + }, + sort_keys=False, + ), + encoding="utf-8", + ) + try: + verify_script.verify_manifest_integrity_shape_only(manifest_path, require_signature=True) + except ValueError as exc: + assert "signature" in str(exc).lower() + else: + raise AssertionError("expected ValueError") diff --git a/tests/unit/workflows/test_pr_orchestrator_signing.py b/tests/unit/workflows/test_pr_orchestrator_signing.py index fd81093..d3a0037 100644 --- a/tests/unit/workflows/test_pr_orchestrator_signing.py +++ b/tests/unit/workflows/test_pr_orchestrator_signing.py @@ -6,21 +6,43 @@ REPO_ROOT = Path(__file__).resolve().parents[3] -def test_pr_orchestrator_verify_splits_signature_requirement_by_target_branch() -> None: - workflow = (REPO_ROOT / ".github" / "workflows" / "pr-orchestrator.yml").read_text(encoding="utf-8") +def _workflow_text() -> str: + return (REPO_ROOT / ".github" / "workflows" / "pr-orchestrator.yml").read_text(encoding="utf-8") + +def test_pr_orchestrator_verify_has_core_verifier_flags() -> None: + workflow = _workflow_text() assert "verify-module-signatures" in workflow assert "scripts/verify-modules-signature.py" in workflow assert "--payload-from-filesystem" in workflow assert "--enforce-version-bump" in workflow assert "github.event.pull_request.base.ref" in workflow assert "TARGET_BRANCH" in workflow - assert "GITHUB_REF#refs/heads/" in workflow or "${GITHUB_REF#refs/heads/}" in workflow - branch_guard = 'if [ "$TARGET_BRANCH" = "main" ]; then' + assert "github.ref_name" in workflow + assert "VERIFY_CMD" in workflow + + +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 + assert metadata_append in workflow + assert workflow.index(dev_guard) < workflow.index(metadata_append) + + +def test_pr_orchestrator_verify_require_signature_on_main_paths() -> None: + workflow = _workflow_text() + main_ref_guard = '[ "${{ github.ref_name }}" = "main" ]; then' require_append = "VERIFY_CMD+=(--require-signature)" - assert branch_guard in workflow + assert main_ref_guard in workflow assert require_append in workflow - assert workflow.index(branch_guard) < workflow.index(require_append) - assert '[ "$TARGET_BRANCH" = "main" ]' in workflow + assert workflow.count(require_append) == 2 + push_require_block = ( + 'if [ "${{ github.ref_name }}" = "main" ]; then\n VERIFY_CMD+=(--require-signature)' + ) + assert push_require_block in workflow assert "--require-signature" in workflow - assert "VERIFY_CMD" in workflow diff --git a/tests/unit/workflows/test_sign_modules_on_approval.py b/tests/unit/workflows/test_sign_modules_on_approval.py index c4d0cf4..bbd342e 100644 --- a/tests/unit/workflows/test_sign_modules_on_approval.py +++ b/tests/unit/workflows/test_sign_modules_on_approval.py @@ -52,14 +52,28 @@ def _assert_pull_request_review_submitted(doc: dict[Any, Any]) -> None: assert pr_review["types"] == ["submitted"] -def _assert_sign_job_branch_filters(doc: dict[Any, Any]) -> None: +def _assert_sign_job_has_no_top_level_if(doc: dict[Any, Any]) -> None: job = _sign_modules_job(doc) - job_if = job["if"] - assert isinstance(job_if, str) - assert "github.event.review.state == 'approved'" in job_if - assert "github.event.pull_request.base.ref == 'dev'" in job_if - assert "github.event.pull_request.base.ref == 'main'" in job_if - assert "github.event.pull_request.head.repo.full_name == github.repository" in job_if + assert "if" not in job, "Job-level `if` prevents a stable required check; gating belongs in steps" + + +def _assert_eligibility_gate_step(doc: dict[Any, Any]) -> None: + job = _sign_modules_job(doc) + steps = job["steps"] + assert isinstance(steps, list) + gate = steps[0] + assert isinstance(gate, dict) + assert gate.get("name") == "Eligibility gate (required status check)" + assert gate.get("id") == "gate" + run = gate["run"] + assert isinstance(run, str) + assert "github.event.review.state" in run + assert "approved" in run + assert 'echo "sign=false"' in run + assert 'echo "sign=true"' in run + assert "github.event.pull_request.base.ref" in run + assert "github.event.pull_request.head.repo.full_name" in run + assert "github.repository" in run def _assert_concurrency_and_permissions(doc: dict[Any, Any]) -> None: @@ -76,7 +90,8 @@ def _assert_concurrency_and_permissions(doc: dict[Any, Any]) -> None: def test_sign_modules_on_approval_trigger_and_job_filter() -> None: doc = _parsed_workflow() _assert_pull_request_review_submitted(doc) - _assert_sign_job_branch_filters(doc) + _assert_sign_job_has_no_top_level_if(doc) + _assert_eligibility_gate_step(doc) _assert_concurrency_and_permissions(doc) @@ -116,7 +131,7 @@ def test_sign_modules_on_approval_secrets_guard() -> None: def test_sign_modules_on_approval_sign_step_merge_base() -> None: workflow = _workflow_text() - assert "MERGE_BASE=" in workflow + assert "merge-base" in workflow assert "git merge-base HEAD" in workflow assert 'git fetch origin "${PR_BASE_REF}"' in workflow assert "--no-tags" in workflow @@ -126,6 +141,8 @@ def test_sign_modules_on_approval_sign_step_merge_base() -> None: assert '"$MERGE_BASE"' in workflow assert "--bump-version patch" in workflow assert "--payload-from-filesystem" in workflow + assert "steps.gate.outputs.sign == 'true'" in workflow + assert '--base-ref "origin/' not in workflow def _assert_discover_step_writes_outputs(steps: list[Any]) -> None: @@ -151,8 +168,9 @@ def _assert_job_summary_step(steps: list[Any]) -> None: assert summary.get("if") == "always()" env = summary["env"] assert isinstance(env, dict) - assert env["COMMIT_CHANGED"] == "${{ steps.commit.outputs.changed }}" - assert env["MANIFESTS_COUNT"] == "${{ steps.discover.outputs.manifests_count }}" + assert env["COMMIT_CHANGED"] == "${{ steps.commit.outputs.changed || '' }}" + assert env["MANIFESTS_COUNT"] == "${{ steps.discover.outputs.manifests_count || '' }}" + assert env["GATE_SIGN"] == "${{ steps.gate.outputs.sign }}" summary_run = summary["run"] assert isinstance(summary_run, str) assert "GITHUB_STEP_SUMMARY" in summary_run