From a6fc898246ddf13b23a0510155a2a3aeee9aa75a Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Tue, 31 Mar 2026 00:07:17 +0200 Subject: [PATCH 1/2] expand code review clean-code checks --- CHANGELOG.md | 8 + docs/bundles/code-review/overview.md | 5 +- docs/modules/code-review.md | 36 ++- .../TDD_EVIDENCE.md | 30 ++ .../tasks.md | 24 +- .../.semgrep/clean_code.yaml | 22 ++ .../specfact-code-review/module-package.yaml | 6 +- .../specfact/clean-code-principles.yaml | 41 +++ .../specfact/clean-code-principles.yaml | 41 +++ .../skills/specfact-code-review/SKILL.md | 17 +- .../src/specfact_code_review/rules/updater.py | 55 ++-- .../src/specfact_code_review/run/findings.py | 10 + .../src/specfact_code_review/run/runner.py | 137 ++++++--- .../specfact_code_review/tools/__init__.py | 11 +- .../tools/ast_clean_code_runner.py | 197 +++++++++++++ .../tools/radon_runner.py | 261 ++++++++++++++---- .../tools/semgrep_runner.py | 42 ++- skills/specfact-code-review/SKILL.md | 19 +- .../rules/test_updater.py | 15 +- .../specfact_code_review/run/test_findings.py | 26 +- .../specfact_code_review/run/test_runner.py | 60 +++- .../tools/test___init__.py | 15 + .../tools/test_ast_clean_code_runner.py | 81 ++++++ .../tools/test_radon_runner.py | 31 +++ .../tools/test_semgrep_runner.py | 25 ++ tests/unit/test_bundle_resource_payloads.py | 40 +++ 26 files changed, 1076 insertions(+), 179 deletions(-) create mode 100644 openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md create mode 100644 packages/specfact-code-review/resources/policy-packs/specfact/clean-code-principles.yaml create mode 100644 packages/specfact-code-review/src/specfact_code_review/resources/policy-packs/specfact/clean-code-principles.yaml create mode 100644 packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py create mode 100644 tests/unit/specfact_code_review/tools/test___init__.py create mode 100644 tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ba436a..bfd91a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,14 @@ and this project follows SemVer for bundle versions. ### Added - Documentation: authoritative `docs/reference/documentation-url-contract.md` for core vs modules URL ownership; `redirect_from` aliases for legacy `/guides//` on pages whose canonical path is outside `/guides/`; sidebar link to the contract page. +- Add expanded clean-code review coverage to `specfact-code-review`, including + naming, KISS, YAGNI, DRY, SOLID, and PR-checklist findings plus the bundled + `specfact/clean-code-principles` policy-pack payload. + +### Changed + +- 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.0 release. ## [0.44.0] - 2026-03-17 diff --git a/docs/bundles/code-review/overview.md b/docs/bundles/code-review/overview.md index fa33ad9..e6a7206 100644 --- a/docs/bundles/code-review/overview.md +++ b/docs/bundles/code-review/overview.md @@ -45,7 +45,10 @@ Use it together with the [Codebase](../codebase/overview/) bundle (`import`, `an ## Bundle-owned skills and policy packs -House rules and review payloads ship **inside the bundle** (for example Semgrep packs and skill metadata). They are **not** core CLI-owned resources. Install or refresh IDE-side assets with `specfact init ide` after upgrading the bundle. +House rules and review payloads ship **inside the bundle** (for example Semgrep +packs, the `specfact/clean-code-principles` policy-pack manifest, and skill +metadata). They are **not** core CLI-owned resources. Install or refresh +IDE-side assets with `specfact init ide` after upgrading the bundle. ## Quick examples diff --git a/docs/modules/code-review.md b/docs/modules/code-review.md index 46506a9..ecc0bc0 100644 --- a/docs/modules/code-review.md +++ b/docs/modules/code-review.md @@ -323,7 +323,15 @@ Then rerun the ledger command from the same repository checkout. ## House rules skill The `specfact-code-review` bundle can derive a compact house-rules skill from the -reward ledger and keep it small enough for AI session context injection. +reward ledger and keep it small enough for AI session context injection. The +default charter now encodes the clean-code principles directly: + +- Naming: use intention-revealing names instead of placeholders. +- KISS: keep functions small, shallow, and narrow in parameters. +- YAGNI: remove unused private helpers and speculative layers. +- DRY: extract repeated function shapes once duplication appears. +- SOLID: keep transport and persistence responsibilities separate. +- TDD + contracts: keep test-first and icontract discipline in the baseline skill. ### Command flow @@ -362,13 +370,31 @@ bundle runners in this order: 1. Ruff 2. Radon -3. basedpyright -4. pylint -5. contract runner -6. TDD gate, unless `no_tests=True` +3. Semgrep +4. AST clean-code checks +5. basedpyright +6. pylint +7. contract runner +8. 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`, +`SPECFACT_CODE_REVIEW_PR_BODY`, and `SPECFACT_CODE_REVIEW_PR_PROPOSAL` without +adding a new CLI flag. The merged findings are then scored into a governed `ReviewReport`. +## Bundled policy pack + +The bundle now ships `specfact/clean-code-principles` as a resource payload at: + +- `packages/specfact-code-review/resources/policy-packs/specfact/clean-code-principles.yaml` +- `packages/specfact-code-review/src/specfact_code_review/resources/policy-packs/specfact/clean-code-principles.yaml` + +The manifest exposes the clean-code rule IDs directly so downstream policy code +can apply advisory, mixed, or hard modes without a second review-specific +severity schema. + ### TDD gate `specfact_code_review.run.runner.run_tdd_gate(files)` enforces a bundle-local diff --git a/openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md b/openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md new file mode 100644 index 0000000..108a864 --- /dev/null +++ b/openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md @@ -0,0 +1,30 @@ +# TDD Evidence + +## 2026-03-30 + +- `2026-03-30T22:57:17+02:00` Red phase: + `hatch run pytest tests/unit/specfact_code_review/run/test_findings.py tests/unit/specfact_code_review/run/test_runner.py tests/unit/specfact_code_review/tools/test_semgrep_runner.py tests/unit/specfact_code_review/tools/test_radon_runner.py tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py -q` + failed during collection before local `dev-deps` bootstrap because `specfact-cli` + runtime dependencies such as `beartype` were not yet available in the Hatch env. +- `2026-03-30T23:00:00+02:00` Bootstrap: + `hatch run dev-deps` + installed the local `specfact-cli` dependency set required by the bundle review tests. +- `2026-03-30T23:10:00+02:00` Green targeted runner slice: + `hatch run pytest tests/unit/specfact_code_review/run/test_findings.py tests/unit/specfact_code_review/run/test_runner.py tests/unit/specfact_code_review/tools/test_semgrep_runner.py tests/unit/specfact_code_review/tools/test_radon_runner.py tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py -q` + passed after the runner, AST, and test-fixture fixes. +- `2026-03-30T23:57:11+02:00` Green review run: + `SPECFACT_ALLOW_UNSIGNED=1 hatch run specfact code review run --json --out .specfact/code-review.json` + passed with `findings: []` after linking the live dev module and flattening the KISS-sensitive helpers. +- `2026-03-30T23:56:00+02:00` Green full targeted slice: + `hatch run pytest --cov=packages/specfact-code-review/src/specfact_code_review --cov-fail-under=0 --cov-report=json:/tmp/specfact-report.json tests/unit/specfact_code_review/rules/test_updater.py tests/unit/specfact_code_review/run/test_findings.py tests/unit/specfact_code_review/run/test_runner.py tests/unit/specfact_code_review/tools/test___init__.py tests/unit/specfact_code_review/tools/test_radon_runner.py tests/unit/specfact_code_review/tools/test_semgrep_runner.py tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py` + passed in `89 passed in 20.75s`. +- `2026-03-30T23:58:00+02:00` Green quality gates: + `hatch run format`, `hatch run type-check`, `hatch run lint`, `hatch run yaml-lint`, + `hatch run contract-test`, `hatch run smart-test`, and `hatch run test` + all passed in this worktree after the final helper flattening. +- `2026-03-30T23:58:00+02:00` Validation: + `openspec validate clean-code-02-expanded-review-module --strict` + passed with `Change 'clean-code-02-expanded-review-module' is valid`. +- `2026-03-30T23:58:00+02:00` Remaining release blocker: + `hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump` + failed with `packages/specfact-code-review/module-package.yaml: checksum mismatch`. diff --git a/openspec/changes/clean-code-02-expanded-review-module/tasks.md b/openspec/changes/clean-code-02-expanded-review-module/tasks.md index f2d2c33..87574bf 100644 --- a/openspec/changes/clean-code-02-expanded-review-module/tasks.md +++ b/openspec/changes/clean-code-02-expanded-review-module/tasks.md @@ -2,27 +2,27 @@ ## 1. Branch and dependency guardrails -- [ ] 1.1 Create dedicated worktree branch `feature/clean-code-02-expanded-review-module` from `dev` before implementation work. -- [ ] 1.2 Confirm the archived runner and review-run changes are available locally and note the cross-repo dependency on specfact-cli `code-review-zero-findings`. -- [ ] 1.3 Reconfirm scope against the 2026-03-22 clean-code implementation plan and `openspec/CHANGE_ORDER.md`. +- [x] 1.1 Create dedicated worktree branch `feature/clean-code-02-expanded-review-module` from `dev` before implementation work. +- [x] 1.2 Confirm the archived runner and review-run changes are available locally and note the cross-repo dependency on specfact-cli `code-review-zero-findings`. +- [x] 1.3 Reconfirm scope against the 2026-03-22 clean-code implementation plan and `openspec/CHANGE_ORDER.md`. ## 2. Spec-first and test-first preparation -- [ ] 2.1 Finalize spec deltas for finding schema expansion, runner behavior, policy-pack payload, and house-rules skill output. -- [ ] 2.2 Add or update tests derived from those scenarios before touching implementation. -- [ ] 2.3 Run targeted tests expecting failure and record results in `TDD_EVIDENCE.md`. +- [x] 2.1 Finalize spec deltas for finding schema expansion, runner behavior, policy-pack payload, and house-rules skill output. +- [x] 2.2 Add or update tests derived from those scenarios before touching implementation. +- [x] 2.3 Run targeted tests expecting failure and record results in `TDD_EVIDENCE.md`. ## 3. Implementation -- [ ] 3.1 Extend the review finding schema and runner orchestration for the new clean-code categories. -- [ ] 3.2 Implement or update the semgrep, radon, solid, yagni, dry, and checklist paths required by the new scenarios. -- [ ] 3.3 Ship the `specfact/clean-code-principles` policy-pack payload and refresh `skills/specfact-code-review/SKILL.md` with the compact charter. +- [x] 3.1 Extend the review finding schema and runner orchestration for the new clean-code categories. +- [x] 3.2 Implement or update the semgrep, radon, solid, yagni, dry, and checklist paths required by the new scenarios. +- [x] 3.3 Ship the `specfact/clean-code-principles` policy-pack payload and refresh `skills/specfact-code-review/SKILL.md` with the compact charter. ## 4. Validation and documentation -- [ ] 4.1 Re-run targeted tests, quality gates, and review fixtures until all changed scenarios pass. -- [ ] 4.2 Update bundle docs, changelog, and release metadata for the new categories and pack payload. -- [ ] 4.3 Run `openspec validate clean-code-02-expanded-review-module --strict` and resolve all issues. +- [x] 4.1 Re-run targeted tests, quality gates, and review fixtures until all changed scenarios pass. +- [x] 4.2 Update bundle docs, changelog, and release metadata for the new categories and pack payload. +- [x] 4.3 Run `openspec validate clean-code-02-expanded-review-module --strict` and resolve all issues. ## 5. Delivery diff --git a/packages/specfact-code-review/.semgrep/clean_code.yaml b/packages/specfact-code-review/.semgrep/clean_code.yaml index 76e18a6..3e33c86 100644 --- a/packages/specfact-code-review/.semgrep/clean_code.yaml +++ b/packages/specfact-code-review/.semgrep/clean_code.yaml @@ -53,3 +53,25 @@ rules: severity: WARNING languages: [python] pattern: print(...) + + - id: banned-generic-public-names + message: Public API names should be specific; avoid generic names like process, handle, or manager. + severity: WARNING + languages: [python] + pattern-regex: '(?m)^(?:def|class)\s+(?:process|handle|manager|data)\b' + + - id: swallowed-exception-pattern + message: Exception handlers must not swallow failures with pass or silent returns. + severity: WARNING + languages: [python] + pattern-either: + - pattern: | + try: + ... + except Exception: + pass + - pattern: | + try: + ... + except: + pass diff --git a/packages/specfact-code-review/module-package.yaml b/packages/specfact-code-review/module-package.yaml index 04a7a88..44e82dd 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.44.3 +version: 0.45.0 commands: - code tier: official @@ -22,5 +22,5 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:eeef7d281055dceae470e317a37eb7c76087f12994b991d8bce86c6612746758 - signature: BaV6fky8HlxFC5SZFgWAHLMAXf62MEQEp1S6wsgV+otMjkr5IyhCoQ8TJvx072klIAMh11N130Wzg4aexlcADA== + checksum: sha256:d678813d42c84799242282094744369cad8f54942d3cd78b35de3b1b4bcce520 + signature: tt9xLDRe6s8vBSh71rixZl8TC0nOtOuGJmQf32rt3i/Ar0eM6B1VWkZZeW2TPi/J0Fa4MtApemIb2LW1scNXBg== diff --git a/packages/specfact-code-review/resources/policy-packs/specfact/clean-code-principles.yaml b/packages/specfact-code-review/resources/policy-packs/specfact/clean-code-principles.yaml new file mode 100644 index 0000000..4f8d216 --- /dev/null +++ b/packages/specfact-code-review/resources/policy-packs/specfact/clean-code-principles.yaml @@ -0,0 +1,41 @@ +pack_ref: specfact/clean-code-principles +version: 1 +description: Built-in clean-code review rules mapped to the governed code-review bundle outputs. +default_mode: advisory +rules: + - id: banned-generic-public-names + category: naming + principle: naming + - id: swallowed-exception-pattern + category: clean_code + principle: clean_code + - id: kiss.loc.warning + category: kiss + principle: kiss + - id: kiss.loc.error + category: kiss + principle: kiss + - id: kiss.nesting.warning + category: kiss + principle: kiss + - id: kiss.nesting.error + category: kiss + principle: kiss + - id: kiss.parameter-count.warning + category: kiss + principle: kiss + - id: kiss.parameter-count.error + category: kiss + principle: kiss + - id: yagni.unused-private-helper + category: yagni + principle: yagni + - id: dry.duplicate-function-shape + category: dry + principle: dry + - id: solid.mixed-dependency-role + category: solid + principle: solid + - id: clean-code.pr-checklist-missing-rationale + category: clean_code + principle: checklist diff --git a/packages/specfact-code-review/src/specfact_code_review/resources/policy-packs/specfact/clean-code-principles.yaml b/packages/specfact-code-review/src/specfact_code_review/resources/policy-packs/specfact/clean-code-principles.yaml new file mode 100644 index 0000000..4f8d216 --- /dev/null +++ b/packages/specfact-code-review/src/specfact_code_review/resources/policy-packs/specfact/clean-code-principles.yaml @@ -0,0 +1,41 @@ +pack_ref: specfact/clean-code-principles +version: 1 +description: Built-in clean-code review rules mapped to the governed code-review bundle outputs. +default_mode: advisory +rules: + - id: banned-generic-public-names + category: naming + principle: naming + - id: swallowed-exception-pattern + category: clean_code + principle: clean_code + - id: kiss.loc.warning + category: kiss + principle: kiss + - id: kiss.loc.error + category: kiss + principle: kiss + - id: kiss.nesting.warning + category: kiss + principle: kiss + - id: kiss.nesting.error + category: kiss + principle: kiss + - id: kiss.parameter-count.warning + category: kiss + principle: kiss + - id: kiss.parameter-count.error + category: kiss + principle: kiss + - id: yagni.unused-private-helper + category: yagni + principle: yagni + - id: dry.duplicate-function-shape + category: dry + principle: dry + - id: solid.mixed-dependency-role + category: solid + principle: solid + - id: clean-code.pr-checklist-missing-rationale + category: clean_code + principle: checklist diff --git a/packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md b/packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md index dbcd60d..4214e0e 100644 --- a/packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md +++ b/packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md @@ -6,27 +6,28 @@ allowed-tools: [] # House Rules - AI Coding Context (v1) -Updated: 2026-03-16 | Module: nold-ai/specfact-code-review +Updated: 2026-03-30 | Module: nold-ai/specfact-code-review ## DO - Ask whether tests should be included before repo-wide review; default to excluding tests unless test changes are the target -- Keep functions under 120 LOC and cyclomatic complexity <= 12 +- Use intention-revealing names; avoid placeholder public names like data/process/handle +- Keep functions under 120 LOC, shallow nesting, and <= 5 parameters (KISS) +- Delete unused private helpers and speculative abstractions quickly (YAGNI) +- Extract repeated function shapes once the second copy appears (DRY) +- Split persistence and transport concerns instead of mixing repository.* with http_client.* (SOLID) - Add @require/@ensure (icontract) + @beartype to all new public APIs - Run hatch run contract-test-contracts before any commit -- Guard all chained attribute access: a.b.c needs null-check or early return -- Return typed values from all public methods - Write the test file BEFORE the feature file (TDD-first) -- Use get_logger(__name__) from common.logger_setup, never print() +- Return typed values from all public methods and guard chained attribute access ## DON'T - Don't enable known noisy findings unless you explicitly want strict/full review output -- Don't mix read + write in the same method; split responsibilities - Don't use bare except: or except Exception: pass - Don't add # noqa / # type: ignore without inline justification -- Don't call repository.* and http_client.* in the same function +- Don't mix read + write in the same method or call repository.* and http_client.* together - Don't import at module level if it triggers network calls - Don't hardcode secrets; use env vars via pydantic.BaseSettings -- Don't create functions > 120 lines +- Don't create functions that exceed the KISS thresholds without a documented reason ## TOP VIOLATIONS (auto-updated by specfact code review rules update) diff --git a/packages/specfact-code-review/src/specfact_code_review/rules/updater.py b/packages/specfact-code-review/src/specfact_code_review/rules/updater.py index e11c089..9938c01 100644 --- a/packages/specfact-code-review/src/specfact_code_review/rules/updater.py +++ b/packages/specfact-code-review/src/specfact_code_review/rules/updater.py @@ -31,23 +31,24 @@ DEFAULT_DO_RULES = ( "- Ask whether tests should be included before repo-wide review; " "default to excluding tests unless test changes are the target", - "- Keep functions under 120 LOC and cyclomatic complexity <= 12", + "- Use intention-revealing names; avoid placeholder public names like data/process/handle", + "- Keep functions under 120 LOC, shallow nesting, and <= 5 parameters (KISS)", + "- Delete unused private helpers and speculative abstractions quickly (YAGNI)", + "- Extract repeated function shapes once the second copy appears (DRY)", + "- Split persistence and transport concerns instead of mixing repository.* with http_client.* (SOLID)", "- Add @require/@ensure (icontract) + @beartype to all new public APIs", "- Run hatch run contract-test-contracts before any commit", - "- Guard all chained attribute access: a.b.c needs null-check or early return", - "- Return typed values from all public methods", "- Write the test file BEFORE the feature file (TDD-first)", - "- Use get_logger(__name__) from common.logger_setup, never print()", + "- Return typed values from all public methods and guard chained attribute access", ) DEFAULT_DONT_RULES = ( "- Don't enable known noisy findings unless you explicitly want strict/full review output", - "- Don't mix read + write in the same method; split responsibilities", "- Don't use bare except: or except Exception: pass", "- Don't add # noqa / # type: ignore without inline justification", - "- Don't call repository.* and http_client.* in the same function", + "- Don't mix read + write in the same method or call repository.* and http_client.* together", "- Don't import at module level if it triggers network calls", "- Don't hardcode secrets; use env vars via pydantic.BaseSettings", - "- Don't create functions > 120 lines", + "- Don't create functions that exceed the KISS thresholds without a documented reason", ) @@ -105,15 +106,7 @@ def sync_skill_to_ide( @ensure(lambda result: bool(result.strip())) def render_cursor_rule(content: str) -> str: """Render SKILL.md content as a Cursor auto-attached rule.""" - body = content - description = DEFAULT_DESCRIPTION - if content.startswith("---\n"): - _, _, remainder = content.partition("\n---\n") - if remainder: - body = remainder.lstrip("\n") - match = re.search(r"^description:\s*(?P.+)$", content, flags=re.MULTILINE) - if match: - description = match.group("description").strip() + body, description = _cursor_rule_parts(content) lines = [ "---", f"description: {description}", @@ -125,6 +118,23 @@ def render_cursor_rule(content: str) -> str: return "\n".join(lines) + "\n" +def _cursor_rule_parts(content: str) -> tuple[str, str]: + body = content + description = DEFAULT_DESCRIPTION + if not content.startswith("---\n"): + return body, description + + _, _, remainder = content.partition("\n---\n") + if not remainder: + return body, description + + body = remainder.lstrip("\n") + match = re.search(r"^description:\s*(?P.+)$", content, flags=re.MULTILINE) + if match: + description = match.group("description").strip() + return body, description + + @beartype @ensure( lambda result: len(result.splitlines()) <= MAX_SKILL_LINES, @@ -247,13 +257,12 @@ def _next_version(title_line: str) -> int: def _count_rules(runs: Sequence[LedgerRun]) -> Counter[str]: - counts: Counter[str] = Counter() - for run in runs: - for finding in run.findings_json: - rule = finding.get("rule") - if isinstance(rule, str) and rule.strip(): - counts[rule] += 1 - return counts + return Counter( + rule + for run in runs + for finding in run.findings_json + if isinstance(rule := finding.get("rule"), str) and rule.strip() + ) def _parse_existing_rules(lines: Sequence[str]) -> list[str]: diff --git a/packages/specfact-code-review/src/specfact_code_review/run/findings.py b/packages/specfact-code-review/src/specfact_code_review/run/findings.py index 9e1a623..ccaa967 100644 --- a/packages/specfact-code-review/src/specfact_code_review/run/findings.py +++ b/packages/specfact-code-review/src/specfact_code_review/run/findings.py @@ -19,6 +19,11 @@ "style", "architecture", "tool_error", + "naming", + "kiss", + "yagni", + "dry", + "solid", ) VALID_SEVERITIES = ("error", "warning", "info") PASS = "PASS" @@ -38,6 +43,11 @@ class ReviewFinding(BaseModel): "style", "architecture", "tool_error", + "naming", + "kiss", + "yagni", + "dry", + "solid", ] = Field(..., description="Governed code-review category.") severity: Literal["error", "warning", "info"] = Field(..., description="Finding severity.") tool: str = Field(..., description="Originating tool name.") 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 3fc8095..c255786 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 @@ -7,7 +7,7 @@ import subprocess import sys import tempfile -from collections.abc import Callable +from collections.abc import Callable, Iterable from contextlib import suppress from pathlib import Path from uuid import uuid4 @@ -18,6 +18,7 @@ from specfact_code_review._review_utils import _normalize_path_variants, _tool_error from specfact_code_review.run.findings import ReviewFinding, ReviewReport from specfact_code_review.run.scorer import score_review +from specfact_code_review.tools.ast_clean_code_runner import run_ast_clean_code from specfact_code_review.tools.basedpyright_runner import run_basedpyright from specfact_code_review.tools.contract_runner import run_contract_check from specfact_code_review.tools.pylint_runner import run_pylint @@ -40,23 +41,39 @@ ("pylint", "R0801"), } _NOISE_MESSAGE_PREFIXES = ("ValidationError: 1 validation error for LedgerState",) +_PR_MODE_ENV = "SPECFACT_CODE_REVIEW_PR_MODE" +_PR_CONTEXT_ENVS = ( + "SPECFACT_CODE_REVIEW_PR_TITLE", + "SPECFACT_CODE_REVIEW_PR_BODY", + "SPECFACT_CODE_REVIEW_PR_PROPOSAL", +) +_CLEAN_CODE_CONTEXT_HINTS = ("clean code", "naming", "kiss", "yagni", "dry", "solid", "complexity") def _source_relative_path(source_file: Path) -> Path | None: - source_root_candidates = [_SOURCE_ROOT] - with suppress(OSError): - source_root_candidates.append(_SOURCE_ROOT.resolve()) - - source_file_candidates = [source_file] - with suppress(OSError): - source_file_candidates.append(source_file.resolve()) - - for candidate in source_file_candidates: - for source_root in source_root_candidates: - try: - return candidate.relative_to(source_root) - except ValueError: - continue + source_root_candidates = [_SOURCE_ROOT, *_resolved_path_variants(_SOURCE_ROOT)] + source_file_candidates = [source_file, *_resolved_path_variants(source_file)] + return next( + ( + relative_path + for candidate in source_file_candidates + for source_root in source_root_candidates + if (relative_path := _relative_to(candidate, source_root)) is not None + ), + None, + ) + + +def _resolved_path_variants(path: Path) -> list[Path]: + try: + return [path.resolve()] + except OSError: + return [] + + +def _relative_to(candidate: Path, source_root: Path) -> Path | None: + with suppress(ValueError): + return candidate.relative_to(source_root) return None @@ -90,34 +107,38 @@ def _coverage_for_source(source_file: Path, payload: dict[str, object]) -> float def _pytest_env() -> dict[str, str]: env = os.environ.copy() - pythonpath_entries: list[str] = [] - - workspace_root = str(Path.cwd().resolve()) - pythonpath_entries.append(workspace_root) - source_root = str(_SOURCE_ROOT.resolve()) - if source_root not in pythonpath_entries: - pythonpath_entries.append(source_root) - - existing_pythonpath = env.get("PYTHONPATH", "") - if existing_pythonpath: - for entry in existing_pythonpath.split(os.pathsep): - if entry and entry not in pythonpath_entries: - pythonpath_entries.append(entry) - - for entry in sys.path: - if not entry: - continue - entry_path = Path(entry) - if not entry_path.exists(): - continue - resolved = str(entry_path.resolve()) - if resolved not in pythonpath_entries: - pythonpath_entries.append(resolved) - + pythonpath_entries: list[str] = [str(Path.cwd().resolve()), str(_SOURCE_ROOT.resolve())] + _extend_unique_entries(pythonpath_entries, env.get("PYTHONPATH", ""), split_by=os.pathsep) + _extend_unique_entries( + pythonpath_entries, + (str(Path(entry).resolve()) for entry in sys.path if entry and Path(entry).exists()), + ) env["PYTHONPATH"] = os.pathsep.join(pythonpath_entries) return env +def _extend_unique_entries( + entries: list[str], + values: Iterable[str] | str, + *, + split_by: str | None = None, +) -> None: + for entry in _iter_unique_entries(values, split_by=split_by): + if entry and entry not in entries: + entries.append(entry) + + +def _iter_unique_entries( + values: Iterable[str] | str, + *, + split_by: str | None = None, +) -> Iterable[str]: + if isinstance(values, str): + yield from values.split(split_by) if split_by is not None else [values] + return + yield from values + + def _pytest_targets(test_files: list[Path]) -> list[Path]: if len(test_files) <= 1: return test_files @@ -186,11 +207,43 @@ def _suppress_known_noise(findings: list[ReviewFinding]) -> list[ReviewFinding]: return filtered +def _is_truthy_env(name: str) -> bool: + return os.environ.get(name, "").strip().lower() in {"1", "true", "yes", "on"} + + +def _checklist_findings() -> list[ReviewFinding]: + if not _is_truthy_env(_PR_MODE_ENV): + return [] + + context = "\n".join( + os.environ.get(name, "").strip() for name in _PR_CONTEXT_ENVS if os.environ.get(name, "").strip() + ) + if any(hint in context.lower() for hint in _CLEAN_CODE_CONTEXT_HINTS): + return [] + + return [ + ReviewFinding( + category="clean_code", + severity="info", + tool="checklist", + rule="clean-code.pr-checklist-missing-rationale", + file="PR_CONTEXT", + line=1, + message=( + "PR context is missing explicit clean-code reasoning. " + "Call out the naming, KISS, YAGNI, DRY, or SOLID impact in the proposal or PR body." + ), + fixable=False, + ) + ] + + def _tool_steps() -> 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 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), @@ -231,7 +284,7 @@ def _coverage_findings( coverage_by_source: dict[str, float] = {} for source_file in source_files: percent_covered = _coverage_for_source(source_file, coverage_payload) - if percent_covered is None: + if percent_covered is None and source_file.name != "__init__.py": return [ _tool_error( tool="pytest", @@ -239,6 +292,8 @@ def _coverage_findings( message=f"Coverage data missing for {source_file}", ) ], None + if percent_covered is None: + continue coverage_by_source[str(source_file)] = percent_covered if percent_covered >= _COVERAGE_THRESHOLD: continue @@ -359,6 +414,8 @@ def run_review( findings.extend(tdd_findings) coverage_90_plus = bool(coverage_by_source) and all(percent >= 90.0 for percent in coverage_by_source.values()) + findings.extend(_checklist_findings()) + if not include_noise: findings = _suppress_known_noise(findings) diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/__init__.py b/packages/specfact-code-review/src/specfact_code_review/tools/__init__.py index f862909..85e7b19 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/__init__.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/__init__.py @@ -1,5 +1,6 @@ """Tool runners for code-review analysis.""" +from specfact_code_review.tools.ast_clean_code_runner import run_ast_clean_code from specfact_code_review.tools.basedpyright_runner import run_basedpyright from specfact_code_review.tools.contract_runner import run_contract_check from specfact_code_review.tools.pylint_runner import run_pylint @@ -8,4 +9,12 @@ from specfact_code_review.tools.semgrep_runner import run_semgrep -__all__ = ["run_basedpyright", "run_contract_check", "run_pylint", "run_radon", "run_ruff", "run_semgrep"] +__all__ = [ + "run_ast_clean_code", + "run_basedpyright", + "run_contract_check", + "run_pylint", + "run_radon", + "run_ruff", + "run_semgrep", +] 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 new file mode 100644 index 0000000..de83d35 --- /dev/null +++ b/packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py @@ -0,0 +1,197 @@ +"""AST-backed clean-code runner for governed review findings.""" + +from __future__ import annotations + +import ast +import copy +from collections import defaultdict +from pathlib import Path + +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 + + +_REPOSITORY_ROOTS = {"repo", "repository"} +_HTTP_ROOTS = {"client", "http_client", "requests", "session"} +_CONTROL_FLOW_NODES = (ast.If, ast.For, ast.AsyncFor, ast.While, ast.Try, ast.With, ast.AsyncWith, ast.Match) + + +class _ShapeNormalizer(ast.NodeTransformer): + """Erase local naming details while preserving code structure.""" + + @require(lambda node: isinstance(node, ast.Name)) + @ensure(lambda result: isinstance(result, ast.AST)) + def visit_Name(self, node: ast.Name) -> ast.AST: + return ast.copy_location(ast.Name(id="VAR", ctx=node.ctx), node) + + @require(lambda node: isinstance(node, ast.arg)) + @ensure(lambda result: isinstance(result, ast.AST)) + def visit_arg(self, node: ast.arg) -> ast.AST: + return ast.copy_location(ast.arg(arg="ARG", annotation=None, type_comment=None), node) + + @require(lambda node: isinstance(node, ast.Attribute)) + @ensure(lambda result: isinstance(result, ast.AST)) + def visit_Attribute(self, node: ast.Attribute) -> ast.AST: + normalized_value = self.visit(node.value) + return ast.copy_location(ast.Attribute(value=normalized_value, attr="ATTR", ctx=node.ctx), node) + + @require(lambda node: isinstance(node, ast.Constant)) + @ensure(lambda result: isinstance(result, ast.AST)) + def visit_Constant(self, node: ast.Constant) -> ast.AST: + placeholder = node.value if isinstance(node.value, bool | type(None)) else "CONST" + return ast.copy_location(ast.Constant(value=placeholder), node) + + @require(lambda node: isinstance(node, ast.FunctionDef)) + @ensure(lambda result: isinstance(result, ast.AST)) + def visit_FunctionDef(self, node: ast.FunctionDef) -> ast.AST: + normalized = self.generic_visit(node) + assert isinstance(normalized, ast.FunctionDef) + normalized.name = "FUNC" + return normalized + + @require(lambda node: isinstance(node, ast.AsyncFunctionDef)) + @ensure(lambda result: isinstance(result, ast.AST)) + def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> ast.AST: + normalized = self.generic_visit(node) + assert isinstance(normalized, ast.AsyncFunctionDef) + normalized.name = "FUNC" + return normalized + + +def _iter_functions(tree: ast.AST) -> list[ast.FunctionDef | ast.AsyncFunctionDef]: + return [node for node in ast.walk(tree) if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef)] + + +def _module_level_functions(tree: ast.Module) -> list[ast.FunctionDef | ast.AsyncFunctionDef]: + return [node for node in tree.body if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef)] + + +def _loaded_names(tree: ast.AST) -> set[str]: + return {node.id for node in ast.walk(tree) if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Load)} + + +def _leftmost_name(node: ast.AST) -> str | None: + current = node + while isinstance(current, ast.Attribute): + current = current.value + if isinstance(current, ast.Name): + return current.id + return None + + +def _call_roots(function_node: ast.FunctionDef | ast.AsyncFunctionDef) -> set[str]: + roots: set[str] = set() + for node in ast.walk(function_node): + if not isinstance(node, ast.Call): + continue + root = _leftmost_name(node.func) + if root is not None: + roots.add(root) + return roots + + +def _duplicate_shape_id(function_node: ast.FunctionDef | ast.AsyncFunctionDef) -> str: + normalized = _ShapeNormalizer().visit( + ast.fix_missing_locations(ast.Module(body=[copy.deepcopy(function_node)], type_ignores=[])) + ) + return ast.dump(normalized, include_attributes=False) + + +def _yagni_findings(file_path: Path, tree: ast.Module) -> list[ReviewFinding]: + loaded_names = _loaded_names(tree) + findings: list[ReviewFinding] = [] + for function_node in _module_level_functions(tree): + if not function_node.name.startswith("_") or function_node.name.startswith("__"): + continue + if function_node.name in loaded_names: + continue + findings.append( + ReviewFinding( + category="yagni", + severity="warning", + tool="ast", + rule="yagni.unused-private-helper", + file=str(file_path), + line=function_node.lineno, + message=f"Private helper `{function_node.name}` is not referenced in this module.", + fixable=False, + ) + ) + return findings + + +def _dry_findings(file_path: Path, tree: ast.Module) -> list[ReviewFinding]: + functions = _module_level_functions(tree) + grouped: dict[str, list[ast.FunctionDef | ast.AsyncFunctionDef]] = defaultdict(list) + for function_node in functions: + grouped[_duplicate_shape_id(function_node)].append(function_node) + + findings: list[ReviewFinding] = [] + for duplicates in grouped.values(): + if len(duplicates) < 2: + continue + for duplicate in duplicates[1:]: + findings.append( + ReviewFinding( + category="dry", + severity="warning", + tool="ast", + rule="dry.duplicate-function-shape", + file=str(file_path), + line=duplicate.lineno, + message=f"Function `{duplicate.name}` duplicates another function shape in this module.", + fixable=False, + ) + ) + return findings + + +def _solid_findings(file_path: Path, tree: ast.Module) -> list[ReviewFinding]: + findings: list[ReviewFinding] = [] + for function_node in _iter_functions(tree): + roots = _call_roots(function_node) + if roots.isdisjoint(_REPOSITORY_ROOTS) or roots.isdisjoint(_HTTP_ROOTS): + continue + findings.append( + ReviewFinding( + category="solid", + severity="warning", + tool="ast", + rule="solid.mixed-dependency-role", + file=str(file_path), + line=function_node.lineno, + message=( + f"Function `{function_node.name}` mixes repository-style and HTTP-style dependencies; " + "split the responsibility." + ), + fixable=False, + ) + ) + return findings + + +@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_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: + try: + tree = ast.parse(file_path.read_text(encoding="utf-8"), filename=str(file_path)) + except (OSError, SyntaxError) as exc: + return [_tool_error(tool="ast", file_path=file_path, message=f"Unable to parse Python source: {exc}")] + + findings.extend(_solid_findings(file_path, tree)) + findings.extend(_yagni_findings(file_path, tree)) + findings.extend(_dry_findings(file_path, tree)) + + return findings 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 eaa9684..033d90e 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 @@ -1,7 +1,9 @@ +# pylint: disable=line-too-long """Radon runner for governed code-review findings.""" from __future__ import annotations +import ast import json import os import subprocess @@ -14,6 +16,15 @@ from specfact_code_review.run.findings import ReviewFinding +_KISS_LOC_WARNING = 80 +_KISS_LOC_ERROR = 120 +_KISS_NESTING_WARNING = 2 +_KISS_NESTING_ERROR = 3 +_KISS_PARAMETER_WARNING = 5 +_KISS_PARAMETER_ERROR = 7 +_CONTROL_FLOW_NODES = (ast.If, ast.For, ast.AsyncFor, ast.While, ast.Try, ast.With, ast.AsyncWith, ast.Match) + + def _normalize_path_variants(path_value: str | Path) -> set[str]: path = Path(path_value) variants = { @@ -57,27 +68,135 @@ def _iter_blocks(blocks: list[Any]) -> list[dict[str, Any]]: if not isinstance(block, dict): raise ValueError("radon block must be an object") flattened.append(block) - closures = block.get("closures", []) - if closures: - if not isinstance(closures, list): - raise ValueError("radon closures must be a list") - flattened.extend(_iter_blocks(closures)) + _extend_block_closures(flattened, block) return flattened -@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_radon(files: list[Path]) -> list[ReviewFinding]: - """Run Radon for the provided files and map complexity findings into ReviewFinding records.""" - if not files: +def _extend_block_closures(flattened: list[dict[str, Any]], block: dict[str, Any]) -> None: + closures = block.get("closures", []) + if not closures: + return + if not isinstance(closures, list): + raise ValueError("radon closures must be a list") + flattened.extend(_iter_blocks(closures)) + + +def _control_flow_children(node: ast.AST) -> list[ast.AST]: + return [child for child in ast.iter_child_nodes(node) if isinstance(child, _CONTROL_FLOW_NODES)] + + +def _nesting_depth(function_node: ast.FunctionDef | ast.AsyncFunctionDef) -> int: + def _depth(node: ast.AST, current: int) -> int: + best = current + for child in _control_flow_children(node): + best = max(best, _depth(child, current + 1)) + return best + + return _depth(function_node, 0) + + +def _kiss_metric_findings(file_path: Path) -> list[ReviewFinding]: + if not file_path.is_file(): return [] + try: + tree = ast.parse(file_path.read_text(encoding="utf-8"), filename=str(file_path)) + except (OSError, SyntaxError) as exc: + return _tool_error(file_path, f"Unable to parse source for KISS metrics: {exc}") + + findings: list[ReviewFinding] = [] + for function_node in ast.walk(tree): + if not isinstance(function_node, ast.FunctionDef | ast.AsyncFunctionDef): + continue + findings.extend(_kiss_loc_findings(function_node, file_path)) + findings.extend(_kiss_nesting_findings(function_node, file_path)) + findings.extend(_kiss_parameter_findings(function_node, file_path)) + return findings + + +def _kiss_loc_findings(function_node: ast.FunctionDef | ast.AsyncFunctionDef, file_path: Path) -> list[ReviewFinding]: + findings: list[ReviewFinding] = [] + loc = (function_node.end_lineno or function_node.lineno) - function_node.lineno + 1 + if loc <= _KISS_LOC_WARNING: + return findings + severity = "warning" if loc <= _KISS_LOC_ERROR else "error" + suffix = "warning" if severity == "warning" else "error" + findings.append( + ReviewFinding( + category="kiss", + severity=severity, + tool="radon", + rule=f"kiss.loc.{suffix}", + file=str(file_path), + line=function_node.lineno, + message=(f"Function `{function_node.name}` spans {loc} lines; keep it under {{_KISS_LOC_WARNING}}."), + fixable=False, + ) + ) + return findings + + +def _kiss_nesting_findings( + function_node: ast.FunctionDef | ast.AsyncFunctionDef, file_path: Path +) -> list[ReviewFinding]: + findings: list[ReviewFinding] = [] + nesting = _nesting_depth(function_node) + if nesting <= _KISS_NESTING_WARNING: + return findings + severity = "warning" if nesting <= _KISS_NESTING_ERROR else "error" + suffix = "warning" if severity == "warning" else "error" + findings.append( + ReviewFinding( + category="kiss", + severity=severity, + tool="radon", + rule=f"kiss.nesting.{suffix}", + file=str(file_path), + line=function_node.lineno, + message=( + f"Function `{function_node.name}` nests control flow {nesting} levels deep;" + f" keep it under {_KISS_NESTING_WARNING}." + ), + fixable=False, + ) + ) + return findings + + +def _kiss_parameter_findings( + function_node: ast.FunctionDef | ast.AsyncFunctionDef, file_path: Path +) -> list[ReviewFinding]: + findings: list[ReviewFinding] = [] + parameter_count = len(function_node.args.posonlyargs) + parameter_count += len(function_node.args.args) + parameter_count += len(function_node.args.kwonlyargs) + if function_node.args.vararg is not None: + parameter_count += 1 + if function_node.args.kwarg is not None: + parameter_count += 1 + if parameter_count <= _KISS_PARAMETER_WARNING: + return findings + severity = "warning" if parameter_count <= _KISS_PARAMETER_ERROR else "error" + suffix = "warning" if severity == "warning" else "error" + findings.append( + ReviewFinding( + category="kiss", + severity=severity, + tool="radon", + rule=f"kiss.parameter-count.{suffix}", + file=str(file_path), + line=function_node.lineno, + message=( + f"Function `{function_node.name}` accepts {parameter_count} parameters;" + f" keep it under {_KISS_PARAMETER_WARNING}." + ), + fixable=False, + ) + ) + return findings + + +def _run_radon_command(files: list[Path]) -> dict[str, Any] | None: try: result = subprocess.run( ["radon", "cc", "-j", *(str(file_path) for file_path in files)], @@ -89,45 +208,81 @@ def run_radon(files: list[Path]) -> list[ReviewFinding]: payload = json.loads(result.stdout) if not isinstance(payload, dict): raise ValueError("radon output must be an object") - except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: - return _tool_error(files[0], f"Unable to parse Radon output: {exc}") + return payload + except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired): + return None - allowed_paths = _allowed_paths(files) + +def _map_radon_complexity_findings(payload: dict[str, Any], allowed_paths: set[str]) -> list[ReviewFinding]: + findings: list[ReviewFinding] = [] + for filename, blocks in payload.items(): + if not isinstance(filename, str): + raise ValueError("radon filename must be a string") + if _normalize_path_variants(filename).isdisjoint(allowed_paths): + continue + if not isinstance(blocks, list): + raise ValueError("radon file payload must be a list") + findings.extend(_map_radon_blocks(blocks, filename)) + return findings + + +def _map_radon_blocks(blocks: list[Any], filename: str) -> list[ReviewFinding]: findings: list[ReviewFinding] = [] + for block in _iter_blocks(blocks): + complexity = block["complexity"] + line = block["lineno"] + name = block["name"] + if not isinstance(complexity, int): + raise ValueError("radon complexity must be an integer") + if complexity <= 12: + continue + if not isinstance(line, int): + raise ValueError("radon line must be an integer") + if not isinstance(name, str): + raise ValueError("radon name must be a string") + severity = "warning" if complexity <= 15 else "error" + findings.append( + ReviewFinding( + category="clean_code", + severity=severity, + tool="radon", + rule=f"CC{complexity}", + file=filename, + line=line, + message=f"Cyclomatic complexity for {name} is {complexity}.", + fixable=False, + ) + ) + return findings + + +def _ensure_review_findings(result: list[ReviewFinding]) -> bool: + return all(isinstance(finding, ReviewFinding) for finding in result) + + +@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( + _ensure_review_findings, + "result must contain ReviewFinding instances", +) +def run_radon(files: list[Path]) -> list[ReviewFinding]: + """Run Radon for the provided files and map complexity findings into ReviewFinding records.""" + if not files: + return [] + + payload = _run_radon_command(files) + if payload is None: + return _tool_error(files[0], "Unable to execute Radon") + + allowed_paths = _allowed_paths(files) try: - for filename, blocks in payload.items(): - if not isinstance(filename, str): - raise ValueError("radon filename must be a string") - if _normalize_path_variants(filename).isdisjoint(allowed_paths): - continue - if not isinstance(blocks, list): - raise ValueError("radon file payload must be a list") - for block in _iter_blocks(blocks): - complexity = block["complexity"] - line = block["lineno"] - name = block["name"] - if not isinstance(complexity, int): - raise ValueError("radon complexity must be an integer") - if complexity <= 12: - continue - if not isinstance(line, int): - raise ValueError("radon line must be an integer") - if not isinstance(name, str): - raise ValueError("radon name must be a string") - severity = "warning" if complexity <= 15 else "error" - findings.append( - ReviewFinding( - category="clean_code", - severity=severity, - tool="radon", - rule=f"CC{complexity}", - file=filename, - line=line, - message=f"Cyclomatic complexity for {name} is {complexity}.", - fixable=False, - ) - ) - except (KeyError, TypeError, ValueError) as exc: - return _tool_error(files[0], f"Unable to parse Radon finding payload: {exc}") + findings = _map_radon_complexity_findings(payload, allowed_paths) + except ValueError as exc: + return _tool_error(files[0], str(exc)) + for file_path in files: + findings.extend(_kiss_metric_findings(file_path)) return findings diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py index 3d1af20..4db3932 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 @@ -21,10 +21,12 @@ "cross-layer-call": "architecture", "module-level-network": "architecture", "print-in-src": "architecture", + "banned-generic-public-names": "naming", + "swallowed-exception-pattern": "clean_code", } SEMGREP_TIMEOUT_SECONDS = 90 SEMGREP_RETRY_ATTEMPTS = 2 -SemgrepCategory = Literal["clean_code", "architecture"] +SemgrepCategory = Literal["clean_code", "architecture", "naming"] def _normalize_path_variants(path_value: str | Path) -> set[str]: @@ -112,13 +114,7 @@ def _load_semgrep_results(files: list[Path]) -> list[object]: for _attempt in range(SEMGREP_RETRY_ATTEMPTS): try: result = _run_semgrep_command(files) - payload = json.loads(result.stdout) - if not isinstance(payload, dict): - raise ValueError("semgrep output must be an object") - raw_results = payload.get("results", []) - if not isinstance(raw_results, list): - raise ValueError("semgrep results must be a list") - return raw_results + return _parse_semgrep_results(json.loads(result.stdout)) except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: last_error = exc if last_error is None: @@ -126,9 +122,18 @@ def _load_semgrep_results(files: list[Path]) -> list[object]: raise last_error +def _parse_semgrep_results(payload: dict[str, object]) -> list[object]: + if not isinstance(payload, dict): + raise ValueError("semgrep output must be an object") + raw_results = payload.get("results", []) + if not isinstance(raw_results, list): + raise ValueError("semgrep results must be a list") + return raw_results + + def _category_for_rule(rule: str) -> SemgrepCategory | None: category = SEMGREP_RULE_CATEGORY.get(rule) - if category in {"clean_code", "architecture"}: + if category in {"clean_code", "architecture", "naming"}: return cast(SemgrepCategory, category) return None @@ -196,12 +201,21 @@ def run_semgrep(files: list[Path]) -> list[ReviewFinding]: findings: list[ReviewFinding] = [] try: for item in raw_results: - if not isinstance(item, dict): - raise ValueError("semgrep finding must be an object") - finding = _finding_from_result(item, allowed_paths=allowed_paths) - if finding is not None: - findings.append(finding) + _append_semgrep_finding(findings, item, allowed_paths=allowed_paths) except (KeyError, TypeError, ValueError) as exc: return _tool_error(files[0], f"Unable to parse Semgrep finding payload: {exc}") return findings + + +def _append_semgrep_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_result(item, allowed_paths=allowed_paths) + if finding is not None: + findings.append(finding) diff --git a/skills/specfact-code-review/SKILL.md b/skills/specfact-code-review/SKILL.md index c90142c..6652019 100644 --- a/skills/specfact-code-review/SKILL.md +++ b/skills/specfact-code-review/SKILL.md @@ -4,29 +4,30 @@ description: House rules for AI coding sessions derived from review findings allowed-tools: [] --- -# House Rules - AI Coding Context (v3) +# House Rules - AI Coding Context (v4) -Updated: 2026-03-16 | Module: nold-ai/specfact-code-review +Updated: 2026-03-30 | Module: nold-ai/specfact-code-review ## DO - Ask whether tests should be included before repo-wide review; default to excluding tests unless test changes are the target -- Keep functions under 120 LOC and cyclomatic complexity <= 12 +- Use intention-revealing names; avoid placeholder public names like data/process/handle +- Keep functions under 120 LOC, shallow nesting, and <= 5 parameters (KISS) +- Delete unused private helpers and speculative abstractions quickly (YAGNI) +- Extract repeated function shapes once the second copy appears (DRY) +- Split persistence and transport concerns instead of mixing repository.* with http_client.* (SOLID) - Add @require/@ensure (icontract) + @beartype to all new public APIs - Run hatch run contract-test-contracts before any commit -- Guard all chained attribute access: a.b.c needs null-check or early return -- Return typed values from all public methods - Write the test file BEFORE the feature file (TDD-first) -- Use get_logger(__name__) from common.logger_setup, never print() +- Return typed values from all public methods and guard chained attribute access ## DON'T - Don't enable known noisy findings unless you explicitly want strict/full review output -- Don't mix read + write in the same method; split responsibilities - Don't use bare except: or except Exception: pass - Don't add # noqa / # type: ignore without inline justification -- Don't call repository.* and http_client.* in the same function +- Don't mix read + write in the same method or call repository.* and http_client.* together - Don't import at module level if it triggers network calls - Don't hardcode secrets; use env vars via pydantic.BaseSettings -- Don't create functions > 120 lines +- Don't create functions that exceed the KISS thresholds without a documented reason ## TOP VIOLATIONS (auto-updated by specfact code review rules update) diff --git a/tests/unit/specfact_code_review/rules/test_updater.py b/tests/unit/specfact_code_review/rules/test_updater.py index 233afac..d693eb2 100644 --- a/tests/unit/specfact_code_review/rules/test_updater.py +++ b/tests/unit/specfact_code_review/rules/test_updater.py @@ -39,26 +39,27 @@ def _skill_text( "- Ask whether tests should be included before repo-wide review; " "default to excluding tests unless test changes are the target" ), - "- Keep functions under 120 LOC and cyclomatic complexity <= 12", + "- Use intention-revealing names; avoid placeholder public names like data/process/handle", + "- Keep functions under 120 LOC, shallow nesting, and <= 5 parameters (KISS)", + "- Delete unused private helpers and speculative abstractions quickly (YAGNI)", + "- Extract repeated function shapes once the second copy appears (DRY)", + "- Split persistence and transport concerns instead of mixing repository.* with http_client.* (SOLID)", "- Add @require/@ensure (icontract) + @beartype to all new public APIs", "- Run hatch run contract-test-contracts before any commit", - "- Guard all chained attribute access: a.b.c needs null-check or early return", - "- Return typed values from all public methods", "- Write the test file BEFORE the feature file (TDD-first)", - "- Use get_logger(__name__) from common.logger_setup, never print()", + "- Return typed values from all public methods and guard chained attribute access", ] if extra_do_rules: do_rules.extend(extra_do_rules) dont_rules = [ "- Don't enable known noisy findings unless you explicitly want strict/full review output", - "- Don't mix read + write in the same method; split responsibilities", "- Don't use bare except: or except Exception: pass", "- Don't add # noqa / # type: ignore without inline justification", - "- Don't call repository.* and http_client.* in the same function", + "- Don't mix read + write in the same method or call repository.* and http_client.* together", "- Don't import at module level if it triggers network calls", "- Don't hardcode secrets; use env vars via pydantic.BaseSettings", - "- Don't create functions > 120 lines", + "- Don't create functions that exceed the KISS thresholds without a documented reason", ] lines = [ diff --git a/tests/unit/specfact_code_review/run/test_findings.py b/tests/unit/specfact_code_review/run/test_findings.py index 2c32083..9cf577d 100644 --- a/tests/unit/specfact_code_review/run/test_findings.py +++ b/tests/unit/specfact_code_review/run/test_findings.py @@ -19,6 +19,11 @@ class ReviewFindingPayload(TypedDict, total=False): "style", "architecture", "tool_error", + "naming", + "kiss", + "yagni", + "dry", + "solid", ] severity: Literal["error", "warning", "info"] tool: str @@ -62,7 +67,21 @@ def test_review_finding_accepts_supported_severity_values( @pytest.mark.parametrize( "category", - ["clean_code", "security", "type_safety", "contracts", "testing", "style", "architecture", "tool_error"], + [ + "clean_code", + "security", + "type_safety", + "contracts", + "testing", + "style", + "architecture", + "tool_error", + "naming", + "kiss", + "yagni", + "dry", + "solid", + ], ) def test_review_finding_accepts_supported_category_values(category: str) -> None: typed_category = cast( @@ -75,6 +94,11 @@ def test_review_finding_accepts_supported_category_values(category: str) -> None "style", "architecture", "tool_error", + "naming", + "kiss", + "yagni", + "dry", + "solid", ], category, ) diff --git a/tests/unit/specfact_code_review/run/test_runner.py b/tests/unit/specfact_code_review/run/test_runner.py index cf2e98f..f98ab86 100644 --- a/tests/unit/specfact_code_review/run/test_runner.py +++ b/tests/unit/specfact_code_review/run/test_runner.py @@ -11,6 +11,7 @@ from specfact_code_review.run.findings import ReviewFinding, ReviewReport from specfact_code_review.run.runner import ( + _coverage_findings, _pytest_python_executable, _pytest_targets, _run_pytest_with_coverage, @@ -25,7 +26,19 @@ def _finding( rule: str, severity: Literal["error", "warning", "info"] = "warning", category: Literal[ - "clean_code", "security", "type_safety", "contracts", "testing", "style", "architecture", "tool_error" + "clean_code", + "security", + "type_safety", + "contracts", + "testing", + "style", + "architecture", + "tool_error", + "naming", + "kiss", + "yagni", + "dry", + "solid", ] = "style", ) -> ReviewFinding: return ReviewFinding( @@ -50,6 +63,7 @@ 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_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")) @@ -64,7 +78,7 @@ 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", "basedpyright", "pylint", "contracts", "testing"] + assert calls == ["ruff", "radon", "semgrep", "ast", "basedpyright", "pylint", "contracts", "testing"] def test_run_review_merges_findings_from_all_runners(monkeypatch: MonkeyPatch) -> None: @@ -76,6 +90,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_ast_clean_code", + lambda files: [_finding(tool="ast", rule="dry.duplicate-function-shape", category="dry")], + ) monkeypatch.setattr( "specfact_code_review.run.runner.run_basedpyright", lambda files: [_finding(tool="basedpyright", rule="reportArgumentType", category="type_safety")], @@ -102,6 +120,7 @@ def test_run_review_merges_findings_from_all_runners(monkeypatch: MonkeyPatch) - "ruff", "radon", "semgrep", + "ast", "basedpyright", "pylint", "contract_runner", @@ -122,6 +141,7 @@ 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_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: []) @@ -142,6 +162,7 @@ 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_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: []) @@ -192,6 +213,7 @@ 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_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]) @@ -228,6 +250,7 @@ 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_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]) @@ -242,6 +265,28 @@ def test_run_review_can_include_known_test_noise(monkeypatch: MonkeyPatch) -> No assert [finding.rule for finding in report.findings] == ["W0212", "MISSING_ICONTRACT"] +def test_run_review_emits_advisory_checklist_finding_in_pr_mode(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_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._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") + monkeypatch.setenv( + "SPECFACT_CODE_REVIEW_PR_BODY", "Adds new review runners without documenting the clean-code rationale." + ) + + report = run_review([Path("packages/specfact-code-review/src/specfact_code_review/run/scorer.py")], no_tests=True) + + assert [finding.rule for finding in report.findings] == ["clean-code.pr-checklist-missing-rationale"] + assert report.findings[0].severity == "info" + assert report.overall_verdict == "PASS" + + def test_run_review_suppresses_global_duplicate_code_noise_by_default(monkeypatch: MonkeyPatch) -> None: duplicate_code_finding = ReviewFinding( category="style", @@ -256,6 +301,7 @@ 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_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: []) @@ -309,6 +355,7 @@ 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_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: []) @@ -396,6 +443,15 @@ def _fake_run(command: list[str], **_: object) -> subprocess.CompletedProcess[st assert findings == [] +def test_coverage_findings_skips_package_initializers_without_coverage_data() -> None: + source_file = Path("packages/specfact-code-review/src/specfact_code_review/tools/__init__.py") + + findings, coverage_by_source = _coverage_findings([source_file], {"files": {}}) + + assert findings == [] + assert coverage_by_source == {} + + def test_run_pytest_with_coverage_disables_global_fail_under(monkeypatch: MonkeyPatch) -> None: recorded: dict[str, object] = {} diff --git a/tests/unit/specfact_code_review/tools/test___init__.py b/tests/unit/specfact_code_review/tools/test___init__.py new file mode 100644 index 0000000..457e8cc --- /dev/null +++ b/tests/unit/specfact_code_review/tools/test___init__.py @@ -0,0 +1,15 @@ +"""Tool export smoke tests.""" + +# pylint: disable=import-outside-toplevel + + +def test_tools_exports_run_functions() -> None: + from specfact_code_review import tools as tools_module + + run_ast_clean_code = tools_module.run_ast_clean_code + run_radon = tools_module.run_radon + run_semgrep = tools_module.run_semgrep + + assert callable(run_ast_clean_code) + assert callable(run_radon) + assert callable(run_semgrep) diff --git a/tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py b/tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py new file mode 100644 index 0000000..83682ac --- /dev/null +++ b/tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py @@ -0,0 +1,81 @@ +from __future__ import annotations + +from pathlib import Path + +from specfact_code_review.tools.ast_clean_code_runner import run_ast_clean_code + + +def test_run_ast_clean_code_reports_unused_private_helper(tmp_path: Path) -> None: + file_path = tmp_path / "target.py" + file_path.write_text( + """ +def _unused_helper(value: int) -> int: + return value + 1 + + +def public_api(value: int) -> int: + return value * 2 +""".strip() + + "\n", + encoding="utf-8", + ) + + findings = run_ast_clean_code([file_path]) + + assert any(finding.category == "yagni" and finding.rule == "yagni.unused-private-helper" for finding in findings) + + +def test_run_ast_clean_code_reports_duplicate_function_shapes(tmp_path: Path) -> None: + file_path = tmp_path / "target.py" + file_path.write_text( + """ +def first(items: list[int]) -> list[int]: + cleaned: list[int] = [] + for item in items: + if item > 0: + cleaned.append(item * 2) + return cleaned + + +def second(values: list[int]) -> list[int]: + doubled: list[int] = [] + for value in values: + if value > 0: + doubled.append(value * 2) + return doubled +""".strip() + + "\n", + encoding="utf-8", + ) + + findings = run_ast_clean_code([file_path]) + + assert any(finding.category == "dry" and finding.rule == "dry.duplicate-function-shape" for finding in findings) + + +def test_run_ast_clean_code_reports_mixed_dependency_roles(tmp_path: Path) -> None: + file_path = tmp_path / "target.py" + file_path.write_text( + """ +def sync_customer(customer_id: str) -> None: + repository.load(customer_id) + http_client.post("/customers/sync", json={"customer_id": customer_id}) +""".strip() + + "\n", + encoding="utf-8", + ) + + findings = run_ast_clean_code([file_path]) + + assert any(finding.category == "solid" and finding.rule == "solid.mixed-dependency-role" for finding in findings) + + +def test_run_ast_clean_code_returns_tool_error_for_syntax_error(tmp_path: Path) -> None: + file_path = tmp_path / "broken.py" + file_path.write_text("def broken(:\n pass\n", encoding="utf-8") + + findings = run_ast_clean_code([file_path]) + + assert len(findings) == 1 + assert findings[0].category == "tool_error" + assert findings[0].tool == "ast" 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 9e7e75a..6d9796d 100644 --- a/tests/unit/specfact_code_review/tools/test_radon_runner.py +++ b/tests/unit/specfact_code_review/tools/test_radon_runner.py @@ -63,3 +63,34 @@ def test_run_radon_returns_tool_error_on_parse_error(tmp_path: Path, monkeypatch assert len(findings) == 1 assert findings[0].category == "tool_error" assert findings[0].tool == "radon" + + +def test_run_radon_emits_kiss_metrics_from_source_shape(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "target.py" + body = "\n".join(f" total += {index}" for index in range(81)) + file_path.write_text( + ( + "def noisy(a, b, c, d, e, f):\n" + " total = 0\n" + " if a:\n" + " if b:\n" + " if c:\n" + f"{body}\n" + " return total\n" + ), + 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 {finding.rule for finding in findings} >= { + "kiss.loc.warning", + "kiss.nesting.warning", + "kiss.parameter-count.warning", + } + assert {finding.category for finding in findings} == {"kiss"} 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 70f0de1..cf7ec25 100644 --- a/tests/unit/specfact_code_review/tools/test_semgrep_runner.py +++ b/tests/unit/specfact_code_review/tools/test_semgrep_runner.py @@ -56,6 +56,31 @@ def test_run_semgrep_maps_findings_to_review_finding(tmp_path: Path, monkeypatch run_mock.assert_called_once() +def test_run_semgrep_maps_naming_rule_to_naming_category(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "target.py" + payload = { + "results": [ + { + "check_id": "banned-generic-public-names", + "path": str(file_path), + "start": {"line": 2}, + "extra": {"message": "Public API name is too generic."}, + } + ] + } + monkeypatch.setattr( + subprocess, + "run", + Mock(return_value=completed_process("semgrep", stdout=json.dumps(payload), returncode=1)), + ) + + findings = run_semgrep([file_path]) + + assert len(findings) == 1 + assert findings[0].category == "naming" + assert findings[0].rule == "banned-generic-public-names" + + def test_run_semgrep_filters_findings_to_requested_files(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" other_path = tmp_path / "other.py" diff --git a/tests/unit/test_bundle_resource_payloads.py b/tests/unit/test_bundle_resource_payloads.py index 22c99c5..a42a339 100644 --- a/tests/unit/test_bundle_resource_payloads.py +++ b/tests/unit/test_bundle_resource_payloads.py @@ -128,6 +128,40 @@ def test_module_package_layout_matches_init_ide_resource_contract() -> None: assert (codebase / "resources" / "prompts" / "specfact.01-import.md").is_file() +def test_code_review_bundle_packages_clean_code_policy_pack_manifest() -> None: + module_root = REPO_ROOT / "packages" / "specfact-code-review" + roots = ( + module_root / "resources" / "policy-packs" / "specfact" / "clean-code-principles.yaml", + module_root + / "src" + / "specfact_code_review" + / "resources" + / "policy-packs" + / "specfact" + / "clean-code-principles.yaml", + ) + expected_rules = { + "banned-generic-public-names", + "swallowed-exception-pattern", + "kiss.loc.warning", + "kiss.loc.error", + "kiss.nesting.warning", + "kiss.nesting.error", + "kiss.parameter-count.warning", + "kiss.parameter-count.error", + "yagni.unused-private-helper", + "dry.duplicate-function-shape", + "solid.mixed-dependency-role", + "clean-code.pr-checklist-missing-rationale", + } + + for manifest_path in roots: + data = yaml.safe_load(manifest_path.read_text(encoding="utf-8")) + assert data["pack_ref"] == "specfact/clean-code-principles" + assert data["default_mode"] == "advisory" + assert {rule["id"] for rule in data["rules"]} == expected_rules + + def test_backlog_artifact_contains_prompt_payload(tmp_path: Path) -> None: artifact = _build_bundle_artifact("specfact-backlog", tmp_path) with tarfile.open(artifact, "r:gz") as archive: @@ -141,6 +175,12 @@ def test_backlog_artifact_contains_prompt_payload(tmp_path: Path) -> None: assert names == expected +def test_code_review_artifact_contains_policy_pack_payload(tmp_path: Path) -> None: + artifact = _build_bundle_artifact("specfact-code-review", tmp_path) + with tarfile.open(artifact, "r:gz") as archive: + assert "specfact-code-review/resources/policy-packs/specfact/clean-code-principles.yaml" in archive.getnames() + + def test_core_prompt_discovery_finds_installed_backlog_bundle(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: modules_root = tmp_path / "modules" installed_bundle = modules_root / "specfact-backlog" From 488f39f47ee180a2f6c8a67b496ecab61b1fe3f8 Mon Sep 17 00:00:00 2001 From: Dominikus Nold Date: Tue, 31 Mar 2026 01:16:21 +0200 Subject: [PATCH 2/2] Fix clean-code review findings --- docs/modules/code-review.md | 6 +- .../TDD_EVIDENCE.md | 58 +++++++++++-------- .../specfact-code-review/module-package.yaml | 6 +- .../skills/specfact-code-review/SKILL.md | 4 +- .../src/specfact_code_review/run/runner.py | 3 +- .../tools/ast_clean_code_runner.py | 9 ++- .../tools/radon_runner.py | 23 ++++---- .../tools/semgrep_runner.py | 4 +- .../specfact_code_review/run/test_runner.py | 19 ++++++ .../tools/test_ast_clean_code_runner.py | 37 ++++++++++++ .../tools/test_radon_runner.py | 28 +++++++++ .../tools/test_semgrep_runner.py | 15 +++++ 12 files changed, 167 insertions(+), 45 deletions(-) diff --git a/docs/modules/code-review.md b/docs/modules/code-review.md index ecc0bc0..d5c188f 100644 --- a/docs/modules/code-review.md +++ b/docs/modules/code-review.md @@ -225,6 +225,8 @@ Custom rule mapping: | Semgrep rule | ReviewFinding category | | --- | --- | +| `banned-generic-public-names` | `naming` | +| `swallowed-exception-pattern` | `clean_code` | | `get-modify-same-method` | `clean_code` | | `unguarded-nested-access` | `clean_code` | | `cross-layer-call` | `architecture` | @@ -234,6 +236,8 @@ Custom rule mapping: Representative anti-patterns covered by the ruleset: - methods that both read state and mutate it +- public symbols that use banned generic names like `data` or `process` +- swallowed exceptions that hide an underlying failure path - direct nested attribute access like `obj.config.value` - repository and HTTP client calls in the same function - module-level network client instantiation @@ -243,7 +247,7 @@ Additional behavior: - only the provided file list is considered - semgrep rule IDs emitted with path prefixes are normalized back to the governed rule IDs above -- malformed output or a missing Semgrep executable yields a single `tool_error` finding +- malformed output, a missing `results` list, or a missing Semgrep executable yields a single `tool_error` finding ### Contract runner diff --git a/openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md b/openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md index 108a864..7641008 100644 --- a/openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md +++ b/openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md @@ -1,30 +1,38 @@ # TDD Evidence -## 2026-03-30 +## 2026-03-31 -- `2026-03-30T22:57:17+02:00` Red phase: - `hatch run pytest tests/unit/specfact_code_review/run/test_findings.py tests/unit/specfact_code_review/run/test_runner.py tests/unit/specfact_code_review/tools/test_semgrep_runner.py tests/unit/specfact_code_review/tools/test_radon_runner.py tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py -q` - failed during collection before local `dev-deps` bootstrap because `specfact-cli` - runtime dependencies such as `beartype` were not yet available in the Hatch env. -- `2026-03-30T23:00:00+02:00` Bootstrap: +- `2026-03-31T00:56:00+02:00` Bootstrap: `hatch run dev-deps` - installed the local `specfact-cli` dependency set required by the bundle review tests. -- `2026-03-30T23:10:00+02:00` Green targeted runner slice: - `hatch run pytest tests/unit/specfact_code_review/run/test_findings.py tests/unit/specfact_code_review/run/test_runner.py tests/unit/specfact_code_review/tools/test_semgrep_runner.py tests/unit/specfact_code_review/tools/test_radon_runner.py tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py -q` - passed after the runner, AST, and test-fixture fixes. -- `2026-03-30T23:57:11+02:00` Green review run: - `SPECFACT_ALLOW_UNSIGNED=1 hatch run specfact code review run --json --out .specfact/code-review.json` - passed with `findings: []` after linking the live dev module and flattening the KISS-sensitive helpers. -- `2026-03-30T23:56:00+02:00` Green full targeted slice: - `hatch run pytest --cov=packages/specfact-code-review/src/specfact_code_review --cov-fail-under=0 --cov-report=json:/tmp/specfact-report.json tests/unit/specfact_code_review/rules/test_updater.py tests/unit/specfact_code_review/run/test_findings.py tests/unit/specfact_code_review/run/test_runner.py tests/unit/specfact_code_review/tools/test___init__.py tests/unit/specfact_code_review/tools/test_radon_runner.py tests/unit/specfact_code_review/tools/test_semgrep_runner.py tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py` - passed in `89 passed in 20.75s`. -- `2026-03-30T23:58:00+02:00` Green quality gates: - `hatch run format`, `hatch run type-check`, `hatch run lint`, `hatch run yaml-lint`, - `hatch run contract-test`, `hatch run smart-test`, and `hatch run test` - all passed in this worktree after the final helper flattening. -- `2026-03-30T23:58:00+02:00` Validation: - `openspec validate clean-code-02-expanded-review-module --strict` - passed with `Change 'clean-code-02-expanded-review-module' is valid`. -- `2026-03-30T23:58:00+02:00` Remaining release blocker: + linked the local `specfact-cli` checkout into this worktree so the bundle tests and review runner could execute against the live code. +- `2026-03-31T01:02:00+02:00` Red phase: + `SPECFACT_ALLOW_UNSIGNED=1 hatch run pytest tests/unit/specfact_code_review/run/test_runner.py tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py tests/unit/specfact_code_review/tools/test_radon_runner.py tests/unit/specfact_code_review/tools/test_semgrep_runner.py -q` + failed with 5 targeted regressions that matched the new spec change: + - `test_run_review_requires_explicit_pr_mode_token_for_clean_code_reasoning` + expected `clean-code.pr-checklist-missing-rationale` but `_checklist_findings()` returned `[]`. + - `test_run_ast_clean_code_reports_mixed_dependency_roles_for_injected_dependencies` + expected `solid.mixed-dependency-role` for `self.repo.save()` / `self.client.get()` but the leftmost dependency was still treated as `self`. + - `test_run_ast_clean_code_continues_after_parse_error` + expected a per-file `tool_error` plus later-file findings, but the parser branch returned early. + - `test_run_radon_uses_dedicated_tool_identifier_for_kiss_findings` + expected `tool="radon-kiss"` but the emitted finding still used `tool="radon"`. + - `test_run_semgrep_returns_tool_error_when_results_key_is_missing` + expected a `tool_error` for malformed Semgrep JSON, but the runner treated a missing `results` key as a clean run. +- `2026-03-31T01:04:30+02:00` Implementation: + updated `packages/specfact-code-review/src/specfact_code_review/run/runner.py`, + `packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py`, + `packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py`, + `packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py`, + `packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md`, + `packages/specfact-code-review/src/specfact_code_review/resources/policy-packs/specfact/clean-code-principles.yaml`, + `docs/modules/code-review.md`, + and the targeted unit tests so the new clean-code checks, strict PR-mode gating, dependency-root detection, KISS tool labeling, and Semgrep parsing behavior matched the review comments. +- `2026-03-31T01:06:30+02:00` Green phase: + `SPECFACT_ALLOW_UNSIGNED=1 hatch run pytest tests/unit/specfact_code_review/run/test_runner.py tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py tests/unit/specfact_code_review/tools/test_radon_runner.py tests/unit/specfact_code_review/tools/test_semgrep_runner.py -q` + passed with `50 passed in 20.26s`. +- `2026-03-31T01:08:11+02:00` Release validation: `hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump` - failed with `packages/specfact-code-review/module-package.yaml: checksum mismatch`. + passed after the module was signed again. +- `2026-03-31T01:10:42+02:00` Review validation: + `SPECFACT_ALLOW_UNSIGNED=1 hatch run specfact code review run --json --out .specfact/code-review.json` + passed with `findings: []`. diff --git a/packages/specfact-code-review/module-package.yaml b/packages/specfact-code-review/module-package.yaml index 44e82dd..766d229 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.45.0 +version: 0.45.1 commands: - code tier: official @@ -22,5 +22,5 @@ description: Official SpecFact code review bundle package. category: codebase bundle_group_command: code integrity: - checksum: sha256:d678813d42c84799242282094744369cad8f54942d3cd78b35de3b1b4bcce520 - signature: tt9xLDRe6s8vBSh71rixZl8TC0nOtOuGJmQf32rt3i/Ar0eM6B1VWkZZeW2TPi/J0Fa4MtApemIb2LW1scNXBg== + checksum: sha256:db46665149d4931c3f99da03395a172810e4b9ef2cabd23d46e177a23983e7f4 + signature: RNvYgAPLfFtV6ywXvs/9umIAyewZPbEZD+homAIt1+n4IwDFhwneEwqzpK7RlfvCnT0Rb3Xefa5ZMW7GwiWXBw== diff --git a/packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md b/packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md index 4214e0e..b2f2341 100644 --- a/packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md +++ b/packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md @@ -14,7 +14,7 @@ Updated: 2026-03-30 | Module: nold-ai/specfact-code-review - Keep functions under 120 LOC, shallow nesting, and <= 5 parameters (KISS) - Delete unused private helpers and speculative abstractions quickly (YAGNI) - Extract repeated function shapes once the second copy appears (DRY) -- Split persistence and transport concerns instead of mixing repository.* with http_client.* (SOLID) +- Split persistence and transport concerns instead of mixing `repository.*` with `http_client.*` (SOLID) - Add @require/@ensure (icontract) + @beartype to all new public APIs - Run hatch run contract-test-contracts before any commit - Write the test file BEFORE the feature file (TDD-first) @@ -24,7 +24,7 @@ Updated: 2026-03-30 | Module: nold-ai/specfact-code-review - Don't enable known noisy findings unless you explicitly want strict/full review output - Don't use bare except: or except Exception: pass - Don't add # noqa / # type: ignore without inline justification -- Don't mix read + write in the same method or call repository.* and http_client.* together +- Don't mix read + write in the same method or call `repository.*` and `http_client.*` together - Don't import at module level if it triggers network calls - Don't hardcode secrets; use env vars via pydantic.BaseSettings - Don't create functions that exceed the KISS thresholds without a documented reason 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 c255786..ff03ed7 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 @@ -4,6 +4,7 @@ import json import os +import re import subprocess import sys import tempfile @@ -218,7 +219,7 @@ def _checklist_findings() -> list[ReviewFinding]: context = "\n".join( os.environ.get(name, "").strip() for name in _PR_CONTEXT_ENVS if os.environ.get(name, "").strip() ) - if any(hint in context.lower() for hint in _CLEAN_CODE_CONTEXT_HINTS): + if any(re.search(rf"\b{re.escape(hint)}\b", context, flags=re.IGNORECASE) for hint in _CLEAN_CODE_CONTEXT_HINTS): return [] return [ 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 de83d35..2d55773 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 @@ -75,9 +75,13 @@ def _loaded_names(tree: ast.AST) -> set[str]: def _leftmost_name(node: ast.AST) -> str | None: current = node + first_attribute: str | None = None while isinstance(current, ast.Attribute): + first_attribute = current.attr current = current.value if isinstance(current, ast.Name): + if current.id in {"self", "cls"} and first_attribute is not None: + return first_attribute return current.id return None @@ -188,7 +192,10 @@ def run_ast_clean_code(files: list[Path]) -> list[ReviewFinding]: try: tree = ast.parse(file_path.read_text(encoding="utf-8"), filename=str(file_path)) except (OSError, SyntaxError) as exc: - return [_tool_error(tool="ast", file_path=file_path, message=f"Unable to parse Python source: {exc}")] + findings.append( + _tool_error(tool="ast", file_path=file_path, message=f"Unable to parse Python source: {exc}") + ) + continue findings.extend(_solid_findings(file_path, tree)) findings.extend(_yagni_findings(file_path, tree)) 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 033d90e..2a38fe2 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 @@ -125,11 +125,11 @@ def _kiss_loc_findings(function_node: ast.FunctionDef | ast.AsyncFunctionDef, fi ReviewFinding( category="kiss", severity=severity, - tool="radon", + tool="radon-kiss", rule=f"kiss.loc.{suffix}", file=str(file_path), line=function_node.lineno, - message=(f"Function `{function_node.name}` spans {loc} lines; keep it under {{_KISS_LOC_WARNING}}."), + message=f"Function `{function_node.name}` spans {loc} lines; keep it under {_KISS_LOC_WARNING}.", fixable=False, ) ) @@ -149,7 +149,7 @@ def _kiss_nesting_findings( ReviewFinding( category="kiss", severity=severity, - tool="radon", + tool="radon-kiss", rule=f"kiss.nesting.{suffix}", file=str(file_path), line=function_node.lineno, @@ -182,7 +182,7 @@ def _kiss_parameter_findings( ReviewFinding( category="kiss", severity=severity, - tool="radon", + tool="radon-kiss", rule=f"kiss.parameter-count.{suffix}", file=str(file_path), line=function_node.lineno, @@ -274,14 +274,15 @@ def run_radon(files: list[Path]) -> list[ReviewFinding]: return [] payload = _run_radon_command(files) + findings: list[ReviewFinding] = [] if payload is None: - return _tool_error(files[0], "Unable to execute Radon") - - allowed_paths = _allowed_paths(files) - try: - findings = _map_radon_complexity_findings(payload, allowed_paths) - except ValueError as exc: - return _tool_error(files[0], str(exc)) + findings.extend(_tool_error(files[0], "Unable to execute Radon")) + else: + allowed_paths = _allowed_paths(files) + try: + findings.extend(_map_radon_complexity_findings(payload, allowed_paths)) + except ValueError as exc: + findings.extend(_tool_error(files[0], str(exc))) for file_path in files: findings.extend(_kiss_metric_findings(file_path)) diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py index 4db3932..0fc7f2a 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 @@ -125,7 +125,9 @@ def _load_semgrep_results(files: list[Path]) -> list[object]: def _parse_semgrep_results(payload: dict[str, object]) -> list[object]: if not isinstance(payload, dict): raise ValueError("semgrep output must be an object") - raw_results = payload.get("results", []) + if "results" not in payload: + raise ValueError("semgrep output missing results key") + raw_results = payload["results"] if not isinstance(raw_results, list): raise ValueError("semgrep results must be a list") return raw_results diff --git a/tests/unit/specfact_code_review/run/test_runner.py b/tests/unit/specfact_code_review/run/test_runner.py index f98ab86..80ccbba 100644 --- a/tests/unit/specfact_code_review/run/test_runner.py +++ b/tests/unit/specfact_code_review/run/test_runner.py @@ -287,6 +287,25 @@ def test_run_review_emits_advisory_checklist_finding_in_pr_mode(monkeypatch: Mon assert report.overall_verdict == "PASS" +def test_run_review_requires_explicit_pr_mode_token_for_clean_code_reasoning(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_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._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") + monkeypatch.setenv("SPECFACT_CODE_REVIEW_PR_BODY", "We are renaming helper functions for clarity.") + monkeypatch.setenv("SPECFACT_CODE_REVIEW_PR_PROPOSAL", "") + + report = run_review([Path("packages/specfact-code-review/src/specfact_code_review/run/scorer.py")], no_tests=True) + + assert [finding.rule for finding in report.findings] == ["clean-code.pr-checklist-missing-rationale"] + + def test_run_review_suppresses_global_duplicate_code_noise_by_default(monkeypatch: MonkeyPatch) -> None: duplicate_code_finding = ReviewFinding( category="style", diff --git a/tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py b/tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py index 83682ac..554b20f 100644 --- a/tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py +++ b/tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py @@ -70,6 +70,43 @@ def sync_customer(customer_id: str) -> None: assert any(finding.category == "solid" and finding.rule == "solid.mixed-dependency-role" for finding in findings) +def test_run_ast_clean_code_reports_mixed_dependency_roles_for_injected_dependencies(tmp_path: Path) -> None: + file_path = tmp_path / "target.py" + file_path.write_text( + """ +class SyncClient: + def sync(self) -> None: + self.repository.load() + self.http_client.post() +""".strip() + + "\n", + encoding="utf-8", + ) + + findings = run_ast_clean_code([file_path]) + + assert any(finding.category == "solid" and finding.rule == "solid.mixed-dependency-role" for finding in findings) + + +def test_run_ast_clean_code_continues_after_parse_error(tmp_path: Path) -> None: + broken_path = tmp_path / "broken.py" + broken_path.write_text("def broken(:\n pass\n", encoding="utf-8") + healthy_path = tmp_path / "healthy.py" + healthy_path.write_text( + """ +def _unused_helper(value: int) -> int: + return value + 1 +""".strip() + + "\n", + encoding="utf-8", + ) + + findings = run_ast_clean_code([broken_path, healthy_path]) + + assert any(finding.category == "tool_error" and finding.file == str(broken_path) for finding in findings) + assert any(finding.rule == "yagni.unused-private-helper" for finding in findings) + + def test_run_ast_clean_code_returns_tool_error_for_syntax_error(tmp_path: Path) -> None: file_path = tmp_path / "broken.py" file_path.write_text("def broken(:\n pass\n", encoding="utf-8") 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 6d9796d..1bfc57a 100644 --- a/tests/unit/specfact_code_review/tools/test_radon_runner.py +++ b/tests/unit/specfact_code_review/tools/test_radon_runner.py @@ -94,3 +94,31 @@ def test_run_radon_emits_kiss_metrics_from_source_shape(tmp_path: Path, monkeypa "kiss.parameter-count.warning", } assert {finding.category for finding in findings} == {"kiss"} + + +def test_run_radon_uses_dedicated_tool_identifier_for_kiss_findings(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "target.py" + body = "\n".join(f" total += {index}" for index in range(81)) + file_path.write_text( + ( + "def noisy(a, b, c, d, e, f):\n" + " total = 0\n" + " if a:\n" + " if b:\n" + " if c:\n" + f"{body}\n" + " return total\n" + ), + encoding="utf-8", + ) + monkeypatch.setattr( + subprocess, + "run", + Mock(return_value=completed_process("radon", stdout=json.dumps({str(file_path): []}))), + ) + + findings = run_radon([file_path]) + + 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"} 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 cf7ec25..9a0ef29 100644 --- a/tests/unit/specfact_code_review/tools/test_semgrep_runner.py +++ b/tests/unit/specfact_code_review/tools/test_semgrep_runner.py @@ -139,6 +139,21 @@ def test_run_semgrep_returns_empty_list_for_clean_file(tmp_path: Path, monkeypat assert not findings +def test_run_semgrep_returns_tool_error_when_results_key_is_missing(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "target.py" + monkeypatch.setattr( + subprocess, + "run", + Mock(return_value=completed_process("semgrep", stdout=json.dumps({"version": "1.0"}))), + ) + + findings = run_semgrep([file_path]) + + assert len(findings) == 1 + assert findings[0].category == "tool_error" + assert findings[0].tool == "semgrep" + + def test_run_semgrep_ignores_unsupported_rules(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" payload = {