Conversation
- Exclude .specfact/.git/__pycache__/node_modules from sidecar Python scans (codebase 0.41.5). - MISSING_ICONTRACT only when icontract is imported; CrossHair bug-hunt timeouts; Semgrep bugs pass. - Review CLI: shadow/enforce, focus facets, level filter, tool availability skips; Typer KISS exemption. - Pre-commit review: pass only .py/.pyi to SpecFact; block commits on error findings, not warning-only. - Registry tarballs and index checksums for bumped modules; OpenSpec tasks + TDD evidence. Made-with: Cursor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughBundle & module surface
Manifest & integrity
Cross-repo compatibility & specfact-cli alignment
Docs, registry, and release readiness
Cross-cutting behavioral changes & maintainer notes
OpenSpec / registry references
Recommended reviewer focus
WalkthroughAdds a “bug-hunt” scan pass and focus/level/shadow CLI controls, tool-availability short-circuiting, icontract AST-scan suppression, .specfact/venv exclusion for sidecar extractors, Semgrep bug rules, manifest/registry bumps, and supporting tests, docs, and CI/workflow script updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Runner as run_review
participant SemgrepClean as Semgrep(clean)
participant SemgrepBugs as Semgrep(bugs)
participant Contracts as ContractRunner
participant CrossHair as CrossHair
participant Tests as PyTest
participant Reporter as Reporter
rect rgba(200,200,255,0.5)
CLI->>Runner: invoke run_review(files, focus*, mode*, level*, bug_hunt*)
end
Runner->>SemgrepClean: run_semgrep(files)
SemgrepClean-->>Runner: findings (clean)
Runner->>SemgrepBugs: run_semgrep_bugs(files) (if config && not skipped)
SemgrepBugs-->>Runner: findings (bugs)
Runner->>Contracts: run_contract_check(files, bug_hunt=...)
Contracts->>CrossHair: invoke CrossHair (if available)
CrossHair-->>Contracts: findings / tool-skip warning
Contracts-->>Runner: contract findings
Runner->>Tests: run pytest (skip if pytest unavailable)
Tests-->>Runner: test findings / skip
Runner->>Reporter: aggregate findings, apply review_level filter
alt mode == "shadow"
Runner->>Reporter: set ci_exit_code = 0 (preserve verdict)
end
Reporter-->>CLI: write review-report.json + exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6db629a5ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
djm81
added
module
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
registry/index.json (1)
79-94:⚠️ Potential issue | 🔴 CriticalIntegrity checksum mismatch and missing module signature for 0.47.0.
The registry entry at lines 79-94 has inconsistencies that violate module signing requirements:
Checksum mismatch: The checksum in
index.json(42ea7d2d16c5b500787468d3aef529e7e7ac4d8e21ae2b3b7bd14c802256b0e8) does not match the integrity checksum declared inmodule-package.yaml(0631d016bbdab90f30ea7c1ebdc68407c964be3983aee03cabf3d38b58d42fa4).Missing signature file: No
.tar.sigfile exists for version 0.47.0 inregistry/signatures/, while all recent versions (0.46.1, 0.46.3, 0.46.4) have signatures. Module signing is required per repository policy.Ensure the module-package.yaml integrity checksum matches the tarball, and generate and commit the signature file for 0.47.0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/index.json` around lines 79 - 94, Update the registry entry and add the missing signature for nold-ai/specfact-code-review v0.47.0: recompute the SHA256 of modules/specfact-code-review-0.47.0.tar.gz and replace the checksum value in index.json (the "checksum_sha256" field for id "nold-ai/specfact-code-review") to match the integrity value recorded in module-package.yaml (or vice‑versa ensure module-package.yaml's integrity matches the tarball), then generate a detached signature for the tarball and commit the corresponding signatures entry (registry/signatures/*specfact-code-review-0.47.0.tar.gz.sig) so the release has a matching checksum and a signature file as required by the signing policy.packages/specfact-code-review/src/specfact_code_review/run/commands.py (2)
581-593:⚠️ Potential issue | 🟠 Major
--focus testsis pruned before facet selection.
_resolve_files(... include_tests=request.include_tests ...)strips tests during auto-scope, and the new focus filter runs only after that. As written,--focus testsonchangedorfullwill always fall into the empty-set error unless files are passed positionally. As per coding guidelines,packages/**/src/**/*.py: “Flag breaking assumptions about … environment/mode behavior.”💡 Proposed fix
- resolved_files = _resolve_files( + include_tests = request.include_tests or "tests" in request.focus_facets + resolved_files = _resolve_files( request.files, - include_tests=request.include_tests, + include_tests=include_tests, scope=request.scope, path_filters=request.path_filters or [], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/specfact-code-review/src/specfact_code_review/run/commands.py` around lines 581 - 593, The code prunes test files by passing request.include_tests into _resolve_files before applying _filter_files_by_focus, so using --focus tests on auto-scope modes yields no files; update the logic in the block that calls _resolve_files/_filter_files_by_focus so that tests are preserved when focus includes "tests" (either call _filter_files_by_focus earlier or ensure include_tests is forced true when "tests" is present in request.focus_facets)—modify the invocation around _resolve_files and the subsequent filtering to check request.focus_facets for "tests" and set include_tests=True (or reorder to filter by focus before excluding tests) while keeping the existing variables _resolve_files, _filter_files_by_focus, resolved_files, request.include_tests, and request.focus_facets.
475-513:⚠️ Potential issue | 🟠 MajorDo not treat
include_tests=Falseas a focus conflict.This guard keys off argument presence, not value. Any caller that explicitly passes
include_tests=Falsenow gets aValueError, and an adapter that forwards its defaultFalsewill make every focused run invalid. As per coding guidelines,packages/**/src/**/*.py: “Focus on adapter and bridge patterns… and clear boundaries so core upgrades do not silently break bundles.”💡 Proposed fix
- had_include_tests_key = "include_tests" in request_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 @@ - focus_facets = cast(tuple[str, ...], _as_focus_facets(request_kwargs.pop("focus_facets", None))) - if focus_facets and had_include_tests_key: - raise ValueError("Cannot combine focus_facets with include_tests; use --focus alone to scope files.") - # 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 + + focus_facets = cast(tuple[str, ...], _as_focus_facets(request_kwargs.pop("focus_facets", None))) + if focus_facets and include_tests: + raise ValueError("Cannot combine focus_facets with include_tests; use --focus alone to scope files.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/specfact-code-review/src/specfact_code_review/run/commands.py` around lines 475 - 513, The code treats presence of the include_tests key (had_include_tests_key) as a conflict with focus_facets even when include_tests was explicitly False; change the conflict check to only trigger when include_tests is truthy. Replace the condition "if focus_facets and had_include_tests_key:" with "if focus_facets and include_tests:" (and optionally remove the now-unused had_include_tests_key variable) so only include_tests=True conflicts with focus_facets; refer to the include_tests, had_include_tests_key and focus_facets symbols to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.md`:
- Around line 26-27: The wording incorrectly states that warnings may cause a
non-zero process exit; update the sentence referencing exit behavior so it
matches the shipped gate semantics: make it explicit that the quality gate exit
policy is "error-severity only" (warnings do not block), keep the scoped-check
result as "PASS_WITH_ADVISORY" with "ci_exit_code 0" and the report location
".specfact/code-review-touch.json"; replace the clause about warnings driving a
non-zero exit with a clear statement that only error-severity findings affect
the CI exit code.
In `@packages/specfact-code-review/.semgrep/bugs.yaml`:
- Around line 30-35: The semgrep rule id "specfact-bugs-yaml-unsafe" currently
flags yaml.load(...) but the codebase already uses yaml.safe_load()/safe_dump();
update the rule to proactively detect unsafe usages by matching yaml.load calls
that omit a Loader argument (e.g., pattern matching yaml.load(... , Loader=...)
vs yaml.load(...)) or alternatively suggest replacing yaml.load with
yaml.safe_load; modify the rule's pattern and message to explicitly target calls
missing the Loader parameter (pattern: yaml.load($ARGLIST) where no Loader=
present) and keep severity as WARNING so future unsafe additions are caught.
In `@packages/specfact-code-review/src/specfact_code_review/run/commands.py`:
- Around line 73-85: The facet matcher _matches_focus currently rejects
non-".py" files which drops ".pyi" stub files; update the initial suffix check
in _matches_focus to accept both ".py" and ".pyi" (e.g., check file_path.suffix
in (".py", ".pyi")) so stub files are considered for "tests", "docs", and
"source" branches; also ensure any downstream helpers referenced here
(_is_test_file, _is_docs_tree_file) operate correctly for .pyi files or adjust
their logic if they currently assume only .py extensions.
In
`@packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py`:
- Around line 17-45: Add a short inline comment explaining why "pytest" is
intentionally omitted from _EXECUTABLE_ON_PATH: note that ReviewToolId and
REVIEW_TOOL_PIP_PACKAGES include "pytest" but skip_if_tool_missing relies on
_EXECUTABLE_ON_PATH so would falsely report pytest as present, and instead
skip_if_pytest_unavailable uses importlib.util.find_spec() to check pytest
availability; place this comment near the _EXECUTABLE_ON_PATH definition and
reference the skip_if_tool_missing and skip_if_pytest_unavailable functions to
make the design decision clear for future maintainers.
In
`@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py`:
- Around line 36-42: The docstring for
BaseFrameworkExtractor._path_touches_excluded_dir claims "venv" is excluded but
_EXCLUDED_DIR_NAMES currently lacks 'venv' and '.venv', causing virtual env
folders to be scanned; update the frozenset _EXCLUDED_DIR_NAMES to include
'venv' and '.venv' (in the BaseFrameworkExtractor class) so
_path_touches_excluded_dir correctly skips those directories, and ensure the
docstring stays accurate to the updated exclusion set.
In
`@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py`:
- Around line 147-167: The _path_method_from_route_decorator currently treats
any decorator name/attribute as an HTTP method and only reads the first
positional arg for the path; modify it (and the similar logic around line ~179)
to (1) explicitly validate decorator names/attributes against known FastAPI
verbs (get, post, put, delete, patch, head, options) and handle the special
"api_route" name, (2) when func is "api_route" look for a "methods" keyword
argument (decorator.keywords) and extract each string literal method from that
list, normalizing to upper-case and validating against allowed verbs, (3)
continue to extract path using _extract_string_literal from either the first
positional arg or the "path"/"route" keyword if present, and (4) return the
resolved path plus a single canonical method or a representative method (e.g.,
first valid method) consistent with how routes are consumed elsewhere; ensure
invalid/unknown method names are ignored or logged and do not get returned as
methods.
In `@scripts/pre_commit_code_review.py`:
- Around line 315-324: The JSON report is currently parsed twice; modify
_print_review_findings_summary(report_path) to return the parsed severity counts
(call count_findings_by_severity on the findings and return the counts dict or
None on error), then in main() replace the second json.loads block with a call
to _print_review_findings_summary(report_path) and use its returned counts to
check counts["error"] == 0 and decide to return 0 or result.returncode; preserve
existing exception handling (OSError/UnicodeDecodeError/JSONDecodeError) and
fall back to returning result.returncode when parsing fails.
---
Outside diff comments:
In `@packages/specfact-code-review/src/specfact_code_review/run/commands.py`:
- Around line 581-593: The code prunes test files by passing
request.include_tests into _resolve_files before applying
_filter_files_by_focus, so using --focus tests on auto-scope modes yields no
files; update the logic in the block that calls
_resolve_files/_filter_files_by_focus so that tests are preserved when focus
includes "tests" (either call _filter_files_by_focus earlier or ensure
include_tests is forced true when "tests" is present in
request.focus_facets)—modify the invocation around _resolve_files and the
subsequent filtering to check request.focus_facets for "tests" and set
include_tests=True (or reorder to filter by focus before excluding tests) while
keeping the existing variables _resolve_files, _filter_files_by_focus,
resolved_files, request.include_tests, and request.focus_facets.
- Around line 475-513: The code treats presence of the include_tests key
(had_include_tests_key) as a conflict with focus_facets even when include_tests
was explicitly False; change the conflict check to only trigger when
include_tests is truthy. Replace the condition "if focus_facets and
had_include_tests_key:" with "if focus_facets and include_tests:" (and
optionally remove the now-unused had_include_tests_key variable) so only
include_tests=True conflicts with focus_facets; refer to the include_tests,
had_include_tests_key and focus_facets symbols to locate the change.
In `@registry/index.json`:
- Around line 79-94: Update the registry entry and add the missing signature for
nold-ai/specfact-code-review v0.47.0: recompute the SHA256 of
modules/specfact-code-review-0.47.0.tar.gz and replace the checksum value in
index.json (the "checksum_sha256" field for id "nold-ai/specfact-code-review")
to match the integrity value recorded in module-package.yaml (or vice‑versa
ensure module-package.yaml's integrity matches the tarball), then generate a
detached signature for the tarball and commit the corresponding signatures entry
(registry/signatures/*specfact-code-review-0.47.0.tar.gz.sig) so the release has
a matching checksum and a signature file as required by the signing policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48a3202d-3541-456d-b945-d9afdf05e37c
⛔ Files ignored due to path filters (2)
registry/modules/specfact-code-review-0.47.0.tar.gzis excluded by!**/*.gzregistry/modules/specfact-codebase-0.41.5.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (34)
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mdpackages/specfact-code-review/.semgrep/bugs.yamlpackages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/run/__init__.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pypackages/specfact-code-review/src/specfact_code_review/run/runner.pypackages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/tool_availability.pypackages/specfact-codebase/module-package.yamlpackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.pyregistry/index.jsonregistry/modules/specfact-code-review-0.47.0.tar.gz.sha256registry/modules/specfact-codebase-0.41.5.tar.gz.sha256scripts/pre_commit_code_review.pytests/cli-contracts/specfact-code-review-run.scenarios.yamltests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.pytests/unit/specfact_code_review/run/test___init__.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/test_review_tool_pip_manifest.pytests/unit/specfact_code_review/tools/test_contract_runner.pytests/unit/specfact_code_review/tools/test_tool_availability.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
tests/unit/specfact_code_review/run/test___init__.pypackages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.pypackages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pytests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.pypackages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.pytests/unit/specfact_code_review/test_review_tool_pip_manifest.pypackages/specfact-code-review/src/specfact_code_review/run/__init__.pypackages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pytests/unit/specfact_code_review/run/test_runner.pyscripts/pre_commit_code_review.pytests/unit/specfact_code_review/tools/test_tool_availability.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_code_review/tools/test_contract_runner.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.pypackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/run/runner.pypackages/specfact-code-review/src/specfact_code_review/tools/tool_availability.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/run/commands.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/specfact_code_review/run/test___init__.pytests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.pytests/unit/specfact_code_review/test_review_tool_pip_manifest.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/tools/test_tool_availability.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_code_review/tools/test_contract_runner.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: 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.
Files:
packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.pypackages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.pypackages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.pypackages/specfact-code-review/src/specfact_code_review/run/__init__.pypackages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.pypackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/run/runner.pypackages/specfact-code-review/src/specfact_code_review/tools/tool_availability.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/run/commands.py
registry/**
⚙️ CodeRabbit configuration file
registry/**: Registry and index consistency: bundle listings, version pins, and compatibility with
published module artifacts.
Files:
registry/modules/specfact-code-review-0.47.0.tar.gz.sha256registry/modules/specfact-codebase-0.41.5.tar.gz.sha256registry/index.json
packages/**/module-package.yaml
⚙️ CodeRabbit configuration file
packages/**/module-package.yaml: Validate metadata: name, version, commands, dependencies, and parity with packaged src.
Call out semver and signing implications when manifests or payloads change.
Files:
packages/specfact-code-review/module-package.yamlpackages/specfact-codebase/module-package.yaml
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/pre_commit_code_review.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Applied to files:
tests/unit/specfact_code_review/run/test___init__.pyregistry/modules/specfact-code-review-0.47.0.tar.gz.sha256packages/specfact-code-review/.semgrep/bugs.yamlpackages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/run/__init__.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdtests/unit/specfact_code_review/run/test_runner.pyscripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pytests/cli-contracts/specfact-code-review-run.scenarios.yamlpackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/run/runner.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mdregistry/index.jsonpackages/specfact-code-review/src/specfact_code_review/run/commands.py
📚 Learning: 2026-04-02T21:49:11.371Z
Learnt from: djm81
Repo: nold-ai/specfact-cli-modules PR: 136
File: registry/modules/specfact-spec-0.40.17.tar.gz.sha256:1-1
Timestamp: 2026-04-02T21:49:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Applied to files:
registry/modules/specfact-code-review-0.47.0.tar.gz.sha256registry/modules/specfact-codebase-0.41.5.tar.gz.sha256packages/specfact-code-review/module-package.yamlpackages/specfact-codebase/module-package.yamlregistry/index.json
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
registry/modules/specfact-code-review-0.47.0.tar.gz.sha256packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pypackages/specfact-code-review/.semgrep/bugs.yamlpackages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.pypackages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/run/__init__.pytests/unit/specfact_code_review/run/test_runner.pyscripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pytests/cli-contracts/specfact-code-review-run.scenarios.yamlpackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/run/runner.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mdpackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/run/commands.py
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
packages/specfact-code-review/module-package.yaml
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Perform `spec -> tests -> failing evidence -> code -> passing evidence` in that order for behavior changes
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md
📚 Learning: 2026-04-13T10:38:15.842Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-13T10:38:15.842Z
Learning: Adhere to worktree policy, OpenSpec gating, GitHub hierarchy-cache refresh, TDD order, quality gates, versioning, and documentation rules as defined in `docs/agent-rules/`
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdscripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pytests/cli-contracts/specfact-code-review-run.scenarios.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mdpackages/specfact-code-review/src/specfact_code_review/run/commands.py
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Treat the clean-code compliance gate as mandatory: the review surface enforces `naming`, `kiss`, `yagni`, `dry`, and `solid` categories and blocks regressions
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Treat clean-code regressions (naming, kiss, yagni, dry, solid violations) as blocking until they are fixed or explicitly justified
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md
🔇 Additional comments (44)
tests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.py (1)
1-5: LGTM!Clean fixture that correctly triggers the
_has_icontract_usage()gate via the aliased import while providing a public function without contract decorators for AST-based finding generation. The suppression comments are appropriate.packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py (1)
44-51: LGTM!The iterator correctly guards against non-existent or non-directory roots and filters out excluded paths. The
beartypedecorator ensures runtime type safety at the adapter boundary.packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.py (1)
41-47: LGTM!Clean refactor to use the inherited
_iter_python_files()iterator for consistent directory exclusion. The requirements file checks correctly remain as direct path access since they target known specific files.packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.py (1)
41-42: LGTM!The refactor correctly uses the shared iterator with filename filtering. While iterating all
.pyfiles then filtering forurls.pyis slightly less efficient than a targeted glob, the consistency with other extractors and the exclusion behavior outweigh this minor overhead.packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py (3)
51-58: LGTM!Consistent refactor to use
_iter_python_files()for directory exclusion in thedetect()method.
82-87: LGTM!Consistent refactor to use
_iter_python_files()for directory exclusion inextract_routes().
110-127: LGTM!Well-factored helper that cleanly handles both direct (
Flask(...)) and qualified (flask.Flask(...)) import patterns for Flask app and Blueprint detection. The in-place set mutation is appropriate given the caller-owned context.packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py (2)
56-71: LGTM!The function correctly uses
shutil.which()to detect CLI availability and emits a well-formatted warning finding with installation guidance referencing the module package declaration.
74-103: LGTM!Correct use of
importlib.util.find_spec()to check pytest availability without import side effects. The sequential checks forpytestthenpytest_covwith clear actionable messages provide good developer experience.packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py (1)
95-98: LGTM!Clean integration of the preflight availability check. The early return pattern is consistent with other tool runners, and
OSErrorin the exception handler still coversFileNotFoundErroras a subclass.packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py (3)
185-190: LGTM!Clean integration of the Typer exemption at the start of the parameter counting logic. This allows Typer command callbacks with many injected option parameters to pass the KISS gate.
297-300: LGTM!Consistent preflight availability check pattern, matching the integration in other tool runners.
167-182: Typer CLI exemption is sound; AttributeError catch is unnecessary but harmless.The exemption correctly identifies Typer command callbacks by checking for a first parameter named
ctxwith an annotation ending inContext. This matches the actual usage pattern in the codebase (ctx: typer.Context) and aligns with the documented "Typer KISS exemption."The
AttributeErrorcatch onast.unparse()is unnecessary—the project requires Python 3.11 minimum, andast.unparse()has been available since Python 3.9. Removing it would slightly simplify the code, though keeping it as defensive safety is harmless.The implementation maintains clean adapter boundaries and doesn't introduce breaking assumptions about tool availability or registry behavior.
registry/modules/specfact-code-review-0.47.0.tar.gz.sha256 (1)
1-1: Covered by shared registry checksum verification comment.This line is included in the combined tarball/index parity verification already requested.
packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py (1)
109-123: Good guard-before-exec pattern and stable error mapping.Line 109-Line 111 cleanly short-circuit when
pylintis unavailable, and Line 122 still handles process/parse failures viaOSErrorwithout regressing behavior.packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py (1)
103-117: Runner behavior is now consistent with availability-first orchestration.Line 103-Line 105 correctly avoids invoking Ruff when unavailable, and Line 116 preserves robust failure-to-finding conversion.
tests/unit/specfact_code_review/run/test___init__.py (1)
8-14: Useful API-surface smoke coverage for lazy exports.Line 8-Line 14 provides a minimal but effective guard around
run_reviewexport availability and__all__consistency.openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md (1)
3-57: OpenSpec task-state update is coherent with the implemented workstreams.Marking these items complete keeps proposal/tasks parity clear for the shipped module behavior and release artifacts.
As per coding guidelines,
openspec/**/*.mdmust maintain spec truth and avoid drift versus shipped module behavior and docs.packages/specfact-codebase/module-package.yaml (1)
2-27: Version and integrity metadata moved in lockstep.Line 2 and Line 27 update semver and checksum coherently for the new module payload.
registry/modules/specfact-codebase-0.41.5.tar.gz.sha256 (1)
1-1: Checksum parity verified and synchronized.The .sha256 sidecar for specfact-codebase-0.41.5 is correct and in sync. Actual tarball checksum matches the declared hash (fe8f95c325f21eb80209aa067f6a4f2055f1f5feed4e818a1c9d3061320c2270), and the registry/index.json entry for this module contains the same checksum with proper version pin and download URL alignment.
packages/specfact-code-review/module-package.yaml (1)
25-26: Checksum-only integrity metadata is correct for this release path.Integrity block with
checksumand withoutsignatureis fully supported by the verification pipeline. Signature fields are auto-generated by the GitHub Actions publish workflow and are not committed locally to branch PRs. Module manifests on non-main branches require only the checksum field, which matches the current state of your module-package.yaml.tests/unit/specfact_code_review/test_review_tool_pip_manifest.py (1)
16-21: Good manifest-boundary guard for tool dependencies.This test closes a real drift risk between runtime tool mapping and
module-package.yamlpackaging metadata.tests/cli-contracts/specfact-code-review-run.scenarios.yaml (1)
58-131: CLI contract coverage expansion looks solid.These scenarios materially improve regression protection for the new mode/focus/level controls and conflict handling.
packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py (1)
51-52: Good use of shared Python-file iterator for sidecar traversal.Switching to
self._iter_python_files(...)is the right boundary-safe fix for excluding sidecar-managed dirs like.specfact/venv.Also applies to: 82-83
packages/specfact-code-review/src/specfact_code_review/run/__init__.py (1)
27-40: Public run API expansion is clean and boundary-safe.Defaults preserve compatibility, and forwarding keeps orchestration centralized in
runner.tests/unit/specfact_code_review/run/test_runner.py (1)
66-95: Runner test updates are well-aligned with the new orchestration graph.Order assertions and kwargs-tolerant contract stubs give good protection against regressions in the expanded run pipeline.
Also applies to: 107-110, 163-167
tests/unit/scripts/test_pre_commit_code_review.py (1)
37-50: These pre-commit tests capture the new gate contract well.Great coverage for both Python-only path forwarding and advisory-only (warning) non-blocking behavior.
Also applies to: 109-139
tests/unit/specfact_codebase/test_sidecar_framework_extractors.py (1)
1-43: LGTM! Clean test coverage for sidecar path exclusion.This test properly validates that
FastAPIExtractorrespects the_EXCLUDED_DIR_NAMESset inBaseFrameworkExtractor, ensuring routes under.specfact/venv/...are filtered out. The test structure withtmp_pathis deterministic and the assertions directly verify the intended exclusion behavior.tests/unit/specfact_code_review/tools/test_tool_availability.py (2)
35-44: Solid manifest validation test.This test ensures
REVIEW_TOOL_PIP_PACKAGESstays in sync with the supported tool identifiers. If a new tool runner is added, this test will catch the missing entry.
28-32: Patch target is correct; no changes needed.The test correctly patches
"importlib.util.find_spec"becauseskip_if_pytest_unavailable()calls it via the full module path (importlib.util.find_spec(...)), not through a local import. This is the standard and proper way to mock functions invoked with their full module path.scripts/pre_commit_code_review.py (1)
86-96: Python-only filtering aligns with SpecFact code review scope.This change correctly narrows the pre-commit gate to Python sources (
.py/.pyi), which matches what the underlying SpecFact code review tooling actually analyzes. Non-Python files (Markdown, YAML, registry bundles) are now correctly skipped rather than passed to tools that ignore them anyway.tests/unit/specfact_code_review/tools/test_contract_runner.py (2)
18-29: Well-designed autouse fixture for tool availability stubbing.The fixture correctly patches
shutil.whichat the tool_availability module's import location, allowingskip_if_tool_missing("crosshair", ...)to pass while preserving the realshutil.whichfor other tools. This ensures tests exercise the actual contract runner logic rather than short-circuiting on tool availability.
31-39: Test correctly validates icontract-conditional AST scanning.The renamed test
test_run_contract_check_skips_missing_icontract_when_package_unusedaccurately reflects the new behavior: when no reviewed file importsicontract, theMISSING_ICONTRACTAST findings are suppressed. This aligns with the_has_icontract_usage()check added tocontract_runner.py.packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py (3)
22-36: Correct icontract import detection logic.The
_has_icontract_usage()function properly handles both import styles:
from icontract import require, ensure(viaast.ImportFrom)import icontract(viaast.Import)The early-exit pattern (
return Trueon first match) is efficient for the common case.
127-143: Tool availability guard and bug-hunt timeouts look correct.The
skip_if_tool_missingcall at line 131-133 ensures graceful degradation when CrossHair isn't installed, returning atool_errorfinding rather than failing. The bug-hunt timeouts (10s per-path, 120s subprocess) provide reasonable headroom for deeper analysis without being excessive.
192-201: Conditional AST scanning prevents false-positive MISSING_ICONTRACT findings.By gating
_scan_file()behind_has_icontract_usage(files), modules that don't use icontract won't be flagged for missing@require/@ensuredecorators. This is the correct semantic: only enforce contract coverage where the project has adopted icontract.Note: Files are parsed twice when icontract is used (once in
_has_icontract_usage, once in_scan_file). If performance becomes a concern with large file sets, consider caching the parsed ASTs, but this is unlikely to be material for typical review scopes.packages/specfact-code-review/src/specfact_code_review/run/runner.py (4)
262-270: Level filtering logic is correct and complete.The
_filter_findings_by_review_levelfunction correctly implements the three cases:
None: Return all findings (no filtering)"error": Keep only error-severity findings"warning": Keep error and warning findings (excludes info/advisory)This aligns with the pre-commit gate's new behavior where warnings no longer block.
384-386: Graceful degradation when pytest is unavailable.The
skip_if_pytest_unavailableguard ensures the TDD gate doesn't fail outright when pytest isn't installed (e.g., in minimal CI environments). Instead, it returns atool_errorfinding withseverity="warning"so users know tests were skipped.
498-505: Shadow mode correctly bypasses CI exit code.When
review_mode == "shadow", the report'sci_exit_codeis forced to0viamodel_copy(update=...), allowing teams to run the full review and collect findings without blocking CI. This is useful for gradual rollout of new rules or stricter checks.
249-259: Tool steps orchestration correctly wired for multiple review modes.The
_tool_stepsfunction properly integrates both approaches:partial(run_contract_check, bug_hunt=bug_hunt)passes the runtime flag to contract checks, whilerun_semgrep_bugsindependently discovers.semgrep/bugs.yamlconfiguration per bundle. This keeps adapter concerns isolated—each tool respects its own contract boundaries (flags vs. config discovery) without silent failures when registry or bundle structure changes.packages/specfact-code-review/src/specfact_code_review/review/commands.py (3)
45-77: Well-structured flag resolution with clear validation.The
_resolve_review_run_flagsfunction properly handles the mutual exclusion rules:
--include-testsvs--exclude-tests(line 55-56)--focusvs--include-tests/--exclude-tests(line 59-61)- Unknown
--focusvalues rejected with clear message (line 62-64)The return type
tuple[list[str], bool, bool]clearly communicates the three resolved flags.
89-92: CLI options for new review modes are properly typed.
--focus: Repeatablelist[str] | Nonefor multi-facet selection--mode:Literal["shadow", "enforce"]with sensible default"enforce"--level:Literal["error", "warning"] | Noneallowing full findings by default--bug-hunt: Simpleboolflag defaulting toFalseThese options provide the flexibility for both strict CI gates and exploratory shadow runs.
113-129: Clean pass-through to run_command with new parameters.The CLI correctly wires all resolved flags to
run_command():
focus_facets=tuple(focus_list)— converts to immutable tuplereview_mode=mode— shadow/enforce behaviorreview_level=level— severity filteringbug_hunt=bug_hunt— extended analysis modeError handling via
_friendly_run_command_errorensures users see actionable messages rather than raw contract violations.registry/index.json (1)
31-45: Registry entry for specfact-codebase 0.41.5 is correct.Version bump, download URL, and checksum have been updated consistently. The checksum in index.json matches the corresponding sidecar file. Core compatibility remains at
>=0.40.0,<1.0.0.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py (1)
223-234:⚠️ Potential issue | 🔴 CriticalOnly emit
RouteInfowhen a FastAPI route decorator is actually matched.Right now every function returns a route, even with no recognized route decorator, which produces false
GET /entries and pollutes sidecar contract output.Suggested fix
- def _path_method_from_route_decorator(self, decorator: ast.expr, path: str, method: str) -> tuple[str, str]: + def _path_method_from_route_decorator( + self, decorator: ast.expr, path: str, method: str + ) -> tuple[str, str, bool]: if not isinstance(decorator, ast.Call): - return path, method + return path, method, False name = self._decorator_route_name(decorator) if name is None: - return path, method + return path, method, False @@ methods = self._http_methods_from_api_route_keywords(decorator) if methods: method = methods[0] - return path, method + return path, method, True @@ if name in _FASTAPI_HTTP_VERBS: extracted_path = self._route_path_from_decorator_call(decorator) if extracted_path is not None: path = extracted_path - return path, name.upper() + return path, name.upper(), True - return path, method + return path, method, False @@ - for decorator in func_node.decorator_list: - path, method = self._path_method_from_route_decorator(decorator, path, method) + matched_route = False + for decorator in func_node.decorator_list: + path, method, matched = self._path_method_from_route_decorator(decorator, path, method) + matched_route = matched_route or matched + + if not matched_route: + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py` around lines 223 - 234, The code currently always returns a RouteInfo even when no FastAPI route decorator matched; update the logic so a RouteInfo is only emitted when a decorator is recognized: add a matched flag (or extend _path_method_from_route_decorator to return (path, method, matched)), set matched=True when a route decorator is detected inside the for loop over func_node.decorator_list, and only construct/return RouteInfo if matched is True (otherwise return None or skip). Update callers of _path_method_from_route_decorator and the RouteInfo creation site (the block using func_node.decorator_list, _path_method_from_route_decorator, _extract_path_parameters, and RouteInfo) accordingly.
♻️ Duplicate comments (1)
packages/specfact-code-review/.semgrep/bugs.yaml (1)
36-38:⚠️ Potential issue | 🟡 MinorUse structural negation for
yaml.loadinstead of regex exclusion.
pattern-not-regexhere is brittle with nested parentheses/multiline argument expressions and can still flag safeLoader=calls. Prefer AST-based negation for this rule.Suggested fix
- id: specfact-bugs-yaml-unsafe languages: [python] @@ severity: WARNING patterns: - pattern: yaml.load(...) - - pattern-not-regex: yaml\.load\([^)]*Loader\s*= + - pattern-not: yaml.load(..., Loader=...) metadata: specfact-category: security🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/specfact-code-review/.semgrep/bugs.yaml` around lines 36 - 38, Replace the brittle regex exclusion with a structural AST negation: remove the pattern-not-regex entry and add a pattern-not entry that matches yaml.load calls which include a Loader argument (e.g., pattern-not: yaml.load(..., Loader=$X) or similar Semgrep AST pattern), so the rule still flags plain yaml.load(...) but excludes calls that pass Loader= without relying on regex; update the patterns list to use pattern: yaml.load(...) together with the new pattern-not: yaml.load(..., Loader=$X) entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/module-security.md`:
- Around line 48-50: The docs mention “strict” and “metadata-only” verification
but omit the default CI/PR verification command; update the section describing
the verification command for scripts/verify-modules-signature.py to explicitly
state the default PR/CI path is `--payload-from-filesystem
--enforce-version-bump` (i.e., checksum + version-bump enforcement) without
`--require-signature`, while `--require-signature` remains the strict mode and
`--metadata-only` is only for local pre-commit on non-main branches; ensure the
text clarifies that the CI workflow invokes the full verifier (not
`--metadata-only`) and optionally allows `--version-check-base <git-ref>` in CI.
In
`@packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py`:
- Around line 17-50: The skip_if_tool_missing API should only accept tool IDs
that map to actual executables; introduce a new type (e.g.,
ExecutableReviewToolId = Literal[...]) listing "ruff", "radon", "semgrep",
"basedpyright", "pylint", "crosshair" and use that type for the
_EXECUTABLE_ON_PATH dict and the skip_if_tool_missing function signature instead
of ReviewToolId so "pytest" (which remains in REVIEW_TOOL_PIP_PACKAGES) cannot
be passed accidentally; update any references to skip_if_tool_missing to use the
new type and keep REVIEW_TOOL_PIP_PACKAGES and ReviewToolId unchanged for
package-level parity.
In `@packages/specfact-codebase/module-package.yaml`:
- Line 2: The module-package.yaml version bump to 0.41.6 is out of sync with the
packaged payload causing a checksum mismatch; to fix, rebuild the 0.41.6 package
bundle (or revert the version change) and update the integrity.checksum field in
module-package.yaml with the digest produced from that exact rebuilt payload so
version and checksum are in parity, ensuring metadata fields (name, version,
commands, dependencies) remain unchanged.
In
`@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py`:
- Around line 47-53: _iter_python_files currently uses root.rglob("*.py") and
only filters files after discovery, which still traverses excluded directory
trees; change it to perform a recursive directory walk that checks directories
before descending (e.g., use root.iterdir() recursion or os.walk) and call
_path_touches_excluded_dir on each directory entry to skip recursing into
excluded subtrees so files under excluded dirs are never visited; update the
body of _iter_python_files to skip directory recursion when
_path_touches_excluded_dir(dir_path) is True and continue yielding *.py from
allowed directories only.
In `@registry/index.json`:
- Around line 33-35: Registry index entries (latest_version, download_url,
checksum_sha256) for specfact-codebase are inconsistent with
packages/specfact-codebase/module-package.yaml; update the registry index to
match the module-package.yaml version (set latest_version to 0.41.6), update
download_url to the corresponding tarball name, and replace checksum_sha256 with
the checksum from module-package.yaml (or alternatively change
module-package.yaml to 0.41.5 if you prefer keeping the older release) so the
version, filename, and checksum are all consistent.
In `@scripts/pre_commit_code_review.py`:
- Around line 217-228: The code assumes json.loads(...) returns a dict and calls
data.get("findings"); add a guard after loading JSON to validate the root is a
mapping (dict) before accessing .get: check isinstance(data, dict) and emit the
existing stderr message and return None if not; then proceed to read
findings_raw = data.get("findings") and keep the existing isinstance(..., list)
validation for findings_raw. This change touches the block that reads data, the
variable data, and findings_raw so locate that section and insert the root-type
check immediately after json.loads succeeds.
In `@tests/unit/specfact_codebase/test_sidecar_framework_extractors.py`:
- Around line 62-84: Update test_fastapi_extractor_ignores_specfact_venv_routes
to also create and assert exclusion for root-level venv and .venv fixtures:
after writing the real main and the .specfact/venv fake app, create parallel
directories tmp_path / "venv" / "lib".../fastapi_app" and tmp_path / ".venv" /
"lib".../fastapi_app" containing the same noise.py route, call
FastAPIExtractor().extract_routes(tmp_path), and assert that "/ghost-from-venv"
is not present for both root-level "venv" and ".venv" in addition to the
existing .specfact case so FastAPIExtractor's exclusion behavior is covered.
In `@tests/unit/test_git_branch_module_signature_flag_script.py`:
- Around line 10-15: The test
test_git_branch_module_signature_flag_script_documents_cli_parity currently only
checks file contents; instead run the script at SCRIPT_PATH and assert its
stdout for two env cases (GITHUB_BASE_REF=main and a non-main value) to verify
the signature-policy output (e.g., that the main-case contains the "require"
token and the non-main case contains "omit" or the opposite as defined by
specfact_cli). Update the test to invoke SCRIPT_PATH via subprocess.run (or
similar), passing env variables (copy os.environ and set GITHUB_BASE_REF),
capture stdout, and replace the string-content assertions with assertions on the
captured stdout for both env variants while still ensuring "specfact-cli" and
required tokens appear.
In `@tests/unit/test_verify_modules_signature_script.py`:
- Around line 73-92: The test currently supplies an integrity.signature even
though the project contract for unsigned manifests omits it; update the
happy-path to match the unsigned-manifest contract by removing the "signature"
key from the integrity block in the manifest written by
test_verify_manifest_metadata_only_accepts_valid_manifest (or alternatively call
verify_manifest_metadata_only with require_signature=True and keep the
signature), ensuring the manifest written to module-package.yaml only contains
integrity.checksum when exercising verify_manifest_metadata_only.
---
Outside diff comments:
In
`@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py`:
- Around line 223-234: The code currently always returns a RouteInfo even when
no FastAPI route decorator matched; update the logic so a RouteInfo is only
emitted when a decorator is recognized: add a matched flag (or extend
_path_method_from_route_decorator to return (path, method, matched)), set
matched=True when a route decorator is detected inside the for loop over
func_node.decorator_list, and only construct/return RouteInfo if matched is True
(otherwise return None or skip). Update callers of
_path_method_from_route_decorator and the RouteInfo creation site (the block
using func_node.decorator_list, _path_method_from_route_decorator,
_extract_path_parameters, and RouteInfo) accordingly.
---
Duplicate comments:
In `@packages/specfact-code-review/.semgrep/bugs.yaml`:
- Around line 36-38: Replace the brittle regex exclusion with a structural AST
negation: remove the pattern-not-regex entry and add a pattern-not entry that
matches yaml.load calls which include a Loader argument (e.g., pattern-not:
yaml.load(..., Loader=$X) or similar Semgrep AST pattern), so the rule still
flags plain yaml.load(...) but excludes calls that pass Loader= without relying
on regex; update the patterns list to use pattern: yaml.load(...) together with
the new pattern-not: yaml.load(..., Loader=$X) entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd8b24a2-1cc9-4cda-8b40-34055f09b679
⛔ Files ignored due to path filters (1)
registry/modules/specfact-code-review-0.47.0.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (25)
.github/workflows/sign-modules-on-approval.ymlCHANGELOG.mdREADME.mddocs/modules/code-review.mddocs/reference/module-security.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdpackages/specfact-code-review/.semgrep/bugs.yamlpackages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/run/commands.pypackages/specfact-code-review/src/specfact_code_review/tools/tool_availability.pypackages/specfact-codebase/module-package.yamlpackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pyregistry/index.jsonregistry/modules/specfact-code-review-0.47.0.tar.gz.sha256scripts/git-branch-module-signature-flag.shscripts/pre-commit-verify-modules-signature.shscripts/pre_commit_code_review.pyscripts/validate_agent_rule_applies_when.pyscripts/verify-modules-signature.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pytests/unit/test_git_branch_module_signature_flag_script.pytests/unit/test_pre_commit_verify_modules_signature_script.pytests/unit/test_verify_modules_signature_script.pytests/unit/workflows/test_sign_modules_on_approval.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
registry/**
⚙️ CodeRabbit configuration file
registry/**: Registry and index consistency: bundle listings, version pins, and compatibility with
published module artifacts.
Files:
registry/modules/specfact-code-review-0.47.0.tar.gz.sha256registry/index.json
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
tests/unit/test_git_branch_module_signature_flag_script.pyscripts/pre_commit_code_review.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pytests/unit/test_verify_modules_signature_script.pytests/unit/test_pre_commit_verify_modules_signature_script.pypackages/specfact-code-review/src/specfact_code_review/tools/tool_availability.pytests/unit/workflows/test_sign_modules_on_approval.pyscripts/verify-modules-signature.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.pypackages/specfact-code-review/src/specfact_code_review/run/commands.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/test_git_branch_module_signature_flag_script.pytests/unit/test_verify_modules_signature_script.pytests/unit/test_pre_commit_verify_modules_signature_script.pytests/unit/workflows/test_sign_modules_on_approval.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.py
packages/**/module-package.yaml
⚙️ CodeRabbit configuration file
packages/**/module-package.yaml: Validate metadata: name, version, commands, dependencies, and parity with packaged src.
Call out semver and signing implications when manifests or payloads change.
Files:
packages/specfact-code-review/module-package.yamlpackages/specfact-codebase/module-package.yaml
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/pre_commit_code_review.pyscripts/verify-modules-signature.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: 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.
Files:
packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pypackages/specfact-code-review/src/specfact_code_review/tools/tool_availability.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.pypackages/specfact-code-review/src/specfact_code_review/run/commands.py
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.md
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: User-facing and cross-site accuracy: Jekyll front matter, links per documentation-url-contract,
CLI examples matching bundled commands.
Files:
docs/modules/code-review.mddocs/reference/module-security.md
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
Files:
.github/workflows/sign-modules-on-approval.yml
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Learnt from: djm81
Repo: nold-ai/specfact-cli-modules PR: 136
File: registry/modules/specfact-spec-0.40.17.tar.gz.sha256:1-1
Timestamp: 2026-04-02T21:49:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
CHANGELOG.mdregistry/modules/specfact-code-review-0.47.0.tar.gz.sha256packages/specfact-code-review/module-package.yamlscripts/git-branch-module-signature-flag.shscripts/pre-commit-verify-modules-signature.shtests/unit/test_verify_modules_signature_script.pydocs/modules/code-review.mdREADME.mdtests/unit/workflows/test_sign_modules_on_approval.pyscripts/verify-modules-signature.py.github/workflows/sign-modules-on-approval.ymlpackages/specfact-codebase/module-package.yamldocs/reference/module-security.md
📚 Learning: 2026-04-02T21:49:11.371Z
Learnt from: djm81
Repo: nold-ai/specfact-cli-modules PR: 136
File: registry/modules/specfact-spec-0.40.17.tar.gz.sha256:1-1
Timestamp: 2026-04-02T21:49:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Applied to files:
CHANGELOG.mdregistry/modules/specfact-code-review-0.47.0.tar.gz.sha256tests/unit/test_git_branch_module_signature_flag_script.pypackages/specfact-code-review/module-package.yamlscripts/git-branch-module-signature-flag.shscripts/pre-commit-verify-modules-signature.shREADME.mdtests/unit/workflows/test_sign_modules_on_approval.py.github/workflows/sign-modules-on-approval.ymlpackages/specfact-codebase/module-package.yamldocs/reference/module-security.mdregistry/index.json
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
CHANGELOG.mdpackages/specfact-code-review/module-package.yamlscripts/git-branch-module-signature-flag.shscripts/pre-commit-verify-modules-signature.shtests/unit/test_verify_modules_signature_script.pyREADME.mdscripts/verify-modules-signature.py.github/workflows/sign-modules-on-approval.ymldocs/reference/module-security.md
📚 Learning: 2026-04-13T10:38:15.842Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-13T10:38:15.842Z
Learning: Adhere to worktree policy, OpenSpec gating, GitHub hierarchy-cache refresh, TDD order, quality gates, versioning, and documentation rules as defined in `docs/agent-rules/`
Applied to files:
CHANGELOG.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mddocs/modules/code-review.mdREADME.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Treat canonical rule docs (docs/agent-rules/INDEX.md) as the source of truth for worktree policy, OpenSpec gating, GitHub completeness checks, TDD order, quality gates, versioning, and documentation rules
Applied to files:
CHANGELOG.md
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Applied to files:
registry/modules/specfact-code-review-0.47.0.tar.gz.sha256packages/specfact-code-review/module-package.yamlscripts/pre_commit_code_review.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mddocs/modules/code-review.mdREADME.md.github/workflows/sign-modules-on-approval.ymlpackages/specfact-code-review/.semgrep/bugs.yamlregistry/index.jsonpackages/specfact-code-review/src/specfact_code_review/run/commands.py
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
packages/specfact-code-review/module-package.yamlscripts/pre_commit_code_review.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mddocs/modules/code-review.mdpackages/specfact-code-review/.semgrep/bugs.yamlpackages/specfact-code-review/src/specfact_code_review/run/commands.py
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
Applied to files:
scripts/pre_commit_code_review.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mddocs/modules/code-review.mdREADME.mdtests/unit/workflows/test_sign_modules_on_approval.pypackages/specfact-code-review/src/specfact_code_review/run/commands.py
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Perform `spec -> tests -> failing evidence -> code -> passing evidence` in that order for behavior changes
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mddocs/modules/code-review.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Treat the clean-code compliance gate as mandatory: the review surface enforces `naming`, `kiss`, `yagni`, `dry`, and `solid` categories and blocks regressions
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mddocs/modules/code-review.mdREADME.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Applies to **/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h} : Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Run the required verification and quality gates for the touched scope before finalization
Applied to files:
README.md
🪛 GitHub Actions: pr-orchestrator
packages/specfact-codebase/module-package.yaml
[error] 1-1: FAIL packages/specfact-codebase/module-package.yaml: checksum mismatch
🪛 LanguageTool
docs/reference/module-security.md
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ...** on non-main branches only. CI (.github/workflows/pr-orchestrator.yml) always ...
(GITHUB)
🔀 Multi-repo context nold-ai/specfact-cli
Findings (repository: nold-ai/specfact-cli) [::nold-ai/specfact-cli::]
-
Tests call the code-review runner / expect the module:
- tests/unit/specfact_cli/test_dogfood_self_review.py:26 — defines _run_review() and invokes the review runner (consumer of specfact-code-review runtime behavior). [::nold-ai/specfact-cli::]
- tests/e2e/test_wow_entrypoint.py:77/99/103 — test stubs/fixtures reference installing/using "specfact-code-review" as an installed bundle. [::nold-ai/specfact-cli::]
-
Registry / packaging / publishing consumers that will observe the bumped bundle versions and checksums:
- scripts/sign-modules.py: references discovery/iteration of module-package.yaml manifests (signing flow that will pick up module versions). [::nold-ai/specfact-cli::]
- scripts/verify-modules-signature.py: contains logic to map changed paths -> module-package.yaml and will be affected by metadata-only / signature behavior changes. (file present and exercised by CI hooks). [::nold-ai/specfact-cli::]
- registry/index.json (repo contains registry/index.json and tests that assert registry metadata); CI/tests that validate registry entries will consume updated version/checksum entries. [::nold-ai/specfact-cli::]
-
Pre-commit / CLI integration points that rely on changed code-review behavior:
- scripts/pre_commit_code_review.py (file exists in repo root) — updated logic to forward only .py/.pyi paths and treat warnings as non-blocking; this hook will change developer-local behavior and CI gating. [::nold-ai/specfact-cli::]
- docs/modules/code-review.md and README.md — document new CLI flags (bug-hunt, mode, focus, level) and exit-code semantics; callers/readme consumers will expect those options to exist in installed specfact-code-review. [::nold-ai/specfact-cli::]
-
Module discovery / installer tests reference specfact-code-review as a required bundle:
- Multiple tests under tests/unit/specfact_cli/* assert specfact-code-review is included in canonical profiles and installer flows (e.g., tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py, tests/unit/specfact_cli/registry/test_profile_presets.py). These tests will surface any mismatch between declared module-package.yaml (name/version) and registry/index.json. [::nold-ai/specfact-cli::]
Notes on impact:
- The repo contains consumers (tests, scripts, docs, pre-commit hooks, registry workflows) that expect the specfact-code-review module and module-package metadata; the PR's changes to specfact-code-review APIs/manifest versions and registry checksums are therefore relevant and likely to be exercised by existing tests and scripts above. [::nold-ai/specfact-cli::]
🔇 Additional comments (21)
packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py (1)
38-43: Good split between PATH checks and interpreter import checks for pytest.The explicit comment plus
skip_if_pytest_unavailable()keeps environment detection reliable and avoids false positives from stray CLI scripts.Also applies to: 79-108
packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py (1)
149-213: Good hardening of FastAPI decorator parsing.The split into
_route_path_from_decorator_call,_http_methods_from_api_route_keywords, and_path_method_from_route_decoratoris a solid cleanup and correctly prevents non-HTTP decorator names from becoming bogus verbs..github/workflows/sign-modules-on-approval.yml (1)
24-47: Step-level eligibility gate is correctly implemented for stable required checks.This keeps the workflow status deterministic while still restricting signing to approved same-repo PRs targeting
dev/main. Good change.Also applies to: 50-103
scripts/pre-commit-verify-modules-signature.sh (1)
15-39: Policy token handling and strict/metadata-only branching look solid.The
require|omitcontract, sanitation, and fail-fast default path are implemented cleanly.tests/unit/test_pre_commit_verify_modules_signature_script.py (1)
9-28: Good contract coverage for the newsig_policycase-shape.The test now validates the exact
requirevsomitflag behavior and guards against accidental flag drift.openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.md (1)
21-28: Evidence section now matches shipped gate semantics.The error-only exit policy and scoped
PASS_WITH_ADVISORY/ci_exit_code 0reporting are clearly documented.tests/unit/workflows/test_sign_modules_on_approval.py (1)
55-77: Workflow contract assertions are well-targeted and resilient.The tests now lock in no top-level job
if, gate-step semantics, and summary env wiring with fallbacks.Also applies to: 90-95, 171-174
packages/specfact-code-review/src/specfact_code_review/run/commands.py (4)
73-85: Past review feedback addressed:.pyifiles now included in focus filtering.The suffix check at line 74 now correctly uses
file_path.suffix not in (".py", ".pyi"), ensuring stub files are considered when focus facets are applied. This aligns with the updated file discovery logic at lines 118 and 138.
49-57: Clean refactor:_ReviewLoopFlagsconsolidates review loop parameters.The frozen dataclass groups related flags together, reducing parameter sprawl across
_run_review_once,_run_review_with_status, and_run_review_with_progress. This improves maintainability when adding future flags without expanding function signatures.
510-512: Good guard:focus_facetsandinclude_testsconflict check.The validation correctly prevents combining
--focuswith--include-tests/--exclude-tests, as documented indocs/modules/code-review.md. This ensures users express test intent through focus alone when using facet filtering.
580-593: Focus filtering integration with file resolution is correct.The implementation:
- Expands
include_for_resolvewhen"tests"is infocus_facets(line 580)- Applies focus filtering after scope resolution (line 587)
- Provides a clear error message distinguishing focus mismatch from empty discovery (lines 589-592)
This matches the documented union semantics where multiple
--focusvalues union their file sets, then intersect with the resolved scope.README.md (2)
51-51: Documentation accurately reflects the branch-aware signature policy.The description correctly documents that
--require-signatureapplies when the integration target ismain(pushes tomainor PRs with basemain), matching the logic inscripts/git-branch-module-signature-flag.shwhich returns"require"whenGITHUB_BASE_REF == "main"or the current branch ismain, and"omit"otherwise.
62-62: Code review gate description aligns with PR objectives and cross-repo consumers.The documentation now accurately describes:
- Staged Python files (
.py/.pyi) only are forwarded to SpecFact- Error-severity findings block commits; warnings do not
This matches the linked repository's
scripts/pre_commit_code_review.pybehavior described in the PR objectives. The reference todocs/modules/code-review.mdfor full CLI option details is appropriate.scripts/verify-modules-signature.py (3)
393-411: Well-structured metadata-only validation with appropriate guardrails.The function correctly validates:
- YAML is a dict (manifest shape)
integrityblock exists and is a dictchecksumis present and passes_parse_checksumformat validation- When
require_signature=True, signature must be present- When signature is present, it must be at least 32 characters (sanity check for base64-encoded Ed25519/RSA signatures)
The decorators (
@beartype,@require) are consistent withverify_manifest, maintaining the adapter boundary contract. This aligns with the documented behavior indocs/reference/module-security.mdwhere--metadata-onlyvalidates manifest integrity metadata shape without hashing/verifying crypto.
446-455: CLI flag documentation clearly communicates the operational boundary.The help text appropriately warns that CI must run the full verifier without
--metadata-only, which aligns with the pre-commit hook behavior inscripts/pre-commit-verify-modules-signature.shwhere--metadata-onlyis only used for theomitpolicy path.
467-485: Control flow correctly branches between metadata-only and full verification.The output suffix
(metadata-only)clearly distinguishes the verification mode in logs, which is helpful for debugging pre-commit vs CI discrepancies. The public key resolution is correctly skipped when--metadata-onlyis set (line 458), avoiding unnecessary file reads or environment variable lookups.docs/modules/code-review.md (5)
40-55: CLI options documentation accurately reflects implementation.The documented options align with the CLI definition in
packages/specfact-code-review/src/specfact_code_review/review/commands.py(context snippet 3):
--bug-hunt→bug_hunt: bool = typer.Option(False, "--bug-hunt")--mode shadow|enforce→mode: Literal["shadow", "enforce"] = typer.Option("enforce", "--mode")--focus→focus: list[str] | None = typer.Option(None, "--focus", ...)--level error|warning→level: Literal["error", "warning"] | None = typer.Option(None, "--level")The semantic descriptions (bug-hunt timeouts, shadow forcing exit 0, focus union semantics, level filtering before scoring) match the implementation in
commands.pyandrunner.py.
99-103: Exit code semantics correctly document shadow mode behavior.The documentation accurately states that
--mode shadowforcesci_exit_codeand process exit to0while preservingoverall_verdictin JSON. This matches the implementation inrunner.py:500-506(context snippet 1) wherereview_mode == "shadow"returnsreport.model_copy(update={"ci_exit_code": 0}).
270-276: Semgrep bug-rules pass documentation is accurate.The documentation correctly describes:
- The pass runs after clean-code Semgrep
- Uses
packages/specfact-code-review/.semgrep/bugs.yaml- Findings map to
security/correctness- Pass is silently skipped if config is missing
This matches the PR objectives and coding guidelines which specify "Semgrep bug-rules pass must be after clean-code Semgrep and should be silently skipped if
packages/specfact-code-review/.semgrep/bugs.yamlis absent."
290-294: Contract runnerMISSING_ICONTRACTsuppression documented correctly.The documentation now clarifies that the AST scan for
MISSING_ICONTRACTruns only when the file importsicontract. This matches the PR objectives stating "MISSING_ICONTRACT suppression" and ensures files that don't use icontract aren't flagged for missing decorators.
409-415: Review orchestration order reflects the expanded pipeline.The updated order correctly shows:
- Semgrep clean-code (step 3)
- Semgrep bug rules (step 4, new)
- Contract runner with bug-hunt timeouts (step 8)
- TDD gate adjusted to step 9
This maintains the documented separation between clean-code and bug-finding passes.
1 participant
Summary
Implements OpenSpec change
code-review-bug-finding-and-sidecar-venv-fix: sidecar extractors skip.specfact(and peers) so venv files are not scanned; code-review gains MISSING_ICONTRACT suppression, CrossHair bug-hunt mode, Semgrepbugs.yamlpass, shadow/enforce mode, focus facets, severity level filtering, and tool-availability skips. Bumps specfact-codebase to 0.41.5 and specfact-code-review to 0.47.0 with registry tarballs and index checksums.Pre-commit
.py/.pyipaths to SpecFact (registry tarballs and YAML no longer break the hook).Evidence
See
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdfor tests and quality gates run in the worktree.Notes
Use
hatch run python scripts/link_dev_module.py specfact-code-review --forcelocally when dogfooding Typer flags not yet in the published CLI.Made with Cursor