Conversation
…mplification-enforcement [codex] Add guided simplification review enforcement
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. |
📝 WalkthroughWalkthroughAdds guided simplification metadata and summary, enriches ai-bloat/Semgrep findings, implements an AST-based safe-mechanical autofix pipeline applied before Ruff, enforces simplify-mode failures only for unresolved safe_mechanical items, and updates prompts/skill/CLI/docs and tests; package/registry metadata bumped. ChangesGuided Simplification Enforcement
Sequence Diagram (high-level simplify/run flow): sequenceDiagram
participant CLI as specfact CLI
participant AI_Bloat as ai_bloat/Semgrep
participant Report as ReviewReport
participant Fixer as AST Fixer
participant Linter as Ruff
CLI->>AI_Bloat: run focus=simplify (emit .specfact JSON)
AI_Bloat->>Report: produce ReviewFinding with guidance_kind
CLI->>Fixer: if --fix && guidance safe_mechanical -> apply fixes
Fixer->>Linter: run Ruff autofix
Linter->>CLI: rerun review -> updated ReviewReport
Report->>CLI: enforce fails if blocking safe_mechanical
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Focus notes: review for correctness of schema validation (preserve_reason constraint), AST rewrite safety (no semantic regressions), evidence/reference integrity for before/after, and the packaging/signature updates to avoid release integrity issues. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
djm81
added
enhancement
![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: 9007aa9ece
ℹ️ 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: 4
🧹 Nitpick comments (1)
packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py (1)
372-384: ⚡ Quick winConsider adding a parity test for
SEMGREP_RULE_CATEGORYandAI_BLOAT_GUIDANCE.The direct lookup
AI_BLOAT_GUIDANCE[rule]at line 388 relies on both dictionaries having matching keys for ai_bloat rules. While theKeyErroris caught higher up (returning a tool error), this coupling is a maintenance risk—adding a new ai-bloat rule to one dictionary but forgetting the other will silently degrade to a tool error at runtime.A simple test asserting parity would catch drift early:
def test_ai_bloat_guidance_covers_all_ai_bloat_rules(): ai_bloat_rules = {k for k, v in SEMGREP_RULE_CATEGORY.items() if v == "ai_bloat"} assert ai_bloat_rules == set(AI_BLOAT_GUIDANCE.keys())Also applies to: 387-408
🤖 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/tools/semgrep_runner.py` around lines 372 - 384, Add a unit test that asserts parity between SEMGREP_RULE_CATEGORY and AI_BLOAT_GUIDANCE so every rule categorized as "ai_bloat" has a corresponding entry in AI_BLOAT_GUIDANCE; specifically collect keys where SEMGREP_RULE_CATEGORY[key] == "ai_bloat" and compare that set to set(AI_BLOAT_GUIDANCE.keys()), failing the test if they differ to prevent silent KeyError/tool errors at runtime.
🤖 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.
Inline comments:
In `@packages/specfact-code-review/module-package.yaml`:
- Line 2: The package shows version: 0.47.23 and TDD_EVIDENCE mentions deferred
signing/--allow-unsigned; verify the integrity/signature entry (the "integrity:"
block referenced around lines 26-27) is a real cryptographic signature (not a
placeholder) and, if it was produced later, replace the deferred placeholder
with the actual signed value or attach the verification artifact and update
TDD_EVIDENCE to record how/when it was signed; additionally, document the
repository's semver policy (e.g., README.md or CONTRIBUTING.md) clarifying
whether additive, backward-compatible schema additions use patch bumps or minor
bumps and, if needed, adjust the version or add a release note explaining why
0.47.23 is correct.
In `@packages/specfact-code-review/src/specfact_code_review/run/commands.py`:
- Around line 315-335: The current dead-branch autofix in the loop (using
prior_terminal_tests, stmt.test, _terminal_return and _replace_line_range)
treats AST-test equality as sufficient to drop a later if, which is unsafe for
stateful/non-deterministic predicates; add a syntactic "purity" check (e.g. a
new helper like _is_pure_test(test_node)) and only consider a test for
deduplication/rewriting when _is_pure_test(stmt.test) returns True. Implement
_is_pure_test to reject nodes that can have side effects or nondeterminism (at
least ast.Call, ast.Yield, ast.Await, ast.GeneratorExp, comprehensions, and
other nodes you deem non-pure) and use it in the loop before adding to
prior_terminal_tests or performing the _replace_line_range; if the test is not
pure, skip deduplication for that stmt.
In `@packages/specfact-code-review/src/specfact_code_review/run/findings.py`:
- Around line 190-218: The _validate_guided_metadata method currently requires
action_status whenever guidance_kind is present; update it to allow
action_status to be optional by removing or disabling the check that raises
ValueError for self.action_status is None while keeping all other required-field
checks (recommended_action, clean_code_principle, rationale, safety_checks, and
preserve_reason when guidance_kind == "preserve"). Also audit any logic that
counts or filters "blocking" findings by hard-checking action_status values (the
code that currently restricts blocking to "recommended"/"failed") and change it
to rely on the unresolved-safe classification or presence/semantics of
guidance_kind instead of requiring a specific action_status value.
In `@tests/unit/specfact_code_review/run/test_runner.py`:
- Around line 65-71: The fixture builds a guidance dict where
"recommended_action" is hard-coded to "collapse" even when guidance_kind ==
"preserve", causing inconsistent test data; update the construction so that when
guidance_kind == "preserve" the "recommended_action" key is set to "preserve"
(e.g., recommended_action = "preserve" if guidance_kind == "preserve" else
"collapse"), keeping the existing preserve_reason expression unchanged and
leaving other keys as-is.
---
Nitpick comments:
In
`@packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py`:
- Around line 372-384: Add a unit test that asserts parity between
SEMGREP_RULE_CATEGORY and AI_BLOAT_GUIDANCE so every rule categorized as
"ai_bloat" has a corresponding entry in AI_BLOAT_GUIDANCE; specifically collect
keys where SEMGREP_RULE_CATEGORY[key] == "ai_bloat" and compare that set to
set(AI_BLOAT_GUIDANCE.keys()), failing the test if they differ to prevent silent
KeyError/tool errors at runtime.
🪄 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: CHILL
Plan: Pro
Run ID: 08194948-2801-4462-b10b-1e050689e965
⛔ Files ignored due to path filters (2)
registry/modules/specfact-code-review-0.47.23.tar.gzis excluded by!**/*.gzregistry/modules/specfact-project-0.41.15.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (33)
.vibe/skills/specfact-code-review/SKILL.mdopenspec/CHANGE_ORDER.mdopenspec/changes/code-review-12-guided-simplification-enforcement/.openspec.yamlopenspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.mdopenspec/changes/code-review-12-guided-simplification-enforcement/design.mdopenspec/changes/code-review-12-guided-simplification-enforcement/proposal.mdopenspec/changes/code-review-12-guided-simplification-enforcement/specs/guided-simplification-review/spec.mdopenspec/changes/code-review-12-guided-simplification-enforcement/specs/review-finding-model/spec.mdopenspec/changes/code-review-12-guided-simplification-enforcement/specs/review-run-command/spec.mdopenspec/changes/code-review-12-guided-simplification-enforcement/tasks.mdpackages/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/rules/updater.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-code-review/src/specfact_code_review/tools/ai_bloat_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pypackages/specfact-project/module-package.yamlpackages/specfact-project/resources/prompts/specfact.08-simplify.mdregistry/index.jsonregistry/modules/specfact-code-review-0.47.23.tar.gz.sha256registry/modules/specfact-project-0.41.15.tar.gz.sha256registry/signatures/specfact-code-review-0.47.23.tar.sigregistry/signatures/specfact-project-0.41.15.tar.sigskills/specfact-code-review/SKILL.mdtests/unit/specfact_code_review/rules/test_updater.pytests/unit/specfact_code_review/run/test_commands.pytests/unit/specfact_code_review/run/test_findings.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/test_guided_simplify_resources.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
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-project/module-package.yamlpackages/specfact-code-review/module-package.yaml
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/code-review-12-guided-simplification-enforcement/specs/review-run-command/spec.mdopenspec/CHANGE_ORDER.mdopenspec/changes/code-review-12-guided-simplification-enforcement/specs/review-finding-model/spec.mdopenspec/changes/code-review-12-guided-simplification-enforcement/proposal.mdopenspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.mdopenspec/changes/code-review-12-guided-simplification-enforcement/tasks.mdopenspec/changes/code-review-12-guided-simplification-enforcement/specs/guided-simplification-review/spec.mdopenspec/changes/code-review-12-guided-simplification-enforcement/design.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.23.tar.gz.sha256registry/signatures/specfact-code-review-0.47.23.tar.sigregistry/signatures/specfact-project-0.41.15.tar.sigregistry/index.jsonregistry/modules/specfact-project-0.41.15.tar.gz.sha256
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
tests/unit/test_guided_simplify_resources.pypackages/specfact-code-review/src/specfact_code_review/rules/updater.pypackages/specfact-code-review/src/specfact_code_review/run/runner.pytests/unit/specfact_code_review/rules/test_updater.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/specfact_code_review/run/test_findings.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/run/findings.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pytests/unit/specfact_code_review/run/test_commands.pypackages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_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/test_guided_simplify_resources.pytests/unit/specfact_code_review/rules/test_updater.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/specfact_code_review/run/test_findings.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/specfact_code_review/run/test_commands.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators),
Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles.
Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior.
Files:
packages/specfact-code-review/src/specfact_code_review/rules/updater.pypackages/specfact-code-review/src/specfact_code_review/run/runner.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/tools/semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.py
🧠 Learnings (1)
📚 Learning: 2026-04-02T21:49:07.435Z
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:07.435Z
Learning: In nold-ai/specfact-cli-modules, module tarball signature files under registry/signatures/*.tar.sig are produced by the publish-modules GitHub Actions runner during the publish workflow (not committed to the branch). During PR pre-merge review, do not flag missing *.tar.sig files as blockers; treat signatures as publish-time artifacts.
Applied to files:
registry/signatures/specfact-code-review-0.47.23.tar.sigregistry/signatures/specfact-project-0.41.15.tar.sig
🔀 Multi-repo context nold-ai/specfact-cli
Findings (repo: nold-ai/specfact-cli)
-
This repo references the specfact-code-review and specfact-project bundles extensively (tests, scripts, openspec). Many tests/assertions expect nold-ai/specfact-code-review and specific bundle versions (e.g. dependency expectations for nold-ai/specfact-project >=0.41.0). Evidence: multiple test files and scripts that mention specfact-code-review/specfact-project (examples: tests/unit/specfact_cli/registry/test_profile_presets.py, scripts/runtime_discovery_smoke.py, tests/unit/scripts/test_publish_module_bundle.py). [::nold-ai/specfact-cli::]
-
Openspec and spec docs in this repo include specs that reference the code-review module and the simplify flow (e.g., openspec/specs/house-rules-skill/spec.md, openspec/specs/review-cli-contracts/spec.md). These imply that changes to review findings/report schema or CLI review behavior (simplify/enforce/fix) in the specfact-code-review module could affect CLI-level contracts and tests. [::nold-ai/specfact-cli::]
-
I attempted to open the package files/tests added in the PR (tests/unit/specfact_code_review/... and packages/specfact-code-review/...) but those paths are not present in this repository clone (sed failed; "No such file or directory"). That indicates the actual modified module code (specfact-code-review) lives in a different repo (specfact-cli-modules) which was not available to inspect here. Evidence: sed errors in shell output. [::nold-ai/specfact-cli::]
Implication: the PR changes (guided simplification metadata, schema version bump, safe-mechanical autofix/enforce behavior) are relevant to this repo because tests and openspec references expect the code-review bundle and specific version constraints; however, I could not inspect the modified module source here — to validate cross-repo compatibility you should run the same searches in the specfact-cli-modules repo (nold-ai/specfact-cli-modules) or provide its files. [::nold-ai/specfact-cli::]
🔇 Additional comments (61)
packages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.py (18)
21-36: LGTM!
43-64: LGTM!
127-139: LGTM!
150-186: LGTM!
214-228: LGTM!
275-291: LGTM!
314-321: LGTM!
360-365: LGTM!
390-397: LGTM!
410-418: LGTM!
463-471: LGTM!
499-503: LGTM!
532-537: LGTM!
553-555: LGTM!
590-594: LGTM!
619-623: LGTM!
647-647: LGTM!
702-709: LGTM!tests/unit/specfact_code_review/tools/test_ai_bloat_runner.py (5)
33-34: LGTM!
77-81: LGTM!
94-99: LGTM!
205-209: LGTM!
212-231: LGTM!packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py (2)
42-87: LGTM!
254-254: LGTM!tests/unit/specfact_code_review/tools/test_semgrep_runner.py (5)
13-21: LGTM!
166-170: LGTM!
271-277: LGTM!
287-308: LGTM!
345-398: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/proposal.md (1)
1-47: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/design.md (1)
1-71: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/specs/guided-simplification-review/spec.md (1)
1-64: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/specs/review-finding-model/spec.md (1)
1-19: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/specs/review-run-command/spec.md (1)
1-26: LGTM!openspec/CHANGE_ORDER.md (1)
105-105: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/.openspec.yaml (1)
1-2: LGTM!tests/unit/specfact_code_review/run/test_findings.py (3)
42-51: LGTM!
118-184: LGTM!
310-371: LGTM!packages/specfact-project/resources/prompts/specfact.08-simplify.md (1)
21-37: LGTM!Also applies to: 59-62, 67-73, 78-85, 87-93, 102-103
packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md (1)
9-9: LGTM!Also applies to: 14-17
packages/specfact-code-review/src/specfact_code_review/rules/updater.py (1)
37-44: LGTM!.vibe/skills/specfact-code-review/SKILL.md (1)
9-9: LGTM!Also applies to: 13-17
skills/specfact-code-review/SKILL.md (1)
9-9: LGTM!Also applies to: 13-17
tests/unit/specfact_code_review/rules/test_updater.py (1)
140-141: LGTM!tests/unit/test_guided_simplify_resources.py (1)
1-46: LGTM!tests/unit/specfact_code_review/run/test_runner.py (1)
233-294: LGTM!tests/unit/specfact_code_review/run/test_commands.py (1)
19-25: LGTM!Also applies to: 38-69, 315-503
packages/specfact-code-review/src/specfact_code_review/run/runner.py (1)
103-106: LGTM!Also applies to: 654-660
registry/index.json (2)
5-7: LGTM!
81-83: LGTM!registry/modules/specfact-code-review-0.47.23.tar.gz.sha256 (1)
1-1: LGTM!registry/modules/specfact-project-0.41.15.tar.gz.sha256 (1)
1-1: LGTM!registry/signatures/specfact-code-review-0.47.23.tar.sig (1)
1-1: LGTM!registry/signatures/specfact-project-0.41.15.tar.sig (1)
1-1: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md (2)
1-72: LGTM!
73-75: 🏗️ Heavy liftConfirm signatures are cryptographically generated (or block merge/publish).
openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md(lines 73-75) sayshatch run sign-modules ...initially failed without a private signing key and was rerun with--allow-unsigned, while the PR already contains signature artifacts. If--allow-unsignedresulted in placeholder/invalid signatures (checksum-only), this is a release-integrity blocker—merge and publication must be blocked until signatures are produced with the real signing secret. If signing was completed cryptographically, update the TDD evidence to reflect that the final signatures were generated post-rerun.packages/specfact-project/module-package.yaml (1)
2-2: 🏗️ Heavy liftMajor: Validate specfact-project 0.41.15 release signature/integrity and confirm prompt contract semver impact
File:
packages/specfact-project/module-package.yaml(line 2) setsversion: 0.41.15.
- Cryptographically verify that this manifest’s
integrity/signaturefor0.41.15matches the exactspecfact-project-0.41.15.tarartifact (byte-for-byte); release integrity must not rely on any “allow unsigned” bypass.- Prompt/behavior contract changes (
walkthroughlevels,guidance_kind,action_status) are user-facing; confirm they are strictly additive for a patch-level semver bump, or bump level if any output/behavior contract changes.- Check downstream consumers for references to the updated prompt structure (
specfact.08-simplify,walkthrough.*level,guidance_kind) to prevent runtime regressions.openspec/changes/code-review-12-guided-simplification-enforcement/tasks.md (2)
50-50: 🏗️ Heavy liftRelease integrity: reconcile “signing deferred” vs “ci sign changed modules”
- The tasks doc states signing is deferred because the cryptographic key wasn’t available locally, but the PR includes “chore(modules): ci sign changed modules.”
- Before promoting, confirm the published registry artifacts for
specfact-code-review-0.47.23andspecfact-project-0.41.15have valid, non-placeholder signature files (registry/signatures/*.tar.sig) and that the module/package metadata (packages/**/module-package.yaml) points to the expected signature/verification information; block merge if signatures are missing/invalid.
6-6: ⚡ Quick winEnsure CHANGE_ORDER order 05’s “blocked by
#276” dependency is correct for dev→main.For the dev→main promotion, double-check that issue
#276is the right/current tracker and that its open/closed status matches the intended readiness of order 05. If#276is already resolved, update the entry to “ready/unblocked” (and fix the reference/link if it changed).
…-safe Fix code review dead branch safety
chore(registry): publish changed modules
|
@codex review this PR |
|
@coderabbitai review this PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the key implementation files before writing the review. 🏁 Script executed: Repository: nold-ai/specfact-cli-modules Let me check the registry more carefully before finalizing my review. 🏁 Script executed: Repository: nold-ai/specfact-cli-modules
PR
|
| Source | Hash |
|---|---|
packages/specfact-code-review/module-package.yaml integrity.checksum |
32bdb881f64482da... |
registry/modules/specfact-code-review-0.47.24.tar.gz.sha256 |
2ada51a507e42475... |
If both are expected to be the SHA-256 of the same published tarball, this is a release integrity mismatch — a downloader verifying against the registry sha256 file would get a different result than the CLI verifying against module-package.yaml. Please confirm these are intentionally checksumming different artifacts (e.g., the tarball vs. the package contents directory), and if so, add a comment in the YAML clarifying what integrity.checksum covers.
🟡 Medium — Orphaned intermediate v0.47.23 registry artifacts
The PR's multiple CI sign/publish rounds left registry/modules/specfact-code-review-0.47.23.tar.gz.sha256 and registry/signatures/specfact-code-review-0.47.23.tar.sig committed alongside the canonical v0.47.24 artifacts. The final published version is 0.47.24 — the 0.47.23 files represent a superseded intermediate bump.
While non-blocking (the registry index correctly points to 0.47.24 and both 0.47.24 files are present), these stale entries could mislead audits or any tooling that walks registry/modules/. Consider removing them before merging, or confirm they're intentionally retained as historical registry entries.
🟡 Medium — _apply_simplification_fixes return value is silently discarded; action_status="applied" never written to the re-run report
In commands.py at both call sites (lines ~640 and ~665):
_apply_simplification_fixes(report) # int return discardedThe function signature is -> int (applied-fix count), but the result is dropped. More importantly, since the design applies fixes then re-runs a fresh review, the final report never has action_status="applied" set for findings that were actually rewritten. The design.md / proposal specify "record recommendation vs. actual change, remaining items, and improvement evidence." The current flow records nothing in the output report about what was mechanically applied.
This isn't a regression (the feature is new), but it means the before_ref/after_ref/improvement/action_status fields on ReviewFinding are populated by the LLM-prompt path only, not the autofix path. If the autofix path is expected to populate them too, that work is missing.
🔵 Low — Redundant guard in _derive_governance_fields
In findings.py:
if self.simplification_summary is not None or any(
finding.has_guided_simplification_metadata() for finding in self.findings
):
self.schema_version = "1.2"_build_simplification_summary is called on the line immediately above and only returns non-None when guided metadata exists. The any(has_guided_simplification_metadata()) branch therefore can never trigger independently of simplification_summary is not None. This is dead logic — safe to remove to reduce cognitive load.
✅ What looks good
findings.pyvalidation:_validate_guided_metadatacorrectly enforcespreserve_reasonwhenguidance_kind == "preserve", and rejects guided fields without aguidance_kind. The cross-field invariants are sound.runner.pyenforce mode: Themodel_copy(update={...})approach correctly bypasses the model validator re-run (Pydantic v2 behaviour), so the post-hocFAILoverride works without double-firing_derive_governance_fields.specfact-projectrelease: registry index, module-package.yaml, sha256, and sig all consistently point to v0.41.15. ✓- Cross-repo compatibility (
nold-ai/specfact-cli): the linked repo's>=0.41.0constraint forspecfact-projectis satisfied;schema_version="1.2"is additive and backward-compatible per the design doc. ✓ is_safe_mechanical_simplification: correctly treatsaction_status in {None, "recommended", "failed"}as blocking (open or failed work), while"applied"/"kept"/"skipped"are non-blocking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f323a2bae7
ℹ️ 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".
Fix PR 289 code review findings
chore(registry): publish changed modules
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/specfact-code-review/src/specfact_code_review/review/commands.py (1)
36-37: ⚡ Quick winMake the base-ref example branch-agnostic in printed instructions.
Line 36 hard-codes
origin/dev...HEAD. That guidance can be wrong for repos/PRs based onmain(or another base), producing misleading file discovery. Prefer a<base-ref>placeholder (or explicitly stateorigin/mainas the current default example) and keep docs/tests in sync.Suggested edit
- If the worktree is clean on a PR branch and --scope changed finds no files, review the branch-delta Python files as explicit positional files and omit --scope. Find them with the PR base ref, for example: git diff --name-only origin/dev...HEAD -- '*.py' '*.pyi' + If the worktree is clean on a PR branch and --scope changed finds no files, review the branch-delta Python files as explicit positional files and omit --scope. Find them with the PR base ref, for example: git diff --name-only origin/<base-ref>...HEAD -- '*.py' '*.pyi'🤖 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/review/commands.py` around lines 36 - 37, Update the printed guidance that currently hard-codes "origin/dev...HEAD" to use a branch-agnostic placeholder or default, i.e., replace the literal "origin/dev...HEAD" in the help/usage string with "<base-ref>...HEAD" (or "origin/main...HEAD" as the explicit default) inside the code that emits that guidance (search for the string "origin/dev...HEAD" in packages/specfact-code-review/src/specfact_code_review/review/commands.py), and update any tests or docs that assert the old string so they expect the new placeholder/default text.
🤖 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.
Inline comments:
In
`@openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md`:
- Line 109: The signing evidence in TDD_EVIDENCE.md is inconsistent: it claims
specfact-code-review was refreshed to 0.47.25 while the PR ships
registry/modules/specfact-code-review-0.47.26.tar.gz.sha256; update the
TDD_EVIDENCE.md entry to reflect the final shipped version (change the 0.47.25
reference to 0.47.26) or explicitly annotate 0.47.25 as an intermediate step and
document the final signing step that produced 0.47.26 (referencing the hatch
commands used: `hatch run verify-modules-signature ...` and `hatch run
sign-modules ...`) so the recorded evidence unambiguously matches the shipped
artifact.
In `@packages/specfact-code-review/module-package.yaml`:
- Around line 26-27: The manifest's checksum/signature (checksum:
sha256:87a17f... and signature: uGnmR...) does not match the published registry
digest (registry/modules/specfact-code-review-0.47.26.tar.gz.sha256 ->
bd2c228...), so regenerate the canonical tarball for specfact-code-review
v0.47.26, compute the correct SHA256 checksum and new signature from that single
canonical artifact, and atomically update the checksum and signature fields in
module-package.yaml to the regenerated values while also updating the registry
digest file to match; ensure metadata parity validation is performed
(packages/**/module-package.yaml vs packaged source) and document the signing
implications when you change manifest or payload.
---
Nitpick comments:
In `@packages/specfact-code-review/src/specfact_code_review/review/commands.py`:
- Around line 36-37: Update the printed guidance that currently hard-codes
"origin/dev...HEAD" to use a branch-agnostic placeholder or default, i.e.,
replace the literal "origin/dev...HEAD" in the help/usage string with
"<base-ref>...HEAD" (or "origin/main...HEAD" as the explicit default) inside the
code that emits that guidance (search for the string "origin/dev...HEAD" in
packages/specfact-code-review/src/specfact_code_review/review/commands.py), and
update any tests or docs that assert the old string so they expect the new
placeholder/default text.
🪄 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: CHILL
Plan: Pro
Run ID: 22127567-0774-434c-8e5c-20fcc7728787
⛔ Files ignored due to path filters (3)
registry/modules/specfact-code-review-0.47.24.tar.gzis excluded by!**/*.gzregistry/modules/specfact-code-review-0.47.26.tar.gzis excluded by!**/*.gzregistry/modules/specfact-project-0.41.17.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (30)
.vibe/skills/specfact-code-review/SKILL.mdREADME.mddocs/bundles/code-review/run.mddocs/modules/code-review.mdopenspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.mdopenspec/changes/code-review-12-guided-simplification-enforcement/design.mdopenspec/changes/code-review-12-guided-simplification-enforcement/specs/review-finding-model/spec.mdpackages/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/commands.pypackages/specfact-code-review/src/specfact_code_review/run/findings.pypackages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.pypackages/specfact-project/module-package.yamlpackages/specfact-project/resources/prompts/specfact.08-simplify.mdregistry/index.jsonregistry/modules/specfact-code-review-0.47.24.tar.gz.sha256registry/modules/specfact-code-review-0.47.26.tar.gz.sha256registry/modules/specfact-project-0.41.17.tar.gz.sha256registry/signatures/specfact-code-review-0.47.24.tar.sigregistry/signatures/specfact-code-review-0.47.26.tar.sigregistry/signatures/specfact-project-0.41.17.tar.sigskills/specfact-code-review/SKILL.mdtests/unit/specfact_code_review/review/test_commands.pytests/unit/specfact_code_review/run/test_commands.pytests/unit/specfact_code_review/run/test_findings.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/test_guided_simplify_resources.py
✅ Files skipped from review due to trivial changes (9)
- registry/signatures/specfact-code-review-0.47.26.tar.sig
- registry/signatures/specfact-project-0.41.17.tar.sig
- registry/modules/specfact-project-0.41.17.tar.gz.sha256
- README.md
- registry/modules/specfact-code-review-0.47.24.tar.gz.sha256
- docs/bundles/code-review/run.md
- registry/signatures/specfact-code-review-0.47.24.tar.sig
- registry/index.json
- openspec/changes/code-review-12-guided-simplification-enforcement/specs/review-finding-model/spec.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
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-project/module-package.yamlpackages/specfact-code-review/module-package.yaml
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: User-facing and cross-site accuracy: Jekyll front matter, links per documentation-url-contract,
CLI examples matching bundled commands.
Files:
docs/modules/code-review.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.26.tar.gz.sha256
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
packages/specfact-code-review/src/specfact_code_review/review/commands.pytests/unit/specfact_code_review/review/test_commands.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/test_guided_simplify_resources.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pypackages/specfact-code-review/src/specfact_code_review/run/findings.pytests/unit/specfact_code_review/run/test_findings.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/run/test_commands.pypackages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_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/review/commands.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/tools/ai_bloat_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/review/test_commands.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/test_guided_simplify_resources.pytests/unit/specfact_code_review/run/test_findings.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/run/test_commands.py
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/code-review-12-guided-simplification-enforcement/design.mdopenspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md
🪛 LanguageTool
packages/specfact-code-review/src/specfact_code_review/resources/skills/specfact-code-review/SKILL.md
[style] ~18-~18: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...lify review after each accepted file or very small batch; if validation cannot prove safet...
(EN_WEAK_ADJECTIVE)
packages/specfact-project/resources/prompts/specfact.08-simplify.md
[style] ~99-~99: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...pted edits. After each accepted file or very small batch, run the most targeted relevant t...
(EN_WEAK_ADJECTIVE)
.vibe/skills/specfact-code-review/SKILL.md
[style] ~23-~23: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...lify review after each accepted file or very small batch; if validation cannot prove safet...
(EN_WEAK_ADJECTIVE)
openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md
[style] ~43-~43: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...atches_ai_bloat_rule_categories -q` - Result after final PR review fixes: 9 passed. ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~45-~45: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ew/tools/test_semgrep_runner.py -q` - Result after final PR review fixes: 172 passed...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~47-~47: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...es_keeps_impure_duplicate_guard -q - Result after complexity cleanup: 3 passed. -...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~49-~49: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..._ignores_impure_duplicate_guard -q` - Result after final detector tightening: 5 pass...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~51-~51: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mplification_fixes_before_rerun -q` - Result after PR 289 dev-branch review fixes: 3...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~53-~53: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...st_guided_simplify_resources.py -q` - Result after prompt/skill user-experience tigh...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hatch run validate-prompt-commands` - Result after prompt/skill user-experience tigh...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~57-~57: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ..._valid_structure_when_available -q` - Result after adding the AI instructions fallba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...act code review run --instructions` - Result after adding the AI instructions fallba...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~69-~69: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rnings, 0 notes. - hatch run lint - Result after AI instructions fallback: 10.00/1...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~71-~71: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...: 10.00/10. - hatch run yaml-lint - Result after AI instructions fallback: validat...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~77-~77: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mp --version-check-base origin/dev` - Result after AI instructions fallback: verifie...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~79-~79: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t/code-review.json --scope changed` - Result after final PR review fixes: PASS, CI e...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~81-~81: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t/code-review.json --scope changed` - Result after AI instructions fallback: PASS, C...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~83-~83: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...implification-enforcement --strict` - Result after AI instructions fallback: valid. ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
skills/specfact-code-review/SKILL.md
[style] ~23-~23: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...lify review after each accepted file or very small batch; if validation cannot prove safet...
(EN_WEAK_ADJECTIVE)
🔀 Multi-repo context nold-ai/specfact-cli
[::nold-ai/specfact-cli::]
-
Tests and code reference the specfact-code-review bundle extensively (install, profiles, registry, module installer, module package handling, and CLI contract tests):
- tests/unit/specfact_cli/registry/test_profile_presets.py — expects "specfact-code-review" in canonical bundles and installs.
- tests/unit/registry/test_module_installer.py — builds module dir "specfact-code-review" and archive entries.
- tests/unit/scripts/test_check_license_compliance.py — looks for packages/specfact-code-review in license checks.
- tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py — installs/uninstalls nold-ai/specfact-code-review.
- tests/unit/specfact_cli/registry/test_versioned_bundle_deps.py — refers to specfact-code-review dependency entries.
-
Schema/versioning consumers and validation:
- Many places read/compare bundle schema_version (src/specfact_cli/migrations/plan_migrator.py, src/specfact_cli/registry/module_packages.py, src/specfact_cli/models/module_package.py). Module packages and registry index entries are validated against ProjectBundle schema_version expectations. Changes that bump ReviewReport.schema_version to "1.2" or add guided simplification fields will interact with these validation paths.
-
OpenSpec and docs reference the code-review module and simplify flow:
- openspec/specs/* contain requirements expecting skills/specfact-code-review/SKILL.md, specfact.08-simplify prompt, and review-report-model presence.
- skills/specfact-code-review/SKILL.md exists in this repo (skills/specfact-code-review/SKILL.md) and will be read by users/tools; updated skill text in the PR must remain compatible with that path/expectation.
-
CLI/docs/tooling expectations:
- docs/modules/code-review.md, docs/modules/code-review.md and README mention .specfact/code-review.json and the /specfact.08-simplify flow; the new CLI --instructions option and simplification JSON path must match these docs or tests that parse them.
Implication: the specfact-cli repo consumes the specfact-code-review bundle via installation, docs, tests, and schema/version checks. The guided-simplification schema/report changes (schema_version "1.2", new guided fields, simplification_summary, CLI behavior) are relevant cross-repo concerns and should be validated against specfact-cli's module-installation, registry, and schema-validation code and tests. Specific areas to verify:
- Registry/module-package checksum and manifest version updates align with specfact-cli's registry/index.json expectations and module installation tests.
- Schema/version bump handling in src/specfact_cli/registry/module_packages.py and module-package metadata validation (ensure compatibility logic allows new schema_version values or module metadata includes schema_version where required).
- Skills/prompt file paths and expected content strings (skills/specfact-code-review/SKILL.md and specfact.08-simplify) — tests in this repo assert presence of those strings.
🔇 Additional comments (12)
tests/unit/specfact_code_review/tools/test_ai_bloat_runner.py (1)
70-86: LGTM!Also applies to: 88-103, 105-121, 123-138, 140-154
tests/unit/test_guided_simplify_resources.py (1)
18-22: LGTM!Also applies to: 27-48, 55-76
openspec/changes/code-review-12-guided-simplification-enforcement/design.md (1)
38-38: LGTM!packages/specfact-project/resources/prompts/specfact.08-simplify.md (1)
27-27: LGTM!Also applies to: 31-31, 38-38, 46-46, 50-50, 72-72, 74-74, 77-87, 94-94, 99-99
packages/specfact-code-review/src/specfact_code_review/run/commands.py (1)
19-19: LGTM!Also applies to: 283-299, 301-313, 315-322, 339-340, 344-378, 495-511, 664-674, 688-716
packages/specfact-code-review/src/specfact_code_review/run/findings.py (1)
325-328: LGTM!Also applies to: 368-368
tests/unit/specfact_code_review/run/test_findings.py (1)
141-161: LGTM!Also applies to: 395-422
tests/unit/specfact_code_review/run/test_runner.py (1)
65-65: LGTM!tests/unit/specfact_code_review/run/test_commands.py (1)
326-327: LGTM!Also applies to: 339-340, 354-355, 374-377, 398-400, 402-440, 458-460, 490-499, 511-513, 542-547
packages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.py (1)
195-255: LGTM!.vibe/skills/specfact-code-review/SKILL.md (1)
3-4: LGTM!Also applies to: 7-12, 15-24, 38-41
skills/specfact-code-review/SKILL.md (1)
3-4: LGTM!Also applies to: 7-12, 15-24, 38-41
fix(code-review): stabilize PR 289 guidance checks
chore(registry): publish changed modules
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md (1)
127-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSigning-note state drifts from shipped 0.47.27 artifacts
Line 127 and Line 129 describe signature restoration/publish as pending, but this PR already carries
registry/signatures/specfact-code-review-0.47.27.tar.sigand indexes0.47.27inregistry/index.json. Please update the note to reflect final shipped state to avoid release-audit ambiguity.Suggested doc correction
-This PR `#289` follow-up changes the `specfact-code-review` source payload again, so the local manifest is refreshed to `0.47.27` with `hatch run sign-modules --changed-only --base-ref origin/dev --bump-version patch --allow-unsigned --payload-from-filesystem`. CI must restore the cryptographic signature with the repository private key before the follow-up lands on `main`. +This PR `#289` follow-up changed the `specfact-code-review` source payload and refreshed the manifest to `0.47.27`. The publish/sign follow-up produced the shipped `0.47.27` signature and registry metadata. -The `packages/specfact-code-review/module-package.yaml` `integrity.checksum` covers the canonical module source payload, while `registry/modules/specfact-code-review-0.47.26.tar.gz.sha256` covers the published tarball artifact. These digests are intentionally different; the registry sidecar matches the `0.47.26` tarball SHA256, and the manifest signature verifier validates the source-payload checksum/signature. The next publish step will produce the corresponding `0.47.27` registry artifact after signing. +The `packages/specfact-code-review/module-package.yaml` `integrity.checksum` covers the canonical module source payload, while `registry/modules/specfact-code-review-0.47.27.tar.gz.sha256` covers the published tarball artifact. These digests are intentionally different; the registry sidecar matches the `0.47.27` tarball SHA256, and the manifest signature verifier validates the source-payload checksum/signature.As per coding guidelines,
openspec/**/*.md: "Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and drift vs. shipped modules or docs."🤖 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/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md` around lines 127 - 129, Update the signing-note paragraph to reflect that the 0.47.27 artifacts are already shipped: replace the current text that says CI must restore the signature and publish is pending with a statement that registry/signatures/specfact-code-review-0.47.27.tar.sig is present and registry/index.json already indexes 0.47.27, and clarify that packages/specfact-code-review/module-package.yaml integrity.checksum and registry/modules/specfact-code-review-0.47.26.tar.gz.sha256 intentionally differ (source vs published tarball) while the next publish step produced the matching 0.47.27 registry artifact; ensure the note no longer implies pending signature restoration or publish.
🤖 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
`@openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md`:
- Around line 127-129: Update the signing-note paragraph to reflect that the
0.47.27 artifacts are already shipped: replace the current text that says CI
must restore the signature and publish is pending with a statement that
registry/signatures/specfact-code-review-0.47.27.tar.sig is present and
registry/index.json already indexes 0.47.27, and clarify that
packages/specfact-code-review/module-package.yaml integrity.checksum and
registry/modules/specfact-code-review-0.47.26.tar.gz.sha256 intentionally differ
(source vs published tarball) while the next publish step produced the matching
0.47.27 registry artifact; ensure the note no longer implies pending signature
restoration or publish.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e53ad34d-a461-4084-9eb1-4d27ddd1345d
⛔ Files ignored due to path filters (1)
registry/modules/specfact-code-review-0.47.27.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (9)
docs/bundles/code-review/run.mddocs/modules/code-review.mdopenspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.mdpackages/specfact-code-review/module-package.yamlpackages/specfact-code-review/src/specfact_code_review/review/commands.pyregistry/index.jsonregistry/modules/specfact-code-review-0.47.27.tar.gz.sha256registry/signatures/specfact-code-review-0.47.27.tar.sigtests/unit/specfact_code_review/review/test_commands.py
✅ Files skipped from review due to trivial changes (3)
- registry/modules/specfact-code-review-0.47.27.tar.gz.sha256
- docs/bundles/code-review/run.md
- docs/modules/code-review.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
🧰 Additional context used
📓 Path-based instructions (6)
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
registry/**
⚙️ CodeRabbit configuration file
registry/**: Registry and index consistency: bundle listings, version pins, and compatibility with
published module artifacts.
Files:
registry/index.jsonregistry/signatures/specfact-code-review-0.47.27.tar.sig
**/*.{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/review/test_commands.pypackages/specfact-code-review/src/specfact_code_review/review/commands.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/specfact_code_review/review/test_commands.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators),
Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles.
Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior.
Files:
packages/specfact-code-review/src/specfact_code_review/review/commands.py
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md
🧠 Learnings (1)
📚 Learning: 2026-04-02T21:49:07.435Z
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:07.435Z
Learning: In nold-ai/specfact-cli-modules, module tarball signature files under registry/signatures/*.tar.sig are produced by the publish-modules GitHub Actions runner during the publish workflow (not committed to the branch). During PR pre-merge review, do not flag missing *.tar.sig files as blockers; treat signatures as publish-time artifacts.
Applied to files:
registry/signatures/specfact-code-review-0.47.27.tar.sig
🪛 LanguageTool
openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md
[style] ~85-~85: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...st_guided_simplify_resources.py -q - Result after PR#289` follow-up fixes: 5 passed...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~87-~87: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mp --version-check-base origin/dev - Result after PR#289` follow-up fixes: verified...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~89-~89: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... manifests. - hatch run yaml-lint - Result after PR #289 follow-up fixes: validate...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~91-~91: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...implification-enforcement --strict - Result after PR#289` follow-up fixes: valid. -...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~93-~93: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... valid. - hatch run contract-test - Result after PR #289 follow-up fixes: 773 pass...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~95-~95: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... warnings. - hatch run type-check - Result after PR #289 follow-up fixes: 0 errors...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~97-~97: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rnings, 0 notes. - hatch run lint - Result after PR #289 follow-up fixes: formatte...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~99-~99: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...t/code-review.json --scope changed - Result after PR#289` follow-up fixes: PASS, CI...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
Repository: nold-ai/specfact-cli
-
Module install / usage expectations:
- tests/unit/specfact_cli/registry/test_profile_presets.py — several assertions require "specfact-code-review" be included in profile presets and CANONICAL_BUNDLES. [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_profile_presets.py]
- tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py — tests install/uninstall calls reference "nold-ai/specfact-code-review". [::nold-ai/specfact-cli::tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py]
- scripts/runtime_discovery_smoke.py — MODULE_IDS includes "nold-ai/specfact-code-review". [::nold-ai/specfact-cli::scripts/runtime_discovery_smoke.py]
- docs/modules/code-review.md and skills/specfact-code-review/SKILL.md are referenced in multiple specs and docs. [::nold-ai/specfact-cli::docs/modules/code-review.md] [::nold-ai/specfact-cli::skills/specfact-code-review/SKILL.md]
-
Module package / registry & schema handling:
- src/specfact_cli/registry/module_packages.py contains schema/version compatibility checks and logic that validates module metadata schema_version against CURRENT_PROJECT_SCHEMA_VERSION. Changes that bump bundle schema_version (e.g., ReviewReport schema_version -> "1.2") could interact with this logic. [::nold-ai/specfact-cli::src/specfact_cli/registry/module_packages.py:389,1537-1554]
- src/specfact_cli/models/module_package.py defines schema_version field on module package metadata. [::nold-ai/specfact-cli::src/specfact_cli/models/module_package.py]
- Multiple tests and docs assert expectations about bundle/schema versions (tests/unit/models/test_project_bundle_schema.py, docs/reference/schema-versioning.md, etc.), so consumers verify schema_version values. [::nold-ai/specfact-cli::tests/unit/models/test_project_bundle_schema.py] [::nold-ai/specfact-cli::docs/reference/schema-versioning.md]
-
Review/report consumers and expectations:
- openspec/specs/review-report-model/spec.md and docs/modules/code-review.md reference ReviewReport schema and fields; tests and code read/compare manifest.schema_version values in integration/e2e tests. If ReviewReport.schema_version is set to "1.2" by specfact-code-review, specfact-cli code that checks compatibility may be exercised. [::nold-ai/specfact-cli::openspec/specs/review-report-model/spec.md] [::nold-ai/specfact-cli::docs/modules/code-review.md]
- src/specfact_cli/models/project.py and other code check bundle manifest versions and schema_version semantics. [::nold-ai/specfact-cli::src/specfact_cli/models/project.py]
-
Skill / prompt file placement and tests:
- tests and specs expect skills/specfact-code-review/SKILL.md path and specific strings; the PR updates SKILL.md in the modules repo and specfact-cli contains the canonical path and tests referencing it. Ensure updated skill content location/format matches expectations. [::nold-ai/specfact-cli::openspec/specs/house-rules-skill/spec.md] [::nold-ai/specfact-cli::skills/specfact-code-review/SKILL.md]
-
Registry/index.json not present in this repo:
- attempted sed on registry/index.json failed (file not found). The modules repo publishes registry/index.json; specfact-cli expects registry index formats in tests, but this repo does not contain the registry index file. [::nold-ai/specfact-cli::] (stderr)
Conclusion / impact for PR under review:
- The PR's schema/report changes (ReviewReport schema_version bump to "1.2" and new guided-simplification fields) will interact with specfact-cli's module package/registry schema compatibility logic in src/specfact_cli/registry/module_packages.py and related tests that assert schema_version handling. Verify module-package metadata and registry index entries published by the modules repo declare compatible schema_version or that specfact-cli's compatibility checks accept the new schema version.
- Ensure skill/prompt files remain at expected repository-relative paths and include required strings, since specfact-cli tests and openspec specs assert their presence.
🔇 Additional comments (6)
packages/specfact-code-review/module-package.yaml (1)
2-2: LGTM!Also applies to: 26-27
registry/index.json (1)
81-83: LGTM!registry/signatures/specfact-code-review-0.47.27.tar.sig (1)
1-1: LGTM!tests/unit/specfact_code_review/review/test_commands.py (1)
22-26: LGTM!Also applies to: 42-42
packages/specfact-code-review/src/specfact_code_review/review/commands.py (1)
36-36: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md (1)
84-99: LGTM!
1 participant
Summary
Validation