fix PR 193 review findings

.github/workflows/pr-orchestrator.yml

@@ -89,11 +89,12 @@ jobs:
           if [ "${{ github.event_name }}" = "pull_request" ]; then
             BASE_REF="origin/${{ github.event.pull_request.base.ref }}"
             TARGET_BRANCH="${{ github.event.pull_request.base.ref }}"
+            HEAD_REPO="${{ github.event.pull_request.head.repo.full_name }}"
+            THIS_REPO="${{ github.repository }}"
             VERIFY_CMD+=(--version-check-base "$BASE_REF")
             if [ "$TARGET_BRANCH" = "dev" ]; then
               VERIFY_CMD+=(--metadata-only)
-            elif [ "$TARGET_BRANCH" = "main" ] && \
-                 [ "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]; then
+            elif [ "$TARGET_BRANCH" = "main" ] && [ "$HEAD_REPO" = "$THIS_REPO" ]; then
               VERIFY_CMD+=(--require-signature)
             fi
           else

.github/workflows/sign-modules-on-approval.yml

@@ -30,6 +30,16 @@ jobs:
             echo "::notice::Skipping module signing: review state is not approved."
             exit 0
           fi
+          author_association="${{ github.event.review.user.author_association }}"
+          case "$author_association" in
+            COLLABORATOR|MEMBER|OWNER)
+              ;;
+            *)
+              echo "sign=false" >> "$GITHUB_OUTPUT"
+              echo "::notice::Skipping module signing: reviewer association '${author_association}' is not trusted for signing."
+              exit 0
+              ;;
+          esac
           base_ref="${{ github.event.pull_request.base.ref }}"
           if [ "$base_ref" != "dev" ] && [ "$base_ref" != "main" ]; then
             echo "sign=false" >> "$GITHUB_OUTPUT"
@@ -44,7 +54,7 @@ jobs:
             exit 0
           fi
           echo "sign=true" >> "$GITHUB_OUTPUT"
-          echo "Eligible for module signing (approved, same-repo PR to dev or main)."
+          echo "Eligible for module signing (approved by trusted reviewer, same-repo PR to dev or main)."
 
       - name: Guard signing secrets
         if: steps.gate.outputs.sign == 'true'

README.md

@@ -59,7 +59,7 @@ pre-commit install
 pre-commit run --all-files
 ```
 
-**Code review gate (matches specfact-cli core):** runs in **Block 2** after the module verify hook and Block 1 quality hooks (`pre-commit-quality-checks.sh block2`, which calls `scripts/pre_commit_code_review.py`). Staged paths under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, and `openspec/changes/` are eligible; `openspec/changes/**/TDD_EVIDENCE.md` is excluded from the gate. Only staged **`.py` / `.pyi`** files are forwarded to SpecFact (YAML, registry tarballs, and similar are skipped). The hook blocks the commit when the JSON report contains **error**-severity findings; warning-only outcomes do not block. The helper runs `specfact code review run --json --out .specfact/code-review.json` on those Python paths and prints a short findings summary on stderr. Full CLI options (`--mode`, `--focus`, `--level`, `--bug-hunt`, etc.) are documented under [Code review module](./docs/modules/code-review.md). Block 1 is split into separate pre-commit hooks so output appears between stages instead of buffering until the end. Requires a local **specfact-cli** install (`hatch run dev-deps` resolves sibling `../specfact-cli` or `SPECFACT_CLI_REPO`).
+**Code review gate (matches specfact-cli core):** runs in **Block 2** after the module verify hook and Block 1 quality hooks (`pre-commit-quality-checks.sh block2`, which calls `scripts/pre_commit_code_review.py`). Staged paths under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, and `openspec/changes/` are eligible; `openspec/changes/**/TDD_EVIDENCE.md` is excluded from the gate. Only staged **`.py` / `.pyi`** files are forwarded to SpecFact (YAML, registry tarballs, and similar are skipped). The hook blocks the commit when the JSON report contains **error**-severity findings; warning-only outcomes do not block. Non-blocking warnings reported by SpecFact still require remediation prior to merge unless a documented, approved exception exists. The helper runs `specfact code review run --json --out .specfact/code-review.json` on those Python paths and prints a short findings summary on stderr. Full CLI options (`--mode`, `--focus`, `--level`, `--bug-hunt`, etc.) are documented under [Code review module](./docs/modules/code-review.md). Block 1 is split into separate pre-commit hooks so output appears between stages instead of buffering until the end. Requires a local **specfact-cli** install (`hatch run dev-deps` resolves sibling `../specfact-cli` or `SPECFACT_CLI_REPO`).
 
 Scope notes:
 - Pre-commit runs `hatch run lint` in the **Block 1 — lint** hook when any staged path matches `*.py` / `*.pyi`, matching the CI quality job (Ruff alone does not run pylint).

docs/modules/code-review.md

@@ -277,7 +277,7 @@ is skipped with no error.
 
 ### Contract runner
 
-`specfact_code_review.tools.contract_runner.run_contract_check(files)` combines two
+`specfact_code_review.tools.contract_runner.run_contract_check(files, *, bug_hunt=False)` combines two
 contract-oriented checks:
 
 1. an AST scan for public functions missing `@require` / `@ensure`
@@ -287,9 +287,10 @@ AST scan behavior:
 
 - only public module-level and class-level functions are checked
 - functions prefixed with `_` are treated as private and skipped
-- the AST scan for `MISSING_ICONTRACT` runs **only when the file imports
-  `icontract`** (`from icontract …` or `import icontract`). Files that never
-  reference icontract skip the decorator scan and rely on CrossHair only
+- the AST scan for `MISSING_ICONTRACT` runs only when a batch-level package/repo
+  scan root imports `icontract` (`from icontract …` or `import icontract`).
+  Reviewed files in a package that uses icontract are scanned even when the
+  changed file itself does not import icontract
 - missing icontract decorators become `contracts` findings with rule
   `MISSING_ICONTRACT` when the scan runs
 - unreadable or invalid Python files degrade to a single `tool_error` finding instead
@@ -401,8 +402,15 @@ specfact code review rules update
 
 ## Review orchestration
 
-`specfact_code_review.run.runner.run_review(files, no_tests=False)` orchestrates the
-bundle runners in this order:
+`specfact_code_review.run.runner.run_review(
+files,
+no_tests=False,
+include_noise=False,
+progress_callback=None,
+bug_hunt=False,
+review_level=None,
+review_mode="enforce",
+)` orchestrates the bundle runners in this order:
 
 1. Ruff
 2. Radon
@@ -421,6 +429,23 @@ adding a new CLI flag.
 
 The merged findings are then scored into a governed `ReviewReport`.
 
+Representative programmatic use:
+
+```python
+from pathlib import Path
+
+from specfact_code_review.run.runner import run_review
+
+report = run_review(
+    [Path("src/app.py")],
+    no_tests=False,
+    include_noise=False,
+    bug_hunt=True,
+    review_level="error",
+    review_mode="shadow",
+)
+```
+
 ## Bundled policy pack
 
 The bundle now ships `specfact/clean-code-principles` as a resource payload at:

docs/reference/module-security.md

@@ -46,11 +46,13 @@ Module packages carry **publisher** and **integrity** metadata so installation,
   - `SPECFACT_MODULE_PRIVATE_SIGN_KEY`
   - `SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE`
 - **Verification command** (`scripts/verify-modules-signature.py`):
-  - **Baseline (PR/CI and local hook)**: `--payload-from-filesystem --enforce-version-bump` — full payload checksum verification plus version-bump enforcement. This is the default integration path **without** `--require-signature` when the target branch is **`dev`** (pull requests to `dev`, or pushes to `dev`).
-  - **Strict mode**: add `--require-signature` so every manifest must include a verifiable `integrity.signature`. In `.github/workflows/pr-orchestrator.yml` this is appended for **pull requests whose base is `main`** and for **pushes to `main`**, in addition to the baseline flags. Locally, `scripts/pre-commit-verify-modules-signature.sh` adds `--require-signature` only when the checkout (or `GITHUB_BASE_REF` in Actions) is **`main`**; otherwise it runs the same baseline flags only.
+  - **Baseline (CI)**: `--payload-from-filesystem --enforce-version-bump` — full payload checksum verification plus version-bump enforcement.
+  - **Dev-target PR mode**: `.github/workflows/pr-orchestrator.yml` appends `--metadata-only` for pull requests targeting `dev` so branch work is not blocked before approval-time signing refreshes manifests.
+  - **Strict mode**: add `--require-signature` so every manifest must include a verifiable `integrity.signature`. In `.github/workflows/pr-orchestrator.yml` this is appended for **same-repository pull requests whose base is `main`** (fork heads skip strict signature enforcement here) and for **pushes to `main`**, in addition to the baseline flags. Locally, `scripts/pre-commit-verify-modules-signature.sh` adds `--require-signature` only when the checkout (or `GITHUB_BASE_REF` in Actions) is **`main`**.
+  - **Local non-main hook mode**: `scripts/pre-commit-verify-modules-signature.sh` otherwise keeps the baseline command shape but adds `--metadata-only`, avoiding local checksum/signature enforcement on branches that are expected to be signed by CI.
   - **Pull request CI** also passes `--version-check-base <git-ref>` (typically `origin/<base branch>`) so version rules compare against the PR base.
-  - **CI uses the full verifier** (payload digest + rules above). It does **not** pass `--metadata-only`. The script still supports `--metadata-only` for optional tooling that only needs manifest shape and checksum format checks.
-- **CI signing**: Approved same-repo PRs to `dev` or `main` may receive automated signing commits via `sign-modules-on-approval.yml` (repository secrets; merge-base scoped `--changed-only`).
+  - **CI uses the full verifier** for `main` and push checks (payload digest + rules above), while PRs targeting `dev` intentionally pass `--metadata-only`. The script still supports `--metadata-only` for optional tooling that only needs manifest shape and checksum format checks.
+- **CI signing**: Approved same-repo PRs to `dev` or `main` from trusted reviewer associations may receive automated signing commits via `sign-modules-on-approval.yml` (repository secrets; merge-base scoped `--changed-only`).
 
 ## Public key and key rotation
 

openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md

@@ -1,3 +1,5 @@
+# Code Review Bug Finding
+
 ## ADDED Requirements
 
 ### Requirement: Semgrep bug-finding rules pass
@@ -10,21 +12,22 @@ silently skipped without error.
 #### Scenario: bugs.yaml present — security findings emitted
 
 - **WHEN** `.semgrep/bugs.yaml` exists in the bundle
-- **AND** `run_semgrep` is called on Python files matching a bug rule
-- **THEN** `ReviewFinding` records are returned with `category="security"` or `category="correctness"`
+- **AND** `run_semgrep_bugs` is called on Python files matching a bug rule
+- **THEN** `ReviewFinding` records are returned with `category="security"` or `category="clean_code"`
 - **AND** findings reference the matched rule id from `bugs.yaml`
 
 #### Scenario: bugs.yaml absent — pass is a no-op
 
 - **WHEN** no `.semgrep/bugs.yaml` file is discoverable
-- **AND** `run_semgrep` is called
+- **AND** `run_semgrep_bugs` is called
 - **THEN** no finding is returned for the missing bugs pass
 - **AND** no exception propagates to the caller
 
 #### 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
 

openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md

@@ -1,24 +1,26 @@
+# Contract Runner
+
 ## MODIFIED Requirements
 
 ### Requirement: icontract Decorator AST Scan and CrossHair Fast Pass
 
-The system SHALL AST-scan changed Python files for public functions missing
+The system SHALL AST-scan reviewed Python files for public functions missing
 `@require` / `@ensure` decorators, and run CrossHair with a configurable
 per-path timeout for counterexample discovery. When no icontract usage is
-detected in the reviewed files, `MISSING_ICONTRACT` findings SHALL be
+detected in the reviewed files' package/repo scan roots, `MISSING_ICONTRACT` findings SHALL be
 suppressed entirely for that run.
 
 #### Scenario: Public function without icontract decorators produces a contracts finding when icontract is in use
 
 - **GIVEN** a Python file with a public function lacking icontract decorators
-- **AND** at least one other reviewed file imports from `icontract`
+- **AND** at least one file in the reviewed batch's package/repo scan roots imports from `icontract`
 - **WHEN** `run_contract_check(files=[...])` is called
 - **THEN** a `ReviewFinding` is returned with `category="contracts"` and
   `severity="warning"`
 
 #### Scenario: MISSING_ICONTRACT suppressed when no icontract usage detected
 
-- **GIVEN** a set of reviewed Python files containing no `from icontract import` or
+- **GIVEN** the package/repo scan roots for the reviewed Python files contain no `from icontract import` or
   `import icontract` statements
 - **WHEN** `run_contract_check(files=[...])` is called
 - **THEN** no `MISSING_ICONTRACT` findings are returned

openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md

@@ -1,6 +1,8 @@
+# Review CLI Contracts
+
 ## ADDED Requirements
 
-### Requirement: Review-run CLI scenarios cover enforcement mode, focus facets, and severity level
+### Requirement: Review-run CLI scenarios cover enforcement mode, bug-hunt, focus facets, and severity level
 
 The modules repository SHALL extend `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` so contract tests exercise the new `specfact code review run` flags together with existing scope and JSON output behaviour.
 
@@ -11,20 +13,30 @@ The modules repository SHALL extend `tests/cli-contracts/specfact-code-review-ru
 - **THEN** it includes at least one scenario asserting `--mode shadow` yields process success (exit `0`) while JSON still reports a failing verdict when findings warrant it
 - **AND** it includes a control scenario showing `--mode enforce` (or default) preserves non-zero exit on blocking failures
 
+#### Scenario: Scenarios cover --bug-hunt in shadow and enforce modes
+
+- **GIVEN** `tests/cli-contracts/specfact-code-review-run.scenarios.yaml`
+- **WHEN** it is validated after this change
+- **THEN** it includes at least one scenario with `--bug-hunt --mode shadow`
+- **AND** it includes at least one scenario with `--bug-hunt --mode enforce`
+
 #### Scenario: Scenarios cover --focus facets
 
 - **GIVEN** the same scenario file
 - **WHEN** it is validated
 - **THEN** it includes coverage for `--focus` union behaviour (e.g. `source` + `docs`) and for `--focus tests` narrowing the file set
+- **AND** it includes coverage for `--bug-hunt` composed with `--focus`
 
 #### Scenario: Scenarios cover --level filtering
 
 - **GIVEN** the same scenario file
 - **WHEN** it is validated
 - **THEN** it includes at least one scenario where `--level error` removes warnings from the JSON `findings` list
+- **AND** it includes coverage for `--bug-hunt` composed with `--level error`
 
 #### Scenario: Scenarios cover invalid flag combinations
 
 - **GIVEN** the same scenario file
 - **WHEN** it is validated
 - **THEN** it includes an error-path scenario for `--focus` combined with `--include-tests` or `--exclude-tests`
+- **AND** `--bug-hunt` remains composable with test-selection flags

openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md

@@ -1,3 +1,5 @@
+# Review Run Command Specification
+
 ## ADDED Requirements
 
 ### Requirement: --bug-hunt flag on review run command

openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md

@@ -1,3 +1,5 @@
+# Sidecar Route Extraction
+
 ## ADDED Requirements
 
 ### Requirement: Framework extractors exclude .specfact from scan paths

openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md

@@ -1,4 +1,4 @@
-## 1. Sidecar venv self-scan fix (specfact-codebase)
+# 1. Sidecar venv self-scan fix (specfact-codebase)
 
 - [x] 1.1 Add `_EXCLUDED_DIR_NAMES` constant and `_iter_python_files(root)` generator to `BaseFrameworkExtractor` in `frameworks/base.py` that skips `.specfact`, `.git`, `__pycache__`, `node_modules`
 - [x] 1.2 Replace `search_path.rglob("*.py")` with `self._iter_python_files(search_path)` in `FastAPIExtractor.detect()` and `FastAPIExtractor.extract_routes()`

packages/specfact-code-review/.semgrep/bugs.yaml

@@ -35,7 +35,7 @@ rules:
     severity: WARNING
     patterns:
       - pattern: yaml.load(...)
-      - pattern-not-regex: yaml\.load\([^)]*Loader\s*=
+      - pattern-not-regex: (?s)yaml\.load\([\s\S]*?Loader\s*=
     metadata:
       specfact-category: security
 

packages/specfact-code-review/module-package.yaml

@@ -1,5 +1,5 @@
 name: nold-ai/specfact-code-review
-version: 0.47.2
+version: 0.47.6
 commands:
   - code
 tier: official
@@ -23,5 +23,4 @@ description: Official SpecFact code review bundle package.
 category: codebase
 bundle_group_command: code
 integrity:
-  checksum: sha256:f57b0d5c239273df9c87eea31fabc07df630d44d89f3f552da20b1816dd316b5
-  signature: uYll3YRRrGsNOHniavYpDE8Guq/bcwAT2vQ0GtmreHPi4lAHzdqv7ui8tlRh7uS/EFiqgIkBatULph2qXHoPBA==
+  checksum: sha256:d48dfce318a75ea66d9a2bb2b69c7ed0c29e1228732b138719555a470e9fc47b

packages/specfact-code-review/src/specfact_code_review/_review_utils.py

@@ -39,7 +39,7 @@ def normalize_path_variants(path_value: str | Path) -> set[str]:
 @require(lambda files: all(isinstance(p, Path) for p in files))
 @ensure(lambda result: isinstance(result, list))
 def python_source_paths_for_tools(files: list[Path]) -> list[Path]:
-    """Paths Python linters and typecheckers should analyze (excludes YAML manifests, etc.)."""
+    """Python source and type stub paths linters/typecheckers should analyze."""
     return [path for path in files if path.suffix in _PYTHON_LINTER_SUFFIXES]
 
 

packages/specfact-code-review/src/specfact_code_review/review/commands.py

@@ -11,25 +11,34 @@
 
 from specfact_code_review.ledger.commands import app as ledger_app
 from specfact_code_review.rules.commands import app as rules_app
-from specfact_code_review.run.commands import run_command
+from specfact_code_review.run.commands import (
+    ConflictingScopeError,
+    FocusFacetConflictError,
+    InvalidOptionCombinationError,
+    MissingOutForJsonError,
+    NoReviewableFilesError,
+    RunCommandError,
+    run_command,
+)
 
 
 app = typer.Typer(help="Code command extensions for structured review workflows.", no_args_is_help=True)
 review_app = typer.Typer(help="Governed code review workflows.", no_args_is_help=True)
 
 
-def _friendly_run_command_error(exc: ValueError | ViolationError) -> str:
-    message = str(exc)
-    for expected in (
-        "Use either --json or --score-only, not both.",
-        "Use --out together with --json.",
-        "Choose positional files or auto-scope controls, not both.",
-        "Cannot combine focus_facets with include_tests",
-        "No reviewable Python files matched the selected --focus facets.",
+def _friendly_run_command_error(exc: RunCommandError | ValueError | ViolationError) -> str:
+    if isinstance(
+        exc,
+        (
+            InvalidOptionCombinationError,
+            MissingOutForJsonError,
+            ConflictingScopeError,
+            FocusFacetConflictError,
+            NoReviewableFilesError,
+        ),
     ):
-        if expected in message:
-            return expected
-    return message
+        return str(exc)
+    return str(exc)
 
 
 def _resolve_include_tests(*, files: list[Path], include_tests: bool | None, interactive: bool) -> bool: