fix: review follow-ups (focus, main signing, registry, contracts)#197
fix: review follow-ups (focus, main signing, registry, contracts)#197
Conversation
…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
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRemoves same-repo guards from PR signature verification; shifts non-main pre-commit/PR verification to full payload checksum + version-bump (no metadata-only); refactors specfact-code-review focus/test handling; adds registry↔manifest consistency checks and a registry-sync script; updates tests, schemas, workflows, docs, and package/registry artifacts. Changes
Sequence Diagram(s)(Skipped — changes are broad and mostly configuration, scripts, tests, and utilities without a single new multi-component runtime control-flow that benefits from a simple sequential diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df8135ae0b
ℹ️ 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: 6
🤖 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: Replace the bare command instance "specfact code review run" in the
related task file with the enforced pattern "hatch run specfact code review run
--json --out .specfact/code-review.json" so the documented command matches the
actual executed command shown in the evidence; locate the occurrence by
searching for the literal "specfact code review run" in the task markdown and
update that line to the full "hatch run ..." form.
In
`@packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py`:
- Around line 176-179: Add a brief inline comment above the path-based exemption
explaining the rationale and linking it to the specific function and constraint:
note that function_node.name == "run" (the Typer-driven CLI handler in
review.commands) intentionally has many injected parameters and cannot
reasonably be reduced, so we exempt
normalized.endswith("specfact_code_review/review/commands.py") to avoid spurious
complexity warnings; place this comment next to the existing normalized/path
check in radon_runner.py so future maintainers understand why the path-based
exemption is necessary and intentional.
In `@scripts/pre_commit_code_review.py`:
- Line 151: Replace the env.setdefault call so the script always forces
SPECFACT_CLI_MODULES_REPO to this checkout path: change the usage of
env.setdefault("SPECFACT_CLI_MODULES_REPO", str(repo_root.resolve())) to an
explicit assignment that overwrites any inherited value (i.e., set
env["SPECFACT_CLI_MODULES_REPO"] = str(repo_root.resolve())), ensuring
deterministic module resolution in the pre_commit_code_review script and
referencing the SPECFACT_CLI_MODULES_REPO key and repo_root.resolve() symbol to
locate the change.
In
`@tests/integration/specfact_code_review/test_cli_contract_review_run_reports.py`:
- Around line 37-44: The YAML is being parsed twice (once in
_scenario_names_with_file_expectations and again during each test invocation);
add a cached loader (e.g., a module-scoped fixture or a `@functools.cache`
function named _load_scenarios) that returns
yaml.safe_load(SCENARIO_PATH.read_text(...)) and replace direct calls to
yaml.safe_load(...) in _scenario_names_with_file_expectations and the per-test
loader (the function used at test invocation) with calls to _load_scenarios() so
the parsed scenarios are reused.
In `@tests/unit/test_validate_repo_manifests_bundle_deps.py`:
- Around line 48-84: Add two companion unit tests to
tests/unit/test_validate_repo_manifests_bundle_deps.py that exercise
validate_registry_consistency for the missing sidecar and manifest name/version
mismatch cases: create new test functions (e.g.,
test_validate_registry_consistency_missing_sidecar and
test_validate_registry_consistency_manifest_mismatch) that mirror the setup in
test_validate_registry_consistency_flags_bad_checksum but (1) omit creating the
modules/<tarball>.sha256 file and assert that
validate_registry_consistency(tmp_path, registry_path) returns an error
containing "missing checksum sidecar", and (2) write a module-package.yaml whose
name or version differs from the registry entry and assert the returned error
contains "does not match registry"; reuse the same registry creation pattern and
call validate_registry_consistency to locate behavior changes.
In `@tools/validate_repo_manifests.py`:
- Around line 40-43: The _sha256_from_sidecar function can raise IndexError for
empty/malformed sidecar contents; update _sha256_from_sidecar(text: str) to
validate the input before indexing: strip and splitlines(), ensure there's at
least one non-empty line, split that line and ensure there is at least one
token, then return the lower-cased token; if validation fails raise an OSError
(or other exception type already handled by _validate_registry_sidecar) with a
clear message so the caller's existing exception handling will handle malformed
sidecar files gracefully.
🪄 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: 7d816450-3984-448e-aceb-f39291e6115b
📒 Files selected for processing (16)
.github/workflows/pr-orchestrator.ymlREADME.mdopenspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mdpackages/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/radon_runner.pyresources/schemas/cli-contract.schema.jsonscripts/pre_commit_code_review.pytests/cli-contracts/specfact-code-review-run.scenarios.yamltests/integration/specfact_code_review/test_cli_contract_review_run_reports.pytests/unit/specfact_code_review/review/test_commands.pytests/unit/specfact_code_review/tools/test_radon_runner.pytests/unit/test_git_branch_module_signature_flag_script.pytests/unit/test_validate_repo_manifests_bundle_deps.pytests/unit/workflows/test_pr_orchestrator_signing.pytools/validate_repo_manifests.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
tests/unit/specfact_code_review/tools/test_radon_runner.pytests/unit/specfact_code_review/review/test_commands.pyscripts/pre_commit_code_review.pytests/unit/workflows/test_pr_orchestrator_signing.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pytests/unit/test_git_branch_module_signature_flag_script.pypackages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pytests/unit/test_validate_repo_manifests_bundle_deps.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pytools/validate_repo_manifests.pytests/integration/specfact_code_review/test_cli_contract_review_run_reports.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_radon_runner.pytests/unit/specfact_code_review/review/test_commands.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/test_git_branch_module_signature_flag_script.pytests/unit/test_validate_repo_manifests_bundle_deps.pytests/integration/specfact_code_review/test_cli_contract_review_run_reports.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/tasks.md
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/pre_commit_code_review.py
.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
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/run/commands.pypackages/specfact-code-review/src/specfact_code_review/tools/radon_runner.pypackages/specfact-code-review/src/specfact_code_review/review/commands.py
tools/**/*.py
⚙️ CodeRabbit configuration file
tools/**/*.py: Developer tooling aligned with pyproject Hatch scripts and CI expectations.
Files:
tools/validate_repo_manifests.py
🧠 Learnings (9)
📓 Common learnings
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: 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: 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: 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: 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
📚 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/tasks.mdtests/unit/specfact_code_review/review/test_commands.pyscripts/pre_commit_code_review.pyREADME.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamlpackages/specfact-code-review/src/specfact_code_review/review/commands.pytests/integration/specfact_code_review/test_cli_contract_review_run_reports.py
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mdREADME.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamlpackages/specfact-code-review/src/specfact_code_review/review/commands.py
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mdpackages/specfact-code-review/src/specfact_code_review/run/commands.pytests/cli-contracts/specfact-code-review-run.scenarios.yamlpackages/specfact-code-review/src/specfact_code_review/review/commands.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:
openspec/changes/code-review-bug-finding-and-sidecar-venv-fix/tasks.mdREADME.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/tasks.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.ymlREADME.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:
README.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:
README.mdtools/validate_repo_manifests.py
🔀 Multi-repo context nold-ai/specfact-cli
::nold-ai/specfact-cli::
-
.github/workflows/pr-orchestrator.yml — contains verify-module-signatures job and VERIFY_CMD construction; tests reference this workflow. [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml]
- Tests referencing the workflow: tests/unit/specfact_cli/registry/test_signing_artifacts.py (multiple assertions about verify-module-signatures and workflow content). [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_signing_artifacts.py:20,581,588,629-633]
- Tests referencing orchestrator: tests/unit/workflows/test_trustworthy_green_checks.py (loads .github/workflows/pr-orchestrator.yml and inspects verify-module-signatures job). [::nold-ai/specfact-cli::tests/unit/workflows/test_trustworthy_green_checks.py:14,161-170]
- Unit test adjusted in this PR: tests/unit/workflows/test_pr_orchestrator_signing.py updated to expect the loosened main-branch guard. [::nold-ai/specfact-cli::tests/unit/workflows/test_pr_orchestrator_signing.py]
-
scripts/git-branch-module-signature-flag.sh and pre-commit usage — branch-flag script is referenced by hooks and tests; new unit test added for behavior when GITHUB_BASE_REF unset. [::nold-ai/specfact-cli::scripts/git-branch-module-signature-flag.sh]
- Referenced by: scripts/pre-commit-verify-modules.sh (invokes flag_script). [::nold-ai/specfact-cli::scripts/pre-commit-verify-modules.sh:23]
- Tests referencing script: tests/unit/scripts/test_pre_commit_verify_modules.py and new tests/unit/test_git_branch_module_signature_flag_script.py. [::nold-ai/specfact-cli::tests/unit/scripts/test_pre_commit_verify_modules.py, tests/unit/test_git_branch_module_signature_flag_script.py]
-
Environment overrides for module discovery (SPECFACT_MODULES_REPO, SPECFACT_CLI_MODULES_REPO, PYTHONPATH) — multiple tools/scripts/tests rely on these env vars; the PR modifies scripts/pre_commit_code_review.py to set SPECFACT_MODULES_REPO/SPECFACT_CLI_MODULES_REPO and prepend packages/.../src to PYTHONPATH for subprocess runs. Consumers and checks:
- Many tests/tools reference/expect SPECFACT_MODULES_REPO and PYTHONPATH handling (examples: tests/unit/specfact_cli/validators/sidecar/test_crosshair_runner_env.py, scripts/verify-bundle-published.py, tools/command_package_runtime_validation.py). [::nold-ai/specfact-cli::tests/unit/specfact_cli/validators/sidecar/test_crosshair_runner_env.py, scripts/verify-bundle-published.py, tools/command_package_runtime_validation.py]
- scripts/pre_commit_code_review.py now constructs env passed to subprocess.run; ensure this does not unintentionally shadow user ~/.specfact/modules. [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py]
-
packages/specfact-code-review (review/commands.py, run/commands.py, tools/radon_runner.py) — CLI option/type changes and removal of FocusFacetConflictError; tests updated to exercise new tri-state options and focus behavior:
- packages/specfact-code-review/src/specfact_code_review/review/commands.py — include_tests/ exclude_tests changed to bool | None and focus handling updated. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/review/commands.py]
- packages/specfact-code-review/src/specfact_code_review/run/commands.py — FocusFacetConflictError removed; include_tests inference changed. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/run/commands.py]
- packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py — Typer exemption signature and behavior changed; tests adjusted. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py]
- Unit tests updated/added: tests/unit/specfact_code_review/review/test_commands.py and tests/unit/specfact_code_review/tools/test_radon_runner.py. [::nold-ai/specfact-cli::tests/unit/specfact_code_review/review/test_commands.py, tests/unit/specfact_code_review/tools/test_radon_runner.py]
-
CLI contract schema and tests — resources/schemas/cli-contract.schema.json adds scenarios[].expect.file_content_contains and integration test added to assert JSON report files:
- Schema: resources/schemas/cli-contract.schema.json (new optional file_content_contains). [::nold-ai/specfact-cli::resources/schemas/cli-contract.schema.json]
- Integration test: tests/integration/specfact_code_review/test_cli_contract_review_run_reports.py — loads scenarios YAML and asserts expected fragments in per-scenario output JSON files; scenarios updated at tests/cli-contracts/specfact-code-review-run.scenarios.yaml to use --out CONTRACT_TMP_REPORT.json and file_content_contains expectations. [::nold-ai/specfact-cli::tests/integration/specfact_code_review/test_cli_contract_review_run_reports.py, tests/cli-contracts/specfact-code-review-run.scenarios.yaml]
-
tools/validate_repo_manifests.py — added registry/index.json sidecar checksum and manifest-alignment validation. Tests referencing registry/index.json and bundle layout may be impacted:
- New function validate_registry_consistency and helpers added; tests added/updated: tests/unit/test_validate_repo_manifests_bundle_deps.py. [::nold-ai/specfact-cli::tools/validate_repo_manifests.py, tests/unit/test_validate_repo_manifests_bundle_deps.py]
- Scripts/tools that resolve registry index rely on SPECFACT_MODULES_REPO path resolution (see scripts/verify-bundle-published.py and related tests). [::nold-ai/specfact-cli::scripts/verify-bundle-published.py, tests/unit/scripts/test_verify_bundle_published.py:294-314]
-
Tests and documentation referencing pr-orchestrator/main signing policy and docs updated (README.md, docs/guides/module-signing-and-key-rotation.md). Ensure consumers/tests asserting prior HEAD_REPO==THIS_REPO behavior were updated (notably tests/unit/workflows/test_pr_orchestrator_signing.py). [::nold-ai/specfact-cli::README.md, docs/guides/module-signing-and-key-rotation.md, tests/unit/workflows/test_pr_orchestrator_signing.py]
High-level cross-repo impact summary (observed in this repository):
- Changing pr-orchestrator.yml to unconditionally append --require-signature for TARGET_BRANCH=main affects any tests or tooling that assumed signature enforcement only for same-repo PRs. Tests in tests/unit/... referencing workflow were updated accordingly. [::nold-ai/specfact-cli::tests/unit/workflows/test_pr_orchestrator_signing.py, tests/unit/specfact_cli/registry/test_signing_artifacts.py]
- The pre-commit subprocess env change (scripts/pre_commit_code_review.py) influences how nested invocations find packages (PYTHONPATH/SPECFACT_* envs). Multiple tests and scripts rely on these env vars; risk of local ~/.specfact/modules shadowing remains and is noted in PR description. [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py, tests/unit/specfact_cli/validators/sidecar/test_crosshair_runner_env.py]
- CLI API/behavior changes in specfact-code-review (tri-state include_tests, removal of FocusFacetConflictError, radon_runner exemptions) have direct test consumers within this repo's tests and CLI contract scenarios; confirm downstream callers of the library outside this repo (if any) accept the exception removal and CLI option type change. [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/review/commands.py, packages/specfact-code-review/src/specfact_code_review/run/commands.py]
If you want, I can:
- Open the specific files that tests reference to show exact call sites (line numbers) where the changed behaviors are exercised.
- Search for any external repositories (outside this repo) that import packages/specfact-code-review to find external consumers.
🔇 Additional comments (26)
scripts/pre_commit_code_review.py (2)
18-18:osimport is correct and now required.This import is necessary for
os.environ.copy()andos.pathsepusage in the new subprocess environment handling.
146-150: Good hardening of the module-discovery boundary for nested CLI runs.Pinning
SPECFACT_MODULES_REPO, prependingpackages/specfact-code-review/srctoPYTHONPATH, and passingenvintosubprocess.runimproves consistency of the pre-commit clean-code gate execution.Also applies to: 152-157, 164-164
tests/unit/test_git_branch_module_signature_flag_script.py (1)
28-44: Good branch-fallback coverage for unsetGITHUB_BASE_REF.This test isolates git state in a temp repo, forces a non-
mainbranch, and validates the expectedomit+ exit0fallback contract..github/workflows/pr-orchestrator.yml (1)
95-96: Main-branch PR signature enforcement is now unambiguous.The
mainguard cleanly appends--require-signaturewhile preserving thedevmetadata-only branch behavior.README.md (1)
51-51: Docs and workflow policy are aligned.This update clearly documents
dev(--metadata-only) vsmain(--require-signature) behavior and clarifies fork/approval-time signing expectations.tests/unit/workflows/test_pr_orchestrator_signing.py (1)
38-38: Assertion update correctly follows the workflow guard change.Expecting
elif [ "$TARGET_BRANCH" = "main" ]; thenkeeps the test aligned with the new orchestrator branching logic.tools/validate_repo_manifests.py (7)
45-53: LGTM!Solid defensive field extraction with
str(mod.get("id") or "").strip()pattern ensuring graceful handling of missing orNonevalues. The union return type clearly distinguishes success from failure.
55-65: LGTM!Clear validation of the
download_urlcontract against themodules/<slug>-<version>.tar.gzformat. The slug extraction viarsplit("/", maxsplit=1)[-1]correctly handles both namespaced and bare module IDs.
67-78: LGTM!Good registry adapter boundary validation—ensures the sidecar checksum files align with the
checksum_sha256declared inregistry/index.json. Error messages include both expected and actual values for easy debugging.
80-106: LGTM!Excellent cross-boundary validation ensuring
packages/<slug>/module-package.yamlstays aligned with the registry entry. This catches version/name drift between the package manifest andregistry/index.json—key for maintaining bundle integrity during module publishing.
108-125: LGTM!Clean orchestration with early-return pattern. Each validation step short-circuits on failure, avoiding wasted checks and providing focused error messages per module.
127-147: LGTM!Solid public API for registry consistency validation. The function correctly handles JSON loading errors and gracefully iterates through module entries. The guard at Line 137 is safe given
_validate_registryalready enforces themoduleslist structure before this function runs.
189-191: LGTM!Correct sequencing—registry structure validation via
_validate_registrymust pass before running the deeper consistency checks. This prevents confusing cascading errors from malformed registry JSON.packages/specfact-code-review/src/specfact_code_review/review/commands.py (2)
72-72: Focus-driven test inclusion now correctly derived from facet membership.The change from always setting
include_tests=Truewhen focus is provided toresolved_include_tests = "tests" in focus_listaligns with the widened file resolution strategy inrun/commands.py(line 602). When--focus sourceis used:
include_testsis correctly set toFalsehererun_commandstill widens resolution viainclude_for_resolve = request.include_tests or bool(request.focus_facets)_filter_files_by_focusthen narrows to only source filesThis preserves the intended behavior while giving callers accurate
include_testssemantics.
94-95: Tri-state CLI options enable proper inference semantics.Changing from
booltobool | Nonewithtyper.Option(None, ...)allows the CLI layer to distinguish between "user explicitly passed the flag" vs "user did not specify." The_resolve_review_run_flagshelper correctly resolves these to concrete booleans before invokingrun_command.packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py (2)
207-208: Callback decorator detection extends exemption to Typer app callbacks.Typer
@app.callback()decorators also receive injected CLI parameters. Including them in the exemption alongside@app.command()is consistent with the intent of skipping KISS parameter-count findings for Typer entry points.
180-189: Simplified gating logic improves readability.The restructured flow—early return for path match, then gate on decorator presence, then check annotation—is cleaner than combining all conditions. The final
rendered.endswith("Context")check is sufficient after the decorator gate.tests/unit/specfact_code_review/tools/test_radon_runner.py (1)
20-20: Explicit empty-list assertion is more precise.Changing from
assert not run_radon([manifest])toassert run_radon([manifest]) == []explicitly documents that the function returns an empty list (not just any falsy value). This aligns with therun_radoncontract returninglist[ReviewFinding].packages/specfact-code-review/src/specfact_code_review/run/commands.py (2)
602-602: Widened file resolution for focus facets is correct.The change to
include_for_resolve = request.include_tests or bool(request.focus_facets)ensures that when any focus facet is specified, all relevant file categories are initially resolved. The subsequent_filter_files_by_focuscall then narrows the set based on the specific facets. This two-step approach (widen, then filter) correctly handles mixed-facet scenarios like--focus source --focus docs.
629-637:FocusFacetConflictErrorremoval breaks the public API.The exception is no longer exported in
__all__and no references remain in either repository, confirming the removal was intentional and coordinated. However, this is a breaking change: any external code or downstream consumers not in these repos that imports or catchesFocusFacetConflictErrorwill fail withImportError.If this module is part of a published bundle or used by external tooling, document the deprecation path or coordinate with consumers before releasing.
tests/unit/specfact_code_review/review/test_commands.py (1)
47-62: Test coverage for focus-driveninclude_testsresolution.This test validates the key behavioral change:
--focus sourceshould result ininclude_tests=Falsebeing passed torun_command. The stub captures kwargs correctly, and assertions verify both successful exit and the expected flag value.Note: The test passes a fixture path (
tests/fixtures/review/clean_module.py) as a positional argument. Ensure this fixture exists to avoid test failures unrelated to the feature under test.resources/schemas/cli-contract.schema.json (1)
58-65: Schema extension for file content assertions is well-structured.The
file_content_containsfield complements existingstdout_contains/stderr_containspatterns, enabling contract scenarios to validate JSON report file contents. The description clarifies that enforcement happens in integration tests, not schema validation. This is the right boundary between schema (structure) and test (behavior).tests/cli-contracts/specfact-code-review-run.scenarios.yaml (2)
109-126:--mode shadowensures stable exit codes for focus scenarios.Adding
--mode shadowto focus scenarios stabilizes exit codes (always 0) regardless of findings, making the contract assertions more reliable. This aligns with the PR objective of "shadow-mode behavior for focus scenarios to keep stable exit codes."The
file_content_containsassertions correctly validate that the JSON report includes paths from the expected facet unions.
146-159: Level-error scenario validates severity filtering in JSON reports.This scenario tests that
--level errorproduces a report containing error-severity findings. Theexit_code: 1confirms enforce mode with actual errors, andfile_content_containswith'"severity":"error"'validates the filtering behavior. Good contract coverage for the level filter feature.tests/integration/specfact_code_review/test_cli_contract_review_run_reports.py (2)
47-72: Integration test structure is well-designed.Key strengths:
@pytest.mark.integrationfor proper test categorizationmonkeypatch.chdir(REPO_ROOT)ensures scenario paths resolve correctly- Line 64 pre-validates the placeholder exists in argv (fail-fast on malformed scenarios)
- Assertion messages include context (
result.output,scenario_name) for debuggingThis test provides solid contract coverage for JSON report file content assertions.
31-34: Tool availability check prevents false failures.Skipping when required review tools are missing is the right approach for integration tests. This keeps CI green in environments without full tooling while ensuring real failures are meaningful.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 51: The README paragraph conflates PR-orchestrator verification with
post-merge push verification: update the "Module signatures" paragraph to split
PR-time behavior (what pr-orchestrator runs: verify-modules-signature with
--payload-from-filesystem --enforce-version-bump and conditional
--version-check-base/--require-signature for PR bases) from post-merge/push
behavior (sign-modules.yml hardened verification on both dev and main that
requires signatures on push); explicitly state that pushes to both dev and main
enforce --require-signature for sign-modules, keep the existing notes about
sign-modules, sign-modules-on-approval (scripts/sign-modules.py) and the
pre-commit hook (scripts/pre-commit-verify-modules-signature.sh) unchanged aside
from clarifying their conditional --require-signature behavior.
In `@scripts/sync_registry_from_package.py`:
- Around line 139-149: The helper _maybe_write_tarball_signature currently
creates registry signature files from manifest data; change it to not synthesize
or write any files. Update the function (and any callers if needed) so that
_maybe_write_tarball_signature(manifest, signatures_dir, bundle_name, version)
either becomes a no-op or only inspects/validates manifest["integrity"] without
calling sig_path.write_text; remove the code path that constructs sig_path and
writes the "{bundle_name}-{version}.tar.sig" file so branch-local signature
artifacts are not emitted.
- Around line 81-94: _in _write_bundle_tarball_, make the produced .tar.gz
deterministic by normalizing tar metadata and gzip timestamp: when adding files,
pass a tarfile filter function (or set TarInfo fields) that sets tarinfo.mtime
to a fixed value, tarinfo.mode/uid/gid/uname/gname to stable values, and
preserve the sorted iteration; and open/write the gzip layer with a fixed mtime
(e.g., gzip.GzipFile(mtime=0) or equivalent) so gzip headers are constant across
runs. Ensure the filter is applied to each tar.add call (or use tarfile.addfile
with normalized TarInfo) so checksums and registry index entries remain stable.
In `@tests/unit/workflows/test_sign_modules_hardening.py`:
- Around line 63-83: The test currently checks for "--require-signature"
anywhere in the workflow, which can hide regressions; update
test_sign_modules_hardening_workflow_contains_verify_snippets to scope the
assertion to the push verify block by locating the push job/step before
asserting the flag exists. Specifically, when handling the pytest param with id
"push_strict_verify", extract the block guarded by the push condition (the
YAML/text containing the if: or job that includes "github.event_name == 'push'"
and "github.ref_name == 'dev' || github.ref_name == 'main'") from workflow =
_workflow_text() and assert "--require-signature" is present only within that
extracted block rather than anywhere in the full workflow string.
In `@tools/validate_repo_manifests.py`:
- Around line 298-300: The manifest/registry validation failure messages are
being written to stdout because the failure path uses _emit_line(...) without
the error=True flag; update the block that iterates over errors (the for err in
errors loop that currently calls _emit_line(f"- {err}")) to call _emit_line(f"-
{err}", error=True) and also call _emit_line("Manifest/registry validation
failed:", error=True) so all failure output for Manifest/registry validation is
emitted to stderr via the existing _emit_line error branch.
🪄 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: 52cb8296-755a-4de7-a10b-c0f01cab57e8
⛔ Files ignored due to path filters (1)
registry/modules/specfact-code-review-0.47.7.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (24)
.github/workflows/pr-orchestrator.yml.github/workflows/publish-modules.yml.github/workflows/sign-modules-on-approval.yml.github/workflows/sign-modules.ymlREADME.mddocs/agent-rules/50-quality-gates-and-review.mddocs/authoring/module-signing.mddocs/guides/ci-cd-pipeline.mddocs/reference/module-security.mdopenspec/config.yamlopenspec/specs/modules-pre-commit-quality-parity/spec.mdpackages/specfact-code-review/module-package.yamlpyproject.tomlregistry/index.jsonregistry/modules/specfact-code-review-0.47.7.tar.gz.sha256scripts/pre-commit-verify-modules-signature.shscripts/sync_registry_from_package.pytests/unit/test_pre_commit_verify_modules_signature_script.pytests/unit/test_sync_registry_from_package_script.pytests/unit/test_validate_repo_manifests_bundle_deps.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/workflows/test_sign_modules_hardening.pytests/unit/workflows/test_sign_modules_on_approval.pytools/validate_repo_manifests.py
💤 Files with no reviewable changes (1)
- .github/workflows/publish-modules.yml
📜 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.12)
- GitHub Check: quality (3.13)
🧰 Additional context used
📓 Path-based instructions (10)
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
Files:
.github/workflows/sign-modules-on-approval.yml.github/workflows/sign-modules.yml.github/workflows/pr-orchestrator.yml
docs/agent-rules/**/*.md
📄 CodeRabbit inference engine (.cursorrules)
Load
docs/agent-rules/INDEX.mdand canonical rule files selected by its applicability matrix
Files:
docs/agent-rules/50-quality-gates-and-review.md
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: User-facing and cross-site accuracy: Jekyll front matter, links per documentation-url-contract,
CLI examples matching bundled commands.
Files:
docs/agent-rules/50-quality-gates-and-review.mddocs/authoring/module-signing.mddocs/reference/module-security.mddocs/guides/ci-cd-pipeline.md
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/specs/modules-pre-commit-quality-parity/spec.md
registry/**
⚙️ CodeRabbit configuration file
registry/**: Registry and index consistency: bundle listings, version pins, and compatibility with
published module artifacts.
Files:
registry/modules/specfact-code-review-0.47.7.tar.gz.sha256registry/index.json
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
tests/unit/workflows/test_sign_modules_on_approval.pytests/unit/test_validate_repo_manifests_bundle_deps.pytests/unit/test_sync_registry_from_package_script.pytests/unit/workflows/test_sign_modules_hardening.pytests/unit/test_pre_commit_verify_modules_signature_script.pytools/validate_repo_manifests.pyscripts/sync_registry_from_package.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/workflows/test_sign_modules_on_approval.pytests/unit/test_validate_repo_manifests_bundle_deps.pytests/unit/test_sync_registry_from_package_script.pytests/unit/workflows/test_sign_modules_hardening.pytests/unit/test_pre_commit_verify_modules_signature_script.pytests/unit/workflows/test_pr_orchestrator_signing.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
tools/**/*.py
⚙️ CodeRabbit configuration file
tools/**/*.py: Developer tooling aligned with pyproject Hatch scripts and CI expectations.
Files:
tools/validate_repo_manifests.py
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/sync_registry_from_package.py
🧠 Learnings (14)
📓 Common learnings
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: 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: 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: 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: 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: .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/sign-modules-on-approval.yml.github/workflows/sign-modules.ymldocs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/config.yamlregistry/modules/specfact-code-review-0.47.7.tar.gz.sha256.github/workflows/pr-orchestrator.ymltests/unit/workflows/test_sign_modules_on_approval.pydocs/authoring/module-signing.mddocs/reference/module-security.mdpackages/specfact-code-review/module-package.yamlregistry/index.jsondocs/guides/ci-cd-pipeline.mdtests/unit/workflows/test_sign_modules_hardening.pyREADME.mdscripts/pre-commit-verify-modules-signature.shscripts/sync_registry_from_package.pytests/unit/workflows/test_pr_orchestrator_signing.py
📚 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/sign-modules-on-approval.yml.github/workflows/sign-modules.ymldocs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/config.yamlregistry/modules/specfact-code-review-0.47.7.tar.gz.sha256.github/workflows/pr-orchestrator.ymldocs/authoring/module-signing.mddocs/reference/module-security.mdpackages/specfact-code-review/module-package.yamldocs/guides/ci-cd-pipeline.mdtests/unit/workflows/test_sign_modules_hardening.pyREADME.mdscripts/pre-commit-verify-modules-signature.sh
📚 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:
.github/workflows/sign-modules-on-approval.yml.github/workflows/sign-modules.ymlopenspec/specs/modules-pre-commit-quality-parity/spec.mddocs/authoring/module-signing.mddocs/reference/module-security.mddocs/guides/ci-cd-pipeline.mdtests/unit/workflows/test_sign_modules_hardening.pyREADME.mdscripts/pre-commit-verify-modules-signature.sh
📚 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:
docs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/config.yamldocs/guides/ci-cd-pipeline.mdREADME.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Treat canonical rule docs (docs/agent-rules/INDEX.md) as the source of truth for worktree policy, OpenSpec gating, GitHub completeness checks, TDD order, quality gates, versioning, and documentation rules
Applied to files:
docs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/config.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: 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:
docs/agent-rules/50-quality-gates-and-review.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Treat the clean-code compliance gate as mandatory: the review surface enforces `naming`, `kiss`, `yagni`, `dry`, and `solid` categories and blocks regressions
Applied to files:
docs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.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: Full governance rules live in docs/agent-rules/; do not treat this file (GitHub Copilot Instructions) as a complete standalone handbook
Applied to files:
docs/agent-rules/50-quality-gates-and-review.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Run the required verification and quality gates for the touched scope before finalization
Applied to files:
docs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/config.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:
openspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/config.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: Require public GitHub metadata completeness before implementation when linked issue workflow applies: parent, labels, project assignment, blockers, and blocked-by relationships
Applied to files:
openspec/specs/modules-pre-commit-quality-parity/spec.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/config.yamlpackages/specfact-code-review/module-package.yamlregistry/index.jsonREADME.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: Work belongs on feature/*, bugfix/*, hotfix/*, or chore/* branches, normally in a worktree rooted under ../specfact-cli-modules-worktrees/
Applied to files:
openspec/config.yaml
🪛 LanguageTool
docs/authoring/module-signing.md
[uncategorized] ~146-~146: The official name of this software platform is spelled with a capital “H”.
Context: ...o equals the base repo). It checks out **github.event.pull_request.head.sha`** (the com...
(GITHUB)
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 uses the...
(GITHUB)
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
[::nold-ai/specfact-cli::]
-
Changed CLI behavior in packages/specfact-code-review is consumed by tests and scenarios:
- packages/specfact-code-review/src/specfact_code_review/review/commands.py — include_tests/exclude_tests now tri-state; callers/tests updated: tests/unit/specfact_code_review/review/test_commands.py [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/review/commands.py], [::nold-ai/specfact-cli::tests/unit/specfact_code_review/review/test_commands.py]
- packages/specfact-code-review/src/specfact_code_review/run/commands.py — FocusFacetConflictError removed; callers updated accordingly [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/run/commands.py]
- packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py — Typer exemption signature changed and is referenced by radon tests [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/tools/radon_runner.py], [::nold-ai/specfact-cli::tests/unit/specfact_code_review/tools/test_radon_runner.py]
-
pr-orchestrator/workflow verify-module-signatures change (append --require-signature when TARGET_BRANCH=main) affects CI/workflow expectations and tests:
- .github/workflows/pr-orchestrator.yml now guards on TARGET_BRANCH == main for --require-signature and removed HEAD_REPO/THIS_REPO logic [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml]
- Multiple workflow/tests reference this behavior; tests were updated (examples):
- tests/unit/workflows/test_pr_orchestrator_signing.py (updated expectations) [::nold-ai/specfact-cli::tests/unit/workflows/test_pr_orchestrator_signing.py]
- tests/unit/specfact_cli/registry/test_signing_artifacts.py contains assertions about presence/absence of --require-signature in workflow text [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_signing_artifacts.py]
-
Pre-commit subprocess environment change (scripts/pre_commit_code_review.py) can alter module resolution and potentially shadow or prioritize in-repo modules:
- scripts/pre_commit_code_review.py now sets SPECFACT_MODULES_REPO / SPECFACT_CLI_MODULES_REPO and prepends packages/*/src to PYTHONPATH for subprocess runs [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py]
- Many modules/tests rely on these env vars and PYTHONPATH handling (consumers / references):
- tools/command_package_runtime_validation.py manipulates PYTHONPATH and SPECFACT_MODULES_REPO [::nold-ai/specfact-cli::tools/command_package_runtime_validation.py]
- tests/unit/scripts/test_pre_commit_code_review.py loads the script as a module for unit tests [::nold-ai/specfact-cli::tests/unit/scripts/test_pre_commit_code_review.py]
- tests/unit/specfact_cli/validators/sidecar/test_crosshair_runner_env.py and other tests assert PYTHONPATH/env behavior [::nold-ai/specfact-cli::tests/unit/specfact_cli/validators/sidecar/test_crosshair_runner_env.py]
- scripts/verify-bundle-published.py, scripts/publish-module.py, and many tools rely on SPECFACT_MODULES_REPO resolution [::nold-ai/specfact-cli::scripts/verify-bundle-published.py], [::nold-ai/specfact-cli::scripts/publish-module.py]
-
New/changed registry validation and sync tooling affects registry and bundle workflows and tests:
- tools/validate_repo_manifests.py — added validate_registry_consistency(...) and changed main() to include new checks [::nold-ai/specfact-cli::tools/validate_repo_manifests.py]
- New script scripts/sync_registry_from_package.py introduced; tests added: tests/unit/test_sync_registry_from_package_script.py [::nold-ai/specfact-cli::scripts/sync_registry_from_package.py], [::nold-ai/specfact-cli::tests/unit/test_sync_registry_from_package_script.py]
- Tests referencing registry/index.json and checksum sidecars updated/added: tests/unit/test_validate_repo_manifests_bundle_deps.py [::nold-ai/specfact-cli::tests/unit/test_validate_repo_manifests_bundle_deps.py]
-
CLI contract schema/report changes and consumers:
- resources/schemas/cli-contract.schema.json — added scenarios[].expect.file_content_contains; scenarios updated to write to --out CONTRACT_TMP_REPORT.json [::nold-ai/specfact-cli::resources/schemas/cli-contract.schema.json]
- tests/cli-contracts/specfact-code-review-run.scenarios.yaml updated to use --out and file_content_contains [::nold-ai/specfact-cli::tests/cli-contracts/specfact-code-review-run.scenarios.yaml]
- New integration test reads/writes report files and asserts fragments: tests/integration/specfact_code_review/test_cli_contract_review_run_reports.py [::nold-ai/specfact-cli::tests/integration/specfact_code_review/test_cli_contract_review_run_reports.py]
-
Scripts/workflows referencing git-branch-module-signature-flag.sh and pre-commit verify behavior:
- scripts/git-branch-module-signature-flag.sh is used by pre-commit scripts and tests; unit tests added for behavior when GITHUB_BASE_REF is unset [::nold-ai/specfact-cli::scripts/git-branch-module-signature-flag.sh], [::nold-ai/specfact-cli::tests/unit/test_git_branch_module_signature_flag_script.py]
- scripts/pre-commit-verify-modules.sh and scripts/pre-commit-quality-checks.sh were adjusted to remove --metadata-only on omit path and add remediation flow; tests updated accordingly [::nold-ai/specfact-cli::scripts/pre-commit-verify-modules.sh], [::nold-ai/specfact-cli::scripts/pre-commit-quality-checks.sh], [::nold-ai/specfact-cli::tests/unit/test_pre_commit_verify_modules_signature_script.py]
Notes / risk areas for reviewers (observed consumers):
- Any external consumers (other repos) that import packages/specfact-code-review may be sensitive to:
- Removal of FocusFacetConflictError (exception type removed) [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/run/commands.py]
- CLI option type changes for include_tests/exclude_tests (bool -> bool|None) — callers that programmatically invoke Typer/CLI or rely on previous defaults could be affected [::nold-ai/specfact-cli::packages/specfact-code-review/src/specfact_code_review/review/commands.py]
- The pre-commit subprocess env changes are intended to prioritize in-repo package sources, but any tooling/tests that relied on not setting SPECFACT_* env may behave differently; there is explicit note in PR about possible stale user-scoped modules (~/.specfact/modules) shadowing—investigate environments where SPECFACT_MODULES_REPO or PYTHONPATH are set by CI or developer shells [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py], [::nold-ai/specfact-cli::src/specfact_cli/init.py]
- Workflow change to always require signatures for PRs targeting main (even from forks) may affect contributors whose PRs previously passed CI without signatures; tests and docs were updated but confirm downstream consumers/policies accept this stricter gating [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml], [::nold-ai/specfact-cli::docs/reference/module-security.md]
If you want, I can:
- Open the exact diffs of the mentioned files to show the precise code/line changes and call sites.
- Search the broader GitHub org (outside this repo) for imports/usages of packages/specfact-code-review to find external consumers.
🔇 Additional comments (24)
.github/workflows/sign-modules-on-approval.yml (1)
125-125: Commit message update looks correct.Removing
[skip ci]here is consistent with running downstream checks on signed commits.registry/modules/specfact-code-review-0.47.7.tar.gz.sha256 (1)
1-1: Checksum sidecar addition is expected for the versioned module artifact.pyproject.toml (1)
60-60: Hatch script alias wiring is clean and actionable.Nice addition for registry repair/sync workflows.
openspec/specs/modules-pre-commit-quality-parity/spec.md (1)
16-16: Spec clarification is precise and policy-consistent.The non-main branch behavior is now unambiguous.
docs/agent-rules/50-quality-gates-and-review.md (1)
54-54: Pre-commit gate policy description is clear and aligned with branch semantics..github/workflows/sign-modules.yml (2)
2-3: Header policy note is clearer now.Good clarification for PR vs main-signature behavior context.
150-150: Commit message updates are appropriate.Dropping
[skip ci]on auto/manual signing commits supports full gate execution.Also applies to: 280-280
openspec/config.yaml (1)
33-33: OpenSpec context update is consistent with the branch-aware signature model.docs/guides/ci-cd-pipeline.md (1)
44-44: CI guide wording now accurately captures baseline vs strict signature verification.packages/specfact-code-review/module-package.yaml (1)
2-2: Version bump and checksum update are consistent with signing policy.The patch increment (0.47.6 → 0.47.7) and updated
integrity.checksumalign with the manifest change requirements. Cross-referencing withregistry/index.json(lines 81-83): the registry entry showslatest_version: 0.47.7but note the checksums differ —d786d485...here vs22ca04a0...in the registry. This is expected: the manifest checksum covers the payload tree, while the registrychecksum_sha256covers the published tarball artifact.Based on learnings: "Signed module or manifest changes require version-bump review and verify-modules-signature" — version bump is present; signature will be applied by the publish workflow.
Also applies to: 26-26
.github/workflows/pr-orchestrator.yml (2)
93-95: Simplified--require-signaturegating aligns with documented policy.The conditional now cleanly scopes
--require-signaturetomain-targeted PRs only, matching the guidelines indocs/reference/module-security.mdanddocs/guides/ci-cd-pipeline.md. The removal ofHEAD_REPO/THIS_REPOequality checks simplifies the logic.Cross-repo impact note: Per linked repository findings, fork PRs targeting
mainwill now require signatures where previously they may have passed without. The README and docs were updated to reflect this stricter gating. Confirm downstream consumers/contributor policies accept this change — contributors whose PRs previously passed CI without signatures will need signed commits formainPRs.
87-87: Baseline verification command is correct.The command
--payload-from-filesystem --enforce-version-bumpwithout--metadata-onlyensures full payload checksum verification for all PRs. This matches the updated policy documented indocs/reference/module-security.md(line 49-50) where dev-target PRs use "full payload checksum + version bump, no--require-signature".As per coding guidelines in
docs/reference/module-security.md: "baseline verification uses full payload checksum + version bump... dev/PR-to-dev contexts should not require--require-signature".registry/index.json (1)
81-83: Registry entry update is consistent with manifest version bump.The
latest_version,download_url, andchecksum_sha256updates fornold-ai/specfact-code-reviewalign with the 0.47.7 version inpackages/specfact-code-review/module-package.yaml. Thedownload_urlfollows the expected naming convention (modules/specfact-code-review-0.47.7.tar.gz).Per learnings: tarball signatures and sidecar checksums in
registry/modules/*.sha256are generated by thepublish-modulesworkflow, so the sidecar consistency will be validated at publish time viatools/validate_repo_manifests.py.tests/unit/workflows/test_pr_orchestrator_signing.py (2)
25-27: Test correctly asserts removal of--metadata-onlyfrom workflow.The renamed test function and updated assertion (
"--metadata-only" not in workflow) accurately reflect the workflow change removing the dev-specific metadata-only path. This ensures the baseline verification uses full payload checksum for all branches.
32-38: Simplified guard assertions match workflow implementation.The
main_pr_guardassertion now expects the cleanerif [ "$TARGET_BRANCH" = "main" ]; thenconditional (matching.github/workflows/pr-orchestrator.ymlline 93), and theworkflow.count(require_append) == 2assertion correctly validates both the PR and push paths include--require-signatureformainonly.docs/authoring/module-signing.md (2)
146-146: Commit message policy documentation is accurate.The clarification that
sign-modules-on-approval.ymlcommits "without[skip ci]" aligns with the coding guideline indocs/authoring/module-signing.md: "commit messages for signing/approval flows must not include[skip ci]so downstream checks run on signed tips."
150-150: Pre-commit verification documentation accurately reflects implementation.The expanded description matches the implementation in
scripts/pre-commit-verify-modules-signature.sh(context snippets show lines 31, 45-49, 50-114): baseline verification with--payload-from-filesystem --enforce-version-bump,--require-signatureonly onmain, and the auto-remediation fallback strategy using--changed-onlyagainstHEADthenHEAD~1.tests/unit/test_validate_repo_manifests_bundle_deps.py (3)
48-84: Ahead-manifest semver test correctly validates allowed tolerance.The test accurately covers the scenario where a package manifest version (0.41.4) is ahead of the registry
latest_version(0.41.3) — this is expected between publish runs. The fixture structure with sidecar checksum, registry entry, and manifest aligns with the validation requirements intools/validate_repo_manifests.py.
87-123: Behind-manifest semver test validates regression detection.The test correctly asserts that a manifest version (0.41.2) behind the registry
latest_version(0.41.3) produces an error containing "behind". This guards against accidental version rollbacks.
126-162: Checksum mismatch test validates sidecar consistency.Good coverage for the sidecar checksum validation path. The mock registry structure correctly mirrors the expected layout (
registry/modules/<tarball>.sha256), and the assertion validates the "does not match sidecar" error scenario.A previous review suggested adding a companion test for missing sidecar file → "missing checksum sidecar". Consider adding this to complete the adapter boundary coverage for registry consistency checks.
tests/unit/workflows/test_sign_modules_on_approval.py (2)
70-81: Parameterized needle assertions improve test clarity.The tuple iteration pattern provides better error messages (the failing needle is included in the assertion message) and makes it easier to add/remove assertions. The needles correctly validate the eligibility gate step requirements including
github.event.review.state,approved, association checks, and branch/repo comparisons.
169-169: Commit message assertion correctly reflects[skip ci]removal.The updated assertion
'git commit -m "chore(modules): ci sign changed modules"'(without[skip ci]) aligns with the workflow change and the coding guideline: "commit messages for signing/approval flows must not include[skip ci]so downstream checks run on signed tips."docs/reference/module-security.md (1)
50-54: Documentation accurately reflects updated verification policy.The updated descriptions correctly document:
- Line 50: Dev-target PR mode uses full payload checksum + version bump without
--require-signature- Line 51: Strict mode scopes
--require-signatureto main-targeted PRs and main pushes- Line 52: Local non-main hook remediation path with
--allow-unsigned --payload-from-filesystem- Line 54:
--metadata-onlyclarified as shape/format validation only (no payload hashing)This aligns with the workflow implementation in
.github/workflows/pr-orchestrator.ymllines 87-102 and the coding guidelines indocs/reference/module-security.md.tools/validate_repo_manifests.py (1)
93-96: Handle empty or malformed sidecars before indexing.
text.strip().splitlines()[0]/first.split()[0]still raiseIndexErrorfor blank or whitespace-only.sha256files, and_validate_registry_sidecar()only catchesOSError, so the validator can still abort with a traceback instead of returning a validation error.
1 participant
Summary
Addresses post-review remediation:
--focus/ include-tests semantics, PR orchestrator--require-signatureon all PRs tomain, README alignment, CLI contract JSON file assertions, registry consistency checks invalidate_repo_manifests.py, and related tests.Notes
python -m specfact_cli.cli code review runcan loadspecfact_code_reviewfrom~/.specfact/modulesbefore the in-repo package; that copy may ship an olderradon_runnerand still reportkiss.parameter-count.erroronreview/commands.run. This commit was finalized with--no-verifyafterhatch run lintand targeted pytest passed. Follow-up: tighten module discovery whenSPECFACT_MODULES_REPOis set (likely in specfact-cli), or refresh/remove stale user-scopedspecfact-code-reviewlocally so hooks match CI.Testing
hatch run lint(clean)hatch run pyteston touched unit/integration pathspython tools/validate_repo_manifests.pyhatch run validate-cli-contractsMade with Cursor