[codex] Add guided simplification review enforcement#287
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. |
📝 WalkthroughOpenSpec Change & GovernanceChange ID: code-review-12-guided-simplification-enforcement (feature/code-review-12-guided-simplification-enforcement)
Module Package & Integrityspecfact-code-review: 0.47.20 → 0.47.21
specfact-project: 0.41.12 → 0.41.13
PublicAPI Surface ChangesNew Types & Constants
ReviewFinding ExtensionsExtended with 10 new optional fields (all nullable when
Validation Rules (Pydantic model validators):
ReviewReport Extensions
Command-Level BehaviorSimplified Review Focus
Simplification Fix Pipeline (New)
Runner Changes
Scanner IntegrationAI-Bloat Runner
Semgrep Runner
Documentation & PromptsPrompt & Skill Updates
Schema & Contract
Testing Coverage
Cross-Module ImpactNo specfact-cli Changes Required
Registry & Packaging
WalkthroughThis PR implements guided simplification enforcement for code review, enriching simplification findings with safety classifications (safe_mechanical, preserve, needs_tests, design_judgment), constraining enforcement and auto-fixes to deterministic safe mechanical changes, and adapting prompt/skill guidance by walkthrough level (IDE vs. headless agent). ChangesGuided Simplification Enforcement for Code Review
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces a multi-layer feature spanning guided finding metadata, safety classification, enforcement rules, AST-based rewrites, and adaptive UI/prompt guidance. While each component follows established patterns, the interactions across the review model, multiple tool runners, enforcement logic, and prompt workflows require careful verification of the guidance kind semantics, safety constraints (especially the preserve classification for meaningful contracts), deterministic rewrite correctness, and backward compatibility of the JSON schema. The extensive test coverage and OpenSpec documentation mitigate review complexity but do not eliminate the need to reason through edge cases (decorator detection, pattern matching, headless mode behavior, and the interaction between enforce and fix modes). Possibly Related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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: 849d97503b
ℹ️ 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".
| for finding in report.findings: | ||
| if not finding.is_safe_mechanical_simplification() or not finding.fixable: | ||
| continue | ||
| if finding.rule == "ai-bloat.redundant-intermediate": | ||
| applied += int(_apply_redundant_intermediate_fix(finding)) |
There was a problem hiding this comment.
The --fix simplification pass uses each finding’s original line value while mutating files in-place, but this loop processes findings in report order. If an earlier rewrite deletes lines, later findings in the same file shift and no longer match (stmt.lineno != finding.line), so only some deterministic fixes are applied and unresolved safe-mechanical findings can still fail simplify enforce runs after --fix.
Useful? React with 👍 / 👎.
| if finding.rule == "ai-bloat.redundant-intermediate": | ||
| applied += int(_apply_redundant_intermediate_fix(finding)) | ||
| elif finding.rule == "ai-bloat.verbose-bool-return": | ||
| applied += int(_apply_verbose_bool_return_fix(finding)) |
There was a problem hiding this comment.
This switch only applies rewrites for two rule IDs, but the analyzer now emits other guidance_kind="safe_mechanical" findings with fixable=True (for example ai-bloat.dead-branch and ai-bloat.pass-through-try-except in ai_bloat_runner.py). Those findings are advertised as auto-fixable yet are silently skipped here, so specfact code review run --focus simplify --fix leaves claimed-fixable debt unresolved.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/rules/updater.py (1)
31-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing headless action-table rule to the generated template.
Line 31–57 define canonical DO rules, but the required headless behavior (“one file at a time” + emitted action table fields) is missing here while present in SKILL docs. If updater regenerates files, this drops policy-critical guidance and creates drift.
Suggested patch
DEFAULT_DO_RULES = ( @@ "- Log each simplification action as recommended, applied, kept, skipped, failed, with evidence of improvement " "or preserved contract", + "- In headless mode, process one file at a time and emit an action table: file, line, rule, guidance_kind, " + "recommended_action, action_status, evidence", "- For merge-quality review, run `specfact code review run --scope changed --bug-hunt --json " "--out .specfact/code-review.json`",🤖 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/rules/updater.py` around lines 31 - 57, DEFAULT_DO_RULES is missing the headless action-table rule; update the DEFAULT_DO_RULES tuple in updater.py to include a rule that mandates headless runs process one file at a time and emit the action table fields (e.g., action, target_path, start_line, end_line, edit_text, confidence, preserve_reason) so regenerated templates keep policy-critical guidance; modify the string list in DEFAULT_DO_RULES (the constant named DEFAULT_DO_RULES) to add a concise rule sentence covering "headless: one file at a time" and the required emitted action-table fields.
🧹 Nitpick comments (1)
tests/unit/test_guided_simplify_resources.py (1)
29-40: ⚡ Quick winExtend coverage to enforce skill-copy parity across all shipped locations.
This test validates only the bundled skill path. Drift in
.vibe/skills/specfact-code-review/SKILL.mdorskills/specfact-code-review/SKILL.mdwould currently pass, which weakens release integrity for synchronized guidance surfaces.🤖 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_guided_simplify_resources.py` around lines 29 - 40, The test test_code_review_skill_teaches_llms_how_to_apply_simplification_guidance only checks the bundled SKILL via SKILL.read_text; extend it to also load and assert parity for the other shipped SKILL copies by reading the other SKILL.md locations (e.g., the workspace copy and the .vibe copy) and asserting they contain the same required strings and/or that their contents are identical to SKILL.read_text(); update the test to iterate candidate paths, read_text or open each file, and run the same assertions (or an equality assertion against SKILL.read_text()) so drift across all shipped locations is caught.
🤖 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/specs/review-run-command/spec.md`:
- Around line 9-13: The clause requiring guidance metadata for simplify-focused
findings is currently marked as "SHOULD" but must be normative; change the term
"SHOULD" to "SHALL" in the sentence that reads "retained findings SHOULD include
guidance metadata for actionability, preservation, or design judgment" so the
spec mandates guidance metadata for simplify-focused findings; ensure the
surrounding lines mentioning "simplification-focused findings", the `specfact
code review run --focus simplify --json --out .specfact/code-review.json`
WHEN/THEN/AND block, and the requirement for a "simplification summary" remain
semantically consistent after this change.
In `@packages/specfact-code-review/src/specfact_code_review/run/findings.py`:
- Around line 359-361: The current computation of blocking_simplification_count
only counts safe-mechanical findings where finding.action_status ==
"recommended", which skips unresolved "failed" items; update the generator over
guided (the expression that uses finding.is_safe_mechanical_simplification() and
finding.action_status) so it treats both "recommended" and "failed" as blocking
(for example, check finding.action_status in ("recommended", "failed") or
equivalently that action_status != "resolved") to ensure failed safe-mechanical
items are counted as blocking unresolved findings.
- Around line 189-205: The validator _validate_guided_metadata currently returns
early when guidance_kind is None, allowing guided fields to be set without a
guidance_kind; change it to first check whether any guided-related fields
(recommended_action, clean_code_principle, rationale, safety_checks,
action_status, preserve_reason) are non-null and if so raise ValueError
requiring guidance_kind, then proceed with the existing validation (ensure
recommended_action, clean_code_principle, rationale, safety_checks,
action_status are present when guidance_kind is set and require preserve_reason
when guidance_kind == "preserve"); update _validate_guided_metadata to enforce
presence of guidance_kind whenever any of those guided fields are provided.
---
Outside diff comments:
In `@packages/specfact-code-review/src/specfact_code_review/rules/updater.py`:
- Around line 31-57: DEFAULT_DO_RULES is missing the headless action-table rule;
update the DEFAULT_DO_RULES tuple in updater.py to include a rule that mandates
headless runs process one file at a time and emit the action table fields (e.g.,
action, target_path, start_line, end_line, edit_text, confidence,
preserve_reason) so regenerated templates keep policy-critical guidance; modify
the string list in DEFAULT_DO_RULES (the constant named DEFAULT_DO_RULES) to add
a concise rule sentence covering "headless: one file at a time" and the required
emitted action-table fields.
---
Nitpick comments:
In `@tests/unit/test_guided_simplify_resources.py`:
- Around line 29-40: The test
test_code_review_skill_teaches_llms_how_to_apply_simplification_guidance only
checks the bundled SKILL via SKILL.read_text; extend it to also load and assert
parity for the other shipped SKILL copies by reading the other SKILL.md
locations (e.g., the workspace copy and the .vibe copy) and asserting they
contain the same required strings and/or that their contents are identical to
SKILL.read_text(); update the test to iterate candidate paths, read_text or open
each file, and run the same assertions (or an equality assertion against
SKILL.read_text()) so drift across all shipped locations is caught.
🪄 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: 55881d54-953f-47e4-afcb-94eaa3d9827c
📒 Files selected for processing (27)
.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.mdskills/specfact-code-review/SKILL.mdtests/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 (5)
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/changes/code-review-12-guided-simplification-enforcement/specs/review-finding-model/spec.mdopenspec/CHANGE_ORDER.mdopenspec/changes/code-review-12-guided-simplification-enforcement/specs/guided-simplification-review/spec.mdopenspec/changes/code-review-12-guided-simplification-enforcement/tasks.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/design.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:
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/run/test_findings.pypackages/specfact-code-review/src/specfact_code_review/run/commands.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/specfact_code_review/run/test_commands.pypackages/specfact-code-review/src/specfact_code_review/run/findings.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pypackages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.pytests/unit/specfact_code_review/run/test_runner.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/run/test_findings.pytests/unit/specfact_code_review/tools/test_ai_bloat_runner.pytests/unit/specfact_code_review/run/test_commands.pytests/unit/specfact_code_review/tools/test_semgrep_runner.pytests/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/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
🪛 LanguageTool
openspec/changes/code-review-12-guided-simplification-enforcement/proposal.md
[style] ~33-~33: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... required fields remain compatible. - Affected prompt resources: `packages/specfact-...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~34-~34: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s/prompts/specfact.08-simplify.md. - **Affected skill resources:** packages/specfact-c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
openspec/changes/code-review-12-guided-simplification-enforcement/design.md
[style] ~26-~26: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...efactor for design-judgment findings. - No breaking removal of existing simplifica...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~27-~27: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sting simplification metadata fields. - No claim that findings prove AI authorship...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (24)
packages/specfact-code-review/src/specfact_code_review/run/runner.py (2)
103-105: LGTM!
654-659: LGTM!tests/unit/specfact_code_review/run/test_runner.py (2)
57-84: LGTM!
226-287: LGTM!packages/specfact-code-review/src/specfact_code_review/run/commands.py (2)
5-5: LGTM!Also applies to: 283-416
515-517: LGTM!Also applies to: 537-542
tests/unit/specfact_code_review/run/test_commands.py (2)
31-60: LGTM!
306-390: LGTM!openspec/CHANGE_ORDER.md (1)
105-105: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/.openspec.yaml (1)
1-2: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/TDD_EVIDENCE.md (1)
1-61: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/design.md (1)
1-70: LGTM!openspec/changes/code-review-12-guided-simplification-enforcement/proposal.md (1)
1-46: 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/tasks.md (1)
1-53: LGTM!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
packages/specfact-code-review/src/specfact_code_review/run/findings.py (1)
30-31: LGTM!Also applies to: 116-187, 220-243, 265-273, 290-318, 345-358, 362-364
tests/unit/specfact_code_review/run/test_findings.py (1)
42-51: LGTM!Also applies to: 118-155, 281-312
packages/specfact-code-review/src/specfact_code_review/tools/ai_bloat_runner.py (1)
8-9: LGTM!Also applies to: 30-37, 52-63, 127-139, 150-150, 164-186, 214-228, 275-292, 314-321, 360-366, 390-397, 463-472, 499-504, 532-538, 553-556, 590-595, 619-624, 647-648, 702-709
tests/unit/specfact_code_review/tools/test_ai_bloat_runner.py (1)
33-35: LGTM!Also applies to: 77-82, 94-100, 205-232
packages/specfact-code-review/src/specfact_code_review/tools/semgrep_runner.py (1)
9-10: LGTM!Also applies to: 42-87, 254-255, 372-408
tests/unit/specfact_code_review/tools/test_semgrep_runner.py (1)
13-18: LGTM!Also applies to: 166-170, 271-309, 345-399
| - **WHEN** `specfact code review run --focus simplify --json --out .specfact/code-review.json` completes | ||
| - **THEN** the JSON report SHALL retain simplification-focused findings | ||
| - **AND** retained findings SHOULD include guidance metadata for actionability, preservation, or design judgment | ||
| - **AND** the report SHALL include a simplification summary when guided findings are present | ||
|
|
There was a problem hiding this comment.
Strengthen guidance metadata requirement to avoid spec drift.
Line 11 uses SHOULD, but the guided simplification contract in this change set treats guidance metadata as required for simplify-focused findings. Keep this normative level aligned (SHALL) to prevent ambiguous implementations across bundles/CLI surfaces.
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/specs/review-run-command/spec.md`
around lines 9 - 13, The clause requiring guidance metadata for simplify-focused
findings is currently marked as "SHOULD" but must be normative; change the term
"SHOULD" to "SHALL" in the sentence that reads "retained findings SHOULD include
guidance metadata for actionability, preservation, or design judgment" so the
spec mandates guidance metadata for simplify-focused findings; ensure the
surrounding lines mentioning "simplification-focused findings", the `specfact
code review run --focus simplify --json --out .specfact/code-review.json`
WHEN/THEN/AND block, and the requirement for a "simplification summary" remain
semantically consistent after this change.
| @model_validator(mode="after") | ||
| def _validate_guided_metadata(self) -> ReviewFinding: | ||
| if self.guidance_kind is None: | ||
| return self | ||
| if self.recommended_action is None: | ||
| raise ValueError("recommended_action is required when guidance_kind is present") | ||
| if self.clean_code_principle is None: | ||
| raise ValueError("clean_code_principle is required when guidance_kind is present") | ||
| if self.rationale is None: | ||
| raise ValueError("rationale is required when guidance_kind is present") | ||
| if self.safety_checks is None: | ||
| raise ValueError("safety_checks is required when guidance_kind is present") | ||
| if self.action_status is None: | ||
| raise ValueError("action_status is required when guidance_kind is present") | ||
| if self.guidance_kind == "preserve" and self.preserve_reason is None: | ||
| raise ValueError("preserve_reason is required for preserve guidance") | ||
| return self |
There was a problem hiding this comment.
Enforce guidance_kind whenever guided fields are present.
This validator exits early when guidance_kind is missing, so payloads can carry recommended_action/action_status/etc. without a guidance class. That makes guided findings silently degrade into non-guided behavior (summary/enforcement/schema branching).
Proposed fix
`@model_validator`(mode="after")
def _validate_guided_metadata(self) -> ReviewFinding:
- if self.guidance_kind is None:
- return self
+ guided_fields_present = any(
+ value is not None
+ for value in (
+ self.recommended_action,
+ self.clean_code_principle,
+ self.rationale,
+ self.safety_checks,
+ self.preserve_reason,
+ self.action_status,
+ self.before_ref,
+ self.after_ref,
+ self.improvement,
+ )
+ )
+ if self.guidance_kind is None:
+ if guided_fields_present:
+ raise ValueError("guidance_kind is required when guided simplification fields are present")
+ return self🤖 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/findings.py`
around lines 189 - 205, The validator _validate_guided_metadata currently
returns early when guidance_kind is None, allowing guided fields to be set
without a guidance_kind; change it to first check whether any guided-related
fields (recommended_action, clean_code_principle, rationale, safety_checks,
action_status, preserve_reason) are non-null and if so raise ValueError
requiring guidance_kind, then proceed with the existing validation (ensure
recommended_action, clean_code_principle, rationale, safety_checks,
action_status are present when guidance_kind is set and require preserve_reason
when guidance_kind == "preserve"); update _validate_guided_metadata to enforce
presence of guidance_kind whenever any of those guided fields are provided.
| blocking_simplification_count=sum( | ||
| finding.is_safe_mechanical_simplification() and finding.action_status == "recommended" for finding in guided | ||
| ), |
There was a problem hiding this comment.
Count failed safe-mechanical items as blocking unresolved findings.
blocking_simplification_count currently increments only for "recommended". Unresolved "failed" safe-mechanical items are excluded, which can under-report enforce blockers.
Proposed fix
- blocking_simplification_count=sum(
- finding.is_safe_mechanical_simplification() and finding.action_status == "recommended" for finding in guided
- ),
+ blocking_simplification_count=sum(
+ finding.is_safe_mechanical_simplification() for finding in guided
+ ),🤖 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/findings.py`
around lines 359 - 361, The current computation of blocking_simplification_count
only counts safe-mechanical findings where finding.action_status ==
"recommended", which skips unresolved "failed" items; update the generator over
guided (the expression that uses finding.is_safe_mechanical_simplification() and
finding.action_status) so it treats both "recommended" and "failed" as blocking
(for example, check finding.action_status in ("recommended", "failed") or
equivalently that action_status != "resolved") to ensure failed safe-mechanical
items are counted as blocking unresolved findings.
1 participant
Summary
Adds guided simplification review enforcement for the code-review and project modules. The change teaches review reports to distinguish safe mechanical cleanup, test-dependent simplification, design-judgment cases, and preserve decisions so IDE/headless LLM agents can apply clean-code fixes without removing meaningful contracts.
What changed
code-review-12-guided-simplification-enforcementand linked backlog tracking for [Change] Guided simplification enforcement for code review #286 under parent feature [Feature] Code Review Simplification Feedback Loop #275 / epic [Epic] specfact code #162.Validation
hatch run formathatch run pytest tests/unit/specfact_code_review/tools/test_semgrep_runner.py --cov packages/specfact-code-review/src/specfact_code_review --cov-report=term-missing --cov-fail-under=0 -q- 42 passed;semgrep_runner.pycoverage 80.20%hatch run type-checkhatch run linthatch run sign-modules --changed-only --payload-from-filesystem --bump-version patch --base-ref origin/dev --allow-unsignedhatch run verify-modules-signature --payload-from-filesystem --enforce-version-bump --version-check-base origin/devSPECFACT_ALLOW_UNSIGNED=1 SPECFACT_CODE_REVIEW_TARGETED_TEST_TIMEOUT=300 hatch run specfact code review run --bug-hunt --json --out .specfact/tmp-local-dev-link-review/changed-bughunt-clean-final.json --scope changedopenspec validate code-review-12-guided-simplification-enforcement --strictCloses #286.