Skip to content

v0.3.1#8

Open
mattyhansen wants to merge 4 commits into
mainfrom
dev
Open

v0.3.1#8
mattyhansen wants to merge 4 commits into
mainfrom
dev

Conversation

@mattyhansen
Copy link
Copy Markdown
Contributor

@mattyhansen mattyhansen commented Jun 3, 2026

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 *_exists style assertions) and promotes test-quality.no-assertions, test-quality.sut-not-called, and test-quality.tautological-type-assertion to error severity. Dead-code indexing now treats Symfony YAML _controller FQCN::method values 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 reconciled suppressedCount (also diff.suppressedCount). Text analyse / summary output leads with Composite and Findings tallies.

.claude/hooks/gruff-code-quality.sh and .codex/hooks/gruff-code-quality.sh are aligned: multi-file path extraction, severity sort/floor/cap env vars, gruff-py native changed-region flags, a --self-test=smoke mode, and richer hook output. .claude/settings.json resolves repo root with git rev-parse --show-toplevel and 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

    • New advisory rule flagging redundant static-existence assertions in same-file tests.
    • Analyzer can delegate changed-region scoping to supported tools; added a smoke self-test.
  • Bug Fixes

    • More robust git-root detection, timeout/exit handling, and analyzer-output error reporting.
    • Better ignore-path handling and staged-change path discovery.
  • Improvements

    • Severity-ranked, capped per-line finding reports with min-severity floor.
    • Expanded file-extension support, consolidated text/json summaries, and Symfony YAML dead-code indexing.
  • Documentation

    • Rule catalogue updated: 132 → 133 rules; adjusted test-quality severities.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

Adds 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.

Changes

Static-analysis-redundant-test rule and hook script refactoring

Layer / File(s) Summary
Payload and scope extraction refactoring
.claude/hooks/gruff-code-quality.sh, .codex/hooks/gruff-code-quality.sh
Multi-path jq-first JSON extraction (json_file_paths) replaces single-path logic; regex fallback (fallback_file_paths) supports jq-less environments; staged changes are included via git diff --cached; gruff-ts extension mapping expanded to mts/cts/mjs/cjs.
Severity ranking and findings reporting
.claude/hooks/gruff-code-quality.sh, .codex/hooks/gruff-code-quality.sh
min_severity_rank maps env-configurable floors; changed_findings_report builds structured JSON with severity-sorted, capped listings and counts; native_suppressed_count reads analyzer suppression when available; ignore descriptors are merged; print_scope_header standardizes headers.
Analyzer invocation and timeout handling
.claude/hooks/gruff-code-quality.sh, .codex/hooks/gruff-code-quality.sh
run_gruff_json detects native changed-region support and passes --no-baseline --changed-ranges --changed-scope symbol when supported; validates GRUFF_CODE_QUALITY_TIMEOUT_SECONDS and treats common kill/timeout exit codes specially.
Process file orchestration with new reporting
.claude/hooks/gruff-code-quality.sh, .codex/hooks/gruff-code-quality.sh
process_file uses native-regions detection, invokes run_gruff_json(binary,ranges), falls back .yaml.yml, includes exit status in JSON validation diagnostics, replaces filter_findings with changed_findings_report, and selects suppressed-count source based on native mode.
Hook self-testing and main arg handling
.claude/hooks/gruff-code-quality.sh, .codex/hooks/gruff-code-quality.sh
Adds self_test() exercising jq, payload/range parsing, severity ordering, caps/flooring, and native-vs-fallback scoping; main() accepts --self-test=smoke.
Documentation and runtime contracts
.claude/hooks/gruff-code-quality.sh, .codex/hooks/gruff-code-quality.sh
Comments updated to reflect multi-config support, expanded extensions, native scoping ownership, and the new JSON reporting contract.
Settings git root detection and failure handling
.claude/settings.json
Use git rev-parse --show-toplevel; PreToolUse now blocks with exit 2 if root unavailable; PostToolUse hooks skip gracefully with exit 0 when root cannot be determined.
StaticAnalysisRedundantTestRule implementation
src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php
New advisory rule indexes same-file class-like declarations and members, detects assertTrue-wrapped existence checks resolved via ::class or member literals, and emits advisory findings for statically provable redundant assertions.
Rule registration and fixture setup
src/Rule/RuleRegistry.php, tests/Fixtures/TestQuality/static-analysis-redundant-test.php
Registers the new rule in defaults and adds a fixture with classes, interface, trait, enum, factory, and PHPUnit tests exercising redundant and behavioral assertions.
Rule test coverage and snapshot updates
tests/Rule/RuleRegistryTest.php, tests/Rule/RuleRegressionSnapshotTest.php, tests/Rule/TestQuality/TestQualityRulesTest.php
Adds tests and providers validating redundant candidates, metadata (variant/evidenceSymbol), severity/confidence, non-overlap with neighboring rules, and updates registry/regression snapshots and hashes.
Text reporter and summary alignment
src/Reporting/TextReporter.php, src/Command/SummaryCommand.php
TextReporter emits tool/version, conditional Composite line, and a consolidated Findings totals line at the top; SummaryCommand formatting updated to match.
Golden fixtures and CLI tests
tests/Fixtures/Cli/Golden/*, tests/Console/*
Golden text/JSON fixtures and CLI tests updated to expect version 0.3.1 and the new header/findings layout; tests strengthened to assert absence of per-finding lines.
Dead-code YAML indexing and tests
src/Rule/DeadCode/DeadCodeProjectIndex.php, src/Rule/ProjectSourceTextRuleAccumulator.php, src/Rule/DeadCode/UnusedInternalClassRule.php, tests/Fixtures/DeadCode/..., tests/Rule/DeadCode/ProjectDeadCodeRulesTest.php
DeadCodeProjectIndex parses Symfony YAML _controller entries for FQCN::method shapes, records class references, adds a marker interface for project-source-text accumulators, updates UnusedInternalClassRule signature to implement it, and adds YAML route fixtures plus tests asserting referenced controllers remain live.
Diff suppressedCount and legacy pipeline filtering
src/Diff/DiffResult.php, src/Command/AnalyseCommand.php, src/Command/AnalysisFindingSupport.php, src/Command/AnalysisPipeline.php, src/Command/BranchReviewBuilder.php
DiffResult carries suppressedCount; AnalyseCommand persists it; AnalysisFindingSupport adds filterProjectRuleFindingsToFiles; AnalysisPipeline/BranchReviewBuilder use sourceFilePaths and apply filtering to project-rule findings when CLI paths are specified.
Analyse CLI changed-ranges tests
tests/Console/AnalyseCliDiffTest.php
Adds tests and helpers to validate changed-ranges reconciliation for intra-file and project-wide scenarios and asserts suppressed/diff-suppressed counts.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

In burrowed code I nibbled through the churn,
Found assertions redundant where symbols already turn.
Hooks gather paths from streams both wide and sly,
Rank findings, cap their lists, and let suppressed counts fly —
A rabbit hops, reports in JSON bright, and grins. 🐇✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'v0.3.1' is extremely vague and does not clearly convey the primary changes in this substantial release. Consider using a more descriptive title such as 'Release v0.3.1: Add static-analysis-redundant-test rule and enhance gruff-code-quality hooks' to better summarize the main changes.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread .claude/hooks/gruff-code-quality.sh
Comment thread .claude/hooks/gruff-code-quality.sh
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
.codex/hooks/gruff-code-quality.sh (1)

1-857: 💤 Low value

Consider 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 like bin/ 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

📥 Commits

Reviewing files that changed from the base of the PR and between 17baeae and f55f5ee.

📒 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.yaml
  • composer.json
  • docs/rules.md
  • src/Command/SummaryCommand.php
  • src/Reporting/TextReporter.php
  • src/Rule/RuleRegistry.php
  • src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php
  • tests/Console/GruffCliSummaryTest.php
  • tests/Fixtures/Cli/Golden/text-warning.txt
  • tests/Fixtures/TestQuality/static-analysis-redundant-test.php
  • tests/Rule/RuleRegistryTest.php
  • tests/Rule/RuleRegressionSnapshotTest.php
  • tests/Rule/TestQuality/TestQualityRulesTest.php

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread .claude/hooks/gruff-code-quality.sh
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +125 to +128
foreach (NodeIndex::nodesOfAny(
$analysisUnit,
[Stmt\Class_::class, Stmt\Interface_::class, Stmt\Trait_::class, Stmt\Enum_::class],
) as $node) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/Console/GruffCliSummaryTest.php (1)

40-40: ⚡ Quick win

Use Application::VERSION instead 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

📥 Commits

Reviewing files that changed from the base of the PR and between f55f5ee and f6b30ae.

⛔ Files ignored due to path filters (1)
  • composer.lock is excluded by !**/*.lock
📒 Files selected for processing (19)
  • CHANGELOG.md
  • src/Console/Application.php
  • src/Rule/DeadCode/DeadCodeProjectIndex.php
  • src/Rule/DeadCode/UnusedInternalClassRule.php
  • src/Rule/ProjectSourceTextRuleAccumulator.php
  • src/Rule/RuleRegistry.php
  • tests/Console/AnalyseCliTest.php
  • tests/Console/GruffCliSummaryTest.php
  • tests/Console/ListRulesCliTest.php
  • tests/Fixtures/Cli/Golden/json-warning.json
  • tests/Fixtures/Cli/Golden/text-warning.txt
  • tests/Fixtures/DeadCode/project-wide/config/routes/block.yml
  • tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml
  • tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml
  • tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml
  • tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php
  • tests/Rule/DeadCode/ProjectDeadCodeRulesTest.php
  • tests/Rule/RuleRegressionSnapshotTest.php
  • tests/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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between f6b30ae and 1db79ca.

📒 Files selected for processing (10)
  • CHANGELOG.md
  • docs/output-formats.md
  • src/Command/AnalyseCommand.php
  • src/Command/AnalysisFindingSupport.php
  • src/Command/AnalysisPipeline.php
  • src/Command/BranchReviewBuilder.php
  • src/Diff/DiffResult.php
  • src/Rule/DeadCode/DeadCodeProjectIndex.php
  • src/Rule/RuleRegistry.php
  • tests/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

Comment on lines +79 to +80
if ($projectRuleIds === [] || $filePaths === []) {
return $findings;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Comment on lines +137 to +143
$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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-test and corresponding fixtures/tests.
  • Extend dead-code project indexing to treat Symfony YAML _controller: FQCN::method callables 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.

Comment on lines +310 to +314
private function sourceFilePaths(AnalysisSourceSet $sources): array
{
return array_map(
static fn(SourceFile $sourceFile): string => $sourceFile->displayPath,
$sources->discovery->files,
Comment on lines +334 to +338
private function sourceFilePaths(AnalysisSourceSet $sources): array
{
return array_map(
static fn(SourceFile $sourceFile): string => $sourceFile->displayPath,
$sources->discovery->files,
Comment on lines +695 to +696
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"
Comment on lines +695 to +696
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"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants