Merge pull request #128 from nold-ai/feature/clean-code-02-expanded-review-module

[codex] expand code review clean-code checks

Author: djm81

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/<basename>/` 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
 

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
 

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
 
@@ -323,7 +327,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 +374,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

openspec/changes/clean-code-02-expanded-review-module/TDD_EVIDENCE.md

@@ -0,0 +1,38 @@
+# TDD Evidence
+
+## 2026-03-31
+
+- `2026-03-31T00:56:00+02:00` Bootstrap:
+  `hatch run dev-deps`
+  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`
+  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: []`.

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
 

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

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

@@ -1,5 +1,5 @@
 name: nold-ai/specfact-code-review
-version: 0.44.3
+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:eeef7d281055dceae470e317a37eb7c76087f12994b991d8bce86c6612746758
-  signature: BaV6fky8HlxFC5SZFgWAHLMAXf62MEQEp1S6wsgV+otMjkr5IyhCoQ8TJvx072klIAMh11N130Wzg4aexlcADA==
+  checksum: sha256:db46665149d4931c3f99da03395a172810e4b9ef2cabd23d46e177a23983e7f4
+  signature: RNvYgAPLfFtV6ywXvs/9umIAyewZPbEZD+homAIt1+n4IwDFhwneEwqzpK7RlfvCnT0Rb3Xefa5ZMW7GwiWXBw==

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

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

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)
 <!-- auto-managed: do not edit manually -->