Implement code review simplification feedback loop#278
Merged
Conversation
|
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. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughBundle and Module Surfacespecfact-code-review v0.47.18 (from 0.47.17):
specfact-project v0.41.12 (from 0.41.11):
Review Model & Report MetadataReviewFinding dataclass extended with optional simplification metadata:
ReviewReport schema version now Simplification findings remain score-neutral and non-blocking; Deterministic Simplification Analyzersai_bloat_runner.py (+345 lines):
ast_clean_code_runner.py (+51 lines):
Manifest & Integrityspecfact-code-review/module-package.yaml:
specfact-project/module-package.yaml:
Documentation & Contractdocs/bundles/code-review/:
docs/bundles/project/overview.md:
docs/modules/code-review.md:
packages/specfact-code-review/resources/skills/specfact-code-review/SKILL.md:
Cross-Repo & CLI Integration
OpenSpec Change Coveragecode-review-11-simplification-feedback-loop (all 7 checklist sections completed):
Skill & Prompt Verification
WalkthroughAdds a simplify-focused review flow: new simplification metadata on findings, ChangesSimplification feedback loop
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
djm81
added
enhancement
5 tasks
![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: fa5399f7b9
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/specfact-code-review/src/specfact_code_review/review/commands.py (1)
102-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
--focushelp text to includesimplify.Line 102 still advertises only
source/tests/docs, but the resolver acceptssimplify. This creates CLI contract drift and hurts discoverability.Suggested fix
- focus: list[str] | None = typer.Option(None, "--focus", help="Limit to source, tests, and/or docs (repeatable)."), + focus: list[str] | None = typer.Option( + None, + "--focus", + help="Limit to source, tests, docs, and/or simplify (repeatable).", + ),🤖 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` at line 102, The CLI option declaration for focus (the variable named focus using typer.Option in the command) advertises only "source, tests, and/or docs" while the resolver also accepts "simplify"; update the help text on the focus typer.Option (the focus: list[str] | None = typer.Option(...)) to include "simplify" so the CLI help matches supported values and remains discoverable.
🤖 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 `@docs/bundles/code-review/overview.md`:
- Around line 28-29: The intro paragraph currently uses the phrase “house-rules
skill management” which conflicts with the table entry that references the
`rules` command managing the bundled code-review skill; update the opening
paragraph to use the same terminology as the table (e.g., “bundled code-review
skill management” or explicitly mention the `rules` command) so the page
consistently refers to the code-review skill and `rules` throughout the
document.
In `@docs/bundles/code-review/run.md`:
- Around line 125-132: Update the docs to consistently document the new simplify
focus: add "simplify" to the key-options table and to the "--focus" facet
section so the command contract matches the example using "--focus simplify";
ensure the description explains that "simplify" produces simplification-focused
reports (retaining ai_bloat plus high-confidence dry and kiss findings with
deterministic simplification metadata such as rewrite_hint, canonical_pattern,
intent_key, estimated_deletion_lines, and related_locations) and mark it as
score-neutral/non-blocking just like the example; verify any lists or validation
text that previously limited --focus to "source/tests/docs" now include
"simplify" and update any related explanatory text to match.
In `@packages/specfact-code-review/src/specfact_code_review/run/runner.py`:
- Around line 312-317: The current _belongs_to_simplification_queue allows
dry/kiss findings with any metadata present; change it to require deterministic
simplification metadata by updating the condition for these categories to
include a deterministic check (e.g., replace the existing
has_simplification_metadata() call with something like
finding.has_simplification_metadata() and
finding.simplification_metadata_is_deterministic(), or check for presence of
explicit deterministic fields such as a concrete "replacement"/"patch" value),
and if that helper doesn't exist add a method on ReviewFinding (e.g.,
simplification_metadata_is_deterministic()) to encapsulate the
deterministic-field checks used by _belongs_to_simplification_queue.
- Around line 509-525: The _review_options_from_kwargs function currently
silently casts arbitrary entries from overrides into ReviewOptions; change it to
validate keys and value types and raise TypeError for unknown keys or invalid
types/values. Implement an explicit allowed_keys set for overrides and check
overrides.keys() ⊆ allowed_keys, validate booleans for
no_tests/include_noise/bug_hunt (using isinstance(..., bool)), ensure
progress_callback is callable or None, ensure review_level is one of
{"error","warning",None} and review_mode is one of {"shadow","enforce"}, and
validate focus against ReviewFocus or None; if any check fails, raise TypeError
explaining the offending key. Keep the constructor return logic using validated
values to build and return ReviewOptions.
In `@packages/specfact-project/resources/prompts/specfact.08-simplify.md`:
- Around line 83-85: The two specfact commands both write to the same artifact
`.specfact/code-review.json` and the second (`specfact code review run --scope
changed --bug-hunt --json --out .specfact/code-review.json`) overwrites the
first; update the outputs so they are distinct (for example change the first to
`--out .specfact/code-review-simplify.json` and the second to `--out
.specfact/code-review-bughunt.json`) by editing the two command lines in
specfact.08-simplify.md so the simplify snapshot is not lost.
- Line 50: The prompt currently instructs to stop when simplification metadata
(intent_key, rewrite_hint, canonical_pattern) is present, which is inverted;
change the condition so the tool only stops when there are no findings with
category == "ai_bloat" AND no simplification metadata present. Update the
sentence around the phrase 'category == "ai_bloat"' to read that you should
report "no simplification candidates and stop" only if neither ai_bloat findings
nor any of the metadata fields (intent_key, rewrite_hint, canonical_pattern) are
present, thereby allowing metadata-backed findings to be processed.
---
Outside diff comments:
In `@packages/specfact-code-review/src/specfact_code_review/review/commands.py`:
- Line 102: The CLI option declaration for focus (the variable named focus using
typer.Option in the command) advertises only "source, tests, and/or docs" while
the resolver also accepts "simplify"; update the help text on the focus
typer.Option (the focus: list[str] | None = typer.Option(...)) to include
"simplify" so the CLI help matches supported values and remains discoverable.
🪄 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: b220f582-3e32-4410-9e45-329638215c5f
📒 Files selected for processing (29)
docs/bundles/code-review/overview.mddocs/bundles/code-review/rules.mddocs/bundles/code-review/run.mddocs/bundles/project/overview.mddocs/modules/code-review.mdopenspec/changes/code-review-11-simplification-feedback-loop/TDD_EVIDENCE.mdopenspec/changes/code-review-11-simplification-feedback-loop/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/review/commands.pypackages/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/run/scorer.pypackages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.pypackages/specfact-project/module-package.yamlpackages/specfact-project/resources/prompts/specfact.03-review.mdpackages/specfact-project/resources/prompts/specfact.08-simplify.mdtests/unit/docs/test_code_review_docs_parity.pytests/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_ast_clean_code_runner.pytests/unit/test_bundle_resource_payloads.pytests/unit/test_check_prompt_commands_script.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: quality (3.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.yamlpackages/specfact-project/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/bundles/code-review/run.mddocs/bundles/project/overview.mddocs/modules/code-review.mddocs/bundles/code-review/rules.mddocs/bundles/code-review/overview.md
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/code-review-11-simplification-feedback-loop/TDD_EVIDENCE.mdopenspec/changes/code-review-11-simplification-feedback-loop/tasks.md
**/*.{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/run/scorer.pytests/unit/test_bundle_resource_payloads.pytests/unit/specfact_code_review/rules/test_updater.pytests/unit/specfact_code_review/tools/test_ast_clean_code_runner.pytests/unit/test_check_prompt_commands_script.pytests/unit/specfact_code_review/run/test_findings.pypackages/specfact-code-review/src/specfact_code_review/run/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/ast_clean_code_runner.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pypackages/specfact-code-review/src/specfact_code_review/rules/updater.pytests/unit/docs/test_code_review_docs_parity.pypackages/specfact-code-review/src/specfact_code_review/run/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/scorer.pypackages/specfact-code-review/src/specfact_code_review/run/findings.pypackages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pypackages/specfact-code-review/src/specfact_code_review/review/commands.pypackages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.pypackages/specfact-code-review/src/specfact_code_review/rules/updater.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/test_bundle_resource_payloads.pytests/unit/specfact_code_review/rules/test_updater.pytests/unit/specfact_code_review/tools/test_ast_clean_code_runner.pytests/unit/test_check_prompt_commands_script.pytests/unit/specfact_code_review/run/test_findings.pytests/unit/specfact_code_review/run/test_runner.pytests/unit/specfact_code_review/run/test_commands.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/docs/test_code_review_docs_parity.py
🔀 Multi-repo context nold-ai/specfact-cli
Findings for nold-ai/specfact-cli
-
Tests and runtime expect an installable specfact-code-review module and the bundled skill:
- skills/specfact-code-review/SKILL.md exists in this repo and is referenced by multiple tests/specs (e.g. skills/specfact-code-review/SKILL.md lines) — callers and specs expect the skill file to be present. [::nold-ai/specfact-cli::skills/specfact-code-review/SKILL.md]
- Many unit/integration tests reference "specfact-code-review" module ids, install flows, and module-package.yaml presence (examples: tests/unit/specfact_cli/registry/test_module_availability.py, tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py, tests/integration/test_bundle_install.py). These tests will consume any changes to the packaged module (version/integrity/manifest). [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_module_availability.py] [::nold-ai/specfact-cli::tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py] [::nold-ai/specfact-cli::tests/integration/test_bundle_install.py]
-
CI / verification tooling will exercise module manifest/signature changes:
- scripts/verify-modules-signature.py, scripts/sign-modules.py, scripts/publish-module.py, scripts/verify-bundle-published.py and related tests reference module-package.yaml processing and signature checks. Changes to module-package.yaml (version/integrity) in specfact-code-review must be consistent with these scripts/tests. [::nold-ai/specfact-cli::scripts/verify-modules-signature.py] [::nold-ai/specfact-cli::scripts/sign-modules.py]
-
Specifications and openspec references assume the run_review contract:
- openspec entries reference the specfact-code-review module and a run_review signature (e.g. def run_review(files: list[Path], options: ReviewOptions) in archived docs). If run_review signature changes upstream (ReviewOptions added), openspec/specs or any CLI-contract tests in this repo that validate review-run scenarios may need updating. [::nold-ai/specfact-cli::openspec/changes/archive/2026-03-17-code-review-04-contract-test-runners/design.md] [::nold-ai/specfact-cli::openspec/specs/code-review-module/spec.md]
-
IDE / prompt generation and skill installation paths:
- src/specfact_cli/utils/ide_setup.py builds prompt templates and references module-package.yaml/name-based grouping; changes to how the code-review module exposes SKILL.md or commands (e.g., new flags like --focus simplify) may affect generated IDE instructions or the text expected by tests that assert presence of certain CLI flags in prompts/skills. [::nold-ai/specfact-cli::src/specfact_cli/utils/ide_setup.py]
Summary / impact
- This repo contains tests, scripts, and specs that reference the specfact-code-review module, its SKILL.md, and module-package.yaml processing/signature verification. Upstream changes that alter module-package.yaml (version/integrity), exported CLI help text (adding flags like --focus), or public function signatures used by contract tests (run_review / ReviewOptions) are relevant here because CI scripts and tests will validate installations, signatures, and prompt/skill content.
- No direct code-level callers of the new ReviewFinding simplification fields or internal runner types were found in this repo; impact is primarily via module packaging, skill text, CLI help text, module manifest/signature, and openspec/contract test expectations.
🔇 Additional comments (20)
packages/specfact-code-review/module-package.yaml (1)
2-2: LGTM!Also applies to: 26-27
packages/specfact-project/module-package.yaml (1)
2-2: LGTM!Also applies to: 30-31
tests/unit/test_bundle_resource_payloads.py (1)
272-272: LGTM!tests/unit/test_check_prompt_commands_script.py (1)
14-15: LGTM!Also applies to: 243-243, 249-272
openspec/changes/code-review-11-simplification-feedback-loop/TDD_EVIDENCE.md (1)
1-56: LGTM!openspec/changes/code-review-11-simplification-feedback-loop/tasks.md (1)
3-8: LGTM!Also applies to: 12-18, 22-25, 29-33, 37-40, 44-46, 50-54
packages/specfact-project/resources/prompts/specfact.03-review.md (1)
27-40: LGTM!Also applies to: 731-745
packages/specfact-code-review/src/specfact_code_review/run/findings.py (1)
98-113: LGTM!Also applies to: 115-120, 122-136, 178-179
tests/unit/specfact_code_review/run/test_findings.py (1)
9-9: LGTM!Also applies to: 36-41, 66-89, 176-199
packages/specfact-code-review/src/specfact_code_review/review/commands.py (1)
29-37: LGTM!Also applies to: 64-75, 78-90, 116-127
tests/unit/docs/test_code_review_docs_parity.py (1)
13-13: LGTM!Also applies to: 66-74, 80-88
packages/specfact-code-review/src/specfact_code_review/run/commands.py (1)
19-19: LGTM!Also applies to: 69-69, 81-81, 90-92, 100-103, 105-105, 395-395, 415-415, 465-473, 538-538, 590-592, 608-617
tests/unit/specfact_code_review/run/test_commands.py (1)
205-243: LGTM!packages/specfact-code-review/src/specfact_code_review/run/runner.py (1)
59-73: LGTM!Also applies to: 227-253, 320-326, 528-605
tests/unit/specfact_code_review/run/test_runner.py (1)
57-77: LGTM!Also applies to: 182-216
packages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.py (1)
20-49: LGTM!Also applies to: 223-239, 268-305, 307-332, 334-421, 423-528, 538-545, 547-579, 600-600
tests/unit/specfact_code_review/tools/test_ai_bloat_runner.py (1)
5-6: LGTM!Also applies to: 91-194, 210-226
packages/specfact-code-review/src/specfact_code_review/tools/ast_clean_code_runner.py (1)
14-15: LGTM!Also applies to: 19-37, 125-138, 173-200
tests/unit/specfact_code_review/tools/test_ast_clean_code_runner.py (1)
56-107: LGTM!packages/specfact-code-review/src/specfact_code_review/run/scorer.py (1)
55-56: LGTM!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Notes: the latest changed-scope SpecFact review exits 0. Remaining findings are non-blocking Typer wrapper shape advisories and score-neutral ai-bloat LOC-vs-complexity hints, not the pasted review findings. Initial skill smoke without SPECFACT_MODULES_ROOTS exposed that the user-scoped installed module can mask a local checkout, so the recorded smoke pins the local module root explicitly.