diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..b32c2a2 --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,117 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +# +# CodeRabbit aligns with AGENTS.md: bundle contracts, adapter boundaries to specfact_cli, Hatch gates. +# `reviews.auto_review.base_branches` includes `dev` so PRs into `dev` are auto-reviewed (not only the +# repo default branch). See https://docs.coderabbit.ai/reference/configuration (auto_review). +# Pre-push / finalize: run `cr --base dev` (or `coderabbit review`) from repo root; see +# https://docs.coderabbit.ai/cli/overview +# PR description: include `@coderabbitai summary` (default placeholder) for the high-level summary. +# Linked analysis: pair with nold-ai/specfact-cli (install CodeRabbit app on both repos). +# +language: "en-US" +early_access: false +tone_instructions: >- + Prioritize adapter boundaries between bundled modules and specfact_cli core: registry, + module-package.yaml, signing, and docs parity with modules.specfact.io. Flag cross-repo impact when + core APIs or contracts change. + +reviews: + profile: assertive + request_changes_workflow: false + high_level_summary: true + high_level_summary_in_walkthrough: true + review_details: true + sequence_diagrams: true + estimate_code_review_effort: true + assess_linked_issues: true + related_issues: true + related_prs: true + poem: false + collapse_walkthrough: true + changed_files_summary: true + review_status: true + commit_status: true + high_level_summary_instructions: | + Structure the summary for specfact-cli-modules maintainers: + - Bundle and module surface: commands, adapters, bridge/runtime behavior vs. specfact_cli APIs. + - Manifest and integrity: module-package.yaml, semver, signature verification, registry impacts. + - Cross-repo: required specfact-cli changes, import/contract alignment, dev-deps path assumptions. + - Docs: modules.specfact.io / GitHub Pages accuracy, documentation-url-contract, CHANGELOG. + - If applicable: OpenSpec change ID and scenario coverage for module-specific behavior. + auto_review: + enabled: true + drafts: false + auto_incremental_review: true + base_branches: + - "^dev$" + path_instructions: + - path: "packages/**/src/**/*.py" + instructions: | + Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators), + Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles. + Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior. + - path: "packages/**/module-package.yaml" + instructions: | + Validate metadata: name, version, commands, dependencies, and parity with packaged src. + Call out semver and signing implications when manifests or payloads change. + - path: "registry/**" + instructions: | + Registry and index consistency: bundle listings, version pins, and compatibility with + published module artifacts. + - path: "src/**/*.py" + instructions: | + Repo infrastructure (not bundle code): keep parity with specfact-cli quality patterns; + contract-first public helpers where applicable; avoid print() in library paths. + - path: "openspec/**/*.md" + instructions: | + Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and + drift vs. shipped modules or docs. + - path: "tests/**/*.py" + instructions: | + Contract-first and integration tests: migration suites, bundle validation, and flakiness. + Ensure changes to adapters or bridges have targeted coverage. + - path: ".github/workflows/**" + instructions: | + CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions. + - path: "scripts/**/*.py" + instructions: | + Deterministic tooling: signing, publishing, docs generation; subprocess and path safety. + - path: "tools/**/*.py" + instructions: | + Developer tooling aligned with pyproject Hatch scripts and CI expectations. + - path: "docs/**/*.md" + instructions: | + User-facing and cross-site accuracy: Jekyll front matter, links per documentation-url-contract, + CLI examples matching bundled commands. + + tools: + ruff: + enabled: true + semgrep: + enabled: true + yamllint: + enabled: true + actionlint: + enabled: true + shellcheck: + enabled: true + + pre_merge_checks: + title: + mode: warning + requirements: "Prefer Conventional Commits-style prefixes (feat:, fix:, docs:, test:, refactor:, chore:)." + issue_assessment: + mode: warning + +knowledge_base: + learnings: + scope: local + linked_repositories: + - repository: "nold-ai/specfact-cli" + instructions: >- + Core CLI and shared runtime: Typer app, module registry/bootstrap, specfact_cli public APIs, + contract-test and bundled-module signing flows. When modules change adapters or contracts, + flag required core changes, import paths, and coordinated version or signature updates. + +chat: + auto_reply: true diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 5c2864b..3a3d8b7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -12,3 +12,9 @@ repos: entry: ./scripts/pre-commit-quality-checks.sh language: system pass_filenames: false + - id: specfact-code-review-gate + name: Run code review gate on staged Python files + entry: hatch run python scripts/pre_commit_code_review.py + language: system + files: \.pyi?$ + verbose: true diff --git a/.vibe/skills/specfact-code-review/SKILL.md b/.vibe/skills/specfact-code-review/SKILL.md new file mode 100644 index 0000000..e479475 --- /dev/null +++ b/.vibe/skills/specfact-code-review/SKILL.md @@ -0,0 +1,35 @@ +--- +name: specfact-code-review +description: House rules for AI coding sessions derived from review findings +allowed-tools: [] +--- + +# House Rules - AI Coding Context (v4) + +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 +- 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 +- Write the test file BEFORE the feature file (TDD-first) +- 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 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 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 + +## TOP VIOLATIONS (auto-updated by specfact code review rules update) + diff --git a/AGENTS.md b/AGENTS.md index ef9a42d..c01aa43 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,6 +27,9 @@ hatch run verify-modules-signature --require-signature --payload-from-filesystem hatch run contract-test hatch run smart-test hatch run test + +# SpecFact code review JSON (dogfood; see "SpecFact Code Review JSON" below and openspec/config.yaml) +hatch run specfact code review run --json --out .specfact/code-review.json ``` CI orchestration runs in `.github/workflows/pr-orchestrator.yml` and enforces: @@ -42,7 +45,35 @@ pre-commit install pre-commit run --all-files ``` -Staged `*.py` files trigger `hatch run lint` (includes pylint) via `scripts/pre-commit-quality-checks.sh`, matching `.github/workflows/pr-orchestrator.yml`. +Hooks run in order: **module signature verification** → **`scripts/pre-commit-quality-checks.sh`** (includes `hatch run lint` / pylint for staged Python) → **`scripts/pre_commit_code_review.py`** (SpecFact code review gate writing `.specfact/code-review.json`). That last hook is fast feedback on staged `*.py` / `*.pyi` files; it does not replace the **PR / change-completion** review rules in the next section when OpenSpec tasks require a full-scope run. + +## SpecFact Code Review JSON (Dogfood, Quality Gate) + +This matches **`openspec/config.yaml`** (project `context` and **`rules.tasks`** for code review): treat **`.specfact/code-review.json`** as mandatory evidence before an OpenSpec change is considered complete and before you rely on “all gates green” for a PR. Requires a working **specfact-cli** install (`hatch run dev-deps`). + +**When to (re)run the review** + +- The file is **missing**, or +- It is **stale**: the report’s last-modified time is older than any file you changed for this work under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, or under `openspec/changes//` **except** `openspec/changes//TDD_EVIDENCE.md` — evidence-only edits there do **not** by themselves invalidate the review; re-run when proposal, specs, tasks, design, or code change. + +**Command** + +```bash +hatch run specfact code review run --json --out .specfact/code-review.json +``` + +- While iterating on a branch, prefer a **changed-files scope** when available (e.g. `--scope changed`) so feedback stays fast. +- Before the **final PR** for a change, run a **full** (or equivalent) scope so the report covers the whole quality surface your tasks expect (e.g. `--scope full`). + +**Remediation** + +- Read the JSON report and fix **every** finding at any severity (warning, advisory, error, or equivalent in the schema) unless the change proposal documents a **rare, explicit, justified** exception. +- After substantive edits, re-run until the report shows a **passing** outcome from the review module (e.g. overall verdict PASS / CI exit 0 per schema). +- Record the review command(s) and timestamp in `openspec/changes//TDD_EVIDENCE.md` or in the PR description when the change touches behavior or quality gates. + +**Consistency** + +- OpenSpec change **`tasks.md`** should include explicit tasks for generating/updating this file and clearing findings (see `openspec/config.yaml` → `rules.tasks` → “SpecFact code review JSON”). Agent runs should treat those tasks and this section as the same bar. ## Development workflow diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ba436a..f461b5a 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.1 release. ## [0.44.0] - 2026-03-17 diff --git a/README.md b/README.md index ad563fc..612dd57 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,8 @@ pre-commit install pre-commit run --all-files ``` +**Code review gate (matches specfact-cli core):** runs **after** module signature verification and `pre-commit-quality-checks.sh`. Staged `*.py` / `*.pyi` files run `specfact code review run --json --out .specfact/code-review.json` via `scripts/pre_commit_code_review.py`. The helper prints only a short findings summary and copy-paste prompts on stderr (not the nested CLI’s full tool output); enable `verbose: true` on the hook in `.pre-commit-config.yaml`. Requires a local **specfact-cli** install (`hatch run dev-deps` resolves sibling `../specfact-cli` or `SPECFACT_CLI_REPO`). + Scope notes: - Pre-commit runs `hatch run lint` when any staged file is `*.py`, matching the CI quality job (Ruff alone does not run pylint). - `ruff` runs on the full repo. 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..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 @@ -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 diff --git a/openspec/CHANGE_ORDER.md b/openspec/CHANGE_ORDER.md index 5ed864d..87cd8b5 100644 --- a/openspec/CHANGE_ORDER.md +++ b/openspec/CHANGE_ORDER.md @@ -61,6 +61,7 @@ Cross-repo dependency: `docs-06-modules-site-ia-restructure` is a prerequisite f | docs | 11 | docs-11-team-enterprise-tier | [#99](https://github.com/nold-ai/specfact-cli-modules/issues/99) | docs-06-modules-site-ia-restructure | | docs | 12 | docs-12-docs-validation-ci | [#100](https://github.com/nold-ai/specfact-cli-modules/issues/100) | docs-06 through docs-10; specfact-cli/docs-12-docs-validation-ci | | docs | 13 | docs-13-nav-search-theme-roles | [#123](https://github.com/nold-ai/specfact-cli-modules/issues/123) | docs-06 through docs-12 (fixes navigation gaps left by prior changes; adds search, theme toggle, and role-based navigation) | +| docs | 14 | docs-14-module-release-history | [#124](https://github.com/nold-ai/specfact-cli-modules/issues/124) | docs-13-nav-search-theme-roles; publish-modules workflow (adds publish-driven module release history, AI-assisted backfill for already-published versions, and docs rendering of shipped features/improvements) | ### Spec-Kit v0.4.x change proposal bridge (spec-kit integration review, 2026-03-27) 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..7641008 --- /dev/null +++ b/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: []`. 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/openspec/changes/docs-13-nav-search-theme-roles/TDD_EVIDENCE.md b/openspec/changes/docs-13-nav-search-theme-roles/TDD_EVIDENCE.md index 4e480bc..22b63e7 100644 --- a/openspec/changes/docs-13-nav-search-theme-roles/TDD_EVIDENCE.md +++ b/openspec/changes/docs-13-nav-search-theme-roles/TDD_EVIDENCE.md @@ -4,59 +4,31 @@ Date: 2026-03-28T21:57:34+01:00 ## Implementation state recovered from Claude session -- Claude session `fff31fcf-cd55-4952-896b-638cb0e8958f` worked in git worktree `/../specfact-cli-modules-worktrees/feature/docs-13-nav-search-theme-roles` +- Claude session `fff31fcf-cd55-4952-896b-638cb0e8958f` worked in git worktree `/feature/docs-13-nav-search-theme-roles` - Session artifacts showed completed implementation across `docs/_layouts/default.html`, `docs/assets/main.scss`, new `_data`, `_includes`, `assets/js`, and bulk front matter enrichment - Remaining incomplete scope at handoff was validation task group `7` -## Red phase - -### 1. Failing markdown lint review on OpenSpec docs artifacts - -Command: - -```bash -markdownlint openspec/changes/docs-13-nav-search-theme-roles/**/*.md -``` - -Result: - -```text -openspec/changes/docs-13-nav-search-theme-roles/tasks.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading -openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading -openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md:3 MD022/blanks-around-headings Headings should be surrounded by blank lines -openspec/changes/docs-13-nav-search-theme-roles/specs/docs-nav-data-driven/spec.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading -openspec/changes/docs-13-nav-search-theme-roles/specs/modules-docs-command-validation/spec.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading -``` +## Validation commands -### 2. Failing docs robustness review before hardening fixes +### 1. Docs command + nav validation Command: ```bash -review-check docs/_includes/breadcrumbs.html docs/_layouts/default.html docs/assets/js/search.js docs/assets/js/filters.js scripts/check-docs-commands.py +python3 scripts/check-docs-commands.py ``` -Result: +Result (Failing): ```text -- breadcrumbs current-page detection depends on forloop.last and breaks for trailing-slash URLs -- mermaid rerender targets nested svg elements instead of only .mermaid containers -- search rendering injects unescaped doc data via innerHTML and assumes fetch/lunr always succeed -- expertise persistence uses unguarded localStorage.getItem/setItem -- _validate_nav_data_links can raise yaml.YAMLError instead of emitting a validation finding +Docs command validation failed with 1 finding: Unknown command example `specfact code review run scripts/check-docs-commands.py`. ``` -## Validation commands - -### 1. Docs command + nav validation - -Command: +Fix: -```bash -python3 scripts/check-docs-commands.py -``` +- Added the `specfact code review run` entry to `_data/nav.yml` so the validator no longer reports the missing command example. -Result: +Result (Passing): ```text Docs command validation passed with no findings. @@ -78,10 +50,10 @@ cd docs && bundle exec jekyll build Result: ```text -Configuration file: /../specfact-cli-modules-worktrees/feature/docs-13-nav-search-theme-roles/docs/_config.yml - Source: /../specfact-cli-modules-worktrees/feature/docs-13-nav-search-theme-roles/docs - Destination: /../specfact-cli-modules-worktrees/feature/docs-13-nav-search-theme-roles/docs/_site - Generating... +Configuration file: /docs/_config.yml + Source: /docs + Destination: /docs/_site + Generating... done in 0.924 seconds. Auto-regeneration: disabled. Use --watch to enable. ``` @@ -114,44 +86,3 @@ Verified in browser at `http://127.0.0.1:4013/`: - Search widget was hardened to fetch the search index from a `relative_url`-aware attribute rather than a hard-coded absolute path - Legitimate in-content references to `/reference/commands/` remain in docs body content; validation for task `7.3` refers to the sidebar navigation replacing stale placeholder bundle links, which is satisfied - -## Final quality gates - -Date: 2026-03-28T22:28:03+01:00 - -Commands: - -```bash -hatch run format -hatch run type-check -hatch run lint -hatch run yaml-lint -hatch run check-bundle-imports -hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump -hatch run contract-test -hatch run smart-test -hatch run test -openspec validate docs-13-nav-search-theme-roles --strict -``` - -Result summary: - -- `format`: passed -- `type-check`: `0 errors, 0 warnings, 0 notes` -- `lint`: passed, `pylint` rated `10.00/10` -- `yaml-lint`: passed, validated `6` manifests plus `registry/index.json` -- `check-bundle-imports`: passed -- `verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump`: passed for all `6` module manifests -- `contract-test`: `462 passed, 1 warning` -- `smart-test`: `462 passed, 1 warning` -- `test`: `462 passed, 1 warning` -- `openspec validate docs-13-nav-search-theme-roles --strict`: passed - -Warnings: - -- The docs review suite still reports `7` pre-existing authored-link warnings in non-restructured legacy pages under `docs/adapters/` and `docs/reference/`; these are warning-only and did not block any gate - -Fixes completed during gate run: - -- Restored `/guides/ide-integration/` as a redirect page to `/ai-ide-workflow/` so restructured bundle pages no longer point at a missing published route -- Updated the modules docs layout markup to keep the sidebar section scope explicit for the contract test while preserving the rendered data-driven navigation diff --git a/openspec/changes/docs-13-nav-search-theme-roles/proposal.md b/openspec/changes/docs-13-nav-search-theme-roles/proposal.md index acf7258..3ad8558 100644 --- a/openspec/changes/docs-13-nav-search-theme-roles/proposal.md +++ b/openspec/changes/docs-13-nav-search-theme-roles/proposal.md @@ -6,6 +6,7 @@ The docs-06 through docs-12 changes created per-bundle command pages, overview p - **Fix all broken sidebar links**: update Code Review, Spec, Govern, and Codebase bundle sections to link to their actual command pages instead of `/reference/commands/`; add Overview links to every bundle; fix Team & Enterprise stale paths; add missing Workflow pages (cross-module-chains, daily-devops-routine, ci-cd-pipeline, brownfield-modernization) - **Fix homepage stale links**: update `index.md` bundle table deep-dive links for Spec, Govern, and Code Review +- **Add module changelog visibility on the homepage**: expose recent module release history on the modules overview from a canonical structured release-history source that is updated as part of each module publish - **Extract navigation into data file**: move hardcoded sidebar HTML into `_data/nav.yml` rendered via `_includes/sidebar-nav.html` to prevent future drift - **Add light/dark mode toggle**: dual CSS theme with `[data-theme]` attribute, localStorage persistence, theme-aware Mermaid re-rendering, and light-mode Rouge syntax highlighting - **Add client-side search**: Lunr.js-powered search with a Jekyll-generated index from front matter metadata (title, keywords, audience, expertise_level); keyboard shortcuts (Ctrl+K / Cmd+K); dropdown results with snippets @@ -20,9 +21,11 @@ The docs-06 through docs-12 changes created per-bundle command pages, overview p - `docs-theme-toggle`: light/dark mode toggle with localStorage persistence, dual CSS theme, theme-aware Mermaid and syntax highlighting - `docs-client-search`: Lunr.js client-side search powered by front matter metadata index, keyboard shortcuts, and result snippets - `docs-role-expertise-nav`: expertise-level sidebar filter and homepage role-based entry cards for solo/startup/corporate/enterprise profiles +- `docs-module-changelog`: homepage or overview surfaces recent per-module release history from a canonical publish-driven release-history source + ### Modified Capabilities - `bundle-overview-pages`: sidebar links updated to point to actual overview and command pages instead of generic commands reference -- `modules-homepage-overview`: bundle overview section updated to surface corrected deep-dive links and role-based entry paths +- `modules-homepage-overview`: bundle overview section enhanced to expose recent per-module release history instead of only bundle links - `cross-module-workflow-docs`: sidebar Workflows section updated to include all docs-10 deliverables - `team-setup-docs`: sidebar Team & Enterprise section updated to use correct `/team-and-enterprise/` paths and include all docs-11 deliverables - `modules-docs-command-validation`: CI validation script may need updates to cover new `_data/nav.yml` link targets and enriched front matter fields @@ -30,7 +33,7 @@ The docs-06 through docs-12 changes created per-bundle command pages, overview p ## Impact - **Files modified**: `docs/_layouts/default.html`, `docs/assets/main.scss`, `docs/_config.yml`, `docs/index.md`, ~50+ `docs/**/*.md` (front matter only) -- **New files**: `docs/_data/nav.yml`, `docs/_includes/{sidebar-nav,search,theme-toggle,expertise-filter,breadcrumbs}.html`, `docs/assets/js/{theme,search,filters}.js`, `docs/assets/js/search-index.json` +- **New files**: `docs/_data/nav.yml`, `docs/_includes/{sidebar-nav,search,theme-toggle,expertise-filter,breadcrumbs}.html`, `docs/assets/js/{theme,search,filters}.js`, `docs/assets/js/search-index.json`, plus a structured release-history data source if required - **Dependencies**: Lunr.js loaded from CDN (~8 KB gzipped) - **No URL changes**: all existing `redirect_from` entries preserved; cross-site links from `docs.specfact.io` unaffected - **Cross-site**: `docs/reference/documentation-url-contract.md` unchanged diff --git a/openspec/changes/docs-13-nav-search-theme-roles/specs/cross-module-workflow-docs/spec.md b/openspec/changes/docs-13-nav-search-theme-roles/specs/cross-module-workflow-docs/spec.md index e9f455e..9c1a714 100644 --- a/openspec/changes/docs-13-nav-search-theme-roles/specs/cross-module-workflow-docs/spec.md +++ b/openspec/changes/docs-13-nav-search-theme-roles/specs/cross-module-workflow-docs/spec.md @@ -1,5 +1,3 @@ -# Specs - Cross Module Workflow Docs - ## MODIFIED Requirements ### Requirement: Workflow docs SHALL cover current cross-module flows and setup prerequisites diff --git a/openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md b/openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md index 9165cb1..201bdff 100644 --- a/openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md +++ b/openspec/changes/docs-13-nav-search-theme-roles/specs/docs-client-search/spec.md @@ -1,73 +1,56 @@ -# Specs - Docs Client Search - ## ADDED Requirements ### Requirement: Search index generation - A Jekyll Liquid template at `docs/assets/js/search-index.json` SHALL generate a JSON array at build time containing one entry per page with fields: `url`, `title`, `keywords` (from front matter), `audience`, `expertise_level`, and `content` (page content truncated to the first 200 words with HTML tags stripped). #### Scenario: Search index contains all pages - - **WHEN** the Jekyll site is built - **THEN** the generated `search-index.json` SHALL contain entries for every page that has a `title` in its front matter #### Scenario: Search index includes front matter metadata - - **WHEN** a page has `keywords: [backlog, refinement, sprint]` in its front matter - **THEN** its search index entry SHALL include those keywords in the `keywords` field ### Requirement: Search UI in sidebar - A search input field SHALL be rendered in the sidebar above the navigation sections via `docs/_includes/search.html`. The search input SHALL have placeholder text "Search docs... (Ctrl+K)". #### Scenario: Search input is visible - - **WHEN** any page loads - **THEN** a search input field SHALL appear at the top of the sidebar, above all navigation sections #### Scenario: Keyboard shortcut focuses search - - **WHEN** the user presses Ctrl+K (or Cmd+K on macOS) - **THEN** the search input SHALL receive focus ### Requirement: Lunr.js search integration - The search SHALL use Lunr.js loaded from CDN. The search index SHALL be lazy-loaded on first search input focus. Lunr SHALL be configured with field boosting: title at 10x, keywords at 5x, content at 1x. #### Scenario: First search triggers index load - - **WHEN** the user focuses the search input for the first time - **THEN** the `search-index.json` SHALL be fetched and a Lunr index SHALL be built #### Scenario: Search results appear on input - - **WHEN** the user types at least 2 characters in the search input - **THEN** a dropdown SHALL appear below the search input showing matching results with page title and a content snippet #### Scenario: No results message - - **WHEN** the user's query matches no pages - **THEN** the dropdown SHALL show "No results found" ### Requirement: Search result navigation - The search results dropdown SHALL support keyboard navigation with arrow keys and Enter to follow a link. Clicking a result SHALL navigate to that page. #### Scenario: Arrow key navigation - - **WHEN** the search dropdown is open and the user presses the down arrow - **THEN** the next result SHALL be highlighted #### Scenario: Enter navigates to result - - **WHEN** a result is highlighted and the user presses Enter - **THEN** the browser SHALL navigate to that result's URL ### Requirement: Search results show metadata tags - Each search result SHALL display matching front matter tags (audience, expertise_level) as small pills/badges alongside the title. #### Scenario: Result shows audience tag - - **WHEN** a search result is for a page with `audience: [team, enterprise]` - **THEN** the result SHALL display "team" and "enterprise" as tag pills diff --git a/openspec/changes/docs-13-nav-search-theme-roles/specs/docs-nav-data-driven/spec.md b/openspec/changes/docs-13-nav-search-theme-roles/specs/docs-nav-data-driven/spec.md index d52c6ed..8488db7 100644 --- a/openspec/changes/docs-13-nav-search-theme-roles/specs/docs-nav-data-driven/spec.md +++ b/openspec/changes/docs-13-nav-search-theme-roles/specs/docs-nav-data-driven/spec.md @@ -1,93 +1,72 @@ -# Specs - Navigation Data Driven - ## ADDED Requirements ### Requirement: Navigation data file - The sidebar navigation structure SHALL be defined in `docs/_data/nav.yml` as a YAML data file. Each top-level entry SHALL have a `section` name and either an `items` array (flat list) or a `bundles` array (collapsible groups). Each item SHALL have `title`, `url`, and optionally `expertise` fields. #### Scenario: Nav data file defines all sections - - **WHEN** the `_data/nav.yml` file is loaded - **THEN** it SHALL contain entries for all seven sections: Getting Started, Bundles, Workflows, Integrations, Team & Enterprise, Authoring, Reference #### Scenario: Bundle section uses collapsible groups - - **WHEN** the Bundles section is defined in `nav.yml` - **THEN** it SHALL use a `bundles` array with entries for Backlog, Project, Codebase, Code Review, Spec, and Govern, each containing an `items` array ### Requirement: Sidebar nav rendered from data - The sidebar navigation in `docs/_layouts/default.html` SHALL render navigation by including `docs/_includes/sidebar-nav.html` which iterates over `site.data.nav` using Liquid loops. Hardcoded navigation HTML SHALL be removed. #### Scenario: Sidebar renders all nav items - - **WHEN** a page loads with the default layout - **THEN** the sidebar SHALL display all sections and items defined in `nav.yml` #### Scenario: Bundle sections render as collapsible details - - **WHEN** a bundle group is rendered - **THEN** it SHALL use `
` HTML elements with the bundle name as `` ### Requirement: All bundle links point to actual pages - Every bundle navigation link SHALL point to an existing page URL, not to the generic `/reference/commands/` placeholder. #### Scenario: Code Review bundle links - - **WHEN** the Code Review bundle section is expanded - **THEN** it SHALL contain links to Overview, Run, Ledger, and Rules pages at their `/bundles/code-review/` paths #### Scenario: Spec bundle links - - **WHEN** the Spec bundle section is expanded - **THEN** it SHALL contain links to Overview, Validate, Generate Tests, and Mock pages at their `/bundles/spec/` paths #### Scenario: Govern bundle links - - **WHEN** the Govern bundle section is expanded - **THEN** it SHALL contain links to Overview, Enforce, and Patch pages at their `/bundles/govern/` paths #### Scenario: Codebase bundle links - - **WHEN** the Codebase bundle section is expanded - **THEN** it SHALL contain links to Overview, Sidecar Validation, Analyze, Drift, and Repro pages at their `/bundles/codebase/` paths ### Requirement: Active page highlighting - The sidebar SHALL highlight the currently active page by matching `page.url` against nav item URLs and applying a CSS active class. #### Scenario: Current page is highlighted - - **WHEN** a user visits `/bundles/spec/validate/` - **THEN** the Validate link in the Spec bundle section SHALL have the active CSS class applied - **AND** the Spec bundle `
` element SHALL be in the open state ### Requirement: Team & Enterprise links use correct paths - The Team & Enterprise section SHALL link to pages under `/team-and-enterprise/` with all four deliverables from docs-11. #### Scenario: Team & Enterprise navigation completeness - - **WHEN** the Team & Enterprise section is rendered - **THEN** it SHALL contain links to Team Collaboration, Agile & Scrum Setup, Multi-Repo Setup, and Enterprise Configuration at their `/team-and-enterprise/` paths ### Requirement: Workflows section includes all docs-10 pages - The Workflows section SHALL include links to all workflow pages created in docs-10. #### Scenario: Workflows navigation completeness - - **WHEN** the Workflows section is rendered - **THEN** it SHALL contain links to Cross-Module Chains, Daily DevOps Routine, CI/CD Pipeline, and Brownfield Modernization in addition to existing workflow links ### Requirement: Breadcrumb navigation above content - Each page SHALL display a breadcrumb trail above the content area, derived from the page URL segments, to provide orientation and allow quick navigation to parent sections. #### Scenario: Bundle command page shows breadcrumb trail - - **WHEN** a user visits `/bundles/spec/validate/` - **THEN** a breadcrumb trail SHALL be displayed showing: Home > Bundles > Spec > Validate - **AND** each breadcrumb segment except the current page SHALL be a clickable link diff --git a/openspec/changes/docs-13-nav-search-theme-roles/specs/modules-docs-command-validation/spec.md b/openspec/changes/docs-13-nav-search-theme-roles/specs/modules-docs-command-validation/spec.md index 7536583..2dfda5d 100644 --- a/openspec/changes/docs-13-nav-search-theme-roles/specs/modules-docs-command-validation/spec.md +++ b/openspec/changes/docs-13-nav-search-theme-roles/specs/modules-docs-command-validation/spec.md @@ -1,5 +1,3 @@ -# Specs - Modules Docs Command Validation - ## MODIFIED Requirements ### Requirement: Docs validation SHALL reject stale command and resource references diff --git a/openspec/changes/docs-13-nav-search-theme-roles/tasks.md b/openspec/changes/docs-13-nav-search-theme-roles/tasks.md index 510ef06..606faa1 100644 --- a/openspec/changes/docs-13-nav-search-theme-roles/tasks.md +++ b/openspec/changes/docs-13-nav-search-theme-roles/tasks.md @@ -1,5 +1,3 @@ -# docs-13 Tasks - ## 1. Data-Driven Navigation - [x] 1.1 Create `docs/_data/nav.yml` with all seven sections, correct bundle links (Overview + command pages for all 6 bundles), correct Team & Enterprise paths, and complete Workflows section @@ -54,3 +52,10 @@ - [x] 7.4 Verify light/dark toggle works and persists across page loads - [x] 7.5 Verify search returns results for known keywords - [x] 7.6 Verify expertise filter hides/shows nav items correctly + +## 8. Module Changelog Visibility + +- [ ] 8.1 Define a canonical source for per-module release history that is updated automatically whenever a module version is published +- [ ] 8.2 Add homepage or overview rendering for recent module release history using that canonical source, with graceful fallback when no history is available +- [ ] 8.3 Extend the publish flow so `.github/workflows/publish-modules.yml` writes the new release-history entry alongside the existing registry metadata update +- [ ] 8.4 Verify the rendered changelog entries match the canonical release-history source for all official modules diff --git a/openspec/changes/docs-14-module-release-history/CHANGE_VALIDATION.md b/openspec/changes/docs-14-module-release-history/CHANGE_VALIDATION.md new file mode 100644 index 0000000..d934903 --- /dev/null +++ b/openspec/changes/docs-14-module-release-history/CHANGE_VALIDATION.md @@ -0,0 +1,46 @@ +# Change Validation: docs-14-module-release-history + +Date: 2026-03-28 + +## Scope Reviewed + +- `proposal.md` +- `tasks.md` +- `design.md` +- `specs/module-release-history-registry/spec.md` +- `specs/module-release-history-docs/spec.md` +- related repository inputs: + - `CHANGELOG.md` + - `registry/index.json` + - `.github/workflows/publish-modules.yml` + - `openspec/config.yaml` + +## Validation Commands + +```bash +openspec validate docs-14-module-release-history --strict +``` + +Result: + +```text +Change 'docs-14-module-release-history' is valid +``` + +## Findings + +- No schema or artifact-format validation errors were reported by `openspec validate --strict`. +- The proposed change is correctly separated from `docs-13-nav-search-theme-roles`; it introduces new scope across publish automation, historical backfill, docs rendering, and OpenSpec rule updates. +- The current repository-level `CHANGELOG.md` is not sufficient as the canonical release-history source because it is repo-level prose, not structured per module/version, and is not currently part of the publish automation contract. + +## Dependency / Risk Notes + +- Publish workflow integration is the critical dependency because future correctness depends on release-history entries being written whenever a module is published. +- Historical backfill requires explicit human review because there is no reliable per-module tag history to reconstruct shipped scope deterministically. +- `openspec/config.yaml` rule updates should remain advisory and scoped to release-oriented changes; they should not force unrelated docs changes to invent release notes. + +## Recommendation + +- Proceed with `docs-14-module-release-history` as a standalone change. +- Keep `CHANGELOG.md` as a repo-level narrative changelog. +- Introduce a separate canonical structured release-history source for official modules and drive docs rendering from that source. diff --git a/openspec/changes/docs-14-module-release-history/design.md b/openspec/changes/docs-14-module-release-history/design.md new file mode 100644 index 0000000..fec20cf --- /dev/null +++ b/openspec/changes/docs-14-module-release-history/design.md @@ -0,0 +1,90 @@ +# Design: Module Release History As Publish-Driven Structured Data + +## Summary + +This change introduces a canonical structured release-history source for official modules and uses it to render user-facing release summaries in the modules docs. The source is updated by the publish workflow whenever a module version is released. Existing releases are backfilled once using AI-assisted extraction plus review. The repository-level `CHANGELOG.md` remains a narrative repo changelog and is not treated as the canonical machine-readable source. + +## Design Goals + +- Keep docs rendering static and repository-driven +- Avoid parsing prose from `CHANGELOG.md` at runtime or build time +- Ensure future publishes cannot forget release-history updates +- Support one-time historical backfill with human review +- Preserve a user-friendly summary of shipped features and improvements per module version +- Keep AI-generated release and patch notes clear, concise, and user-focused rather than implementation-noise-heavy + +## Proposed Data Shape + +Suggested canonical record fields: + +- `module_id` +- `version` +- `published_at` +- `summary` +- `features` +- `improvements` +- `fixes` +- `breaking_changes` +- `source_refs` + +Suggested authoring split: + +- structured fields hold normalized release facts (`module_id`, `version`, `published_at`, categorized bullets) +- AI-generated summary fields turn those facts into concise user-facing release and patch notes for docs consumption + +The canonical store should be append-only by module/version, with updates allowed only for corrective edits to already-recorded entries. + +## Repository Placement + +Preferred model: + +- Canonical source under `registry/` because it is publish-owned metadata +- Optional docs projection under `docs/_data/` generated or synchronized from the canonical source for simple Jekyll rendering + +This keeps install/search metadata lean in `registry/index.json` while preserving a richer history model elsewhere. + +## Publish Integration + +The publish workflow already detects changed bundles, builds artifacts, and updates `registry/index.json`. The same step should also persist a release-history entry for each published bundle version. That requires a stable input contract for release notes so publish automation is not forced to infer meaning from code diffs at publish time. + +Possible input sources: + +- manifest-adjacent structured release note file committed with the change +- release metadata block in `module-package.yaml` +- curated workflow input consumed by the publish action + +The most maintainable path is a committed structured source in the repo so the release note content is reviewable in PRs and can later feed docs automatically. + +## AI Release Note Style + +AI-assisted release-note drafting should be constrained by explicit project rules: + +- summarize shipped user-facing scope first +- prefer concrete feature/improvement bullets over internal implementation detail +- avoid jargon-heavy narration unless needed for user understanding +- avoid empty hype and generic filler phrasing +- keep patch notes concise and scannable +- make clear which module and version each note applies to + +## Historical Backfill + +Already-published versions in `registry/index.json` need initial coverage. Because there are no per-module tags and `CHANGELOG.md` is incomplete/module-skewed, backfill should use AI-assisted extraction from: + +- module manifest version history available in repo history +- merged PR descriptions and commit messages where available +- root `CHANGELOG.md` +- docs additions that clearly announce new commands/features + +Backfilled entries must be marked as reviewed before becoming canonical. + +## OpenSpec Rule Updates + +`openspec/config.yaml` should gain rules that make release-history updates explicit when a change: + +- modifies a published module payload +- changes docs that summarize current module capabilities +- introduces or revises publish automation for official modules + +This keeps future docs and publish metadata aligned without relying on memory. + +It should also add release-note generation guidance so future AI-assisted updates use the same user-focused summarization style during publish and backfill work. diff --git a/openspec/changes/docs-14-module-release-history/proposal.md b/openspec/changes/docs-14-module-release-history/proposal.md new file mode 100644 index 0000000..7bc323c --- /dev/null +++ b/openspec/changes/docs-14-module-release-history/proposal.md @@ -0,0 +1,46 @@ +# Change: Publish-Driven Module Release History And Docs Rendering + +## Why + +The modules docs homepage currently tells users what each official module does, but it does not answer the more operational question: which module versions have been published recently, and what features or improvements each release shipped. The repository-level `CHANGELOG.md` is not a good source for that view because it is prose-first, repo-level, and not consistently structured per module/version. The publish workflow already updates `registry/index.json` whenever a module is released, so release-history metadata should be captured in the same publish path and then rendered in docs from repository data without dynamic loading. + +Already-published module versions also need historical coverage so the feature is useful from day one. That requires a one-time backfill from existing repo evidence with AI-assisted extraction, followed by a stable structured format for future publishes. OpenSpec project rules should also be tightened so release-history extraction/update becomes part of the expected workflow for future module releases and docs refreshes. + +## What Changes + +- Define a canonical structured release-history source for official modules, separate from the lean install/search registry index +- Extend `.github/workflows/publish-modules.yml` so each published module version writes a release-history entry alongside the existing registry metadata update +- Add a one-time backfill workflow for already-published module versions using AI-assisted extraction from existing repo evidence and human-reviewable structured output +- Let AI copilot draft module-specific release and patch notes in a regular changelog style, but constrained to clear user-facing scope, shipped value, and concise summaries rather than low-signal technical detail +- Render recent per-module release history in the modules docs overview so users can immediately see published versions and shipped features/improvements +- Document the authoring/publish expectations for maintaining release-history metadata +- Update `openspec/config.yaml` rules so future changes involving module releases or docs synchronization account for release-history extraction/update requirements + +## Capabilities + +### New Capabilities + +- `module-release-history-registry`: canonical structured release-history metadata for official module publishes +- `module-release-history-docs`: docs overview renders recent published module versions with shipped features and improvements +- `module-release-note-summarization`: AI-assisted release-note drafting produces user-facing module release summaries with explicit shipped scope and minimal technical noise + +### Modified Capabilities + +- `publish-modules-workflow`: publish automation writes release-history entries in addition to updating `registry/index.json` +- `openspec-project-rules`: OpenSpec configuration guides future release-oriented changes to include release-history extraction/update where applicable + +## Impact + +- New publish-driven data source under `registry/` and/or `docs/_data/` +- Modified workflow: `.github/workflows/publish-modules.yml` +- Modified docs: homepage/overview rendering plus publishing guidance +- Modified OpenSpec project config: `openspec/config.yaml` +- Backfill scope: existing published official module versions in `registry/index.json` + +## Source Tracking + + +- **GitHub Issue**: #124 +- **Issue URL**: https://github.com/nold-ai/specfact-cli-modules/issues/124 +- **Last Synced Status**: synced +- **Sanitized**: true diff --git a/openspec/changes/docs-14-module-release-history/specs/module-release-history-docs/spec.md b/openspec/changes/docs-14-module-release-history/specs/module-release-history-docs/spec.md new file mode 100644 index 0000000..bf76169 --- /dev/null +++ b/openspec/changes/docs-14-module-release-history/specs/module-release-history-docs/spec.md @@ -0,0 +1,38 @@ +# Module Release History Docs + +## ADDED Requirements + +### Requirement: Modules docs overview SHALL show recent published module releases + +The modules docs overview SHALL present recent published module versions together with shipped features and improvements so users can quickly understand what each module has released. + +#### Scenario: Overview shows recent release details + +- **GIVEN** structured release-history entries exist for official modules +- **WHEN** the modules docs site is built +- **THEN** the overview renders recent module versions with user-friendly shipped features and improvements +- **AND** the rendering uses repository data at build time without runtime network fetches + +#### Scenario: Sparse history degrades gracefully + +- **GIVEN** a module has only partial or newly initialized release-history data +- **WHEN** the docs overview renders that module +- **THEN** the page still shows the published version context available +- **AND** it does not break the rest of the overview + +### Requirement: OpenSpec project rules SHALL describe release-history update expectations + +The project OpenSpec configuration SHALL guide future release-oriented changes to include release-history extraction or update steps when they affect published modules or docs that summarize them. + +#### Scenario: Release-oriented change references release-history expectations + +- **GIVEN** a future change modifies an official module payload or publish workflow +- **WHEN** proposal artifacts are created under the project OpenSpec rules +- **THEN** the rules call out release-history update expectations where applicable +- **AND** docs-sync changes can rely on that canonical release-history source + +#### Scenario: Release-note style guidance is available to AI copilot + +- **GIVEN** a future release-oriented change uses AI copilot to help draft module release or patch notes +- **WHEN** project OpenSpec rules are consulted +- **THEN** they instruct the AI to keep notes user-focused, scope-explicit, and they MUST avoid unnecessary technical jargon or generic filler text diff --git a/openspec/changes/docs-14-module-release-history/specs/module-release-history-registry/spec.md b/openspec/changes/docs-14-module-release-history/specs/module-release-history-registry/spec.md new file mode 100644 index 0000000..465f02f --- /dev/null +++ b/openspec/changes/docs-14-module-release-history/specs/module-release-history-registry/spec.md @@ -0,0 +1,44 @@ +# Module Release History Registry + +## ADDED Requirements + +### Requirement: Official module publishes SHALL persist structured release-history entries + +The modules repository SHALL maintain a canonical structured release-history source for official modules, and each newly published module version SHALL add a corresponding release-history entry as part of the publish workflow. + +#### Scenario: Publish writes release-history entry + +- **GIVEN** an official module version is published through the repository publish workflow +- **WHEN** the workflow updates registry metadata for that published version +- **THEN** it also records a structured release-history entry for that module id and version +- **AND** the entry includes user-facing shipped features and/or improvements for that release + +### Requirement: AI-assisted module release notes SHALL stay user-focused + +AI-assisted module release-note generation SHALL produce clear user-facing summaries of shipped scope and SHALL avoid low-signal technical filler. + +#### Scenario: Publish-time release note is user-facing + +- **GIVEN** a module publish includes AI-assisted release-note drafting +- **WHEN** the release-history entry is generated +- **THEN** the resulting summary explains what shipped in user-facing language +- **AND** it prioritizes concrete features, improvements, or fixes +- **AND** it avoids unnecessary implementation detail that does not help users understand the release + +#### Scenario: Canonical history is separate from lean registry index + +- **GIVEN** the repository maintains `registry/index.json` for install/search metadata +- **WHEN** release-history metadata is added +- **THEN** the richer per-version history is stored in a separate canonical source +- **AND** `registry/index.json` remains focused on latest install/search metadata + +### Requirement: Existing published module versions SHALL be backfilled through a reviewable extraction flow + +The repository SHALL support a one-time backfill process for already-published official module versions so the docs can show useful release history from launch. + +#### Scenario: Historical versions produce candidate entries + +- **GIVEN** official module versions already listed in `registry/index.json` +- **WHEN** the historical backfill process runs +- **THEN** it produces candidate structured release-history entries for those versions +- **AND** the candidates are presented in a reviewable form before being accepted as canonical diff --git a/openspec/changes/docs-14-module-release-history/tasks.md b/openspec/changes/docs-14-module-release-history/tasks.md new file mode 100644 index 0000000..4c1882f --- /dev/null +++ b/openspec/changes/docs-14-module-release-history/tasks.md @@ -0,0 +1,42 @@ +# 1. Change Setup + +- [ ] 1.1 Update `openspec/CHANGE_ORDER.md` with `docs-14-module-release-history` +- [ ] 1.2 Add capability specs for structured module release history and docs rendering + +## 2. Release History Data Model + +- [ ] 2.1 Define a canonical structured schema for per-module release history with fields for module id, version, published date, shipped features, shipped improvements, and optional links/notes +- [ ] 2.2 Decide repository ownership for the canonical history source and any docs-consumable projection derived from it +- [ ] 2.3 Document why `CHANGELOG.md` remains a repo-level narrative changelog rather than the canonical module release-history source + +## 3. Publish Workflow Integration + +- [ ] 3.1 Extend `.github/workflows/publish-modules.yml` so each module publish writes a release-history entry together with the registry metadata update +- [ ] 3.2 Define the publish-time input contract for shipped features/improvements so the workflow can record user-friendly release notes without free-form drift +- [ ] 3.3 Define AI-assisted release-note drafting rules so copilot writes clear user-facing module release/patch notes with explicit shipped scope and no low-signal technical filler +- [ ] 3.4 Update publishing docs to describe the new release-history requirement, AI drafting rules, and review flow + +## 4. Historical Backfill + +- [ ] 4.1 Inventory already-published official module versions from `registry/index.json` +- [ ] 4.2 Define an AI-assisted backfill procedure that extracts candidate shipped features/improvements from existing repository evidence into the canonical structured format +- [ ] 4.3 Add human-review guidance so backfilled release-history entries are approved before becoming canonical +- [ ] 4.4 Ensure backfilled AI-generated summaries follow the same user-focused release-note style as future publish-time entries + +## 5. Docs Rendering + +- [ ] 5.1 Add homepage or overview rendering that clearly shows recent published module versions and their shipped features/improvements +- [ ] 5.2 Ensure docs rendering is build-time/static and does not depend on runtime network fetches +- [ ] 5.3 Provide graceful handling for modules with sparse or not-yet-backfilled history + +## 6. OpenSpec Rule Updates + +- [ ] 6.1 Update `openspec/config.yaml` rules so release-oriented changes include release-history extraction/update expectations where applicable +- [ ] 6.2 Add rule guidance for future docs updates that depend on publish-driven module history +- [ ] 6.3 Add rule guidance for AI copilot release-note generation style: user-facing benefits first, shipped scope explicit, and no unnecessary technical jargon or generic filler text + +## 7. Verification + +- [ ] 7.1 Verify the publish workflow can write a correct release-history entry for a newly published module version +- [ ] 7.2 Verify the docs overview renders release-history data accurately for all official modules +- [ ] 7.3 Verify the backfill procedure produces reviewable candidate entries for existing published versions diff --git a/openspec/config.yaml b/openspec/config.yaml index 392946c..7b2e78d 100644 --- a/openspec/config.yaml +++ b/openspec/config.yaml @@ -1,20 +1,86 @@ schema: spec-driven -# Project context (optional) -# This is shown to AI when creating artifacts. -# Add your tech stack, conventions, style guides, domain knowledge, etc. -# Example: -# context: | -# Tech stack: TypeScript, React, Node.js -# We use conventional commits -# Domain: e-commerce platform - -# Per-artifact rules (optional) -# Add custom rules for specific artifacts. -# Example: -# rules: -# proposal: -# - Keep proposals under 500 words -# - Always include a "Non-goals" section -# tasks: -# - Break tasks into chunks of max 2 hours +# Project context (injected into ALL artifact requests) +# Keep concise; focus on ecosystem role, layout, and constraints agents often miss. +context: | + Ecosystem role: **specfact-cli-modules** is the official home for nold-ai **bundled module packages** and the + **module registry** consumed by SpecFact CLI. The CLI discovers and loads bundles via `registry/index.json` + and `packages//module-package.yaml`; published user docs for bundles live on **modules.specfact.io** + (and GitHub Pages under this repo). This repo does **not** ship the core CLI—peer **specfact-cli** is a dev + dependency (`hatch run dev-deps` → sibling `../specfact-cli`, `SPECFACT_CLI_REPO`, or matching worktree). + + Repository layout: + - `packages//` — bundle source, `module-package.yaml`, Typer apps and commands + - `registry/index.json` — marketplace index (versions, URLs, checksums) + - `scripts/`, `tools/` — signing, verify, publish, pre-commit helpers + - `tests/` — bundle behavior, migration parity, registry tests + - `docs/` — bundle and registry documentation (Jekyll; permalink rules in front matter) + - `openspec/` — OpenSpec changes; use `openspec/CHANGE_ORDER.md` for sequencing + + Tech stack (bundle + tooling): Python 3.11+, Hatch, Typer, Pydantic. Contract-first public APIs: + `@icontract` (`@require`/`@ensure`) and `@beartype` where bundle code exposes stable surfaces. Testing: pytest; + `hatch run contract-test` / `smart-test` align with specfact-cli discipline when bundles integrate core APIs. + + Bundle ↔ core coupling: + - Bundle Python may `import specfact_cli` (models, validators, registry helpers)—keep **core_compatibility** + in `module-package.yaml` truthful when raising minimum specfact-cli. + - Version bumps: **semver** per AGENTS.md (`patch`/`minor`/`major`); registry rows and signatures must stay + consistent with published artifacts. + + Quality & CI (typical order): `format` → `type-check` → `lint` → `yaml-lint` → module **signature verification** + (`verify-modules-signature`, enforce version bump when manifests change) → `contract-test` → `smart-test` → + `test`. Pre-commit: signatures → `pre-commit-quality-checks.sh` → `pre_commit_code_review.py` (JSON report under + `.specfact/`). CI: `.github/workflows/pr-orchestrator.yml` (matrix Python, gates above). + + Documentation & cross-site: Canonical modules URLs and core↔modules handoffs—**docs/reference/documentation-url-contract.md** + (do not invent permalinks; match published modules.specfact.io). Core user docs live on **docs.specfact.io**; + keep cross-links honest when a change affects both repos. + + OpenSpec discipline: Spec deltas before tests before implementation; **TDD_EVIDENCE.md** for behavior changes. + **Archive** changes only via `openspec archive ` (no manual folder moves); update **CHANGE_ORDER.md** + when lifecycle changes. + + Philosophy (aligned with specfact-cli): Contract-first and regression-safe bundle evolution; offline-first + publishing assumptions unless a task explicitly adds network steps. + + Code review JSON (dogfood): Treat `.specfact/code-review.json` as mandatory evidence before an OpenSpec + change is complete. If the file is missing or stale (see tasks), run + `specfact code review run --json --out .specfact/code-review.json` and resolve **every** finding at any + severity (warning or error) unless the proposal documents a rare, explicit exception. + +# Per-artifact rules (only injected into matching artifacts) +rules: + proposal: + - Align bundle and registry changes with semver, `core_compatibility`, signing, and AGENTS.md release policy. + - State impact on `registry/index.json` and any `packages//module-package.yaml` when versions or artifacts change. + - For user-facing doc or command changes, note affected `docs/` paths and modules.specfact.io permalinks. + + specs: + - Use Given/When/Then for scenarios; tie scenarios to tests under `tests/` for bundle or registry behavior. + - Call out new or changed Typer commands and Pydantic models with contract expectations where relevant. + + design: + - Describe how the bundle integrates with `specfact_cli` imports and registry discovery—avoid circular or undeclared core dependencies. + - For publish/sign flows, reference `scripts/` entrypoints and integrity expectations. + + tasks: + - Enforce SDD+TDD order: branch/worktree → spec deltas → failing tests → implementation → passing tests → + TDD_EVIDENCE.md → quality gates → PR. + - Include module signing / version-bump tasks when `module-package.yaml` or bundle payloads change (see AGENTS.md). + - Record TDD evidence in `openspec/changes//TDD_EVIDENCE.md` for behavior changes. + - |- + SpecFact code review JSON (dogfood, required before PR): Include tasks to + - (1) Ensure `.specfact/code-review.json` is present and **fresh**: if the file is missing, or its + last-modified time is older than any file you changed in this change under `packages/`, `registry/`, + `scripts/`, `tools/`, `tests/`, or under `openspec/changes//` **excluding** + `openspec/changes//TDD_EVIDENCE.md` (evidence-only updates there do not by themselves + invalidate the review file; re-run when proposal, specs, tasks, design, or code change). Run a new + review, e.g. + `hatch run specfact code review run --json --out .specfact/code-review.json` with `--scope changed` + during iteration and `--scope full` (or equivalent coverage) before the final PR. + - (2) Read the JSON report and remediate **all** findings regardless of severity (warning, advisory, + error, or equivalent in the schema): treat them as blocking until fixed or the proposal explicitly + documents a justified exception approved in the change. + - (3) Re-run the review after substantive edits until the report shows a passing outcome per the + review module (e.g. overall verdict PASS / CI exit 0); record the review command(s) and timestamp + in `TDD_EVIDENCE.md` or the PR description when the change touches behavior or quality gates. diff --git a/openspec/specs/review-finding-model/spec.md b/openspec/specs/review-finding-model/spec.md new file mode 100644 index 0000000..5b04e99 --- /dev/null +++ b/openspec/specs/review-finding-model/spec.md @@ -0,0 +1,141 @@ +# Review Finding Model Specification + +## Overview + +The `ReviewFinding` model represents structured code-review findings emitted by the `specfact-code-review` bundle. This specification defines the canonical schema, category enumeration, and tool mapping for all review runners. + +## Schema Definition + +### Core Fields + +| Field | Type | Description | Required | Constraints | +|-------|------|-------------|----------|-------------| +| `category` | string (enum) | Governed code-review category | Yes | Must be one of the defined categories | +| `severity` | string (enum) | Finding severity level | Yes | Must be "error", "warning", or "info" | +| `tool` | string | Originating tool name | Yes | Non-empty string | +| `rule` | string | Originating rule identifier | Yes | Non-empty string | +| `file` | string | Repository-relative file path | Yes | Non-empty string | +| `line` | integer | 1-based source line number | Yes | Must be ≥ 1 | +| `message` | string | User-facing finding message | Yes | Non-empty string | +| `fixable` | boolean | Whether finding can be auto-fixed | No | Default: false | + +### Category Enumeration + +The following categories are supported: + +- `clean_code`: General clean-code violations (e.g., complexity, readability) +- `security`: Security-related issues +- `type_safety`: Type checking violations +- `contracts`: Contract/precondition violations +- `testing`: Test-related findings (coverage, missing tests) +- `style`: Code style violations +- `architecture`: Architectural concerns +- `tool_error`: Tool execution/parsing errors +- `naming`: Naming convention violations +- `kiss`: KISS principle violations (Keep It Simple, Stupid) +- `yagni`: YAGNI principle violations (You Aren't Gonna Need It) +- `dry`: DRY principle violations (Don't Repeat Yourself) +- `solid`: SOLID principle violations + +### Tool Enumeration + +The following tools are officially supported: + +- `ruff`: Style and formatting linter +- `radon`: Complexity analyzer +- `radon-kiss`: KISS metrics analyzer +- `semgrep`: Pattern-based static analysis +- `basedpyright`: Type checker +- `pylint`: Architecture and quality linter +- `contract_runner`: Contract validation +- `pytest`: Test execution and coverage +- `checklist`: PR checklist validator +- `ast`: AST-based clean-code analyzer + +## Category-Tool Mapping + +### Clean Code Tools + +- `radon`: Emits `clean_code` findings for cyclomatic complexity +- `radon-kiss`: Emits `kiss` findings for LOC, nesting, and parameter counts +- `ast`: Emits `naming`, `kiss`, `yagni`, `dry`, `solid` findings from AST analysis + +### Style Tools + +- `ruff`: Emits `style` findings for formatting and conventions + +### Type Safety Tools + +- `basedpyright`: Emits `type_safety` findings for type violations + +### Architecture Tools + +- `pylint`: Emits `architecture` findings for design issues + +### Testing Tools + +- `pytest`: Emits `testing` findings for test failures and coverage +- `contract_runner`: Emits `contracts` findings for contract violations + +### Checklist Tools + +- `checklist`: Emits `clean_code` findings for PR checklist items + +## Examples + +### KISS Violation + +```json +{ + "category": "kiss", + "severity": "warning", + "tool": "radon-kiss", + "rule": "kiss.loc.warning", + "file": "src/module.py", + "line": 42, + "message": "Function `process_data` spans 85 lines; keep it under 80.", + "fixable": false +} +``` + +### Naming Violation + +```json +{ + "category": "naming", + "severity": "warning", + "tool": "ast", + "rule": "naming.generic-public-name", + "file": "src/api.py", + "line": 15, + "message": "Public API names should be specific; avoid generic names like process, handle, or manager.", + "fixable": true +} +``` + +### SOLID Violation + +```json +{ + "category": "solid", + "severity": "error", + "tool": "ast", + "rule": "solid.single-responsibility", + "file": "src/service.py", + "line": 28, + "message": "Function mixes persistence and transport concerns; split repository and HTTP client calls.", + "fixable": false +} +``` + +## Validation Rules + +1. All string fields must be non-empty after stripping whitespace +2. The `line` field must be a positive integer (≥ 1) +3. The `category` field must be one of the enumerated values +4. The `severity` field must be one of: "error", "warning", "info" +5. Tool names should match the official tool enumeration where possible + +## Backward Compatibility + +This specification is backward compatible with existing `ReviewFinding` consumers. New categories (`naming`, `kiss`, `yagni`, `dry`, `solid`) and tools (`ast`, `checklist`) extend rather than replace the existing schema. \ No newline at end of file diff --git a/packages/specfact-code-review/.semgrep/clean_code.yaml b/packages/specfact-code-review/.semgrep/clean_code.yaml index 76e18a6..ffc57e6 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: '(?im)^(?:def|class)\s+(?!_+)\w*(?:process|handle|manager|data)\w*\b' + + - id: swallowed-exception-pattern + message: Exception handlers must not swallow failures with pass. + 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..3f3867d 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.4 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:78f1426086fb967c2f0ba8b0f5e65c5368205cd2a2ecd6f9d2a98531ed3ee402 + signature: 8AadiIQUR3mxaxh0Mk6voN1zX94GeVLbL57ZUjTF//cqz8mQaOmd4Vz4/zEMVNRstYkQ4l9rrzays4C47tnpBQ== 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/__init__.py b/packages/specfact-code-review/src/specfact_code_review/__init__.py index 49aaacb..6e2e2b6 100644 --- a/packages/specfact-code-review/src/specfact_code_review/__init__.py +++ b/packages/specfact-code-review/src/specfact_code_review/__init__.py @@ -2,15 +2,26 @@ from __future__ import annotations +from importlib import import_module +from typing import TYPE_CHECKING + __all__ = ("app", "export_from_bundle", "import_to_bundle", "sync_with_bundle", "validate_bundle") +if TYPE_CHECKING: + from specfact_code_review.review.app import ( + app, + export_from_bundle, + import_to_bundle, + sync_with_bundle, + validate_bundle, + ) + def __getattr__(name: str) -> object: if name not in __all__: msg = f"module {__name__!r} has no attribute {name!r}" raise AttributeError(msg) - from specfact_code_review.review import app as review_app_module - + review_app_module = import_module("specfact_code_review.review.app") return getattr(review_app_module, name) diff --git a/packages/specfact-code-review/src/specfact_code_review/_review_utils.py b/packages/specfact-code-review/src/specfact_code_review/_review_utils.py index 38431ed..33323ad 100644 --- a/packages/specfact-code-review/src/specfact_code_review/_review_utils.py +++ b/packages/specfact-code-review/src/specfact_code_review/_review_utils.py @@ -6,10 +6,17 @@ from pathlib import Path from typing import Literal +from beartype import beartype +from icontract import ensure, require + from specfact_code_review.run.findings import ReviewFinding -def _normalize_path_variants(path_value: str | Path) -> set[str]: +@beartype +@require(lambda path_value: isinstance(path_value, str | Path)) +@ensure(lambda result: isinstance(result, set)) +def normalize_path_variants(path_value: str | Path) -> set[str]: + """Return a normalized set of path spellings for source matching.""" path = Path(path_value) variants = { os.path.normpath(str(path)), @@ -24,7 +31,13 @@ def _normalize_path_variants(path_value: str | Path) -> set[str]: return variants -def _tool_error( +@beartype +@require(lambda tool: isinstance(tool, str) and bool(tool.strip())) +@require(lambda file_path: isinstance(file_path, Path)) +@require(lambda message: isinstance(message, str) and bool(message.strip())) +@require(lambda severity: severity in {"error", "warning", "info"}) +@ensure(lambda result: isinstance(result, ReviewFinding)) +def tool_error( *, tool: str, file_path: Path, diff --git a/packages/specfact-code-review/src/specfact_code_review/ledger/client.py b/packages/specfact-code-review/src/specfact_code_review/ledger/client.py index 8f214c8..7a958bb 100644 --- a/packages/specfact-code-review/src/specfact_code_review/ledger/client.py +++ b/packages/specfact-code-review/src/specfact_code_review/ledger/client.py @@ -184,6 +184,14 @@ def _write_local_state(self, state: LedgerState) -> None: self._local_path.parent.mkdir(parents=True, exist_ok=True) self._local_path.write_text(state.model_dump_json(indent=2), encoding="utf-8") + def _ledger_run_from_payload_entry(self, entry: object) -> LedgerRun | None: + if not isinstance(entry, dict): + return None + try: + return LedgerRun.model_validate(entry) + except ValidationError: + return None + def _read_supabase_runs(self, *, limit: int) -> list[LedgerRun] | None: try: response = requests.get( @@ -206,11 +214,10 @@ def _read_supabase_runs(self, *, limit: int) -> list[LedgerRun] | None: runs: list[LedgerRun] = [] for entry in reversed(payload): - if isinstance(entry, dict): - try: - runs.append(LedgerRun.model_validate(entry)) - except ValidationError: - return None + run = self._ledger_run_from_payload_entry(entry) + if run is None: + return None + runs.append(run) return runs def _read_supabase_state(self) -> LedgerState | None: 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..4f6a118 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,31 @@ 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/review/commands.py b/packages/specfact-code-review/src/specfact_code_review/review/commands.py index ff73483..3021d66 100644 --- a/packages/specfact-code-review/src/specfact_code_review/review/commands.py +++ b/packages/specfact-code-review/src/specfact_code_review/review/commands.py @@ -6,6 +6,7 @@ from typing import Literal import typer +from icontract import ensure, require from icontract.errors import ViolationError from specfact_code_review.ledger.commands import app as ledger_app @@ -40,57 +41,45 @@ def _resolve_include_tests(*, files: list[Path], include_tests: bool | None, int @review_app.command("run") -def _run( - files: list[Path] = typer.Argument(None, metavar="FILES..."), - *, - scope: Literal["changed", "full"] | None = typer.Option( - None, - "--scope", - help="Auto-discovery scope when positional files are omitted: changed or full.", - ), - path_filters: list[Path] | None = typer.Option( - None, - "--path", - help="Repeatable repo-relative path prefix used to limit auto-discovered review files.", - ), - include_tests: bool | None = typer.Option( - None, - "--include-tests/--exclude-tests", - help="Include changed test files when review scope is auto-detected from git diff.", - ), - include_noise: bool = typer.Option( - False, - "--include-noise/--suppress-noise", - help="Include known low-signal findings such as test-scope contract noise.", - ), - json_output: bool = typer.Option( - False, - "--json", - help="Write ReviewReport JSON to a file. Use --out to override the default path.", - ), - out: Path | None = typer.Option( - None, - "--out", - help="JSON output file path used with --json. Default: review-report.json.", - ), - score_only: bool = typer.Option(False, "--score-only", help="Print only the reward delta integer."), - no_tests: bool = typer.Option(False, "--no-tests", help="Skip the TDD gate."), - fix: bool = typer.Option(False, "--fix", help="Apply Ruff autofixes and re-run the review."), - interactive: bool = typer.Option(False, "--interactive", help="Ask review-scope questions before running."), +@require(lambda ctx: True, "run command validation") +@ensure(lambda result: result is None, "run command does not return") +def run( + ctx: typer.Context, + files: list[Path] = typer.Argument(None), + scope: Literal["changed", "full"] = typer.Option(None), + path: list[Path] = typer.Option(None, "--path"), + include_tests: bool = typer.Option(None, "--include-tests"), + exclude_tests: bool = typer.Option(None, "--exclude-tests"), + include_noise: bool = typer.Option(False, "--include-noise"), + suppress_noise: bool = typer.Option(False, "--suppress-noise"), + json_output: bool = typer.Option(False, "--json"), + out: Path = typer.Option(None, "--out"), + score_only: bool = typer.Option(False, "--score-only"), + no_tests: bool = typer.Option(False, "--no-tests"), + fix: bool = typer.Option(False, "--fix"), + interactive: bool = typer.Option(False, "--interactive"), ) -> None: - """Execute code review runs.""" + """Run the full code review workflow.""" + # Resolve mutually exclusive test inclusion options + if include_tests is not None and exclude_tests is not None: + raise typer.BadParameter("Cannot use both --include-tests and --exclude-tests") + + resolved_include_tests = _resolve_include_tests( + files=files or [], + include_tests=include_tests, + interactive=interactive, + ) + + # Resolve noise inclusion (suppress-noise takes precedence) + resolved_include_noise = include_noise and not suppress_noise + try: - resolved_include_tests = _resolve_include_tests( - files=files, - include_tests=include_tests, - interactive=interactive, - ) exit_code, output = run_command( - files, + files or [], include_tests=resolved_include_tests, scope=scope, - path_filters=path_filters, - include_noise=include_noise, + path_filters=path, + include_noise=resolved_include_noise, json_output=json_output, out=out, score_only=score_only, diff --git a/packages/specfact-code-review/src/specfact_code_review/rules/commands.py b/packages/specfact-code-review/src/specfact_code_review/rules/commands.py index d0f3b58..2e78247 100644 --- a/packages/specfact-code-review/src/specfact_code_review/rules/commands.py +++ b/packages/specfact-code-review/src/specfact_code_review/rules/commands.py @@ -21,7 +21,7 @@ @app.command("show") -def show() -> None: +def _show() -> None: """Print the current skill content.""" skill_path = _skill_path() if not skill_path.exists(): @@ -34,11 +34,13 @@ def show() -> None: @app.command("init") -def init( +def _init( ide: SupportedIde | None = typer.Option( None, "--ide", - help="Install to the canonical target path for one IDE. Omit to keep only skills/specfact-code-review/SKILL.md.", + help=( + "Install to the canonical target path for one IDE. Omit to keep only skills/specfact-code-review/SKILL.md." + ), ), ) -> None: """Create the default skill file and optionally install it to one canonical IDE target.""" @@ -56,11 +58,11 @@ def init( @app.command("update") -def update( +def _update( ide: SupportedIde | None = typer.Option( None, "--ide", - help="Refresh one canonical IDE target. Omit to refresh only IDE targets already installed in the project.", + help=("Refresh one canonical IDE target. Omit to refresh only IDE targets already installed in the project."), ), ) -> None: """Update the TOP VIOLATIONS section and refresh canonical IDE targets.""" 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..e5db700 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 @@ -19,7 +19,7 @@ _BUNDLED_SKILL_PATH = ("resources", "skills", "specfact-code-review", "SKILL.md") -MAX_SKILL_LINES = 35 +MAX_SKILL_LINES = 40 SKILL_PATH = Path("skills/specfact-code-review/SKILL.md") CURSOR_RULES_PATH = Path(".cursor/rules/house_rules.mdc") @@ -29,25 +29,27 @@ TOP_VIOLATIONS_MARKER = "" DEFAULT_DESCRIPTION = "House rules for AI coding sessions derived from review findings" DEFAULT_DO_RULES = ( + "- Verify an active OpenSpec change covers the requested scope and follow the sequence: spec delta → failing tests → implementation → passing tests → quality gates", "- 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 +107,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 +119,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 + + front_matter, separator, remainder = content.partition("\n---\n") + if not separator or not remainder: + return body, description + + body = remainder.lstrip("\n") + match = re.search(r"^description:\s*(?P.+)$", front_matter, 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 +258,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/__init__.py b/packages/specfact-code-review/src/specfact_code_review/run/__init__.py index 66c6b90..b570b0d 100644 --- a/packages/specfact-code-review/src/specfact_code_review/run/__init__.py +++ b/packages/specfact-code-review/src/specfact_code_review/run/__init__.py @@ -1,23 +1,48 @@ """Runtime helpers for code review.""" -from typing import Any +from __future__ import annotations + +from collections.abc import Callable +from importlib import import_module +from pathlib import Path + +from beartype import beartype +from icontract import ensure, require from specfact_code_review.run.findings import ReviewFinding, ReviewReport from specfact_code_review.run.scorer import ReviewScore, score_review -def run_review(*args: Any, **kwargs: Any) -> ReviewReport: +@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, ReviewReport)) +def run_review( + files: list[Path], + *, + no_tests: bool = False, + include_noise: bool = False, + progress_callback: Callable[[str], None] | None = None, +) -> ReviewReport: """Lazily import the orchestrator to avoid package import cycles.""" - from specfact_code_review.run.runner import run_review as _run_review - - return _run_review(*args, **kwargs) - - -def run_tdd_gate(*args: Any, **kwargs: Any) -> list[ReviewFinding]: + run_review_impl = import_module("specfact_code_review.run.runner").run_review + return run_review_impl( + files, + no_tests=no_tests, + include_noise=include_noise, + progress_callback=progress_callback, + ) + + +@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)) +@ensure(lambda result: all(isinstance(finding, ReviewFinding) for finding in result)) +def run_tdd_gate(files: list[Path]) -> list[ReviewFinding]: """Lazily import the TDD gate to avoid package import cycles.""" - from specfact_code_review.run.runner import run_tdd_gate as _run_tdd_gate - - return _run_tdd_gate(*args, **kwargs) + run_tdd_gate_impl = import_module("specfact_code_review.run.runner").run_tdd_gate + return run_tdd_gate_impl(files) __all__ = ["ReviewFinding", "ReviewReport", "ReviewScore", "run_review", "run_tdd_gate", "score_review"] diff --git a/packages/specfact-code-review/src/specfact_code_review/run/commands.py b/packages/specfact-code-review/src/specfact_code_review/run/commands.py index 19cd191..9be7a20 100644 --- a/packages/specfact-code-review/src/specfact_code_review/run/commands.py +++ b/packages/specfact-code-review/src/specfact_code_review/run/commands.py @@ -5,9 +5,10 @@ import subprocess import sys from collections import defaultdict -from collections.abc import Iterable +from collections.abc import Callable, Iterable +from dataclasses import dataclass from pathlib import Path -from typing import Literal +from typing import Literal, cast from beartype import beartype from icontract import ensure, require @@ -23,6 +24,22 @@ AutoScope = Literal["changed", "full"] +@dataclass(frozen=True) +class ReviewRunRequest: + """Inputs needed to execute a governed review run.""" + + files: list[Path] + include_tests: bool = False + scope: AutoScope | None = None + path_filters: list[Path] | None = None + include_noise: bool = False + json_output: bool = False + out: Path | None = None + score_only: bool = False + no_tests: bool = False + fix: bool = False + + def _is_test_file(file_path: Path) -> bool: return "tests" in file_path.parts @@ -205,30 +222,11 @@ def _render_report(report: ReviewReport) -> None: grouped[finding.category].append(finding) if not grouped: - console.print("Code Review") - console.print(report.summary) - else: - for category in sorted(grouped): - table = Table(title=f"Code Review: {category}", show_header=True, header_style="bold cyan") - table.add_column("File", style="cyan") - table.add_column("Line", justify="right") - table.add_column("Tool") - table.add_column("Rule") - table.add_column("Severity") - table.add_column("Message", overflow="fold") - for finding in grouped[category]: - row = [ - finding.file, - str(finding.line), - finding.tool, - finding.rule, - finding.severity, - finding.message, - ] - table.add_row( - *row, - ) - console.print(table) + _render_empty_report(report) + return + + for category in sorted(grouped): + _render_category_report(category, grouped[category]) console.print( f"Verdict: {report.overall_verdict} | CI exit: {report.ci_exit_code} | " @@ -237,6 +235,31 @@ def _render_report(report: ReviewReport) -> None: console.print(report.summary) +def _render_empty_report(report: ReviewReport) -> None: + console.print("Code Review") + console.print(report.summary) + + +def _render_category_report(category: str, findings: list[ReviewFinding]) -> None: + table = Table(title=f"Code Review: {category}", show_header=True, header_style="bold cyan") + table.add_column("File", style="cyan") + table.add_column("Line", justify="right") + table.add_column("Tool") + table.add_column("Rule") + table.add_column("Severity") + table.add_column("Message", overflow="fold") + for finding in findings: + table.add_row( + finding.file, + str(finding.line), + finding.tool, + finding.rule, + finding.severity, + finding.message, + ) + console.print(table) + + def _json_output_path(out: Path | None) -> Path: return out or Path("review-report.json") @@ -256,92 +279,227 @@ def _run_review_with_progress( fix: bool, ) -> ReviewReport: if _is_interactive_terminal(): - with progress_console.status("Preparing code review...") as status: - report = run_review( + return _run_review_with_status(files, no_tests=no_tests, include_noise=include_noise, fix=fix) + + def _emit_progress(description: str) -> None: + progress_console.print(f"[dim]{description}[/dim]") + + return _run_review_once( + files, + no_tests=no_tests, + include_noise=include_noise, + fix=fix, + progress_callback=_emit_progress, + ) + + +def _run_review_with_status( + files: list[Path], + *, + no_tests: bool, + include_noise: bool, + fix: bool, +) -> ReviewReport: + with progress_console.status("Preparing code review...") as status: + report = _run_review_once( + files, + no_tests=no_tests, + include_noise=include_noise, + fix=False, + progress_callback=status.update, + ) + if fix: + status.update("Applying Ruff autofixes...") + _apply_fixes(files) + status.update("Re-running review after autofixes...") + report = _run_review_once( files, no_tests=no_tests, include_noise=include_noise, + fix=False, progress_callback=status.update, ) - if fix: - status.update("Applying Ruff autofixes...") - _apply_fixes(files) - status.update("Re-running review after autofixes...") - report = run_review( - files, - no_tests=no_tests, - include_noise=include_noise, - progress_callback=status.update, - ) - return report + return report - def _emit_progress(description: str) -> None: - progress_console.print(f"[dim]{description}[/dim]") +def _run_review_once( + files: list[Path], + *, + no_tests: bool, + include_noise: bool, + fix: bool, + progress_callback: Callable[[str], None] | None, +) -> ReviewReport: report = run_review( files, no_tests=no_tests, include_noise=include_noise, - progress_callback=_emit_progress, + progress_callback=progress_callback, ) if fix: - _emit_progress("Applying Ruff autofixes...") + if progress_callback is not None: + progress_callback("Applying Ruff autofixes...") + else: + progress_console.print("[dim]Applying Ruff autofixes...[/dim]") _apply_fixes(files) - _emit_progress("Re-running review after autofixes...") + if progress_callback is not None: + progress_callback("Re-running review after autofixes...") + else: + progress_console.print("[dim]Re-running review after autofixes...[/dim]") report = run_review( files, no_tests=no_tests, include_noise=include_noise, - progress_callback=_emit_progress, + progress_callback=progress_callback, ) return report +def _as_auto_scope(value: object) -> AutoScope | None: + if value is None: + return None + if isinstance(value, str) and value in {"changed", "full"}: + return cast(AutoScope, value) + raise ValueError(f"Invalid scope value: {value!r}") + + +def _as_path_filters(value: object) -> list[Path] | None: + if value is None: + return None + if isinstance(value, list) and all(isinstance(path_filter, Path) for path_filter in value): + return value + raise ValueError("Path filters must be a list of Path instances.") + + +def _as_optional_path(value: object) -> Path | None: + if value is None: + return None + if isinstance(value, Path): + return value + raise ValueError("Output path must be a Path instance.") + + +def _build_review_run_request( + files: list[Path], + kwargs: dict[str, object], +) -> ReviewRunRequest: + # Validate files is a list of Path instances + if not isinstance(files, list): + raise ValueError(f"files must be a list, got {type(files).__name__}") + if not all(isinstance(file_path, Path) for file_path in files): + raise ValueError("files must contain only Path instances") + + request_kwargs = dict(kwargs) + + # Validate and extract known boolean flags with proper type checking + def _get_bool_param(name: str, default: bool = False) -> bool: + value = request_kwargs.pop(name, default) + if value is None: + return default + if not isinstance(value, bool): + raise ValueError(f"{name} must be a boolean, got {type(value).__name__}") + return value + + # Validate and extract known path/scope parameters + def _get_optional_param(name: str, validator: Callable[[object], object], default: object = None) -> object: + value = request_kwargs.pop(name, default) + if value is None or value == default: + return default + return validator(value) + + # Get include_tests with proper default + include_tests_value = request_kwargs.pop("include_tests", None) + include_tests = False # default value + if include_tests_value is not None: + if not isinstance(include_tests_value, bool): + raise ValueError(f"include_tests must be a boolean, got {type(include_tests_value).__name__}") + include_tests = include_tests_value + + # Get optional parameters with proper type casting + scope_value = _get_optional_param("scope", _as_auto_scope) + path_filters_value = _get_optional_param("path_filters", _as_path_filters) + out_value = _get_optional_param("out", _as_optional_path) + + # Cast the optional parameters to their proper types + scope = cast(AutoScope | None, scope_value) + path_filters = cast(list[Path] | None, path_filters_value) + out = cast(Path | None, out_value) + + request = ReviewRunRequest( + files=files, + include_tests=include_tests, + scope=scope, + path_filters=path_filters, + include_noise=_get_bool_param("include_noise"), + json_output=_get_bool_param("json_output"), + out=out, + score_only=_get_bool_param("score_only"), + no_tests=_get_bool_param("no_tests"), + fix=_get_bool_param("fix"), + ) + + # Reject any unexpected keyword arguments + if request_kwargs: + unexpected = ", ".join(sorted(request_kwargs)) + raise ValueError(f"Unexpected keyword arguments: {unexpected}") + + return request + + +def _render_review_result(report: ReviewReport, request: ReviewRunRequest) -> tuple[int, str | None]: + if request.json_output: + output_path = _json_output_path(request.out) + output_path.parent.mkdir(parents=True, exist_ok=True) + output_path.write_text(report.model_dump_json(), encoding="utf-8") + return report.ci_exit_code or 0, str(output_path) + if request.score_only: + return report.ci_exit_code or 0, str(report.score) + + _render_report(report) + return report.ci_exit_code or 0, None + + +def _validate_review_request(request: ReviewRunRequest) -> None: + if request.json_output and request.score_only: + raise ValueError("Use either --json or --score-only, not both.") + if not request.json_output and request.out is not None: + raise ValueError("Use --out together with --json.") + + @beartype -@require(lambda files: files is None or all(isinstance(file_path, Path) for file_path in files)) @require( - lambda json_output, score_only: not (json_output and score_only), - "Use either --json or --score-only, not both.", + lambda request_or_files: request_or_files is None or isinstance(request_or_files, (list, ReviewRunRequest)), + "request must be a review request or a list of Path objects", ) -@require(lambda json_output, out: json_output or out is None, "Use --out together with --json.") @ensure(lambda result: isinstance(result, tuple)) def run_command( - files: list[Path] | None = None, - *, - include_tests: bool = False, - scope: AutoScope | None = None, - path_filters: list[Path] | None = None, - include_noise: bool = False, - json_output: bool = False, - out: Path | None = None, - score_only: bool = False, - no_tests: bool = False, - fix: bool = False, + request_or_files: ReviewRunRequest | list[Path] | None = None, + **kwargs: object, ) -> tuple[int, str | None]: """Execute a governed review run over the provided files.""" + request = ( + request_or_files + if isinstance(request_or_files, ReviewRunRequest) + else _build_review_run_request( + list(request_or_files or []), + kwargs, + ) + ) + _validate_review_request(request) + resolved_files = _resolve_files( - files or [], - include_tests=include_tests, - scope=scope, - path_filters=path_filters or [], + request.files, + include_tests=request.include_tests, + scope=request.scope, + path_filters=request.path_filters or [], ) report = _run_review_with_progress( resolved_files, - no_tests=no_tests, - include_noise=include_noise, - fix=fix, + no_tests=request.no_tests, + include_noise=request.include_noise, + fix=request.fix, ) - - if json_output: - output_path = _json_output_path(out) - output_path.parent.mkdir(parents=True, exist_ok=True) - output_path.write_text(report.model_dump_json(), encoding="utf-8") - return report.ci_exit_code or 0, str(output_path) - if score_only: - return report.ci_exit_code or 0, str(report.reward_delta) - - _render_report(report) - return report.ci_exit_code or 0, None + return _render_review_result(report, request) -__all__ = ["run_command"] +__all__ = ["ReviewRunRequest", "run_command"] 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..4ccd86f 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,10 +4,11 @@ import json import os +import re 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 @@ -15,9 +16,10 @@ from beartype import beartype from icontract import ensure, require -from specfact_code_review._review_utils import _normalize_path_variants, _tool_error +from specfact_code_review._review_utils import normalize_path_variants, 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 +42,40 @@ ("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") +_TARGETED_TEST_TIMEOUT = int(os.environ.get("SPECFACT_CODE_REVIEW_TARGETED_TEST_TIMEOUT", "120")) 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 @@ -71,11 +90,11 @@ def _coverage_for_source(source_file: Path, payload: dict[str, object]) -> float files_payload = payload.get("files") if not isinstance(files_payload, dict): return None - allowed_paths = _normalize_path_variants(source_file) + allowed_paths = normalize_path_variants(source_file) for filename, file_payload in files_payload.items(): if not isinstance(filename, str): continue - if _normalize_path_variants(filename).isdisjoint(allowed_paths): + if normalize_path_variants(filename).isdisjoint(allowed_paths): continue if not isinstance(file_payload, dict): return None @@ -90,32 +109,36 @@ 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] = [] + pythonpath_entries: list[str] = [str(_SOURCE_ROOT.resolve()), str(Path.cwd().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 - 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) +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) - 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) - env["PYTHONPATH"] = os.pathsep.join(pythonpath_entries) - return env +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]: @@ -128,11 +151,6 @@ def _pytest_targets(test_files: list[Path]) -> list[Path]: def _pytest_python_executable() -> str: - local_candidates = [Path(".venv/bin/python"), Path(".venv/Scripts/python.exe")] - for candidate in local_candidates: - resolved = candidate.resolve() - if resolved.is_file(): - return str(resolved) return sys.executable @@ -141,10 +159,18 @@ def _run_pytest_with_coverage(test_files: list[Path]) -> tuple[subprocess.Comple coverage_path = Path(coverage_file.name) test_targets = _pytest_targets(test_files) + source_root = str(_SOURCE_ROOT.resolve()) + repo_root = str(Path.cwd().resolve()) command = [ _pytest_python_executable(), - "-m", - "pytest", + "-c", + ( + "import pathlib, sys, pytest; " + f"sys.path[:0] = [{source_root!r}, {repo_root!r}]; " + "import specfact_code_review; " + "raise SystemExit(pytest.main(sys.argv[1:]))" + ), + "--import-mode=importlib", "--cov", str(_PACKAGE_ROOT), "--cov-fail-under=0", @@ -156,7 +182,7 @@ def _run_pytest_with_coverage(test_files: list[Path]) -> tuple[subprocess.Comple capture_output=True, text=True, check=False, - timeout=120, + timeout=_TARGETED_TEST_TIMEOUT, env=_pytest_env(), ) return result, coverage_path @@ -186,11 +212,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(re.search(rf"\b{re.escape(hint)}\b", context, flags=re.IGNORECASE) 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), @@ -223,6 +281,26 @@ def _collect_tdd_inputs(files: list[Path]) -> tuple[list[Path], list[Path], list return source_files, test_files, findings +def _is_empty_init_file(source_file: Path) -> bool: + """Check if __init__.py is a marker/empty module with no executable statements.""" + if source_file.name != "__init__.py": + return False + + try: + content = source_file.read_text(encoding="utf-8") + except OSError: + return False + + # Strip whitespace, comments, and docstrings + stripped_content = re.sub(r'"""[^"""]*"""', "", content, flags=re.DOTALL) + stripped_content = re.sub(r"'''[^']*'''", "", stripped_content, flags=re.DOTALL) + stripped_content = re.sub(r"#.*$", "", stripped_content, flags=re.MULTILINE) + stripped_content = stripped_content.strip() + + # Consider empty if only contains 'pass' or is completely empty + return stripped_content in ("", "pass") + + def _coverage_findings( source_files: list[Path], coverage_payload: dict[str, object], @@ -232,8 +310,10 @@ def _coverage_findings( for source_file in source_files: percent_covered = _coverage_for_source(source_file, coverage_payload) if percent_covered is None: + if source_file.name == "__init__.py" and _is_empty_init_file(source_file): + continue # Exempt empty __init__.py files return [ - _tool_error( + tool_error( tool="pytest", file_path=source_file, message=f"Coverage data missing for {source_file}", @@ -271,7 +351,7 @@ def _evaluate_tdd_gate(files: list[Path]) -> tuple[list[ReviewFinding], dict[str test_result, coverage_path = _run_pytest_with_coverage(test_files) except (FileNotFoundError, OSError, subprocess.TimeoutExpired) as exc: return [ - _tool_error( + tool_error( tool="pytest", file_path=source_files[0], message=f"Unable to execute targeted tests: {exc}", @@ -296,7 +376,7 @@ def _evaluate_tdd_gate(files: list[Path]) -> tuple[list[ReviewFinding], dict[str coverage_payload = json.loads(coverage_path.read_text(encoding="utf-8")) except (OSError, json.JSONDecodeError) as exc: return [ - _tool_error( + tool_error( tool="pytest", file_path=source_files[0], message=f"Unable to read coverage report: {exc}", @@ -359,6 +439,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/run/scorer.py b/packages/specfact-code-review/src/specfact_code_review/run/scorer.py index a2eb0f7..c10ce1f 100644 --- a/packages/specfact-code-review/src/specfact_code_review/run/scorer.py +++ b/packages/specfact-code-review/src/specfact_code_review/run/scorer.py @@ -2,6 +2,7 @@ from __future__ import annotations +from dataclasses import dataclass from typing import Literal from beartype import beartype @@ -25,20 +26,25 @@ class ReviewScore(BaseModel): ci_exit_code: Literal[0, 1] = Field(..., description="CI-compatible exit code.") -def _bonus_points( - zero_loc_violations: bool, - zero_complexity_violations: bool, - all_apis_have_icontract: bool, - coverage_90_plus: bool, - no_new_suppressions: bool, -) -> int: +@dataclass(frozen=True) +class ReviewScoreModifiers: + """Optional bonuses that influence the computed score.""" + + zero_loc_violations: bool = False + zero_complexity_violations: bool = False + all_apis_have_icontract: bool = False + coverage_90_plus: bool = False + no_new_suppressions: bool = False + + +def _bonus_points(modifiers: ReviewScoreModifiers) -> int: return 5 * sum( [ - zero_loc_violations, - zero_complexity_violations, - all_apis_have_icontract, - coverage_90_plus, - no_new_suppressions, + modifiers.zero_loc_violations, + modifiers.zero_complexity_violations, + modifiers.all_apis_have_icontract, + modifiers.coverage_90_plus, + modifiers.no_new_suppressions, ] ) @@ -70,23 +76,23 @@ def _determine_verdict( @ensure(lambda result: 0 <= result.score <= 120) def score_review( findings: list[ReviewFinding], - *, - zero_loc_violations: bool = False, - zero_complexity_violations: bool = False, - all_apis_have_icontract: bool = False, - coverage_90_plus: bool = False, - no_new_suppressions: bool = False, + **kwargs: object, ) -> ReviewScore: """Compute the governed review score, reward delta, and verdict.""" + modifiers = ReviewScoreModifiers( + zero_loc_violations=bool(kwargs.pop("zero_loc_violations", False)), + zero_complexity_violations=bool(kwargs.pop("zero_complexity_violations", False)), + all_apis_have_icontract=bool(kwargs.pop("all_apis_have_icontract", False)), + coverage_90_plus=bool(kwargs.pop("coverage_90_plus", False)), + no_new_suppressions=bool(kwargs.pop("no_new_suppressions", False)), + ) + if kwargs: + unexpected = ", ".join(sorted(kwargs)) + raise ValueError(f"Unexpected keyword arguments: {unexpected}") + score = 100 score -= sum(_deduction_for_finding(finding) for finding in findings) - score += _bonus_points( - zero_loc_violations=zero_loc_violations, - zero_complexity_violations=zero_complexity_violations, - all_apis_have_icontract=all_apis_have_icontract, - coverage_90_plus=coverage_90_plus, - no_new_suppressions=no_new_suppressions, - ) + score += _bonus_points(modifiers) score = max(0, min(120, score)) overall_verdict, ci_exit_code = _determine_verdict(score=score, findings=findings) return ReviewScore( 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..7b122be --- /dev/null +++ b/packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py @@ -0,0 +1,203 @@ +"""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"} + + +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 + 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 + + +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: + 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)) + findings.extend(_dry_findings(file_path, tree)) + + return findings diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py index 5f5d869..17a253d 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py @@ -3,39 +3,25 @@ from __future__ import annotations import json -import os import subprocess from pathlib import Path +from typing import Literal from beartype import beartype -from icontract import ensure, require +from icontract import require +from specfact_code_review._review_utils import normalize_path_variants, tool_error from specfact_code_review.run.findings import ReviewFinding -def _normalize_path_variants(path_value: str | Path) -> set[str]: - path = Path(path_value) - variants = { - os.path.normpath(str(path)), - os.path.normpath(path.as_posix()), - } - try: - resolved = path.resolve() - except OSError: - return variants - variants.add(os.path.normpath(str(resolved))) - variants.add(os.path.normpath(resolved.as_posix())) - return variants - - def _allowed_paths(files: list[Path]) -> set[str]: allowed: set[str] = set() for file_path in files: - allowed.update(_normalize_path_variants(file_path)) + allowed.update(normalize_path_variants(file_path)) return allowed -def _map_severity(raw_severity: str) -> str: +def _map_severity(raw_severity: str) -> Literal["error", "warning", "info"]: if raw_severity == "error": return "error" if raw_severity == "warning": @@ -43,29 +29,63 @@ def _map_severity(raw_severity: str) -> str: return "info" -def _tool_error(file_path: Path, message: str) -> list[ReviewFinding]: - return [ - ReviewFinding( - category="tool_error", - severity="error", - tool="basedpyright", - rule="tool_error", - file=str(file_path), - line=1, - message=message, - fixable=False, - ) - ] +def _finding_from_diagnostic(diagnostic: object, *, allowed_paths: set[str]) -> ReviewFinding | None: + if not isinstance(diagnostic, dict): + raise ValueError("basedpyright diagnostic must be an object") + + filename = diagnostic["file"] + if not isinstance(filename, str): + raise ValueError("basedpyright filename must be a string") + if normalize_path_variants(filename).isdisjoint(allowed_paths): + return None + + raw_severity = diagnostic["severity"] + if not isinstance(raw_severity, str): + raise ValueError("basedpyright severity must be a string") + message = diagnostic["message"] + if not isinstance(message, str): + raise ValueError("basedpyright message must be a string") + line = diagnostic["range"]["start"]["line"] + if not isinstance(line, int): + raise ValueError("basedpyright line must be an integer") + rule = diagnostic.get("rule") + if rule is not None and not isinstance(rule, str): + raise ValueError("basedpyright rule must be a string when present") + + return ReviewFinding( + category="type_safety", + severity=_map_severity(raw_severity), + tool="basedpyright", + rule=rule or "basedpyright", + file=filename, + line=line + 1, + message=message, + fixable=False, + ) + + +def _diagnostics_from_output(stdout: str) -> list[object]: + payload = json.loads(stdout) + if not isinstance(payload, dict): + raise ValueError("basedpyright output must be an object") + diagnostics = payload["generalDiagnostics"] + if not isinstance(diagnostics, list): + raise ValueError("generalDiagnostics must be a list") + return diagnostics + + +def _findings_from_diagnostics(diagnostics: list[object], *, allowed_paths: set[str]) -> list[ReviewFinding]: + findings: list[ReviewFinding] = [] + for diagnostic in diagnostics: + finding = _finding_from_diagnostic(diagnostic, allowed_paths=allowed_paths) + if finding is not None: + findings.append(finding) + 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_basedpyright(files: list[Path]) -> list[ReviewFinding]: """Run basedpyright and map diagnostics into ReviewFinding records.""" if not files: @@ -73,57 +93,30 @@ def run_basedpyright(files: list[Path]) -> list[ReviewFinding]: try: result = subprocess.run( - ["basedpyright", "--outputjson", "--project", ".", *(str(file_path) for file_path in files)], + ["basedpyright", "--outputjson", "--project", ".", *[str(file_path) for file_path in files]], capture_output=True, text=True, check=False, timeout=30, ) - payload = json.loads(result.stdout) - if not isinstance(payload, dict): - raise ValueError("basedpyright output must be an object") - diagnostics = payload["generalDiagnostics"] - if not isinstance(diagnostics, list): - raise ValueError("generalDiagnostics must be a list") + diagnostics = _diagnostics_from_output(result.stdout) except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, KeyError, subprocess.TimeoutExpired) as exc: - return _tool_error(files[0], f"Unable to parse basedpyright output: {exc}") + return [ + tool_error( + tool="basedpyright", + file_path=files[0], + message=f"Unable to parse basedpyright output: {exc}", + ) + ] allowed_paths = _allowed_paths(files) - findings: list[ReviewFinding] = [] try: - for diagnostic in diagnostics: - if not isinstance(diagnostic, dict): - raise ValueError("basedpyright diagnostic must be an object") - filename = diagnostic["file"] - if not isinstance(filename, str): - raise ValueError("basedpyright filename must be a string") - if _normalize_path_variants(filename).isdisjoint(allowed_paths): - continue - raw_severity = diagnostic["severity"] - if not isinstance(raw_severity, str): - raise ValueError("basedpyright severity must be a string") - message = diagnostic["message"] - if not isinstance(message, str): - raise ValueError("basedpyright message must be a string") - line = diagnostic["range"]["start"]["line"] - if not isinstance(line, int): - raise ValueError("basedpyright line must be an integer") - rule = diagnostic.get("rule") - if rule is not None and not isinstance(rule, str): - raise ValueError("basedpyright rule must be a string when present") - findings.append( - ReviewFinding( - category="type_safety", - severity=_map_severity(raw_severity), - tool="basedpyright", - rule=rule or "basedpyright", - file=filename, - line=line + 1, - message=message, - fixable=False, - ) - ) + return _findings_from_diagnostics(diagnostics, allowed_paths=allowed_paths) except (KeyError, TypeError, ValueError) as exc: - return _tool_error(files[0], f"Unable to parse basedpyright finding payload: {exc}") - - return findings + return [ + tool_error( + tool="basedpyright", + file_path=files[0], + message=f"Unable to parse basedpyright finding payload: {exc}", + ) + ] diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py index cf94da3..fd04bf0 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py @@ -10,7 +10,7 @@ from beartype import beartype from icontract import ensure, require -from specfact_code_review._review_utils import _normalize_path_variants, _tool_error +from specfact_code_review._review_utils import normalize_path_variants, tool_error from specfact_code_review.run.findings import ReviewFinding @@ -29,7 +29,7 @@ def _allowed_paths(files: list[Path]) -> set[str]: allowed: set[str] = set() for file_path in files: - allowed.update(_normalize_path_variants(file_path)) + allowed.update(normalize_path_variants(file_path)) return allowed @@ -47,17 +47,22 @@ def _has_icontract(node: ast.FunctionDef | ast.AsyncFunctionDef) -> bool: return any(_decorator_name(decorator) in {"require", "ensure"} for decorator in node.decorator_list) +def _class_public_nodes(node: ast.ClassDef) -> list[ast.FunctionDef | ast.AsyncFunctionDef]: + return [ + class_node + for class_node in ast.iter_child_nodes(node) + if isinstance(class_node, (ast.FunctionDef, ast.AsyncFunctionDef)) and not class_node.name.startswith("_") + ] + + def _public_api_nodes(tree: ast.AST) -> list[ast.FunctionDef | ast.AsyncFunctionDef]: public_nodes: list[ast.FunctionDef | ast.AsyncFunctionDef] = [] for node in ast.iter_child_nodes(tree): if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and not node.name.startswith("_"): public_nodes.append(node) + continue if isinstance(node, ast.ClassDef): - for class_node in ast.iter_child_nodes(node): - if isinstance(class_node, (ast.FunctionDef, ast.AsyncFunctionDef)) and not class_node.name.startswith( - "_" - ): - public_nodes.append(class_node) + public_nodes.extend(_class_public_nodes(node)) return public_nodes @@ -78,7 +83,7 @@ def _scan_file(file_path: Path) -> list[ReviewFinding]: try: tree = ast.parse(file_path.read_text(encoding="utf-8")) except (OSError, UnicodeDecodeError, SyntaxError) as exc: - return [_tool_error(tool="contract_runner", file_path=file_path, message=f"Unable to parse AST: {exc}")] + return [tool_error(tool="contract_runner", file_path=file_path, message=f"Unable to parse AST: {exc}")] findings: list[ReviewFinding] = [] for node in _public_api_nodes(tree): @@ -115,7 +120,7 @@ def _run_crosshair(files: list[Path]) -> list[ReviewFinding]: return [] except (FileNotFoundError, OSError) as exc: return [ - _tool_error( + tool_error( tool="crosshair", file_path=files[0], message=f"Unable to execute CrossHair: {exc}", @@ -130,7 +135,7 @@ def _run_crosshair(files: list[Path]) -> list[ReviewFinding]: if match is None: continue filename = match.group("file") - if _normalize_path_variants(filename).isdisjoint(allowed_paths): + if normalize_path_variants(filename).isdisjoint(allowed_paths): continue message = match.group("message") if message.startswith(_IGNORED_CROSSHAIR_PREFIXES): diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py index 628dbcd..e194f90 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py @@ -3,17 +3,18 @@ from __future__ import annotations import json -import os import subprocess from pathlib import Path +from typing import Literal from beartype import beartype from icontract import ensure, require +from specfact_code_review._review_utils import normalize_path_variants, tool_error from specfact_code_review.run.findings import ReviewFinding -PYLINT_CATEGORY_MAP = { +PYLINT_CATEGORY_MAP: dict[str, Literal["architecture"]] = { "W0702": "architecture", "W0703": "architecture", "T201": "architecture", @@ -21,29 +22,14 @@ } -def _normalize_path_variants(path_value: str | Path) -> set[str]: - path = Path(path_value) - variants = { - os.path.normpath(str(path)), - os.path.normpath(path.as_posix()), - } - try: - resolved = path.resolve() - except OSError: - return variants - variants.add(os.path.normpath(str(resolved))) - variants.add(os.path.normpath(resolved.as_posix())) - return variants - - def _allowed_paths(files: list[Path]) -> set[str]: allowed: set[str] = set() for file_path in files: - allowed.update(_normalize_path_variants(file_path)) + allowed.update(normalize_path_variants(file_path)) return allowed -def _map_severity(message_id: str) -> str: +def _map_severity(message_id: str) -> Literal["error", "warning", "info"]: if message_id.startswith(("E", "F")): return "error" if message_id.startswith("I"): @@ -51,29 +37,69 @@ def _map_severity(message_id: str) -> str: return "warning" -def _tool_error(file_path: Path, message: str) -> list[ReviewFinding]: - return [ - ReviewFinding( - category="tool_error", - severity="error", - tool="pylint", - rule="tool_error", - file=str(file_path), - line=1, - message=message, - fixable=False, - ) - ] +def _category_for_message_id(message_id: str) -> Literal["architecture", "style"]: + if message_id in PYLINT_CATEGORY_MAP: + return "architecture" + return "style" + + +def _finding_from_item(item: object, *, allowed_paths: set[str]) -> ReviewFinding | None: + if not isinstance(item, dict): + raise ValueError("pylint finding must be an object") + + filename = item["path"] + if not isinstance(filename, str): + raise ValueError("pylint path must be a string") + if normalize_path_variants(filename).isdisjoint(allowed_paths): + return None + + message_id = item["message-id"] + if not isinstance(message_id, str): + raise ValueError("pylint message-id must be a string") + line = item["line"] + if not isinstance(line, int): + raise ValueError("pylint line must be an integer") + message = item["message"] + if not isinstance(message, str): + raise ValueError("pylint message must be a string") + + return ReviewFinding( + category=_category_for_message_id(message_id), + severity=_map_severity(message_id), + tool="pylint", + rule=message_id, + file=filename, + line=line, + message=message, + fixable=False, + ) + + +def _payload_from_output(stdout: str) -> list[object]: + payload = json.loads(stdout) + if not isinstance(payload, list): + raise ValueError("pylint output must be a list") + return payload + + +def _findings_from_payload(payload: list[object], *, allowed_paths: set[str]) -> list[ReviewFinding]: + findings: list[ReviewFinding] = [] + for item in payload: + finding = _finding_from_item(item, allowed_paths=allowed_paths) + if finding is not None: + findings.append(finding) + return findings + + +def _result_is_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( - lambda result: all(isinstance(finding, ReviewFinding) for finding in result), - "result must contain ReviewFinding instances", -) +@ensure(_result_is_review_findings, "result must contain ReviewFinding instances") def run_pylint(files: list[Path]) -> list[ReviewFinding]: """Run pylint and map message IDs into ReviewFinding records.""" if not files: @@ -81,51 +107,24 @@ def run_pylint(files: list[Path]) -> list[ReviewFinding]: try: result = subprocess.run( - ["pylint", "--output-format", "json", *(str(file_path) for file_path in files)], + ["pylint", "--output-format", "json", *[str(file_path) for file_path in files]], capture_output=True, text=True, check=False, timeout=30, ) - payload = json.loads(result.stdout) - if not isinstance(payload, list): - raise ValueError("pylint output must be a list") + payload = _payload_from_output(result.stdout) except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: - return _tool_error(files[0], f"Unable to parse pylint output: {exc}") + return [tool_error(tool="pylint", file_path=files[0], message=f"Unable to parse pylint output: {exc}")] allowed_paths = _allowed_paths(files) - findings: list[ReviewFinding] = [] try: - for item in payload: - if not isinstance(item, dict): - raise ValueError("pylint finding must be an object") - filename = item["path"] - if not isinstance(filename, str): - raise ValueError("pylint path must be a string") - if _normalize_path_variants(filename).isdisjoint(allowed_paths): - continue - message_id = item["message-id"] - if not isinstance(message_id, str): - raise ValueError("pylint message-id must be a string") - line = item["line"] - if not isinstance(line, int): - raise ValueError("pylint line must be an integer") - message = item["message"] - if not isinstance(message, str): - raise ValueError("pylint message must be a string") - findings.append( - ReviewFinding( - category=PYLINT_CATEGORY_MAP.get(message_id, "style"), - severity=_map_severity(message_id), - tool="pylint", - rule=message_id, - file=filename, - line=line, - message=message, - fixable=False, - ) - ) + return _findings_from_payload(payload, allowed_paths=allowed_paths) except (KeyError, TypeError, ValueError) as exc: - return _tool_error(files[0], f"Unable to parse pylint finding payload: {exc}") - - return findings + return [ + tool_error( + tool="pylint", + file_path=files[0], + message=f"Unable to parse pylint finding payload: {exc}", + ) + ] 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..7e31382 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 = 3 +_KISS_NESTING_ERROR = 5 +_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-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}.", + 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-kiss", + 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-kiss", + 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,82 @@ 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] = [] - 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}") + 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) + findings: list[ReviewFinding] = [] + if payload is None: + 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)) return findings diff --git a/packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py b/packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py index 3336d4d..93f1a9d 100644 --- a/packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py +++ b/packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py @@ -3,39 +3,25 @@ from __future__ import annotations import json -import os import subprocess from pathlib import Path +from typing import Literal from beartype import beartype from icontract import ensure, require +from specfact_code_review._review_utils import normalize_path_variants, tool_error from specfact_code_review.run.findings import ReviewFinding -def _normalize_path_variants(path_value: str | Path) -> set[str]: - path = Path(path_value) - variants = { - os.path.normpath(str(path)), - os.path.normpath(path.as_posix()), - } - try: - resolved = path.resolve() - except OSError: - return variants - variants.add(os.path.normpath(str(resolved))) - variants.add(os.path.normpath(resolved.as_posix())) - return variants - - def _allowed_paths(files: list[Path]) -> set[str]: allowed: set[str] = set() for file_path in files: - allowed.update(_normalize_path_variants(file_path)) + allowed.update(normalize_path_variants(file_path)) return allowed -def _category_for_rule(rule: str) -> str | None: +def _category_for_rule(rule: str) -> Literal["security", "clean_code", "style"] | None: if rule.startswith("S"): return "security" if rule.startswith("C9"): @@ -45,29 +31,69 @@ def _category_for_rule(rule: str) -> str | None: return None -def _tool_error(file_path: Path, message: str) -> list[ReviewFinding]: - return [ - ReviewFinding( - category="tool_error", - severity="error", - tool="ruff", - rule="tool_error", - file=str(file_path), - line=1, - message=message, - fixable=False, - ) - ] +def _finding_from_item(item: object, *, allowed_paths: set[str]) -> ReviewFinding | None: + if not isinstance(item, dict): + raise ValueError("ruff finding must be an object") + + filename = item["filename"] + if not isinstance(filename, str): + raise ValueError("ruff filename must be a string") + if normalize_path_variants(filename).isdisjoint(allowed_paths): + return None + + location = item["location"] + if not isinstance(location, dict): + raise ValueError("ruff location must be an object") + rule = item.get("code") or item.get("rule") + if not isinstance(rule, str): + raise ValueError("ruff rule must be a string") + category = _category_for_rule(rule) + if category is None: + return None + line = location["row"] + if not isinstance(line, int): + raise ValueError("ruff line must be an integer") + message = item["message"] + if not isinstance(message, str): + raise ValueError("ruff message must be a string") + + return ReviewFinding( + category=category, + severity="warning", + tool="ruff", + rule=rule, + file=filename, + line=line, + message=message, + fixable=bool(item.get("fix")), + ) + + +def _payload_from_output(stdout: str) -> list[object]: + payload = json.loads(stdout) + if not isinstance(payload, list): + raise ValueError("ruff output must be a list") + return payload + + +def _findings_from_payload(payload: list[object], *, allowed_paths: set[str]) -> list[ReviewFinding]: + findings: list[ReviewFinding] = [] + for item in payload: + finding = _finding_from_item(item, allowed_paths=allowed_paths) + if finding is not None: + findings.append(finding) + return findings + + +def _result_is_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( - lambda result: all(isinstance(finding, ReviewFinding) for finding in result), - "result must contain ReviewFinding instances", -) +@ensure(_result_is_review_findings, "result must contain ReviewFinding instances") def run_ruff(files: list[Path]) -> list[ReviewFinding]: """Run Ruff for the provided files and map findings into ReviewFinding records.""" if not files: @@ -75,57 +101,24 @@ def run_ruff(files: list[Path]) -> list[ReviewFinding]: try: result = subprocess.run( - ["ruff", "check", "--output-format", "json", *(str(file_path) for file_path in files)], + ["ruff", "check", "--output-format", "json", *[str(file_path) for file_path in files]], capture_output=True, text=True, check=False, timeout=30, ) - payload = json.loads(result.stdout) - if not isinstance(payload, list): - raise ValueError("ruff output must be a list") + payload = _payload_from_output(result.stdout) except (FileNotFoundError, OSError, ValueError, json.JSONDecodeError, subprocess.TimeoutExpired) as exc: - return _tool_error(files[0], f"Unable to parse Ruff output: {exc}") + return [tool_error(tool="ruff", file_path=files[0], message=f"Unable to parse Ruff output: {exc}")] allowed_paths = _allowed_paths(files) - findings: list[ReviewFinding] = [] try: - for item in payload: - if not isinstance(item, dict): - raise ValueError("ruff finding must be an object") - filename = item["filename"] - if not isinstance(filename, str): - raise ValueError("ruff filename must be a string") - if _normalize_path_variants(filename).isdisjoint(allowed_paths): - continue - location = item["location"] - if not isinstance(location, dict): - raise ValueError("ruff location must be an object") - rule = item.get("code") or item.get("rule") - if not isinstance(rule, str): - raise ValueError("ruff rule must be a string") - category = _category_for_rule(rule) - if category is None: - continue - line = location["row"] - if not isinstance(line, int): - raise ValueError("ruff line must be an integer") - message = item["message"] - if not isinstance(message, str): - raise ValueError("ruff message must be a string") - findings.append( - ReviewFinding( - category=category, - severity="warning", - tool="ruff", - rule=rule, - file=filename, - line=line, - message=message, - fixable=bool(item.get("fix")), - ) - ) + return _findings_from_payload(payload, allowed_paths=allowed_paths) except (KeyError, TypeError, ValueError) as exc: - return _tool_error(files[0], f"Unable to parse Ruff finding payload: {exc}") - - return findings + return [ + tool_error( + tool="ruff", + file_path=files[0], + message=f"Unable to parse Ruff finding payload: {exc}", + ) + ] 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..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 @@ -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,20 @@ 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") + 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 + + 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 +203,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/pyproject.toml b/pyproject.toml index fa0d8f0..689fbe0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -16,6 +16,7 @@ dev = [] type = "virtual" path = ".venv" dependencies = [ + "icontract>=2.7.1", "pytest>=8.4.2", "pytest-cov>=7.0.0", "pytest-mock>=3.15.1", @@ -91,6 +92,7 @@ venvPath = "." venv = ".venv" executionEnvironments = [ { root = "src" }, + { root = "scripts" }, { root = "tools" }, { root = "tests", reportMissingImports = false }, { root = "packages/specfact-project/src", reportMissingImports = false, reportAttributeAccessIssue = false }, diff --git a/registry/index.json b/registry/index.json index 5736edc..afd7ff8 100644 --- a/registry/index.json +++ b/registry/index.json @@ -73,9 +73,10 @@ }, { "id": "nold-ai/specfact-code-review", - "latest_version": "0.44.3", - "download_url": "modules/specfact-code-review-0.44.3.tar.gz", - "checksum_sha256": "bc138f1c8da8c14b5da3a8f3f7de6cc0524ae784d833b8c3f6e49d574e7b205c", + "latest_version": "0.45.4", + "download_url": "modules/specfact-code-review-0.45.4.tar.gz", + "checksum_sha256": "54f2318ebe85546631b65786c9c77f04ede8629b6a2f8fcbda2664c4fb68f56c", + "core_compatibility": ">=0.40.0,<1.0.0", "tier": "official", "publisher": { "name": "nold-ai", diff --git a/registry/modules/specfact-code-review-0.45.1.tar.gz b/registry/modules/specfact-code-review-0.45.1.tar.gz new file mode 100644 index 0000000..cd80516 Binary files /dev/null and b/registry/modules/specfact-code-review-0.45.1.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.45.1.tar.gz.sha256 b/registry/modules/specfact-code-review-0.45.1.tar.gz.sha256 new file mode 100644 index 0000000..397d807 --- /dev/null +++ b/registry/modules/specfact-code-review-0.45.1.tar.gz.sha256 @@ -0,0 +1 @@ +72372145e9633d55c559f1efe4c0eb284d5814398b5bad837810dd69654f1dbb diff --git a/registry/modules/specfact-code-review-0.45.4.tar.gz b/registry/modules/specfact-code-review-0.45.4.tar.gz new file mode 100644 index 0000000..61a58e7 Binary files /dev/null and b/registry/modules/specfact-code-review-0.45.4.tar.gz differ diff --git a/registry/modules/specfact-code-review-0.45.4.tar.gz.sha256 b/registry/modules/specfact-code-review-0.45.4.tar.gz.sha256 new file mode 100644 index 0000000..c20de7d --- /dev/null +++ b/registry/modules/specfact-code-review-0.45.4.tar.gz.sha256 @@ -0,0 +1 @@ +54f2318ebe85546631b65786c9c77f04ede8629b6a2f8fcbda2664c4fb68f56c diff --git a/registry/signatures/specfact-code-review-0.45.1.tar.sig b/registry/signatures/specfact-code-review-0.45.1.tar.sig new file mode 100644 index 0000000..0808e0c --- /dev/null +++ b/registry/signatures/specfact-code-review-0.45.1.tar.sig @@ -0,0 +1 @@ +RNvYgAPLfFtV6ywXvs/9umIAyewZPbEZD+homAIt1+n4IwDFhwneEwqzpK7RlfvCnT0Rb3Xefa5ZMW7GwiWXBw== diff --git a/registry/signatures/specfact-code-review-0.45.4.tar.sig b/registry/signatures/specfact-code-review-0.45.4.tar.sig new file mode 100644 index 0000000..f26ba95 --- /dev/null +++ b/registry/signatures/specfact-code-review-0.45.4.tar.sig @@ -0,0 +1 @@ +8AadiIQUR3mxaxh0Mk6voN1zX94GeVLbL57ZUjTF//cqz8mQaOmd4Vz4/zEMVNRstYkQ4l9rrzays4C47tnpBQ== diff --git a/scripts/__init__.py b/scripts/__init__.py old mode 100644 new mode 100755 diff --git a/scripts/check-docs-commands.py b/scripts/check-docs-commands.py old mode 100644 new mode 100755 index a2a9c54..8cb7741 --- a/scripts/check-docs-commands.py +++ b/scripts/check-docs-commands.py @@ -123,11 +123,10 @@ def _iter_inline_examples(text: str, source: Path) -> list[CommandExample]: return examples -def _extract_command_examples(path: Path) -> list[CommandExample]: - text = path.read_text(encoding="utf-8") +def _extract_command_examples_from_text(text: str, source: Path) -> list[CommandExample]: seen: set[tuple[int, str]] = set() examples: list[CommandExample] = [] - for example in [*_iter_bash_examples(text, path), *_iter_inline_examples(text, path)]: + for example in [*_iter_bash_examples(text, source), *_iter_inline_examples(text, source)]: key = (example.line_number, example.text) if key in seen: continue @@ -136,6 +135,11 @@ def _extract_command_examples(path: Path) -> list[CommandExample]: return examples +def _extract_command_examples(path: Path, *, text: str | None = None) -> list[CommandExample]: + content = text or path.read_text(encoding="utf-8") + return _extract_command_examples_from_text(content, path) + + def _load_docs_texts(paths: list[Path]) -> dict[Path, str]: return {path: path.read_text(encoding="utf-8") for path in paths} @@ -197,12 +201,7 @@ def _command_example_is_valid(command_text: str, valid_paths: set[CommandPath]) def _validate_command_examples(text_by_path: dict[Path, str], valid_paths: set[CommandPath]) -> list[ValidationFinding]: findings: list[ValidationFinding] = [] for path, text in text_by_path.items(): - seen: set[tuple[int, str]] = set() - for example in [*_iter_bash_examples(text, path), *_iter_inline_examples(text, path)]: - key = (example.line_number, example.text) - if key in seen: - continue - seen.add(key) + for example in _extract_command_examples_from_text(text, path): if _command_example_is_valid(example.text, valid_paths): continue findings.append( diff --git a/scripts/pre_commit_code_review.py b/scripts/pre_commit_code_review.py new file mode 100755 index 0000000..11815f5 --- /dev/null +++ b/scripts/pre_commit_code_review.py @@ -0,0 +1,204 @@ +"""Run specfact code review as a staged-file pre-commit gate (modules repo). + +Writes a machine-readable JSON report to ``.specfact/code-review.json`` (gitignored) +so IDEs and Copilot can read findings; exit code still reflects the governed CI verdict. + +If ``specfact_cli`` is not installed, attempts ``hatch run dev-deps`` / ``ensure_core_dependency`` +(sibling ``specfact-cli`` checkout) before failing. +""" + +# CrossHair: ignore +# This helper shells out to the CLI and is intentionally side-effecting. + +from __future__ import annotations + +import importlib +import json +import subprocess +import sys +from collections.abc import Sequence +from pathlib import Path +from subprocess import TimeoutExpired +from typing import Any, cast + +from icontract import ensure, require + +from specfact_cli_modules.dev_bootstrap import ensure_core_dependency + + +PYTHON_SUFFIXES = {".py", ".pyi"} + +# Default matches dogfood / OpenSpec: machine-readable report under ignored ``.specfact/``. +REVIEW_JSON_OUT = ".specfact/code-review.json" + + +@require(lambda paths: paths is not None) +@ensure(lambda result: len(result) == len(set(result))) +@ensure(lambda result: all(Path(path).suffix.lower() in PYTHON_SUFFIXES for path in result)) +def filter_review_files(paths: Sequence[str]) -> list[str]: + """Return only staged Python source files relevant to code review.""" + seen: set[str] = set() + filtered: list[str] = [] + for path in paths: + if Path(path).suffix.lower() not in PYTHON_SUFFIXES: + continue + if path in seen: + continue + seen.add(path) + filtered.append(path) + return filtered + + +@require(lambda files: files is not None) +@ensure(lambda result: result[:5] == [sys.executable, "-m", "specfact_cli.cli", "code", "review"]) +@ensure(lambda result: "--json" in result and "--out" in result) +@ensure(lambda result: REVIEW_JSON_OUT in result) +def build_review_command(files: Sequence[str]) -> list[str]: + """Build ``code review run --json --out …`` so findings are written for tooling.""" + return [ + sys.executable, + "-m", + "specfact_cli.cli", + "code", + "review", + "run", + "--json", + "--out", + REVIEW_JSON_OUT, + *files, + ] + + +def _repo_root() -> Path: + """Repository root (parent of ``scripts/``).""" + return Path(__file__).resolve().parents[1] + + +def count_findings_by_severity(findings: list[object]) -> dict[str, int]: + """Bucket review findings by severity (unknown severities go to ``other``).""" + buckets = {"error": 0, "warning": 0, "advisory": 0, "info": 0, "other": 0} + for item in findings: + if not isinstance(item, dict): + buckets["other"] += 1 + continue + row = cast(dict[str, Any], item) + raw = row.get("severity") + if not isinstance(raw, str): + buckets["other"] += 1 + continue + key = raw.lower().strip() + if key in ("error", "err"): + buckets["error"] += 1 + elif key in ("warning", "warn"): + buckets["warning"] += 1 + elif key in ("advisory", "advise"): + buckets["advisory"] += 1 + elif key == "info": + buckets["info"] += 1 + else: + buckets["other"] += 1 + return buckets + + +def _print_review_findings_summary(repo_root: Path) -> None: + """Parse ``REVIEW_JSON_OUT`` and print a one-line findings count (errors / warnings / etc.).""" + report_path = repo_root / REVIEW_JSON_OUT + if not report_path.is_file(): + sys.stderr.write(f"Code review: no report file at {REVIEW_JSON_OUT} (could not print findings summary).\n") + return + try: + data = json.loads(report_path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError) as exc: + sys.stderr.write(f"Code review: could not read {REVIEW_JSON_OUT}: {exc}\n") + return + except json.JSONDecodeError as exc: + sys.stderr.write(f"Code review: invalid JSON in {REVIEW_JSON_OUT}: {exc}\n") + return + + findings_raw = data.get("findings") + if not isinstance(findings_raw, list): + sys.stderr.write(f"Code review: report has no findings list in {REVIEW_JSON_OUT}.\n") + return + + counts = count_findings_by_severity(findings_raw) + total = len(findings_raw) + verdict = data.get("overall_verdict", "?") + parts = [ + f"errors={counts['error']}", + f"warnings={counts['warning']}", + f"advisory={counts['advisory']}", + ] + if counts["info"]: + parts.append(f"info={counts['info']}") + if counts["other"]: + parts.append(f"other={counts['other']}") + summary = ", ".join(parts) + sys.stderr.write(f"Code review summary: {total} finding(s) ({summary}); overall_verdict={verdict!r}.\n") + abs_report = report_path.resolve() + sys.stderr.write(f"Code review report file: {REVIEW_JSON_OUT}\n") + sys.stderr.write(f" absolute path: {abs_report}\n") + sys.stderr.write("Copy-paste for Copilot or Cursor:\n") + sys.stderr.write( + f" Read `{REVIEW_JSON_OUT}` and fix every finding (errors first), using file and line from each entry.\n" + ) + sys.stderr.write(f" @workspace Open `{REVIEW_JSON_OUT}` and remediate each item in `findings`.\n") + + +@ensure(lambda result: isinstance(result, tuple) and len(result) == 2) +@ensure(lambda result: isinstance(result[0], bool) and (result[1] is None or isinstance(result[1], str))) +def ensure_runtime_available() -> tuple[bool, str | None]: + """Verify the current Python environment can import SpecFact CLI; try local sibling install.""" + try: + importlib.import_module("specfact_cli.cli") + except ModuleNotFoundError: + root = _repo_root() + if ensure_core_dependency(root) != 0: + return ( + False, + "Could not install local specfact-cli. Run `hatch run dev-deps` or set SPECFACT_CLI_REPO.", + ) + try: + importlib.import_module("specfact_cli.cli") + except ModuleNotFoundError: + return ( + False, + "specfact_cli still not importable after ensure_core_dependency; check sibling checkout.", + ) + return True, None + + +@ensure(lambda result: isinstance(result, int)) +def main(argv: Sequence[str] | None = None) -> int: + """Run the code review gate; write JSON under ``.specfact/`` and return CLI exit code.""" + files = filter_review_files(list(argv or [])) + if len(files) == 0: + sys.stdout.write("No staged Python files to review; skipping code review gate.\n") + return 0 + + available, guidance = ensure_runtime_available() + if available is False: + sys.stdout.write(f"Unable to run the code review gate. {guidance}\n") + return 1 + + cmd = build_review_command(files) + try: + result = subprocess.run( + cmd, + check=False, + text=True, + capture_output=True, + cwd=str(_repo_root()), + timeout=300, + ) + except TimeoutExpired: + joined_cmd = " ".join(cmd) + sys.stderr.write(f"Code review gate timed out after 300s (command: {joined_cmd!r}, files: {files!r}).\n") + return 1 + # Do not echo nested `specfact code review run` stdout/stderr (verbose tool banners and runner + # spam); the report is in REVIEW_JSON_OUT and we print a short summary below. + _print_review_findings_summary(_repo_root()) + return result.returncode + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv[1:])) diff --git a/skills/specfact-code-review/SKILL.md b/skills/specfact-code-review/SKILL.md index c90142c..e479475 100644 --- a/skills/specfact-code-review/SKILL.md +++ b/skills/specfact-code-review/SKILL.md @@ -4,29 +4,32 @@ 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/scripts/test_pre_commit_code_review.py b/tests/unit/scripts/test_pre_commit_code_review.py new file mode 100644 index 0000000..3be55fd --- /dev/null +++ b/tests/unit/scripts/test_pre_commit_code_review.py @@ -0,0 +1,200 @@ +"""Tests for scripts/pre_commit_code_review.py.""" + +# pyright: reportUnknownMemberType=false + +from __future__ import annotations + +import importlib.util +import json +import subprocess +import sys +from pathlib import Path +from typing import Any + +import pytest + + +def _load_script_module() -> Any: + """Load scripts/pre_commit_code_review.py as a Python module.""" + script_path = Path(__file__).resolve().parents[3] / "scripts" / "pre_commit_code_review.py" + spec = importlib.util.spec_from_file_location("pre_commit_code_review", script_path) + if spec is None or spec.loader is None: + raise AssertionError(f"Unable to load script module at {script_path}") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_filter_review_files_keeps_only_python_sources() -> None: + """Only relevant staged Python files should be reviewed.""" + module = _load_script_module() + + assert module.filter_review_files(["src/app.py", "README.md", "tests/test_app.py", "notes.txt"]) == [ + "src/app.py", + "tests/test_app.py", + ] + + +def test_build_review_command_writes_json_report() -> None: + """Pre-commit gate should write ReviewReport JSON for IDE/Copilot and use exit verdict.""" + module = _load_script_module() + + command = module.build_review_command(["src/app.py", "tests/test_app.py"]) + + assert command[:5] == [sys.executable, "-m", "specfact_cli.cli", "code", "review"] + assert "--json" in command + assert "--out" in command + assert module.REVIEW_JSON_OUT in command + assert command[-2:] == ["src/app.py", "tests/test_app.py"] + + +def test_main_skips_when_no_relevant_files(capsys: pytest.CaptureFixture[str]) -> None: + """Hook should not fail commits when no staged Python files are present.""" + module = _load_script_module() + + exit_code = module.main(["README.md", "docs/guide.md"]) + + assert exit_code == 0 + assert "No staged Python files" in capsys.readouterr().out + + +def test_main_propagates_review_gate_exit_code( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Blocking review verdicts must block the commit by returning non-zero.""" + module = _load_script_module() + repo_root = tmp_path + _write_sample_review_report( + repo_root, + { + "overall_verdict": "FAIL", + "findings": [ + {"severity": "error", "rule": "e1"}, + {"severity": "warning", "rule": "w1"}, + ], + }, + ) + + def _fake_root() -> Path: + return repo_root + + def _fake_ensure() -> tuple[bool, str | None]: + return True, None + + def _fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + assert "--json" in cmd + assert module.REVIEW_JSON_OUT in cmd + assert kwargs.get("cwd") == str(repo_root) + assert kwargs.get("timeout") == 300 + return subprocess.CompletedProcess(cmd, 1, stdout=".specfact/code-review.json\n", stderr="") + + monkeypatch.setattr(module, "_repo_root", _fake_root) + monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) + monkeypatch.setattr(module.subprocess, "run", _fake_run) + + exit_code = module.main(["src/app.py"]) + + assert exit_code == 1 + captured = capsys.readouterr() + assert captured.out == "" + err = captured.err + assert "Code review summary: 2 finding(s)" in err + assert "errors=1" in err + assert "warnings=1" in err + assert "overall_verdict='FAIL'" in err + assert "Code review report file:" in err + assert "absolute path:" in err + assert "Copy-paste for Copilot or Cursor:" in err + assert "Read `.specfact/code-review.json`" in err + assert "@workspace Open `.specfact/code-review.json`" in err + + +def _write_sample_review_report(repo_root: Path, payload: dict[str, object]) -> None: + spec_dir = repo_root / ".specfact" + spec_dir.mkdir(parents=True, exist_ok=True) + (spec_dir / "code-review.json").write_text(json.dumps(payload), encoding="utf-8") + + +def test_count_findings_by_severity_buckets_unknown() -> None: + """Severities map to error/warning/advisory; others go to other.""" + module = _load_script_module() + counts = module.count_findings_by_severity( + [ + {"severity": "error"}, + {"severity": "WARN"}, + {"severity": "advisory"}, + {"severity": "info"}, + {"severity": "custom"}, + "not-a-dict", + ] + ) + assert counts == {"error": 1, "warning": 1, "advisory": 1, "info": 1, "other": 2} + + +def test_main_missing_report_still_returns_exit_code_and_warns( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """If JSON is not on disk, stderr explains; exit code still comes from the review subprocess.""" + module = _load_script_module() + + def _fake_root() -> Path: + return tmp_path + + def _fake_ensure() -> tuple[bool, str | None]: + return True, None + + def _fake_run(cmd: list[str], **_kwargs: object) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess(cmd, 2, stdout="", stderr="") + + monkeypatch.setattr(module, "_repo_root", _fake_root) + monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) + monkeypatch.setattr(module.subprocess, "run", _fake_run) + + exit_code = module.main(["src/app.py"]) + + assert exit_code == 2 + err = capsys.readouterr().err + assert "no report file" in err + assert ".specfact/code-review.json" in err + + +def test_main_timeout_fails_hook(monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str]) -> None: + """Subprocess timeout must fail the hook with a clear message.""" + module = _load_script_module() + repo_root = Path(__file__).resolve().parents[3] + + def _fake_ensure() -> tuple[bool, str | None]: + return True, None + + def _fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[str]: + assert kwargs.get("cwd") == str(repo_root) + assert kwargs.get("timeout") == 300 + raise subprocess.TimeoutExpired(cmd, 300) + + monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) + monkeypatch.setattr(module.subprocess, "run", _fake_run) + monkeypatch.setattr(module, "_repo_root", lambda: repo_root) + + exit_code = module.main(["src/app.py"]) + + assert exit_code == 1 + err = capsys.readouterr().err + assert "timed out after 300s" in err + assert "src/app.py" in err + + +def test_main_prints_actionable_setup_guidance_when_runtime_missing( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """Missing review runtime should fail with actionable setup guidance.""" + module = _load_script_module() + + def _fake_ensure() -> tuple[bool, str | None]: + return False, "Run `hatch run dev-deps` to install specfact-cli." + + monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) + + exit_code = module.main(["src/app.py"]) + + assert exit_code == 1 + assert "dev-deps" in capsys.readouterr().out diff --git a/tests/unit/specfact_code_review/__init__.py b/tests/unit/specfact_code_review/__init__.py deleted file mode 100644 index a035a7b..0000000 --- a/tests/unit/specfact_code_review/__init__.py +++ /dev/null @@ -1 +0,0 @@ -"""Tests for specfact_code_review.""" diff --git a/tests/unit/specfact_code_review/review/test_commands.py b/tests/unit/specfact_code_review/review/test_commands.py index f72417b..24e4aff 100644 --- a/tests/unit/specfact_code_review/review/test_commands.py +++ b/tests/unit/specfact_code_review/review/test_commands.py @@ -25,7 +25,7 @@ def _fake_run_command(files: list[Path], **kwargs: object) -> tuple[int, str | N assert result.exit_code == 0 assert "Include changed and untracked test files in this review?" in result.output - assert recorded["files"] is None + assert recorded["files"] == [] assert recorded["kwargs"]["include_tests"] is True diff --git a/tests/unit/specfact_code_review/rules/test_updater.py b/tests/unit/specfact_code_review/rules/test_updater.py index 233afac..39d273b 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 = [ @@ -73,9 +74,11 @@ def _skill_text( f"Updated: {updated_on} | Module: nold-ai/specfact-code-review", "", "## DO", + "", *do_rules, "", "## DON'T", + "", *dont_rules, "", "## TOP VIOLATIONS (auto-updated by specfact code review rules update)", diff --git a/tests/unit/specfact_code_review/run/test_commands.py b/tests/unit/specfact_code_review/run/test_commands.py index 66246ed..8da804a 100644 --- a/tests/unit/specfact_code_review/run/test_commands.py +++ b/tests/unit/specfact_code_review/run/test_commands.py @@ -87,7 +87,7 @@ def test_run_command_score_only_prints_reward_delta(monkeypatch: Any) -> None: result = runner.invoke(app, ["review", "run", "--score-only", "tests/fixtures/review/clean_module.py"]) assert result.exit_code == 0 - assert result.output == "12\n" + assert result.output == "92\n" def test_run_command_uses_git_diff_when_files_are_omitted(monkeypatch: Any, tmp_path: Path) -> None: 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..01df32e 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,47 @@ 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_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", @@ -256,6 +320,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 +374,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 +462,26 @@ 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/review/__init__.py") + + findings, coverage_by_source = _coverage_findings([source_file], {"files": {}}) + + assert not findings + assert coverage_by_source == {} + + +def test_coverage_findings_does_not_skip_non_empty_package_initializers() -> 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 len(findings) == 1 + assert findings[0].category == "tool_error" + assert "Coverage data missing" in findings[0].message + assert coverage_by_source is None + + def test_run_pytest_with_coverage_disables_global_fail_under(monkeypatch: MonkeyPatch) -> None: recorded: dict[str, object] = {} @@ -410,18 +496,15 @@ def _fake_run(command: list[str], **kwargs: object) -> subprocess.CompletedProce command = recorded["command"] assert isinstance(command, list) - assert command[:3] == [_pytest_python_executable(), "-m", "pytest"] + assert command[0] == _pytest_python_executable() + assert command[1] == "-c" + assert "import specfact_code_review" in command[2] + assert "--import-mode=importlib" in command assert "--cov-fail-under=0" in command -def test_pytest_python_executable_prefers_local_venv(monkeypatch: MonkeyPatch, tmp_path: Path) -> None: - monkeypatch.chdir(tmp_path) - venv_python = tmp_path / ".venv/bin/python" - venv_python.parent.mkdir(parents=True) - venv_python.write_text("#!/bin/sh\n", encoding="utf-8") - venv_python.chmod(0o755) - - assert _pytest_python_executable() == str(venv_python.resolve()) +def test_pytest_python_executable_uses_current_interpreter() -> None: + assert _pytest_python_executable() == sys.executable def test_pytest_targets_collapse_multi_file_batch_to_common_test_directory() -> None: @@ -457,8 +540,8 @@ def _fake_run(command: list[str], **kwargs: object) -> subprocess.CompletedProce env = kwargs["env"] assert isinstance(env, dict) assert env["PYTHONPATH"].split(os.pathsep) == [ - str(workspace_root.resolve()), str(Path("packages/specfact-code-review/src").resolve()), + str(workspace_root.resolve()), str(tmp_path / "existing"), str(bundle_root.resolve()), ] diff --git a/tests/unit/specfact_code_review/test___init__.py b/tests/unit/specfact_code_review/test___init__.py new file mode 100644 index 0000000..d12e778 --- /dev/null +++ b/tests/unit/specfact_code_review/test___init__.py @@ -0,0 +1,46 @@ +"""Test for specfact_code_review.__init__ module.""" + +from __future__ import annotations + +import importlib.util + + +# pylint: disable=import-outside-toplevel + + +def test_all_exports() -> None: + """Test that __all__ contains expected exports.""" + import specfact_code_review + + assert isinstance(specfact_code_review.__all__, tuple) + assert len(specfact_code_review.__all__) > 0 + assert "app" in specfact_code_review.__all__ + assert "export_from_bundle" in specfact_code_review.__all__ + assert "import_to_bundle" in specfact_code_review.__all__ + assert "sync_with_bundle" in specfact_code_review.__all__ + assert "validate_bundle" in specfact_code_review.__all__ + + +def test_getattr_raises_for_invalid_attribute() -> None: + """Test that __getattr__ raises AttributeError for invalid attributes.""" + # Test that invalid attribute raises an error + spec = importlib.util.find_spec("specfact_code_review.invalid_attribute") + assert spec is None, "Invalid attribute should not be found" + + +def test_getattr_returns_valid_attributes() -> None: + """Test that __getattr__ returns valid attributes.""" + # Test that __getattr__ works by accessing an attribute + import specfact_code_review + + # Access the attribute to trigger __getattr__ + app = specfact_code_review.app + + # Just verify it doesn't raise an exception and returns something + assert app is not None + + # Clean up to avoid test pollution + import sys + + sys.modules.pop("specfact_code_review.review.app", None) + sys.modules.pop("specfact_code_review.review", None) diff --git a/tests/unit/specfact_code_review/test__review_utils.py b/tests/unit/specfact_code_review/test__review_utils.py index ba30d93..d886eb4 100644 --- a/tests/unit/specfact_code_review/test__review_utils.py +++ b/tests/unit/specfact_code_review/test__review_utils.py @@ -2,7 +2,7 @@ from pathlib import Path -from specfact_code_review._review_utils import _normalize_path_variants, _tool_error +from specfact_code_review._review_utils import normalize_path_variants, tool_error def test_normalize_path_variants_includes_relative_and_resolved_paths(tmp_path: Path) -> None: @@ -10,7 +10,7 @@ def test_normalize_path_variants_includes_relative_and_resolved_paths(tmp_path: file_path.parent.mkdir(parents=True) file_path.write_text("VALUE = 1\n", encoding="utf-8") - variants = _normalize_path_variants(file_path) + variants = normalize_path_variants(file_path) assert str(file_path.resolve()) in variants assert file_path.resolve().as_posix() in variants @@ -20,7 +20,7 @@ def test_tool_error_returns_review_finding_defaults(tmp_path: Path) -> None: file_path = tmp_path / "example.py" file_path.write_text("VALUE = 1\n", encoding="utf-8") - finding = _tool_error( + finding = tool_error( tool="pytest", file_path=file_path, message="Coverage data missing", diff --git a/tests/unit/specfact_code_review/tools/helpers.py b/tests/unit/specfact_code_review/tools/helpers.py index 944574a..9a8b30c 100644 --- a/tests/unit/specfact_code_review/tools/helpers.py +++ b/tests/unit/specfact_code_review/tools/helpers.py @@ -1,6 +1,7 @@ from __future__ import annotations import subprocess +from pathlib import Path from unittest.mock import Mock @@ -18,3 +19,22 @@ def assert_tool_run(run_mock: Mock, expected_command: list[str]) -> None: check=False, timeout=30, ) + + +def create_noisy_file(tmp_path: Path, *, name: str = "target.py", body_lines: int = 81) -> Path: + file_path = tmp_path / name + body = "\n".join(f" total += {index}" for index in range(body_lines)) + 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" + " if d:\n" + f"{body}\n" + " return total\n" + ), + encoding="utf-8", + ) + return file_path 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..554b20f --- /dev/null +++ b/tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py @@ -0,0 +1,118 @@ +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_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") + + 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_basedpyright_runner.py b/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py index c6a7c97..52ac4e6 100644 --- a/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py +++ b/tests/unit/specfact_code_review/tools/test_basedpyright_runner.py @@ -11,6 +11,10 @@ from tests.unit.specfact_code_review.tools.helpers import assert_tool_run, completed_process +def test_run_basedpyright_returns_empty_for_no_files() -> None: + assert run_basedpyright([]) == [] + + def test_run_basedpyright_maps_error_diagnostic_to_type_safety(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" payload = { @@ -104,3 +108,19 @@ def test_run_basedpyright_returns_tool_error_when_unavailable(tmp_path: Path, mo assert len(findings) == 1 assert findings[0].category == "tool_error" assert findings[0].tool == "basedpyright" + + +def test_run_basedpyright_returns_tool_error_for_invalid_diagnostic_payload( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + file_path = tmp_path / "target.py" + payload = {"generalDiagnostics": [{"file": str(file_path)}]} + monkeypatch.setattr( + subprocess, "run", Mock(return_value=completed_process("basedpyright", stdout=json.dumps(payload))) + ) + + findings = run_basedpyright([file_path]) + + assert len(findings) == 1 + assert findings[0].category == "tool_error" + assert findings[0].tool == "basedpyright" diff --git a/tests/unit/specfact_code_review/tools/test_pylint_runner.py b/tests/unit/specfact_code_review/tools/test_pylint_runner.py index 4b0a71c..b981f14 100644 --- a/tests/unit/specfact_code_review/tools/test_pylint_runner.py +++ b/tests/unit/specfact_code_review/tools/test_pylint_runner.py @@ -11,6 +11,10 @@ from tests.unit.specfact_code_review.tools.helpers import assert_tool_run, completed_process +def test_run_pylint_returns_empty_for_no_files() -> None: + assert run_pylint([]) == [] + + def test_run_pylint_maps_bare_except_to_architecture(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: file_path = tmp_path / "target.py" payload = [ @@ -91,3 +95,15 @@ def test_run_pylint_returns_tool_error_on_parse_error(tmp_path: Path, monkeypatc assert len(findings) == 1 assert findings[0].category == "tool_error" assert findings[0].tool == "pylint" + + +def test_run_pylint_returns_tool_error_for_invalid_payload_item(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = tmp_path / "target.py" + payload = [{"path": str(file_path), "line": 7, "message": "No exception type(s) specified"}] + monkeypatch.setattr(subprocess, "run", Mock(return_value=completed_process("pylint", stdout=json.dumps(payload)))) + + findings = run_pylint([file_path]) + + assert len(findings) == 1 + assert findings[0].category == "tool_error" + assert findings[0].tool == "pylint" 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..6a70133 100644 --- a/tests/unit/specfact_code_review/tools/test_radon_runner.py +++ b/tests/unit/specfact_code_review/tools/test_radon_runner.py @@ -8,7 +8,7 @@ from pytest import MonkeyPatch from specfact_code_review.tools.radon_runner import run_radon -from tests.unit.specfact_code_review.tools.helpers import assert_tool_run, completed_process +from tests.unit.specfact_code_review.tools.helpers import assert_tool_run, completed_process, create_noisy_file def test_run_radon_maps_complexity_thresholds_and_filters_files(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: @@ -63,3 +63,36 @@ 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 = create_noisy_file(tmp_path) + 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"} + + +def test_run_radon_uses_dedicated_tool_identifier_for_kiss_findings(tmp_path: Path, monkeypatch: MonkeyPatch) -> None: + file_path = create_noisy_file(tmp_path) + 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 70f0de1..9a0ef29 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" @@ -114,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 = { 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" diff --git a/tests/unit/test_pre_commit_quality_parity.py b/tests/unit/test_pre_commit_quality_parity.py index 7b3e145..47d5b9e 100644 --- a/tests/unit/test_pre_commit_quality_parity.py +++ b/tests/unit/test_pre_commit_quality_parity.py @@ -1,5 +1,6 @@ from __future__ import annotations +import itertools from pathlib import Path import yaml @@ -18,17 +19,36 @@ def test_pre_commit_config_has_signature_and_modules_quality_hooks() -> None: repos = config.get("repos") assert isinstance(repos, list) - hook_ids = { - hook.get("id") - for repo in repos - if isinstance(repo, dict) - for hook in repo.get("hooks", []) - if isinstance(hook, dict) - } - + hook_ids: set[str] = set() + ordered_hook_ids: list[str] = [] + seen: set[str] = set() + for repo in repos: + if not isinstance(repo, dict): + continue + for hook in repo.get("hooks", []): + if not isinstance(hook, dict): + continue + hook_id = hook.get("id") + if not isinstance(hook_id, str): + continue + hook_ids.add(hook_id) + if hook_id not in seen: + ordered_hook_ids.append(hook_id) + seen.add(hook_id) + + assert "specfact-code-review-gate" in hook_ids assert "verify-module-signatures" in hook_ids assert "modules-quality-checks" in hook_ids + expected_order = [ + "verify-module-signatures", + "modules-quality-checks", + "specfact-code-review-gate", + ] + index_map = {hook_id: index for index, hook_id in enumerate(ordered_hook_ids)} + for earlier, later in itertools.pairwise(expected_order): + assert index_map[earlier] < index_map[later] + def test_modules_pre_commit_script_enforces_required_quality_commands() -> None: script_path = REPO_ROOT / "scripts" / "pre-commit-quality-checks.sh"