v0.4.0#9
Merged
Merged
Conversation
… with rule catalogue changes
…ter sets and quoted key literals; refactor related methods for clarity
|
Warning Review limit reached
More reviews will be available in 54 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (112)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates gruff-php to reflect the retirement of the bundled project-wide rules, improves CLI/hook robustness in non-interactive environments, and adds several precision/performance improvements (notably: enabled-rule memoisation, safer missing-config prompting, and result-cache eviction changes). It also updates fixtures/tests/docs to lock in the new behaviors and rule catalogue counts.
Changes:
- Retires bundled
ProjectRuleInterfacerules (dead-code unused-internal-* and design single-implementor-interface) and updates registry/docs/config/tests accordingly. - Makes
analyse --include-rule/--exclude-ruleexecution-level (matchinghook), adds unknown-rule-id compatibility forrules:blocks (warn-and-ignore), and adds missing-config prompt guards for non-TTY / machine-readable outputs. - Improves performance/precision: enabled-rule memoisation in
RuleRegistry, once-per-runResultCache::finalizeRun()eviction + higher cap, and multiple rule precision tweaks (SensitiveData, Security, Naming, TestQuality, Modernisation).
Reviewed changes
Copilot reviewed 105 out of 105 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Rule/TestQuality/TestQualityRulesTest.php | Updates expected finding count for extends-production fixture. |
| tests/Rule/TestQuality/TestQualityConfigurationRulesTest.php | Adds config coverage for additionalTestBaseClasses. |
| tests/Rule/SensitiveData/SensitiveDataAllowanceRulesTest.php | New precision test asserting sensitive-data allowances and counter-fixtures. |
| tests/Rule/Security/SecurityRulesTest.php | Adds precision fixtures + strict-option override coverage; adds shared findingLines() helper; allows config override in helper. |
| tests/Rule/RuleRegressionSnapshotTest.php | Removes retired rule ids from registry coverage; updates snapshot counts/hash. |
| tests/Rule/RuleRegistryTest.php | Removes retired ids; adds project-rule accumulator seam test and enabled-rule memoisation tests. |
| tests/Rule/Naming/NamingRulesTest.php | Extends boolean-prefix assertions for snake_case and lowercase-boundary cases. |
| tests/Rule/Naming/NamingRuleConfigurationTest.php | Extends boolean-prefix config assertions (prefix position + snake_case). |
| tests/Rule/Naming/NamingAdvancedRulesTest.php | Adds callback-parameter loop-threshold test for identifier quality rule. |
| tests/Rule/Modernisation/ModernisationRulesTest.php | Adds write-position skip coverage for forbidden-global-access. |
| tests/Rule/Design/SingleImplementorInterfaceRuleTest.php | Deletes test for retired design rule. |
| tests/Review/AgentWorkflowFixtureSources.php | Removes fixture sources used for retired project-rule review tests. |
| tests/Review/AgentWorkflowCliTest.php | Adjusts include-rule/min-severity semantics test and removes project-rule changed-only test. |
| tests/Reporting/OutputFormatTest.php | New test covering OutputFormat::isMachineReadable(). |
| tests/Fixtures/TestQuality/extends-production.php | Expands fixture for underscore test bases + configurable base class case. |
| tests/Fixtures/SensitiveData/secret-counter-shapes.php | New counter-fixture for high-entropy / JWT / AWS-key shapes. |
| tests/Fixtures/SensitiveData/pii-synthetic-allowed.php | New fixture for reserved-domain emails + marker-word addresses. |
| tests/Fixtures/SensitiveData/pii-realistic.php | New fixture containing realistic PII that must keep flagging. |
| tests/Fixtures/SensitiveData/identifier-slug-literals.php | New fixture of identifier/slug literals that should not flag as secrets. |
| tests/Fixtures/Security/variable-include-precision.php | New fixture covering variable-include precision (attacks + fixed shapes). |
| tests/Fixtures/Security/sql-concatenation-precision.php | New fixture covering SQL concatenation precision (wpdb/prepare/non-SQL). |
| tests/Fixtures/Naming/identifier-quality-callback-params.php | New fixture for iteration-callback parameter loop escape hatch. |
| tests/Fixtures/Naming/boolean-prefix.php | Adds snake_case predicate names + lowercase-boundary cases. |
| tests/Fixtures/Naming/boolean-prefix-properties.php | Adds snake_case + non-leading-prefix boolean property cases. |
| tests/Fixtures/Modernisation/forbidden-global-write-positions.php | New fixture separating superglobal reads vs write-position uses. |
| tests/Fixtures/Design/single-implementor-interface/symfony-tagged/SymfonyTaggedListener.php | Deletes fixture for retired design rule. |
| tests/Fixtures/Design/single-implementor-interface/psr/AuditPsrLogger.php | Deletes fixture for retired design rule. |
| tests/Fixtures/Design/single-implementor-interface/mutation-cases/MutationCases.php | Deletes fixture for retired design rule. |
| tests/Fixtures/Design/single-implementor-interface/multi-impl/Renderer.php | Deletes fixture for retired design rule. |
| tests/Fixtures/Design/single-implementor-interface/mock-only/BookingEventSink.php | Deletes fixture for retired design rule. |
| tests/Fixtures/Design/single-implementor-interface/internal-one-impl/BookingOtpGateway.php | Deletes fixture for retired design rule. |
| tests/Fixtures/Design/single-implementor-interface/interface-hierarchy/InterfaceHierarchy.php | Deletes fixture for retired design rule. |
| tests/Fixtures/DeadCode/project-wide/tests/TestReferences.php | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/src/Symbols.php | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/src/references.php | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/src/FrameworkCommand.php | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/src/External/Vendored.php | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/entrypoints/Entrypoints.php | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/config/routes/block.yml | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/DeadCode/project-wide/composer.json | Deletes fixture project for retired project-wide dead-code rules. |
| tests/Fixtures/Config/retired-project-rules.yaml | New fixture config carrying retired rule blocks to ensure compatibility behavior. |
| tests/Console/ResultCacheCliTest.php | Updates expectation: default run now writes .gruff-cache with no project rules. |
| tests/Console/HookCliFilteringTest.php | Adds hook rejection tests for unknown include/exclude rule filters. |
| tests/Console/AnalyseCliUnknownRuleIdTest.php | New CLI tests for warn-and-run on unknown config rule ids + reject unknown execution filters. |
| tests/Console/AnalyseCliTest.php | Replaces invalid-config failure test with execution-level exclude-rule semantics test. |
| tests/Console/AnalyseCliRuntimeTest.php | Adds --no-cache note; adds runtime assertions that excluded rules do not execute; adds phase coverage test; factors runtime helper. |
| tests/Console/AnalyseCliMissingConfigGuardTest.php | New tests preventing hangs/config writes on piped/non-TTY stdin. |
| tests/Console/AnalyseCliDiffTest.php | Removes changed-range project-wide findings test and supporting fixtures. |
| tests/Config/ConfigLoaderUnknownRuleIdTest.php | New tests ensuring ConfigLoader ignores unknown/retired rule ids in rules: blocks. |
| tests/Config/ConfigLoaderTest.php | Removes test that expected unknown rule ids to hard-fail. |
| tests/Config/ConfigLoaderRuleOptionsTest.php | Replaces project-rule option tests with unknown-rule warnings test + new option-load tests for updated rules. |
| tests/Command/MissingConfigPromptTest.php | Adds tests for real-STDIN TTY probing, explicit stream behavior, and machine-readable suppression. |
| tests/Cache/ResultCacheTest.php | New unit tests for deferred eviction and run-cap logic. |
| src/Rule/TestQuality/ExtendsProductionClassRule.php | Adds underscore-insensitive *TestCase detection and configurable additional test base classes. |
| src/Rule/SensitiveData/PiiTestFixtureRule.php | Adds allowances for reserved-domain emails + synthetic address markers; consolidates suppression logic. |
| src/Rule/SensitiveData/HighEntropyStringRule.php | Adds identifier/slug/key/keyspace allowances and quoted-key detection to reduce false positives. |
| src/Rule/Security/SecurityNodeHelper.php | Exposes enclosingFunctionLike() for reuse by other security rules. |
| src/Rule/RuleRegistry.php | Removes retired rules; snapshots definitions at construction; memoises enabled rules by config; adds unknown-rule-id helper. |
| src/Rule/Naming/IdentifierQualityRule.php | Extends loop escape hatch to sole-parameter inline iteration callbacks. |
| src/Rule/Naming/BooleanPrefixRule.php | Treats snake_case underscore as predicate boundary for allowed/negative prefixes. |
| src/Rule/Modernisation/ForbiddenGlobalAccessRule.php | Skips superglobal write positions (assign/unset), continues flagging reads/compound ops. |
| src/Rule/DeadCode/UnusedInternalFunctionRule.php | Deletes retired project-wide rule implementation. |
| src/Rule/DeadCode/UnusedInternalConstantRule.php | Deletes retired project-wide rule implementation. |
| src/Rule/DeadCode/UnusedInternalClassRule.php | Deletes retired project-wide rule implementation. |
| src/Rule/DeadCode/DeadCodeSymbolReference.php | Deletes support type for retired rules. |
| src/Rule/DeadCode/DeadCodeSymbolDeclaration.php | Deletes support type for retired rules. |
| src/Rule/DeadCode/DeadCodeProjectScope.php | Deletes support machinery for retired rules. |
| src/Rule/DeadCode/DeadCodeNameResolver.php | Deletes support machinery for retired rules. |
| src/Rule/DeadCode/AbstractUnusedInternalSymbolRule.php | Deletes support machinery for retired rules. |
| src/Reporting/OutputFormat.php | Adds OutputFormat::isMachineReadable(). |
| src/Config/RuleConfigApplier.php | Warn-and-ignore unknown rule ids in rules: blocks; injectable warning sink for tests. |
| src/Command/SummaryCommand.php | Suppresses missing-config init offer for machine-readable summary output. |
| src/Command/ReportCommand.php | Updates option text; suppresses init offer for json; forwards --no-interaction to child analyse. |
| src/Command/MissingConfigPrompt.php | Adds machine-readable suppression + real-STDIN TTY guard (prevents hangs / accidental writes on piped stdin). |
| src/Command/HookCommand.php | Validates include/exclude rule filters against registry (errors on unknown). |
| src/Command/AnalysisPipeline.php | Enables caching when no project rules and working set fits cap; calls cache finalize once per run. |
| src/Command/AnalyseCommandSetupBuilder.php | Applies execution-level rule filtering, rejects unknown filter ids, and suppresses init offer for machine-readable formats. |
| src/Command/AnalyseCommandOptions.php | Updates docs to reflect include/exclude rule as execution-level (metadata still records them). |
| src/Command/AnalyseCommand.php | Updates CLI option help text to reflect execution-level semantics. |
| src/Cache/ResultCache.php | Raises cap, defers eviction to finalizeRun(), adds canHoldRun() eligibility check, makes cap injectable for tests. |
| README.md | Updates rule/pillar counts and clarifies rule-filter execution vs display semantics. |
| docs/rules.md | Updates catalogue counts and adds narrative notes for several updated rule behaviors. |
| docs/gruff-cli-branch-review.md | Updates project-rule guidance post-retirement. |
| docs/gruff-cli-agent-instructions.md | Updates rule-filter semantics and project-rule guidance post-retirement. |
| CHANGELOG.md | Adds unreleased 0.4.0 notes describing project-rule retirement and related behavior changes. |
| .gruff-php.yaml | Removes retired rule blocks and excludes; updates config to match new catalogue. |
| .goat-flow/learning-loop/footguns/commands.md | Adds resolved footguns for project-rule cost and missing-config prompt behavior. |
| .goat-flow/learning-loop/decisions/README.md | Adds ADR-025/ADR-026 to index. |
| .goat-flow/learning-loop/decisions/ADR-026-retire-project-rules.md | New ADR documenting project-rule retirement rationale and consequences. |
| .goat-flow/learning-loop/decisions/ADR-020-incremental-result-cache.md | Addendum correcting project-rule count and documenting cache scale fixes. |
| .goat-flow/code-map.md | Updates code map to remove retired rule references and reflect reserved Design pillar. |
| .codex/hooks.json | Makes local hook invocation robust (skip gracefully when repo root/hook missing). |
| .claude/settings.json | Makes local hook invocation robust (skip gracefully when repo root/hook missing). |
Comment on lines
+121
to
+124
| public static function canHoldRun(int $discoveredFileCount): bool | ||
| { | ||
| return $discoveredFileCount <= self::MAX_ENTRIES; | ||
| } |
Comment on lines
+311
to
+313
| // Match the dotted-identifier shape PHPCS sniff ids use: letter-led segments joined by two or more dots. | ||
| $hasDottedIdentifierShape = preg_match('/^[A-Za-z][A-Za-z0-9_]*(?:\.[A-Za-z][A-Za-z0-9_]*){2,}$/', $candidateSecret) === 1; | ||
| // Match the underscore-identifier shape class names such as WPCOM_REST_API_V2_Endpoint_External_Media use. |
Comment on lines
+121
to
+124
| public static function canHoldRun(int $discoveredFileCount): bool | ||
| { | ||
| return $discoveredFileCount <= self::MAX_ENTRIES; | ||
| } |
… strings in tests and README
…y/polyfill-mbstring and friendsofphp/php-cs-fixer
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…s, behavior modifications, and performance improvements
![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: c394da0cd3
ℹ️ 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".
| | Package | `blundergoat/gruff-php` | | ||
| | Binary | `bin/gruff-php` from checkout; `vendor/bin/gruff-php` after install | | ||
| | Rule catalogue | 133 rules across 11 pillars | | ||
| | Rule catalogue | 129 rules across 10 pillars | |
There was a problem hiding this comment.
This release changes the public rule count to 129 here, but composer.json still advertises the package as an analyzer with 133 rules. Anyone viewing the package metadata on Packagist/Composer will see the old catalogue size after the four project-wide rules are removed, so the release ships contradictory user-facing claims; update the Composer description or remove the count.
Useful? React with 👍 / 👎.
…sage error handling
| root="$(repo_root)" | ||
| cd "$root" || exit 0 | ||
| payload_paths="$(payload_file_paths "$payload")" | ||
| payload_paths="$(payload_supported_file_paths "$root" "$payload")" |
There was a problem hiding this comment.
Hook scans all git changes
Medium Severity
When the PostToolUse payload lists edited paths but none are gruff-supported (for example markdown or docs-only edits), payload_supported_file_paths returns empty and main treats that like a missing payload, falling back to git_changed_supported_paths. The hook then runs changed-line analysis on every supported file in the working tree instead of doing nothing for that edit.
Reviewed by Cursor Bugbot for commit 4dd1a2e. Configure here.
…e number of rules
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b84c152. Configure here.
| empty | ||
| end | ||
| ' 2>/dev/null || true | ||
| } |
There was a problem hiding this comment.
Config diagnostic matcher too broad
Medium Severity
The new config_error_message helper treats any diagnostic whose type/code/kind/category contains the substring config as a config failure. Valid JSON with findings can be misclassified, so the hook prints a config error and returns without surfacing changed-line results.
Reviewed by Cursor Bugbot for commit b84c152. Configure here.
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
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.
This pull request primarily updates documentation and configuration to reflect the retirement of project-wide rules in the code quality analysis tool, and improves the robustness of code quality hook scripts in local development environments. The most significant changes are the removal of references to project-wide dead-code and design rules, updates to the rule catalogue, and improved error handling in hook scripts.
Documentation updates for rule retirement:
.goat-flow/architecture.md: Updated to clarify that no bundled rule currently implementsProjectRuleInterface, and that the previous project rules (such as internal dead-code checks and the design single-implementor interface rule) were retired in ADR-026. Adjusted the rule catalogue to remove these rules from the active set and to update pillar counts and descriptions accordingly. [1] [2].goat-flow/code-map.md: Removed references to source files and classes related to the retired project-wide dead-code rules and the design single-implementor interface rule.Configuration and hook script improvements:
.claude/settings.jsonand.codex/hooks.json: Simplified thegruff-code-quality.shhook command logic to handle missing repository roots or hook scripts gracefully by skipping the hook instead of blocking execution. Updated error messages to be less severe and to exit with status 0 when skipping. [1] [2] [3] [4]Note
High Risk
Breaking rule removals and changed
--include-rule/--exclude-rulesemantics affect CI gates, baselines, and upgrade paths; performance and finding sets change materially on default configs.Overview
gruff-php 0.4.0 removes the four built-in project-wide rules (
dead-code.unused-internal-*,design.single-implementor-interface) and their dead-code/design indexing machinery, dropping the catalogue from 133 → 129 rules. That is a breaking behaviour change: narrowanalyse/hookruns no longer re-parse the whole tree for those rules, and the per-file result cache can engage on default runs (cap 32768, eviction once per run viafinalizeRun(), runs over the cap skip caching).CLI and config:
--include-rule/--exclude-rulenow control which rules run (aligned withhook), not just report display; unknown ids fail fast on analyse/report/hook. Retired rule blocks underrules:warn and are ignored instead of failing load. Missing-config init is suppressed for machine-readable formats and non-TTY stdin;reportforwards--no-interactionto its analyse child.Agent hooks:
.claude/.codexPostToolUse hooks simplify repo-root resolution and skip (exit 0) when the gruff hook is missing;gruff-code-quality.shgains binary override env vars, longer default timeout, config-error surfacing, and clearer “binary not found” diagnostics.Docs/ADR-026, default
.gruff-php.yaml, and rule-catalogue notes reflect the retirement; several per-unit rules get precision tweaks called out inCHANGELOG.mdanddocs/rules.md.Reviewed by Cursor Bugbot for commit b84c152. Bugbot is set up for automated code reviews on this repo. Configure here.