diff --git a/.github/workflows/pr-orchestrator.yml b/.github/workflows/pr-orchestrator.yml index a8638d9b..4c14a6ad 100644 --- a/.github/workflows/pr-orchestrator.yml +++ b/.github/workflows/pr-orchestrator.yml @@ -80,29 +80,25 @@ 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" = "main" ]; 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/publish-modules.yml b/.github/workflows/publish-modules.yml index 2a94aada..9b8ff5ca 100644 --- a/.github/workflows/publish-modules.yml +++ b/.github/workflows/publish-modules.yml @@ -26,8 +26,10 @@ concurrency: jobs: publish: - if: github.actor != 'github-actions[bot]' runs-on: ubuntu-latest + env: + SPECFACT_MODULE_PRIVATE_SIGN_KEY: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY }} + SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE }} steps: - name: Checkout uses: actions/checkout@v4 @@ -42,7 +44,9 @@ jobs: - name: Install publish dependencies run: | python -m pip install --upgrade pip - python -m pip install pyyaml packaging + # cryptography/cffi: required when the publish step invokes scripts/sign-modules.py + # to add integrity.signature before packaging (same stack as sign-modules.yml). + python -m pip install pyyaml packaging cryptography cffi - name: Resolve publish bundle set id: bundles @@ -197,6 +201,37 @@ jobs: ignored_dir_names = {".git", "tests", "__pycache__", ".pytest_cache", ".mypy_cache", ".ruff_cache", "logs"} ignored_suffixes = {".pyc", ".pyo"} + def manifest_has_signature(data: dict) -> bool: + integrity_obj = data.get("integrity") + if not isinstance(integrity_obj, dict): + return False + return bool(str(integrity_obj.get("signature") or "").strip()) + + signing_key = os.environ.get("SPECFACT_MODULE_PRIVATE_SIGN_KEY", "").strip() + + def sign_manifest_if_unsigned(manifest_path: Path, *, reason: str) -> None: + if not signing_key: + return + if not manifest_path.is_file(): + return + raw = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + if not isinstance(raw, dict): + return + if manifest_has_signature(raw): + return + print(f"Signing {manifest_path} ({reason}).", flush=True) + subprocess.run( + [ + "python", + "scripts/sign-modules.py", + "--payload-from-filesystem", + "--allow-same-version", + str(manifest_path), + ], + cwd=str(repo_root), + check=True, + ) + skipped_bundles: list[str] = [] current_branch = os.environ.get("GITHUB_REF_NAME", "").strip() baseline_ref = determine_registry_baseline_ref( @@ -260,7 +295,24 @@ jobs: manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) if not isinstance(manifest, dict): - raise ValueError(f"Invalid manifest content: {manifest_path}") + raise ValueError(f"Invalid manifest content: {manifest_path}") + + if not manifest_has_signature(manifest): + sign_manifest_if_unsigned( + manifest_path, + reason="missing integrity.signature before registry packaging", + ) + manifest = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + if not isinstance(manifest, dict): + raise ValueError(f"Invalid manifest content after signing: {manifest_path}") + if signing_key and not manifest_has_signature(manifest): + raise ValueError(f"Signing did not produce integrity.signature: {manifest_path}") + if not signing_key: + print( + f"::warning::Publishing {bundle} with checksum-only tree manifest " + "(SPECFACT_MODULE_PRIVATE_SIGN_KEY unset).", + flush=True, + ) module_id = str(manifest.get("name") or f"nold-ai/{bundle}") version = str(manifest.get("version") or "").strip() @@ -320,6 +372,18 @@ jobs: if skipped_bundles: print(f"Skipped already-published bundles: {skipped_bundles}") + for bundle in skipped_bundles: + sign_manifest_if_unsigned( + repo_root / "packages" / bundle / "module-package.yaml", + reason="registry version already published; still align git manifest signature", + ) + + for manifest_path in sorted((repo_root / "packages").glob("*/module-package.yaml")): + sign_manifest_if_unsigned( + manifest_path, + reason="final pass: ensure no unsigned module-package.yaml remains before commit", + ) + registry_index_path.write_text(json.dumps(registry, indent=2) + "\n", encoding="utf-8") PY @@ -330,8 +394,8 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} BUNDLE_REASONS_JSON: ${{ steps.bundles.outputs.bundle_reasons_json }} run: | - if git diff --quiet -- registry/index.json registry/modules registry/signatures; then - echo "No registry changes to commit." + if git diff --quiet -- registry/index.json registry/modules registry/signatures && git diff --quiet -- packages/; then + echo "No registry or signed package manifest changes to commit." exit 0 fi @@ -342,7 +406,10 @@ jobs: PUBLISH_BRANCH="auto/publish-${TARGET_BRANCH}-${GITHUB_RUN_ID}" git checkout -b "${PUBLISH_BRANCH}" + # Registry artifacts plus any module-package.yaml updates from in-workflow signing + # (otherwise dev→main PRs fail verify-modules-signature --require-signature). git add registry/index.json registry/modules registry/signatures + git add -u packages/ git commit -m "chore(registry): publish changed modules [skip ci]" if [ "${DRY_RUN:-false}" = "true" ]; then diff --git a/.github/workflows/sign-modules-on-approval.yml b/.github/workflows/sign-modules-on-approval.yml index c6d71bb7..33f45915 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,43 @@ 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 + author_association="${{ github.event.review.user.author_association }}" + case "$author_association" in + COLLABORATOR|MEMBER|OWNER) + ;; + *) + echo "sign=false" >> "$GITHUB_OUTPUT" + echo "::notice::Skipping module signing: reviewer association '${author_association}' is not trusted for signing." + exit 0 + ;; + esac + base_ref="${{ github.event.pull_request.base.ref }}" + if [ "$base_ref" != "dev" ] && [ "$base_ref" != "main" ]; then + echo "sign=false" >> "$GITHUB_OUTPUT" + 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 by trusted reviewer, 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 +70,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 +97,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 +110,7 @@ jobs: --payload-from-filesystem - name: Commit and push signed manifests + if: steps.gate.outputs.sign == 'true' id: commit run: | set -euo pipefail @@ -84,7 +122,7 @@ jobs: exit 0 fi git add -u -- packages/ - git commit -m "chore(modules): ci sign changed modules [skip ci]" + git commit -m "chore(modules): ci sign changed modules" echo "changed=true" >> "$GITHUB_OUTPUT" if ! git push origin "HEAD:${PR_HEAD_REF}"; then echo "::error::Push to ${PR_HEAD_REF} failed (branch may have advanced after the approved commit). Update the PR branch and re-approve if signing is still required." @@ -94,15 +132,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/.github/workflows/sign-modules.yml b/.github/workflows/sign-modules.yml new file mode 100644 index 00000000..daa26d39 --- /dev/null +++ b/.github/workflows/sign-modules.yml @@ -0,0 +1,375 @@ +# yaml-language-server: $schema=https://json.schemastore.org/github-workflow.json +# Auto-sign changed module manifests on push to dev/main, then strict-verify. PRs use full payload +# checksum + version bump without `--require-signature` until `main`. +name: Module Signature Hardening + +on: + workflow_dispatch: + inputs: + base_branch: + description: Remote branch to compare for --changed-only (fetches origin/) + type: choice + options: + - dev + - main + default: dev + version_bump: + description: Auto-bump when module version is still unchanged from the base ref + type: choice + options: + - patch + - minor + - major + default: patch + resign_all_manifests: + description: Sign every packages/*/module-package.yaml (not only --changed-only vs base). Use when manifests match the base but lack signatures. + type: boolean + default: false + push: + branches: [dev, main] + paths: + - "packages/**" + # Registry-only publish merges do not touch packages/**; still run signing so git manifests + # stay aligned with dev→main --require-signature checks. + - "registry/**" + - "scripts/sign-modules.py" + - "scripts/verify-modules-signature.py" + - ".github/workflows/sign-modules.yml" + - ".github/workflows/sign-modules-on-approval.yml" + pull_request: + branches: [dev, main] + paths: + - "packages/**" + - "registry/**" + - "scripts/sign-modules.py" + - "scripts/verify-modules-signature.py" + - ".github/workflows/sign-modules.yml" + - ".github/workflows/sign-modules-on-approval.yml" + +concurrency: + group: sign-modules-${{ github.ref }} + cancel-in-progress: false + +jobs: + verify: + name: Verify Module Signatures + runs-on: ubuntu-latest + outputs: + # Skip reproducibility when we only opened a PR: origin/main is still unsigned until merge. + opened_sign_pr: ${{ steps.commit_auto_sign.outputs.opened_sign_pr }} + permissions: + contents: write + pull-requests: write + # Same public-key env as pr-orchestrator so strict verify can check signatures against the + # configured release key (not only resources/keys/module-signing-public.pem in the checkout). + env: + SPECFACT_MODULE_PUBLIC_SIGN_KEY: ${{ secrets.SPECFACT_MODULE_PUBLIC_SIGN_KEY }} + SPECFACT_MODULE_SIGNING_PUBLIC_KEY_PEM: ${{ secrets.SPECFACT_MODULE_SIGNING_PUBLIC_KEY_PEM }} + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Fetch workflow_dispatch comparison base + if: github.event_name == 'workflow_dispatch' + run: git fetch --no-tags origin "${{ github.event.inputs.base_branch }}" + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install signer dependencies + run: | + python -m pip install --upgrade pip + python -m pip install pyyaml beartype icontract cryptography cffi + + - name: Auto-sign changed module manifests (push to dev/main, non-bot actors) + if: >- + github.event_name == 'push' && + (github.ref_name == 'dev' || github.ref_name == 'main') && + github.actor != 'github-actions[bot]' + env: + SPECFACT_MODULE_PRIVATE_SIGN_KEY: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY }} + SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE }} + run: | + set -euo pipefail + if [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY}" ]; then + echo "::error::Missing SPECFACT_MODULE_PRIVATE_SIGN_KEY. Configure the secret so pushes to ${GITHUB_REF_NAME} can auto-sign module manifests." + exit 1 + fi + BEFORE="${{ github.event.before }}" + if [ "$BEFORE" = "0000000000000000000000000000000000000000" ]; then + BEFORE="$(git rev-parse HEAD~1 2>/dev/null || true)" + fi + if [ -z "$BEFORE" ]; then + echo "::error::Unable to resolve parent commit for --changed-only signing." + exit 1 + fi + python scripts/sign-modules.py \ + --changed-only \ + --base-ref "$BEFORE" \ + --bump-version patch \ + --payload-from-filesystem + + # Registry-only merges leave packages/** unchanged, so --changed-only signs nothing. + # Sign any manifest still missing integrity.signature (same CI key as publish-modules). + python - <<'PY' + import os + import subprocess + import sys + from pathlib import Path + + import yaml + + if not os.environ.get("SPECFACT_MODULE_PRIVATE_SIGN_KEY", "").strip(): + raise SystemExit(0) + + def manifest_has_signature(data: dict) -> bool: + integrity_obj = data.get("integrity") + if not isinstance(integrity_obj, dict): + return False + return bool(str(integrity_obj.get("signature") or "").strip()) + + root = Path(".").resolve() + for manifest in sorted((root / "packages").glob("*/module-package.yaml")): + raw = yaml.safe_load(manifest.read_text(encoding="utf-8")) + if not isinstance(raw, dict) or manifest_has_signature(raw): + continue + print(f"Signing unsigned manifest {manifest} (post --changed-only sweep).", flush=True) + subprocess.run( + [ + sys.executable, + "scripts/sign-modules.py", + "--payload-from-filesystem", + "--allow-same-version", + str(manifest), + ], + cwd=str(root), + check=True, + ) + PY + + - name: Strict verify module manifests (push to dev/main) + if: github.event_name == 'push' && (github.ref_name == 'dev' || github.ref_name == 'main') + run: | + set -euo pipefail + BEFORE="${{ github.event.before }}" + if [ "$BEFORE" = "0000000000000000000000000000000000000000" ]; then + BEFORE="HEAD~1" + fi + python scripts/verify-modules-signature.py \ + --require-signature \ + --payload-from-filesystem \ + --enforce-version-bump \ + --version-check-base "$BEFORE" + + - name: PR or dispatch verify (checksum-only, no signature required on head) + if: github.event_name != 'push' + run: | + set -euo pipefail + BASE_REF="" + if [ "${{ github.event_name }}" = "pull_request" ]; then + BASE_REF="origin/${{ github.event.pull_request.base.ref }}" + elif [ "${{ github.event_name }}" = "workflow_dispatch" ]; then + BASE_REF="origin/${{ github.event.inputs.base_branch }}" + fi + if [ -z "$BASE_REF" ]; then + echo "::error::Missing comparison base for module verification." + exit 1 + fi + python scripts/verify-modules-signature.py \ + --payload-from-filesystem \ + --enforce-version-bump \ + --version-check-base "$BASE_REF" + + - name: Commit auto-signed manifests (push to dev/main, non-bot actors) + id: commit_auto_sign + if: >- + github.event_name == 'push' && + (github.ref_name == 'dev' || github.ref_name == 'main') && + github.actor != 'github-actions[bot]' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + set -euo pipefail + echo "opened_sign_pr=false" >> "$GITHUB_OUTPUT" + + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git add -u -- packages/ + if git diff --cached --quiet; then + echo "No manifest signing changes to commit." + exit 0 + fi + git commit -m "chore(modules): auto-sign module manifests" + + TARGET_BRANCH="${GITHUB_REF_NAME}" + SIGN_BRANCH="auto/sign-${TARGET_BRANCH}-${GITHUB_RUN_ID}" + git push origin "HEAD:refs/heads/${SIGN_BRANCH}" + + gh pr create \ + --base "${TARGET_BRANCH}" \ + --head "${SIGN_BRANCH}" \ + --title "chore(modules): auto-sign module manifests" \ + --body "Automated signing from [workflow run ${GITHUB_RUN_ID}](${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}). + + Branch \`${TARGET_BRANCH}\` is protected; merge this PR to land signed \`packages/**/module-package.yaml\` updates." + + echo "opened_sign_pr=true" >> "$GITHUB_OUTPUT" + echo "::notice::Opened a pull request to merge signed manifests into ${TARGET_BRANCH}." + + reproducibility: + name: Assert signing reproducibility + if: >- + github.event_name == 'push' && + github.ref_name == 'main' && + needs.verify.outputs.opened_sign_pr != 'true' + runs-on: ubuntu-latest + needs: [verify] + permissions: + contents: read + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Sync to remote branch tip (after verify job; merge sign PR before expecting new signatures on main) + run: | + set -euo pipefail + git fetch origin "${GITHUB_REF_NAME}" + git reset --hard "origin/${GITHUB_REF_NAME}" + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install signer dependencies + run: | + python -m pip install --upgrade pip + python -m pip install pyyaml beartype icontract cryptography cffi + + - name: Re-sign manifests and assert no diff + env: + SPECFACT_MODULE_PRIVATE_SIGN_KEY: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY }} + SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE }} + run: | + if [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY}" ]; then + echo "::notice::Skipping reproducibility check because SPECFACT_MODULE_PRIVATE_SIGN_KEY is not configured." + exit 0 + fi + + mapfile -t MANIFESTS < <(find packages -name 'module-package.yaml' -type f | sort) + if [ "${#MANIFESTS[@]}" -eq 0 ]; then + echo "No module manifests found" + exit 0 + fi + + python scripts/sign-modules.py --payload-from-filesystem "${MANIFESTS[@]}" + + if ! git diff --exit-code -- packages/; then + echo "::error::Module signatures are stale for the configured signing key. Re-sign and commit manifest updates." + git --no-pager diff --name-only -- packages/ + exit 1 + fi + + sign-and-push: + name: Sign changed modules (manual dispatch) + if: github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + needs: [verify] + permissions: + contents: write + pull-requests: write + steps: + - name: Require module signing key secret + env: + SPECFACT_MODULE_PRIVATE_SIGN_KEY: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY }} + run: | + if [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY}" ]; then + echo "::error::Missing or empty repository secret SPECFACT_MODULE_PRIVATE_SIGN_KEY." + exit 1 + fi + + - name: Checkout branch + uses: actions/checkout@v4 + with: + fetch-depth: 0 + ref: ${{ github.ref }} + persist-credentials: true + + - name: Fetch comparison base + run: git fetch --no-tags origin "${{ github.event.inputs.base_branch }}" + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install signer dependencies + run: | + python -m pip install --upgrade pip + python -m pip install pyyaml beartype icontract cryptography cffi + + - name: Sign module manifests + env: + SPECFACT_MODULE_PRIVATE_SIGN_KEY: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY }} + SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE }} + run: | + set -euo pipefail + MERGE_BASE="$(git merge-base HEAD "origin/${{ github.event.inputs.base_branch }}")" + BUMP="${{ github.event.inputs.version_bump }}" + if [ "${{ github.event.inputs.resign_all_manifests }}" = "true" ]; then + mapfile -t MANIFESTS < <(find packages -name 'module-package.yaml' -type f | sort) + if [ "${#MANIFESTS[@]}" -eq 0 ]; then + echo "No module manifests found" + exit 0 + fi + python scripts/sign-modules.py --payload-from-filesystem "${MANIFESTS[@]}" + else + python scripts/sign-modules.py \ + --changed-only \ + --base-ref "$MERGE_BASE" \ + --bump-version "${BUMP}" \ + --payload-from-filesystem + fi + + - name: Commit and push signed manifests + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + set -euo pipefail + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + if git diff --quiet; then + echo "No manifest changes to commit." + echo "## No signing changes" >> "${GITHUB_STEP_SUMMARY}" + exit 0 + fi + git add -u -- packages/ + if git diff --cached --quiet; then + echo "No staged module manifest updates." + exit 0 + fi + git commit -m "chore(modules): manual workflow_dispatch sign changed modules" + + TARGET_BRANCH="${GITHUB_REF_NAME}" + if [ "${TARGET_BRANCH}" = "dev" ] || [ "${TARGET_BRANCH}" = "main" ]; then + SIGN_BRANCH="auto/sign-dispatch-${TARGET_BRANCH}-${GITHUB_RUN_ID}" + git push origin "HEAD:refs/heads/${SIGN_BRANCH}" + gh pr create \ + --base "${TARGET_BRANCH}" \ + --head "${SIGN_BRANCH}" \ + --title "chore(modules): manual sign changed modules" \ + --body "Manual signing from [workflow run ${GITHUB_RUN_ID}](${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}). + + Protected branch \`${TARGET_BRANCH}\`: merge this PR to land updates (base compare: \`origin/${{ github.event.inputs.base_branch }}\`)." + echo "## Opened pull request" >> "${GITHUB_STEP_SUMMARY}" + else + git push origin "HEAD:${TARGET_BRANCH}" + echo "## Signed manifests pushed" >> "${GITHUB_STEP_SUMMARY}" + fi + echo "Branch: \`${GITHUB_REF_NAME}\` (base: \`origin/${{ github.event.inputs.base_branch }}\`, bump: \`${{ github.event.inputs.version_bump }}\`, resign_all: \`${{ github.event.inputs.resign_all_manifests }}\`)." >> "${GITHUB_STEP_SUMMARY}" diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 92810078..3fee3058 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 f461b5a0..c527f849 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 ed4e049b..22af882b 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:** Split **PR-time** checks from **post-merge branch** checks. **`pr-orchestrator`** (on PRs and related events) runs `verify-modules-signature` with **`--payload-from-filesystem --enforce-version-bump`**, and for pull requests adds **`--version-check-base`**. PRs whose base is **`dev`** use payload checksum + version bump **without** **`--require-signature`**. PRs whose base is **`main`** append **`--require-signature`**; **`push`** paths in that workflow that target **`main`** also append **`--require-signature`**. Separately, **`.github/workflows/sign-modules.yml`** (**Module Signature Hardening**) runs its own verifier: **pushes to `dev` or `main`** execute the **Strict verify** step with **`--require-signature`** (plus **`--payload-from-filesystem --enforce-version-bump`** and **`--version-check-base`** against the push parent); **pull requests** and **`workflow_dispatch`** in that same workflow use **`--payload-from-filesystem --enforce-version-bump`** and **`--version-check-base`** **without** **`--require-signature`** on the head. After merge to **`dev`** or **`main`**, **`sign-modules`** auto-signs (non-bot pushes), strict-verifies on those pushes, and commits without **`[skip ci]`** so follow-up workflows (including **`publish-modules`**) run on the signed tip. 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`: **`--require-signature`** only on branch **`main`** or when **`GITHUB_BASE_REF=main`** in Actions; otherwise the same baseline formal verify as PRs to **`dev`**. Refresh checksums locally without a private key via **`python scripts/sign-modules.py --allow-unsigned --payload-from-filesystem`** on changed manifests. On non-`main` branches, the pre-commit hook **auto-runs** that flow (`--changed-only` vs `HEAD`, then vs `HEAD~1` when needed), re-stages updated **`module-package.yaml`** files, and re-verifies. **`registry/index.json`** and published tarballs are **not** updated locally: a manifest may temporarily be **ahead** of `latest_version` until **`publish-modules`** runs on **`dev`**/**`main`** (see **`hatch run yaml-lint`** / `tools/validate_repo_manifests.py`). For rare manual registry repair only, use **`hatch run sync-registry-from-package --bundle`** with a bundle name (for example **`specfact-code-review`**); it is **not** wired into pre-commit so CI publish stays authoritative. **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. Non-blocking warnings reported by SpecFact still require remediation prior to merge unless a documented, approved exception exists. The helper runs `specfact code review run --json --out .specfact/code-review.json` on those Python paths and prints a short findings summary on stderr. Full CLI options (`--mode`, `--focus`, `--level`, `--bug-hunt`, etc.) are documented under [Code review module](./docs/modules/code-review.md). Block 1 is split into separate pre-commit hooks so output appears between stages instead of buffering until the end. Requires a local **specfact-cli** install (`hatch run dev-deps` resolves sibling `../specfact-cli` or `SPECFACT_CLI_REPO`). Scope notes: - Pre-commit runs `hatch run lint` in the **Block 1 — lint** hook when any staged path matches `*.py` / `*.pyi`, matching the CI quality job (Ruff alone does not run pylint). diff --git a/docs/agent-rules/50-quality-gates-and-review.md b/docs/agent-rules/50-quality-gates-and-review.md index a2b49f1f..8582b653 100644 --- a/docs/agent-rules/50-quality-gates-and-review.md +++ b/docs/agent-rules/50-quality-gates-and-review.md @@ -51,7 +51,7 @@ depends_on: ## Pre-commit order -1. Module signature verification via `scripts/pre-commit-verify-modules-signature.sh` (`.pre-commit-config.yaml`; `fail_fast: true` so a failing earlier hook never runs later stages). The hook adds `--require-signature` on branch `main`, or when `GITHUB_BASE_REF` is `main` (PR target in Actions). +1. Module signature verification via `scripts/pre-commit-verify-modules-signature.sh` (`.pre-commit-config.yaml`; `fail_fast: true` so a failing earlier hook never runs later stages). The hook adds `--require-signature` on branch `main`, or when `GITHUB_BASE_REF` is `main` (PR target in Actions); otherwise it runs the baseline `--payload-from-filesystem --enforce-version-bump` verifier (same formal policy as PRs targeting `dev`). 2. **Block 1** — four separate hooks (each flushes pre-commit output when it exits, so you see progress between stages): `pre-commit-quality-checks.sh block1-format` (always), `block1-yaml` when staged `*.yaml` / `*.yml`, `block1-bundle` (always), `block1-lint` when staged `*.py` / `*.pyi`. 3. **Block 2** — `pre-commit-quality-checks.sh block2` (skipped for “safe-only” staged paths): `hatch run python scripts/pre_commit_code_review.py …` on **staged paths under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, and `openspec/changes/`** (excluding `TDD_EVIDENCE.md`), then `contract-test-status` / `hatch run contract-test`. diff --git a/docs/authoring/module-signing.md b/docs/authoring/module-signing.md index 9f02de93..602a4fa1 100644 --- a/docs/authoring/module-signing.md +++ b/docs/authoring/module-signing.md @@ -143,11 +143,11 @@ python scripts/verify-modules-signature.py --require-signature --payload-from-fi ### Signing on approval (same-repo PRs) -Workflow **`sign-modules-on-approval.yml`** runs when a review is **submitted** and **approved** on a PR whose base is **`dev`** or **`main`**, and only when the PR head is in **this** repository (`head.repo` equals the base repo). It checks out **`github.event.pull_request.head.sha`** (the commit that was approved, not the moving branch tip), uses `SPECFACT_MODULE_PRIVATE_SIGN_KEY` and `SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE` (each validated with a named error if missing), discovers changes against the **merge-base** with the base branch (not the moving base tip alone), runs `scripts/sign-modules.py --changed-only --bump-version patch --payload-from-filesystem`, and commits results with `[skip ci]`. If `git push` is rejected because the PR branch advanced after approval, the job fails with guidance to update the branch and re-approve. **Fork PRs** are skipped (the default `GITHUB_TOKEN` cannot push to a contributor fork). +Workflow **`sign-modules-on-approval.yml`** runs when a review is **submitted** and **approved** on a PR whose base is **`dev`** or **`main`**, and only when the PR head is in **this** repository (`head.repo` equals the base repo). It checks out **`github.event.pull_request.head.sha`** (the commit that was approved, not the moving branch tip), uses `SPECFACT_MODULE_PRIVATE_SIGN_KEY` and `SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE` (each validated with a named error if missing), discovers changes against the **merge-base** with the base branch (not the moving base tip alone), runs `scripts/sign-modules.py --changed-only --bump-version patch --payload-from-filesystem`, and commits results **without** `[skip ci]` so PR checks and downstream workflows run on the signed head. If `git push` is rejected because the PR branch advanced after approval, the job fails with guidance to update the branch and re-approve. **Fork PRs** are skipped (the default `GITHUB_TOKEN` cannot push to a contributor fork). ### Pre-commit -The first pre-commit hook runs **`scripts/pre-commit-verify-modules-signature.sh`**, which mirrors CI: **`--require-signature` on branch `main`**, or when **`GITHUB_BASE_REF=main`** in Actions pull-request contexts; otherwise checksum + version enforcement only. +The first pre-commit hook runs **`scripts/pre-commit-verify-modules-signature.sh`**, which mirrors CI: **`--require-signature` on branch `main`**, or when **`GITHUB_BASE_REF=main`** in Actions pull-request contexts; otherwise the same baseline formal verify as PRs to **`dev`** (`--payload-from-filesystem --enforce-version-bump`, no **`--require-signature`**). On failure it runs **`sign-modules.py --allow-unsigned --payload-from-filesystem`** (`--changed-only` vs **`HEAD`**, then vs **`HEAD~1`** for manifests still failing), **`git add`** those `module-package.yaml` paths, and re-verifies. It does **not** rewrite **`registry/`** (publish workflows own signed artifacts and index updates). **`yaml-lint`** allows a semver **ahead** manifest vs **`registry/index.json`** until **`publish-modules`** reconciles. ## Rotation Procedure diff --git a/docs/guides/ci-cd-pipeline.md b/docs/guides/ci-cd-pipeline.md index 355c414f..17e05783 100644 --- a/docs/guides/ci-cd-pipeline.md +++ b/docs/guides/ci-cd-pipeline.md @@ -41,7 +41,7 @@ hatch run smart-test hatch run test ``` -Add `--require-signature` to the verify step when checking the same policy as **`main`** (for example before promoting work to `main`). On feature branches and for PRs targeting **`dev`**, CI does not require signatures yet; pre-commit matches that via `scripts/pre-commit-verify-modules-signature.sh`. +Add `--require-signature` to the verify step when checking the same policy as **`main`** (for example before promoting work to `main`). On feature branches and for PRs targeting **`dev`**, CI still enforces payload checksums and version bumps but does not require `integrity.signature` yet; pre-commit matches that via `scripts/pre-commit-verify-modules-signature.sh`. Use the same order locally before pushing changes that affect docs, bundles, or registry metadata. diff --git a/docs/modules/code-review.md b/docs/modules/code-review.md index d5c188f5..2b42fc91 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,9 +267,17 @@ 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 +`specfact_code_review.tools.contract_runner.run_contract_check(files, *, bug_hunt=False)` combines two contract-oriented checks: 1. an AST scan for public functions missing `@require` / `@ensure` @@ -261,20 +287,27 @@ 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 a batch-level package/repo + scan root imports `icontract` (`from icontract …` or `import icontract`). + Reviewed files in a package that uses icontract are scanned even when the + changed file itself does not import icontract - missing icontract decorators become `contracts` findings with rule - `MISSING_ICONTRACT` + `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: @@ -369,17 +402,25 @@ specfact code review rules update ## Review orchestration -`specfact_code_review.run.runner.run_review(files, no_tests=False)` orchestrates the -bundle runners in this order: +`specfact_code_review.run.runner.run_review( +files, +no_tests=False, +include_noise=False, +progress_callback=None, +bug_hunt=False, +review_level=None, +review_mode="enforce", +)` orchestrates the bundle runners in this order: 1. Ruff 2. Radon -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`, @@ -388,6 +429,23 @@ adding a new CLI flag. The merged findings are then scored into a governed `ReviewReport`. +Representative programmatic use: + +```python +from pathlib import Path + +from specfact_code_review.run.runner import run_review + +report = run_review( + [Path("src/app.py")], + no_tests=False, + include_noise=False, + bug_hunt=True, + review_level="error", + review_mode="shadow", +) +``` + ## Bundled policy pack The bundle now ships `specfact/clean-code-principles` as a resource payload at: diff --git a/docs/reference/module-security.md b/docs/reference/module-security.md index 5edfb5e0..2db1c546 100644 --- a/docs/reference/module-security.md +++ b/docs/reference/module-security.md @@ -45,11 +45,14 @@ 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. -- **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`). +- **Verification command** (`scripts/verify-modules-signature.py`): + - **Baseline (CI)**: `--payload-from-filesystem --enforce-version-bump` — full payload checksum verification plus version-bump enforcement. + - **Dev-target PR mode**: `.github/workflows/pr-orchestrator.yml` uses the baseline verifier for pull requests targeting `dev` (full payload checksum + version bump, **no** `--require-signature`). Cryptographic signing is applied after merge via `sign-modules` / approval workflows, not required on the PR head. + - **Strict mode**: add `--require-signature` so every manifest must include a verifiable `integrity.signature`. In `.github/workflows/pr-orchestrator.yml` this is appended for **pull requests whose base is `main`** and for **pushes to `main`**, in addition to the baseline flags. Locally, `scripts/pre-commit-verify-modules-signature.sh` adds `--require-signature` only when the checkout (or `GITHUB_BASE_REF` in Actions) is **`main`**. + - **Local non-main hook mode**: `scripts/pre-commit-verify-modules-signature.sh` otherwise runs the same baseline flags as dev-target PR CI (no `--require-signature`). Refresh `integrity.checksum` without a private key using `scripts/sign-modules.py --allow-unsigned --payload-from-filesystem`. + - **Pull request CI** also passes `--version-check-base ` (typically `origin/`) so version rules compare against the PR base. + - **`--metadata-only`** remains available for optional tooling that only needs manifest shape and checksum **format** checks without hashing module trees. +- **CI signing**: Approved same-repo PRs to `dev` or `main` from trusted reviewer associations may receive automated signing commits via `sign-modules-on-approval.yml` (repository secrets; merge-base scoped `--changed-only`). ## Public key and key rotation diff --git a/openspec/CHANGE_ORDER.md b/openspec/CHANGE_ORDER.md index 673e74b5..d5ba18e2 100644 --- a/openspec/CHANGE_ORDER.md +++ b/openspec/CHANGE_ORDER.md @@ -79,6 +79,12 @@ These changes are the modules-side runtime companions to split core governance a | governance | 04 | governance-04-deterministic-agent-governance-loading | [#181](https://github.com/nold-ai/specfact-cli-modules/issues/181) | Parent Feature: [#163](https://github.com/nold-ai/specfact-cli-modules/issues/163); paired core [specfact-cli#494](https://github.com/nold-ai/specfact-cli/issues/494); baseline [#178](https://github.com/nold-ai/specfact-cli-modules/issues/178) (implements archived `governance-03-github-hierarchy-cache`, paired core [specfact-cli#491](https://github.com/nold-ai/specfact-cli/issues/491)) | | validation | 02 | validation-02-full-chain-engine | [#171](https://github.com/nold-ai/specfact-cli-modules/issues/171) | Parent Feature: [#163](https://github.com/nold-ai/specfact-cli-modules/issues/163); core counterpart `specfact-cli#241`; runtime inputs from `#164` and `#165`; policy semantics from `#158` | +### Code review and sidecar validation improvements + +| Module | Order | Change folder | GitHub # | Blocked by | +|--------|-------|---------------|----------|------------| +| code-review + codebase | 01 | code-review-bug-finding-and-sidecar-venv-fix | [#174](https://github.com/nold-ai/specfact-cli-modules/issues/174) | Parent Feature: [#175](https://github.com/nold-ai/specfact-cli-modules/issues/175); Epic: [#162](https://github.com/nold-ai/specfact-cli-modules/issues/162) | + ### Module trust chain and CI security | Module | Order | Change folder | GitHub # | Blocked by | diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/.openspec.yaml b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/.openspec.yaml new file mode 100644 index 00000000..23ef75a1 --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-04-08 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 00000000..5902d26c --- /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/design.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/design.md new file mode 100644 index 00000000..789720e2 --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/design.md @@ -0,0 +1,129 @@ +## Context + +The `specfact-code-review` bundle runs seven analysis tools in sequence (ruff, radon, semgrep, AST, basedpyright, pylint, contract_runner). External-repo validation across 10 Python repos revealed two gaps: + +1. **No bug-finding signal on repos without icontract**: CrossHair already runs (`contract_runner.py:112`) but with `--per_path_timeout 2` and a 30s hard cap — too tight to find counterexamples in type-annotated code. Semgrep runs only `clean_code.yaml` rules (style/architecture); no security or bug-pattern rules exist. `MISSING_ICONTRACT` fires on every public function in any repo that hasn't opted into icontract, producing hundreds of low-value warnings that drown real signal. + +2. **Sidecar venv self-scan**: All three framework extractors (FastAPI, Flask, Django/DRF) use `search_path.rglob("*.py")` with no path exclusions. The sidecar dependency installer writes into `.specfact/venv/` before extraction runs. Result: the FastAPI extractor on gpt-researcher picked up FastAPI's own source from the venv and reported 25,947 routes (actual: 19). The fix is a single shared exclusion applied at the `rglob` call site. + +## Goals / Non-Goals + +**Goals:** +- Add a `--bug-hunt` flag to `specfact code review run` that gives CrossHair meaningful budget (10s/path, 120s total) +- Add a `bugs.yaml` semgrep config with Python security/bug rules, wired as a second semgrep pass +- Auto-suppress `MISSING_ICONTRACT` when no `@require`/`@ensure` imports are found in the reviewed files +- Exclude `.specfact/` from all sidecar framework extractor scan paths +- Add `--mode shadow|enforce`, repeatable `--focus source|tests|docs`, and `--level error|warning` with clear CLI validation and stable JSON/human output +- Align `module-package.yaml` `pip_dependencies` with every external review tool and implement runtime skips with explicit `tool_error` messages when a tool is missing + +**Non-Goals:** +- Replacing CrossHair with a different symbolic execution engine +- Adding semgrep cloud/registry rules (offline-first; rules must be bundled locally) +- Changing default **enforce** exit semantics for existing users (shadow is strictly opt-in) +- Fixing CrossHair's analysis depth beyond timeout tuning +- Teaching the review runner to lint non-Python doc formats (Markdown/RST) in this change — `--focus docs` applies to **Python files under `docs/`** only + +## Decisions + +**D1: `bugs.yaml` as a separate semgrep config, not merged into `clean_code.yaml`** + +Keeps clean-code rules and bug-finding rules independently evolvable. The semgrep runner already has `find_semgrep_config` that walks parents for `clean_code.yaml` by name — add a parallel `find_semgrep_bugs_config` that looks for `.semgrep/bugs.yaml` and returns `None` (not an error) when absent. Runner calls both; missing `bugs.yaml` is a no-op. + +Alternative considered: a single merged config. Rejected — mixing style warnings and bug errors in one file makes the rule set harder to reason about and harder to disable selectively. + +**D2: `--bug-hunt` flag rather than a separate command** + +`specfact code review run --bug-hunt` passes `bug_hunt=True` through `ReviewRunRequest` → `run_review` → `run_contract_check`. No new command surface, no CLI help restructuring. The flag increases CrossHair timeouts only; all other tools run at normal speed. + +Alternative considered: `specfact code review bug-hunt` as a new subcommand. Rejected — unnecessary API surface; `--bug-hunt` is composable with `--scope full`, `--json`, `--out`, etc. + +**D3: MISSING_ICONTRACT auto-suppression via import scan, not a flag** + +Scan the reviewed file list for any `from icontract import` or `import icontract` statement before running `contract_runner`. If none found, skip `MISSING_ICONTRACT` findings entirely. This is automatic — no user flag needed, works correctly on both internal bundles (which do use icontract) and external repos (which don't). + +Alternative considered: `--suppress-missing-contracts` flag. Rejected — requires users to know to pass it; auto-detection is always correct. + +**D4: Sidecar exclusion via a shared `_is_excluded_path` helper on `BaseFrameworkExtractor`** + +Add `_EXCLUDED_DIR_NAMES = frozenset({".specfact", ".git", "__pycache__", "node_modules"})` and a `_iter_python_files(root: Path)` generator on `BaseFrameworkExtractor` that yields only files whose parts contain no excluded dir name. All three framework extractors replace `search_path.rglob("*.py")` with `self._iter_python_files(search_path)`. + +Alternative considered: filtering at the orchestrator level before passing `repo_path` to extractors. Rejected — extractors own the scan, fixing at the source is cleaner and prevents future extractors from re-introducing the bug. + +**D5: `--mode` with default `enforce`** + +- **`enforce`**: After findings are collected and post-processed (`--level` filter), compute score/verdict/`ci_exit_code` exactly as today; process exit matches `ci_exit_code`. +- **`shadow`**: Same tool execution and same reported findings (after `--level`), but **force** `ci_exit_code = 0` and process exit `0` even when verdict would be `FAIL`. Human and JSON output still show the real `overall_verdict` so operators can see risk while not blocking the pipeline. + +Alternative considered: shadow mode hiding failures in JSON. Rejected — that would defeat auditability; only enforcement (exit) is relaxed. + +**D6: Repeatable `--focus` as a set union over path facets** + +Facets (Python files only, after normal ignore rules): + +| Facet | Membership rule | +|-------|-----------------| +| `tests` | `tests` appears in `path.parts` (same heuristic as `_is_test_file`) | +| `docs` | `docs` appears in `path.parts` | +| `source` | suffix `.py`, **not** `tests` facet, **not** `docs` facet | + +When `--focus` is passed one or more times, the reviewed file set is the **union** of matching files intersected with the scope-resolved set (positional, `--scope changed|full`, `--path`, etc.). When `--focus` is **omitted**, file resolution stays backward compatible with `--include-tests` / `--exclude-tests` / interactive defaults. + +When **any** `--focus` is present, **`--include-tests` and `--exclude-tests` are disallowed** (Typer `BadParameter`) to avoid contradictory intent. + +**D7: `--level` as a severity floor on the reported and scored finding list** + +Apply **after** all tools finish: + +- **`error`**: keep findings where `severity == "error"` only. +- **`warning`**: keep `severity in {"error", "warning"}`; drop `info`. +- **Omitted**: keep all severities (current behaviour). + +Recompute `score`, `overall_verdict`, `reward_delta`, and `ci_exit_code` from the **filtered** list so tables, JSON, score line, and exit code stay consistent. Tools still run on the full resolved file set (performance unchanged); only the governance envelope uses the filtered list. + +**D8: Typer wiring stays in `review/commands.py`** + +New options are declared next to existing `run` flags; parsing/validation errors surface as `typer.BadParameter` with stable messages suitable for cli-contract tests. + +**D9: Canonical tool → pip package map owned by the code-review package** + +Maintain a single source of truth (Python module or documented table consumed by a test) mapping each **review tool id** to: + +- the CLI executable name probed on `PATH` (where applicable), and +- the **PyPI distribution name** declared in `module-package.yaml` `pip_dependencies` (e.g. `crosshair-tool` → executable `crosshair`). + +Covered tools for the default pipeline: `ruff`, `radon`, `semgrep`, `basedpyright`, `pylint`, `crosshair`, plus `pytest` / `pytest-cov` for the TDD gate subprocess. **AST clean-code** analysis uses the stdlib and shipped Python code only — no extra `pip_dependencies` row. + +When a new runner is added (e.g. second Semgrep pass), the map and `pip_dependencies` MUST be updated in the same change. + +**D10: Availability check before subprocess; skip with one `tool_error`** + +Each external runner SHALL call a shared helper (e.g. `ensure_tool_available(tool_id, file_path) -> list[ReviewFinding]`) that returns a non-empty list (single finding) when the tool is missing, so the runner returns immediately **without** invoking the tool. This avoids misclassifying `FileNotFoundError` as “parse JSON failed” or similar. + +**D11: Standard skip message shape** + +Skip findings SHALL use `category="tool_error"`, `rule="tool_error"` (or a dedicated `TOOL_SKIPPED` rule if implementation prefers — spec requires stable filtering by category/tool), `severity="warning"` unless policy elevates missing tools to `error`. The message MUST: + +- name the **tool id** (e.g. `ruff`, `semgrep`), +- state that **review checks for that tool were skipped** because it is **not installed** or not on `PATH`, +- cite the **pip package name** from the manifest (e.g. “install `ruff`”). + +For pytest invoked via `sys.executable`, treat “pytest not importable” / failed launcher the same way with tool id `pytest` and packages `pytest` / `pytest-cov` as appropriate. + +## Risks / Trade-offs + +- **CrossHair false positives in bug-hunt mode**: Longer timeouts mean CrossHair explores more paths and may surface `SideEffectDetected` warnings on I/O-heavy functions. Mitigated: `_IGNORED_CROSSHAIR_PREFIXES` already filters `SideEffectDetected`; no change needed. +- **`bugs.yaml` rule maintenance**: Bundled semgrep rules can go stale. Mitigated: keep the initial set small (5–10 high-confidence rules), document in the file header that additions require a PR with test evidence. +- **False confidence when many tools are skipped**: A run may PASS while major tools were skipped. Mitigated: skip findings are visible in human and JSON output; optional follow-up could promote missing-tool severity in enforce mode — out of scope unless added to tasks later. +- **MISSING_ICONTRACT auto-suppression masking real gaps**: If a bundle file imports icontract but only for one function, all other uncovered functions are still flagged. The auto-suppression only fires when zero icontract imports exist — so internal bundles are unaffected. +- **Sidecar exclusion breaking legitimate `.specfact/` source scanning**: No known repo puts application source under `.specfact/`. Risk is negligible. + +## Migration Plan + +No migrations required. New flags are additive with **defaults preserving today’s behaviour**: `--mode` defaults to `enforce`, `--focus` omitted uses legacy test inclusion rules, `--level` omitted keeps all severities. Sidecar and MISSING_ICONTRACT changes remain backward compatible when not triggered. + +Patch version bump on `specfact-codebase` (sidecar bug fix). `specfact-code-review` should receive at least a **patch** bump once shipped (new CLI surface + behaviour). If policy treats bundled `bugs.yaml` as a material artefact, prefer a **minor** bump for the review bundle. + +## Open Questions + +- Which specific semgrep rules to include in `bugs.yaml` v1? Candidates: `python.lang.security.audit.dangerous-system-call`, `python.lang.correctness.useless-eqeq`, `python.lang.security.audit.hardcoded-password`, `python.lang.correctness.none-not-null`. Needs sign-off before implementation to avoid shipping noisy rules. +- Should `--bug-hunt` findings appear in the score model? Currently CrossHair counterexamples are `severity=warning`; they could be promoted to `severity=error` in bug-hunt mode. Defer to implementation task. diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/proposal.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/proposal.md new file mode 100644 index 00000000..488217fd --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/proposal.md @@ -0,0 +1,51 @@ +## Why + +External repo validation (crewAI, gpt-researcher, and 8 OSS baseline repos) revealed two concrete tool gaps: the code review module produces no bug-finding signal on repos that don't use icontract, and the sidecar route extractor inflates route counts by scanning its own installed venv. Both blockers limit the tool's usefulness on arbitrary Python codebases. + +## What Changes + +- **Semgrep bug-finding rules**: Add a `bugs.yaml` semgrep config alongside the existing `clean_code.yaml`, covering Python security patterns, unsafe access, known bug-prone patterns. Wire it as a second semgrep pass in the runner. +- **CrossHair timeout increase + bug-hunt mode**: Expose a `--bug-hunt` flag on `specfact code review run` that raises `--per_path_timeout` from 2s to 10s and total timeout from 30s to 120s, giving CrossHair enough budget to find counterexamples in type-annotated code without icontract. +- **MISSING_ICONTRACT suppression on external repos**: When a reviewed codebase has zero icontract usage, suppress `MISSING_ICONTRACT` warnings rather than emitting one per public function. Auto-detect by scanning the reviewed files for any `@require`/`@ensure` import; if none found, omit the rule entirely for that run. +- **Sidecar venv self-scan fix**: Exclude `.specfact/` from the route extraction scan path in all framework extractors (`FlaskExtractor`, `FastAPIExtractor`, `DjangoExtractor`). Currently the sidecar installs deps into `.specfact/venv` then scans the full repo tree, picking up the installed framework's own source (gpt-researcher: 25,947 reported routes vs 19 real). +- **Review enforcement mode**: Add `--mode shadow` and `--mode enforce` on `specfact code review run`. **Shadow** runs the full tool chain and emits findings (human table and/or JSON) but **never fails the process** (`ci_exit_code` and process exit are `0`) so CI or local hooks can log signal without blocking. **Enforce** preserves today’s governed exit behaviour (blocking findings still yield a non-zero exit). Default is **enforce** for backward compatibility. +- **Review scope facets**: Add repeatable `--focus` options (`source`, `tests`, `docs`) to restrict which Python files are reviewed after auto-scope or positional resolution. **Source** means non-test application Python (same class of paths as the default when tests are excluded). **Tests** means paths that match the existing test-path heuristic (`tests` path segment). **Docs** means Python files under a top-level or nested `docs/` directory segment (e.g. `docs/conf.py`). Multiple `--focus` values union their file sets. This complements (and must be reconciled with) existing `--include-tests` / `--exclude-tests`; passing conflicting combinations SHALL be rejected with a clear CLI error. +- **Review severity floor**: Add `--level error` and `--level warning` to control which findings appear in output and participate in scoring/verdict. **`error`** keeps only `severity="error"` findings. **`warning`** keeps `error` and `warning` and drops `info`. Omitted `--level` keeps all severities (current behaviour). +- **Mandatory tool dependencies + graceful skips**: The code-review module manifest (`packages/specfact-code-review/module-package.yaml` `pip_dependencies`) SHALL list every **external** CLI and Python package required to execute the full review pipeline (Ruff, Radon, Semgrep, basedpyright, Pylint, CrossHair, pytest/pytest-cov for the TDD gate, plus any new runners such as a second Semgrep pass). At **runtime**, before invoking each external tool, the runner SHALL detect availability (`shutil.which` for CLIs; import/subprocess probe for pytest as appropriate). If a tool is missing, that step SHALL be **skipped** (no partial subprocess), and the run SHALL record **exactly one** `ReviewFinding` with `category="tool_error"` whose message states that **review checks for that tool were skipped** because it is not installed, and names the **pip package** from the manifest users should install. The AST clean-code pass remains in-process Python and does not require a separate CLI dependency row. + +## Capabilities + +### New Capabilities + +- `code-review-bug-finding`: Semgrep security/bug rules pass and CrossHair bug-hunt mode — a second analysis layer focused on detecting actual bugs rather than clean-code style issues. +- `code-review-tool-dependencies`: Declared pip dependencies match all external review tools; missing tools produce explicit skip findings instead of opaque failures. + +### Modified Capabilities + +- `contract-runner`: CrossHair timeout parameters increase for bug-hunt mode; MISSING_ICONTRACT auto-suppressed when no icontract usage is detected in the reviewed files. +- `clean-code-policy-pack`: Second semgrep config (`bugs.yaml`) added alongside `clean_code.yaml`; semgrep runner gains a second config load pass. +- `review-run-command`: New `--bug-hunt`, `--mode`, `--focus`, and `--level` flags wired through `ReviewRunRequest` and `run_command`; file resolution and report rendering respect focus and severity floor; enforcement mode overrides exit code in shadow. +- `review-cli-contracts`: CLI contract scenarios extended for the new flags and guardrails. + +## Impact + +- `packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py` — CrossHair timeout params, MISSING_ICONTRACT suppression logic +- `packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py` — second config pass for `bugs.yaml` +- `packages/specfact-code-review/src/specfact_code_review/run/commands.py` — `--bug-hunt`, `--mode`, `--focus`, `--level`; extend `ReviewRunRequest`; apply focus to resolved files, severity filter and shadow exit override before render +- `packages/specfact-code-review/src/specfact_code_review/review/commands.py` — Typer options and validation for new flags; thread into `run_command` +- `packages/specfact-code-review/src/specfact_code_review/run/runner.py` — pass `bug_hunt` through to tool steps; TDD gate skip messages when pytest/cov unavailable; after tools produce raw findings, apply `--level` filtering and call `score_review` on the filtered list so `ReviewReport` fields stay aligned; apply `--mode shadow` exit override on the assembled report +- `packages/specfact-code-review/src/specfact_code_review/tools/*.py` — early availability checks; standardized skip `tool_error` messages (replace misleading “parse output” errors when the executable is missing) +- `packages/specfact-code-review/module-package.yaml` — audit `pip_dependencies` against the canonical tool map; add any missing packages when new runners ship +- `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` — scenarios for mode, focus, level, and conflict errors +- `packages/specfact-code-review/.semgrep/bugs.yaml` — new file +- `packages/specfact-codebase/` — sidecar framework extractors: exclude `.specfact/` from scan paths +- Versioning: patch bump `specfact-codebase` for the sidecar fix; patch or minor bump `specfact-code-review` when behaviour and manifest changes ship (tooling + CLI surface) + +## Source Tracking + +- **GitHub Issue**: [#174](https://github.com/nold-ai/specfact-cli-modules/issues/174) +- **Repository**: nold-ai/specfact-cli-modules +- **Parent Feature**: [#175](https://github.com/nold-ai/specfact-cli-modules/issues/175) — Code review external repo quality and bug finding +- **Epic**: [#162](https://github.com/nold-ai/specfact-cli-modules/issues/162) +- **OpenSpec change folder**: `code-review-bug-finding-and-sidecar-venv-fix` +- **Change order**: `openspec/CHANGE_ORDER.md` — Pending → “Code review and sidecar validation improvements” diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md new file mode 100644 index 00000000..26f46c5f --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md @@ -0,0 +1,55 @@ +# Code Review Bug Finding + +## ADDED Requirements + +### Requirement: Semgrep bug-finding rules pass + +The system SHALL run a second semgrep pass using a `bugs.yaml` config alongside +the existing `clean_code.yaml` pass. The `bugs.yaml` config SHALL cover Python +security and correctness patterns. When `bugs.yaml` is absent the pass SHALL be +silently skipped without error. + +#### Scenario: bugs.yaml present — security findings emitted + +- **WHEN** `.semgrep/bugs.yaml` exists in the bundle +- **AND** `run_semgrep_bugs` is called on Python files matching a bug rule +- **THEN** `ReviewFinding` records are returned with `category="security"` or `category="clean_code"` +- **AND** findings reference the matched rule id from `bugs.yaml` + +#### Scenario: bugs.yaml absent — pass is a no-op + +- **WHEN** no `.semgrep/bugs.yaml` file is discoverable +- **AND** `run_semgrep_bugs` is called +- **THEN** no finding is returned for the missing bugs pass +- **AND** no exception propagates to the caller + +#### Scenario: bugs.yaml findings are included in the JSON report + +- **WHEN** `specfact code review run --json` is executed on a file matching a bug rule +- **THEN** the output JSON contains findings from both the clean-code and bug-finding passes +- **AND** `run_review` wires `run_semgrep_bugs` alongside the existing `run_semgrep` pass + +### Requirement: CrossHair bug-hunt mode + +The system SHALL support a `--bug-hunt` flag on `specfact code review run` that +increases the CrossHair per-path timeout to 10 seconds and the total CrossHair +timeout to 120 seconds. Without `--bug-hunt` the existing timeouts SHALL remain +unchanged. + +#### Scenario: --bug-hunt increases CrossHair timeouts + +- **WHEN** `specfact code review run --bug-hunt --json ` is executed +- **THEN** CrossHair runs with `--per_path_timeout 10` +- **AND** the total CrossHair subprocess timeout is 120 seconds + +#### Scenario: Default run uses original CrossHair timeouts + +- **WHEN** `specfact code review run --json ` is executed without `--bug-hunt` +- **THEN** CrossHair runs with `--per_path_timeout 2` +- **AND** the total CrossHair subprocess timeout is 30 seconds + +#### Scenario: --bug-hunt is composable with --scope and --out + +- **WHEN** `specfact code review run --bug-hunt --scope full --json --out report.json` is executed +- **THEN** the command completes without error +- **AND** the output JSON is written to `report.json` diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-tool-dependencies/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-tool-dependencies/spec.md new file mode 100644 index 00000000..4da6c600 --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-tool-dependencies/spec.md @@ -0,0 +1,66 @@ +## ADDED Requirements + +### Requirement: pip_dependencies cover all external review tools + +The `specfact-code-review` bundle manifest (`packages/specfact-code-review/module-package.yaml`) SHALL declare, under `pip_dependencies`, every PyPI distribution required so that **all external** tools invoked by the default `run_review` pipeline (including the TDD gate and CrossHair) can run in a normal bundle install. + +#### Scenario: Manifest includes core CLI tools + +- **WHEN** a maintainer inspects `module-package.yaml` +- **THEN** `pip_dependencies` includes packages that provide the `ruff`, `radon`, `semgrep`, `basedpyright`, `pylint`, and `crosshair` CLIs on `PATH` after install +- **AND** `pytest` and `pytest-cov` are listed for the targeted test / coverage subprocess + +#### Scenario: New runner adds a new external executable + +- **WHEN** a new subprocess-backed review step is added to the pipeline +- **THEN** the change updates `pip_dependencies` and the canonical tool map (see design D9) in the same delivery + +### Requirement: Runtime detection skips missing tools with a clear tool_error + +Before each external tool subprocess runs, the implementation SHALL verify the tool is available. If the executable is not found on `PATH` (or pytest cannot be launched as today’s gate requires), the implementation SHALL **not** invoke that tool and SHALL return **exactly one** `ReviewFinding` per skipped tool. + +#### Scenario: Missing Ruff executable produces a skip finding + +- **GIVEN** `ruff` is not on `PATH` +- **WHEN** `run_ruff` is executed for a non-empty file list +- **THEN** no `ruff` subprocess is started +- **AND** exactly one finding is returned with `category="tool_error"` and `tool="ruff"` +- **AND** the message states that review checks for `ruff` were **skipped** because it is **not installed** or unavailable, and names the pip package to install (e.g. `ruff`) + +#### Scenario: Missing Semgrep does not raise + +- **GIVEN** `semgrep` is not on `PATH` +- **WHEN** the semgrep review step runs +- **THEN** the step returns a single skip finding as above for `semgrep` +- **AND** no uncaught exception propagates + +#### Scenario: AST clean-code pass requires no extra CLI dependency + +- **WHEN** `run_ast_clean_code` runs +- **THEN** it does not depend on a `pip_dependencies` CLI entry (stdlib / in-package Python only) + +#### Scenario: TDD gate reports pytest unavailable clearly + +- **GIVEN** `pytest` (or coverage support) is missing such that the targeted test subprocess cannot run +- **WHEN** `_evaluate_tdd_gate` would run tests +- **THEN** findings include a `tool_error` for `pytest` (or the agreed tool id) with a skip / not-installed style message referencing `pytest` and/or `pytest-cov` + +### Requirement: No misleading errors when the binary is absent + +If a tool executable is missing, runners SHALL NOT surface generic failures such as “Unable to parse JSON output” that imply the tool ran but returned bad data. + +#### Scenario: Ruff missing is not reported as a parse failure + +- **GIVEN** `ruff` is absent +- **WHEN** `run_ruff` completes +- **THEN** the user-visible message indicates the tool was **skipped** / **not installed**, not a JSON parse error from Ruff output + +### Requirement: Automated guard for manifest vs tool map + +The repository SHALL include an automated check (unit test or small validation script invoked by the normal test suite) that fails if `module-package.yaml` `pip_dependencies` omits any package from the canonical tool → pip map for the default pipeline. + +#### Scenario: Drift fails CI + +- **GIVEN** the canonical map lists a pip package for an active runner +- **WHEN** that package is removed from `pip_dependencies` without updating the map +- **THEN** `hatch run test` (or the chosen gate) fails with a clear assertion message diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md new file mode 100644 index 00000000..5f8705d6 --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md @@ -0,0 +1,63 @@ +# Contract Runner + +## MODIFIED Requirements + +### Requirement: icontract Decorator AST Scan and CrossHair Fast Pass + +The system SHALL AST-scan reviewed Python files for public functions missing +`@require` / `@ensure` decorators, and run CrossHair with a configurable +per-path timeout for counterexample discovery. When no icontract usage is +detected in the reviewed files' package/repo scan roots, `MISSING_ICONTRACT` findings SHALL be +suppressed entirely for that run. + +#### Scenario: Public function without icontract decorators produces a contracts finding when icontract is in use + +- **GIVEN** a Python file with a public function lacking icontract decorators +- **AND** at least one file in the reviewed batch's package/repo scan roots imports from `icontract` +- **WHEN** `run_contract_check(files=[...])` is called +- **THEN** a `ReviewFinding` is returned with `category="contracts"` and + `severity="warning"` + +#### Scenario: MISSING_ICONTRACT suppressed when no icontract usage detected + +- **GIVEN** the package/repo scan roots for the reviewed Python files contain no `from icontract import` or + `import icontract` statements +- **WHEN** `run_contract_check(files=[...])` is called +- **THEN** no `MISSING_ICONTRACT` findings are returned +- **AND** CrossHair still runs on the files + +#### Scenario: Decorated public function produces no contracts finding + +- **GIVEN** a Python file with a public function decorated with both `@require` and + `@ensure` +- **WHEN** `run_contract_check(files=[...])` is called +- **THEN** no contract-related finding is returned for that function + +#### Scenario: Private functions are excluded from the scan + +- **GIVEN** a Python file with a function named `_private_helper` and no icontract + decorators +- **WHEN** `run_contract_check(files=[...])` is called +- **THEN** no finding is produced for `_private_helper` + +#### Scenario: CrossHair counterexample maps to a contracts warning + +- **GIVEN** CrossHair finds a counterexample for a reviewed function +- **WHEN** `run_contract_check(files=[...])` is called +- **THEN** a `ReviewFinding` is returned with `category="contracts"`, + `severity="warning"`, and `tool="crosshair"` + +#### Scenario: CrossHair timeout or unavailability degrades gracefully + +- **GIVEN** CrossHair times out or is not installed +- **WHEN** `run_contract_check(files=[...])` is called +- **THEN** the AST scan still runs +- **AND** no exception propagates to the caller +- **AND** CrossHair unavailability does not produce a blocking finding + +#### Scenario: Bug-hunt mode uses extended CrossHair timeouts + +- **GIVEN** `run_contract_check(files=[...], bug_hunt=True)` is called +- **WHEN** CrossHair executes +- **THEN** CrossHair runs with `--per_path_timeout 10` +- **AND** the subprocess timeout is 120 seconds diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md new file mode 100644 index 00000000..92256946 --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md @@ -0,0 +1,42 @@ +# Review CLI Contracts + +## ADDED Requirements + +### Requirement: Review-run CLI scenarios cover enforcement mode, bug-hunt, focus facets, and severity level + +The modules repository SHALL extend `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` so contract tests exercise the new `specfact code review run` flags together with existing scope and JSON output behaviour. + +#### Scenario: Scenarios cover shadow versus enforce exit behaviour + +- **GIVEN** `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` +- **WHEN** it is validated after this change +- **THEN** it includes at least one scenario asserting `--mode shadow` yields process success (exit `0`) while JSON still reports a failing verdict when findings warrant it +- **AND** it includes a control scenario showing `--mode enforce` (or default) preserves non-zero exit on blocking failures + +#### Scenario: Scenarios cover --bug-hunt in shadow and enforce modes + +- **GIVEN** `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` +- **WHEN** it is validated after this change +- **THEN** it includes at least one scenario with `--bug-hunt --mode shadow` +- **AND** it includes at least one scenario with `--bug-hunt --mode enforce` + +#### Scenario: Scenarios cover --focus facets + +- **GIVEN** the same scenario file +- **WHEN** it is validated +- **THEN** it includes coverage for `--focus` union behaviour (e.g. `source` + `docs`) and for `--focus tests` narrowing the file set +- **AND** it includes coverage for `--bug-hunt` composed with `--focus` + +#### Scenario: Scenarios cover --level filtering + +- **GIVEN** the same scenario file +- **WHEN** it is validated +- **THEN** it includes at least one scenario where `--level error` removes warnings from the JSON `findings` list +- **AND** it includes coverage for `--bug-hunt` composed with `--level error` + +#### Scenario: Scenarios cover invalid flag combinations + +- **GIVEN** the same scenario file +- **WHEN** it is validated +- **THEN** it includes an error-path scenario for `--focus` combined with `--include-tests` or `--exclude-tests` +- **AND** `--bug-hunt` remains composable with test-selection flags diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md new file mode 100644 index 00000000..c8cf4e13 --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md @@ -0,0 +1,107 @@ +# Review Run Command Specification + +## ADDED Requirements + +### Requirement: --bug-hunt flag on review run command + +The `specfact code review run` command SHALL accept a `--bug-hunt` flag that +enables extended CrossHair timeouts and is composable with all existing flags. + +#### Scenario: --bug-hunt flag accepted without error + +- **GIVEN** `specfact code review run --bug-hunt --json ` is executed +- **WHEN** the command parses its arguments +- **THEN** the command proceeds without a CLI argument error +- **AND** `ReviewRunRequest.bug_hunt` is `True` + +#### Scenario: --bug-hunt flag absent defaults to False + +- **GIVEN** `specfact code review run --json ` is executed without `--bug-hunt` +- **WHEN** the command parses its arguments +- **THEN** `ReviewRunRequest.bug_hunt` is `False` +- **AND** CrossHair uses the standard 2-second per-path timeout + +### Requirement: --mode shadow and --mode enforce + +The `specfact code review run` command SHALL accept `--mode shadow` or `--mode enforce`. + +#### Scenario: Default mode is enforce + +- **GIVEN** `specfact code review run` is invoked without `--mode` +- **WHEN** the command parses its arguments +- **THEN** enforcement behaves as today: `ci_exit_code` reflects blocking findings + +#### Scenario: Shadow mode never returns a failing process exit + +- **GIVEN** a review run that would yield `ci_exit_code == 1` under enforce semantics +- **WHEN** `specfact code review run --mode shadow` completes +- **THEN** the process exit code is `0` +- **AND** `ReviewReport.ci_exit_code` in JSON is `0` +- **AND** `overall_verdict` still reflects the computed verdict (including `FAIL` when applicable) + +#### Scenario: Enforce mode matches legacy exit behaviour + +- **GIVEN** the same findings payload as today for a failing run +- **WHEN** `specfact code review run --mode enforce` completes +- **THEN** process exit and `ci_exit_code` match the pre-change `enforce` default + +#### Scenario: --mode composes with --bug-hunt and --json + +- **WHEN** `specfact code review run --bug-hunt --mode shadow --json --out report.json` is executed +- **THEN** the command parses successfully +- **AND** CrossHair uses bug-hunt timeouts +- **AND** the process exits `0` even if findings would fail under enforce semantics + +### Requirement: Repeatable --focus for source, tests, and docs + +The command SHALL accept repeated `--focus` options with values `source`, `tests`, and `docs`. When at least one `--focus` is present, the reviewed Python file set SHALL be the intersection of the scope-resolved files with the **union** of the selected facets: + +- `tests`: files where `tests` appears in the path’s directory components (same rule as existing test detection). +- `docs`: Python files where `docs` appears in the path’s directory components. +- `source`: Python files that match neither the `tests` nor the `docs` facet. + +#### Scenario: --focus tests restricts to test paths + +- **GIVEN** a repository with both `src/app.py` and `tests/test_app.py` in scope +- **WHEN** `specfact code review run --scope full --focus tests --json` runs +- **THEN** only files under the `tests` facet are analyzed + +#### Scenario: Union of multiple focuses + +- **GIVEN** scope includes `src/a.py`, `tests/t.py`, and `docs/conf.py` +- **WHEN** `specfact code review run --scope full --focus source --focus docs --json` runs +- **THEN** `tests/t.py` is excluded +- **AND** `src/a.py` and `docs/conf.py` are included + +#### Scenario: --focus conflicts with --include-tests + +- **WHEN** `specfact code review run --focus source --include-tests` is parsed +- **THEN** the CLI rejects the combination with a clear error + +#### Scenario: --focus conflicts with --exclude-tests + +- **WHEN** `specfact code review run --focus tests --exclude-tests` is parsed +- **THEN** the CLI rejects the combination with a clear error + +### Requirement: --level error and --level warning + +The command SHALL accept `--level error` or `--level warning` to filter findings **before** scoring and verdict. + +#### Scenario: --level error drops warnings and info + +- **GIVEN** a run that produces both `warning` and `error` severity findings +- **WHEN** `specfact code review run --level error --json` completes +- **THEN** the JSON `findings` list contains only `severity == "error"` items +- **AND** score and verdict are computed from that filtered list + +#### Scenario: --level warning retains errors and warnings + +- **GIVEN** a run that produces `info`, `warning`, and `error` findings +- **WHEN** `specfact code review run --level warning --json` completes +- **THEN** the JSON `findings` list contains no `severity == "info"` items +- **AND** score and verdict are computed from the filtered list + +#### Scenario: Omitted --level keeps all severities + +- **WHEN** `specfact code review run --json` runs without `--level` +- **THEN** all severities appear in output as they do today diff --git a/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md new file mode 100644 index 00000000..214dc426 --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md @@ -0,0 +1,41 @@ +# Sidecar Route Extraction + +## ADDED Requirements + +### Requirement: Framework extractors exclude .specfact from scan paths + +All sidecar framework extractors (FastAPI, Flask, Django, DRF) SHALL exclude +`.specfact/` directories from Python file discovery. This prevents the sidecar's +own installed venv and workspace artefacts from being scanned as application +source. + +#### Scenario: FastAPI extractor ignores .specfact/venv + +- **GIVEN** a repo with a `.specfact/venv/` directory containing FastAPI source +- **WHEN** the FastAPI extractor runs route extraction on that repo +- **THEN** no routes are extracted from files under `.specfact/` +- **AND** only routes from application source files are returned + +#### Scenario: Flask extractor ignores .specfact/venv + +- **GIVEN** a repo with a `.specfact/venv/` directory containing Flask source +- **WHEN** the Flask extractor runs route extraction on that repo +- **THEN** no routes are extracted from files under `.specfact/` + +#### Scenario: Django extractor ignores .specfact/venv + +- **GIVEN** a repo with a `.specfact/venv/` directory containing Django source +- **WHEN** the Django extractor runs route extraction on that repo +- **THEN** no routes are extracted from files under `.specfact/` + +#### Scenario: Other standard exclusions also apply + +- **WHEN** any framework extractor scans a repo +- **THEN** files under `.git/`, `__pycache__/`, and `node_modules/` are also excluded +- **AND** legitimate application source outside these directories is not affected + +#### Scenario: Route count reflects real application routes only + +- **GIVEN** gpt-researcher repo with 19 real FastAPI routes and `.specfact/venv` installed +- **WHEN** sidecar validation runs route extraction +- **THEN** routes extracted is approximately 19, not 25,947 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 new file mode 100644 index 00000000..7a545b5a --- /dev/null +++ b/openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md @@ -0,0 +1,57 @@ +# 1. Sidecar venv self-scan fix (specfact-codebase) + +- [x] 1.1 Add `_EXCLUDED_DIR_NAMES` constant and `_iter_python_files(root)` generator to `BaseFrameworkExtractor` in `frameworks/base.py` that skips `.specfact`, `.git`, `__pycache__`, `node_modules` +- [x] 1.2 Replace `search_path.rglob("*.py")` with `self._iter_python_files(search_path)` in `FastAPIExtractor.detect()` and `FastAPIExtractor.extract_routes()` +- [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) + +- [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) + +- [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) + +- [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) + +- [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) + +- [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 + +- [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 `hatch 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/openspec/config.yaml b/openspec/config.yaml index 066eb77d..6a7e6415 100644 --- a/openspec/config.yaml +++ b/openspec/config.yaml @@ -30,7 +30,7 @@ context: | Quality & CI (typical order): `format` → `type-check` → `lint` → `yaml-lint` → module **signature verification** (`verify-modules-signature`, enforce version bump when manifests change) → `contract-test` → `smart-test` → `test`. Pre-commit: `pre-commit-verify-modules-signature.sh` (branch-aware `--require-signature` when target is - `main`: local branch `main` or `GITHUB_BASE_REF=main` on PR events), + `main`: local branch `main` or `GITHUB_BASE_REF=main` on PR events; otherwise baseline payload checksum + version bump), then split `pre-commit-quality-checks.sh` hooks (format, yaml-if-staged, bundle, lint-if-staged, then block2: `pre_commit_code_review.py` + contract tests; JSON report under `.specfact/`). Hatch default env sets `SPECFACT_MODULES_REPO={root}`; `apply_specfact_workspace_env` also sets diff --git a/openspec/specs/modules-pre-commit-quality-parity/spec.md b/openspec/specs/modules-pre-commit-quality-parity/spec.md index d03c0991..7c61762d 100644 --- a/openspec/specs/modules-pre-commit-quality-parity/spec.md +++ b/openspec/specs/modules-pre-commit-quality-parity/spec.md @@ -13,7 +13,7 @@ The modules repo pre-commit configuration SHALL fail a commit when module payloa - **THEN** the hook set includes an always-run signature verification command - **AND** that command always enforces filesystem payload checksums and version-bump policy (`--payload-from-filesystem --enforce-version-bump`) - **AND** when the active Git branch is `main`, or GitHub Actions sets `GITHUB_BASE_REF` to `main` (PR target branch), that command also enforces `--require-signature` -- **AND** on any other branch (for example `dev` or a feature branch), that command SHALL NOT pass `--require-signature`, matching `pr-orchestrator` behavior for non-`main` targets +- **AND** on any other branch (for example `dev` or a feature branch), that command SHALL NOT pass `--require-signature` and SHALL NOT pass `--metadata-only`, matching `pr-orchestrator` behavior for PRs whose base is not `main` (full payload checksum + version bump without cryptographic signature on the branch head) ### Requirement: Modules Repo Pre-Commit Must Catch Formatting And Quality Drift Early diff --git a/packages/specfact-code-review/.semgrep/bugs.yaml b/packages/specfact-code-review/.semgrep/bugs.yaml new file mode 100644 index 00000000..bb66f91b --- /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: (?s)yaml\.load\([\s\S]*?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 68caac35..7483d4f0 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.9 commands: - code tier: official @@ -23,5 +23,5 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:ee7d224ac2a7894cc67d3e052764137b034e089624d5007989cab818839e5449 - signature: V+GNklTfgmdYKDWgp53SDw4s1R5GE1UF/745CVnXFJ0v3WSCWLY8APqyuabetBU6/Z1UQ1lKfEiRiZOFJw7WBg== + checksum: sha256:64d68f426a6140fdff3ceffd51c3960fb36745a040068722ae37271dc378659e + signature: PcP7OeNMkZqTtRpRBkpr2v2od0RX7fM4VCKXyWAYyP7bBRsKyqyRJI5oxX/0BEIWPt29o6y4t93WyJisWgWKBQ== 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 33323add..bbefd78a 100644 --- a/packages/specfact-code-review/src/specfact_code_review/_review_utils.py +++ b/packages/specfact-code-review/src/specfact_code_review/_review_utils.py @@ -31,6 +31,18 @@ def normalize_path_variants(path_value: str | Path) -> set[str]: return variants +_PYTHON_LINTER_SUFFIXES = frozenset({".py", ".pyi"}) + + +@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]: + """Python source and type stub paths linters/typecheckers should analyze.""" + return [path for path in files if path.suffix in _PYTHON_LINTER_SUFFIXES] + + @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 3021d668..2ad7606e 100644 --- a/packages/specfact-code-review/src/specfact_code_review/review/commands.py +++ b/packages/specfact-code-review/src/specfact_code_review/review/commands.py @@ -11,23 +11,32 @@ from specfact_code_review.ledger.commands import app as ledger_app from specfact_code_review.rules.commands import app as rules_app -from specfact_code_review.run.commands import run_command +from specfact_code_review.run.commands import ( + ConflictingScopeError, + InvalidOptionCombinationError, + MissingOutForJsonError, + NoReviewableFilesError, + RunCommandError, + run_command, +) app = typer.Typer(help="Code command extensions for structured review workflows.", no_args_is_help=True) review_app = typer.Typer(help="Governed code review workflows.", no_args_is_help=True) -def _friendly_run_command_error(exc: ValueError | ViolationError) -> str: - message = str(exc) - for expected in ( - "Use either --json or --score-only, not both.", - "Use --out together with --json.", - "Choose positional files or auto-scope controls, not both.", +def _friendly_run_command_error(exc: RunCommandError | ValueError | ViolationError) -> str: + if isinstance( + exc, + ( + InvalidOptionCombinationError, + MissingOutForJsonError, + ConflictingScopeError, + NoReviewableFilesError, + ), ): - if expected in message: - return expected - return message + return str(exc) + return str(exc) def _resolve_include_tests(*, files: list[Path], include_tests: bool | None, interactive: bool) -> bool: @@ -40,6 +49,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 = "tests" in focus_list + 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") @@ -48,8 +91,12 @@ def run( files: list[Path] = typer.Argument(None), scope: Literal["changed", "full"] = typer.Option(None), path: list[Path] = typer.Option(None, "--path"), - include_tests: bool = typer.Option(None, "--include-tests"), - exclude_tests: bool = typer.Option(None, "--exclude-tests"), + include_tests: bool | None = typer.Option(None, "--include-tests"), + exclude_tests: bool | None = 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 +107,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 b570b0d0..dd37c658 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 9be7a20b..1e02c507 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,30 @@ console = Console() progress_console = Console(stderr=True) AutoScope = Literal["changed", "full"] +ReviewRunMode = Literal["shadow", "enforce"] +ReviewLevelFilter = Literal["error", "warning"] + + +class RunCommandError(ValueError): + """Structured validation error for review run command options.""" + + error_code = "run_command_error" + + +class InvalidOptionCombinationError(RunCommandError): + error_code = "invalid_option_combination" + + +class MissingOutForJsonError(RunCommandError): + error_code = "missing_out_for_json" + + +class ConflictingScopeError(RunCommandError): + error_code = "conflicting_scope" + + +class NoReviewableFilesError(RunCommandError): + error_code = "no_reviewable_files" @dataclass(frozen=True) @@ -38,12 +62,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) @@ -58,7 +120,7 @@ def _git_file_list(command: list[str], *, error_message: str) -> list[Path]: timeout=30, ) if result.returncode != 0: - raise ValueError(error_message) + raise RunCommandError(error_message) return [Path(line.strip()) for line in result.stdout.splitlines() if line.strip()] @@ -75,7 +137,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 +157,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)) @@ -110,7 +172,7 @@ def _filtered_files(files: Iterable[Path], *, path_filters: list[Path]) -> list[ normalized_filters = [path_filter for path_filter in path_filters if str(path_filter).strip()] for path_filter in normalized_filters: if path_filter.is_absolute(): - raise ValueError(f"Path filters must be repo-relative: {path_filter}") + raise RunCommandError(f"Path filters must be repo-relative: {path_filter}") return [ file_path for file_path in files @@ -130,14 +192,16 @@ def _raise_if_targeting_styles_conflict( path_filters: list[Path], ) -> None: if files and (scope is not None or path_filters): - raise ValueError("Choose positional files or auto-scope controls, not both.") + raise ConflictingScopeError("Choose positional files or auto-scope controls, not both.") def _resolve_positional_files(files: list[Path]) -> list[Path]: resolved = [file_path for file_path in files if not _is_ignored_review_path(file_path)] if resolved: return resolved - raise ValueError("No Python files to review were provided or detected from tracked or untracked changes.") + raise NoReviewableFilesError( + "No Python files to review were provided or detected from tracked or untracked changes." + ) def _resolve_auto_discovered_files( @@ -165,7 +229,7 @@ def _resolve_changed_scope_files(*, include_tests: bool, path_filters: list[Path def _raise_for_empty_auto_scope(*, scope: AutoScope, path_filters: list[Path]) -> None: auto_scope_message = _auto_scope_message(scope=scope, path_filters=path_filters) - raise ValueError( + raise NoReviewableFilesError( f"No reviewable files matched the selected auto-scope controls ({auto_scope_message}). " "Adjust --scope/--path or pass positional files." ) @@ -196,7 +260,7 @@ def _resolve_files( missing = [file_path for file_path in resolved if not file_path.is_file()] if missing: - raise ValueError(f"File not found: {missing[0]}") + raise NoReviewableFilesError(f"File not found: {missing[0]}") return resolved @@ -277,19 +341,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 +379,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 @@ -360,7 +439,7 @@ def _as_auto_scope(value: object) -> AutoScope | None: return None if isinstance(value, str) and value in {"changed", "full"}: return cast(AutoScope, value) - raise ValueError(f"Invalid scope value: {value!r}") + raise RunCommandError(f"Invalid scope value: {value!r}") def _as_path_filters(value: object) -> list[Path] | None: @@ -368,7 +447,7 @@ def _as_path_filters(value: object) -> list[Path] | None: return None if isinstance(value, list) and all(isinstance(path_filter, Path) for path_filter in value): return value - raise ValueError("Path filters must be a list of Path instances.") + raise RunCommandError("Path filters must be a list of Path instances.") def _as_optional_path(value: object) -> Path | None: @@ -376,7 +455,34 @@ def _as_optional_path(value: object) -> Path | None: return None if isinstance(value, Path): return value - raise ValueError("Output path must be a Path instance.") + raise RunCommandError("Output path must be a Path instance.") + + +def _as_review_mode(value: object) -> ReviewRunMode: + if value is None or value == "enforce": + return "enforce" + if value == "shadow": + return "shadow" + raise RunCommandError(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 RunCommandError(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 RunCommandError(f"Invalid focus facet: {item!r}") + return tuple(value) + raise RunCommandError("focus facets must be a list or tuple of strings") def _build_review_run_request( @@ -385,9 +491,9 @@ def _build_review_run_request( ) -> ReviewRunRequest: # Validate files is a list of Path instances if not isinstance(files, list): - raise ValueError(f"files must be a list, got {type(files).__name__}") + raise RunCommandError(f"files must be a list, got {type(files).__name__}") if not all(isinstance(file_path, Path) for file_path in files): - raise ValueError("files must contain only Path instances") + raise RunCommandError("files must contain only Path instances") request_kwargs = dict(kwargs) @@ -397,7 +503,7 @@ def _get_bool_param(name: str, default: bool = False) -> bool: if value is None: return default if not isinstance(value, bool): - raise ValueError(f"{name} must be a boolean, got {type(value).__name__}") + raise RunCommandError(f"{name} must be a boolean, got {type(value).__name__}") return value # Validate and extract known path/scope parameters @@ -412,7 +518,7 @@ def _get_optional_param(name: str, validator: Callable[[object], object], defaul include_tests = False # default value if include_tests_value is not None: if not isinstance(include_tests_value, bool): - raise ValueError(f"include_tests must be a boolean, got {type(include_tests_value).__name__}") + raise RunCommandError(f"include_tests must be a boolean, got {type(include_tests_value).__name__}") include_tests = include_tests_value # Get optional parameters with proper type casting @@ -425,6 +531,8 @@ 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))) + request = ReviewRunRequest( files=files, include_tests=include_tests, @@ -436,12 +544,16 @@ 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 if request_kwargs: unexpected = ", ".join(sorted(request_kwargs)) - raise ValueError(f"Unexpected keyword arguments: {unexpected}") + raise RunCommandError(f"Unexpected keyword arguments: {unexpected}") return request @@ -461,9 +573,9 @@ def _render_review_result(report: ReviewReport, request: ReviewRunRequest) -> tu def _validate_review_request(request: ReviewRunRequest) -> None: if request.json_output and request.score_only: - raise ValueError("Use either --json or --score-only, not both.") + raise InvalidOptionCombinationError("Use either --json or --score-only, not both.") if not request.json_output and request.out is not None: - raise ValueError("Use --out together with --json.") + raise MissingOutForJsonError("Use --out together with --json.") @beartype @@ -487,19 +599,39 @@ def run_command( ) _validate_review_request(request) + include_for_resolve = request.include_tests or bool(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 NoReviewableFilesError( + "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) -__all__ = ["ReviewRunRequest", "run_command"] +__all__ = [ + "ConflictingScopeError", + "InvalidOptionCombinationError", + "MissingOutForJsonError", + "NoReviewableFilesError", + "ReviewRunRequest", + "RunCommandError", + "run_command", +] 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 53426ccd..fab4d2f4 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 7b122beb..ab5842fe 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 17a253dc..3746b53d 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 fd04bf0e..e6bed716 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py @@ -10,12 +10,159 @@ 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:",) +_ICONTRACT_SCAN_EXCLUDED_DIRS = frozenset( + {".git", ".mypy_cache", ".pytest_cache", ".ruff_cache", "__pycache__", "venv", ".venv"} +) + + +def _repo_root_from_review_paths(files: list[Path]) -> Path | None: + """Locate the git repository root from any reviewed path (``.git`` file or directory).""" + for file_path in files: + try: + resolved = file_path.resolve() + except OSError: + continue + for parent in [resolved, *resolved.parents]: + if (parent / ".git").exists(): + return parent + return None + + +def _icontract_usage_scan_roots(files: list[Path]) -> list[Path]: + """Bundle scan roots for icontract discovery, using paths relative to the repo root when known.""" + roots: list[Path] = [] + repo_root = _repo_root_from_review_paths(files) + repo_resolved: Path | None = None + if repo_root is not None: + try: + repo_resolved = repo_root.resolve() + except OSError: + repo_resolved = None + + for file_path in files: + if repo_resolved is not None: + try: + rel_parts = file_path.resolve().relative_to(repo_resolved).parts + except (OSError, ValueError): + rel_parts = () + if rel_parts and rel_parts[0] == "packages" and len(rel_parts) > 1: + roots.append(repo_resolved / "packages" / rel_parts[1]) + continue + else: + try: + abs_parts = file_path.resolve().parts + except OSError: + abs_parts = file_path.parts + if abs_parts and abs_parts[0] == "packages" and len(abs_parts) > 1: + roots.append(Path(*abs_parts[:2])) + continue + try: + roots.append(file_path.resolve().parent) + except OSError: + roots.append(file_path.parent) + + return list(dict.fromkeys(roots)) + + +def _iter_icontract_usage_candidates(root: Path) -> list[Path]: + if not root.exists() or not root.is_dir(): + return [] + collected: list[Path] = [] + for path in root.rglob("*"): + if not path.is_file(): + continue + if path.suffix not in {".py", ".pyi"}: + continue + if any(part in _ICONTRACT_SCAN_EXCLUDED_DIRS for part in path.parts): + continue + collected.append(path) + return sorted(collected, key=lambda candidate: candidate.as_posix()) + + +def _review_paths_include_repo_packages_tree(py_files: list[Path]) -> bool: + """True when any reviewed path lies under ``/packages/…`` (strict repo-relative prefix). + + Without a discoverable git root, fall back to ``\"packages\" in resolved.parts`` so callers + outside a normal repo layout still participate in the monorepo ``packages/`` scan when appropriate. + """ + repo_root = _repo_root_from_review_paths(py_files) + if repo_root is None: + for path in py_files: + try: + if "packages" in path.resolve().parts: + return True + except OSError: + if "packages" in path.parts: + return True + return False + try: + repo_resolved = repo_root.resolve() + except OSError: + for path in py_files: + try: + if "packages" in path.resolve().parts: + return True + except OSError: + continue + return False + for path in py_files: + try: + rel = path.resolve().relative_to(repo_resolved) + except (OSError, ValueError): + continue + if rel.parts and rel.parts[0] == "packages": + return True + return False + + +def _root_imports_icontract(root: Path) -> bool: + """True when any ``.py`` / ``.pyi`` file under ``root`` imports the ``icontract`` package.""" + return any(_file_imports_icontract(file_path) for file_path in _iter_icontract_usage_candidates(root)) + + +def _has_icontract_usage(py_files: list[Path]) -> bool: + """True when icontract is used in any per-path scan root or elsewhere under ``packages/``. + + Review runs often pass only the changed file under ``packages//…``. Icontract may live + in a sibling module under the same ``packages/`` tree; a per-bundle root scan alone would miss + that signal and incorrectly skip ``MISSING_ICONTRACT`` for the edited file. + """ + for root in dict.fromkeys(_icontract_usage_scan_roots(py_files)): + if _root_imports_icontract(root): + return True + repo_root = _repo_root_from_review_paths(py_files) + if repo_root is None: + return False + if not _review_paths_include_repo_packages_tree(py_files): + return False + bundled = repo_root / "packages" + if not bundled.is_dir(): + return False + return _root_imports_icontract(bundled) + + +def _file_imports_icontract(file_path: Path) -> bool: + try: + tree = ast.parse(file_path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, SyntaxError): + return False + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module == "icontract": + return True + if isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "icontract": + return True + return False + + _SYNC_RUNTIME_ICONTRACT_ENTRYPOINTS = { "bridge_probe.py", "bridge_sync.py", @@ -104,17 +251,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 +316,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 e194f90a..af333ff4 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 7e31382b..7955dba4 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,59 @@ def _kiss_nesting_findings( return findings +def _typer_cli_entrypoint_exempt(function_node: ast.FunctionDef | ast.AsyncFunctionDef, file_path: Path) -> 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 + normalized = str(file_path).replace("\\", "/") + # Stable path suffix: matches in-repo and user-scoped installs (~/.specfact/modules/.../src/...). + # Typer CLI handler `run(ctx: Context, ...)` in review.commands injects many option parameters by + # design; Radon CC would flag it spuriously. Exempt only that callback so other `run` symbols + # elsewhere still get complexity checks. + if function_node.name == "run" and normalized.endswith("specfact_code_review/review/commands.py"): + return True + if not _has_typer_command_decorator(function_node): + 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 _decorator_name_parts(decorator: ast.expr) -> tuple[str, ...]: + if isinstance(decorator, ast.Call): + return _decorator_name_parts(decorator.func) + if isinstance(decorator, ast.Name): + return (decorator.id,) + if isinstance(decorator, ast.Attribute): + return (*_decorator_name_parts(decorator.value), decorator.attr) + return () + + +def _has_typer_command_decorator(function_node: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: + for decorator in function_node.decorator_list: + parts = _decorator_name_parts(decorator) + if parts == ("command",) or parts[-1:] == ("command",): + return True + if parts[-1:] == ("callback",): + return True + return False + + 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, file_path): + 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 +321,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 93f1a9dc..c4da5ca7 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 7118cd7b..67fdb70f 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py @@ -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 "") @@ -193,7 +237,9 @@ def _category_for_rule(rule: str) -> SemgrepCategory | None: return None -def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> ReviewFinding | None: +def _extract_common_finding_fields( + item: dict[str, object], *, allowed_paths: set[str] +) -> tuple[str, str, int, str, dict[str, object]] | None: filename = item["path"] if not isinstance(filename, str): raise ValueError("semgrep filename must be a string") @@ -203,10 +249,6 @@ def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> raw_rule = item["check_id"] if not isinstance(raw_rule, str): raise ValueError("semgrep rule must be a string") - rule = _normalize_rule_id(raw_rule) - category = _category_for_rule(rule) - if category is None: - return None start = item["start"] if not isinstance(start, dict): @@ -222,6 +264,19 @@ def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> if not isinstance(message, str): raise ValueError("semgrep message must be a string") + return filename, raw_rule, line, message, extra + + +def _finding_from_result(item: dict[str, object], *, allowed_paths: set[str]) -> ReviewFinding | None: + extracted = _extract_common_finding_fields(item, allowed_paths=allowed_paths) + if extracted is None: + return None + filename, raw_rule, line, message, _extra = extracted + rule = _normalize_rule_id(raw_rule) + category = _category_for_rule(rule) + if category is None: + return None + return ReviewFinding( category=category, severity="warning", @@ -254,8 +309,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 +341,91 @@ 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: + extracted = _extract_common_finding_fields(item, allowed_paths=allowed_paths) + if extracted is None: + return None + filename, raw_rule, line, message, extra = extracted + rule = _normalize_bug_rule_id(raw_rule) + category = BUG_RULE_CATEGORY.get(rule) + if category is None: + return None + + 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. + + When ``semgrep`` is not on PATH, returns no findings: ``run_semgrep`` (first pass) already emits + the single skip finding for the missing tool. + """ + if not files: + return [] + + skipped = skip_if_tool_missing("semgrep", files[0]) + if skipped: + return [] + + config_path = find_semgrep_bugs_config(bundle_root=bundle_root) + if config_path is None: + return [] + + 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 00000000..a42b2fcd --- /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 9776e9d8..406505ca 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.9 commands: - code tier: official @@ -24,5 +24,5 @@ description: Official SpecFact codebase bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:436e9e3d0d56a6eae861c4a6bef0a8f805f00b8b5bd8b0e037c19dede4972117 - signature: FQOsqabH5ATcyLfjTVrVJPIC4KiuwwFbCQZL+BZAu6dFoOz/DLjk91NZAyc7z6oq5dpVfwMHFsKEYqzW4wlDDA== + checksum: sha256:b7e6e2893e4398abca25c0bba4862663ceda8c7ae4df97ed1ad840f7eef3d2d5 + signature: xl1MEWdJFcA9PKmvEA0/cJV+X0Wv4lHIDxXUEsj+iSbfQ5TGs4c4TYd8/OWq6WjQDzq4vAC9/0m11PdpuNgZDQ== 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 f97dadb9..d3abf46c 100644 --- a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py +++ b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py @@ -7,7 +7,10 @@ from __future__ import annotations +import os from abc import ABC, abstractmethod +from collections.abc import Iterator +from pathlib import Path from typing import Any from beartype import beartype @@ -31,6 +34,29 @@ 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 + def _path_touches_excluded_dir(self, path: Path) -> bool: + """True when any path component is an excluded dir (.specfact, venvs, VCS, caches, node_modules).""" + return any(part in self._EXCLUDED_DIR_NAMES for part in path.parts) + + @beartype + def _iter_python_files(self, root: Path) -> Iterator[Path]: + """Yield ``*.py`` files under ``root``, skipping excluded directory subtrees by path.""" + if not root.exists() or not root.is_dir(): + return + for dirpath, dirnames, filenames in os.walk(root): + current_dir = Path(dirpath) + dirnames[:] = [ + dirname for dirname in dirnames if not self._path_touches_excluded_dir(current_dir / dirname) + ] + for filename in filenames: + if filename.endswith(".py"): + yield current_dir / filename + @abstractmethod @beartype @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 e7093c76..913f6f17 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 66d89ad3..9af097ad 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 34a353f3..4e8e0392 100644 --- a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py +++ b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py @@ -17,46 +17,70 @@ from specfact_codebase.validators.sidecar.frameworks.base import BaseFrameworkExtractor, RouteInfo +_ROUTE_HTTP_METHODS = frozenset( + {"get", "post", "put", "delete", "patch", "options", "head", "trace"}, +) +_FASTAPI_EXTRA_EXCLUDED_DIR_NAMES = frozenset({".mypy_cache", ".pytest_cache", ".ruff_cache"}) + + +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 + + class FastAPIExtractor(BaseFrameworkExtractor): """FastAPI framework extractor.""" + @beartype + def _path_touches_excluded_dir(self, path: Path) -> bool: + return super()._path_touches_excluded_dir(path) or any( + part in _FASTAPI_EXTRA_EXCLUDED_DIR_NAMES for part in path.parts + ) + @beartype @require(lambda repo_path: repo_path.exists(), "Repository path must exist") @require(lambda repo_path: repo_path.is_dir(), "Repository path must be a directory") @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 self._path_touches_excluded_dir(file_path): + continue + content = _read_text_if_exists(file_path) + if content is not None and _content_suggests_fastapi(content): + return True - # 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 self._scan_known_app_files(search_path): + return True + + return False + @beartype + def _scan_known_app_files(self, search_path: Path) -> bool: + for py_file in self._iter_python_files(search_path): + if py_file.name not in {"main.py", "app.py"}: + continue + content = _read_text_if_exists(py_file) + if content is not None and _content_suggests_fastapi(content): + return True return False @beartype @@ -65,21 +89,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 self._iter_python_files(search_path): try: routes = self._extract_routes_from_file(py_file) results.extend(routes) @@ -94,17 +117,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,69 +139,94 @@ 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) - if route_info: - results.append(route_info) + results.extend(self._extract_routes_from_function(node)) 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 _extract_route_from_function( - self, func_node: ast.FunctionDef, imports: dict[str, str], py_file: Path - ) -> RouteInfo | None: - """Extract route information from a function with FastAPI decorators.""" + def _path_methods_from_api_route_call(self, decorator: ast.Call) -> list[tuple[str, str]]: + """If ``decorator`` is ``@router.api_route(path, methods=[...])``, return all methods + path.""" + if not isinstance(decorator.func, ast.Attribute): + return [] + if decorator.func.attr != "api_route": + return [] path = "/" - method = "GET" - operation_id = func_node.name + 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 [(method, path) for method in methods] + + @beartype + def _extract_routes_from_function(self, func_node: ast.FunctionDef) -> list[RouteInfo]: + """Extract route information from a function with FastAPI decorators.""" + matched_routes: list[tuple[str, str]] = [] - # 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 - - normalized_path, path_params = self._extract_path_parameters(path) - - return RouteInfo( - path=normalized_path, - method=method, - operation_id=operation_id, - function=func_node.name, - path_params=path_params, - ) + if not isinstance(decorator, ast.Call): + continue + got = self._path_method_from_route_call(decorator) + if got is not None: + matched_routes.append(got) + continue + matched_routes.extend(self._path_methods_from_api_route_call(decorator)) + + if not matched_routes: + return [] + + results: list[RouteInfo] = [] + for method, path in matched_routes: + normalized_path, path_params = self._extract_path_parameters(path) + results.append( + RouteInfo( + path=normalized_path, + method=method, + operation_id=func_node.name, + function=func_node.name, + path_params=path_params, + ) + ) + return results @beartype def _extract_string_literal(self, node: ast.AST) -> str | None: @@ -193,7 +241,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 c0462315..582dfd56 100644 --- a/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py +++ b/packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py @@ -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): @@ -179,6 +185,7 @@ def _extract_route_from_function( """Extract route information from a function with Flask decorators.""" path = None methods = ["GET"] # Default method + _ = (imports, py_file) # Check decorators for route information for decorator in func_node.decorator_list: @@ -187,6 +194,7 @@ def _extract_route_from_function( isinstance(decorator, ast.Call) and isinstance(decorator.func, ast.Attribute) and decorator.func.attr == "route" + and self._is_owned_route_decorator(decorator.func, app_names, bp_names) ): # Extract path from first argument if decorator.args: @@ -220,6 +228,12 @@ def _extract_route_from_function( return results + @beartype + def _is_owned_route_decorator(self, func: ast.Attribute, app_names: set[str], bp_names: set[str]) -> bool: + if isinstance(func.value, ast.Name): + return func.value.id in app_names or func.value.id in bp_names + return False + @beartype def _extract_string_literal(self, node: ast.AST) -> str | None: """Extract string literal from AST node (Python 3.8+ uses ast.Constant).""" diff --git a/pyproject.toml b/pyproject.toml index 259a54bd..8078226d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,6 +57,7 @@ validate-cli-contracts = "python tools/validate_cli_contracts.py" check-bundle-imports = "python scripts/check-bundle-imports.py" sign-modules = "python scripts/sign-modules.py {args}" verify-modules-signature = "python scripts/verify-modules-signature.py {args}" +sync-registry-from-package = "python scripts/sync_registry_from_package.py {args}" link-dev-module = "python scripts/link_dev_module.py {args}" smart-test = "python tools/smart_test_coverage.py run {args}" smart-test-status = "python tools/smart_test_coverage.py status" diff --git a/registry/index.json b/registry/index.json index 935d33a0..f9461119 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.9", + "download_url": "modules/specfact-codebase-0.41.9.tar.gz", + "checksum_sha256": "5aeec7735644108ae1861a7f1913d38761ae37612d76bfa145131e37869704c9", "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.9", + "download_url": "modules/specfact-code-review-0.47.9.tar.gz", + "checksum_sha256": "8471e41b3567021834229c970365f537e1ac1ebe3c174c64a5a21304105eacd9", "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 00000000..7f59324c 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 00000000..369986c4 --- /dev/null +++ b/registry/modules/specfact-code-review-0.47.0.tar.gz.sha256 @@ -0,0 +1 @@ +7bda277c0c8fb137750ee6b88090e0df929e6e699bf5c1c048d18679890bb347 diff --git a/registry/modules/specfact-code-review-0.47.1.tar.gz b/registry/modules/specfact-code-review-0.47.1.tar.gz new file mode 100644 index 00000000..0bcc8301 Binary files /dev/null and b/registry/modules/specfact-code-review-0.47.1.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.47.1.tar.gz.sha256 b/registry/modules/specfact-code-review-0.47.1.tar.gz.sha256 new file mode 100644 index 00000000..586ecaf5 --- /dev/null +++ b/registry/modules/specfact-code-review-0.47.1.tar.gz.sha256 @@ -0,0 +1 @@ +d97d23466bb00952df18e2835e27b687a2325c1a8196dd75e9738b00e79910b9 diff --git a/registry/modules/specfact-code-review-0.47.2.tar.gz b/registry/modules/specfact-code-review-0.47.2.tar.gz new file mode 100644 index 00000000..574aba90 Binary files /dev/null and b/registry/modules/specfact-code-review-0.47.2.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.47.2.tar.gz.sha256 b/registry/modules/specfact-code-review-0.47.2.tar.gz.sha256 new file mode 100644 index 00000000..1d6096fe --- /dev/null +++ b/registry/modules/specfact-code-review-0.47.2.tar.gz.sha256 @@ -0,0 +1 @@ +e672610e15369f9a546aa28e5e19ab6eb4f5a347c1255d7604ae25cce65b899b diff --git a/registry/modules/specfact-code-review-0.47.6.tar.gz b/registry/modules/specfact-code-review-0.47.6.tar.gz new file mode 100644 index 00000000..7e79db48 Binary files /dev/null and b/registry/modules/specfact-code-review-0.47.6.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.47.6.tar.gz.sha256 b/registry/modules/specfact-code-review-0.47.6.tar.gz.sha256 new file mode 100644 index 00000000..24449fc1 --- /dev/null +++ b/registry/modules/specfact-code-review-0.47.6.tar.gz.sha256 @@ -0,0 +1 @@ +b8b39ecf993f04f266a431871e35171696c8d184cb5e5a41b3edd02bff246e1a diff --git a/registry/modules/specfact-code-review-0.47.7.tar.gz b/registry/modules/specfact-code-review-0.47.7.tar.gz new file mode 100644 index 00000000..c2103d0d Binary files /dev/null and b/registry/modules/specfact-code-review-0.47.7.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.47.7.tar.gz.sha256 b/registry/modules/specfact-code-review-0.47.7.tar.gz.sha256 new file mode 100644 index 00000000..80b8a37c --- /dev/null +++ b/registry/modules/specfact-code-review-0.47.7.tar.gz.sha256 @@ -0,0 +1 @@ +22ca04a00e6079daac6850c7ee33ce2b79c3caae57960028347b891271ae646f diff --git a/registry/modules/specfact-code-review-0.47.8.tar.gz b/registry/modules/specfact-code-review-0.47.8.tar.gz new file mode 100644 index 00000000..cb378498 Binary files /dev/null and b/registry/modules/specfact-code-review-0.47.8.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.47.8.tar.gz.sha256 b/registry/modules/specfact-code-review-0.47.8.tar.gz.sha256 new file mode 100644 index 00000000..f26ae027 --- /dev/null +++ b/registry/modules/specfact-code-review-0.47.8.tar.gz.sha256 @@ -0,0 +1 @@ +a07a21bda71392ed9c39fc3bd5541686cf5ca569c8d9a0102d8de656839b471d diff --git a/registry/modules/specfact-code-review-0.47.9.tar.gz b/registry/modules/specfact-code-review-0.47.9.tar.gz new file mode 100644 index 00000000..832327ae Binary files /dev/null and b/registry/modules/specfact-code-review-0.47.9.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.47.9.tar.gz.sha256 b/registry/modules/specfact-code-review-0.47.9.tar.gz.sha256 new file mode 100644 index 00000000..5dd64900 --- /dev/null +++ b/registry/modules/specfact-code-review-0.47.9.tar.gz.sha256 @@ -0,0 +1 @@ +8471e41b3567021834229c970365f537e1ac1ebe3c174c64a5a21304105eacd9 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 00000000..c705667c 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 00000000..227d9a9c --- /dev/null +++ b/registry/modules/specfact-codebase-0.41.5.tar.gz.sha256 @@ -0,0 +1 @@ +fe8f95c325f21eb80209aa067f6a4f2055f1f5feed4e818a1c9d3061320c2270 diff --git a/registry/modules/specfact-codebase-0.41.6.tar.gz b/registry/modules/specfact-codebase-0.41.6.tar.gz new file mode 100644 index 00000000..3a2bf102 Binary files /dev/null and b/registry/modules/specfact-codebase-0.41.6.tar.gz differ diff --git a/registry/modules/specfact-codebase-0.41.6.tar.gz.sha256 b/registry/modules/specfact-codebase-0.41.6.tar.gz.sha256 new file mode 100644 index 00000000..a70a6346 --- /dev/null +++ b/registry/modules/specfact-codebase-0.41.6.tar.gz.sha256 @@ -0,0 +1 @@ +39e620d5a6b8fe12b283c4edb68f959398981207f00060609350d93fb37f3bb2 diff --git a/registry/modules/specfact-codebase-0.41.7.tar.gz b/registry/modules/specfact-codebase-0.41.7.tar.gz new file mode 100644 index 00000000..058e1bb4 Binary files /dev/null and b/registry/modules/specfact-codebase-0.41.7.tar.gz differ diff --git a/registry/modules/specfact-codebase-0.41.7.tar.gz.sha256 b/registry/modules/specfact-codebase-0.41.7.tar.gz.sha256 new file mode 100644 index 00000000..11e9cf30 --- /dev/null +++ b/registry/modules/specfact-codebase-0.41.7.tar.gz.sha256 @@ -0,0 +1 @@ +a22d75ac1211e736cbd2ab775f7512a61407583a5e5c74ce7d51c8ecc855fc9b diff --git a/registry/modules/specfact-codebase-0.41.8.tar.gz b/registry/modules/specfact-codebase-0.41.8.tar.gz new file mode 100644 index 00000000..5fa0e966 Binary files /dev/null and b/registry/modules/specfact-codebase-0.41.8.tar.gz differ diff --git a/registry/modules/specfact-codebase-0.41.8.tar.gz.sha256 b/registry/modules/specfact-codebase-0.41.8.tar.gz.sha256 new file mode 100644 index 00000000..510231a7 --- /dev/null +++ b/registry/modules/specfact-codebase-0.41.8.tar.gz.sha256 @@ -0,0 +1 @@ +14f3a799d79c1d919755f258ce99a9ed1a0415488e9e9790821b080295a9d555 diff --git a/registry/modules/specfact-codebase-0.41.9.tar.gz b/registry/modules/specfact-codebase-0.41.9.tar.gz new file mode 100644 index 00000000..f05255b2 Binary files /dev/null and b/registry/modules/specfact-codebase-0.41.9.tar.gz differ diff --git a/registry/modules/specfact-codebase-0.41.9.tar.gz.sha256 b/registry/modules/specfact-codebase-0.41.9.tar.gz.sha256 new file mode 100644 index 00000000..2aad059e --- /dev/null +++ b/registry/modules/specfact-codebase-0.41.9.tar.gz.sha256 @@ -0,0 +1 @@ +5aeec7735644108ae1861a7f1913d38761ae37612d76bfa145131e37869704c9 diff --git a/registry/signatures/specfact-code-review-0.47.2.tar.sig b/registry/signatures/specfact-code-review-0.47.2.tar.sig new file mode 100644 index 00000000..6b71bd4c --- /dev/null +++ b/registry/signatures/specfact-code-review-0.47.2.tar.sig @@ -0,0 +1 @@ +uYll3YRRrGsNOHniavYpDE8Guq/bcwAT2vQ0GtmreHPi4lAHzdqv7ui8tlRh7uS/EFiqgIkBatULph2qXHoPBA== diff --git a/registry/signatures/specfact-code-review-0.47.8.tar.sig b/registry/signatures/specfact-code-review-0.47.8.tar.sig new file mode 100644 index 00000000..105912c3 --- /dev/null +++ b/registry/signatures/specfact-code-review-0.47.8.tar.sig @@ -0,0 +1 @@ +j3DH2rFTYo1zLkVOVe6gy+fcoYtRVF+pMvQqL3aipWgOg/ao0/aHmOIQw6w2FRtSTsIYyx3hgkajw0GcKppNCA== diff --git a/registry/signatures/specfact-code-review-0.47.9.tar.sig b/registry/signatures/specfact-code-review-0.47.9.tar.sig new file mode 100644 index 00000000..09b25ac6 --- /dev/null +++ b/registry/signatures/specfact-code-review-0.47.9.tar.sig @@ -0,0 +1 @@ +PcP7OeNMkZqTtRpRBkpr2v2od0RX7fM4VCKXyWAYyP7bBRsKyqyRJI5oxX/0BEIWPt29o6y4t93WyJisWgWKBQ== diff --git a/registry/signatures/specfact-codebase-0.41.7.tar.sig b/registry/signatures/specfact-codebase-0.41.7.tar.sig new file mode 100644 index 00000000..a5b6859b --- /dev/null +++ b/registry/signatures/specfact-codebase-0.41.7.tar.sig @@ -0,0 +1 @@ +UPhlWxq4ikGSCWx2rfPLSEd/b5eGbqaJxUA7fHAQ4fsCmdeW4HZuPZ4hFgRQmBhg+Pk8WeFuMp9g/JdCGXJJDg== diff --git a/registry/signatures/specfact-codebase-0.41.9.tar.sig b/registry/signatures/specfact-codebase-0.41.9.tar.sig new file mode 100644 index 00000000..9723abdb --- /dev/null +++ b/registry/signatures/specfact-codebase-0.41.9.tar.sig @@ -0,0 +1 @@ +xl1MEWdJFcA9PKmvEA0/cJV+X0Wv4lHIDxXUEsj+iSbfQ5TGs4c4TYd8/OWq6WjQDzq4vAC9/0m11PdpuNgZDQ== diff --git a/resources/schemas/cli-contract.schema.json b/resources/schemas/cli-contract.schema.json index fd523e74..d10a2625 100644 --- a/resources/schemas/cli-contract.schema.json +++ b/resources/schemas/cli-contract.schema.json @@ -55,6 +55,13 @@ "items": { "type": "string" } + }, + "file_content_contains": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Each string must appear in the UTF-8 report file selected via --out (integration tests enforce this; schema-only validation accepts the field)." } } } diff --git a/scripts/git-branch-module-signature-flag.sh b/scripts/git-branch-module-signature-flag.sh new file mode 100755 index 00000000..99bcdf1e --- /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 2fcd3b97..b75c0a7a 100755 --- a/scripts/pre-commit-verify-modules-signature.sh +++ b/scripts/pre-commit-verify-modules-signature.sh @@ -1,24 +1,127 @@ #!/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 the same baseline verifier as PRs targeting `dev` (full payload checksum + version bump; +# cryptographic signature is enforced only in the `require` branch below). Contributors refresh +# checksums with `scripts/sign-modules.py +# --allow-unsigned --payload-from-filesystem` when they lack a release signing key. +# +# On the `omit` policy, if verify fails, the hook runs `sign-modules.py --changed-only` against +# `HEAD` (staged + unstaged changes) with `--bump-version patch --allow-unsigned`, re-stages only +# manifests that script reports updating, then re-runs verify so commits self-heal formal drift. +# Registry rows and published tarballs are intentionally left to CI (`publish-modules`); do not +# rewrite registry/index.json or registry/modules from pre-commit. 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 + +_stage_manifests_from_sign_output() { + # sign-modules prints lines like "packages//module-package.yaml: checksum" or ": version a -> b" + local line mf + while IFS= read -r line || [[ -n "${line}" ]]; do + [[ "${line}" == *:* ]] || continue + mf="${line%%:*}" + [[ "${mf}" == packages/*/module-package.yaml ]] || continue + [[ -f "${mf}" ]] || continue + git add -- "${mf}" + done +} + +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 (formal: payload checksum + version bump; signatures not required on this branch — see docs/reference/module-security.md)" >&2 + set +e + _verify_out="$("${_base[@]}" 2>&1)" + _verify_rc=$? + set -e + if ((_verify_rc == 0)); then + exit 0 + fi + printf '%s\n' "${_verify_out}" >&2 + + _failed_manifests=() + while IFS= read -r mf; do + [[ -n "${mf}" ]] && _failed_manifests+=("${mf}") + done < <(printf '%s\n' "${_verify_out}" | grep '^FAIL packages/' | sed -n 's/^FAIL \(packages\/[^[:space:]]*\/module-package\.yaml\):.*/\1/p' | sort -u) + + echo "⚠️ Module verify failed; auto-remediating checksums and patch bumps for changed modules..." >&2 + _sign_log="$(mktemp "${TMPDIR:-/tmp}/specfact-sign-modules.XXXXXX")" + trap 'rm -f "${_sign_log}"' EXIT + if ! hatch run ./scripts/sign-modules.py \ + --changed-only \ + --base-ref HEAD \ + --bump-version patch \ + --allow-unsigned \ + --payload-from-filesystem >"${_sign_log}" 2>&1 + then + cat "${_sign_log}" >&2 + echo "❌ sign-modules auto-remediation failed." >&2 + exit 1 + fi + if [[ -s "${_sign_log}" ]]; then + cat "${_sign_log}" >&2 + fi + + _stage_manifests_from_sign_output <"${_sign_log}" + echo "🔐 Re-verifying after auto-remediation..." >&2 + set +e + _verify2_out="$("${_base[@]}" 2>&1)" + _verify2_rc=$? + set -e + if ((_verify2_rc != 0)) && ((${#_failed_manifests[@]} > 0)); then + # Covers committed manifest drift (no diff vs HEAD) or partial first-pass fixes. + printf '%s\n' "${_verify2_out}" >&2 + echo "⚠️ Retrying sign for failing manifests (compare base HEAD~1)..." >&2 + if ! hatch run ./scripts/sign-modules.py \ + --changed-only \ + --base-ref HEAD~1 \ + --bump-version patch \ + --allow-unsigned \ + --payload-from-filesystem \ + "${_failed_manifests[@]}" >>"${_sign_log}" 2>&1 + then + cat "${_sign_log}" >&2 + echo "❌ sign-modules fallback remediation failed." >&2 + exit 1 + fi + _stage_manifests_from_sign_output <"${_sign_log}" + echo "🔐 Re-verifying after fallback remediation..." >&2 + if ! "${_base[@]}"; then + echo "❌ Module verify still failing after remediation (manual fix or signing key may be required)." >&2 + exit 1 + fi + echo "✅ Module manifests updated and staged; continuing the commit." >&2 + exit 0 + fi + if ((_verify2_rc != 0)); then + printf '%s\n' "${_verify2_out}" >&2 + echo "❌ Module verify still failing after remediation (manual fix or signing key may be required)." >&2 + exit 1 + fi + echo "✅ Module manifests updated and staged; continuing the commit." >&2 + exit 0 + ;; + *) + 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 62569677..50a0879d 100755 --- a/scripts/pre_commit_code_review.py +++ b/scripts/pre_commit_code_review.py @@ -15,6 +15,7 @@ import importlib import importlib.util import json +import os import subprocess import sys from collections.abc import Callable, Sequence @@ -53,6 +54,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 +86,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 @@ -138,6 +143,17 @@ def _run_review_subprocess( files: Sequence[str], ) -> subprocess.CompletedProcess[str] | None: """Run the nested SpecFact review command and handle timeout reporting.""" + env = os.environ.copy() + # Ensure nested `python -m specfact_cli.cli` bootstraps this checkout's bundle sources first + # (see `specfact_cli/__init__.py::_bootstrap_bundle_paths`) so ~/.specfact/modules tarballs do not + # shadow in-repo `specfact_code_review` during the pre-commit gate. + env["SPECFACT_MODULES_REPO"] = str(repo_root.resolve()) + env["SPECFACT_CLI_MODULES_REPO"] = str(repo_root.resolve()) + code_review_src = repo_root / "packages" / "specfact-code-review" / "src" + if code_review_src.is_dir(): + prefix = str(code_review_src) + previous = env.get("PYTHONPATH", "").strip() + env["PYTHONPATH"] = f"{prefix}{os.pathsep}{previous}" if previous else prefix try: return subprocess.run( cmd, @@ -145,6 +161,7 @@ def _run_review_subprocess( text=True, capture_output=True, cwd=str(repo_root), + env=env, timeout=300, ) except TimeoutExpired: @@ -204,29 +221,40 @@ 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, int | None]: + """Parse ``REVIEW_JSON_OUT``, print counts, return ``(ok, error_count, ci_exit_code)``. + + Callers should use ``ci_exit_code`` as the hook exit code; ``error_count`` is informational only + because fixable error-severity findings may still yield a passing ``ci_exit_code``. + """ report_path = _report_path(repo_root) if not report_path.is_file(): sys.stderr.write(f"Code review: no report file at {REVIEW_JSON_OUT} (could not print findings summary).\n") - return False + return False, None, None try: data = json.loads(report_path.read_text(encoding="utf-8")) except (OSError, UnicodeDecodeError) as exc: sys.stderr.write(f"Code review: could not read {REVIEW_JSON_OUT}: {exc}\n") - return False + return False, None, None except json.JSONDecodeError as exc: sys.stderr.write(f"Code review: invalid JSON in {REVIEW_JSON_OUT}: {exc}\n") - return False + return False, None, None + + if not isinstance(data, dict): + sys.stderr.write(f"Code review: expected top-level JSON object in {REVIEW_JSON_OUT}.\n") + return False, None, 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, None counts = count_findings_by_severity(findings_raw) total = len(findings_raw) verdict = data.get("overall_verdict", "?") + ci_exit_code = data.get("ci_exit_code") + if ci_exit_code not in {0, 1}: + ci_exit_code = 1 if verdict == "FAIL" else 0 parts = [ f"errors={counts['error']}", f"warnings={counts['warning']}", @@ -246,7 +274,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"], ci_exit_code @ensure(lambda result: isinstance(result, tuple) and len(result) == 2) @@ -288,7 +316,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,9 +335,10 @@ def main(argv: Sequence[str] | None = None) -> int: return _missing_report_exit_code(report_path, result) # Do not echo nested `specfact code review run` stdout/stderr (verbose tool banners); full report # is in REVIEW_JSON_OUT; we print a short summary on stderr below. - if not _print_review_findings_summary(repo_root): + summary_ok, _error_count, ci_exit_code = _print_review_findings_summary(repo_root) + if not summary_ok or ci_exit_code is None: return 1 - return result.returncode + return int(ci_exit_code) if __name__ == "__main__": diff --git a/scripts/sync_registry_from_package.py b/scripts/sync_registry_from_package.py new file mode 100644 index 00000000..2dbd6dcf --- /dev/null +++ b/scripts/sync_registry_from_package.py @@ -0,0 +1,231 @@ +#!/usr/bin/env python3 +"""Align registry/index.json and registry/modules artifacts with packages/*/module-package.yaml. + +**Not** a substitute for CI: ``publish-modules`` is the canonical path that signs, selects bundles, +and opens registry PRs. Use this script only for deliberate local tooling or recovery — never from +pre-commit — or you risk skipping or confusing the real publish flow on ``dev``/``main``. +""" + +from __future__ import annotations + +import argparse +import gzip +import hashlib +import io +import json +import sys +import tarfile +from collections.abc import Callable +from functools import wraps +from pathlib import Path +from typing import TYPE_CHECKING, Any, TypeVar, cast + +import yaml + + +_FuncT = TypeVar("_FuncT", bound=Callable[..., Any]) + +if TYPE_CHECKING: + from beartype import beartype + from icontract import ensure, require +else: + try: + from beartype import beartype + except ImportError: # pragma: no cover - exercised in plain-python CI/runtime + + def beartype(func: _FuncT) -> _FuncT: + return func + + try: + from icontract import ensure, require + except ImportError: # pragma: no cover - exercised in plain-python CI/runtime + + def require( + _condition: Callable[..., bool], + _description: str | None = None, + ) -> Callable[[_FuncT], _FuncT]: + def decorator(func: _FuncT) -> _FuncT: + @wraps(func) + def wrapper(*args: Any, **kwargs: Any) -> Any: + return func(*args, **kwargs) + + return cast(_FuncT, wrapper) + + return decorator + + def ensure( + _condition: Callable[..., bool], + _description: str | None = None, + ) -> Callable[[_FuncT], _FuncT]: + return require(_condition, _description) + + +_IGNORED_DIR_NAMES = {".git", "tests", "__pycache__", ".pytest_cache", ".mypy_cache", ".ruff_cache", "logs"} +_IGNORED_SUFFIXES = {".pyc", ".pyo"} + + +def _emit_line(message: str, *, error: bool = False) -> None: + stream = sys.stderr if error else sys.stdout + stream.write(f"{message}\n") + + +def _bundle_dir(repo_root: Path, bundle: str) -> Path: + name = bundle.strip() + if not name.startswith("specfact-"): + name = f"specfact-{name}" + path = repo_root / "packages" / name + if not path.is_dir(): + msg = f"Bundle directory not found: {path}" + raise FileNotFoundError(msg) + return path + + +_TAR_DETERMINISTIC_MTIME = 0 + + +def _write_bundle_tarball(bundle_dir: Path, dest: Path) -> None: + """Write ``.tar.gz`` with stable member metadata and gzip header so digest/registry rows match across runs.""" + dest.parent.mkdir(parents=True, exist_ok=True) + bundle_name = bundle_dir.name + gz_buffer = io.BytesIO() + with ( + gzip.GzipFile(fileobj=gz_buffer, mode="wb", mtime=_TAR_DETERMINISTIC_MTIME) as gz_stream, + tarfile.open(fileobj=gz_stream, mode="w", format=tarfile.GNU_FORMAT) as tar, + ): + for path in sorted(bundle_dir.rglob("*")): + if not path.is_file(): + continue + rel = path.relative_to(bundle_dir) + if any(part in _IGNORED_DIR_NAMES for part in rel.parts): + continue + if path.suffix.lower() in _IGNORED_SUFFIXES: + continue + arcname = f"{bundle_name}/{rel.as_posix()}" + payload = path.read_bytes() + info = tarfile.TarInfo(arcname) + info.size = len(payload) + info.mtime = _TAR_DETERMINISTIC_MTIME + info.mode = 0o644 + info.uid = 0 + info.gid = 0 + info.uname = "root" + info.gname = "root" + info.type = tarfile.REGTYPE + tar.addfile(info, io.BytesIO(payload)) + dest.write_bytes(gz_buffer.getvalue()) + + +def _load_module_manifest(bundle_dir: Path) -> tuple[dict[str, object], str, str]: + manifest_path = bundle_dir / "module-package.yaml" + data = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + if not isinstance(data, dict): + msg = f"Invalid manifest: {manifest_path}" + raise ValueError(msg) + bundle_name = bundle_dir.name + module_id = str(data.get("name") or f"nold-ai/{bundle_name}").strip() + version = str(data.get("version") or "").strip() + if not module_id or not version: + msg = f"Manifest missing name or version: {manifest_path}" + raise ValueError(msg) + return data, module_id, version + + +def _load_registry_index(repo_root: Path) -> tuple[Path, dict[str, object]]: + registry_path = repo_root / "registry" / "index.json" + reg = json.loads(registry_path.read_text(encoding="utf-8")) + if not isinstance(reg, dict): + msg = "registry/index.json root must be an object" + raise ValueError(msg) + return registry_path, reg + + +def _prepare_registry_modules_dir(repo_root: Path) -> Path: + modules_dir = repo_root / "registry" / "modules" + modules_dir.mkdir(parents=True, exist_ok=True) + return modules_dir + + +def _build_registry_tarball_and_digest( + bundle_dir: Path, modules_dir: Path, bundle_name: str, version: str +) -> tuple[str, str]: + artifact_name = f"{bundle_name}-{version}.tar.gz" + artifact_path = modules_dir / artifact_name + _write_bundle_tarball(bundle_dir, artifact_path) + digest = hashlib.sha256(artifact_path.read_bytes()).hexdigest() + (artifact_path.with_suffix(artifact_path.suffix + ".sha256")).write_text(f"{digest}\n", encoding="utf-8") + return artifact_name, digest + + +def _upsert_registry_module_row( + registry: dict[str, object], + *, + module_id: str, + manifest: dict[str, object], + release: dict[str, str], +) -> None: + version = release["version"] + digest = release["digest"] + artifact_name = release["artifact"] + modules = registry.get("modules") + if not isinstance(modules, list): + msg = "registry index missing modules list" + raise ValueError(msg) + download_url = f"modules/{artifact_name}" + entry: dict[str, object] | None = next( + (item for item in modules if isinstance(item, dict) and str(item.get("id") or "").strip() == module_id), + None, + ) + if entry is None: + entry = {"id": module_id} + modules.append(entry) + entry["latest_version"] = version + entry["download_url"] = download_url + entry["checksum_sha256"] = digest + for key in ("tier", "publisher", "bundle_dependencies", "description", "core_compatibility"): + if key in manifest: + entry[key] = manifest[key] + + +@beartype +@require(lambda repo_root: repo_root.is_dir(), "repo_root must be an existing directory") +@require(lambda bundle: bool(bundle.strip()), "bundle must be non-empty") +def _sync_one_bundle(repo_root: Path, bundle: str) -> None: + bundle_dir = _bundle_dir(repo_root, bundle) + manifest, module_id, version = _load_module_manifest(bundle_dir) + registry_path, registry = _load_registry_index(repo_root) + modules_dir = _prepare_registry_modules_dir(repo_root) + bundle_name = bundle_dir.name + artifact_name, digest = _build_registry_tarball_and_digest(bundle_dir, modules_dir, bundle_name, version) + _upsert_registry_module_row( + registry, + module_id=module_id, + manifest=manifest, + release={"version": version, "digest": digest, "artifact": artifact_name}, + ) + registry_path.write_text(json.dumps(registry, indent=2) + "\n", encoding="utf-8") + _emit_line(f"synced registry for {module_id} v{version} ({artifact_name})") + + +@beartype +@ensure(lambda result: result in {0, 1}, "main must return a process exit code") +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--repo-root", default=".", type=Path, help="Repository root") + parser.add_argument( + "--bundle", + action="append", + dest="bundles", + default=[], + help="Bundle directory name under packages/ (repeatable), e.g. specfact-code-review", + ) + args = parser.parse_args() + repo_root = args.repo_root.resolve() + if not args.bundles: + parser.error("at least one --bundle is required") + for bundle in args.bundles: + _sync_one_bundle(repo_root, bundle) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) 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 8a75a728..565bc20c 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 7be31def..43e63148 100644 --- a/tests/cli-contracts/specfact-code-review-run.scenarios.yaml +++ b/tests/cli-contracts/specfact-code-review-run.scenarios.yaml @@ -55,3 +55,116 @@ 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: bug-hunt-shadow-dirty-exit-zero + type: pattern + argv: + - --json + - --bug-hunt + - --mode + - shadow + - tests/fixtures/review/dirty_module.py + expect: + exit_code: 0 + stdout_contains: + - review-report.json + - name: mode-enforce-dirty-exit-nonzero + type: pattern + argv: + - --json + - --mode + - enforce + - tests/fixtures/review/dirty_module.py + expect: + exit_code: 1 + stdout_contains: + - review-report.json + - name: bug-hunt-enforce-dirty-exit-nonzero + type: pattern + argv: + - --json + - --bug-hunt + - --mode + - enforce + - tests/fixtures/review/dirty_module.py + expect: + exit_code: 1 + stdout_contains: + - review-report.json + - name: focus-source-and-docs-union + type: pattern + argv: + - --scope + - full + - --mode + - shadow + - --path + - packages/specfact-code-review + - --path + - tests/unit/docs + - --json + - --out + - CONTRACT_TMP_REPORT.json + - --focus + - source + - --focus + - docs + expect: + exit_code: 0 + file_content_contains: + - packages/specfact-code-review/src/specfact_code_review + - tests/unit/docs + - name: focus-tests-narrows-to-test-tree + type: pattern + argv: + - --scope + - full + - --mode + - shadow + - --path + - tests/unit/specfact_code_review + - --json + - --out + - CONTRACT_TMP_REPORT.json + - --bug-hunt + - --focus + - tests + expect: + exit_code: 0 + file_content_contains: + - tests/unit/specfact_code_review + - name: level-error-json-clean-module + type: pattern + argv: + - --json + - --out + - CONTRACT_TMP_REPORT.json + - --bug-hunt + - --level + - error + - tests/fixtures/review/dirty_module.py + expect: + exit_code: 1 + file_content_contains: + - '"severity":"error"' + - 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/integration/specfact_code_review/test_cli_contract_review_run_reports.py b/tests/integration/specfact_code_review/test_cli_contract_review_run_reports.py new file mode 100644 index 00000000..22359bc4 --- /dev/null +++ b/tests/integration/specfact_code_review/test_cli_contract_review_run_reports.py @@ -0,0 +1,78 @@ +"""Execute CLI contract scenarios that assert JSON report file contents.""" + +from __future__ import annotations + +import functools +import shutil +from pathlib import Path + +import pytest +import yaml +from typer.testing import CliRunner + +from specfact_code_review.review.commands import app + + +def _repo_root() -> Path: + here = Path(__file__).resolve() + for parent in (here, *here.parents): + if (parent / "pyproject.toml").is_file() and (parent / "registry" / "index.json").is_file(): + return parent + raise RuntimeError("cannot locate repository root from test file path") + + +REPO_ROOT = _repo_root() +SCENARIO_PATH = REPO_ROOT / "tests" / "cli-contracts" / "specfact-code-review-run.scenarios.yaml" +REQUIRED_TOOLS = ("ruff", "radon", "basedpyright", "pylint", "semgrep") +REPORT_PLACEHOLDER = "CONTRACT_TMP_REPORT.json" + +runner = CliRunner() + + +@functools.cache +def _load_scenarios() -> dict: + return yaml.safe_load(SCENARIO_PATH.read_text(encoding="utf-8")) + + +def _skip_if_tools_missing() -> None: + missing = [tool for tool in REQUIRED_TOOLS if shutil.which(tool) is None] + if missing: + pytest.skip(f"Missing required review tools: {', '.join(missing)}") + + +def _scenario_names_with_file_expectations() -> list[str]: + data = _load_scenarios() + names: list[str] = [] + for scenario in data.get("scenarios", []): + expect = scenario.get("expect") or {} + if expect.get("file_content_contains"): + names.append(scenario["name"]) + return names + + +@pytest.mark.integration +@pytest.mark.parametrize("scenario_name", _scenario_names_with_file_expectations()) +def test_cli_contract_review_run_json_report_file( + tmp_path: Path, scenario_name: str, monkeypatch: pytest.MonkeyPatch +) -> None: + _skip_if_tools_missing() + monkeypatch.chdir(REPO_ROOT) + data = _load_scenarios() + scenario = next(s for s in data["scenarios"] if s["name"] == scenario_name) + expect = scenario["expect"] + fragments: list[str] = expect["file_content_contains"] + + out_path = tmp_path / f"{scenario_name}.json" + argv: list[str] = [] + for arg in scenario["argv"]: + argv.append(str(out_path) if arg == REPORT_PLACEHOLDER else arg) + + assert REPORT_PLACEHOLDER in scenario["argv"], "expected CONTRACT_TMP_REPORT.json placeholder in argv" + + result = runner.invoke(app, ["review", "run", *argv]) + + assert result.exit_code == expect["exit_code"], result.output + assert out_path.is_file(), f"expected JSON report at {out_path}" + report_text = out_path.read_text(encoding="utf-8") + for fragment in fragments: + assert fragment in report_text, f"missing {fragment!r} in report for {scenario_name!r}" diff --git a/tests/unit/scripts/test_pre_commit_code_review.py b/tests/unit/scripts/test_pre_commit_code_review.py index dc433625..dabb11d7 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,40 @@ 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", + "ci_exit_code": 0, + "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 +160,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="") @@ -140,6 +189,26 @@ def _fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[s assert "@workspace Open `.specfact/code-review.json`" in err +def test_main_uses_report_ci_exit_code_for_fixable_errors(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + module = _load_script_module() + repo_root = tmp_path + payload = { + "overall_verdict": "PASS_WITH_ADVISORY", + "ci_exit_code": 0, + "findings": [{"severity": "error", "rule": "fixable", "fixable": True}], + } + + def _fake_run(cmd: list[str], **_kwargs: object) -> subprocess.CompletedProcess[str]: + _write_sample_review_report(repo_root, payload) + return subprocess.CompletedProcess(cmd, 1, stdout=".specfact/code-review.json\n", stderr="") + + monkeypatch.setattr(module, "_repo_root", lambda: repo_root) + monkeypatch.setattr(module, "ensure_runtime_available", lambda: (True, None)) + monkeypatch.setattr(module.subprocess, "run", _fake_run) + + assert module.main(["tests/unit/test_app.py"]) == 0 + + def _write_sample_review_report(repo_root: Path, payload: dict[str, object]) -> None: spec_dir = repo_root / ".specfact" spec_dir.mkdir(parents=True, exist_ok=True) @@ -198,9 +267,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 00000000..ff43db2f --- /dev/null +++ b/tests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.py @@ -0,0 +1,11 @@ +"""Fixture parsed by contract_runner tests: ``icontract`` is imported but public API has no contracts.""" + +import icontract + + +# Bind so static analyzers treat the import as used; runners only ``ast.parse`` this file. +__fixture_icontract_ref = icontract + + +def public_without_contracts(value: int) -> int: + return value + 1 diff --git a/tests/unit/specfact_code_review/review/test_commands.py b/tests/unit/specfact_code_review/review/test_commands.py index 24e4affd..63ef964c 100644 --- a/tests/unit/specfact_code_review/review/test_commands.py +++ b/tests/unit/specfact_code_review/review/test_commands.py @@ -44,6 +44,24 @@ def _fake_run_command(_files: list[Path], **kwargs: object) -> tuple[int, str | assert recorded["kwargs"]["include_tests"] is False +def test_review_run_focus_source_sets_include_tests_false(monkeypatch: Any) -> None: + recorded: dict[str, Any] = {} + + def _fake_run_command(_files: list[Path], **kwargs: object) -> tuple[int, str | None]: + recorded["kwargs"] = kwargs + return 0, None + + monkeypatch.setattr("specfact_code_review.review.commands.run_command", _fake_run_command) + + result = runner.invoke( + app, + ["review", "run", "--focus", "source", "tests/fixtures/review/clean_module.py"], + ) + + assert result.exit_code == 0 + assert recorded["kwargs"]["include_tests"] is False + + def test_review_run_explicit_files_do_not_prompt_and_keep_tests(monkeypatch: Any) -> None: recorded: dict[str, Any] = {} 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 00000000..69f96adb --- /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 2bb18fb8..c776939c 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 d886eb49..ba2ca82a 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,17 @@ 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_py_and_pyi_suffixes(tmp_path: Path) -> None: + py_file = tmp_path / "a.py" + pyi_file = tmp_path / "b.pyi" + yaml_file = tmp_path / "module-package.yaml" + py_file.write_text("x = 1\n", encoding="utf-8") + pyi_file.write_text("def f() -> None: ...\n", encoding="utf-8") + yaml_file.write_text("name: t\n", encoding="utf-8") + + assert python_source_paths_for_tools([py_file, pyi_file, yaml_file]) == [py_file, pyi_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 00000000..abca5192 --- /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 52ac4e66..22821079 100644 --- a/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py +++ b/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py @@ -15,6 +15,17 @@ def test_run_basedpyright_returns_empty_for_no_files() -> None: assert 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 run_basedpyright([manifest]) == [] + + run_mock.assert_not_called() + + def test_run_basedpyright_maps_error_diagnostic_to_type_safety(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" payload = { 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 cccf1fd4..3cc55a4d 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,21 +15,93 @@ FIXTURES_DIR = Path(__file__).resolve().parent.parent / "fixtures" / "contract_runner" -def test_run_contract_check_reports_public_function_without_contracts(monkeypatch: MonkeyPatch) -> None: - file_path = FIXTURES_DIR / "public_without_contracts.py" +@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( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + file_path = tmp_path / "public_without_contracts.py" + file_path.write_text("def public_without_contracts(value: int) -> int:\n return value + 1\n", encoding="utf-8") run_mock = Mock(return_value=completed_process("crosshair", stdout="")) monkeypatch.setattr(subprocess, "run", run_mock) 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)]) +def test_run_contract_check_uses_batch_level_icontract_detection(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + """Icontract usage in a sibling module under the same bundle roots ``MISSING_ICONTRACT`` on the edited file.""" + (tmp_path / ".git").mkdir() + pkg = tmp_path / "packages" / "demo_pkg" + pkg.mkdir(parents=True) + (pkg / "uses_icontract.py").write_text("import icontract\n", encoding="utf-8") + tmp_file = pkg / "public_without_contracts.py" + tmp_file.write_text( + "def public_without_contracts(value: int) -> int:\n return value + 1\n", + encoding="utf-8", + ) + monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) + + findings = run_contract_check([tmp_file]) + + assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} + + +def test_run_contract_check_detects_icontract_across_package_bundles(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + """MISSING_ICONTRACT must not depend only on the edited bundle: scan ``packages/`` when needed.""" + (tmp_path / ".git").mkdir() + pkg_a = tmp_path / "packages" / "pkg_a" + pkg_b = tmp_path / "packages" / "pkg_b" + pkg_a.mkdir(parents=True) + pkg_b.mkdir(parents=True) + (pkg_b / "icontract_anchor.py").write_text("import icontract\n", encoding="utf-8") + edited = pkg_a / "new_public_api.py" + edited.write_text( + "def public_no_contracts(value: int) -> int:\n return value + 1\n", + encoding="utf-8", + ) + monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) + + findings = run_contract_check([edited]) + + assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} + + +def test_run_contract_check_detects_icontract_anchor_in_stub_across_package_bundles( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """Cross-bundle icontract discovery must observe ``import icontract`` in a ``.pyi`` stub.""" + (tmp_path / ".git").mkdir() + pkg_a = tmp_path / "packages" / "pkg_a" + pkg_b = tmp_path / "packages" / "pkg_b" + pkg_a.mkdir(parents=True) + pkg_b.mkdir(parents=True) + (pkg_b / "icontract_anchor.pyi").write_text("import icontract\n", encoding="utf-8") + edited = pkg_a / "new_public_api.py" + edited.write_text( + "def public_no_contracts(value: int) -> int:\n return value + 1\n", + encoding="utf-8", + ) + monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) + + findings = run_contract_check([edited]) + + assert "MISSING_ICONTRACT" in {finding.rule for finding in findings} + + def test_run_contract_check_skips_decorated_public_function(monkeypatch: MonkeyPatch) -> None: file_path = FIXTURES_DIR / "public_with_contracts.py" monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("crosshair", stdout=""))) @@ -88,7 +162,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 6a70133d..e3c32e56 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 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" @@ -96,3 +107,46 @@ def test_run_radon_uses_dedicated_tool_identifier_for_kiss_findings(tmp_path: Pa kiss_findings = [finding for finding in findings if finding.rule.startswith("kiss.")] assert kiss_findings assert {finding.tool for finding in kiss_findings} == {"radon-kiss"} + + +def test_run_radon_requires_typer_decorator_for_context_parameter_exemption( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + file_path = tmp_path / "commands.py" + file_path.write_text( + """ +def callback(ctx: typer.Context, a: str, b: str, c: str, d: str, e: str) -> None: + return None +""", + encoding="utf-8", + ) + monkeypatch.setattr( + subprocess, + "run", + Mock(return_value=completed_process("radon", stdout=json.dumps({str(file_path): []}))), + ) + + findings = run_radon([file_path]) + + assert "kiss.parameter-count.warning" in {finding.rule for finding in findings} + + +def test_run_radon_exempts_typer_command_context_parameters(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "commands.py" + file_path.write_text( + """ +@app.command("run") +def callback(ctx: typer.Context, a: str, b: str, c: str, d: str, e: str) -> None: + return None +""", + encoding="utf-8", + ) + monkeypatch.setattr( + subprocess, + "run", + Mock(return_value=completed_process("radon", stdout=json.dumps({str(file_path): []}))), + ) + + findings = run_radon([file_path]) + + assert "kiss.parameter-count.warning" not in {finding.rule for finding in findings} diff --git a/tests/unit/specfact_code_review/tools/test_semgrep_runner.py b/tests/unit/specfact_code_review/tools/test_semgrep_runner.py index 93156c20..b2f12078 100644 --- a/tests/unit/specfact_code_review/tools/test_semgrep_runner.py +++ b/tests/unit/specfact_code_review/tools/test_semgrep_runner.py @@ -9,7 +9,12 @@ import pytest from pytest import MonkeyPatch -from specfact_code_review.tools.semgrep_runner import _snip_stderr_tail, find_semgrep_config, run_semgrep +from specfact_code_review.tools.semgrep_runner import ( + _snip_stderr_tail, + find_semgrep_config, + run_semgrep, + run_semgrep_bugs, +) from tests.unit.specfact_code_review.tools.helpers import completed_process @@ -30,6 +35,18 @@ ] +@pytest.fixture(autouse=True) +def _stub_semgrep_on_path(monkeypatch: MonkeyPatch) -> None: # pyright: ignore[reportUnusedFunction] + real_which = shutil.which + + def _which(name: str) -> str | None: + if name == "semgrep": + return "/fake/semgrep" + return real_which(name) + + monkeypatch.setattr("specfact_code_review.tools.tool_availability.shutil.which", _which) + + def test_run_semgrep_maps_findings_to_review_finding(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" payload = { @@ -115,7 +132,8 @@ def test_run_semgrep_filters_findings_to_requested_files(tmp_path: Path, monkeyp def test_run_semgrep_returns_tool_error_when_semgrep_is_unavailable(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" - run_mock = Mock(side_effect=FileNotFoundError("semgrep not found")) + run_mock = Mock() + monkeypatch.setattr("specfact_code_review.tools.tool_availability.shutil.which", lambda _name: None) monkeypatch.setattr(subprocess, "run", run_mock) findings = run_semgrep([file_path]) @@ -123,7 +141,8 @@ def test_run_semgrep_returns_tool_error_when_semgrep_is_unavailable(tmp_path: Pa assert len(findings) == 1 assert findings[0].category == "tool_error" assert findings[0].tool == "semgrep" - assert findings[0].severity == "error" + assert findings[0].severity == "warning" + run_mock.assert_not_called() def test_run_semgrep_returns_empty_list_for_clean_file(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: @@ -206,6 +225,18 @@ def test_find_semgrep_config_stops_at_git_directory(tmp_path: Path) -> None: assert find_semgrep_config(module_file=fake_here) == repo / "nested" / ".semgrep" / "clean_code.yaml" +def test_run_semgrep_bugs_returns_empty_when_semgrep_cli_missing(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + """Bug pass must not emit a second skip finding; run_semgrep already reports missing semgrep.""" + bundle = tmp_path / "bundle" + (bundle / ".semgrep").mkdir(parents=True) + (bundle / ".semgrep" / "bugs.yaml").write_text("rules: []\n", encoding="utf-8") + target = tmp_path / "x.py" + target.write_text("x = 1\n", encoding="utf-8") + monkeypatch.setattr("specfact_code_review.tools.tool_availability.shutil.which", lambda _name: None) + + assert run_semgrep_bugs([target], bundle_root=bundle) == [] + + def test_run_semgrep_retries_after_transient_parse_failure(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" payload = { 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 00000000..d047410b --- /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 00000000..474b5b72 --- /dev/null +++ b/tests/unit/specfact_codebase/test_sidecar_framework_extractors.py @@ -0,0 +1,129 @@ +"""Tests for sidecar framework extractors (path exclusions).""" + +from __future__ import annotations + +from pathlib import Path + +from specfact_codebase.validators.sidecar.frameworks.fastapi import FastAPIExtractor +from specfact_codebase.validators.sidecar.frameworks.flask import FlaskExtractor + + +def _fake_fastapi_main() -> str: + 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) + methods = {route.method for route in routes if route.path == "/multi"} + assert methods == {"GET", "POST"} + + +def test_fastapi_extractor_preserves_multiple_route_decorators(tmp_path: Path) -> None: + (tmp_path / "routes.py").write_text( + """ +from fastapi import APIRouter +router = APIRouter() + +@router.get("/read") +@router.post("/write") +def multi(): + return {"ok": True} +""", + encoding="utf-8", + ) + extractor = FastAPIExtractor() + routes = extractor.extract_routes(tmp_path) + assert {(route.method, route.path) for route in routes} == {("GET", "/read"), ("POST", "/write")} + + +def test_fastapi_extractor_ignores_non_http_decorators(tmp_path: Path) -> None: + """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 + + +def test_flask_extractor_uses_registered_app_and_blueprint_symbols(tmp_path: Path) -> None: + (tmp_path / "app.py").write_text( + """ +from flask import Blueprint, Flask +app = Flask(__name__) +bp = Blueprint("api", __name__) + +@app.route("/app") +def app_route(): + return "ok" + +@bp.route("/bp", methods=["POST"]) +def bp_route(): + return "ok" + +@other.route("/ignored") +def unrelated(): + return "no" +""", + encoding="utf-8", + ) + + routes = FlaskExtractor().extract_routes(tmp_path) + + assert {(route.method, route.path) for route in routes} == {("GET", "/app"), ("POST", "/bp")} diff --git a/tests/unit/test_git_branch_module_signature_flag_script.py b/tests/unit/test_git_branch_module_signature_flag_script.py new file mode 100644 index 00000000..ea950bb8 --- /dev/null +++ b/tests/unit/test_git_branch_module_signature_flag_script.py @@ -0,0 +1,52 @@ +from __future__ import annotations + +import os +import subprocess +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 + + +def test_git_branch_module_signature_flag_script_requires_for_main_base() -> None: + env = {**os.environ, "GITHUB_BASE_REF": "main"} + result = subprocess.run([SCRIPT_PATH], capture_output=True, text=True, check=False, env=env) + + assert result.returncode == 0 + assert result.stdout.strip() == "require" + + +def test_git_branch_module_signature_flag_script_omits_when_base_ref_unset(tmp_path: Path) -> None: + # Without GITHUB_BASE_REF the script falls back to the current git branch; use an isolated + # repo on a non-main branch so the outcome is "omit" regardless of the outer worktree branch. + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init"], cwd=repo, check=True, capture_output=True) + subprocess.run(["git", "config", "user.email", "omit-test@example.com"], cwd=repo, check=True) + subprocess.run(["git", "config", "user.name", "omit-test"], cwd=repo, check=True) + (repo / "tracked").write_text("x", encoding="utf-8") + subprocess.run(["git", "add", "tracked"], cwd=repo, check=True) + subprocess.run(["git", "commit", "-m", "init"], cwd=repo, check=True) + subprocess.run(["git", "checkout", "-b", "side"], cwd=repo, check=True) + env = {k: v for k, v in os.environ.items() if k != "GITHUB_BASE_REF"} + result = subprocess.run([SCRIPT_PATH], cwd=repo, capture_output=True, text=True, check=False, env=env) + + assert result.returncode == 0 + assert result.stdout.strip() == "omit" + + +def test_git_branch_module_signature_flag_script_omits_for_non_main_base() -> None: + env = {**os.environ, "GITHUB_BASE_REF": "feature/x"} + result = subprocess.run([SCRIPT_PATH], capture_output=True, text=True, check=False, env=env) + + assert result.returncode == 0 + assert result.stdout.strip() == "omit" diff --git a/tests/unit/test_pre_commit_verify_modules_signature_script.py b/tests/unit/test_pre_commit_verify_modules_signature_script.py index a2a31d7c..d2186961 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,42 @@ 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 _pre_commit_verify_script_text() -> str: + return (REPO_ROOT / "scripts/pre-commit-verify-modules-signature.sh").read_text(encoding="utf-8") + + +def test_pre_commit_verify_modules_signature_script_has_expected_entrypoints() -> None: + text = _pre_commit_verify_script_text() + 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 - marker = 'if [[ "$_require_signature" == true ]]; then' - assert marker in text + +def test_pre_commit_verify_modules_signature_script_require_branch_uses_strict_verify() -> None: + text = _pre_commit_verify_script_text() + marker = 'case "${sig_policy}" in' _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 + + +def test_pre_commit_verify_modules_signature_script_omit_branch_remediation_shape() -> None: + text = _pre_commit_verify_script_text() + marker = 'case "${sig_policy}" in' + _tail = text.split(marker, 1)[1] + omit_block = _tail.split("omit)", 1)[1].split("*)", 1)[0] + assert "--require-signature" not in omit_block + assert "--metadata-only" not in omit_block + assert '"${_base[@]}"' in omit_block + assert "sign-modules.py" in omit_block + assert "--changed-only" in omit_block + assert "--bump-version patch" in omit_block + assert "--allow-unsigned" in omit_block + assert "_stage_manifests_from_sign_output" in omit_block + assert "HEAD~1" in omit_block + assert "_failed_manifests" in omit_block diff --git a/tests/unit/test_sync_registry_from_package_script.py b/tests/unit/test_sync_registry_from_package_script.py new file mode 100644 index 00000000..fa14f78b --- /dev/null +++ b/tests/unit/test_sync_registry_from_package_script.py @@ -0,0 +1,152 @@ +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + +import yaml + + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = REPO_ROOT / "scripts" / "sync_registry_from_package.py" + + +def _minimal_registry(module_id: str, version: str, checksum: str, download_url: str) -> dict: + return { + "modules": [ + { + "id": module_id, + "latest_version": version, + "download_url": download_url, + "checksum_sha256": checksum, + "tier": "official", + "publisher": {"name": "nold-ai", "email": "hello@noldai.com"}, + "description": "test", + } + ] + } + + +def test_sync_registry_tarball_bytes_match_for_identical_trees(tmp_path: Path) -> None: + """Tar/gzip layers use fixed metadata so two runs produce identical artifact bytes.""" + + def _write_minimal_repo(root: Path) -> str: + (root / "registry" / "modules").mkdir(parents=True) + (root / "registry" / "signatures").mkdir(parents=True) + bundle = "specfact-syncregdet" + bdir = root / "packages" / bundle + bdir.mkdir(parents=True) + old_ver = "0.1.0" + old_name = f"{bundle}-{old_ver}.tar.gz" + (root / "registry" / "modules" / old_name).write_bytes(b"old") + (root / "registry" / "modules" / f"{old_name}.sha256").write_text( + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n", encoding="utf-8" + ) + manifest = { + "name": "nold-ai/specfact-syncregdet", + "version": "0.1.1", + "tier": "official", + "publisher": {"name": "nold-ai", "email": "hello@noldai.com"}, + "description": "test bundle", + "bundle_group_command": "syncregdet", + "integrity": {"checksum": "sha256:deadbeef", "signature": "dummy"}, + } + (bdir / "module-package.yaml").write_text(yaml.safe_dump(manifest, sort_keys=False), encoding="utf-8") + (bdir / "README.md").write_text("hello", encoding="utf-8") + reg_path = root / "registry" / "index.json" + reg_path.write_text( + json.dumps( + _minimal_registry( + "nold-ai/specfact-syncregdet", + old_ver, + "a" * 64, + f"modules/{old_name}", + ), + indent=2, + ) + + "\n", + encoding="utf-8", + ) + return bundle + + root_a = tmp_path / "a" + root_b = tmp_path / "b" + bundle = _write_minimal_repo(root_a) + _write_minimal_repo(root_b) + cmd_a = [sys.executable, str(SCRIPT), "--repo-root", str(root_a), "--bundle", bundle] + cmd_b = [sys.executable, str(SCRIPT), "--repo-root", str(root_b), "--bundle", bundle] + subprocess.run(cmd_a, check=True, cwd=str(REPO_ROOT)) + subprocess.run(cmd_b, check=True, cwd=str(REPO_ROOT)) + art_a = root_a / "registry" / "modules" / f"{bundle}-0.1.1.tar.gz" + art_b = root_b / "registry" / "modules" / f"{bundle}-0.1.1.tar.gz" + assert art_a.read_bytes() == art_b.read_bytes() + + +def test_sync_registry_from_package_updates_index_and_artifacts(tmp_path: Path) -> None: + root = tmp_path / "repo" + (root / "registry" / "modules").mkdir(parents=True) + (root / "registry" / "signatures").mkdir(parents=True) + bundle = "specfact-syncregtest" + bdir = root / "packages" / bundle + bdir.mkdir(parents=True) + old_ver = "0.1.0" + old_name = f"{bundle}-{old_ver}.tar.gz" + (root / "registry" / "modules" / old_name).write_bytes(b"old") + (root / "registry" / "modules" / f"{old_name}.sha256").write_text( + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n", encoding="utf-8" + ) + manifest = { + "name": "nold-ai/specfact-syncregtest", + "version": "0.1.1", + "tier": "official", + "publisher": {"name": "nold-ai", "email": "hello@noldai.com"}, + "description": "test bundle", + "bundle_group_command": "syncregtest", + "integrity": {"checksum": "sha256:deadbeef"}, + } + (bdir / "module-package.yaml").write_text(yaml.safe_dump(manifest, sort_keys=False), encoding="utf-8") + (bdir / "README.md").write_text("hello", encoding="utf-8") + + reg_path = root / "registry" / "index.json" + reg_path.write_text( + json.dumps( + _minimal_registry( + "nold-ai/specfact-syncregtest", + old_ver, + "a" * 64, + f"modules/{old_name}", + ), + indent=2, + ) + + "\n", + encoding="utf-8", + ) + + subprocess.run( + [sys.executable, str(SCRIPT), "--repo-root", str(root), "--bundle", bundle], + check=True, + cwd=str(REPO_ROOT), + ) + + data = json.loads(reg_path.read_text(encoding="utf-8")) + mod = next(m for m in data["modules"] if m["id"] == "nold-ai/specfact-syncregtest") + assert mod["latest_version"] == "0.1.1" + assert mod["download_url"] == f"modules/{bundle}-0.1.1.tar.gz" + + art = root / "registry" / "modules" / f"{bundle}-0.1.1.tar.gz" + assert art.is_file() + side = art.with_suffix(art.suffix + ".sha256") + assert side.is_file() + assert mod["checksum_sha256"] == side.read_text(encoding="utf-8").strip().split()[0] + + +def test_sync_registry_from_package_cli_requires_bundle() -> None: + proc = subprocess.run( + [sys.executable, str(SCRIPT), "--repo-root", str(REPO_ROOT)], + capture_output=True, + text=True, + cwd=str(REPO_ROOT), + check=False, + ) + assert proc.returncode != 0 diff --git a/tests/unit/test_validate_repo_manifests_bundle_deps.py b/tests/unit/test_validate_repo_manifests_bundle_deps.py index 4eb22cec..b488e69a 100644 --- a/tests/unit/test_validate_repo_manifests_bundle_deps.py +++ b/tests/unit/test_validate_repo_manifests_bundle_deps.py @@ -45,6 +45,237 @@ def test_validate_manifest_bundle_dependency_refs_flags_dangling_id(tmp_path: Pa assert str(manifest) in errors[0] +def test_validate_registry_consistency_allows_manifest_version_ahead_of_registry(tmp_path: Path) -> None: + """Between publish runs the package manifest may bump while index.json still lists the last publish.""" + v = _load_validate_repo_module() + modules_dir = tmp_path / "registry" / "modules" + modules_dir.mkdir(parents=True) + tarball_name = "specfact-project-0.41.3.tar.gz" + digest = "a3df973c103e0708bef7a6ad23ead9b45e3354ba2ecb878f4d64e753e163a817" + (modules_dir / f"{tarball_name}.sha256").write_text(f"{digest}\n", encoding="utf-8") + registry_path = tmp_path / "index.json" + registry_path.write_text( + json.dumps( + { + "modules": [ + { + "id": "nold-ai/specfact-project", + "latest_version": "0.41.3", + "download_url": f"modules/{tarball_name}", + "checksum_sha256": digest, + } + ] + } + ), + encoding="utf-8", + ) + pkg = tmp_path / "packages" / "specfact-project" + pkg.mkdir(parents=True) + (pkg / "module-package.yaml").write_text( + "name: nold-ai/specfact-project\n" + "version: 0.41.4\n" + "tier: official\n" + "publisher:\n name: nold-ai\n email: hello@noldai.com\n" + "description: d\n" + "bundle_group_command: x\n", + encoding="utf-8", + ) + errors = v.validate_registry_consistency(tmp_path, registry_path) + assert errors == [] + + +def test_validate_registry_consistency_flags_manifest_behind_registry(tmp_path: Path) -> None: + v = _load_validate_repo_module() + modules_dir = tmp_path / "registry" / "modules" + modules_dir.mkdir(parents=True) + tarball_name = "specfact-project-0.41.3.tar.gz" + digest = "a3df973c103e0708bef7a6ad23ead9b45e3354ba2ecb878f4d64e753e163a817" + (modules_dir / f"{tarball_name}.sha256").write_text(f"{digest}\n", encoding="utf-8") + registry_path = tmp_path / "index.json" + registry_path.write_text( + json.dumps( + { + "modules": [ + { + "id": "nold-ai/specfact-project", + "latest_version": "0.41.3", + "download_url": f"modules/{tarball_name}", + "checksum_sha256": digest, + } + ] + } + ), + encoding="utf-8", + ) + pkg = tmp_path / "packages" / "specfact-project" + pkg.mkdir(parents=True) + (pkg / "module-package.yaml").write_text( + "name: nold-ai/specfact-project\n" + "version: 0.41.2\n" + "tier: official\n" + "publisher:\n name: nold-ai\n email: hello@noldai.com\n" + "description: d\n" + "bundle_group_command: x\n", + encoding="utf-8", + ) + errors = v.validate_registry_consistency(tmp_path, registry_path) + assert len(errors) == 1 + assert "behind" in errors[0] + + +def test_validate_registry_consistency_missing_sidecar(tmp_path: Path) -> None: + v = _load_validate_repo_module() + modules_dir = tmp_path / "registry" / "modules" + modules_dir.mkdir(parents=True) + registry_path = tmp_path / "index.json" + tarball_name = "specfact-project-0.41.3.tar.gz" + registry_path.write_text( + json.dumps( + { + "modules": [ + { + "id": "nold-ai/specfact-project", + "latest_version": "0.41.3", + "download_url": f"modules/{tarball_name}", + "checksum_sha256": "a3df973c103e0708bef7a6ad23ead9b45e3354ba2ecb878f4d64e753e163a817", + } + ] + } + ), + encoding="utf-8", + ) + pkg = tmp_path / "packages" / "specfact-project" + pkg.mkdir(parents=True) + (pkg / "module-package.yaml").write_text( + "name: nold-ai/specfact-project\n" + "version: 0.41.3\n" + "tier: official\n" + "publisher:\n name: nold-ai\n email: hello@noldai.com\n" + "description: d\n" + "bundle_group_command: x\n", + encoding="utf-8", + ) + errors = v.validate_registry_consistency(tmp_path, registry_path) + assert len(errors) == 1 + assert "missing checksum sidecar" in errors[0] + + +def test_validate_registry_consistency_manifest_mismatch_with_registry(tmp_path: Path) -> None: + v = _load_validate_repo_module() + modules_dir = tmp_path / "registry" / "modules" + modules_dir.mkdir(parents=True) + tarball_name = "specfact-project-0.41.3.tar.gz" + digest = "a3df973c103e0708bef7a6ad23ead9b45e3354ba2ecb878f4d64e753e163a817" + (modules_dir / f"{tarball_name}.sha256").write_text(f"{digest}\n", encoding="utf-8") + registry_path = tmp_path / "index.json" + registry_path.write_text( + json.dumps( + { + "modules": [ + { + "id": "nold-ai/specfact-project", + "latest_version": "0.41.3", + "download_url": f"modules/{tarball_name}", + "checksum_sha256": digest, + } + ] + } + ), + encoding="utf-8", + ) + pkg = tmp_path / "packages" / "specfact-project" + pkg.mkdir(parents=True) + (pkg / "module-package.yaml").write_text( + "name: nold-ai/specfact-wrong-id\n" + "version: 0.41.3\n" + "tier: official\n" + "publisher:\n name: nold-ai\n email: hello@noldai.com\n" + "description: d\n" + "bundle_group_command: x\n", + encoding="utf-8", + ) + errors = v.validate_registry_consistency(tmp_path, registry_path) + assert len(errors) == 1 + assert "does not match registry" in errors[0] + + +def test_validate_registry_consistency_empty_sidecar_returns_structured_error(tmp_path: Path) -> None: + v = _load_validate_repo_module() + modules_dir = tmp_path / "registry" / "modules" + modules_dir.mkdir(parents=True) + tarball_name = "specfact-project-0.41.3.tar.gz" + (modules_dir / f"{tarball_name}.sha256").write_text("", encoding="utf-8") + digest = "a3df973c103e0708bef7a6ad23ead9b45e3354ba2ecb878f4d64e753e163a817" + registry_path = tmp_path / "index.json" + registry_path.write_text( + json.dumps( + { + "modules": [ + { + "id": "nold-ai/specfact-project", + "latest_version": "0.41.3", + "download_url": f"modules/{tarball_name}", + "checksum_sha256": digest, + } + ] + } + ), + encoding="utf-8", + ) + pkg = tmp_path / "packages" / "specfact-project" + pkg.mkdir(parents=True) + (pkg / "module-package.yaml").write_text( + "name: nold-ai/specfact-project\n" + "version: 0.41.3\n" + "tier: official\n" + "publisher:\n name: nold-ai\n email: hello@noldai.com\n" + "description: d\n" + "bundle_group_command: x\n", + encoding="utf-8", + ) + errors = v.validate_registry_consistency(tmp_path, registry_path) + assert len(errors) == 1 + assert "cannot read sidecar" in errors[0] + + +def test_validate_registry_consistency_flags_bad_checksum(tmp_path: Path) -> None: + v = _load_validate_repo_module() + modules_dir = tmp_path / "registry" / "modules" + modules_dir.mkdir(parents=True) + registry_path = tmp_path / "index.json" + tarball_name = "specfact-project-0.41.3.tar.gz" + (modules_dir / f"{tarball_name}.sha256").write_text("deadbeef\n", encoding="utf-8") + registry_path.write_text( + json.dumps( + { + "modules": [ + { + "id": "nold-ai/specfact-project", + "latest_version": "0.41.3", + "download_url": f"modules/{tarball_name}", + "checksum_sha256": "a3df973c103e0708bef7a6ad23ead9b45e3354ba2ecb878f4d64e753e163a817", + } + ] + } + ), + encoding="utf-8", + ) + pkg = tmp_path / "packages" / "specfact-project" + pkg.mkdir(parents=True) + (pkg / "module-package.yaml").write_text( + "name: nold-ai/specfact-project\n" + "version: 0.41.3\n" + "tier: official\n" + "publisher:\n name: nold-ai\n email: hello@noldai.com\n" + "description: d\n" + "bundle_group_command: x\n", + encoding="utf-8", + ) + errors = v.validate_registry_consistency(tmp_path, registry_path) + assert len(errors) == 1 + assert "does not match sidecar" in errors[0] + + def test_validate_manifest_bundle_dependency_refs_ok_when_all_present(tmp_path: Path) -> None: v = _load_validate_repo_module() registry_path = tmp_path / "index.json" diff --git a/tests/unit/test_verify_modules_signature_script.py b/tests/unit/test_verify_modules_signature_script.py index 683b042c..eb25349d 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 fd810937..305f77db 100644 --- a/tests/unit/workflows/test_pr_orchestrator_signing.py +++ b/tests/unit/workflows/test_pr_orchestrator_signing.py @@ -6,21 +6,38 @@ 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_pr_to_dev_verifier_omits_loose_integrity_mode() -> None: + workflow = _workflow_text() + assert "--metadata-only" not in workflow + + +def test_pr_orchestrator_verify_require_signature_on_main_paths() -> None: + workflow = _workflow_text() + main_pr_guard = 'if [ "$TARGET_BRANCH" = "main" ]; then' + main_ref_guard = '[ "${{ github.ref_name }}" = "main" ]; then' require_append = "VERIFY_CMD+=(--require-signature)" - assert branch_guard in workflow + assert main_pr_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_hardening.py b/tests/unit/workflows/test_sign_modules_hardening.py new file mode 100644 index 00000000..da2128f6 --- /dev/null +++ b/tests/unit/workflows/test_sign_modules_hardening.py @@ -0,0 +1,152 @@ +from __future__ import annotations + +from pathlib import Path +from typing import Any, cast + +import pytest +import yaml +from pytest import FixtureRequest + + +REPO_ROOT = Path(__file__).resolve().parents[3] + + +def _workflow_text() -> str: + path = REPO_ROOT / ".github" / "workflows" / "sign-modules.yml" + return path.read_text(encoding="utf-8") + + +def _parsed_workflow() -> dict[Any, Any]: + loaded = yaml.safe_load(_workflow_text()) + assert isinstance(loaded, dict) + return cast(dict[Any, Any], loaded) + + +def _strict_push_verify_step_block(workflow: str) -> str: + marker = "- name: Strict verify module manifests (push to dev/main)\n" + idx = workflow.find(marker) + if idx < 0: + msg = "strict push verify step not found in sign-modules workflow" + raise AssertionError(msg) + lines = workflow[idx:].splitlines(keepends=True) + block: list[str] = [lines[0]] + for line in lines[1:]: + if line.startswith(" - name:"): + break + block.append(line) + return "".join(block) + + +def _workflow_on_section(doc: dict[Any, Any]) -> dict[str, Any]: + section = doc.get(True) + if isinstance(section, dict): + return cast(dict[str, Any], section) + raw = doc.get("on") + assert isinstance(raw, dict) + return cast(dict[str, Any], raw) + + +def test_sign_modules_hardening_triggers_on_push_pr_and_dispatch() -> None: + doc = _parsed_workflow() + on = _workflow_on_section(doc) + push = on["push"] + assert isinstance(push, dict) + assert push["branches"] == ["dev", "main"] + paths = push["paths"] + assert isinstance(paths, list) + assert "packages/**" in paths + assert "registry/**" in paths + + pr = on["pull_request"] + assert isinstance(pr, dict) + assert pr["branches"] == ["dev", "main"] + pr_paths = pr["paths"] + assert isinstance(pr_paths, list) + assert "registry/**" in pr_paths + + dispatch = on["workflow_dispatch"] + assert isinstance(dispatch, dict) + assert "base_branch" in dispatch["inputs"] + + +def test_sign_modules_hardening_verify_job_exports_public_signing_secrets() -> None: + doc = _parsed_workflow() + verify = doc["jobs"]["verify"] + assert isinstance(verify, dict) + env = verify.get("env") + assert isinstance(env, dict) + assert env["SPECFACT_MODULE_PUBLIC_SIGN_KEY"] == "${{ secrets.SPECFACT_MODULE_PUBLIC_SIGN_KEY }}" + assert env["SPECFACT_MODULE_SIGNING_PUBLIC_KEY_PEM"] == "${{ secrets.SPECFACT_MODULE_SIGNING_PUBLIC_KEY_PEM }}" + + +def test_sign_modules_hardening_verify_job_can_open_sign_prs() -> None: + doc = _parsed_workflow() + verify = doc["jobs"]["verify"] + assert isinstance(verify, dict) + perms = verify.get("permissions") + assert isinstance(perms, dict) + assert perms.get("pull-requests") == "write" + assert verify.get("outputs", {}).get("opened_sign_pr") == "${{ steps.commit_auto_sign.outputs.opened_sign_pr }}" + + +def test_sign_modules_hardening_auto_signs_on_push_non_bot() -> None: + workflow = _workflow_text() + assert "github.event_name == 'push'" in workflow + assert "github.actor != 'github-actions[bot]'" in workflow + assert "scripts/sign-modules.py" in workflow + assert "--changed-only" in workflow + assert "--bump-version patch" in workflow + assert 'git commit -m "chore(modules): auto-sign module manifests"' in workflow + assert "auto-sign module manifests [skip ci]" not in workflow + # Protected dev/main: push signing commit to a side branch, then open a PR (not HEAD -> dev). + assert "gh pr create" in workflow + assert 'git push origin "HEAD:refs/heads/${SIGN_BRANCH}"' in workflow + + +@pytest.mark.parametrize( + "needles", + ( + pytest.param( + ( + "--require-signature", + "github.event_name == 'push'", + "github.ref_name == 'dev' || github.ref_name == 'main'", + ), + id="push_strict_verify", + ), + pytest.param( + ("github.event_name != 'push'", "pull_request", "scripts/verify-modules-signature.py"), + id="pr_non_push_verify", + ), + ), +) +def test_sign_modules_hardening_workflow_contains_verify_snippets( + needles: tuple[str, ...], request: FixtureRequest +) -> None: + workflow = _workflow_text() + if request.node.callspec.id == "push_strict_verify": + block = _strict_push_verify_step_block(workflow) + for needle in needles: + assert needle in block + else: + for needle in needles: + assert needle in workflow + + +def test_sign_modules_hardening_reproducibility_on_main() -> None: + doc = _parsed_workflow() + repro = doc["jobs"]["reproducibility"] + assert isinstance(repro, dict) + assert repro["if"] == ( + "github.event_name == 'push' && github.ref_name == 'main' && needs.verify.outputs.opened_sign_pr != 'true'" + ) + needs = repro["needs"] + assert needs == ["verify"] + + +def test_sign_modules_hardening_manual_dispatch_job() -> None: + doc = _parsed_workflow() + manual = doc["jobs"]["sign-and-push"] + assert isinstance(manual, dict) + assert manual["if"] == "github.event_name == 'workflow_dispatch'" + assert manual["needs"] == ["verify"] diff --git a/tests/unit/workflows/test_sign_modules_on_approval.py b/tests/unit/workflows/test_sign_modules_on_approval.py index c4d0cf49..7adc180a 100644 --- a/tests/unit/workflows/test_sign_modules_on_approval.py +++ b/tests/unit/workflows/test_sign_modules_on_approval.py @@ -52,14 +52,33 @@ 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) + for needle in ( + "github.event.review.state", + "github.event.review.user.author_association", + "approved", + "COLLABORATOR|MEMBER|OWNER", + 'echo "sign=false"', + 'echo "sign=true"', + "github.event.pull_request.base.ref", + "github.event.pull_request.head.repo.full_name", + "github.repository", + ): + assert needle in run, needle def _assert_concurrency_and_permissions(doc: dict[Any, Any]) -> None: @@ -76,7 +95,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,16 +136,21 @@ 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 "git merge-base HEAD" in workflow - assert 'git fetch origin "${PR_BASE_REF}"' in workflow - assert "--no-tags" in workflow - assert "scripts/sign-modules.py" in workflow - assert "--changed-only" in workflow - assert "--base-ref" in workflow - assert '"$MERGE_BASE"' in workflow - assert "--bump-version patch" in workflow - assert "--payload-from-filesystem" in workflow + for needle in ( + "merge-base", + "git merge-base HEAD", + 'git fetch origin "${PR_BASE_REF}"', + "--no-tags", + "scripts/sign-modules.py", + "--changed-only", + "--base-ref", + '"$MERGE_BASE"', + "--bump-version patch", + "--payload-from-filesystem", + "steps.gate.outputs.sign == 'true'", + ): + assert needle in workflow, needle + assert '--base-ref "origin/' not in workflow def _assert_discover_step_writes_outputs(steps: list[Any]) -> None: @@ -141,7 +166,7 @@ def _assert_commit_and_push_step(steps: list[Any]) -> None: assert commit_step.get("id") == "commit" commit_run = commit_step["run"] assert isinstance(commit_run, str) - assert 'git commit -m "chore(modules): ci sign changed modules [skip ci]"' in commit_run + assert 'git commit -m "chore(modules): ci sign changed modules"' in commit_run assert 'git push origin "HEAD:${PR_HEAD_REF}"' in commit_run assert "Push to ${PR_HEAD_REF} failed" in commit_run @@ -151,8 +176,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 diff --git a/tools/validate_repo_manifests.py b/tools/validate_repo_manifests.py index c8737084..a4051b98 100755 --- a/tools/validate_repo_manifests.py +++ b/tools/validate_repo_manifests.py @@ -1,16 +1,69 @@ #!/usr/bin/env python3 -"""Validate bundle manifests and registry JSON in specfact-cli-modules.""" +"""Validate bundle manifests and registry JSON in specfact-cli-modules. + +Registry ``latest_version`` describes the last **published** row in git. A package +``module-package.yaml`` may use a **higher** semver (ahead of publish); that is allowed so local +and PR work does not rewrite ``registry/`` before ``publish-modules`` runs on ``dev``/``main``. +""" from __future__ import annotations import json import re +import sys +from collections.abc import Callable +from functools import wraps from pathlib import Path +from typing import TYPE_CHECKING, Any, TypeVar, cast import yaml +_FuncT = TypeVar("_FuncT", bound=Callable[..., Any]) + +if TYPE_CHECKING: + from beartype import beartype + from icontract import ensure, require +else: + try: + from beartype import beartype + except ImportError: # pragma: no cover - exercised in plain-python CI/runtime + + def beartype(func: _FuncT) -> _FuncT: + return func + + try: + from icontract import ensure, require + except ImportError: # pragma: no cover - exercised in plain-python CI/runtime + + def require( + _condition: Callable[..., bool], + _description: str | None = None, + ) -> Callable[[_FuncT], _FuncT]: + def decorator(func: _FuncT) -> _FuncT: + @wraps(func) + def wrapper(*args: Any, **kwargs: Any) -> Any: + return func(*args, **kwargs) + + return cast(_FuncT, wrapper) + + return decorator + + def ensure( + _condition: Callable[..., bool], + _description: str | None = None, + ) -> Callable[[_FuncT], _FuncT]: + return require(_condition, _description) + + ROOT = Path(__file__).resolve().parent.parent + + +def _emit_line(message: str, *, error: bool = False) -> None: + stream = sys.stderr if error else sys.stdout + stream.write(f"{message}\n") + + REQUIRED_KEYS = {"name", "version", "tier", "publisher", "description", "bundle_group_command"} @@ -37,6 +90,157 @@ def _validate_registry(path: Path) -> list[str]: return [] +def _sha256_from_sidecar(text: str) -> str: + lines = [ln.strip() for ln in text.strip().splitlines() if ln.strip()] + if not lines: + msg = "sidecar is empty or whitespace-only" + raise OSError(msg) + tokens = lines[0].split() + if not tokens: + msg = "sidecar first line has no checksum token" + raise OSError(msg) + return tokens[0].strip().lower() + + +def _parse_registry_module_fields(mod: dict) -> tuple[str, str, str, str] | list[str]: + module_id = str(mod.get("id") or "").strip() + latest_version = str(mod.get("latest_version") or "").strip() + checksum = str(mod.get("checksum_sha256") or "").strip().lower() + download_url = str(mod.get("download_url") or "").strip() + if not module_id or not latest_version or not checksum or not download_url: + return ["missing id, latest_version, checksum_sha256, or download_url"] + return (module_id, latest_version, checksum, download_url) + + +def _validate_registry_download_url(label: str, module_id: str, latest_version: str, download_url: str) -> list[str]: + if not download_url.startswith("modules/") or not download_url.endswith(".tar.gz"): + return [ + f"{label}: download_url {download_url!r} must look like modules/-.tar.gz", + ] + slug = module_id.rsplit("/", maxsplit=1)[-1] + expected_url = f"modules/{slug}-{latest_version}.tar.gz" + if download_url != expected_url: + return [f"{label}: download_url {download_url!r} must match expected pattern {expected_url!r}"] + return [] + + +def _validate_registry_sidecar(root: Path, label: str, download_url: str, checksum: str) -> list[str]: + sidecar = root / "registry" / f"{download_url}.sha256" + if not sidecar.is_file(): + return [f"{label}: missing checksum sidecar {sidecar}"] + try: + got = _sha256_from_sidecar(sidecar.read_text(encoding="utf-8")) + except OSError as exc: + return [f"{label}: cannot read sidecar {sidecar} ({exc})"] + if got != checksum: + return [f"{label}: checksum_sha256 {checksum!r} does not match sidecar {sidecar} ({got!r})"] + return [] + + +def _semver_triplet(version: str) -> tuple[int, int, int] | None: + parts = version.strip().split(".") + if len(parts) != 3 or any(not part.isdigit() for part in parts): + return None + return int(parts[0]), int(parts[1]), int(parts[2]) + + +def _manifest_registry_version_relation(manifest_version: str, registry_version: str) -> str: + """Return ``eq`` | ``gt`` | ``lt`` | ``unknown`` (non-comparable semver triplets).""" + mt = _semver_triplet(manifest_version) + rt = _semver_triplet(registry_version) + if mt is not None and rt is not None: + if mt < rt: + return "lt" + if mt > rt: + return "gt" + return "eq" + if manifest_version == registry_version: + return "eq" + return "unknown" + + +def _validate_registry_manifest_alignment( + root: Path, label: str, slug: str, module_id: str, latest_version: str +) -> list[str]: + errors: list[str] = [] + manifest_path = root / "packages" / slug / "module-package.yaml" + if not manifest_path.is_file(): + return [f"{label}: expected package manifest {manifest_path}"] + try: + raw = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, yaml.YAMLError) as exc: + return [f"{label}: cannot parse {manifest_path} ({exc})"] + if not isinstance(raw, dict): + return [f"{label}: {manifest_path} must parse to a mapping"] + + manifest_name = str(raw.get("name") or "").strip() + if manifest_name != module_id: + errors.append(f"{label}: {manifest_path} name {manifest_name!r} does not match registry id {module_id!r}") + + manifest_version = str(raw.get("version") or "").strip() + relation = _manifest_registry_version_relation(manifest_version, latest_version) + if relation == "lt": + errors.append( + f"{label}: {manifest_path} version {manifest_version!r} is behind " + f"registry latest_version {latest_version!r}" + ) + elif relation == "unknown": + errors.append( + f"{label}: {manifest_path} version {manifest_version!r} does not match " + f"registry latest_version {latest_version!r} (expected semver x.y.z for both or exact match)" + ) + # ``gt``: manifest is ahead of the published registry row — normal between publish runs; CI + # (`publish-modules`) updates registry + artifacts. Do not require local registry edits here. + + return errors + + +def _registry_module_consistency_errors(root: Path, label: str, mod: dict) -> list[str]: + """Return errors for one registry module dict, or an empty list when checks pass.""" + parsed = _parse_registry_module_fields(mod) + if isinstance(parsed, list): + return [f"{label}: {parsed[0]}"] + + module_id, latest_version, checksum, download_url = parsed + errors = _validate_registry_download_url(label, module_id, latest_version, download_url) + if errors: + return errors + + slug = module_id.rsplit("/", maxsplit=1)[-1] + errors = _validate_registry_sidecar(root, label, download_url, checksum) + if errors: + return errors + + return _validate_registry_manifest_alignment(root, label, slug, module_id, latest_version) + + +@beartype +@require(lambda root: root.is_dir(), "root must be a directory") +@require(lambda registry_path: registry_path.is_file(), "registry_path must be a file") +def validate_registry_consistency(root: Path, registry_path: Path) -> list[str]: + """Cross-check registry/index.json against tarball sidecars and package manifests.""" + errors: list[str] = [] + try: + data = json.loads(registry_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + return [f"{registry_path}: cannot load registry for consistency check ({exc})"] + + modules = data.get("modules") + if not isinstance(modules, list): + return errors + + for idx, mod in enumerate(modules): + if not isinstance(mod, dict): + errors.append(f"{registry_path}: modules[{idx}] must be an object") + continue + label = f"{registry_path}: module {str(mod.get('id') or '').strip()!r}" + errors.extend(_registry_module_consistency_errors(root, label, mod)) + + return errors + + +@beartype +@require(lambda registry_path: registry_path.is_file(), "registry_path must be a file") def registry_module_ids(registry_path: Path) -> set[str]: data = json.loads(registry_path.read_text(encoding="utf-8")) modules = data.get("modules") @@ -45,6 +249,9 @@ def registry_module_ids(registry_path: Path) -> set[str]: return {str(m["id"]).strip() for m in modules if isinstance(m, dict) and str(m.get("id") or "").strip()} +@beartype +@require(lambda manifest_path: manifest_path.is_file(), "manifest_path must be a file") +@require(lambda registry_ids: isinstance(registry_ids, set), "registry_ids must be a set") def validate_manifest_bundle_dependency_refs(manifest_path: Path, registry_ids: set[str]) -> list[str]: """Ensure each bundle_dependencies entry targets a module id present in registry/index.json.""" errors: list[str] = [] @@ -71,12 +278,16 @@ def validate_manifest_bundle_dependency_refs(manifest_path: Path, registry_ids: return errors +@beartype +@ensure(lambda result: result in {0, 1}, "main must return a process exit code") def main() -> int: manifest_paths = sorted(ROOT.glob("packages/*/module-package.yaml")) errors: list[str] = [] registry_path = ROOT / "registry" / "index.json" errors.extend(_validate_registry(registry_path)) + if not errors: + errors.extend(validate_registry_consistency(ROOT, registry_path)) registry_ids: set[str] | None = None if not errors: @@ -91,12 +302,12 @@ def main() -> int: errors.extend(validate_manifest_bundle_dependency_refs(manifest, registry_ids)) if errors: - print("Manifest/registry validation failed:") + _emit_line("Manifest/registry validation failed:", error=True) for err in errors: - print(f"- {err}") + _emit_line(f"- {err}", error=True) return 1 - print(f"Validated {len(manifest_paths)} manifests and registry/index.json") + _emit_line(f"Validated {len(manifest_paths)} manifests and registry/index.json") return 0