v0.3.1#8
Conversation
📝 WalkthroughWalkthroughAdds StaticAnalysisRedundantTestRule; refactors gruff-code-quality hooks for jq-first multi-path discovery, native changed-region delegation, and severity-ranked JSON reporting; extends DeadCode YAML _controller indexing; and updates reporters, tests, fixtures, docs, and hook wiring. ChangesStatic-analysis-redundant-test rule and hook script refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f55f5ee263
ℹ️ 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".
| return null; | ||
| } | ||
|
|
||
| $declaredName = $declaration[$memberBucket][strtolower($member)] ?? null; |
There was a problem hiding this comment.
Preserve property-name case in redundant-test matching
For property_exists() candidates this lower-cases the asserted member name before lookup. PHP property names are case-sensitive, so property_exists(Foo::class, 'LABEL') is false when the declaration is only $label; with the new rule enabled, that typo/case regression is reported as a redundant static-fact assertion and can lead users to remove the test instead of fixing the property name. Keep method lookups case-insensitive, but compare properties with their original case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.codex/hooks/gruff-code-quality.sh (1)
1-857: 💤 Low valueConsider extracting shared hook logic to reduce duplication.
This file is nearly identical to
.claude/hooks/gruff-code-quality.sh. Both scripts must be updated in lockstep, which creates maintenance risk. A shared script (e.g., in a common location likebin/or.goat-flow/hooks/) sourced or symlinked by both could reduce drift.That said, separate copies may be intentional for runtime isolation—defer if so.
🤖 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 @.codex/hooks/gruff-code-quality.sh around lines 1 - 857, The hook is duplicated with .claude/hooks/gruff-code-quality.sh; extract the common logic into a single sourced module (e.g., bin/gruff-code-quality-common.sh) and have both scripts source it. Move shared functions and symbols such as read_stdin, json_file_paths, variant_for_path, discover_binary, changed_ranges, run_gruff_json, valid_gruff_json, changed_findings_report, suppressed_count, ignored_descriptor, process_file, and main-support helpers into the common file and leave only small wrapper scripts that set any runner-specific constants and call the shared main/process_file entrypoint; update both .codex/hooks/gruff-code-quality.sh and .claude/hooks/gruff-code-quality.sh to source the common file and delegate to the shared functions so future fixes are made once.
🤖 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.
Nitpick comments:
In @.codex/hooks/gruff-code-quality.sh:
- Around line 1-857: The hook is duplicated with
.claude/hooks/gruff-code-quality.sh; extract the common logic into a single
sourced module (e.g., bin/gruff-code-quality-common.sh) and have both scripts
source it. Move shared functions and symbols such as read_stdin,
json_file_paths, variant_for_path, discover_binary, changed_ranges,
run_gruff_json, valid_gruff_json, changed_findings_report, suppressed_count,
ignored_descriptor, process_file, and main-support helpers into the common file
and leave only small wrapper scripts that set any runner-specific constants and
call the shared main/process_file entrypoint; update both
.codex/hooks/gruff-code-quality.sh and .claude/hooks/gruff-code-quality.sh to
source the common file and delegate to the shared functions so future fixes are
made once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d31c1fa3-5a4e-475b-92e0-84c53599b75a
📒 Files selected for processing (19)
.claude/hooks/gruff-code-quality.sh.claude/settings.json.codex/hooks/gruff-code-quality.sh.goat-flow/architecture.md.goat-flow/code-map.md.goat-flow/decisions/ADR-022-test-quality-gate-parity.md.gruff-php.yamlcomposer.jsondocs/rules.mdsrc/Command/SummaryCommand.phpsrc/Reporting/TextReporter.phpsrc/Rule/RuleRegistry.phpsrc/Rule/TestQuality/StaticAnalysisRedundantTestRule.phptests/Console/GruffCliSummaryTest.phptests/Fixtures/Cli/Golden/text-warning.txttests/Fixtures/TestQuality/static-analysis-redundant-test.phptests/Rule/RuleRegistryTest.phptests/Rule/RuleRegressionSnapshotTest.phptests/Rule/TestQuality/TestQualityRulesTest.php
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f6b30ae. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6b30ae7e0
ℹ️ 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".
| foreach (NodeIndex::nodesOfAny( | ||
| $analysisUnit, | ||
| [Stmt\Class_::class, Stmt\Interface_::class, Stmt\Trait_::class, Stmt\Enum_::class], | ||
| ) as $node) { |
There was a problem hiding this comment.
Skip non-top-level declarations for redundant checks
When a test file contains a class/interface/trait/enum declared inside a function, method, or conditional block, PHP does not register that symbol until that code path executes; for example class_exists(Foo::class, false) before a nested declaration is still false. Because this indexes every class-like node returned by the full-AST NodeIndex walk, the new rule treats those runtime-dependent declarations as statically proven and can report a non-redundant existence assertion as something users should remove. Restrict the declaration index to top-level/namespace declarations, or otherwise exclude conditional/nested declarations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Console/GruffCliSummaryTest.php (1)
40-40: ⚡ Quick winUse
Application::VERSIONinstead of hardcoded release strings in assertions.This avoids touching multiple tests on every version bump.
Proposed refactor
+use GruffPhp\Console\Application; use JsonException; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Symfony\Component\Process\Process; @@ - self::assertStringContainsString('gruff-php 0.3.1 summary', $output); + self::assertStringContainsString('gruff-php ' . Application::VERSION . ' summary', $output); @@ - self::assertSame('0.3.1', $tool['version'] ?? null); + self::assertSame(Application::VERSION, $tool['version'] ?? null);Also applies to: 111-111
🤖 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/Console/GruffCliSummaryTest.php` at line 40, Replace the hardcoded version string in the assertion with the app's version constant: instead of asserting 'gruff-php 0.3.1 summary', build the expected text using Application::VERSION (e.g., "gruff-php " . Application::VERSION . " summary") and use that in the assertStringContainsString call referring to $output; update all similar assertions (e.g., the one at line 111) so tests no longer rely on a pinned release string.
🤖 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.
Nitpick comments:
In `@tests/Console/GruffCliSummaryTest.php`:
- Line 40: Replace the hardcoded version string in the assertion with the app's
version constant: instead of asserting 'gruff-php 0.3.1 summary', build the
expected text using Application::VERSION (e.g., "gruff-php " .
Application::VERSION . " summary") and use that in the
assertStringContainsString call referring to $output; update all similar
assertions (e.g., the one at line 111) so tests no longer rely on a pinned
release string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f2bb75c-576e-4501-8e8d-d21666e03f14
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
CHANGELOG.mdsrc/Console/Application.phpsrc/Rule/DeadCode/DeadCodeProjectIndex.phpsrc/Rule/DeadCode/UnusedInternalClassRule.phpsrc/Rule/ProjectSourceTextRuleAccumulator.phpsrc/Rule/RuleRegistry.phptests/Console/AnalyseCliTest.phptests/Console/GruffCliSummaryTest.phptests/Console/ListRulesCliTest.phptests/Fixtures/Cli/Golden/json-warning.jsontests/Fixtures/Cli/Golden/text-warning.txttests/Fixtures/DeadCode/project-wide/config/routes/block.ymltests/Fixtures/DeadCode/project-wide/config/routes/inline.yamltests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yamltests/Fixtures/DeadCode/project-wide/config/routes/quoted.yamltests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.phptests/Rule/DeadCode/ProjectDeadCodeRulesTest.phptests/Rule/RuleRegressionSnapshotTest.phptests/Rule/TestQuality/TestQualityRulesTest.php
✅ Files skipped from review due to trivial changes (7)
- tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml
- tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml
- tests/Console/AnalyseCliTest.php
- tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml
- CHANGELOG.md
- tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php
- tests/Fixtures/Cli/Golden/json-warning.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/Command/AnalysisFindingSupport.php`:
- Around line 79-80: The early return currently uses the condition "if
($projectRuleIds === [] || $filePaths === []) { return $findings; }" which
treats an empty discovered-file set as unscoped; change the logic so an empty
$filePaths does not preserve all project-rule findings. Replace the OR with an
AND (i.e. only return when both $projectRuleIds and $filePaths are empty) or
alternatively, if $filePaths is empty, ensure project-level findings are
filtered out instead of returning $findings; update the check around
$projectRuleIds, $filePaths and the return so unrelated project-wide findings
are not kept.
In `@src/Command/AnalysisPipeline.php`:
- Around line 137-143: The check that sets $hasNarrowProjectRuleContext treats
any explicit paths (e.g. analyse .) as narrowing the project and disables
streaming; update the predicate in AnalysisPipeline.php so that only genuinely
narrow paths (paths that are not the project root) set
$hasNarrowProjectRuleContext. Concretely, change the condition using
$options->paths to detect and ignore a single root path (or "." equivalent) — or
add/use a helper on the options object (e.g. isRootOnly or similar) — so
BranchReviewBuilder::shouldLoadProjectContext() still recognizes full-project
invocations and avoids an extra full-tree parse.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f125884b-c100-492c-a268-1ec4d2565468
📒 Files selected for processing (10)
CHANGELOG.mddocs/output-formats.mdsrc/Command/AnalyseCommand.phpsrc/Command/AnalysisFindingSupport.phpsrc/Command/AnalysisPipeline.phpsrc/Command/BranchReviewBuilder.phpsrc/Diff/DiffResult.phpsrc/Rule/DeadCode/DeadCodeProjectIndex.phpsrc/Rule/RuleRegistry.phptests/Console/AnalyseCliDiffTest.php
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Rule/DeadCode/DeadCodeProjectIndex.php
| if ($projectRuleIds === [] || $filePaths === []) { | ||
| return $findings; |
There was a problem hiding this comment.
Don't treat an empty discovered-file set as unscoped.
When the requested paths load zero source files, this early return preserves every project-rule finding from the whole-project context. In the new legacy flow, that means analyse can still emit unrelated project-wide findings for files the invocation never loaded.
Suggested fix
public function filterProjectRuleFindingsToFiles(array $findings, array $projectRuleIds, array $filePaths): array
{
- if ($projectRuleIds === [] || $filePaths === []) {
+ if ($projectRuleIds === []) {
return $findings;
}
$projectRules = array_fill_keys($projectRuleIds, true);
+ if ($filePaths === []) {
+ return array_values(array_filter(
+ $findings,
+ static fn(Finding $finding): bool => !isset($projectRules[$finding->ruleId]),
+ ));
+ }
+
$files = array_fill_keys($filePaths, true);
return array_values(array_filter(
$findings,
static fn(Finding $finding): bool => !isset($projectRules[$finding->ruleId])🤖 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 `@src/Command/AnalysisFindingSupport.php` around lines 79 - 80, The early
return currently uses the condition "if ($projectRuleIds === [] || $filePaths
=== []) { return $findings; }" which treats an empty discovered-file set as
unscoped; change the logic so an empty $filePaths does not preserve all
project-rule findings. Replace the OR with an AND (i.e. only return when both
$projectRuleIds and $filePaths are empty) or alternatively, if $filePaths is
empty, ensure project-level findings are filtered out instead of returning
$findings; update the check around $projectRuleIds, $filePaths and the return so
unrelated project-wide findings are not kept.
| $hasNarrowProjectRuleContext = $options->paths !== [] | ||
| && $this->registry->hasEnabledProjectRules($ruleContext->config); | ||
|
|
||
| // Stream only when no review/diff retains the base snapshot and every enabled rule tolerates per-unit release. | ||
| return ($reviewDiff === null || !$reviewDiff->active) | ||
| && !$options->hasChangedRegionMode() | ||
| && !$hasNarrowProjectRuleContext |
There was a problem hiding this comment.
Treat explicit project-root paths as full-project before disabling streaming.
$options->paths !== [] also matches analyse ., so a normal full-project invocation now falls off the streaming path purely because the root was passed explicitly. With the matching predicate in BranchReviewBuilder::shouldLoadProjectContext(), that can turn into an unnecessary second full-tree load/parse in legacy mode.
🤖 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 `@src/Command/AnalysisPipeline.php` around lines 137 - 143, The check that sets
$hasNarrowProjectRuleContext treats any explicit paths (e.g. analyse .) as
narrowing the project and disables streaming; update the predicate in
AnalysisPipeline.php so that only genuinely narrow paths (paths that are not the
project root) set $hasNarrowProjectRuleContext. Concretely, change the condition
using $options->paths to detect and ignore a single root path (or "."
equivalent) — or add/use a helper on the options object (e.g. isRootOnly or
similar) — so BranchReviewBuilder::shouldLoadProjectContext() still recognizes
full-project invocations and avoids an extra full-tree parse.
There was a problem hiding this comment.
Pull request overview
Release v0.3.1 updates the analyzer and tooling to improve test-quality detection, changed-region/diff accounting, dead-code indexing across Symfony YAML routes, and text output ergonomics—alongside the usual version/doc/test snapshot bumps.
Changes:
- Add advisory rule
test-quality.static-analysis-redundant-testand corresponding fixtures/tests. - Extend dead-code project indexing to treat Symfony YAML
_controller: FQCN::methodcallables as live references, including non-PHP source units. - Improve changed-region reporting (
suppressedCount/diff.suppressedCount) and lead text outputs with Composite/Findings tallies; update CLI fixtures/snapshots and agent hook scripts.
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Rule/TestQuality/TestQualityRulesTest.php | Refactors test-quality rule fixture assertions; adds coverage for the new static-analysis-redundant rule. |
| tests/Rule/RuleRegressionSnapshotTest.php | Updates regression snapshot counts/hash for new rules/fixtures. |
| tests/Rule/RuleRegistryTest.php | Registers the new rule in stable rule-id and definition snapshots. |
| tests/Rule/DeadCode/ProjectDeadCodeRulesTest.php | Adds project-wide fixtures/tests to ensure YAML _controller references keep controllers live; parses non-PHP units. |
| tests/Fixtures/TestQuality/static-analysis-redundant-test.php | New fixture exercising static declaration existence assertions and non-candidate cases. |
| tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php | Adds controller classes referenced (or intentionally not referenced) by YAML routes. |
| tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml | YAML route examples with quoted _controller values. |
| tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml | YAML route examples that should not be treated as FQCN controller references. |
| tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml | YAML inline mapping route example for _controller. |
| tests/Fixtures/DeadCode/project-wide/config/routes/block.yml | YAML block mapping route example for _controller. |
| tests/Fixtures/Cli/Golden/text-warning.txt | Updates golden text output (version + new top-of-report tallies). |
| tests/Fixtures/Cli/Golden/json-warning.json | Updates golden JSON output version. |
| tests/Console/ListRulesCliTest.php | Updates expected CLI version output. |
| tests/Console/GruffCliSummaryTest.php | Updates summary output assertions to match new formatting and version. |
| tests/Console/AnalyseCliTest.php | Updates analyse output assertions for version bump. |
| tests/Console/AnalyseCliDiffTest.php | Adds changed-region reconciliation tests and helper methods for JSON report assertions. |
| src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php | Implements the new advisory rule for same-file *_exists-style static existence assertions. |
| src/Rule/RuleRegistry.php | Registers the new rule; adds enabled project-rule id helper; allows project accumulators to optionally consume text units. |
| src/Rule/ProjectSourceTextRuleAccumulator.php | Introduces marker interface for project accumulators that need non-PHP units. |
| src/Rule/DeadCode/UnusedInternalClassRule.php | Marks unused-internal-class rule as needing text units so YAML can contribute references. |
| src/Rule/DeadCode/DeadCodeProjectIndex.php | Adds Symfony YAML _controller parsing and reference recording for dead-code indexing. |
| src/Reporting/TextReporter.php | Moves composite/findings tallies to the top; simplifies score/summary sections. |
| src/Diff/DiffResult.php | Adds suppressedCount to diff metadata serialization. |
| src/Console/Application.php | Bumps CLI version to 0.3.1. |
| src/Command/SummaryCommand.php | Updates summary text formatting (header + Composite/Findings lines). |
| src/Command/BranchReviewBuilder.php | Filters project-rule findings to requested file set in base snapshot comparisons. |
| src/Command/AnalysisPipeline.php | Disables streaming when narrowed paths require project-rule context; filters project-rule findings to requested file set. |
| src/Command/AnalysisFindingSupport.php | Adds helper to scope project-rule findings to the invocation’s requested files. |
| src/Command/AnalyseCommand.php | Threads diff suppressedCount into DiffResult for reporting. |
| docs/rules.md | Updates rule catalogue counts and severities; documents the new rule. |
| docs/output-formats.md | Documents suppressedCount / diff.suppressedCount semantics for changed-region reports. |
| composer.lock | Dev dependency version bumps for the release. |
| composer.json | Updates package description rule count to 133. |
| CHANGELOG.md | Adds 0.3.1 release notes. |
| .gruff-php.yaml | Enables the new test-quality rule by default. |
| .goat-flow/decisions/ADR-022-test-quality-gate-parity.md | Records rationale for the new advisory candidate rule. |
| .goat-flow/code-map.md | Updates code map metadata and references the new rule. |
| .goat-flow/architecture.md | Updates architecture doc rule counts and test-quality rule list. |
| .codex/hooks/gruff-code-quality.sh | Enhances hook path/range extraction, severity sorting/capping/flooring, and native changed-region delegation. |
| .claude/settings.json | Simplifies repo-root detection and makes gruff hook fail-soft when git is unavailable. |
| .claude/hooks/gruff-code-quality.sh | Mirrors the codex hook improvements for Claude environments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private function sourceFilePaths(AnalysisSourceSet $sources): array | ||
| { | ||
| return array_map( | ||
| static fn(SourceFile $sourceFile): string => $sourceFile->displayPath, | ||
| $sources->discovery->files, |
| private function sourceFilePaths(AnalysisSourceSet $sources): array | ||
| { | ||
| return array_map( | ||
| static fn(SourceFile $sourceFile): string => $sourceFile->displayPath, | ||
| $sources->discovery->files, |
| printf 'gruff-code-quality: %s %s changed-lines=%s; %s on changed lines: %s error, %s warning, %s advisory\n' \ | ||
| "$binary" "$rel_path" "$ranges" "$total" "$err" "$warn" "$adv" |
| printf 'gruff-code-quality: %s %s changed-lines=%s; %s on changed lines: %s error, %s warning, %s advisory\n' \ | ||
| "$binary" "$rel_path" "$ranges" "$total" "$err" "$warn" "$adv" |

Note
Medium Risk
Changes affect analysis pipeline scoping, project-wide dead-code reference indexing, and several test-quality severities that can fail CI at
--fail-on error; agent hooks are fail-soft but widely wired in settings.Overview
v0.3.1 bumps the CLI to 0.3.1 and ships analyzer, reporting, and agent-harness updates without JSON schema or config breaking changes.
The analyzer adds advisory
test-quality.static-analysis-redundant-test(same-file*_existsstyle assertions) and promotestest-quality.no-assertions,test-quality.sut-not-called, andtest-quality.tautological-type-assertionto error severity. Dead-code indexing now treats Symfony YAML_controllerFQCN::methodvalues as live references, and project-wide runs scope project-rule findings to requested files while disabling streaming when narrow paths need full project context. Changed-region JSON now carries reconciledsuppressedCount(alsodiff.suppressedCount). Textanalyse/ summary output leads with Composite and Findings tallies..claude/hooks/gruff-code-quality.shand.codex/hooks/gruff-code-quality.share aligned: multi-file path extraction, severity sort/floor/cap env vars,gruff-pynative changed-region flags, a--self-test=smokemode, and richer hook output..claude/settings.jsonresolves repo root withgit rev-parse --show-topleveland fail-soft skips for the gruff hook when git is unavailable.Docs, rule catalogue (133 rules), fixtures, golden outputs, and dev dependency lockfile bumps accompany the release.
Reviewed by Cursor Bugbot for commit 1db79ca. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation