Conversation
Harden module command reliability gates
chore(registry): publish changed modules
|
Strix is installed on this repository, but we could not run this PR security review because this workspace does not have an active plan. If you'd like to continue receiving code reviews, you can add a payment method or manage billing here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 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). (4)
🧰 Additional context used📓 Path-based instructions (3)**/*.{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:
registry/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2026-04-02T21:49:07.435ZApplied to files:
🔀 Multi-repo context nold-ai/specfact-cliLinked repositories findingsnold-ai/specfact-cli
Summary assessment: the specfact-cli repository already expects a local/specfact-cli-modules checkout via SPECFACT_MODULES_REPO and exposes hatch scripts (contract-test-contracts) used by the updated workflows in the modules PR. This confirms the modules PR changes to CI/workflow steps (checking out specfact-cli, running generate/check scripts and using contract-test-contracts) are interoperable with specfact-cli’s existing expectations for SPECFACT_MODULES_REPO and hatch script names. [::nold-ai/specfact-cli::pyproject.toml:292][::nold-ai/specfact-cli::scripts/verify-bundle-published.py:14-18] 🔇 Additional comments (3)
📝 WalkthroughPR
|
| Layer / File(s) | Summary |
|---|---|
Enforcement runtime and report fields packages/specfact-code-review/src/..., packages/specfact-code-review/src/specfact_code_review/run/*, tests/unit/specfact_code_review/* |
Adds enforcement modes (full, changed, shadow), new types/fields (ReviewEnforcementMode, ReviewReport.enforcement_*), changed-line parsing and gating logic, and tests covering enforcement behavior. |
review run CLI surface and wiring packages/specfact-code-review/src/specfact_code_review/review/commands.py, packages/specfact-code-review/src/specfact_code_review/run/commands.py |
Introduces --enforcement CLI flag, maps legacy --mode, centralizes run execution into helper dataclass and executor, updates docs/guidance strings and CLI help examples. |
Pre-commit/CI gate wiring and tools scripts/pre-commit-quality-checks.sh, scripts/pre_commit_code_review.py, .github/workflows/*, pyproject.toml |
Adds command-overview generation/contract checks to pre-commit and CI, replaces contract-test invocation with contract-test-contracts, exposes configurable SPECFACT_CODE_REVIEW_ENFORCEMENT, and updates hatch scripts. |
Command overview generator and contract checker scripts/generate-command-overview.py, scripts/check-command-contract.py, scripts/check-docs-commands.py, scripts/check-prompt-commands.py |
Adds deterministic command tree generator producing JSON/MD/LLMs, and contract/validation scripts that validate docs/prompts against generated command metadata. |
CLI contract hardening and behavioral fixes packages/specfact-codebase/src/.../import_cmd/commands.py, packages/specfact-backlog/src/.../backlog/auth_commands.py, packages/specfact-backlog/src/.../backlog_core/commands/delta.py, packages/specfact-project/src/... |
Normalizes Typer contexts, adds missing-subcommand help+exit behavior, resolves backlog project id from .specfact/backlog-config.yaml, detects legacy option-order misuse, and updates several command help/error flows. |
Docs, prompts, and examples parity docs/**, packages/*/resources/prompts/**, README.md, generated docs/reference/commands.generated.*, llms.txt |
Replaces legacy specfact sync bridge with canonical specfact project sync bridge, updates many examples to new command shapes and enforcement examples, and adds generated command reference artifacts. |
Package manifests, registry, and signatures packages/*/module-package.yaml, registry/modules/*.sha256, registry/signatures/*.tar.sig, registry/index.json |
Bumps several package versions, refreshes checksums and signatures, and updates registry index entries to match new bundle artifacts. |
Tests and e2e scenarios tests/unit/*, tests/e2e/*, tests/cli-contracts/* |
Adds/updates unit, integration, and scenario tests to cover enforcement flags, command-contract parity, prompt validation, delta/backlog behavior, and generated command artifacts. |
Sequence Diagram(s)
sequenceDiagram
participant Dev as Developer
participant PreCommit as Pre-commit hook
participant Gen as generate-command-overview
participant ContractCheck as check-command-contract
participant CI as Docs Review/PR orchestrator
Dev->>PreCommit: commit & push
PreCommit->>Gen: hatch run generate-command-overview
Gen->>PreCommit: writes commands.generated.json, md, llms.txt
PreCommit->>ContractCheck: hatch run check-command-overview / check-command-contract
PreCommit->>CI: proceed when checks pass
CI->>ContractCheck: runs checks in docs-review.yml/pr-orchestrator
CI->>Dev: fail/pass status and enforcement-mode reports
Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related issues
- [User Story] Fix tester-reported module CLI command contracts #306 — Implements the same tester-module CLI reliability scope (command contracts, overview generation, env probing) addressed by these changes.
- Inconsistent backlog CLI UX: daily works from adapter defaults, delta status requires project-id and reports undocumented repo requirements specfact-cli#592 — Matches backlog delta/status config resolution and option-naming fixes present here.
- [User Story] Harden CLI command guidance, command inventory, and package-manager runtime checks specfact-cli#594 — Related to deterministic command-overview generation and CLI contract validation introduced in this PR.
Possibly related PRs
- nold-ai/specfact-cli-modules#307 — CI gate and command-overview checks; overlaps heavily with the generate/check scripts and workflow changes here.
- nold-ai/specfact-cli-modules#287 — Adjacent changes to code-review run behavior and enforcement semantics.
- nold-ai/specfact-cli-modules#292 — Related edits to the code-review CLI wiring and simplify/instructions examples; touches the same command surface.
Focus notes: review for correctness of enforcement gating (changed vs full vs shadow), ensure generated command artifacts match mounted Typer apps, validate pre-commit/CI gating won't block releases incorrectly, and confirm registry signature/checksum updates were produced by the release process (not scripted content edits).
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
dev
djm81
added
bug
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96f5935eab
ℹ️ 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: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/unit/scripts/test_pre_commit_code_review.py (1)
81-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the "default changed" assertions independent of ambient
SPECFACT_CODE_REVIEW_ENFORCEMENT.Both tests currently read the real process environment, so they will fail whenever CI or a parent wrapper exports
SPECFACT_CODE_REVIEW_ENFORCEMENT=fullorshadow. Clear the variable withmonkeypatch.delenv("SPECFACT_CODE_REVIEW_ENFORCEMENT", raising=False)before asserting the default path, or pass the enforcement explicitly in the command-building test.Also applies to: 289-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/scripts/test_pre_commit_code_review.py` around lines 81 - 94, The test test_build_review_command_writes_json_report reads the real environment so its assertion that the enforcement defaults to "changed" can fail; update the test in tests/unit/scripts/test_pre_commit_code_review.py (and the other affected test around lines 289-303) to either clear the env var with monkeypatch.delenv("SPECFACT_CODE_REVIEW_ENFORCEMENT", raising=False) before calling module.build_review_command, or call module.build_review_command with an explicit enforcement argument (e.g., enforcement="changed") so the command construction is deterministic; ensure the change targets the test function name test_build_review_command_writes_json_report and the other noted test to make the "default changed" assertion independent of ambient SPECFACT_CODE_REVIEW_ENFORCEMENT.packages/specfact-project/resources/prompts/specfact.03-review.md (1)
167-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
specfact.03-review.mdcommands:specfact project export --stdoutdoesn’t export questions/findings
- In
packages/specfact-project/resources/prompts/specfact.03-review.md:167-181, the “Export questions to file” and both “Get findings” examples all usespecfact project export ... --stdout, butspecfact project export(theexport_personacommand) outputs persona-owned Markdown to stdout—there are no--questions/--findings(or JSON) modes there, so it won’t produce/tmp/questions.json//tmp/findings.json.- The “questions/findings” export options live under
specfact plan review(e.g.,--list-questions --output-questions /tmp/questions.jsonand--list-findings --findings-format json/--output-findings /tmp/findings.json), so the prompt examples should be updated to usespecfact plan reviewinstead ofspecfact project export.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-project/resources/prompts/specfact.03-review.md` around lines 167 - 181, Update the examples in specfact.03-review.md that currently call "specfact project export ... --stdout" for exporting questions/findings to instead use the correct subcommand and flags under "specfact plan review": replace the "Export questions to file" example with a "specfact plan review --list-questions --output-questions /tmp/questions.json" invocation, and replace the "Get findings" examples with "specfact plan review --list-findings --findings-format json --output-findings /tmp/findings.json" (or combine both flags if showing both outputs at once); ensure the surrounding text and comments reference /tmp/ and mention JSON output where appropriate so the examples no longer point to the persona-only "specfact project export" command.packages/specfact-spec/src/specfact_spec/generate/commands.py (1)
1837-1841:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude bundle context in the final validation step.
The generated prompt now prints the canonical bundle-aware command, but this console summary regresses to bare
specfact govern enforce sdd. In the no-active-bundle path, following the console guidance will fail even though the prompt file contains the right invocation.Suggested fix
- console.print("4. Validate with `specfact govern enforce sdd`") + if bundle: + console.print(f"4. Validate with `specfact govern enforce sdd --bundle {bundle}`") + else: + console.print("4. Validate with `specfact govern enforce sdd --bundle <bundle-name>`")As per coding guidelines, prompt/docs command validation should use canonical supported command forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-spec/src/specfact_spec/generate/commands.py` around lines 1837 - 1841, The final console summary prints a bare "specfact govern enforce sdd" which regresses from the bundle-aware canonical command used elsewhere; replace that hardcoded string with the canonical enforce command variable produced earlier in this routine (e.g., the variable named like canonical_enforce_cmd / enforce_command / enforce_cmd that you used to render the prompt file) so the printed Next Steps always show the bundle-aware, supported invocation even in the no-active-bundle path. Ensure the console.print call uses that variable and retains the same message formatting.docs/guides/openspec-journey.md (1)
357-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't promise alignment-report output in this read-only sync example.
These lines say the current
--mode read-onlybridge run “Generates alignment report” and “Shows gaps,” but the same page still marks alignment reports and gap detection as planned. That leaves the guide internally contradictory and tells users to expect output that is not shipped yet.As per coding guidelines,
docs/**/*.md: User-facing and cross-site accuracy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/openspec-journey.md` around lines 357 - 362, The docs claim that running "specfact project sync bridge --adapter openspec --mode read-only" "Generates alignment report" and "Shows gaps" even though alignment reports and gap detection are still planned; update the Step 6 block so it does not promise those outputs for the read-only run—remove or change the two comment lines that say "→ Generates alignment report" and "→ Shows gaps between OpenSpec specs and code" (or replace them with a clear note that alignment-report/gap detection are planned/not available in read-only mode) and ensure references to "alignment report" or "gap detection" elsewhere in this section reflect that they are not yet shipped..github/workflows/pr-orchestrator.yml (1)
182-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe quality job still misses the docs/prompt command validators.
This adds overview freshness and contract checks, but it never runs
check-docs-commands.pyorcheck-prompt-commands.py. Since this PR changes prompt files underpackages/**/resources/prompts, canonical-command drift can still merge without a CI failure.As per coding guidelines, “run required quality gates after edits, including contract-first/overview freshness and docs/prompt command validators.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-orchestrator.yml around lines 182 - 207, The workflow omitted running the docs/prompt command validators so changes to prompts can bypass CI; add steps that invoke the validators (e.g., run check-docs-commands.py and run check-prompt-commands.py) alongside the other quality gates (similar to the existing steps like "Validate generated command overview" / hatch run check-command-overview); add distinct steps named like "Docs Command Validation" and "Prompt Command Validation" that run the validators (or the equivalent hatch commands) and guard them with the same condition (needs.changes.outputs.skip_tests_dev_to_main != 'true') so prompt/docs changes trigger CI failures.
🟠 Major comments (24)
packages/specfact-govern/resources/prompts/shared/cli-enforcement.md-114-121 (1)
114-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the stale command forms in this shipped prompt list.
This section now advertises
specfact code import from-code ...and aspecfact project sync bridge ...form without--bundle, while the rest of this PR standardizes onspecfact code import --repo <path> <bundle-name>andspecfact project sync bridge --adapter <adapter> --repo <path> --bundle <bundle-name>. Leaving the old forms here will feed invalid commands back into IDE prompts and can break the docs/prompt parity guarantees this release is adding.Suggested fix
-- `specfact code import from-code --repo <path> <bundle-name>` - Create or refresh a project bundle from code +- `specfact code import --repo <path> <bundle-name>` - Create or refresh a project bundle from code ... -- `specfact project sync bridge --adapter <adapter> --repo <path>` - Sync with external tools +- `specfact project sync bridge --adapter <adapter> --repo <path> --bundle <bundle-name>` - Sync with external tools🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-govern/resources/prompts/shared/cli-enforcement.md` around lines 114 - 121, The prompt list contains stale command forms; update the two outdated entries so they match the standardized CLI usage: replace `specfact code import from-code --repo <path> <bundle-name>` with `specfact code import --repo <path> <bundle-name>` and replace `specfact project sync bridge --adapter <adapter> --repo <path>` with `specfact project sync bridge --adapter <adapter> --repo <path> --bundle <bundle-name>`; also verify the surrounding note still correctly advises using explicit bundle/project-name arguments and that any mention of `specfact project --help` remains valid.scripts/pre-commit-quality-checks.sh-162-165 (1)
162-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude module manifests in the command-overview freshness trigger.
This gate only runs when docs or prompt validation is already needed, so a manifest-only change like
packages/*/module-package.yamlskips overview regeneration/checking entirely. That leavesllms.txtanddocs/reference/commands.generated.*free to drift on release bumps even though the overview spec treats manifests as freshness inputs.Suggested fix
+needs_command_overview_validation() { + local line + while IFS= read -r line; do + [ -z "${line}" ] && continue + case "${line}" in + packages/*/module-package.yaml|packages/*/src/**/commands.py|scripts/generate-command-overview.py|scripts/check-command-contract.py|pyproject.toml|docs/*|*.md|llms.txt) + return 0 + ;; + esac + done < <(staged_files) + return 1 +} + run_command_overview_validation_gate() { - if ! needs_docs_site_validation && ! needs_prompt_command_validation; then + if ! needs_command_overview_validation; then return 0 fiAs per coding guidelines,
packages/**/module-package.yamlmust maintain metadata parity with packaged src.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pre-commit-quality-checks.sh` around lines 162 - 165, The run_command_overview_validation_gate currently returns early unless docs or prompt validations are needed, which skips manifest-only changes; update this gate to also consider package module manifest changes by adding a check for changes to packages/**/module-package.yaml (e.g., introduce or reuse a boolean like needs_module_manifest_validation) and include it in the if condition so the function runs when manifests changed; locate run_command_overview_validation_gate and modify the conditional from "if ! needs_docs_site_validation && ! needs_prompt_command_validation; then" to include the manifest flag (e.g., "if ! needs_docs_site_validation && ! needs_prompt_command_validation && ! needs_module_manifest_validation; then") and ensure the manifest detection logic sets needs_module_manifest_validation appropriately.docs/reference/commands.generated.md-52-62 (1)
52-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the duplicated
reviewsegment in the published command tree.This overview now advertises
specfact code review review ..., but the supported contract isspecfact code review ...—the linkednold-ai/specfact-clipre-commit runner invokesspecfact code review run. Shipping the duplicated path here will mislead users and make downstream docs/contract checks drift from the real CLI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/commands.generated.md` around lines 52 - 62, The generated command tree contains a duplicated segment "review" (e.g., `specfact code review review ...`) instead of the correct command hierarchy (`specfact code review ...` and `specfact code review run`); fix the generator or the metadata so the extra "review" nesting is removed. Locate the generation logic that emits the command path strings (look for the output that produces entries like `specfact code review review` and references to the `nold-ai/specfact-code-review` module) and change it to concatenate only the group name once (so entries become `specfact code review run`, `specfact code review rules`, etc.), then regenerate docs/commands.generated.md or update the source CLI metadata to stop emitting the duplicate "review" token.scripts/check-prompt-commands.py-213-230 (1)
213-230:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPython command-source files are now in scope, but this extractor still skips their help strings.
The new gate accepts
packages/*/src/**/commands.py, yet this parser only handles bare command scalars andkey: valuepairs. Common Python forms likeHELP = "specfact code repro --help"never yield a candidate that starts withspecfact, so changed command sources can pass through without any validation.Suggested fix
def _iter_structured_value_command_examples(text: str, source: Path) -> list[PromptCommandExample]: """Extract commands from YAML/JSON/template scalar values.""" if source.suffix.lower() == ".md": return [] examples: list[PromptCommandExample] = [] for line_number, raw_line in enumerate(text.splitlines(), start=1): stripped = _strip_comment(raw_line.strip()).lstrip("-").strip() if not stripped: continue candidates = [stripped] if ":" in stripped: candidates.append(stripped.split(":", 1)[1].strip()) + if source.suffix.lower() == ".py" and "=" in stripped: + candidates.append(stripped.split("=", 1)[1].strip()) for candidate in candidates: candidate = candidate.strip().strip(",").strip("\"'") if not _starts_with_command(candidate): continue examples.append(PromptCommandExample(source, line_number, _normalize_prompt_command(candidate))) break return examplesregistry/modules/specfact-spec-0.40.21.tar.gz.sha256-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not replace a published payload under the same versioned artifact name.
Changing the digest for
specfact-spec-0.40.21.tar.gzmeans the contents of an already-versioned module changed in place. That breaks release immutability for signed artifacts and can desync registry consumers, caches, and checksum verification. This should ship as a new version instead of rewriting0.40.21.As per coding guidelines,
registry/**: Registry and index consistency: bundle listings, version pins, and compatibility with published module artifacts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@registry/modules/specfact-spec-0.40.21.tar.gz.sha256` at line 1, The checksum for the published artifact was changed in place—do not rewrite registry/modules/specfact-spec-0.40.21.tar.gz.sha256 to a new digest for the same versioned artifact; revert this checksum to the original value and restore the original payload, and if the module contents must change publish a new artifact version (e.g., specfact-spec-0.40.22.tar.gz with its own .sha256) instead of altering specfact-spec-0.40.21.tar.gz so registry/index immutability and checksum verification remain correct..github/workflows/docs-review.yml-16-19 (1)
16-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the overview freshness checker to the workflow path filters.
This workflow now enforces
check-command-overview, but edits toscripts/check-command-overview.pystill would not trigger the job. That creates a gap where the freshness gate can regress without this contract running.Suggested fix
- "scripts/check-prompt-commands.py" + - "scripts/check-command-overview.py" - "scripts/check-command-contract.py" - "scripts/generate-command-overview.py" - "docs/reference/commands.generated.*" - "llms.txt" @@ - "scripts/check-prompt-commands.py" + - "scripts/check-command-overview.py" - "scripts/check-command-contract.py" - "scripts/generate-command-overview.py" - "docs/reference/commands.generated.*" - "llms.txt"As per coding guidelines, "
run required quality gates after edits, including contract-first/overview freshness and docs/prompt command validators."Also applies to: 36-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docs-review.yml around lines 16 - 19, The docs-review workflow's path filters omit the overview freshness checker script so changes to scripts/check-command-overview.py won't trigger the job; add "scripts/check-command-overview.py" to the paths arrays alongside the existing entries (the same change where the other scripts are listed and the corresponding second paths block) so edits to the checker will run the check-command-overview gate.packages/specfact-project/src/specfact_project/analyzers/code_analyzer.py-393-403 (1)
393-403:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep Semgrep probing non-fatal and manager-aware.
_probe_semgrep()now lets env-manager failures escape duringCodeAnalyzerconstruction, andget_plugin_status()still falls back to a pip-only hint when the probe returns no message. That turns an optional tool check into a hard failure and can regress the active-context diagnostics this PR is trying to preserve.Suggested hardening
def _probe_semgrep(self) -> tuple[bool, str | None]: """Check if Semgrep is available in the active repository environment.""" # Skip Semgrep check in test mode to avoid timeouts if os.environ.get("TEST_MODE") == "true": return False, "Semgrep skipped in TEST_MODE" - from specfact_cli.utils.env_manager import check_tool_in_env, detect_env_manager - - env_info = detect_env_manager(self.repo_path) - available, message = check_tool_in_env(self.repo_path, "semgrep", env_info) - return available, message + try: + from specfact_cli.utils.env_manager import check_tool_in_env, detect_env_manager + + env_info = detect_env_manager(self.repo_path) + available, message = check_tool_in_env(self.repo_path, "semgrep", env_info) + fallback = f"Semgrep not available in {env_info.manager.value} environment" + return available, message or fallback + except Exception as exc: + return False, f"Semgrep probe failed: {exc}"As per coding guidelines,
packages/**/src/**/*.pyshould flag breaking assumptions about environment/mode behavior, andopenspec/changes/tester-module-cli-reliability/specs/code-review-tool-dependencies/spec.mdrequires manager-specific installation hints instead of pip-only guidance.Also applies to: 432-438
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-project/src/specfact_project/analyzers/code_analyzer.py` around lines 393 - 403, The semgrep probe (_probe_semgrep) lets environment-manager errors escape and can make CodeAnalyzer construction fail; wrap detect_env_manager/check_tool_in_env in a try/except so any env-manager failure is non-fatal, returning (False, <manager-specific hint>) instead of raising, and ensure get_plugin_status receives a manager-aware installation hint (not a generic pip-only message) when probe returns no message; apply the same non-fatal, manager-aware error-handling pattern to the other probe function(s) in this module and ensure CodeAnalyzer construction never propagates env detection exceptions.openspec/changes/tester-module-cli-reliability/TDD_EVIDENCE.md-48-53 (1)
48-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the recorded manifest versions to match the artifacts shipped in this PR.
This evidence block still records earlier patch bumps (
specfact-backlog 0.41.26,specfact-govern 0.40.22,specfact-project 0.41.19,specfact-spec 0.40.19) while the reviewed manifests/registry entries in this PR are higher (0.41.28,0.40.23,0.41.23,0.40.21, plusspecfact-code-review 0.47.41). That makes the release traceability evidence stale.As per coding guidelines,
openspec/changes/tester-module-cli-reliability/tasks.md:record passing evidence back into TDD_EVIDENCE.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/tester-module-cli-reliability/TDD_EVIDENCE.md` around lines 48 - 53, The recorded manifest version list in the TDD_EVIDENCE.md "Release hygiene" block is stale; update the module version entries to match the artifacts shipped in this PR (replace the listed patch versions for specfact-backlog, specfact-govern, specfact-project, specfact-spec and specfact-codebase with the actual shipped versions — e.g. bump specfact-backlog -> 0.41.28, specfact-govern -> 0.40.23, specfact-project -> 0.41.23, specfact-spec -> 0.40.21, and include specfact-code-review -> 0.47.41 as recorded in this PR), and ensure the hatch command lines remain accurate for the same files; edit the TDD_EVIDENCE.md "Release hygiene" section to reflect those exact version numbers and the inclusion of specfact-code-review so the evidence matches the shipped artifacts.packages/specfact-project/resources/prompts/specfact.compare.md-54-55 (1)
54-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese replacement commands no longer generate the comparison artifact the prompt still depends on.
specfact project health-checkandspecfact project exportgive grounding data, but they do not create the comparison report referenced later in Phase 2, Step 3, and the expected output block. As written, the workflow now tells the user to inspect an artifact this command sequence never produces.Also applies to: 85-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-project/resources/prompts/specfact.compare.md` around lines 54 - 55, The listed commands (specfact project health-check and specfact project export) do not produce the comparison artifact used later in Phase 2, Step 3 and the expected-output block; replace them with the command that actually generates the comparison report (for example, invoke the comparison command such as specfact project compare --repo . --bundle <bundle-name> --output comparison.json or the project's equivalent CLI that produces the comparison artifact) and update the subsequent references in Phase 2, Step 3 and the expected output block to point to that generated comparison file (e.g., comparison.json) so the prompt and artifacts are consistent.packages/specfact-project/resources/prompts/specfact.compare.md-150-150 (1)
150-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe recovery command does not create the missing manual plan file.
Line 149 says the missing artifact is
.specfact/plans/main.bundle.yaml, butspecfact code import from-code --repo . <bundle-name>creates or refreshes a project bundle from code, not that manual plan path. Users following this instruction will still be missing the file the error is asking for.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-project/resources/prompts/specfact.compare.md` at line 150, The recovery instruction is incorrect: the command `specfact code import from-code --repo . <bundle-name>` does not create the missing manual plan file `.specfact/plans/main.bundle.yaml`; update the docs to either (A) replace that line with the actual command that produces or writes the manual plan at `.specfact/plans/main.bundle.yaml` (e.g., the CLI subcommand that creates/export a plan to that path), or (B) add an explicit step telling the user to create `.specfact/plans/main.bundle.yaml` (or copy the bundle output) after running `specfact code import from-code --repo . <bundle-name>`, so the file the error references actually exists.packages/specfact-code-review/src/specfact_code_review/run/runner.py-236-256 (1)
236-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChanged-line parsing can still be corrupted by added content that starts with
++.In a unified diff, an added source line like
++ foois emitted as+++ foo. This parser treats any+++line as a new file header instead of only accepting it immediately after a---header, so--enforcement changedcan build the wrong line map and misclassify blocking findings.Suggested fix
def _parse_added_lines_from_diff(diff_text: str) -> dict[str, set[int]]: """Return added new-file line numbers from a zero-context git diff.""" changed_lines: dict[str, set[int]] = {} current_file: str | None = None + saw_source_header = False for line in diff_text.splitlines(): + if line.startswith("--- "): + saw_source_header = True + continue if line.startswith("+++ "): - destination = line[4:].strip() + if not saw_source_header: + continue + saw_source_header = False + destination = line[4:].strip() current_file = None if destination == "/dev/null" else destination.removeprefix("b/") if current_file is not None: changed_lines.setdefault(current_file, set()) continue + saw_source_header = False if current_file is None or not line.startswith("@@ "): continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-code-review/src/specfact_code_review/run/runner.py` around lines 236 - 256, The function _parse_added_lines_from_diff is misidentifying added source lines that start with "++ " as file headers because it treats any line starting with "+++ " as a destination header; update the logic so a "+++ " destination header is accepted only when it immediately follows a corresponding "--- " old-file header. Concretely, add a small state flag (e.g., last_was_old_header) or remember the previous line and only set current_file when you see "+++ " and the previous line started with "--- "; keep existing behavior for "/dev/null" and preserve changed_lines/current_file handling.docs/reference/commands.generated.json-1699-1718 (1)
1699-1718:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegenerate this overview so it includes the
specfact spec contractsubtree.This JSON is now the validator fast-path, but the same PR teaches
scripts/check-docs-commands.pythatspecfact_spec.contract.commandsis a mounted command group. Omittingcontracthere makes the generated contract disagree with the mounted command tree and can produce false docs/prompt validation failures.As per coding guidelines, “Generate deterministic command-overview artifacts from the actual command tree” and “Freshness matters: fail checks when generated artifacts are stale after relevant source/manifests/docs/prompt/validation-script changes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/commands.generated.json` around lines 1699 - 1718, The generated commands overview in docs/reference/commands.generated.json is missing the specfact spec contract subtree; regenerate the artifact from the actual command tree so it includes the mounted group from specfact_spec.contract.commands (the mounted command group under the "specfact spec" app), e.g. re-run the command-generation pipeline/script that produces commands.generated.json (the same logic used by scripts/check-docs-commands.py) so the "specfact spec contract" subtree and its subcommands appear in the "subcommands" list for "specfact spec", then re-run the checker to confirm the generated JSON matches the mounted command tree.llms.txt-17-18 (1)
17-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the required command-overview fields in
llms.txt.This table drops several fields the overview spec makes mandatory for every artifact record, including owner repo, short help, source import path, and hidden/deprecated status. That means
llms.txtno longer carries the same command contract as the generated overview set.As per coding guidelines, “Generate deterministic command-overview artifacts from the actual command tree:
llms.txt,docs/reference/commands.generated.md, anddocs/reference/commands.generated.json. Each record must include command path, owning repo/module package, install prerequisite, short help, arguments/options, subcommands, source import path (when known), and hidden/deprecated status.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@llms.txt` around lines 17 - 18, The llms.txt table is missing required command-overview fields (owner repo/module package, short help, source import path, hidden/deprecated status) required by the overview spec; update the header and each record generation so llms.txt includes columns for Command path, Module (owner repo/package), Install, Short help, Options/Arguments, Subcommands, Source import path, and Hidden/Deprecated status to match the deterministic artifacts contract used by docs/reference/commands.generated.md and docs/reference/commands.generated.json; locate the code that emits the table header/rows (the generator that writes llms.txt and the function that builds each command record) and add those fields into the emitted header and per-command record population using the existing metadata (repo/module, help text, import path, and hidden/deprecation flags).docs/guides/ai-ide-workflow.md-45-45 (1)
45-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the canonical code-review command path here.
specfact code review rundoes not match the generated command contract in this PR; the supported path isspecfact code review review run. As written, this example is user-facing wrong and should trip the docs command validator.💡 Suggested fix
-specfact code review run --scope changed --path src --enforcement shadow --no-tests +specfact code review review run --scope changed --path src --enforcement shadow --no-testsAs per coding guidelines, “Docs/prompt validators must validate shipped examples against the generated module command overview.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/ai-ide-workflow.md` at line 45, The example command uses the wrong canonical command; replace the current line `specfact code review run --scope changed --path src --enforcement shadow --no-tests` with the correct generated command `specfact code review review run --scope changed --path src --enforcement shadow --no-tests` so docs and the prompt validator match the module contract (ensure the `code review review run` token sequence is used wherever this example appears).packages/specfact-backlog/resources/prompts/shared/cli-enforcement.md-114-123 (1)
114-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the placeholder/help rows with canonical commands and drop the legacy flat shim reference.
This section now documents four different actions as
specfact project --help, and the note still says the active plan is set viaplan select. That makes the shipped prompt non-runnable and reintroduces a removed flat command family, which is exactly the contract this PR is hardening. Please regenerate these entries from the command overview and update the fallback note to the supported canonical command path instead ofplan select. As per coding guidelines, "Docs/prompt validators must validate shipped examples against the generated module command overview" and "Prompt validators must not whitelist removed flat mounts (specfact import,specfact sync,specfact plan,specfact migrate) as canonical command groups."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-backlog/resources/prompts/shared/cli-enforcement.md` around lines 114 - 123, The prompt lists placeholder/help rows and a deprecated flat shim (e.g., multiple `specfact project --help` lines and references to removed flat command families) and still references `plan select` for active-plan fallback; replace those placeholder rows with the actual canonical command examples from the generated command overview (e.g., use canonical groups like `specfact project create`, `specfact project set-plan` or the real command names returned by the command overview module) and remove any mention or examples of removed flat mounts (`specfact import`, `specfact sync`, `specfact plan`, `specfact migrate`); also update the fallback note to reference the supported canonical command path for setting the active plan (instead of `plan select`) so examples are runnable and consistent with the command overview and prompt validator rules.scripts/pre_commit_code_review.py-376-385 (1)
376-385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let
changedenforcement suppress repo-level/tooling failures.In the
changedbranch, only findings that can be mapped to a staged file/line are allowed to block. That means blockingtool_erroror other report-level failures with nofilewill be converted into a passing hook, and a failedgit diff --cachedalso degrades to a clean pass because_changed_line_blockers()becomes empty. Preserve blocking behavior for fileless blockers, or fall back toraw_ci_exit_codewhen staged-line evidence cannot be computed.Suggested guard
def _enforced_exit_code( repo_root: Path, findings_raw: list[object], *, enforcement: str, raw_ci_exit_code: int ) -> tuple[int, list[object]]: """Apply configured enforcement to the raw report exit code.""" if enforcement == "full": return raw_ci_exit_code, [] if enforcement == "shadow": return 0, [] + global_blockers = [ + item + for item in findings_raw + if _finding_is_blocking(item) + and _repo_relative_report_path(repo_root, cast(dict[str, Any], item).get("file")) is None + ] + if global_blockers: + return raw_ci_exit_code, global_blockers blockers = _changed_line_blockers(repo_root, findings_raw) return (1 if blockers else 0), blockers🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pre_commit_code_review.py` around lines 376 - 385, The "changed" enforcement must not convert repo-level/tooling failures into passes; in _enforced_exit_code call _changed_line_blockers(repo_root, findings_raw) as you already do but then detect fileless/blocking findings in findings_raw (e.g., entries lacking a 'file' field or with type 'tool_error')—if any such fileless blockers exist or if blockers is empty but there are repo-level failures, return raw_ci_exit_code and the original fileless/blocking findings instead of (0, []); otherwise keep the existing behavior of returning (1 if blockers else 0), blockers. Use the function/variable names _enforced_exit_code, _changed_line_blockers, findings_raw, and raw_ci_exit_code to locate and implement this guard.tests/unit/specfact_backlog/test_auth_commands.py-74-80 (1)
74-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the usage-error exit code explicitly.
The shared CLI error contract for missing subcommands is stronger than “non-zero”: it must exit with the usage-error status. With
assert result.exit_code != 0, an internal failure that returns1would still pass this test and mask a contract regression.Suggested tightening
- assert result.exit_code != 0 + assert result.exit_code == 2As per coding guidelines, "Module commands must follow the shared CLI error contract: when invoked with missing subcommands ... they must render actionable help ... and exit with the usage-error status."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/specfact_backlog/test_auth_commands.py` around lines 74 - 80, Replace the loose non-zero assertion with an explicit check for the shared CLI usage-error exit status: change "assert result.exit_code != 0" to assert that result.exit_code equals the shared usage-error constant (e.g. CLIExitCodes.USAGE_ERROR or USAGE_ERROR) by importing that constant and using it in the assertion so the test fails on internal errors but passes only when the command exits with the expected usage-error code.scripts/generate-command-overview.py-25-32 (1)
25-32:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd missing
specfact_spec.contractmount inscripts/generate-command-overview.py
scripts/generate-command-overview.py’sMODULE_APP_MOUNTSonly mountsspecfact_spec.spec.commands(nospecfact_spec.contract.commands). Thespecfact_spec.contractTyper app exists, but there’s noadd_typer/mountwiring for it anywhere underspecfact_spec(only self-exports inpackages/specfact-spec/src/specfact_spec/contract/app.pyand__init__.py), so the command overview generator won’t include contract commands unless you add a separate mount entry.
enforceis fine: it’s registered as a subcommand viapackages/specfact-govern/src/specfact_govern/govern/commands.py(app.add_typer(..., name="enforce")).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate-command-overview.py` around lines 25 - 32, The MODULE_APP_MOUNTS tuple in scripts/generate-command-overview.py is missing a mount for the specfact_spec.contract Typer app so contract commands aren't discovered; add an entry analogous to the existing specfact_spec.spec.commands tuple that points to "specfact_spec.contract.commands", uses the same "app" mount type, includes the package path tuple ("specfact", "spec", "contract") (or matching naming convention used for other spec modules), and the repository identifier "nold-ai/specfact-spec" so the contract commands are included in the generated overview.docs/guides/cross-module-chains.md-38-38 (1)
38-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the canonical
code importcommand tree in this chain.
specfact code import --repo . legacy-apiis out of sync with the rest of this PR, which documents brownfield intake underspecfact code import from-code/from-bridge. Leaving the flat form here will break the command-validation intent for user-facing examples.Suggested fix
-specfact code import --repo . legacy-api +specfact code import from-code legacy-api --repo .As per coding guidelines, "Docs/prompt validators must validate shipped examples against the generated module command overview."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/cross-module-chains.md` at line 38, The example uses a non-canonical command "specfact code import --repo . legacy-api" which is inconsistent with the rest of the PR; replace it with the canonical tree form "specfact code import from-code" or "specfact code import from-bridge" to match the documented brownfield intake and pass command-validation. Update the example that currently references "specfact code import --repo . legacy-api" to use the appropriate canonical subcommand ("from-code" or "from-bridge") and ensure any flags (e.g., --repo .) are moved into the canonical subcommand usage so the docs and the module command overview stay in sync.packages/specfact-codebase/resources/prompts/shared/cli-enforcement.md-114-118 (1)
114-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore canonical executable commands here instead of
--helpplaceholders.These bullets no longer describe runnable workflows; they just send the user to
specfact project --help, and Line 118 now contradicts the explicit SDD command on Line 119. In a shipped prompt, that breaks the command contract the docs/prompt validators are supposed to enforce.Suggested direction
-- `specfact project --help` - Initialize project bundle -- `specfact project --help` - Set active plan (used as default for other commands) +- `<canonical project-init command>` - Initialize project bundle +- `<canonical active-plan selection command>` - Set active plan (used as default for other commands) ... -- `specfact project --help` - Review plan (uses active plan if bundle not specified) -- `specfact project --help` - Create SDD manifest (uses active plan if bundle not specified) +- `<canonical review command>` - Review plan (uses active plan if bundle not specified) +- `specfact govern enforce sdd [<bundle-name>]` - Create or refresh SDD manifest (uses active plan if bundle not specified)As per coding guidelines, "Docs/prompt validators must validate shipped examples against the generated module command overview."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-codebase/resources/prompts/shared/cli-enforcement.md` around lines 114 - 118, Replace the `--help` placeholders with the canonical runnable commands so examples match the shipped command contract: change the bullets to use the actual commands (e.g., use specfact project init to initialize a project bundle, specfact project set-plan <plan> to set the active plan, specfact code import [<bundle-name>] --repo <path> to import code, specfact project review [<bundle-name>] to review a plan, and specfact sdd create [<bundle-name>] to create the SDD manifest), and ensure the specfact project review and specfact sdd create entries reference the same active-bundle semantics so there is no contradiction between lines.packages/specfact-project/resources/prompts/specfact.02-plan.md-31-31 (1)
31-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore plan-management commands here instead of
code import from-code.This prompt still describes
init,add-feature,add-story, andupdate-*plan flows, but the rewritten examples now ground every phase onspecfact code import from-code. That command family imports code into a bundle; it does not perform the plan mutations this prompt is teaching, so Copilot will be steered onto the wrong command tree and the missing-bundle guidance is wrong as well.As per coding guidelines, prompt validators must validate shipped examples against the generated module command overview.
Also applies to: 62-69, 96-99, 133-138, 163-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-project/resources/prompts/specfact.02-plan.md` at line 31, The examples in the specfact.02-plan prompt incorrectly ground plan-management flows on the 'specfact code import from-code' command (which only imports code) and so must be restored to use the actual plan commands; update the examples and missing-bundle guidance to reference the plan commands 'init', 'add-feature', 'add-story', and the 'update-*' flows instead of 'code import from-code', and ensure the prompt validator checks that each shipped example matches the module command overview so examples and guidance reflect the true behavior of those commands.tests/unit/specfact_backlog/test_delta_command_contract.py-54-83 (1)
54-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the usage-error exit code explicitly here.
The contract for unresolved
delta statusinputs is not just “non-zero”; it is the shared usage-error status. Using!= 0lets a regression to a generic failure code slip through while these tests still pass.Suggested tightening
- assert result.exit_code != 0 + assert result.exit_code == 2 @@ - assert result.exit_code != 0 + assert result.exit_code == 2As per coding guidelines, missing required parameters must exit with the usage-error status and tests should stay contract-first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/specfact_backlog/test_delta_command_contract.py` around lines 54 - 83, Both tests currently assert result.exit_code != 0; change them to assert the specific usage-error exit code instead. In the two tests test_delta_status_missing_config_names_kebab_case_options and test_delta_status_malformed_config_preserves_missing_context_guidance, replace the non-zero assertion on the result from runner.invoke(backlog_app, ["delta", "status", "github"]) with an explicit assertion that result.exit_code equals the CLI usage-error code (use the standard Click usage exit code, e.g., 2) so the contract for missing required parameters is enforced.tests/unit/test_global_cli_error_contract.py-26-45 (1)
26-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a shipped module command here instead of a fresh
Typerapp.This only proves Typer’s default missing-argument behavior. It will still pass if the repo’s shared missing-parameter contract regresses on real mounted apps, so it does not protect the contract this file is supposed to cover.
As per coding guidelines, “Module commands must follow the shared CLI error contract.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_global_cli_error_contract.py` around lines 26 - 45, Replace the ad-hoc Typer app in test_module_leaf_missing_argument_uses_shared_missing_parameter_contract with a real, shipped module command so the test exercises our shared CLI error contract; import or obtain the existing module's Typer/command object (instead of creating app = typer.Typer(...)) and call runner.invoke on that shipped command with ["apply"] (keeping the existing assertions), referencing the test name and the apply_module_change handler replacement to locate the code to edit.scripts/check-command-contract.py-79-83 (1)
79-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail on malformed overview entries instead of silently skipping them.
_load_records()drops any non-dict entry and keeps going. That means a truncated or hand-editedcommands.generated.jsoncan lose commands while this gate still reports OK after validating only the survivors.Suggested hardening
def _load_records() -> list[dict[str, Any]]: raw = json.loads(COMMANDS_JSON.read_text(encoding="utf-8")) if not isinstance(raw, list): raise ValueError(f"{COMMANDS_JSON} must contain a JSON list") - return [entry for entry in raw if isinstance(entry, dict)] + records: list[dict[str, Any]] = [] + for index, entry in enumerate(raw): + if not isinstance(entry, dict): + raise ValueError(f"{COMMANDS_JSON}[{index}] must be an object") + command = entry.get("command") + if not isinstance(command, str) or not command.strip(): + raise ValueError(f"{COMMANDS_JSON}[{index}] is missing a valid 'command'") + records.append(entry) + return recordsAs per coding guidelines, “Each record must include command path ...” and checks should fail when generated artifacts are invalid or stale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-command-contract.py` around lines 79 - 83, The _load_records() function currently silently filters out non-dict entries from the JSON list, allowing truncated or hand-edited COMMANDS_JSON to pass validation; change it to detect and raise an error instead of dropping them: after parsing raw and confirming it's a list, iterate/check that every element is a dict and raise a ValueError (including context like index or the bad entry) if any item is not a dict so malformed overview entries fail the gate rather than being skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6ccb2ea-1b07-4283-9dcf-73a81ca69c74
⛔ Files ignored due to path filters (6)
registry/modules/specfact-backlog-0.41.28.tar.gzis excluded by!**/*.gzregistry/modules/specfact-code-review-0.47.41.tar.gzis excluded by!**/*.gzregistry/modules/specfact-codebase-0.41.11.tar.gzis excluded by!**/*.gzregistry/modules/specfact-govern-0.40.23.tar.gzis excluded by!**/*.gzregistry/modules/specfact-project-0.41.23.tar.gzis excluded by!**/*.gzregistry/modules/specfact-spec-0.40.21.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (130)
.github/pull_request_template.md.github/workflows/docs-review.yml.github/workflows/pr-orchestrator.ymlREADME.mddocs/adapters/azuredevops.mddocs/adapters/github.mddocs/agent-rules/20-repository-context.mddocs/agent-rules/50-quality-gates-and-review.mddocs/bundles/backlog/refinement.mddocs/bundles/code-review/run.mddocs/bundles/project/import-migration.mddocs/bundles/project/overview.mddocs/bundles/spec/validate.mddocs/getting-started/choose-your-modules.mddocs/getting-started/first-steps.mddocs/getting-started/installation.mddocs/guides/ai-ide-workflow.mddocs/guides/brownfield-examples.mddocs/guides/brownfield-faq-and-roi.mddocs/guides/brownfield-modernization.mddocs/guides/ci-cd-pipeline.mddocs/guides/command-chains.mddocs/guides/cross-module-chains.mddocs/guides/daily-devops-routine.mddocs/guides/integrations-overview.mddocs/guides/openspec-journey.mddocs/guides/speckit-comparison.mddocs/guides/speckit-journey.mddocs/integrations/devops-adapter-overview.mddocs/modules/code-review.mddocs/quickstart-ai-bloat.mddocs/reference/README.mddocs/reference/command-syntax-policy.mddocs/reference/commands.generated.jsondocs/reference/commands.generated.mddocs/reference/commands.mddocs/team-and-enterprise/multi-repo.mdllms.txtopenspec/CHANGE_ORDER.mdopenspec/changes/tester-module-cli-reliability/.openspec.yamlopenspec/changes/tester-module-cli-reliability/TDD_EVIDENCE.mdopenspec/changes/tester-module-cli-reliability/proposal.mdopenspec/changes/tester-module-cli-reliability/specs/backlog-delta/spec.mdopenspec/changes/tester-module-cli-reliability/specs/code-review-tool-dependencies/spec.mdopenspec/changes/tester-module-cli-reliability/specs/module-cli-error-contract/spec.mdopenspec/changes/tester-module-cli-reliability/specs/module-command-overview/spec.mdopenspec/changes/tester-module-cli-reliability/specs/modules-docs-command-validation/spec.mdopenspec/changes/tester-module-cli-reliability/tasks.mdpackages/specfact-backlog/module-package.yamlpackages/specfact-backlog/resources/prompts/shared/cli-enforcement.mdpackages/specfact-backlog/resources/prompts/specfact.backlog-refine.mdpackages/specfact-backlog/resources/prompts/specfact.sync-backlog.mdpackages/specfact-backlog/src/specfact_backlog/backlog/auth_commands.pypackages/specfact-backlog/src/specfact_backlog/backlog/commands.pypackages/specfact-backlog/src/specfact_backlog/backlog_core/commands/delta.pypackages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.mdpackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/run/__init__.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pypackages/specfact-code-review/src/specfact_code_review/run/findings.pypackages/specfact-code-review/src/specfact_code_review/run/runner.pypackages/specfact-codebase/module-package.yamlpackages/specfact-codebase/resources/prompts/shared/cli-enforcement.mdpackages/specfact-codebase/src/specfact_codebase/import_cmd/commands.pypackages/specfact-codebase/src/specfact_codebase/repro/commands.pypackages/specfact-codebase/src/specfact_codebase/validate/commands.pypackages/specfact-govern/module-package.yamlpackages/specfact-govern/resources/prompts/shared/cli-enforcement.mdpackages/specfact-govern/resources/prompts/specfact.05-enforce.mdpackages/specfact-govern/src/specfact_govern/enforce/commands.pypackages/specfact-project/module-package.yamlpackages/specfact-project/resources/prompts/shared/cli-enforcement.mdpackages/specfact-project/resources/prompts/specfact.02-plan.mdpackages/specfact-project/resources/prompts/specfact.03-review.mdpackages/specfact-project/resources/prompts/specfact.04-sdd.mdpackages/specfact-project/resources/prompts/specfact.06-sync.mdpackages/specfact-project/resources/prompts/specfact.08-simplify.mdpackages/specfact-project/resources/prompts/specfact.compare.mdpackages/specfact-project/resources/templates/github-action.yml.j2packages/specfact-project/src/specfact_project/analyzers/code_analyzer.pypackages/specfact-project/src/specfact_project/import_cmd/commands.pypackages/specfact-project/src/specfact_project/plan/commands.pypackages/specfact-project/src/specfact_project/project/commands.pypackages/specfact-project/src/specfact_project/sync/commands.pypackages/specfact-project/src/specfact_project/sync_runtime/sync_perform_operation_impl.pypackages/specfact-spec/module-package.yamlpackages/specfact-spec/resources/prompts/shared/cli-enforcement.mdpackages/specfact-spec/resources/prompts/specfact.07-contracts.mdpackages/specfact-spec/src/specfact_spec/generate/commands.pypackages/specfact-spec/src/specfact_spec/sdd/commands.pypyproject.tomlregistry/index.jsonregistry/modules/specfact-backlog-0.41.28.tar.gz.sha256registry/modules/specfact-code-review-0.47.41.tar.gz.sha256registry/modules/specfact-codebase-0.41.11.tar.gz.sha256registry/modules/specfact-govern-0.40.23.tar.gz.sha256registry/modules/specfact-project-0.41.23.tar.gz.sha256registry/modules/specfact-spec-0.40.21.tar.gz.sha256registry/signatures/specfact-backlog-0.41.28.tar.sigregistry/signatures/specfact-code-review-0.47.41.tar.sigregistry/signatures/specfact-codebase-0.41.11.tar.sigregistry/signatures/specfact-govern-0.40.23.tar.sigregistry/signatures/specfact-project-0.41.23.tar.sigregistry/signatures/specfact-spec-0.40.21.tar.sigscripts/check-command-contract.pyscripts/check-docs-commands.pyscripts/check-prompt-commands.pyscripts/generate-command-overview.pyscripts/pre-commit-quality-checks.shscripts/pre_commit_code_review.pyskills/specfact-code-review/SKILL.mdtests/cli-contracts/specfact-code-review-run.scenarios.yamltests/e2e/specfact_project/test_help_smoke.pytests/unit/docs/test_code_review_docs_parity.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_backlog/conftest.pytests/unit/specfact_backlog/test_auth_commands.pytests/unit/specfact_backlog/test_delta_command_contract.pytests/unit/specfact_code_review/review/test_commands.pytests/unit/specfact_code_review/run/test_commands.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_codebase/test_import_command_contract.pytests/unit/specfact_project/test_code_analyzer_semgrep_status.pytests/unit/specfact_project/test_regenerate_command_contract.pytests/unit/test_check_docs_commands_script.pytests/unit/test_check_prompt_commands_script.pytests/unit/test_global_cli_error_contract.pytests/unit/test_pre_commit_quality_parity.pytests/unit/workflows/test_pr_orchestrator_signing.py
chore(modules): auto-sign module manifests
chore(registry): publish changed modules
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/specfact-code-review/src/specfact_code_review/run/runner.py (1)
311-353:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRebuild
ReviewReportthrough validation in_with_enforcement().
model_copy(update=...)does not rerun Pydantic validators, so these branches can stamp a newenforcement_modeonto a report while leaving derived governance fields likeschema_versionstale. That breaks the release/report contract forfull,shadow, andchangedenforcement.In Pydantic v2, does BaseModel.model_copy(update=...) rerun validators or recompute fields derived by validators? What is the official validated pattern for updating a model when derived fields must stay consistent?Suggested fix
+def _validated_report_update(report: ReviewReport, **updates: object) -> ReviewReport: + return ReviewReport.model_validate({**report.model_dump(), **updates}) + def _with_enforcement(report: ReviewReport, *, mode: ReviewEnforcementMode, files: list[Path]) -> ReviewReport: """Apply enforcement mode to report exit code while preserving all findings as evidence.""" if mode == "full": - return report.model_copy( - update={ - "enforcement_mode": "full", - "enforcement_summary": "Full enforcement blocks on any blocking finding in the reviewed files.", - } + return _validated_report_update( + report, + enforcement_mode="full", + enforcement_summary="Full enforcement blocks on any blocking finding in the reviewed files.", ) if mode == "shadow": - return report.model_copy( - update={ - "ci_exit_code": 0, - "enforcement_mode": "shadow", - "enforcement_summary": "Shadow enforcement records findings as evidence and never blocks CI.", - } + return _validated_report_update( + report, + ci_exit_code=0, + enforcement_mode="shadow", + enforcement_summary="Shadow enforcement records findings as evidence and never blocks CI.", ) @@ - return report.model_copy( - update={"ci_exit_code": 1, "enforcement_mode": "changed", "enforcement_summary": summary} - ) + return _validated_report_update( + report, + ci_exit_code=1, + enforcement_mode="changed", + enforcement_summary=summary, + ) @@ - return report.model_copy( - update={ - "overall_verdict": verdict, - "ci_exit_code": 0, - "enforcement_mode": "changed", - "enforcement_summary": summary, - } - ) + return _validated_report_update( + report, + overall_verdict=verdict, + ci_exit_code=0, + enforcement_mode="changed", + enforcement_summary=summary, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/specfact-code-review/src/specfact_code_review/run/runner.py` around lines 311 - 353, The function _with_enforcement currently uses ReviewReport.model_copy(update=...) which does not run Pydantic v2 validators or recompute derived fields (e.g., schema_version, governance-derived fields), so stamping enforcement_mode/ci_exit_code/etc. can leave the report in an inconsistent state; fix by taking the report's serializable state (use report.model_dump()), apply the desired updates for each branch (full, shadow, changed) to that dict, and then rebuild/validate a fresh ReviewReport via ReviewReport.model_validate(updated_dict) (references: _with_enforcement, ReviewReport, model_copy, model_dump, model_validate, enforcement_mode, enforcement_summary, ci_exit_code, overall_verdict).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/specfact-code-review/src/specfact_code_review/run/runner.py`:
- Around line 311-353: The function _with_enforcement currently uses
ReviewReport.model_copy(update=...) which does not run Pydantic v2 validators or
recompute derived fields (e.g., schema_version, governance-derived fields), so
stamping enforcement_mode/ci_exit_code/etc. can leave the report in an
inconsistent state; fix by taking the report's serializable state (use
report.model_dump()), apply the desired updates for each branch (full, shadow,
changed) to that dict, and then rebuild/validate a fresh ReviewReport via
ReviewReport.model_validate(updated_dict) (references: _with_enforcement,
ReviewReport, model_copy, model_dump, model_validate, enforcement_mode,
enforcement_summary, ci_exit_code, overall_verdict).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 590d1dbc-680c-465b-8002-e0f1f4614c52
⛔ Files ignored due to path filters (1)
registry/modules/specfact-code-review-0.47.43.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (10)
.github/workflows/docs-review.ymlpackages/specfact-code-review/module-package.yamlpackages/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/run/runner.pyregistry/index.jsonregistry/modules/specfact-code-review-0.47.43.tar.gz.sha256registry/signatures/specfact-code-review-0.47.43.tar.sigscripts/generate-command-overview.pytests/unit/specfact_code_review/run/test_runner.py
💤 Files with no reviewable changes (1)
- packages/specfact-code-review/src/specfact_code_review/run/commands.py
✅ Files skipped from review due to trivial changes (2)
- registry/modules/specfact-code-review-0.47.43.tar.gz.sha256
- registry/signatures/specfact-code-review-0.47.43.tar.sig
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/specfact-code-review/module-package.yaml
- scripts/generate-command-overview.py
- packages/specfact-code-review/src/specfact_code_review/review/commands.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). (1)
- GitHub Check: quality (3.12)
🧰 Additional context used
📓 Path-based instructions (5)
registry/**
⚙️ CodeRabbit configuration file
registry/**: Registry and index consistency: bundle listings, version pins, and compatibility with
published module artifacts.
Files:
registry/index.json
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
Files:
.github/workflows/docs-review.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/specfact_code_review/run/test_runner.pypackages/specfact-code-review/src/specfact_code_review/run/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/run/test_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/run/runner.py
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
nold-ai/specfact-cli
- Docs workflow expects command-generation/checking commands:
- .github/workflows/docs-review.yml — runs "hatch run check-docs-commands" and installs hatch (validates docs command examples) [::nold-ai/specfact-cli::.github/workflows/docs-review.yml:1-120].
- .github/workflows/pr-orchestrator.yml — checks out specfact-cli-modules and exports SPECFACT_MODULES_REPO for downstream jobs (tests job sets SPECFACT_MODULES_REPO) [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml:tests job Checkout module bundles repo / Export module bundles path].
- Prompt/docs validation tooling present:
- scripts/check-docs-commands.py — validator that imports the Typer app and validates docs examples against CLI help [::nold-ai/specfact-cli::scripts/check-docs-commands.py:1-240].
- scripts/pre_commit_code_review.py — pre-commit gate that builds nested specfact CLI invocation and discovery of local modules; builds env and supports discovering/using specfact-cli-modules (prepends SPECFACT_MODULES_REPO), used by pre-commit/pre-merge gates [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py:1-240].
- pre-commit-quality-checks.sh — invokes pre_commit_code_review.py and runs hatch contract-test-contracts in Block 2 (CI/pre-commit expectations) [::nold-ai/specfact-cli::scripts/pre-commit-quality-checks.sh:... lines ~116,393-398].
- Enforcement gating surfaced:
- pre_commit_code_review.py implements nested-run env and is the place where SPECFACT_MODULES_REPO / XDG_* / semgrep envs are set, and is the natural integration point for SPECFACT_CODE_REVIEW_ENFORCEMENT (workflows and pre-commit refer to enforcement behaviors) [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py:discover_specfact_modules_repo() / build_review_child_env()].
- Missing/absent artifacts in this checkout (observed during exploration):
- scripts/generate-command-overview.py and scripts/check-command-contract.py were not present/readable in the checked-out tree (sed reported "No such file or directory") — but workflows and pre-commit scripts reference check-command-contract / generate/check-command-overview in docs/pre-commit gates, so ensure these scripts are included in the PR branch or installed via hatch scripts referenced in pyproject [::nold-ai/specfact-cli::stdout listing/scripts & sed errors].
Summary implication: The target modules PR updates workflows and pre-commit gates to depend on generated command-overview artifacts and contract-check scripts plus enforcement-aware pre-commit code-review behavior. The specfact-cli repository contains the docs/validation tooling (check-docs-commands.py, pre_commit_code_review.py, pre-commit-quality-checks.sh) and workflows that expect generate/check-command-overview and check-command-contract; verify that the PR adding those generator/checker scripts (and any changes to pyproject hatch scripts) is present in the branch the workflows will use, and confirm scripts/generate-command-overview.py and scripts/check-command-contract.py are available to CI when referenced. [::nold-ai/specfact-cli::]
🔇 Additional comments (2)
registry/index.json (1)
81-83: LGTM!.github/workflows/docs-review.yml (1)
124-124: LGTM!
chore(modules): auto-sign module manifests
chore(registry): publish changed modules
|
Updated Changes:
Local validation:
|
1 participant
Summary
Validation