Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent 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)
🧰 Additional context used📓 Path-based instructions (4)packages/**/module-package.yaml⚙️ CodeRabbit configuration file
Files:
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
tests/**/*.py⚙️ CodeRabbit configuration file
Files:
packages/**/src/**/*.py⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-04-13T10:38:22.837ZApplied to files:
📚 Learning: 2026-04-13T10:38:22.837ZApplied to files:
📚 Learning: 2026-04-13T10:38:43.524ZApplied to files:
🔀 Multi-repo context nold-ai/specfact-clinold-ai/specfact-cli
Potential cross-boundary impacts and breaking-change risk (observed):
Summary: The PR changes workflow gate logic (HEAD_REPO/THIS_REPO), adds a trusted-reviewer association check for signing, and adjusts branch-aware --require-signature behavior. Primary consumers impacted inside this repository are the pr-orchestrator and sign-on-approval workflows, the git-branch-module-signature-flag script, pre-commit verify hook, verify-modules-signature invocations, unit tests that assert workflow content, and documentation/openspec artifacts. These are all present and were updated in-tree; external repositories or CI that depend on the previous semantics should be reviewed to ensure they remain compatible. 🔇 Additional comments (12)
📝 WalkthroughModule surface and API changesspecfact-code-review (version bump → 0.47.6)
specfact-codebase (version bump → 0.41.8)
Manifest and integrity impacts
Cross-repo and CI workflow behavior
Documentation, registry and site implications
OpenSpec change and scenario coverage
Tests & CI validation
Maintainer notes / actions
WalkthroughWorkflows: signing gating and require-signature logic adjusted to use Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9458eebe37
ℹ️ 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".
packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py
Show resolved
Hide resolved
packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md (1)
3-107:⚠️ Potential issue | 🔴 CriticalCritical adapter boundary gap: CLI flag routing not found in specfact-cli core.
This spec and implementation are complete on the module side—all four flags (
--bug-hunt,--mode,--focus,--level) are fully implemented inspecfact-code-review, documented indocs/modules/code-review.md, and theReviewRunRequestschema includes all required fields. However, search of thespecfact-clirepository found no corresponding CLI flag wiring or routing through thecode review runcommand.The proposal references issue
#174inspecfact-cli-modulesbut does not link to a corresponding issue or PR inspecfact-cliwhere the typer CLI argument parsing must be added to expose these flags to users. Without CLI routing in core, the feature is incomplete at the adapter boundary—module and docs are ready, but users cannot invoke the new flags.Verify:
- Is there a paired PR in
specfact-cli(separate or in progress) that adds the CLI flag routing forcode review run?- If paired, ensure the proposal or this spec links to that work so reviewers can validate both sides of the adapter boundary together.
- If not paired, file the corresponding issue in
specfact-cliand update the proposal's source tracking to reference both repos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md` around lines 3 - 107, Summary: The CLI flag wiring for the new code-review flags is missing from specfact-cli; either link the paired PR or create one and update the proposal. Action: Verify whether a specfact-cli PR exists that adds typer argument parsing for the "specfact code review run" command and routes the flags --bug-hunt, --mode, repeated --focus, and --level into the module's ReviewRunRequest fields; if it exists, add the PR link to this proposal/spec; if it does not exist, open an issue/PR in specfact-cli that implements mapping those flags into the code review run handler so they are passed to specfact-code-review (ensure compatibility with --json/--out and existing flags), then update the spec to reference the new issue/PR for cross-repo traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/modules/code-review.md`:
- Around line 405-413: The docs for specfact_code_review.run.runner.run_review
incorrectly show positional arguments after files; update the signature in the
markdown to include the keyword-only marker (*) after the files parameter and
update the surrounding text to state that all parameters following files
(no_tests, include_noise, progress_callback, bug_hunt, review_level,
review_mode) are keyword-only, so callers must pass them by name rather than
position; ensure the example signature and any explanatory prose reflect this
change.
In
`@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md`:
- Around line 16-21: The spec currently only requires scenarios that include the
flags `--bug-hunt --mode shadow` and `--bug-hunt --mode enforce`; change it to
assert the expected exit semantics too by requiring that the
`tests/cli-contracts/specfact-code-review-run.scenarios.yaml` contains at least
one scenario where `--bug-hunt --mode shadow` exits with code 0 (no-blocking
findings) and at least one scenario where `--bug-hunt --mode enforce` exits with
non-zero on blocking findings; update the Scenario text in this spec (and any
scenario templates referenced) to explicitly include those exit-code
expectations rather than just flag presence so the contract locks the behavior.
In `@packages/specfact-code-review/src/specfact_code_review/review/commands.py`:
- Around line 29-41: The helper _friendly_run_command_error currently accepts
ValueError and reduces every branch to str(exc), coupling the CLI adapter to
incidental ValueError inheritance; change its signature to accept
RunCommandError (not ValueError) and update the isinstance checks to reference
only the explicit RunCommandError subclasses you care about (e.g.,
InvalidOptionCombinationError, MissingOutForJsonError, ConflictingScopeError,
FocusFacetConflictError, NoReviewableFilesError) and return their friendly
strings, ensuring any other RunCommandError falls back to str(exc); also check
the run() caller still catches RunCommandError (not ValueError) so the adapter
depends only on the public RunCommandError surface.
In
`@packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py`:
- Line 188: The parameters imports and py_file in _extract_route_from_function
are currently unused and only silenced via "_ = (imports, py_file)"; either
remove these parameters from the function signature and all call sites (update
any callers that pass imports/py_file to stop doing so) or explicitly mark them
as intentionally reserved by keeping them and adding a clear TODO comment inside
_extract_route_from_function referencing future schema resolution, e.g., "TODO:
use imports and py_file for schema resolution" and remove the "_ = ..."
suppression; choose one approach and apply it consistently to the function and
its callers.
In `@scripts/pre_commit_code_review.py`:
- Around line 238-240: The code currently accepts JSON booleans because Python
treats True/False as 1/0; change the validation so ci_exit_code is required to
be an int and then check its value against {0,1} (e.g., ensure
isinstance(ci_exit_code, int) and ci_exit_code in {0,1}); if the check fails,
fall back to the existing logic that sets ci_exit_code = 1 if verdict == "FAIL"
else 0. Reference the variables ci_exit_code, data.get("ci_exit_code") and
verdict when making this type-and-value validation change.
In `@tests/cli-contracts/specfact-code-review-run.scenarios.yaml`:
- Around line 138-149: The scenario name level-error-json-clean-module is
misleading because the argv points to tests/fixtures/review/dirty_module.py and
expects a failing exit_code; rename the scenario to something semantically
accurate (e.g., level-error-json-dirty-module) by updating the YAML key for the
scenario name while keeping the rest of the block (type, argv, expect) unchanged
so the test reflects it is testing a dirty module failure.
In `@tests/unit/workflows/test_pr_orchestrator_signing.py`:
- Around line 38-45: The test shows pr-orchestrator now unconditionally appends
"--require-signature" for PRs targeting main (look for main_pr_guard /
main_ref_guard and the require_append symbol), which conflicts with
sign-modules-on-approval.yml's gate that only signs commits for same-repo PRs
(the head.repo.full_name == github.repository check), making fork PRs impossible
to satisfy; fix by either removing the unconditional require_append in
pr-orchestrator (revert the --require-signature change), or update
sign-modules-on-approval.yml to allow signing approved fork PRs (relax or add a
path around the head.repo.full_name == github.repository guard), or add
documentation/explicit workflow logic that main no longer accepts fork
contributions so the tests and workflows remain consistent.
---
Outside diff comments:
In
`@openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md`:
- Around line 3-107: Summary: The CLI flag wiring for the new code-review flags
is missing from specfact-cli; either link the paired PR or create one and update
the proposal. Action: Verify whether a specfact-cli PR exists that adds typer
argument parsing for the "specfact code review run" command and routes the flags
--bug-hunt, --mode, repeated --focus, and --level into the module's
ReviewRunRequest fields; if it exists, add the PR link to this proposal/spec; if
it does not exist, open an issue/PR in specfact-cli that implements mapping
those flags into the code review run handler so they are passed to
specfact-code-review (ensure compatibility with --json/--out and existing
flags), then update the spec to reference the new issue/PR for cross-repo
traceability.
🪄 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: c6bf785b-41b0-48d9-a179-8b7a2cf6b8cf
📒 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:
packages/specfact-code-review/src/specfact_code_review/_review_utils.pypackages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pytests/unit/specfact_code_review/tools/test_basedpyright_runner.pytests/unit/workflows/test_sign_modules_on_approval.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/scripts/test_pre_commit_code_review.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pytests/unit/specfact_code_review/tools/test_contract_runner.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pytests/unit/test_git_branch_module_signature_flag_script.pyscripts/pre_commit_code_review.pytests/unit/specfact_code_review/tools/test_semgrep_runner.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/base.pytests/unit/specfact_code_review/tools/test_radon_runner.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pypackages/specfact-code-review/src/specfact_code_review/run/commands.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/radon_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/review/commands.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/base.pypackages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.pypackages/specfact-code-review/src/specfact_code_review/run/commands.py
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.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/contract-runner/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-cli-contracts/spec.md
.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
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/workflows/test_sign_modules_on_approval.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_code_review/tools/test_contract_runner.pytests/unit/specfact_codebase/test_sidecar_framework_extractors.pytests/unit/test_git_branch_module_signature_flag_script.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/specfact_code_review/tools/test_radon_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-codebase/module-package.yamlpackages/specfact-code-review/module-package.yaml
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/pre_commit_code_review.py
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
🧠 Learnings (13)
📓 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: 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: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
📚 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/tasks.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md.github/workflows/sign-modules-on-approval.ymlREADME.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mddocs/modules/code-review.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.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:
.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.ymltests/unit/workflows/test_sign_modules_on_approval.pypackages/specfact-codebase/module-package.yamldocs/reference/module-security.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:
.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.ymlpackages/specfact-codebase/module-package.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdpackages/specfact-code-review/module-package.yamldocs/reference/module-security.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:
openspec/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/contract-runner/spec.mdREADME.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdpackages/specfact-code-review/module-package.yamldocs/modules/code-review.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.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: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.mdREADME.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.mdpackages/specfact-code-review/.semgrep/bugs.yamlpackages/specfact-code-review/src/specfact_code_review/review/commands.pydocs/modules/code-review.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamlopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.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: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:
README.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-cli-contracts/spec.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:
README.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-cli-contracts/spec.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Run the required verification and quality gates for the touched scope before finalization
Applied to files:
README.md
📚 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.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Applies to **/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h} : Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.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-code-review/module-package.yamldocs/reference/module-security.md
🪛 LanguageTool
README.md
[style] ~62-~62: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...d by SpecFact still require remediation prior to merge unless a documented, approved exc...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
docs/reference/module-security.md
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ...nforcement. - Dev-target PR mode: .github/workflows/pr-orchestrator.yml appends ...
(GITHUB)
🔀 Multi-repo context nold-ai/specfact-cli
[::nold-ai/specfact-cli::] Findings:
-
.github/workflows/pr-orchestrator.yml defines the verify-module-signatures job (job id: verify-module-signatures). It runs scripts/verify-modules-signature.py in the pull_request path and uses actions/checkout fetch-depth: 0. The job is referenced by many jobs via needs (e.g., Tests) so it gates downstream CI when code_changed == 'true'. (file: .github/workflows/pr-orchestrator.yml)
-
.github/workflows/sign-modules-on-approval.yml exists and defines a sign-on-approval job gated by an if that requires:
github.event_name == 'pull_request_review' &&
github.event.review.state == 'approved' &&
(base.ref == 'dev' || base.ref == 'main') &&
github.event.pull_request.head.repo.full_name == github.repository
— i.e., current workflow only allows same-repo approved reviews to trigger signing. (file: .github/workflows/sign-modules-on-approval.yml) -
scripts/git-branch-module-signature-flag.sh is referenced in multiple docs/tests and used by scripts/pre-commit-verify-modules.sh and docs; tests exist for its behavior (e.g., tests/unit/scripts/test_pre_commit_verify_modules.py and tests/unit/test_git_branch_module_signature_flag_script.py referenced). (multiple locations: scripts/, docs/, tests/)
-
Many spec/openspec and docs files reference the branch-aware policy for verify-module-signatures and sign-modules-on-approval (openspec/changes/marketplace-06-ci-module-signing/*, docs/reference/module-security.md, docs/guides/module-signing-and-key-rotation.md). These are consumers/expectations of the workflows' branch/approval semantics. (files under openspec/ and docs/)
-
Tests that validate workflow text exist and were updated in repo (e.g., tests/unit/workflows/test_pr_orchestrator_signing.py and tests/unit/workflows/test_sign_modules_on_approval.py). These tests assert specific content/guards in the workflow YAMLs, so changes to gating logic must keep tests aligned. (files: tests/unit/workflows/test_pr_orchestrator_signing.py, tests/unit/workflows/test_sign_modules_on_approval.py)
Conclusion: The repo contains multiple consumers and tests tied to the PR's workflow/signing semantics (verify-module-signatures, sign-modules-on-approval, git-branch-module-signature-flag.sh, pre-commit verify script, and many openspec/doc artifacts). Changes to the workflows or the branch/author-association gating will directly affect these tests, scripts, and documentation.
🔇 Additional comments (43)
tests/unit/specfact_code_review/tools/test_basedpyright_runner.py (2)
14-16: Good contract assertion for empty-input adapter path.This explicit
== []check makes the no-file boundary behavior deterministic and aligned with the tool runner contract.
18-26: Nice strengthening of manifest-skip behavior verification.Asserting
run_basedpyright([manifest]) == []alongsideassert_not_called()clearly locks the boundary: YAML manifests are filtered out and never reach the subprocess adapter.openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/sidecar-route-extraction/spec.md (1)
1-1: Spec heading normalization looks correct.Top-level title is clear and consistent with the sidecar extraction delta, with no contract drift introduced.
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.md (1)
1-1: Task heading promotion is fine.This is a structural docs-only change and stays aligned with the implemented task checklist.
packages/specfact-code-review/src/specfact_code_review/_review_utils.py (1)
42-42: LGTM – Docstring improvement.The updated docstring clearly and accurately describes what the function returns: Python source and type stub paths for linters and type checkers. This is a documentation quality improvement with no logic changes.
packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/base.py (1)
51-58: LGTM! Efficient directory pruning withos.walk.The switch from
rglobtoos.walkwith in-placedirnames[:]pruning is a solid improvement—it prevents descending into excluded subtrees entirely rather than filtering post-traversal. This properly supports subclass overrides of_path_touches_excluded_dir(as FastAPIExtractor does).packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py (1)
231-235: Ownership check handles common Flask patterns correctly.The implementation correctly filters
@app.routeand@bp.routedecorators where the base is a simpleast.Name. Edge cases likeself.app.routeormodule.app.route(wherefunc.valueisast.Attribute) will returnFalseand be excluded—acceptable for typical Flask app structures.packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/fastapi.py (2)
40-44: Clean extension of base exclusion logic.The override correctly extends the parent's exclusion set via
super()._path_touches_excluded_dir(path), then adds FastAPI-specific cache directories. This layered approach keeps the base class generic while allowing framework-specific customization.
201-229: Multi-route extraction correctly handles stacked decorators andapi_routemethods.The refactored
_extract_routes_from_functionproperly collects all(method, path)tuples from both:
- Individual HTTP method decorators (
@router.get,@router.post)- Multi-method
@router.api_route(path, methods=[...])decoratorsEach tuple yields a separate
RouteInfo, matching OpenAPI's expectation of one operation per method+path combination.tests/unit/specfact_codebase/test_sidecar_framework_extractors.py (3)
37-38: Assertion correctly validates multi-method extraction.The updated test properly collects all methods for the
/multipath into a set, validating thatapi_route(methods=["GET", "POST"])yields both HTTP verbs.
41-56: Good coverage for stacked decorator extraction.This test validates that when a function has multiple route decorators (
@router.get("/read")and@router.post("/write")), both routes are correctly extracted as separateRouteInfoentries with their respective method and path.
105-129: Well-designed test for Flask ownership filtering.The test cleverly uses an undefined
othersymbol in the test code string—this would raiseNameErrorat runtime but is valid for AST parsing. Sinceotheris never registered viaFlask()orBlueprint(),_is_owned_route_decoratorcorrectly excludes@other.route("/ignored")from extraction results. This validates the filtering logic without requiring actual Flask runtime.openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-run-command/spec.md (1)
1-2: Spec requirements are fully implemented, tested, and documented—no action needed.The heading improves document clarity. All spec requirements (lines 3–107:
--bug-hunt,--mode shadow/enforce,--focus,--level) are already implemented in the code-review module, comprehensively tested intests/cli-contracts/specfact-code-review-run.scenarios.yaml, and documented indocs/modules/code-review.md. The module version bump is confirmed (0.47.0 baseline recorded inTDD_EVIDENCE.md; currentmodule-package.yamlversion 0.47.3). Per the agent rules, this change includes the requiredTDD_EVIDENCE.mdwith passing tests and verified CI gates.packages/specfact-codebase/module-package.yaml (1)
2-2: LGTM! Version bump and checksum update align with CI signing workflow.The patch version increment (0.41.7 → 0.41.8) and updated SHA-256 checksum are consistent with the payload changes. The absence of a local signature is expected—per the established workflow,
publish-modulesgenerates signatures during the publish phase rather than at commit time. Based on learnings: "module tarball signatures (registry/signatures/*.tar.sig) are generated by thepublish-modulesGitHub Actions runner during the publish workflow, not committed locally to the branch."Also applies to: 26-27
packages/specfact-code-review/module-package.yaml (1)
2-2: LGTM! Manifest updated for code-review bug-finding enhancements.Patch bump (0.47.2 → 0.47.3) and checksum update reflect the semgrep runner refactoring and
bugs.yamladditions in this bundle. The signature will be applied by CI upon trusted approval, consistent with the deferred signing workflow.Also applies to: 25-26
packages/specfact-code-review/.semgrep/bugs.yaml (1)
36-38: LGTM! Multiline regex fix correctly handlesyaml.load()calls spanning multiple lines.The updated
pattern-not-regexwith(?s)dotall flag and[\s\S]*?properly excludesyaml.load()invocations whereLoader=appears anywhere in the call, even when arguments span lines. The[\s\S]*?is a belt-and-suspenders approach that works with or without dotall—explicit and safe.tests/unit/test_git_branch_module_signature_flag_script.py (1)
20-33: LGTM! Good subprocess-based validation of branch-conditional signature policy.These tests properly exercise the shell script's contract:
GITHUB_BASE_REF="main"→"require", other branches →"omit". The environment passthrough with{**os.environ, ...}ensures the script runs in a realistic context.packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py (3)
240-267: LGTM! Clean DRY refactor consolidating field extraction.The
_extract_common_finding_fieldshelper eliminates duplicated validation logic between the clean-code and bug-finding passes. The tuple return with earlyNoneexit for filtered paths is a clean pattern that simplifies both callers.
410-412: LGTM! Correct deduplication of tool-missing warnings.Returning
[]instead of the skip finding prevents duplicate "semgrep missing" warnings when both passes run. The first pass (run_semgrep) already emits the single advisory, per the docstring contract.
270-289: LGTM!The refactored
_finding_from_resultcleanly delegates to the shared extraction helper. The unused_extramaintains tuple unpacking symmetry with_finding_from_bug_result, which does useextrafor severity lookup.openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/code-review-bug-finding/spec.md (1)
14-16: LGTM! Spec aligns withrun_semgrep_bugsimplementation.The updated scenario correctly references
run_semgrep_bugsand thesecurity/clean_codecategories, which match theBUG_RULE_CATEGORYmapping insemgrep_runner.py. The wiring requirement (line 30) is satisfied by_tool_stepsinrunner.py.tests/unit/specfact_code_review/tools/test_semgrep_runner.py (2)
38-47: LGTM! Well-designed autouse fixture for tool availability stubbing.The fixture correctly delegates non-semgrep lookups to the real
shutil.which, ensuring other tool checks behave normally while semgrep appears available. Patchingspecfact_code_review.tools.tool_availability.shutil.whichis the right target since that's where the actual lookup occurs.
133-145: LGTM! Test updated to matchskip_if_tool_missingcontract.The severity assertion change to
"warning"aligns withtool_availability.py'sskip_if_tool_missinghelper. The addedrun_mock.assert_not_called()is a valuable guard ensuring the subprocess is never invoked when the tool is absent.packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py (3)
25-35: LGTM! Smart scan-root derivation for batch-level icontract detection.The
_icontract_usage_scan_rootsfunction correctly identifies package directories for files underpackages/by slicing atpackage_index + 2(e.g.,packages/specfact-code-review), falling back tofile.parentfor other layouts. Thedict.fromkeysdeduplication is a nice touch for preserving insertion order.
38-69: LGTM! Robust candidate iteration and AST-based import detection.The exclusion of
.git,__pycache__,venv, etc. prevents scanning noise. The_file_imports_icontracthelper correctly detects bothimport icontractandfrom icontract import ...forms, with graceful fallback on parse/read failures—important since candidate files may include malformed syntax.
48-54: LGTM! Batch-level icontract detection broadens coverage appropriately.The refactored
_has_icontract_usagenow scans the entire package root rather than just reviewed files. This correctly suppressesMISSING_ICONTRACTfindings when the project uses icontract—even if the specific files under review don't directly import it. The short-circuit return is efficient for large codebases.tests/unit/specfact_code_review/tools/test_contract_runner.py (2)
31-42: Good isolation for the “package unused” path.This test reduces fixture coupling and directly validates the no-findings behavior plus the exact CrossHair command contract.
45-52: Nice coverage for batch-level icontract detection.The assertion on
MISSING_ICONTRACTdirectly protects the updated scan-root behavior.openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/contract-runner/spec.md (1)
7-27: Spec update is consistent with runtime behavior.The requirement/scenario wording now matches batch-level scan-root detection and suppression behavior implemented in the contract runner.
docs/modules/code-review.md (2)
280-293: Contract-runner docs are aligned with implementation.The
bug_huntsignature and batch-level icontract scan semantics are documented correctly here.
432-447: Representative usage snippet is clear and useful.The example demonstrates meaningful orchestration flags (
bug_hunt,review_level,review_mode) in a realistic call pattern..github/workflows/pr-orchestrator.yml (1)
95-97: Main-branch signature enforcement is correctly gated.This keeps
devmetadata-only while enforcing strict signature verification formaintargets, matching the intended promotion policy.packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py (1)
168-201: Exemption narrowing is a solid correctness improvement.Requiring both
ctxcontext annotation and acommanddecorator prevents over-broad suppression ofkiss.parameter-count.*findings.tests/unit/workflows/test_sign_modules_on_approval.py (1)
71-74: Good test hardening for reviewer-trust gating.These assertions correctly lock the workflow contract for trusted reviewer associations.
README.md (1)
62-62: Policy clarification is clear and consistent.The distinction between commit-blocking errors and still-mandatory warning remediation is now explicit and useful.
.github/workflows/sign-modules-on-approval.yml (1)
33-42: Trusted-reviewer gate is a good security hardening step.Early exit on non-trusted
author_associationvalues is explicit and keeps signing authority scoped to collaborators/members/owners.Also applies to: 57-57
tests/unit/scripts/test_pre_commit_code_review.py (1)
192-209: Good regression coverage for report-driven exit handling.This pins the adapter boundary that matters here: the pre-commit hook exits from
review-report.json, not the nested CLI process status.As per coding guidelines:
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness. Ensure changes to adapters or bridges have targeted coverage.tests/unit/specfact_code_review/tools/test_radon_runner.py (1)
112-152: Good two-sided coverage for the Typer-context exemption.These tests pin both branches of the AST predicate:
typer.Contextalone still counts toward KISS, buttyper.Contextplus a Typercommanddecorator does not.As per coding guidelines:
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness. Ensure changes to adapters or bridges have targeted coverage.packages/specfact-code-review/src/specfact_code_review/run/commands.py (1)
29-53: TypedRunCommandErrorvariants are a solid contract improvement.This makes downstream adapters and tests much less dependent on brittle message-text matching and gives the review surface a clearer public error model.
As per coding guidelines:
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.Also applies to: 635-644
tests/cli-contracts/specfact-code-review-run.scenarios.yaml (4)
69-80: Solid shadow-mode bug-hunt contract coverage.This scenario correctly locks the adapter contract that
--bug-hunt+--mode shadowremains non-blocking (exit_code: 0) while still emitting report output markers.
92-103: Good enforce-mode bug-hunt failure contract.This adds the needed counterpart to shadow mode and protects the core CLI boundary by asserting blocking behavior in enforce mode (
exit_code: 1).
111-122: Nice improvement: path-level assertions are stronger thanrun_idchecks.Asserting reviewed paths directly makes this contract more deterministic and better aligned with focus/path routing behavior.
129-137: Test-focus scenario update is well scoped.Switching to the test subtree and asserting that subtree in stdout makes this scenario precise and reduces false-positive passes.
...spec/changes/code-review-bug-finding-and-sidecar-venv-fix/specs/review-cli-contracts/spec.md
Show resolved
Hide resolved
packages/specfact-code-review/src/specfact_code_review/review/commands.py
Show resolved
Hide resolved
packages/specfact-codebase/src/specfact_codebase/validators/sidecar/frameworks/flask.py
Show resolved
Hide resolved
…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
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/contract_runner.py`:
- Around line 25-35: The current detection uses any "packages" segment in the
absolute path; change _icontract_usage_scan_roots to resolve each file_path
relative to the repository root (repo_root) before searching for the "packages"
segment: for each file, compute a relative path w.r.t. repo_root (fallback to
file_path if not relative), use rel.parts to find "packages" and when found
construct the bundle root as repo_root / "packages" /
rel.parts[package_index+1]; otherwise fall back to file_path.parent; ensure you
preserve deduplication via dict.fromkeys; apply the same fix to the other
occurrence of the same logic (the code around the other packages-check at the
other location).
- Around line 38-45: _iter_icontract_usage_candidates currently only globs
"*.py" so .pyi stubs are ignored; update it to include both .py and .pyi files
(e.g., iterate rglob over both patterns or collect files whose suffix is in
{".py", ".pyi"}) while preserving the existing filters (path.is_file() and
exclusion via _ICONTRACT_SCAN_EXCLUDED_DIRS) so run_contract_check and
_has_icontract_usage can detect icontract usage in stub files too.
In `@tests/unit/specfact_code_review/tools/test_contract_runner.py`:
- Around line 45-52: The test
test_run_contract_check_uses_batch_level_icontract_detection should be made
self-contained by creating the anchor file in the test's tmp_path instead of
relying on FIXTURES_DIR; create a temporary Python file in tmp_path (e.g.,
tmp_file = tmp_path / "public_without_contracts.py") with the minimal contents
needed to trigger contract checking, monkeypatch subprocess.run to return
completed_process("crosshair", stdout="") as before, and call
run_contract_check([tmp_file]) so the assertion on MISSING_ICONTRACT only
depends on the file you create; update references from FIXTURES_DIR to the
tmp_path file and keep using run_contract_check, subprocess.run, and
completed_process as the same symbols.
🪄 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: d9f954b8-b78b-4e5e-b848-3ab6353c09fc
📒 Files selected for processing (7)
.github/workflows/pr-orchestrator.ymldocs/reference/module-security.mdpackages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pyscripts/pre_commit_code_review.pytests/unit/specfact_code_review/tools/test_contract_runner.pytests/unit/workflows/test_pr_orchestrator_signing.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.11)
- GitHub Check: quality (3.13)
🧰 Additional context used
📓 Path-based instructions (7)
.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
**/*.{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/workflows/test_pr_orchestrator_signing.pyscripts/pre_commit_code_review.pytests/unit/specfact_code_review/tools/test_contract_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/contract_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/workflows/test_pr_orchestrator_signing.pytests/unit/specfact_code_review/tools/test_contract_runner.py
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/pre_commit_code_review.py
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.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/contract_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.yaml
🧠 Learnings (9)
📓 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: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .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: .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/`
📚 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:
.github/workflows/pr-orchestrator.ymltests/unit/workflows/test_pr_orchestrator_signing.pydocs/reference/module-security.mdpackages/specfact-code-review/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: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
.github/workflows/pr-orchestrator.ymldocs/reference/module-security.mdpackages/specfact-code-review/module-package.yaml
📚 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:
tests/unit/workflows/test_pr_orchestrator_signing.py
📚 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:
tests/unit/workflows/test_pr_orchestrator_signing.py
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
scripts/pre_commit_code_review.pypackages/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: Enforce module signatures and version bumps when signed module assets or manifests are affected
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 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: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
🪛 LanguageTool
docs/reference/module-security.md
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ...nforcement. - Dev-target PR mode: .github/workflows/pr-orchestrator.yml appends ...
(GITHUB)
🔀 Multi-repo context nold-ai/specfact-cli
[::nold-ai/specfact-cli::] .github/workflows/pr-orchestrator.yml
- verify-module-signatures job now runs verifier without --require-signature for pull_request path by default and sets VERIFY_ARGS=(--payload-from-filesystem --enforce-version-bump). The job contains the pull_request/main/dev guards discussed in PR summary and no longer appends --require-signature unconditionally for PRs. (file content printed)
[::nold-ai/specfact-cli::] .github/workflows/sign-modules-on-approval.yml
- sign-on-approval job gating logic requires same-repo PRs and approved reviews:
github.event_name == 'pull_request_review' &&
github.event.review.state == 'approved' &&
(base ref == dev || main) &&
github.event.pull_request.head.repo.full_name == github.repository
(file content printed). This matches the PR change to require same-repo trusted approvals for signing.
[::nold-ai/specfact-cli::] scripts/git-branch-module-signature-flag.sh
- Emits "require" when branch == main, "omit" otherwise. This script is referenced by pre-commit hooks and docs as the branch-aware source of whether to pass --require-signature. (file content printed)
[::nold-ai/specfact-cli::] scripts/pre-commit-verify-modules.sh / scripts/verify-modules-signature.py / scripts/pre-commit-verify-modules.sh references
- Multiple files/scripts reference or call verify-modules-signature.py with --require-signature; pre-commit and docs rely on git-branch-module-signature-flag.sh to decide when to include the flag. (search hits in repo)
[::nold-ai/specfact-cli::] tests/unit/*
- Tests reference the require/omit behavior:
- tests/unit/scripts/test_pre_commit_verify_modules.py uses TOKEN_REQUIRE_SIGNATURE = "--require-signature".
- tests/unit/test_git_branch_module_signature_flag_script.py (added tests in PR summary) validate the script emits "require" for main and "omit" otherwise (tests exist/updated).
- tests/unit/workflows/test_pr_orchestrator_signing.py and tests/unit/workflows/test_sign_modules_on_approval.py inspect workflow YAML contents and assert presence/absence of HEAD_REPO/THIS_REPO and require-signature guards. These tests are consumers that must align with the workflow changes. (search hits)
[::nold-ai/specfact-cli::] docs & openspec
- Many docs and openspec files reference the branch-aware --require-signature policy and the approval-time signing workflow (numerous matches in docs/, openspec/ and CHANGELOG). These are consumers of the workflow/signing semantics and must remain consistent with the workflow changes. (search hits)
Summary assessment
- The repo contains multiple direct consumers (workflows, scripts, tests, docs) of the changed signing/--require-signature logic. The verify-module-signatures workflow, the sign-on-approval workflow, git-branch-module-signature-flag.sh, pre-commit hooks, and unit tests are interdependent and must be updated together (tests already appear adjusted in the PR). No other repositories were available to search.
🔇 Additional comments (7)
scripts/pre_commit_code_review.py (3)
242-244: Treat JSON booleans as invalidci_exit_codevalues.The validation
ci_exit_code not in {0, 1}accepts Python booleans becauseTrue == 1andFalse == 0. While the upstreamReviewReportusesLiteral[0, 1](which serializes to JSON integers), adding explicit type checking defends against schema drift from malformed or hand-edited reports.🛡️ Minimal fix for type-safe validation
ci_exit_code = data.get("ci_exit_code") - if ci_exit_code not in {0, 1}: + if not isinstance(ci_exit_code, int) or isinstance(ci_exit_code, bool) or ci_exit_code not in {0, 1}: ci_exit_code = 1 if verdict == "FAIL" else 0
211-216: LGTM! Clean 3-tuple return with clear semantics.The refactored signature separates the success flag, informational error count, and the actionable
ci_exit_code. The docstring correctly notes thaterror_countis informational-only since the report'sci_exit_codealready accounts for fixable findings. This aligns with theReviewReport.finalize()logic infindings.pywhere blocking errors and score thresholds determine the exit code.
325-328: LGTM! Exit handling correctly uses the report's governedci_exit_code.The unpacking handles all error paths (
ci_exit_code is None), and the explicitint()cast ensures a clean integer return. This shift from subprocessreturncodeto the report'sci_exit_codeis semantically equivalent—perfindings.py, the tool's exit code and the report field are set identically byfinalize(). Good alignment with the CI/CD signing workflows in the linkedspecfact-clirepo.packages/specfact-code-review/module-package.yaml (1)
2-26: Version bump and checksum refresh match the signing flow.This keeps the manifest’s semver and integrity metadata in step with the refreshed payload, and the missing local
integrity.signatureis consistent with CI-owned signing for this repo.Based on learnings, module tarball signatures are generated by the
publish-modulesGitHub Actions runner during the publish workflow, not committed locally to the branch.docs/reference/module-security.md (1)
49-55: Docs now match the live signing contract.The verifier matrix and CI-signing note are in parity with
.github/workflows/pr-orchestrator.ymland.github/workflows/sign-modules-on-approval.yml, so the module-security reference stays accurate at the module/core boundary.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..github/workflows/pr-orchestrator.yml (1)
92-97: Same-repo guard correctly restores the signing boundary.Limiting
--require-signatureto same-repo PRs targetingmainkeepspr-orchestratoraligned with the approval signer, which can only add signatures back onto same-repo branches.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.
tests/unit/workflows/test_pr_orchestrator_signing.py (1)
38-46: Good contract coverage for the main-path signing gate.These assertions pin the exact
HEAD_REPO/THIS_REPOcondition used by the workflow, which gives the signing boundary a focused regression test.As per coding guidelines,
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness. Ensure changes to adapters or bridges have targeted coverage.
packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py
Show resolved
Hide resolved
packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py
Outdated
Show resolved
Hide resolved
- 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
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/contract_runner.py`:
- Around line 65-71: The logic in contract_runner.py that treats any
repo-relative segment named "packages" as the bundle root is too permissive;
change the checks that currently look for "packages" in rel_parts (the block
around the code in the shown diff and the similar code at lines 123-124) to only
match when the repo-relative path starts with "packages" (i.e., rel_parts is
non-empty and rel_parts[0] == "packages"); then compute the package root using
that leading segment (use repo_resolved / "packages" / rel_parts[1] when
repo_resolved is set, otherwise Path(*rel_parts[:2])) so only paths beginning
with packages/ are treated as bundle roots.
In `@tests/unit/specfact_code_review/tools/test_contract_runner.py`:
- Around line 45-80: Add a new unit test mirroring
test_run_contract_check_detects_icontract_across_package_bundles but create the
anchor as a stub file (write "import icontract\n" to icontract_anchor.pyi in
pkg_b) and assert run_contract_check([edited]) still yields a finding with rule
"MISSING_ICONTRACT"; ensure you monkeypatch subprocess.run
(Mock(return_value=completed_process("crosshair", stdout=""))) and place the
test alongside the others so _iter_icontract_usage_candidates()'s .pyi branch is
exercised.
🪄 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: 72f11830-423c-4957-b386-f8f8c759484e
📒 Files selected for processing (3)
packages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/tools/contract_runner.pytests/unit/specfact_code_review/tools/test_contract_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.11)
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.12)
🧰 Additional context used
📓 Path-based instructions (4)
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.yaml
**/*.{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_contract_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/contract_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/tools/test_contract_runner.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators),
Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles.
Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior.
Files:
packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py
🧠 Learnings (5)
📓 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: 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: 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: .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: Require public GitHub metadata completeness before implementation when linked issue workflow applies: parent, labels, project assignment, blockers, and blocked-by relationships
📚 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.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.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/contract_runner.py
🔀 Multi-repo context nold-ai/specfact-cli
[::nold-ai/specfact-cli::] .github/workflows/pr-orchestrator.yml
- Contains verify-module-signatures job and is a primary consumer of the changed --require-signature logic referenced in the PR. (see workflow file path listed)
[::nold-ai/specfact-cli::] .github/workflows/sign-modules-on-approval.yml
- Workflow referenced by PR summary; tests/assertions updated to validate author association and same-repo signing gate. (file path listed)
[::nold-ai/specfact-cli::] scripts/git-branch-module-signature-flag.sh
- Branch-aware script that prints "require" on main and "omit" elsewhere; used by pre-commit and workflows to decide whether to pass --require-signature. (scripts/git-branch-module-signature-flag.sh:3)
[::nold-ai/specfact-cli::] scripts/pre-commit-verify-modules.sh
- Calls scripts/git-branch-module-signature-flag.sh and runs verify-modules-signature; directly affected by branch-aware --require-signature behavior. (scripts/pre-commit-verify-modules.sh:23)
[::nold-ai/specfact-cli::] scripts/verify-modules-signature.py
- Exposes --require-signature CLI flag; verification behavior relies on this flag. (scripts/verify-modules-signature.py:319)
[::nold-ai/specfact-cli::] scripts/pre-commit-quality-checks.sh
- Documentation/warning messaging about --require-signature on main; consumer of signing policy. (scripts/pre-commit-quality-checks.sh:217)
[::nold-ai/specfact-cli::] tests/unit/scripts/test_pre_commit_verify_modules.py
- Tests reference TOKEN_REQUIRE_SIGNATURE = "--require-signature" and the flag script; must stay aligned with workflow/script behavior. (tests/unit/scripts/test_pre_commit_verify_modules.py:16,100)
[::nold-ai/specfact-cli::] tests/unit/test_git_branch_module_signature_flag_script.py
- New unit tests added (in PR) exercise git-branch-module-signature-flag.sh behavior for main vs non-main branches; these are direct consumers/validators. (tests/unit/test_git_branch_module_signature_flag_script.py: new tests)
[::nold-ai/specfact-cli::] tests/unit/workflows/test_pr_orchestrator_signing.py
- Unit tests inspect pr-orchestrator.yml contents and were updated to assert HEAD_REPO/THIS_REPO variables and the modified main-path guard for --require-signature. (tests/unit/workflows/test_pr_orchestrator_signing.py: updated assertions)
[::nold-ai/specfact-cli::] tests/unit/workflows/test_sign_modules_on_approval.py
- Tests tightened to assert author_association allowlist presence in workflow gate; consumer of sign-on-approval changes. (tests/unit/workflows/test_sign_modules_on_approval.py: updated _assert_eligibility_gate_step)
[::nold-ai/specfact-cli::] docs/reference/module-security.md and multiple docs/openspec files
- Many documentation and openspec artifacts reference the branch-aware --require-signature policy and the approval-time signing workflow; these must remain consistent with the workflow/script changes. (docs/reference/module-security.md and numerous openspec/specs and docs paths listed)
Summary: The repository contains many direct consumers of the changed signing/--require-signature logic: workflows (.github/*), scripts (git-branch-module-signature-flag.sh, verify-modules-signature.py, pre-commit hooks), unit tests that validate workflow content and script behavior, and documentation/openspec artifacts. These will need to remain consistent with the inverted/updated HEAD_REPO/THIS_REPO checks, trusted-reviewer gating, and branch-aware --require-signature semantics introduced by the PR.
🔇 Additional comments (4)
packages/specfact-code-review/module-package.yaml (2)
2-2: Version bump is correctly aligned with manifest change.Patch bump to
0.47.5is the right semver direction for this metadata/checksum refresh and keeps module contract evolution explicit at the module-package boundary.As per coding guidelines,
packages/**/module-package.yamlchanges should call out semver implications when manifests or payloads change.
25-26: Integrity checksum refresh looks consistent with current signing flow.
integrity.checksumis present and updated; given the branch-aware verification/signing flow, this is consistent with unsigned local/dev manifests before CI approval-time signing.Based on learnings, in this repository signing artifacts are produced by GitHub Actions publish/signing workflows and should not be treated as a pre-merge blocker in PR branches.
tests/unit/specfact_code_review/tools/test_contract_runner.py (1)
31-35: Nice regression hardening.Moving this case to
tmp_pathmakes the "package unused" path self-contained and avoids fixture-coupled false positives.packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py (1)
81-93: Good fix for stub-aware discovery.Including
.pyifiles here closes the earlier blind spot for stub-only bundles and keeps icontract detection aligned with the accepted Python inputs.
packages/specfact-code-review/src/specfact_code_review/tools/contract_runner.py
Outdated
Show resolved
Hide resolved
…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
1 participant
Summary
mainand auto-signing trusted approvals ondev/main.Verification
hatch run pytest tests/unit/scripts/test_pre_commit_code_review.py -qhatch run contract-test(588 passed)openspec validate ... --strict; module verification without--require-signatureNotes
--require-signatureverification is intentionally left to GitHub CI/CD after trusted review approval, matching the desired flow fordevandmain.