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
…and-sidecar-impl feat: code review bug-hunt, sidecar venv skip, registry 0.41.5/0.47.0
chore(registry): publish changed modules
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors CI module-signing logic to conditionally require signatures and add metadata-only checks; adds a step-level eligibility gate for signing-on-approval. Expands SpecFact code-review with Semgrep bug pass, CrossHair “bug-hunt”, tool-availability skips, focus/mode/level flags, sidecar extractor exclusions, manifest/registry bumps, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as rgba(31,119,180,0.5)
participant Runner as rgba(44,160,44,0.5)
participant Availability as rgba(214,39,40,0.5)
participant Tools as rgba(148,103,189,0.5)
participant Aggregator as rgba(255,127,14,0.5)
CLI->>Runner: run_review(files, bug_hunt?, focus?, mode?, level?)
Runner->>Availability: skip_if_tool_missing(tool, sample_file)
Availability-->>Runner: [] or [tool_error finding]
Runner->>Tools: run_semgrep / run_semgrep_bugs / run_crosshair(bug_hunt) / run_ruff / ...
Tools-->>Runner: findings (or [] if skipped)
Runner->>Aggregator: merge & filter findings (apply focus & level)
Aggregator-->>Runner: ReviewReport (ci_exit_code adjusted if shadow mode)
Runner->>CLI: emit JSON report & exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
djm81
added
enhancement
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: 8ca7c17e6b
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/specfact-code-review/module-package.yaml (1)
2-26:⚠️ Potential issue | 🔴 CriticalRefresh the manifest checksum from the final packaged payload.
Line 26 is already failing
pr-orchestratorwithchecksum mismatch, so this manifest is not in parity with the bundle contents. Regenerate the package artifact and updateintegrity.checksumonly after the payload is final.As per coding guidelines,
packages/**/module-package.yaml: Validate metadata: name, version, commands, dependencies, and parity with packaged src.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/specfact-code-review/module-package.yaml` around lines 2 - 26, The integrity.checksum in module-package.yaml is out of sync with the final packaged payload causing the pr-orchestrator checksum mismatch; regenerate the package artifact from the finalized bundle (ensuring version, commands, bundle_dependencies and pip_dependencies are correct), compute the SHA256 checksum of the final payload, and update integrity.checksum with that new sha256:<hash> value in module-package.yaml only after packaging is complete so the manifest matches the bundle contents.scripts/verify-modules-signature.py (1)
1-3:⚠️ Potential issue | 🟠 MajorPipeline failure: checksum mismatch on module manifests.
The CI reports
checksum mismatchfor one or moremodule-package.yamlfiles. Since this PR targetsmain, full verification runs (not--metadata-only). Theintegrity.checksumvalues in manifests must match the computed payload digest.Per learnings, signatures are generated by the publish workflow—but checksums must be updated when module content changes. Run the signing/checksum update script before merge:
#!/bin/bash # Re-sign modules to update checksums (run locally or via approval workflow) hatch run scripts/sign-modules.py --payload-from-filesystem🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-modules-signature.py` around lines 1 - 3, CI failed due to checksum mismatches in module manifests: run the signing script to regenerate signatures and update integrity.checksum values in the module-package.yaml files before merging by executing the sign-modules.py workflow (reference script name sign-modules.py) with the payload-from-filesystem option (use --payload-from-filesystem) so the manifest integrity.checksum fields are replaced with the newly computed payload digests; re-run the verification (verify-modules-signature.py) to confirm no checksum mismatches remain.docs/modules/code-review.md (2)
404-415:⚠️ Potential issue | 🟡 MinorUpdate the
run_review(...)signature shown in the orchestration section.The docs still advertise
run_review(files, no_tests=False)even though the paired public surface now includes review mode, level, focus facets, and bug-hunt support. That drift is easy for downstream bundle consumers to copy into integrations.As per coding guidelines, "
docs/**/*.md: User-facing and cross-site accuracy: Jekyll front matter, links per documentation-url-contract, CLI examples matching bundled commands."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/modules/code-review.md` around lines 404 - 415, The docs snippet still shows the old function signature; update the orchestration section to show the current public signature for specfact_code_review.run.runner.run_review (include the new parameters such as review_mode, level, focus facets and bug_hunt support / flags), and update any example call (previously run_review(files, no_tests=False)) to a representative call that includes those parameters (e.g., with explicit review_mode and level and bug_hunt arguments) so downstream consumers see the correct API surface and options.
280-309:⚠️ Potential issue | 🟡 MinorAlign the contract-runner docs with the updated call contract.
This section still documents
run_contract_check(files)and reads as ifMISSING_ICONTRACTis decided per file, but the paired implementation change introducesbug_huntbehavior and batch-level gating around the icontract scan. In mixed-file reviews, this page now describes a different contract than the bundle exposes.Based on learnings, "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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/modules/code-review.md` around lines 280 - 309, The docs for specfact_code_review.tools.contract_runner.run_contract_check are out of sync with the new batch-level contract: update this section to reflect that run_contract_check now accepts/observes the bug_hunt flag and that MISSING_ICONTRACT is gated at the bundle/batch level (not decided per file) — describe that icontract AST scans only run when the batch-level bug_hunt/flag enables them and clarify CrossHair timeout/bug_hunt semantics (per_path_timeout 2 vs 10 and subprocess budgets 30 vs 120) and error behaviors (unreadable files → tool_error, missing crosshair → tool_error) so the documentation matches the implementation and paired specfact-cli changes.packages/specfact-code-review/src/specfact_code_review/review/commands.py (1)
21-32: 🛠️ Refactor suggestion | 🟠 MajorDon’t couple the CLI adapter to
run_commanderror text.
_friendly_run_command_error()now depends on exact downstream message fragments, and one branch already leaks the internal namefocus_facets. A wording-only change inspecfact_code_review.run.commands.run_commandwill silently change this command’s UX. Prefer structured exception types, or keep the user-facing validation entirely in this Typer layer.As per coding guidelines, "
packages/**/src/**/*.py: Focus on adapter and bridge patterns ... and clear boundaries so core upgrades do not silently break bundles."🤖 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/review/commands.py` around lines 21 - 32, The CLI adapter _friendly_run_command_error currently matches hardcoded message substrings from run_command, coupling UX to downstream wording; instead, define and raise structured exceptions from specfact_code_review.run.commands.run_command (e.g., InvalidOptionCombinationError, MissingOutForJsonError, ConflictingScopeError, FocusFacetConflictError, NoReviewableFilesError or a generic RunCommandError with an error_code/enum) and then update _friendly_run_command_error to switch on those exception types or their error_code attribute (accepting the new exception classes instead of matching strings and removing any exposure of internal names like focus_facets); alternatively, move the validation that produces these user-facing errors into this Typer layer so user messages are generated here rather than parsed from run_command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-orchestrator.yml:
- Around line 89-97: The workflow currently only appends --require-signature for
PRs to main when the head repo is different, which exempts same-repo PRs; update
the condition so that VERIFY_CMD+=(--require-signature) is added for
TARGET_BRANCH "main" regardless of head repo (but keep the existing special-case
where TARGET_BRANCH "dev" triggers --metadata-only), i.e., change the elif that
tests both TARGET_BRANCH and repo inequality to simply check TARGET_BRANCH =
"main" (or otherwise ensure main PRs always get --require-signature) so
VERIFY_CMD, --require-signature, TARGET_BRANCH and the
github.event.pull_request.head.repo.full_name check reflect this new behavior.
In @.github/workflows/sign-modules-on-approval.yml:
- Around line 24-47: The eligibility gate currently sets sign=true based only on
github.event.review.state and branch/repo checks; update the gate's run logic in
the "gate" step to also validate github.event.review.user.author_association and
only allow signing when that association is one of the trusted values (e.g.,
"COLLABORATOR", "MEMBER", or "OWNER"). Locate the block that writes to
GITHUB_OUTPUT and add a conditional that reads
github.event.review.user.author_association and rejects (echo "sign=false" and
notice + exit 0) any review where the association is not in the allowed set, so
sign=true is emitted only for approved reviews from trusted reviewer
associations.
In `@docs/reference/module-security.md`:
- Around line 48-52: Update the module-security docs to match the actual
workflow: change the statement that "CI does not pass --metadata-only" to note
that PRs targeting dev append --metadata-only (per
.github/workflows/pr-orchestrator.yml), and clarify that PRs targeting main only
append --require-signature conditionally when the head repo differs from
github.repository (not unconditionally); also ensure the description of
scripts/verify-modules-signature.py and
scripts/pre-commit-verify-modules-signature.sh reflects that the pre-commit hook
adds --require-signature only when the checkout or GITHUB_BASE_REF is main and
otherwise uses baseline flags, and confirm the note about CI passing
--version-check-base remains. Use the existing terms from the diff
(scripts/verify-modules-signature.py, --metadata-only, --require-signature,
scripts/pre-commit-verify-modules-signature.sh,
.github/workflows/pr-orchestrator.yml) so readers can map the docs to the
workflow logic.
In
`@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md`:
- Line 1: The top-level heading in the spec file is currently second-level ("##
ADDED Requirements") which violates MD041; change the first line to a top-level
heading by replacing "## ADDED Requirements" with "# ADDED Requirements" so the
document begins with a single H1 heading.
- Around line 10-22: Update the spec to reference the bugs-pass entrypoint
instead of run_semgrep: change the WHEN/AND lines that mention `run_semgrep` to
point to the new `run_semgrep_bugs` (or the shipped "bugs-pass" entrypoint) and
clarify that `run_review` wires `run_semgrep_bugs` alongside the existing
`run_semgrep`; ensure the scenarios mention `.semgrep/bugs.yaml`,
`run_semgrep_bugs` (or "bugs-pass") and `run_review` explicitly so the OpenSpec
matches the implemented split between `run_semgrep` and `run_semgrep_bugs`.
In
`@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md`:
- Line 1: The file's top-level heading is currently '## MODIFIED Requirements'
which triggers MD041; change the first heading in specs/contract-runner/spec.md
to a top-level heading by replacing the leading '##' with '#' so the file begins
with '# MODIFIED Requirements' (ensure the very first line of the file starts
with a single '#' followed by the heading text).
In
`@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md`:
- Around line 3-30: The spec for
tests/cli-contracts/specfact-code-review-run.scenarios.yaml omitted the new
--bug-hunt flag; update that scenarios file to include contract tests covering
--bug-hunt alongside the existing --mode, --focus, and --level cases: add at
least one scenario asserting behavior when --bug-hunt is present (both in shadow
and enforce modes), include combinations with --focus (e.g., union and narrowing
like --focus tests) and with --level filtering, and add an error-path scenario
for invalid combinations (e.g., --bug-hunt with --include-tests or
--exclude-tests) so the CLI command "specfact code review run" is fully
specified.
In
`@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md`:
- Line 1: Add a top-level H1 heading as the first line of the Markdown file to
satisfy MD041: insert a line like "# Review Run Command Specification"
immediately before the existing "## ADDED Requirements" header so the first line
of the file is an H1; ensure there is a blank line after the H1 for proper
Markdown rendering.
In
`@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md`:
- Line 1: Add a top-level H1 before the existing H2 to satisfy MD041; insert a
descriptive H1 (for example "# Sidecar route extraction" or "# ADDED
Requirements: Sidecar route extraction") above the current "## ADDED
Requirements" heading so the document begins with an H1.
In `@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md`:
- Line 1: The file begins with a level-2 heading "## 1. Sidecar venv self-scan
fix (specfact-codebase)" which triggers MD041; change the first line to a
top-level heading by replacing "## 1. Sidecar venv self-scan fix
(specfact-codebase)" with "# 1. Sidecar venv self-scan fix (specfact-codebase)"
so the top of the tasks.md file uses a single `#` heading.
In `@packages/specfact-code-review/.semgrep/bugs.yaml`:
- Around line 36-40: The YAML semgrep rule can miss multi-line yaml.load(...)
calls because the current pattern-not-regex uses [^)]* which doesn't span
newlines or nested parentheses; update the rule so the negative regex for Loader
detection matches across newlines and nested content (e.g., use a
DOTALL/multi-line approach or a [\s\S]*? non-greedy match) when checking for
"yaml.load(... Loader="; modify the "pattern-not-regex" that references
yaml\.load\([^)]*Loader\s*= to a regex that allows newlines (or enable (?s)) so
calls like yaml.load(\n some_data,\n Loader=yaml.SafeLoader\n) are correctly
excluded.
In `@packages/specfact-code-review/src/specfact_code_review/_review_utils.py`:
- Around line 38-40: The function python_source_paths_for_tools currently only
returns .py files; update it to include .pyi stubs by filtering with path.suffix
in {".py", ".pyi"} and update the docstring to state that Python source and type
stub files are included so linters/typecheckers like basedpyright will analyze
.pyi files as well.
In
`@packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py`:
- Around line 22-36: The current logic suppresses the MISSING_ICONTRACT check by
basing _has_icontract_usage(files) on only the PR-changed files (py_files);
change this so the decision is made from a package/repo-level scan (e.g., scan
all .py files in the package or repo) or alter the caller to always run
_scan_file on changed files and only skip when a repository-wide scan shows zero
icontract imports; update references where MISSING_ICONTRACT is gated (calls
involving _has_icontract_usage, py_files, and _scan_file) so the rule only gets
suppressed when the entire package/repo truly does not use icontract, not just
when the changed files lack the import.
In
`@packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py`:
- Around line 168-191: The current Typer exemption in
_typer_cli_entrypoint_exempt relies only on the first parameter annotation
ending with "Context"; update this heuristic to also require an explicit Typer
signal: inspect the function_node.decorator_list for decorators that are a
call/attribute named "command" or "app.command" (or similar) and/or verify the
module AST contains an import of "typer" or "from typer import ..." before
returning True; implement these checks in _typer_cli_entrypoint_exempt (or call
a helper from it) so _kiss_parameter_findings only exempts when the Context
param AND either a Typer decorator or a Typer import are present.
In
`@packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py`:
- Around line 335-398: The two functions _finding_from_bug_result and
_finding_from_result (and likewise
_append_semgrep_finding/_append_semgrep_bug_finding) duplicate validation and
extraction logic; extract the common checks into a shared helper (e.g.,
_extract_common_finding_fields) that returns (filename, raw_rule, line, message)
or None if path is disallowed, then have _finding_from_bug_result and
_finding_from_result call that helper and only apply their specific differences:
call their respective normalizers (_normalize_bug_rule_id vs
_normalize_rule_id), category maps (BUG_RULE_CATEGORY vs SEMGREP_RULE_CATEGORY),
and severity logic (use extra.severity for bug results, default "warning" for
the other), and update _append... functions to reuse the unified extraction +
per-type finder to append findings.
In `@packages/specfact-codebase/module-package.yaml`:
- Line 27: The checksum value in the module manifest (the checksum field in
module-package.yaml) is mismatched with the packaged payload; regenerate the
SHA-256 digest for the final packaged payload, replace the existing checksum
value (sha256:a1508cf26a2519eefae9106ad991db5406cdbbb46ba0eefd56068aa32e754f83)
with the newly computed digest, then re-run the verify-modules-signature step to
confirm integrity; if any signed assets or the manifest changed, also update the
module version/signature metadata as required by the release process before
committing.
In
`@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py`:
- Around line 47-53: _iter_python_files currently uses root.rglob("*.py") which
still descends into excluded subtrees; change it to a directory-walking approach
that prunes excluded directories as it discovers them (e.g., use os.walk or
Path.iterdir+manual recursion) so you remove entries from the directory list
before descending. Use the existing helper _path_touches_excluded_dir to test
each candidate directory name/path and skip (or remove from the walk's dirs
list) any excluded subtree, and yield files that end with ".py" as before; keep
the early-return behavior when root doesn't exist or isn't a dir.
In
`@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py`:
- Around line 234-246: The loop over func_node.decorator_list currently
overwrites method/path on every matching decorator, causing a
last-decorator-wins behavior; change the logic in the function that processes
decorators so it accumulates all matches instead of replacing them: create a
list (e.g., matched_routes) and for each decorator where
_path_method_from_route_call(decorator) or
_path_method_from_api_route_call(decorator) returns a non-None (method, path)
append that tuple, set matched based on whether the list is non-empty, and
return or expose the full list of routes instead of a single method/path; update
any callers that expect a single tuple to handle multiple entries or explicitly
choose the desired one.
- Around line 24-36: The FastAPI extractor currently reimplements directory/file
scanning via _should_skip_path_for_fastapi_scan and _iter_scan_python_files and
defines _EXCLUDED_DIR_PARTS, diverging from the base class; replace the custom
scanning by removing those helpers and either (A) extend the base exclusion list
by adding ".mypy_cache", ".pytest_cache", ".ruff_cache" to the base class's
_EXCLUDED_DIR_NAMES, or (B) override FastAPIExtractor._path_touches_excluded_dir
to call super()._path_touches_excluded_dir(...) and additionally check for the
three extra cache names, then reuse the base _iter_python_files routine so
FastAPI reuses _iter_python_files and _path_touches_excluded_dir instead of
_should_skip_path_for_fastapi_scan/_iter_scan_python_files.
In
`@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py`:
- Around line 110-127: The _register_flask_assign_symbols method is currently a
dead pass because it populates app_names and bp_names but those sets are never
used; either remove this symbol-registration pass or wire the sets into the
route-decorator ownership logic so they influence route extraction. Locate
_register_flask_assign_symbols and the code paths that extract routes/decorators
(the decorator ownership/check logic that examines decorators on functions) and
either (A) remove the method and any callers if you don't need assignment-based
symbol tracking, or (B) pass app_names and bp_names into the
decorator-resolution/route extraction functions and update the decorator
ownership checks to treat decorators referencing those names as owned by the
app/blueprint. Ensure you update or remove related references at the other
mentioned location (the similar handling around the other assign-symbol block)
to preserve clean-code gates (naming, KISS, YAGNI, DRY, SOLID).
In `@README.md`:
- Line 62: Update the "Code review gate (matches specfact-cli core)" paragraph
in README.md (the block describing pre-commit hook behavior) to explicitly state
that warning-only SpecFact results do not block commits but still must be
addressed before merging unless an explicit exception is documented; add one
concise sentence such as: "Non-blocking warnings reported by SpecFact still
require remediation prior to merge unless a documented, approved exception
exists." Reference the existing "Code review gate (matches specfact-cli core)"
header and the sentence about blocking on error-severity findings when making
this edit.
In `@scripts/pre_commit_code_review.py`:
- Around line 318-323: The current logic in _print_review_findings_summary()
uses error_count to decide exit but that misinterprets the subprocess contract;
instead of falling back to result.returncode when error_count > 0, read and
return the report's actual ci_exit_code (or overall_verdict) from the parsed
JSON produced by the specfact run and use that as the script exit; update the
code path that currently returns result.returncode to extract ci_exit_code (or
overall_verdict) from the parsed report object and return it directly so fixable
"error" findings that yield ci_exit_code=0 do not incorrectly fail the gate.
In `@tests/cli-contracts/specfact-code-review-run.scenarios.yaml`:
- Around line 80-120: The scenarios focus-source-and-docs-union,
focus-tests-narrows-to-test-tree and level-error-json-clean-module currently
only assert a run_id or review-report.json and therefore don't verify that the
--focus and --level flags actually change behavior; update each scenario to
assert an observable filtered output: for focus-source-and-docs-union and
focus-tests-narrows-to-test-tree assert stdout (or the JSON output) contains
metadata or findings limited to the selected scopes (e.g., check for a field
like "selected_paths" or ensure findings reference only "source" and "docs" or
only "tests"), and for level-error-json-clean-module change the expectation to
assert that all reported findings in the JSON output have severity "error" (or
that warnings/info are absent); reference the scenario names and the CLI flags
(--focus, --level) as you modify the expect blocks so the tests fail if the
flags are accepted but ignored.
In `@tests/unit/specfact_code_review/test__review_utils.py`:
- Around line 19-26: The test
test_python_source_paths_for_tools_keeps_only_py_suffix currently only asserts
YAML exclusion; add a `.pyi` stub-file case to lock the filter contract by
creating a tmp `.pyi` Path alongside `py_file` and `yaml_file`, writing minimal
content, and asserting that python_source_paths_for_tools([py_file, yaml_file,
pyi_file]) returns only [py_file] (i.e., the `.pyi` file is excluded). Reference
the existing test name and the function python_source_paths_for_tools to locate
where to add the extra stub-file setup and assertion.
In `@tests/unit/specfact_code_review/tools/test_basedpyright_runner.py`:
- Around line 15-25: The tests use truthy/falsy checks which can mask incorrect
return types; replace the loose assertions in test_run_basedpyright_* to
explicitly assert an empty list return from run_basedpyright. Specifically, in
the tests referencing run_basedpyright (e.g., the empty-input case and the
YAML-manifest skip case), change the assertions from "assert not
run_basedpyright(...)" to "assert run_basedpyright(...) == []" so the tests
enforce an exact empty-list return value.
In `@tests/unit/test_git_branch_module_signature_flag_script.py`:
- Around line 10-15: The current test
test_git_branch_module_signature_flag_script_documents_cli_parity only checks
token presence and should be replaced/augmented with behavioral assertions:
execute the script (via subprocess or by importing the module at SCRIPT_PATH)
with representative GITHUB_BASE_REF environment values (e.g., a protected branch
like "main" and a non-protected branch like "feature/x") and assert the script's
output/state maps to the expected mode strings "require" and "omit"
respectively; keep references to SCRIPT_PATH and the test function name when
locating code, and ensure the test asserts actual behavior (exit code or stdout
content) rather than mere token presence.
---
Outside diff comments:
In `@docs/modules/code-review.md`:
- Around line 404-415: The docs snippet still shows the old function signature;
update the orchestration section to show the current public signature for
specfact_code_review.run.runner.run_review (include the new parameters such as
review_mode, level, focus facets and bug_hunt support / flags), and update any
example call (previously run_review(files, no_tests=False)) to a representative
call that includes those parameters (e.g., with explicit review_mode and level
and bug_hunt arguments) so downstream consumers see the correct API surface and
options.
- Around line 280-309: The docs for
specfact_code_review.tools.contract_runner.run_contract_check are out of sync
with the new batch-level contract: update this section to reflect that
run_contract_check now accepts/observes the bug_hunt flag and that
MISSING_ICONTRACT is gated at the bundle/batch level (not decided per file) —
describe that icontract AST scans only run when the batch-level bug_hunt/flag
enables them and clarify CrossHair timeout/bug_hunt semantics (per_path_timeout
2 vs 10 and subprocess budgets 30 vs 120) and error behaviors (unreadable files
→ tool_error, missing crosshair → tool_error) so the documentation matches the
implementation and paired specfact-cli changes.
In `@packages/specfact-code-review/module-package.yaml`:
- Around line 2-26: The integrity.checksum in module-package.yaml is out of sync
with the final packaged payload causing the pr-orchestrator checksum mismatch;
regenerate the package artifact from the finalized bundle (ensuring version,
commands, bundle_dependencies and pip_dependencies are correct), compute the
SHA256 checksum of the final payload, and update integrity.checksum with that
new sha256:<hash> value in module-package.yaml only after packaging is complete
so the manifest matches the bundle contents.
In `@packages/specfact-code-review/src/specfact_code_review/review/commands.py`:
- Around line 21-32: The CLI adapter _friendly_run_command_error currently
matches hardcoded message substrings from run_command, coupling UX to downstream
wording; instead, define and raise structured exceptions from
specfact_code_review.run.commands.run_command (e.g.,
InvalidOptionCombinationError, MissingOutForJsonError, ConflictingScopeError,
FocusFacetConflictError, NoReviewableFilesError or a generic RunCommandError
with an error_code/enum) and then update _friendly_run_command_error to switch
on those exception types or their error_code attribute (accepting the new
exception classes instead of matching strings and removing any exposure of
internal names like focus_facets); alternatively, move the validation that
produces these user-facing errors into this Typer layer so user messages are
generated here rather than parsed from run_command.
In `@scripts/verify-modules-signature.py`:
- Around line 1-3: CI failed due to checksum mismatches in module manifests: run
the signing script to regenerate signatures and update integrity.checksum values
in the module-package.yaml files before merging by executing the sign-modules.py
workflow (reference script name sign-modules.py) with the
payload-from-filesystem option (use --payload-from-filesystem) so the manifest
integrity.checksum fields are replaced with the newly computed payload digests;
re-run the verification (verify-modules-signature.py) to confirm no checksum
mismatches remain.
🪄 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: 22e7c81f-9bb1-4cc9-92b0-fb0d5d5df757
⛔ Files ignored due to path filters (4)
registry/modules/specfact-code-review-0.47.0.tar.gzis excluded by!**/*.gzregistry/modules/specfact-code-review-0.47.1.tar.gzis excluded by!**/*.gzregistry/modules/specfact-codebase-0.41.5.tar.gzis excluded by!**/*.gzregistry/modules/specfact-codebase-0.41.6.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (67)
.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.yml.pre-commit-config.yamlCHANGELOG.mdREADME.mddocs/modules/code-review.mddocs/reference/module-security.mdopenspec/CHANGE_ORDER.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/.openspec.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/TDD_EVIDENCE.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/design.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/proposal.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-tool-dependencies/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.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_utils.pypackages/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/ast_clean_code_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-code-review-0.47.1.tar.gz.sha256registry/modules/specfact-codebase-0.41.5.tar.gz.sha256registry/modules/specfact-codebase-0.41.6.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/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_utils.pytests/unit/specfact_code_review/test_review_tool_pip_manifest.pytests/unit/specfact_code_review/tools/test_basedpyright_runner.pytests/unit/specfact_code_review/tools/test_contract_runner.pytests/unit/specfact_code_review/tools/test_radon_runner.pytests/unit/specfact_code_review/tools/test_tool_availability.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_pr_orchestrator_signing.pytests/unit/workflows/test_sign_modules_on_approval.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py`:
- Around line 419-420: The direct PATH probe using shutil.which("semgrep")
inside run_semgrep_bugs should be removed and replaced with the shared
availability helper used by run_semgrep (e.g., skip_if_tool_missing or the
project’s tool_availability wrapper) so both passes rely on one contract; locate
run_semgrep_bugs and replace the raw shutil.which check with a call to the
shared helper (propagating its return/behavior) and ensure any early-return
logic mirrors run_semgrep’s semantics to keep adapter/bridge boundaries
consistent.
In `@tests/unit/specfact_code_review/tools/test_semgrep_runner.py`:
- Around line 214-223: The tests for run_semgrep need to also stub the SEMGREP
availability check so they don't short-circuit via skip_if_tool_missing; update
the semgrep runner tests (e.g., the test module containing run_semgrep and
run_semgrep_bugs) to monkeypatch shutil.which (or provide a shared fixture) to
return a non-None value for "semgrep" before calling
run_semgrep/run_semgrep_bugs, so existing subprocess.run mocks are exercised;
locate uses of run_semgrep, run_semgrep_bugs and any subprocess.run mocks in
tests/unit/specfact_code_review/tools/test_semgrep_runner.py and add the shared
monkeypatch/fixture to ensure the PATH probe is consistently patched across
tests.
🪄 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: 6c0954e7-6a84-4074-b36b-2c5229d5c3dc
📒 Files selected for processing (7)
packages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/_review_utils.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-codebase/module-package.yamltests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.pytests/unit/specfact_code_review/test__review_utils.pytests/unit/specfact_code_review/tools/test_semgrep_runner.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.12)
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.11)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/fixtures/contract_runner/public_missing_contract_but_icontract_imported.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/specfact_code_review/test__review_utils.pypackages/specfact-code-review/src/specfact_code_review/_review_utils.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.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/fixtures/contract_runner/public_missing_contract_but_icontract_imported.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/specfact_code_review/test__review_utils.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/_review_utils.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.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
🧠 Learnings (6)
📓 Common learnings
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
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
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
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: Work belongs on feature/*, bugfix/*, hotfix/*, or chore/* branches, normally in a worktree rooted under ../specfact-cli-modules-worktrees/
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
📚 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:
packages/specfact-code-review/module-package.yamlpackages/specfact-codebase/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: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
packages/specfact-code-review/module-package.yamlpackages/specfact-codebase/module-package.yaml
📚 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:
packages/specfact-code-review/module-package.yamlpackages/specfact-codebase/module-package.yaml
📚 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:
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: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
packages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py
🔀 Multi-repo context nold-ai/specfact-cli
nold-ai/specfact-cli
Findings (direct evidence from repository files)
-
New/changed code-review public APIs and callers:
- Added helper python_source_paths_for_tools(...) in:
- packages/specfact-code-review/src/specfact_code_review/_review_utils.py — definition. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/_review_utils.py]
- Multiple tool runners now call python_source_paths_for_tools(files):
- packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py — uses python_source_paths_for_tools(files) as input to AST pass. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py]
- packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py — preprocesses files via python_source_paths_for_tools(files). [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/basedpyright_runner.py]
- packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py — derives py_files via python_source_paths_for_tools(files). [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py]
- packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py — calls python_source_paths_for_tools(files). [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/pylint_runner.py]
- packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py — integrates python_source_paths_for_tools(files). [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py]
- packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py — normalizes files via python_source_paths_for_tools(files). [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/ruff_runner.py]
- packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py — semgrep runners accept file lists (added run_semgrep_bugs and callers in runner). [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py]
- Added helper python_source_paths_for_tools(...) in:
-
Tool-availability API and usage:
- New exported tool availability helpers:
- packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py — defines ReviewToolId, skip_if_tool_missing(...), skip_if_pytest_unavailable(...). [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py]
- Runners that now call skip_if_tool_missing / skip_if_pytest_unavailable:
- basedpyright_runner.py, pylint_runner.py, radon_runner.py, ruff_runner.py, semgrep_runner.py, contract_runner.py (CrossHair), and test TDD gate short-circuit use skip_if_pytest_unavailable. See each file above. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/]
- New exported tool availability helpers:
-
Review CLI / run_review surface changes and callers:
- run_review signature extended with bug_hunt, review_level, review_mode:
- packages/specfact-code-review/src/specfact_code_review/run/init.py — run_review signature updated. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/run/init.py]
- packages/specfact-code-review/src/specfact_code_review/run/runner.py — run_review implementation updated and invoked by run/commands. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/run/runner.py]
- packages/specfact-code-review/src/specfact_code_review/run/commands.py — ReviewRunRequest and command wiring updated; CLI options added (
--focus,--mode,--level,--bug-hunt). [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/run/commands.py] - packages/specfact-code-review/src/specfact_code_review/review/commands.py — Typer CLI run() updated to accept focus/mode/level/bug_hunt and pass them to run_review. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/review/commands.py]
- run_review signature extended with bug_hunt, review_level, review_mode:
-
Semgrep bug-pass:
- New semgrep bugs pass and entrypoints:
- packages/specfact-code-review/.semgrep/bugs.yaml — new ruleset. [::nold-ai/specfact-cli::packages/specfact-code-review/.semgrep/bugs.yaml]
- packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py — find_semgrep_bugs_config and run_semgrep_bugs added; runner integrated into run_review. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py]
- New semgrep bugs pass and entrypoints:
-
Verify modules signature / metadata-only mode:
- scripts/verify-modules-signature.py — added verify_manifest_integrity_shape_only(...) and --metadata-only CLI flag; main() adjusted to skip public-key resolution in metadata-only mode. [::nold-ai/specfact-cli::scripts/verify-modules-signature.py]
- scripts/pre-commit-verify-modules-signature.sh and scripts/git-branch-module-signature-flag.sh — branch-aware require/omit behavior uses the new flag script. [::nold-ai/specfact-cli::scripts/pre-commit-verify-modules-signature.sh] [::nold-ai/specfact-cli::scripts/git-branch-module-signature-flag.sh]
- CI workflows reference verify command changes:
- .github/workflows/pr-orchestrator.yml — VERIFY_CMD branching logic updated to pass --metadata-only for dev and conditional --require-signature for main. [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml]
- .github/workflows/sign-modules-on-approval.yml — approval gating step computes sign=true/false in a gate step and later steps conditional on gate output. [::nold-ai/specfact-cli::.github/workflows/sign-modules-on-approval.yml]
-
Module manifest and registry updates (consumers of signature/manifest changes):
- Multiple places read/write module-package.yaml and registry entries; scripts that canonicalize payloads and verify manifests reference module-package.yaml:
- src/specfact_cli/registry/module_installer.py — canonical payload handling and manifest paths. [::nold-ai/specfact-cli::src/specfact_cli/registry/module_installer.py]
- scripts/sign-modules.py, scripts/sign-module.sh, scripts/publish-module.py — signing/publishing workflows updated. [::nold-ai/specfact-cli::scripts/sign-modules.py] [::nold-ai/specfact-cli::scripts/sign-module.sh] [::nold-ai/specfact-cli::scripts/publish-module.py]
- registry/index.json and packages/*/module-package.yaml files were bumped (versions, checksums) and tests updated to expect these new versions (registry references present). [::nold-ai/specfact-cli::registry/index.json] [::nold-ai/specfact-cli::packages/specfact-code-review/module-package.yaml] [::nold-ai/specfact-cli::packages/specfact-codebase/module-package.yaml]
- Multiple places read/write module-package.yaml and registry entries; scripts that canonicalize payloads and verify manifests reference module-package.yaml:
-
Sidecar framework extractor changes (file-discovery exclusions):
- BaseFrameworkExtractor now has excluded-dir helpers and iterators:
- packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py — _EXCLUDED_DIR_NAMES, _path_touches_excluded_dir, _iter_python_files. [::nold-ai/specfact-cli::packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py]
- FastAPI/Flask/Django/DRF extractors now use _iter_python_files and exclude .specfact venvs:
- packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py [::nold-ai/specfact-cli::packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py]
- packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py [::nold-ai/specfact-cli::packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py]
- packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.py [::nold-ai/specfact-cli::packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/django.py]
- packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.py [::nold-ai/specfact-cli::packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/drf.py]
- BaseFrameworkExtractor now has excluded-dir helpers and iterators:
Tests updated to reflect these changes:
- Many unit tests added/modified to assert new behaviors:
- tests/unit/specfact_code_review/tools/test_tool_availability.py — tests skip_if_tool_missing and skip_if_pytest_unavailable. [::nold-ai/specfact-cli::tests/unit/specfact_code_review/tools/test_tool_availability.py]
- tests/unit/specfact_code_review/test__review_utils.py — tests python_source_paths_for_tools. [::nold-ai/specfact-cli::tests/unit/specfact_code_review/test__review_utils.py]
- tests/unit/test_verify_modules_signature_script.py — covers verify_manifest_integrity_shape_only behavior. [::nold-ai/specfact-cli::tests/unit/test_verify_modules_signature_script.py]
- tests/unit/workflows/test_pr_orchestrator_signing.py and tests/unit/workflows/test_sign_modules_on_approval.py — validate workflow branching/eligibility logic. [::nold-ai/specfact-cli::tests/unit/workflows/]
- tests/unit/specfact_codebase/test_sidecar_framework_extractors.py — verifies exclusion of .specfact venv routes. [::nold-ai/specfact-cli::tests/unit/specfact_codebase/test_sidecar_framework_extractors.py]
Summary / impact notes (observed in repo)
- The PR changes introduce new public helpers (python_source_paths_for_tools, skip_if_tool_missing, skip_if_pytest_unavailable, verify_manifest_integrity_shape_only, run_review signature additions) and update many internal callers across the specfact-code-review and specfact-codebase packages to use them. Consumer sites in this repo are updated accordingly (runners, CLI wiring, workflows, scripts).
- Key cross-boundary surfaces to watch when merging:
- Any external consumers (other repos) that call run_review(...) or import the older run_review signature must update to the new signature/parameters.
- Any external tooling invoking verify-modules-signature.py may observe new --metadata-only behavior; workflows in this repo already adapted (pr-orchestrator, pre-commit wrapper).
- Module registry/publishing scripts and registry/index.json were updated; consumers that pin specific module versions/checksums should reconcile registry changes.
🔇 Additional comments (8)
tests/unit/specfact_code_review/fixtures/contract_runner/public_missing_contract_but_icontract_imported.py (1)
1-11: Fixture behavior is correct and purpose-built.This fixture cleanly models “
icontractimported but public API missing contracts,” and the binding avoids false “unused import” noise while preserving AST-detectable import usage.As per coding guidelines: "tests/**/*.py: Contract-first and integration tests... Ensure changes to adapters or bridges have targeted coverage."
packages/specfact-code-review/module-package.yaml (2)
2-2: Version bump is correctly applied for a signed manifest update.Line 2 updates the module version in tandem with integrity metadata changes, which is the expected semver/signing flow for this package type.
As per coding guidelines: “Call out semver and signing implications when manifests or payloads change.”
26-27: Integrity metadata rotation looks consistent and properly scoped.The checksum/signature update is correctly represented in the manifest and aligns with signed-module release handling for this repo.
Based on learnings: “Signed module or manifest changes require version-bump review and verify-modules-signature.”
packages/specfact-codebase/module-package.yaml (2)
2-2: Version bump is aligned with signed manifest updates.Line 2 semver advancement to
0.41.7is appropriate given the integrity payload updates in this manifest.As per coding guidelines: "Validate metadata: name, version, commands, dependencies, and parity with packaged src. Call out semver and signing implications when manifests or payloads change."
27-28: Integrity metadata refresh looks consistent with release-signing flow.Lines 27–28 update checksum/signature in tandem with the version bump, which is the expected manifest-level contract for module verification workflows.
Based on learnings: "In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the
publish-modulesGitHub 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."packages/specfact-code-review/src/specfact_code_review/_review_utils.py (1)
34-43: Good tool-input boundary hardening for Python sources/stubs.Including both
.pyand.pyiinpython_source_paths_for_toolsis the right contract for downstream linters/typecheckers and prevents silent stub-file drops.As per coding guidelines:
packages/**/src/**/*.py: Focus on adapter and bridge patterns ... and clear boundaries so core upgrades do not silently break bundles.tests/unit/specfact_code_review/test__review_utils.py (1)
5-5: Targeted contract test coverage looks correct.The new test cleanly locks the
.py/.pyiinclusion contract and non-Python exclusion behavior for the shared review-path adapter.As per coding guidelines:
tests/**/*.py: Contract-first and integration tests ... Ensure changes to adapters or bridges have targeted coverage.Also applies to: 19-27
packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py (1)
140-164: ReturningNonefor a missingbugs.yamlkeeps older bundles compatible.This is the right no-op boundary for bundles that have not picked up the second Semgrep pass yet.
chore(registry): publish changed modules
…est) - Detect icontract usage across packages/ when reviewing a path under packages/ so MISSING_ICONTRACT is not suppressed when another bundle imports icontract. - Gate pr-orchestrator --require-signature on same-repo PRs to main; document in module-security reference; extend workflow contract test. - Pre-commit review hook exits strictly from report ci_exit_code; clarify summary helper docstring. - Bump specfact-code-review to 0.47.4 and refresh module checksum. Made-with: Cursor
- Resolve icontract scan bundle roots from paths relative to the git repo root so absolute path spellings cannot mis-detect a packages segment; mirror that logic for the monorepo packages/ fallback gate. - Include .pyi files when scanning for icontract imports. - Make batch-level MISSING_ICONTRACT test self-contained under tmp_path. - Bump specfact-code-review to 0.47.5 and refresh manifest checksum. Made-with: Cursor
…r test - Treat bundle layout only when path is repo-relative packages/<bundle>/…, not when "packages" appears deeper in rel_parts; keep permissive fallback when no git root is found. - Add cross-bundle contract test with icontract_anchor.pyi. - Bump specfact-code-review to 0.47.6 and refresh manifest checksum. Made-with: Cursor
fix PR 193 review findings
chore(registry): publish changed modules
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
.github/workflows/pr-orchestrator.yml (1)
95-98:⚠️ Potential issue | 🟠 MajorFork PRs to
mainstill bypass strict signature verification.The
HEAD_REPO == THIS_REPOguard means a fork can change signed manifests on amainPR, pass the PR verifier without--require-signature, and only fail after merge on themainpush.mainshould fail closed here too.🔐 Suggested fix
- elif [ "$TARGET_BRANCH" = "main" ] && [ "$HEAD_REPO" = "$THIS_REPO" ]; then + elif [ "$TARGET_BRANCH" = "main" ]; then VERIFY_CMD+=(--require-signature)Based on learnings: "Signed module or manifest changes require version-bump review and verify-modules-signature" and "Enforce module signatures and version bumps when signed module assets or manifests are affected."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-orchestrator.yml around lines 95 - 98, The conditional that appends --require-signature to VERIFY_CMD currently gates on both TARGET_BRANCH == "main" and HEAD_REPO == THIS_REPO, letting fork PRs bypass signature verification; update the logic so that when TARGET_BRANCH is "main" you always append --require-signature regardless of HEAD_REPO/THIS_REPO, e.g., change the elif condition to only check TARGET_BRANCH == "main" (leave the dev branch metadata-only handling as-is) so signed manifests on main PRs are verified closed.tests/cli-contracts/specfact-code-review-run.scenarios.yaml (1)
104-149:⚠️ Potential issue | 🟠 MajorAssert the JSON report contents, not stdout, in these new
--focus/--levelcases.These scenarios still run with
--json, but the command implementation writes the JSON payload toreview-report.jsonand prints only that path on stdout. That means the newstdout_containschecks for selected paths and"severity":"error"can never validate the filtered report contents. Switch these to file-content assertions via--out, or drop--jsonand assert the rendered output instead.Based on learnings, perform
spec -> tests -> failing evidence -> code -> passing evidencein that order for behavior changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli-contracts/specfact-code-review-run.scenarios.yaml` around lines 104 - 149, The scenarios focus-source-and-docs-union, focus-tests-narrows-to-test-tree and level-error-json-clean-module currently use --json but assert JSON data via stdout_contains; update each scenario to assert the actual JSON report file contents instead of stdout by adding an --out <path> arg (or remove --json and assert rendered stdout) and replace stdout_contains checks with file_content_contains checks that validate the JSON payload (e.g., for focus scenarios assert the presence of the selected paths in the output file, and for level-error-json-clean-module assert '"severity":"error"' inside the generated report file).
🤖 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/tasks.md`:
- Line 56: Update the task evidence command to match the repository-enforced
gate by replacing the standalone invocation "specfact code review run --json
--out .specfact/code-review.json" with the gated form "hatch run specfact code
review run --json --out .specfact/code-review.json" in the tasks.md checkbox
(the line containing "[x] 7.3 Run ..."); ensure the rest of the text/formatting
remains unchanged so evidence reflects CI/local gate behavior.
In `@packages/specfact-code-review/src/specfact_code_review/review/commands.py`:
- Around line 67-75: The block in commands.py wrongly forces
resolved_include_tests = True for any --focus selection causing run_command() to
see a conflict with include_tests; change the assignment so
resolved_include_tests is set only when the focus actually includes the "tests"
facet (e.g., resolved_include_tests = ("tests" in focus_list)) and keep the
existing explicit conflict check (the raise typer.BadParameter when
include_tests/exclude_tests is provided) intact; update references to
focus_list, resolved_include_tests, run_command, focus_facets, include_tests,
and exclude_tests accordingly so the user-facing conflict is only triggered when
the user actually passes conflicting flags.
In `@packages/specfact-code-review/src/specfact_code_review/run/commands.py`:
- Around line 608-615: The current include_for_resolve only enables test-file
discovery when request.include_tests is true or "tests" is in
request.focus_facets, which prevents other facets like "docs" from discovering
Python candidates; change the logic used before calling _resolve_files so
include_for_resolve = request.include_tests or bool(request.focus_facets) (i.e.,
enable test-file discovery whenever any focus_facets are provided), then call
_resolve_files(request.files, include_tests=include_for_resolve,
scope=request.scope, path_filters=request.path_filters or []) and keep the
existing _filter_files_by_focus(resolved_files, request.focus_facets) step to
apply the facet union after resolution.
In `@README.md`:
- Line 51: Update the README sentence to clarify that `--require-signature` is
only enforced for PRs into `main` when the workflow detects the PR comes from
the same repository (the workflow appends `--require-signature` only when
`TARGET_BRANCH="main"` AND `HEAD_REPO == THIS_REPO`), and that forks do not get
the strict `--require-signature` guard; keep the note that `pr-orchestrator`
runs `verify-modules-signature` with `--payload-from-filesystem
--enforce-version-bump` by default and that pre-commit
(`scripts/pre-commit-verify-modules-signature.sh`) mirrors CI by using
`--require-signature` on branch `main` or when `GITHUB_BASE_REF=main`.
In `@tests/unit/specfact_code_review/tools/test_radon_runner.py`:
- Around line 14-22: Update the assertion in
test_run_radon_returns_empty_when_only_non_python_paths to use a strict
empty-list check instead of a falsy check: call run_radon([manifest]) and assert
it == [] (mirroring the style used in run_basedpyright tests) so the test fails
if run_radon ever returns None or another falsy non-list value; keep the
existing monkeypatched subprocess.run (run_mock) and
run_mock.assert_not_called() as-is.
In `@tests/unit/test_git_branch_module_signature_flag_script.py`:
- Around line 12-17: Add a new test that runs the script with GITHUB_BASE_REF
removed from the environment and asserts it returns exit code 0 and prints
"omit"; specifically, create a test named like
test_git_branch_module_signature_flag_script_omits_when_base_ref_unset that uses
SCRIPT_PATH to run the script via subprocess.run with env built from os.environ
but excluding "GITHUB_BASE_REF", capture_output=True and text=True, then assert
result.returncode == 0 and result.stdout.strip() == "omit" to document the local
(unset) behavior.
---
Duplicate comments:
In @.github/workflows/pr-orchestrator.yml:
- Around line 95-98: The conditional that appends --require-signature to
VERIFY_CMD currently gates on both TARGET_BRANCH == "main" and HEAD_REPO ==
THIS_REPO, letting fork PRs bypass signature verification; update the logic so
that when TARGET_BRANCH is "main" you always append --require-signature
regardless of HEAD_REPO/THIS_REPO, e.g., change the elif condition to only check
TARGET_BRANCH == "main" (leave the dev branch metadata-only handling as-is) so
signed manifests on main PRs are verified closed.
In `@tests/cli-contracts/specfact-code-review-run.scenarios.yaml`:
- Around line 104-149: The scenarios focus-source-and-docs-union,
focus-tests-narrows-to-test-tree and level-error-json-clean-module currently use
--json but assert JSON data via stdout_contains; update each scenario to assert
the actual JSON report file contents instead of stdout by adding an --out <path>
arg (or remove --json and assert rendered stdout) and replace stdout_contains
checks with file_content_contains checks that validate the JSON payload (e.g.,
for focus scenarios assert the presence of the selected paths in the output
file, and for level-error-json-clean-module assert '"severity":"error"' inside
the generated report file).
🪄 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: b90e283d-1676-4abf-ab71-ef9bc226513b
📒 Files selected for processing (34)
.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.ymlREADME.mddocs/modules/code-review.mddocs/reference/module-security.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.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_utils.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pypackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.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.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.pyscripts/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/tools/test_basedpyright_runner.pytests/unit/specfact_code_review/tools/test_contract_runner.pytests/unit/specfact_code_review/tools/test_radon_runner.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pytests/unit/test_git_branch_module_signature_flag_script.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/workflows/test_sign_modules_on_approval.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/tools/test_basedpyright_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pytests/unit/specfact_code_review/tools/test_radon_runner.pypackages/specfact-code-review/src/specfact_code_review/_review_utils.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.pytests/unit/test_git_branch_module_signature_flag_script.pytests/unit/workflows/test_sign_modules_on_approval.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.pytests/unit/specfact_code_review/tools/test_contract_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pyscripts/pre_commit_code_review.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/scripts/test_pre_commit_code_review.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pytests/unit/workflows/test_pr_orchestrator_signing.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/tools/test_basedpyright_runner.pytests/unit/specfact_code_review/tools/test_radon_runner.pytests/unit/test_git_branch_module_signature_flag_script.pytests/unit/workflows/test_sign_modules_on_approval.pytests/unit/specfact_code_review/tools/test_contract_runner.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/workflows/test_pr_orchestrator_signing.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/specs/sidecar-route-extraction/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md
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/radon_runner.pypackages/specfact-code-review/src/specfact_code_review/_review_utils.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.pypackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/run/commands.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-codebase/module-package.yamlpackages/specfact-code-review/module-package.yaml
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
Files:
.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.yml
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/reference/module-security.mddocs/modules/code-review.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 (15)
📓 Common learnings
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: .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: .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
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
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/`
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: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Work belongs on feature/*, bugfix/*, hotfix/*, or chore/* branches, normally in a worktree rooted under ../specfact-cli-modules-worktrees/
📚 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.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdpackages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pyREADME.mdscripts/pre_commit_code_review.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamlpackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/scripts/test_pre_commit_code_review.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mddocs/modules/code-review.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:
packages/specfact-code-review/.semgrep/bugs.yamlpackages/specfact-codebase/module-package.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md.github/workflows/sign-modules-on-approval.ymlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdpackages/specfact-code-review/module-package.yamlREADME.mdscripts/pre_commit_code_review.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamltests/unit/scripts/test_pre_commit_code_review.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mddocs/modules/code-review.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: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
packages/specfact-codebase/module-package.yaml.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.ymldocs/reference/module-security.mdpackages/specfact-code-review/module-package.yamlREADME.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:
packages/specfact-codebase/module-package.yamltests/unit/test_git_branch_module_signature_flag_script.py.github/workflows/pr-orchestrator.ymltests/unit/workflows/test_sign_modules_on_approval.py.github/workflows/sign-modules-on-approval.ymldocs/reference/module-security.mdpackages/specfact-code-review/module-package.yamlREADME.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: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
packages/specfact-codebase/module-package.yaml.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.ymldocs/reference/module-security.mdpackages/specfact-code-review/module-package.yamlREADME.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/specs/contract-runner/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md.github/workflows/sign-modules-on-approval.ymlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdREADME.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamlpackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mddocs/modules/code-review.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/specs/contract-runner/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md.github/workflows/sign-modules-on-approval.ymldocs/reference/module-security.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdREADME.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.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/specs/review-cli-contracts/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdpackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pyREADME.mdscripts/pre_commit_code_review.pyopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.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: 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/specs/review-cli-contracts/spec.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.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:
.github/workflows/pr-orchestrator.ymlREADME.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: Require public GitHub metadata completeness before implementation when linked issue workflow applies: parent, labels, project assignment, blockers, and blocked-by relationships
Applied to files:
.github/workflows/sign-modules-on-approval.yml
📚 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:
docs/reference/module-security.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/specs/code-review-bug-finding/spec.mdpackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pyopenspec/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: 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:
README.md
🔀 Multi-repo context nold-ai/specfact-cli
Findings
[::nold-ai/specfact-cli::]
- scripts/verify-modules-signature.py
- New flag/behavior: --metadata-only and new helper verify_manifest_integrity_shape_only(...) alter when public-key resolution / signature enforcement occurs. Files referencing the verifier: many docs/scripts/workflows and pre-commit wrappers (examples below). Relevant locations:
- .github/workflows/pr-orchestrator.yml (calls verify-modules-signature.py with --version-check-base, conditional --require-signature)
- .github/workflows/sign-modules.yml (signing workflow calls script)
- scripts/pre-commit-verify-modules-signature.sh (invokes verify-modules-signature wrapper)
- scripts/pre-commit-verify-modules.sh, scripts/setup-git-hooks.sh (docs/usage)
- docs/guides/module-signing-and-key-rotation.md, docs/reference/module-security.md, many openspec files reference verify-modules-signature invocation.
- New flag/behavior: --metadata-only and new helper verify_manifest_integrity_shape_only(...) alter when public-key resolution / signature enforcement occurs. Files referencing the verifier: many docs/scripts/workflows and pre-commit wrappers (examples below). Relevant locations:
[::nold-ai/specfact-cli::]
- run_review API surface changed (signature and plumbing):
- packages/specfact-code-review/src/specfact_code_review/run/init.py — run_review signature extended with bug_hunt, review_level, review_mode.
- packages/specfact-code-review/src/specfact_code_review/run/runner.py — run_review implementation updated to accept/propagate bug_hunt/review_level/review_mode and to alter behavior (shadow mode forcing ci_exit_code=0, filtering by review_level, added semgrep bug pass, tool-availability gating).
- packages/specfact-code-review/src/specfact_code_review/review/commands.py — CLI exposes new options (--focus, --mode, --level, --bug-hunt) and wires through to run_review.
- Observed internal callers/tests referencing run_review and run CLI in tests under tests/unit and tests/integration. Any external consumers importing/specfact_code_review.run.run_review must update to the new signature/kwargs.
[::nold-ai/specfact-cli::]
- Tool input normalization and availability APIs:
- packages/specfact-code-review/src/specfact_code_review/_review_utils.py — added python_source_paths_for_tools(...) and _PYTHON_LINTER_SUFFIXES; multiple runners now call it.
- packages/specfact-code-review/src/specfact_code_review/tools/tool_availability.py — new public functions/types: skip_if_tool_missing(tool_id, file_path) and skip_if_pytest_unavailable(file_path). Runners now may return skip-findings instead of failing when CLI tools are missing.
- Runners updated to call python_source_paths_for_tools and skip_if_tool_missing: semgrep_runner.py, ruff_runner.py, pylint_runner.py, radon_runner.py, basedpyright_runner.py, run_contract_check in contract_runner.py, etc. Tests added/updated accordingly.
[::nold-ai/specfact-cli::]
- Semgrep bug-pass and config discovery:
- packages/specfact-code-review/.semgrep/bugs.yaml added and packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py exposes new run_semgrep_bugs(...) and find_semgrep_bugs_config(...). Runner integration present in run/runner.py.
[::nold-ai/specfact-cli::]
- Sidecar framework extractor changes (excluded dirs and iteration):
- packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py — added _EXCLUDED_DIR_NAMES and _iter_python_files; FastAPI/Flask/Django/DRF extractors changed to use iterator and exclude .specfact venvs. Tests added verifying behavior.
[::nold-ai/specfact-cli::]
- Registry / module manifest bumps and integrity changes:
- packages/specfact-code-review/module-package.yaml version bumped (0.46.4 → 0.47.6) and integrity.checksum updated; signature removed.
- packages/specfact-codebase/module-package.yaml version bumped (0.41.4 → 0.41.8) and integrity updated.
- registry/index.json entries updated for specfact-codebase and specfact-code-review; multiple registry/*.sha256 and registry/signatures/ added.
- Impact: downstream consumers that pin module versions/checksums or expect the removed signature field may need to reconcile.
Summary assessment
- The changes touch several public/internal APIs and CLI behaviors that are consumed elsewhere within this repository (new CLI flags, run_review signature, verify-modules-signature flag semantics). External repositories or automation that:
- call specfact_code_review.run.run_review programmatically,
- invoke scripts/verify-modules-signature.py expecting prior semantics,
- pin registry/module versions or expect the previous manifest signature field,
should be reviewed and updated accordingly.
🔇 Additional comments (43)
packages/specfact-codebase/module-package.yaml (2)
26-27: Checksum format looks valid.The integrity checksum follows the expected
sha256:<64-hex-chars>format. The previous CI mismatch was marked as addressed in commit 6765811.
2-2: Confirm module-package.yaml version aligns with registry publication and verify semver compliance.The manifest version was bumped to 0.41.8 with a corresponding checksum update. Ensure this version number and checksum will be published to the registry/index.json as part of the promotion or subsequent publish workflow, and verify that the semver bump (0.41.4 → 0.41.8) reflects the extent of changes in the packaged source code.
packages/specfact-code-review/module-package.yaml (2)
1-2: Version bump and metadata look correct.Version
0.47.6with updated checksum aligns with the feature work merged from dev. Thecore_compatibility: '>=0.44.0,<1.0.0'constraint appropriately gates against older CLI versions that lack the new run_review signature (bug_hunt, review_level, review_mode kwargs).
25-26: Signature field removal is intentional and properly supported in downstream code.The removed
integrity.signaturefield is defined as optional in specfact-cli's module package model and is handled gracefully throughout the codebase. Code that reads this field uses conditional checks (if meta.integrity and meta.integrity.signature:) and a--require-signatureflag for conditional enforcement. Official-tier modules require signatures, but non-official tier modules do not, and the--metadata-onlymode explicitly supports modules without signatures on non-main targets.No action needed; version bump (0.46.4 → 0.47.6) and checksum update are correct.
packages/specfact-code-review/src/specfact_code_review/_review_utils.py (1)
34-43: LGTM —.pyisupport addressed.The
_PYTHON_LINTER_SUFFIXESnow includes both.pyand.pyi, addressing the prior feedback about aligning with pre-commit filtering for type stub analysis. The beartype/icontract contracts provide appropriate runtime validation.packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py (2)
46-58: Traversal pruning implemented correctly.The
os.walkapproach withdirnames[:] = [...]modification now prunes excluded subtrees before descending, addressing the prior feedback about expensive traversal into.specfact/venv,.git, etc. This prevents the O(n) post-filter overhead on large sidecar environments.
37-44: Exclusion set covers expected sidecar artifacts.The
_EXCLUDED_DIR_NAMESfrozenset correctly includes.specfact,.git,__pycache__,node_modules,venv, and.venv. This aligns with the spec requirements and prevents framework extractors from scanning installed dependencies.openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md (1)
1-41: Spec aligns with implementation — LGTM.The specification correctly documents the
.specfact/exclusion requirement and per-framework scenarios. The H1 heading issue from the prior review is resolved. The route-count scenario (Line 39-41) provides a concrete regression guard for the gpt-researcher test case.packages/specfact-code-review/.semgrep/bugs.yaml (3)
36-40: Multi-line yaml.load detection addressed.The
pattern-not-regexnow uses(?s)(DOTALL) with[\s\S]*?to correctly matchLoader=across newlines, addressing the prior feedback about false positives on multi-line calls.
50-56: Useless-comparison rule may flag intentional identity checks.The pattern
$X == $Xcatches always-true comparisons, but note thatx == xcan be intentional in rare cases (e.g., testing operator overloads). Since the severity is WARNING and thespecfact-categoryisclean_code, this is appropriate for advisory flagging. Thex != xNaN-check idiom is unaffected.
1-12: Security rules cover high-confidence patterns — LGTM.The
eval/execrules appropriately use ERROR severity for arbitrary code execution risks. The bundled ruleset provides a solid baseline for the bug-finding pass without excessive noise.packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py (2)
168-201: Typer exemption hardened with decorator detection — LGTM.The exemption now requires both the
ctx: *Contextannotation and acommanddecorator (via_has_typer_command_decorator). This addresses the prior feedback about pattern fragility. The decorator check handles both bare@commandand attribute forms like@app.command(...).
311-319: Tool availability gating is a good resilience pattern.The early
skip_if_tool_missing("radon", ...)check gracefully handles environments where Radon isn't installed, returning a skip-finding rather than failing. Combined withpython_source_paths_for_tools()normalization, this makes the runner more robust across varied CI/local setups.tests/unit/test_git_branch_module_signature_flag_script.py (1)
20-33: Behavioral assertions now included — addresses prior feedback.The subprocess-based tests verify the actual script behavior:
GITHUB_BASE_REF=mainyieldsrequire, and non-main branches yieldomit. This provides the contract coverage the prior review requested.scripts/pre_commit_code_review.py (4)
56-57: LGTM — module-package.yaml exclusion aligns with module-signing workflow.Excluding
module-package.yamlfrom the pre-commit code-review gate is correct. Per learnings, signed module or manifest changes require separate version-bump review andverify-modules-signature, not the clean-code gate.
211-264: Good fix:ci_exit_codefrom report now drives the gate exit.The refactored return type
tuple[bool, int | None, int | None]and the explicitci_exit_codeextraction address the prior contract violation where fixable error-severity findings could incorrectly pass the gate. The normalization fallback at lines 243-244 is sound—deriving fromoverall_verdictwhenci_exit_codeis not0/1.
325-328: Exit code now respects report contract.Returning
int(ci_exit_code)instead ofresult.returncodeensures the gate honors the reviewer's actual blocking/non-blocking verdict rather than subprocess exit semantics.
87-97: Python-only filtering is deterministic and correctly scoped.
_specfact_review_pathsnow filters to.py/.pyionly, which aligns with the SpecFact code-review tool's Python-centric analysis surface. Non-Python artifacts (Markdown, YAML manifests, registry tarballs) are correctly excluded.packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py (4)
40-44: Good fix: FastAPI exclusions now extend base class properly.Using
super()._path_touches_excluded_dir(path)plus the FastAPI-specific cache directories maintains the adapter boundary withBaseFrameworkExtractorwhile adding framework-specific needs. This addresses the prior review concern about reimplementing path-skip logic.
200-229: Multi-decorator accumulation now correct.
_extract_routes_from_functionaccumulates all(method, path)pairs intomatched_routesand emits oneRouteInfoper pair. This fixes the previous last-decorator-wins behavior where stacked decorators like@router.get("/read")and@router.post("/write")would lose earlier routes.
176-198:api_route(methods=[...])handling looks correct.The method correctly extracts multiple HTTP methods from the
methodskeyword argument and defaults to["GET"]when none are specified. One minor observation:methodsiteration (lines 188-195) only handlesast.Listandast.Tuple—asetliteral would be skipped, though that's an uncommon pattern for route definitions.
129-131: Placeholder acknowledgment is fine.The
_ = (repo_path, routes)assignment explicitly marks these parameters as reserved for future Pydantic schema mining, satisfying linter unused-variable warnings while documenting intent.packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py (3)
110-127: Symbol registration is now wired into route extraction.The prior review flagged
_register_flask_assign_symbolsas dead code. This has been addressed: the first pass populatesapp_names/bp_names, which are then passed to_extract_route_from_functionand checked via_is_owned_route_decorator(line 197). Only decorators referencing registered Flask/Blueprint symbols are accepted as valid routes.Also applies to: 145-149, 231-235
51-51: Flask now uses shared_iter_python_filesiterator.Both
detect()andextract_routes()now useself._iter_python_files(search_path)instead ofrglob("*.py"), aligning with the base class's excluded-directory traversal and maintaining adapter consistency with FastAPI/Django/DRF extractors.Also applies to: 82-82
188-188: Unused tuple assignment is acceptable.
_ = (imports, py_file)silences unused-variable warnings while preserving the function signature for potential future use. This is a common Python idiom.tests/unit/specfact_codebase/test_sidecar_framework_extractors.py (4)
22-38: Good contract test forapi_route(methods=[...])resolution.This test validates that
APIRouter.api_route(..., methods=["GET", "POST"])correctly yields both HTTP methods as distinct routes, covering the core behavior change in the FastAPI extractor.
41-56: Multi-decorator preservation test is well-constructed.Verifies the fix for the prior last-decorator-wins bug by asserting both
("GET", "/read")and("POST", "/write")are extracted from a single function with stacked decorators.
80-102: Critical venv exclusion test for sidecar isolation.This test ensures routes under
.specfact/venv/...are not extracted, preventing the sidecar from counting framework-installed dependencies as application routes. This is essential for accurate route extraction in sidecar validation.
105-129: Flask symbol registration test validates ownership checks.The test correctly expects
@other.route("/ignored")to be excluded sinceotheris never registered as a Flask app or Blueprint. This confirms the symbol-tracking pass is effective.Note: Line 120's
@other.route("/ignored")will raise aNameErrorat runtime if executed, but since this is parsed via AST (not executed), the test is valid.tests/unit/scripts/test_pre_commit_code_review.py (4)
116-146: Excellent contract test for warning-only non-blocking behavior.This test validates that when
ci_exit_code=0butoverall_verdict="FAIL"with only warning findings, the gate passes (exit 0). This is the correct behavior—warnings don't block, matching the SpecFact review contract.
192-209: Critical test for fixable-error passthrough.When
ci_exit_code=0andoverall_verdict="PASS_WITH_ADVISORY"with a fixable error finding, the gate should pass. This test confirms the script now correctly uses the report'sci_exit_coderather than counting errors, addressing the prior contract violation.
52-56: Good boundary test for module-package.yaml exclusion.Confirms that
module-package.yamlpaths are excluded from the review gate, aligning with the module-signing workflow where manifest changes go throughverify-modules-signatureinstead.
37-49: Python-only filtering test updated correctly.The test now expects only
.py/.pyifiles to pass through_specfact_review_paths, with registry artifacts, YAML manifests, and Markdown excluded.packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py (5)
312-314: Tool availability now unified across both Semgrep passes.Both
run_semgrep(line 312-314) andrun_semgrep_bugs(line 410-412) useskip_if_tool_missing("semgrep", files[0])before proceeding. This addresses the prior review concern about splitting the adapter contract—both passes now rely on the sharedtool_availabilitylayer.Also applies to: 410-412
240-267: Shared field extraction reduces duplication.
_extract_common_finding_fieldsconsolidates the validation logic for path normalization, rule extraction, and message parsing. Both_finding_from_resultand_finding_from_bug_resultnow call this helper, applying only their specific normalizers and category maps. This addresses the DRY concern from prior review.
135-163: Bug config discovery mirrors clean-code pattern.
find_semgrep_bugs_configfollows the same parent-walk logic asfind_semgrep_configbut returnsNoneinstead of raising when absent. This allowsrun_semgrep_bugsto gracefully no-op whenbugs.yamlisn't present.
401-431:run_semgrep_bugscontract is clear and consistent.The function:
- Returns
[]for empty files- Returns
[]when semgrep is missing (skip finding already emitted by first pass)- Returns
[]whenbugs.yamlis absent- Runs the bug pass and maps findings when all preconditions are met
This maintains clean adapter boundaries with the tool-availability layer.
166-194: Config decoupling improves testability.
_run_semgrep_commandnow accepts an explicitconfig_file: Pathparameter instead of resolving it internally. This separation makes the execution logic reusable across both the clean-code and bug passes.tests/unit/specfact_code_review/tools/test_basedpyright_runner.py (2)
18-26: Good test for YAML manifest filtering with strict assertion.The test verifies that
run_basedpyrightreturns an empty list (not just falsy) when given only amodule-package.yamlpath, and confirmssubprocess.runis never called. This addresses the prior review concern about loose truthy/falsy checks.
14-15: Strict empty-list assertion is correct.
assert run_basedpyright([]) == []explicitly validates the return type, preventing regressions whereNonemight be returned instead.tests/unit/specfact_code_review/tools/test_radon_runner.py (1)
112-152: Typer context exemption tests are well-structured.These tests validate the KISS parameter-count exemption logic:
- Without
@app.command()decorator:typer.Contextparameter is counted → warning emitted- With
@app.command()decorator:typer.Contextparameter is exempted → no warningThis ensures the exemption is decorator-aware, not just type-aware.
packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py (1)
321-329: Exclude.pyistubs from the runtime AST/CrossHair passes.
python_source_paths_for_tools()now admits.pyi, and this block feeds that list straight into_scan_file()and_run_crosshair(). A changed stub likedef public_api(...) -> ...will be reported asMISSING_ICONTRACT, even though declaration files are not the runtime surface that carries@require/@ensure.🩹 Suggested fix
def run_contract_check(files: list[Path], *, bug_hunt: bool = False) -> list[ReviewFinding]: """Run AST-based contract checks and a CrossHair fast pass for the provided files.""" - py_files = python_source_paths_for_tools(files) - if not py_files: + python_files = python_source_paths_for_tools(files) + if not python_files: return [] + runtime_files = [file_path for file_path in python_files if file_path.suffix == ".py"] findings: list[ReviewFinding] = [] - if _has_icontract_usage(py_files): - for file_path in py_files: + if _has_icontract_usage(python_files): + for file_path in runtime_files: findings.extend(_scan_file(file_path)) - findings.extend(_run_crosshair(py_files, bug_hunt=bug_hunt)) + findings.extend(_run_crosshair(runtime_files, bug_hunt=bug_hunt)) return findingsdocs/modules/code-review.md (1)
40-55: Documentation updates are consistent with the new review pipeline behavior.These additions correctly capture the new CLI/runtime contract (
--bug-hunt,--mode,--focus,--level), semgrep bug-pass behavior, and runner orchestration/exit semantics.Based on learnings: “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.”
Also applies to: 99-102, 270-276, 280-310, 405-447
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
registry/index.json (1)
1-96: 🧹 Nitpick | 🔵 TrivialConsider enhancing registry validation to catch cross-file inconsistencies.
The current validation tool (
tools/validate_repo_manifests.py, lines 33-47) only checks JSON structure and the presence of a"modules"list. It does not verify that:
checksum_sha256inregistry/index.jsonmatches the corresponding.tar.gz.sha256filelatest_versionand checksums inregistry/index.jsonmatch themodule-package.yamlfiles- Download URLs follow the expected naming pattern
These cross-file consistency checks would prevent potential publish-time failures and ensure registry integrity.
📋 Enhancement suggestion for validation tooling
Consider adding a validation function to
tools/validate_repo_manifests.py:def _validate_registry_consistency(registry_path: Path) -> list[str]: """Validate that registry/index.json matches SHA256 files and module-package.yaml.""" errors = [] data = json.loads(registry_path.read_text(encoding="utf-8")) for module in data.get("modules", []): module_id = module.get("id", "unknown") version = module.get("latest_version") checksum = module.get("checksum_sha256") download_url = module.get("download_url") # Verify SHA256 file exists and matches sha_file = registry_path.parent / download_url.replace(".tar.gz", ".tar.gz.sha256") if sha_file.exists(): file_checksum = sha_file.read_text(encoding="utf-8").strip() if file_checksum != checksum: errors.append(f"{module_id}: checksum mismatch between index.json and {sha_file.name}") # Verify module-package.yaml version and checksum match # (implementation depends on package structure) return errors🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/index.json` around lines 1 - 96, The registry validator (tools/validate_repo_manifests.py) currently only checks JSON shape; add a new validation function (e.g., _validate_registry_consistency) that iterates data["modules"] and for each module (use module["id"], module["latest_version"], module["checksum_sha256"], module["download_url"]) verifies: 1) the corresponding .tar.gz.sha256 file exists next to the download_url and its contents equal checksum_sha256, 2) the module's latest_version and checksum match the module-package.yaml inside the package (open/read module-package.yaml to compare version and checksum fields), and 3) download_url conforms to the expected naming pattern (module-id-version.tar.gz); collect and return human-readable errors and wire this function into the main validation flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@registry/modules/specfact-code-review-0.47.6.tar.gz.sha256`:
- Line 1: The SHA256 in
registry/modules/specfact-code-review-0.47.6.tar.gz.sha256 does not match the
integrity field in packages/specfact-code-review/module-package.yaml; verify
which artifact is authoritative by checking the actual tarball
(specfact-code-review-0.47.6.tar.gz), compute its SHA256 (e.g., sha256sum or
shasum -a 256), and then either regenerate/update
registry/modules/specfact-code-review-0.47.6.tar.gz.sha256 to the computed value
or update the integrity field in
packages/specfact-code-review/module-package.yaml to include the correct
sha256:<computed_hash>; ensure the final values match exactly and commit the
corrected file.
In `@registry/modules/specfact-codebase-0.41.8.tar.gz.sha256`:
- Line 1: There is a checksum mismatch between
registry/modules/specfact-codebase-0.41.8.tar.gz.sha256 and the integrity value
in packages/specfact-codebase/module-package.yaml; compute the actual SHA256 of
registry/modules/specfact-codebase-0.41.8.tar.gz (e.g., sha256sum), compare it
with the manifest's sha256:4e9e9888... value in module-package.yaml, and update
registry/modules/specfact-codebase-0.41.8.tar.gz.sha256 to the correct checksum
(the one matching the computed hash or, if the manifest is authoritative,
replace the file content with the manifest's checksum), then run
verify-modules-signature to validate the module integrity.
---
Outside diff comments:
In `@registry/index.json`:
- Around line 1-96: The registry validator (tools/validate_repo_manifests.py)
currently only checks JSON shape; add a new validation function (e.g.,
_validate_registry_consistency) that iterates data["modules"] and for each
module (use module["id"], module["latest_version"], module["checksum_sha256"],
module["download_url"]) verifies: 1) the corresponding .tar.gz.sha256 file
exists next to the download_url and its contents equal checksum_sha256, 2) the
module's latest_version and checksum match the module-package.yaml inside the
package (open/read module-package.yaml to compare version and checksum fields),
and 3) download_url conforms to the expected naming pattern
(module-id-version.tar.gz); collect and return human-readable errors and wire
this function into the main validation flow.
🪄 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: 1abca289-7126-48c6-8b31-77b1ad05aa15
⛔ Files ignored due to path filters (2)
registry/modules/specfact-code-review-0.47.6.tar.gzis excluded by!**/*.gzregistry/modules/specfact-codebase-0.41.8.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (3)
registry/index.jsonregistry/modules/specfact-code-review-0.47.6.tar.gz.sha256registry/modules/specfact-codebase-0.41.8.tar.gz.sha256
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
registry/**
⚙️ CodeRabbit configuration file
registry/**: Registry and index consistency: bundle listings, version pins, and compatibility with
published module artifacts.
Files:
registry/modules/specfact-codebase-0.41.8.tar.gz.sha256registry/modules/specfact-code-review-0.47.6.tar.gz.sha256registry/index.json
🧠 Learnings (4)
📓 Common learnings
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: .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
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: Enforce module signatures and version bumps when signed module assets or manifests are affected
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
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: .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/`
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-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-codebase-0.41.8.tar.gz.sha256registry/modules/specfact-code-review-0.47.6.tar.gz.sha256registry/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.6.tar.gz.sha256registry/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: 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/index.json
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
[::nold-ai/specfact-cli::]
-
Consumer: pre-commit & CI wrappers referencing verify-modules-signature
- scripts/pre-commit-verify-modules.sh — invokes verify-modules-signature.py with branch-based flags (lines showing exec hatch run ./scripts/verify-modules-signature.py --require-signature --enforce-version-bump --payload-from-filesystem) [::nold-ai/specfact-cli::]
- scripts/pre-commit-quality-checks.sh — references legacy pre-commit verify wrapper path (scripts/pre-commit-verify-modules-signature.sh) [::nold-ai/specfact-cli::]
- scripts/setup-git-hooks.sh — docs/usage recommending hatch run ./scripts/verify-modules-signature.py --require-signature (docs/hooks) [::nold-ai/specfact-cli::]
- scripts/pre-commit-verify-modules-signature.sh — new wrapper (added in PR) delegates to verify-modules-signature helper script referenced throughout repo [::nold-ai/specfact-cli::]
-
Consumer: CI/workflow and docs references to verify-modules-signature
- .github/workflows/pr-orchestrator.yml (workflow logic in PR) — now composes VERIFY_CMD including conditional --require-signature / --metadata-only; tests and docs assert presence (tests/unit/workflows/* reference) [::nold-ai/specfact-cli::]
- .github/workflows/sign-modules-on-approval.yml — signing workflow uses the verify/sign scripts (gating changes in PR) — reflected in tests [::nold-ai/specfact-cli::]
- Numerous docs and OpenSpec files call verify-modules-signature.py (docs/guides/module-signing-and-key-rotation.md, docs/reference/module-security.md, openspec/*) — these external callers rely on the CLI semantics and flags changed by the PR [::nold-ai/specfact-cli::]
-
Consumer: tests that assert presence/shape of verifier
- tests/unit/specfact_cli/registry/test_signing_artifacts.py — asserts scripts/verify-modules-signature.py exists and is referenced in content (so CLI changes must preserve expected file/name) [::nold-ai/specfact-cli::]
- tests/unit/scripts/test_pre_commit_verify_modules.py & tests/unit/test_pre_commit_verify_modules_signature_script.py — unit tests updated to expect new wrapper/case structure around require/omit and --metadata-only; they directly exercise the script text and behavior [::nold-ai/specfact-cli::]
- tests/unit/workflows/* — tests check workflow composition for verify flags and gating behavior (PR adds/changes these expectations) [::nold-ai/specfact-cli::]
-
Consumer: programmatic run_review mention
- tests/unit/specfact_cli/test_dogfood_self_review.py:def _run_review() -> dict — there is a test helper that calls run_review; PR changes run_review signature (adds bug_hunt/review_level/review_mode) — tests within repo will need to pass new kwargs or remain compatible [::nold-ai/specfact-cli::]
- openspec/changes/.../design.md contains earlier signature example def run_review(files: list[Path], options: ReviewOptions) — documentation may need updating to match new signature/kwargs [::nold-ai/specfact-cli::]
-
Consumers of new utilities and types (python_source_paths_for_tools, skip_if_tool_missing, ReviewToolId)
- Multiple tool runners and modules now call python_source_paths_for_tools and skip_if_tool_missing (seen across PR changes). Within this repo references are present in updated modules and tests; any external code that invoked run_* tool runners directly (rare) would need to pass Path lists and accept skip-findings semantics — tests in this repo were updated accordingly (tests/unit/specfact_code_review/tools/*) [::nold-ai/specfact-cli::]
Notes:
- verify-modules-signature.py is widely invoked by wrappers, CI workflows, docs, and tests. The PR changes its CLI behavior (adds --metadata-only, changes when --require-signature is used). All callers listed above (scripts, CI, docs, tests) are directly relevant and many are already updated in the PR’s changes and tests.
- run_review signature changes are exercised by local tests in this repo; any external repos calling specfact_code_review.run.run_review programmatically must be checked/updated to pass new keyword args.
🔇 Additional comments (2)
registry/index.json (2)
33-35: Registry update for specfact-codebase 0.41.8 is consistent.The version bump to 0.41.8, download URL, and checksum are correctly aligned with the corresponding SHA256 file. The checksum
14f3a799d79c1d919755f258ce99a9ed1a0415488e9e9790821b080295a9d555matchesregistry/modules/specfact-codebase-0.41.8.tar.gz.sha256.
81-83: Registry update for specfact-code-review 0.47.6 is consistent.The version bump to 0.47.6, download URL, and checksum are correctly aligned with the corresponding SHA256 file. The checksum
b8b39ecf993f04f266a431871e35171696c8d184cb5e5a41b3edd02bff246e1amatchesregistry/modules/specfact-code-review-0.47.6.tar.gz.sha256.
…blish - Add Module Signature Hardening workflow (parity with specfact-cli): on push to dev/main for packages/**, auto-sign changed manifests with repo secrets, run strict verify-modules-signature --require-signature, then push a follow-up [skip ci] commit; PRs keep checksum-only verify; add reproducibility check on main and manual dispatch signing job. - Teach publish-modules to sign each bundle manifest in-place when the signing secret is set but integrity.signature is missing, so registry tarballs match main gate expectations without racing unsigned trees. - Add workflow contract tests for sign-modules.yml. Made-with: Cursor
…cks) - Gate code review evidence task with hatch run in OpenSpec tasks. - Resolve --focus vs include_tests: tri-state Typer options, tests facet drives include_tests only when tests are focused; drop redundant run_command focus/include conflict. - Widen file resolution when any focus facet is set; keep facet filter. - pr-orchestrator: always --require-signature for PRs to main; README aligned with fork vs signing workflows. - CLI contracts: JSON report file assertions, schema field, integration tests; shadow mode on focus scenarios for stable exit codes. - Strict radon empty-list assertion; git-branch script test without GITHUB_BASE_REF; registry consistency validation in validate_repo_manifests. - Radon KISS: path-stable Typer run() exempt and callback decorator hint; pre-commit review subprocess pins SPECFACT_MODULES_REPO and PYTHONPATH (user-scoped ~/.specfact/modules may still shadow radon_runner until upstream discovery is tightened). Made-with: Cursor
fix: review follow-ups (focus, main signing, registry, contracts)
chore(registry): publish changed modules
chore(modules): auto-sign module manifests
chore(modules): auto-sign module manifests
chore(registry): publish changed modules
1 participant
Summary
Promotes the current
devintegration line tomainafter merged feature work and follow-up fixes.Highlights (from
main..dev)api_route(methods=[...])support, venv /.specfactpath exclusions, contract tests.Verification
pr-orchestrator(dev→main may use the reduced matrix where configured).Merge note
Use the repository’s normal dev → main merge policy (merge commit vs squash per team convention).
Made with Cursor