From f6b4f88730cdd8a9eaeb45ad29472522a6a38086 Mon Sep 17 00:00:00 2001 From: Matthew Hansen Date: Wed, 3 Jun 2026 06:55:08 +1000 Subject: [PATCH 1/5] Add static-analysis-redundant test rule and update related configurations --- .claude/hooks/gruff-code-quality.sh | 353 +++++++++++++++--- .claude/settings.json | 8 +- .codex/hooks/gruff-code-quality.sh | 353 +++++++++++++++--- .goat-flow/architecture.md | 6 +- .goat-flow/code-map.md | 3 +- .../ADR-022-test-quality-gate-parity.md | 11 +- .gruff-php.yaml | 2 + composer.json | 2 +- src/Command/SummaryCommand.php | 20 +- src/Reporting/TextReporter.php | 46 ++- src/Rule/RuleRegistry.php | 2 + .../StaticAnalysisRedundantTestRule.php | 352 +++++++++++++++++ tests/Console/GruffCliSummaryTest.php | 21 +- tests/Fixtures/Cli/Golden/text-warning.txt | 6 +- .../static-analysis-redundant-test.php | 87 +++++ tests/Rule/RuleRegistryTest.php | 6 +- tests/Rule/RuleRegressionSnapshotTest.php | 6 +- .../Rule/TestQuality/TestQualityRulesTest.php | 88 +++++ 18 files changed, 1189 insertions(+), 183 deletions(-) create mode 100644 src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php create mode 100644 tests/Fixtures/TestQuality/static-analysis-redundant-test.php diff --git a/.claude/hooks/gruff-code-quality.sh b/.claude/hooks/gruff-code-quality.sh index 7ed7d545..2cf90da6 100755 --- a/.claude/hooks/gruff-code-quality.sh +++ b/.claude/hooks/gruff-code-quality.sh @@ -10,7 +10,7 @@ # same file. # # Supported analyzers: -# - gruff-ts for .ts / .tsx / .js / .jsx +# - gruff-ts for .ts / .tsx / .mts / .cts / .js / .jsx / .mjs / .cjs # - gruff-php for .php # - gruff-go for .go # - gruff-rs for .rs @@ -20,8 +20,8 @@ # Payload is read from stdin as agent PostToolUse JSON. The hook prefers # an edited file path from the payload, then falls back to git-changed # supported files for runtimes that only expose the completed file tool -# event. It also needs a matching `.gruff-*.yaml` config at the repo root, -# a matching gruff binary, and `jq` for JSON filtering. Missing +# event. It also needs a matching `.gruff-*.yaml` or `.gruff-*.yml` config at +# the repo root, a matching gruff binary, and `jq` for JSON filtering. Missing # prerequisites fail soft: the edit is not blocked and whole-file gruff # output is not printed as a fallback. # @@ -30,12 +30,26 @@ # Otherwise parse `git diff --unified=0 -- ` for tracked files. # New/untracked files are treated as fully changed. If no range can be # derived, the hook exits quietly apart from a short stderr diagnostic. +# Analyzers with native changed-region support own the filtering: gruff-py is +# invoked with `--changed-ranges`, `--changed-scope symbol`, and `--no-baseline` +# so symbol-aware scope is used and adoption baselines do not hide agent +# feedback. All other analyzers use the portable primary-line fallback above. +# Either way the surfaced findings are severity-sorted, floored, and capped +# identically. # # Output: -# Prints `[severity] path:line rule - message` for findings whose -# primary reported line intersects the changed ranges, then one compact -# suppressed-count line for same-file findings outside those ranges. -# The playbook footer is printed only when at least one changed-line +# Prints a scope/tally header +# `gruff-code-quality: changed-lines=; on changed +# lines: error, warning, advisory`, then one canonical finding line +# per surfaced finding `- [severity] file:line ruleId - message` (matching +# CONTRACT.md's normative per-finding line so hook and native CLI output read +# identically). Findings on changed lines are sorted error -> warning -> +# advisory so the highest-value land first; they are floored at +# GRUFF_CODE_QUALITY_MIN_SEVERITY (default advisory) and capped at +# GRUFF_CODE_QUALITY_MAX_FINDINGS (default 20) with a "( more on changed +# lines)" note when the cap hides some. A trailing line reports findings dropped +# below the floor and the count of same-file findings outside the changed +# ranges. The playbook footer is printed only when at least one changed-line # finding is shown. If the analyzer reports the edited file as ignored by # its `paths.ignore` config, the hook instead prints a single # `skipped - out of scope` line and surfaces no findings, so the @@ -46,7 +60,15 @@ set -euo pipefail FOOTER="For triage: consult .goat-flow/skill-playbooks/gruff-code-quality.md" SUPPORTED_TOOLS=" edit write multiedit write_to_file replace_file_content multi_replace_file_content " -SKIP_DIR_PATTERN='(^|/)(node_modules|vendor|\.goat-flow|dist|build|coverage|\.git)(/|$)' +SKIP_DIR_PATTERN='(^|/)(node_modules|vendor|\.goat-flow|dist|build|coverage|\.git|target|\.venv|\.mypy_cache|\.pytest_cache|\.ruff_cache)(/|$)' +GRUFF_CODE_QUALITY_TIMEOUT_SECONDS="${GRUFF_CODE_QUALITY_TIMEOUT_SECONDS:-30}" +# Max changed-line findings listed per file before the rest are summarised as +# "( more on changed lines)". Keeps a large edit from flooding the agent. +GRUFF_CODE_QUALITY_MAX_FINDINGS="${GRUFF_CODE_QUALITY_MAX_FINDINGS:-20}" +# Lowest severity surfaced on changed lines (advisory|warning|error). Findings +# below it are counted, not listed - a project that only wants the agent pushed on +# warning+ sets this to `warning`. Default `advisory` keeps every finding visible. +GRUFF_CODE_QUALITY_MIN_SEVERITY="${GRUFF_CODE_QUALITY_MIN_SEVERITY:-advisory}" # Payload extraction stays jq-first for correctness but keeps small regex # fallbacks so unsupported tools and paths can still be skipped when jq is @@ -79,34 +101,52 @@ json_tool_name() { ' } -json_file_path() { +json_file_paths() { local input="$1" json_field "$input" ' - def path_from(value): + def string_path_fields(value): + if (value | type) == "object" then + [ + value.file_path?, + value.filePath?, + value.path?, + value.AbsolutePath?, + value.absolutePath?, + value.TargetFile?, + value.targetFile?, + value.FilePath?, + value.SearchPath?, + value.searchPath? + ] + else + [] + end; + def paths_from(value): if value == null then empty + elif (value | type) == "array" then + value[] | paths_from(.) elif (value | type) == "object" then - (value.file_path // value.path // value.AbsolutePath // value.TargetFile // value.FilePath // value.SearchPath // empty) + (string_path_fields(value)[]?), + (value.files? | paths_from(.)), + (value.paths? | paths_from(.)), + (value.edits? | paths_from(.)), + (value.changes? | paths_from(.)), + (value.operations? | paths_from(.)) elif (value | type) == "string" then - ((value | fromjson? // {}) - | if type == "object" then - (.file_path // .path // .AbsolutePath // .TargetFile // .FilePath // .SearchPath // empty) - else - empty - end) + (try (value | fromjson | paths_from(.)) catch value) else empty end; [ - .tool_input.file_path, - .tool_input.path, - path_from(.toolCall.args), - path_from(.toolArgs), - path_from(.tool_args), - .file_path, - .path - ] | map(select(type == "string" and length > 0)) | first + paths_from(.tool_input), + paths_from(.toolCall.args), + paths_from(.toolArgs), + paths_from(.tool_args), + paths_from(.result), + paths_from(.) + ] | map(select(type == "string" and length > 0)) | unique | .[] ' } @@ -121,12 +161,12 @@ fallback_tool_name() { fi } -fallback_file_path() { +fallback_file_paths() { local input="$1" if [[ "$input" =~ \"file_path\"[[:space:]]*:[[:space:]]*\"([^\"]+)\" ]]; then - printf '%s' "${BASH_REMATCH[1]}" + printf '%s\n' "${BASH_REMATCH[1]}" elif [[ "$input" =~ \"path\"[[:space:]]*:[[:space:]]*\"([^\"]+)\" ]]; then - printf '%s' "${BASH_REMATCH[1]}" + printf '%s\n' "${BASH_REMATCH[1]}" fi } @@ -164,7 +204,7 @@ absolute_path() { variant_for_path() { local file_path="$1" case "${file_path##*.}" in - ts|tsx|js|jsx) printf 'gruff-ts' ;; + ts|tsx|mts|cts|js|jsx|mjs|cjs) printf 'gruff-ts' ;; php) printf 'gruff-php' ;; go) printf 'gruff-go' ;; rs) printf 'gruff-rs' ;; @@ -187,6 +227,7 @@ git_changed_supported_paths() { local rel_path { git -C "$root" diff --name-only --diff-filter=ACMR -- 2>/dev/null || true + git -C "$root" diff --cached --name-only --diff-filter=ACMR -- 2>/dev/null || true git -C "$root" ls-files --others --exclude-standard -- 2>/dev/null || true } | while IFS= read -r rel_path; do if supported_candidate_path "$rel_path"; then @@ -198,11 +239,11 @@ git_changed_supported_paths() { file_paths_for_payload() { local payload="$1" local root="$2" - local file_path - file_path="$(json_file_path "$payload")" - [[ -n "$file_path" ]] || file_path="$(fallback_file_path "$payload")" - if [[ -n "$file_path" ]]; then - printf '%s\n' "$file_path" + local paths + paths="$(json_file_paths "$payload" || true)" + [[ -n "$paths" ]] || paths="$(fallback_file_paths "$payload")" + if [[ -n "$paths" ]]; then + printf '%s\n' "$paths" | awk 'length($0) && !seen[$0]++' return fi git_changed_supported_paths "$root" @@ -335,9 +376,87 @@ changed_ranges() { git_diff_ranges "$root" "$rel_path" "$abs_path" } +self_test() { + local payload paths ranges variant report_output report_json first_line + if ! command -v jq >/dev/null 2>&1; then + printf 'gruff-code-quality self-test: jq unavailable\n' >&2 + return 1 + fi + + payload='{"tool_name":"MultiEdit","tool_input":{"edits":[{"file_path":"src/a.mts"},{"path":"src/b.php"}],"changed_ranges":[{"startLine":2,"endLine":4}]}}' + paths="$(json_file_paths "$payload")" + [[ "$paths" == *"src/a.mts"* && "$paths" == *"src/b.php"* ]] || { + printf 'gruff-code-quality self-test: path extraction failed: %s\n' "$paths" >&2 + return 1 + } + ranges="$(payload_ranges "$payload")" + [[ "$ranges" == "2-4" ]] || { + printf 'gruff-code-quality self-test: range extraction failed: %s\n' "$ranges" >&2 + return 1 + } + variant="$(variant_for_path "src/a.mts")" + [[ "$variant" == "gruff-ts" ]] || { + printf 'gruff-code-quality self-test: variant mapping failed: %s\n' "$variant" >&2 + return 1 + } + + [[ "$(min_severity_rank warning)" == "2" && "$(min_severity_rank error)" == "3" && "$(min_severity_rank bogus)" == "1" ]] || { + printf 'gruff-code-quality self-test: min_severity_rank mapping failed\n' >&2 + return 1 + } + + report_output='{"findings":[{"severity":"advisory","line":2,"file":"x.ts","ruleId":"a.one","message":"m1"},{"severity":"error","line":3,"file":"x.ts","ruleId":"z.two","message":"m2"},{"severity":"warning","line":4,"file":"x.ts","ruleId":"m.three","message":"m3"}]}' + report_json="$(changed_findings_report "$report_output" "x.ts" "/tmp/x.ts" "2-4" 1 2)" + first_line="$(printf '%s' "$report_json" | jq -r '.lines[0]')" + [[ "$first_line" == "- [error] x.ts:3 z.two - m2" ]] || { + printf 'gruff-code-quality self-test: severity sort failed: %s\n' "$first_line" >&2 + return 1 + } + [[ "$(printf '%s' "$report_json" | jq -r '.total')" == "3" && "$(printf '%s' "$report_json" | jq -r '.more')" == "1" ]] || { + printf 'gruff-code-quality self-test: volume cap failed\n' >&2 + return 1 + } + report_json="$(changed_findings_report "$report_output" "x.ts" "/tmp/x.ts" "2-4" 2 20 0)" + [[ "$(printf '%s' "$report_json" | jq -r '.surfaced')" == "2" && "$(printf '%s' "$report_json" | jq -r '.floored')" == "1" ]] || { + printf 'gruff-code-quality self-test: severity floor failed\n' >&2 + return 1 + } + + # Native mode (analyzer owns scoping) surfaces a finding outside the literal + # changed range; the portable fallback filters that same finding out. + report_output='{"findings":[{"severity":"warning","line":99,"file":"x.ts","ruleId":"r.one","message":"m"}]}' + report_json="$(changed_findings_report "$report_output" "x.ts" "/tmp/x.ts" "2-4" 1 20 1)" + [[ "$(printf '%s' "$report_json" | jq -r '.total')" == "1" ]] || { + printf 'gruff-code-quality self-test: native scope bypass failed\n' >&2 + return 1 + } + report_json="$(changed_findings_report "$report_output" "x.ts" "/tmp/x.ts" "2-4" 1 20 0)" + [[ "$(printf '%s' "$report_json" | jq -r '.total')" == "0" ]] || { + printf 'gruff-code-quality self-test: fallback range filter failed\n' >&2 + return 1 + } + + printf 'gruff-code-quality self-test: ok\n' +} + +# An analyzer "owns" changed-region filtering when it can scope the scan itself. +# Only gruff-py advertises the symbol-aware trio (`--changed-ranges`, +# `--changed-scope`, `--no-baseline`); when present the hook delegates scoping to +# it instead of filtering by primary line. Any other binary uses the fallback. +supports_native_changed_regions() { + local binary="$1" + local help="$2" + [[ "$binary" == "gruff-py" ]] || return 1 + [[ "$help" == *"--changed-ranges"* ]] || return 1 + [[ "$help" == *"--changed-scope"* ]] || return 1 + [[ "$help" == *"--no-baseline"* ]] || return 1 +} + # Analyzer invocation adapts to the two flag families currently used by the # gruff CLIs: long GNU-style flags (`--format json`) and Go-style single-dash -# flags (`-format json`). Findings never cause a non-zero hook exit. +# flags (`-format json`). When the binary owns changed-region scoping the hook +# passes `--no-baseline --changed-ranges --changed-scope symbol`. +# Findings never cause a non-zero hook exit. analyse_help() { local binary_path="$1" "$binary_path" analyse --help 2>&1 || true @@ -352,21 +471,31 @@ run_gruff_json() { local binary_path="$1" local help="$2" local file_path="$3" - local args + local binary="$4" + local ranges="$5" + local args timeout_seconds args=(analyse) if [[ "$help" == *"--format"* ]]; then args+=(--format json) if [[ "$help" == *"--fail-on"* ]]; then args+=(--fail-on none) fi + if supports_native_changed_regions "$binary" "$help"; then + args+=(--no-baseline --changed-ranges "$ranges" --changed-scope symbol) + fi elif [[ "$help" == *"-format"* ]]; then args+=(-format json) else return 64 fi + timeout_seconds="$GRUFF_CODE_QUALITY_TIMEOUT_SECONDS" + if ! [[ "$timeout_seconds" =~ ^[0-9]+$ ]] || [[ "$timeout_seconds" -lt 1 ]]; then + timeout_seconds=30 + fi + if command -v timeout >/dev/null 2>&1; then - timeout 30 "$binary_path" "${args[@]}" "$file_path" 2>&1 + timeout "$timeout_seconds" "$binary_path" "${args[@]}" "$file_path" 2>&1 return $? fi "$binary_path" "${args[@]}" "$file_path" 2>&1 @@ -377,15 +506,36 @@ valid_gruff_json() { printf '%s' "$output" | jq -e 'type == "object" and (.findings | type == "array")' >/dev/null 2>&1 } -# Report filtering accepts the JSON shapes emitted across gruff-ts, gruff-go, -# gruff-php, gruff-py, and gruff-rs: path may be `filePath`, `file`, or -# `path`; line may be `line`, `location.line`, or `location.startLine`. -filter_findings() { +# Map a min-severity name to its rank (advisory=1, warning=2, error=3). Any +# unrecognised value (or empty) floors at advisory, the default - the hook never +# hides findings because of a typo in GRUFF_CODE_QUALITY_MIN_SEVERITY. +min_severity_rank() { + case "${1,,}" in + warning) printf '2' ;; + error) printf '3' ;; + *) printf '1' ;; + esac +} + +# Build a single JSON control object describing the changed-line findings: +# { total, e, w, a, surfaced, floored, more, lines } +# `total`/`e`/`w`/`a` count every finding whose primary line intersects the +# changed ranges, by severity. `lines` holds the canonical +# `- [severity] file:line ruleId - message` rows for the findings that survive the +# severity floor (rank >= $floor_rank), sorted error -> warning -> advisory then +# file/line/ruleId, capped at $max; `more` is how many surfaced findings the cap +# hid and `floored` how many were dropped below the floor. Accepts the JSON shapes +# emitted across all five ports: path may be `filePath`, `file`, or `path`; line +# may be `line`, `location.line`, or `location.startLine`. +changed_findings_report() { local output="$1" local rel_path="$2" local abs_path="$3" local ranges="$4" - printf '%s' "$output" | jq -r --arg rel "$rel_path" --arg abs "$abs_path" --arg ranges "$ranges" ' + local floor_rank="$5" + local max="$6" + local native="${7:-0}" + printf '%s' "$output" | jq -c --arg rel "$rel_path" --arg abs "$abs_path" --arg ranges "$ranges" --argjson floor_rank "$floor_rank" --argjson max "$max" --argjson native "$native" ' def normalize_path: tostring | gsub("\\\\"; "/") | sub("^\\./"; ""); def finding_path: @@ -414,12 +564,29 @@ filter_findings() { def in_changed_ranges($line): parsed_ranges as $parsed | any($parsed[]; $line >= .start and $line <= .end); + def sev_rank($s): + if $s == "error" then 3 elif $s == "warning" then 2 elif $s == "advisory" then 1 else 0 end; - (.findings // []) - | map(. as $finding | ($finding | line_or_null) as $line | select(($finding | same_file) and $line != null and in_changed_ranges($line))) - | .[] - | line_or_null as $line - | "[\(.severity // "unknown")] \(finding_path):\($line) \(.ruleId // "unknown-rule") - \(.message // "")" + [ (.findings // [])[] + | . as $finding + | ($finding | line_or_null) as $line + | select(($finding | same_file) and $line != null and ($native == 1 or in_changed_ranges($line))) + | { sev: (.severity // "unknown"), + rank: sev_rank(.severity // ""), + line: $line, + file: ($finding | finding_path), + ruleId: (.ruleId // "unknown-rule"), + message: (.message // "") } ] as $all + | ($all | sort_by([ (3 - .rank), .file, .line, .ruleId ])) as $sorted + | [ $sorted[] | select(.rank >= $floor_rank) ] as $surfaced + | { total: ($all | length), + e: ([ $all[] | select(.sev == "error") ] | length), + w: ([ $all[] | select(.sev == "warning") ] | length), + a: ([ $all[] | select(.sev == "advisory") ] | length), + surfaced: ($surfaced | length), + floored: (($all | length) - ($surfaced | length)), + more: (if ($surfaced | length) > $max then ($surfaced | length) - $max else 0 end), + lines: [ limit($max; $surfaced[]) | "- [\(.sev)] \(.file):\(.line) \(.ruleId) - \(.message)" ] } ' 2>/dev/null || true } @@ -469,6 +636,16 @@ suppressed_count() { ' 2>/dev/null || printf '0' } +# When the analyzer owns changed-region scoping, it reports how many findings it +# suppressed as out-of-scope in its own output; read that count rather than +# re-deriving it. Falls back to 0 when the field is absent. +native_suppressed_count() { + local output="$1" + printf '%s' "$output" | jq -r ' + (.suppressedCount? // .diff.suppressedCount? // 0) + ' 2>/dev/null || printf '0' +} + # When the analyzer reports the edited file as ignored by its config # (`paths.ignore`), return a short human descriptor (for example # "ignored by gruff config (matched *.css)") so the hook can tell the agent the @@ -496,9 +673,9 @@ ignored_descriptor() { or $n == ("./" + ($rel | normalize_path)) or ($n | endswith("/" + ($rel | normalize_path)))); - ((.paths.ignoredPaths? // .ignoredPaths? // .paths.skipped? // [])) + ((.paths.ignoredPaths? // []) + (.ignoredPaths? // []) + (.paths.skipped? // [])) | map(select(is_match(entry_path))) - | first + | ((map(select(entry_detail | length > 0)) | first) // first) | if . == null then empty else (entry_detail) as $d | if ($d | length) > 0 then "ignored by gruff config (matched \($d))" @@ -507,12 +684,26 @@ ignored_descriptor() { ' 2>/dev/null || true } +print_scope_header() { + local binary="$1" + local rel_path="$2" + local ranges="$3" + local total="$4" + local err="$5" + local warn="$6" + local adv="$7" + 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" +} + process_file() { local payload="$1" local root="$2" local file_path="$3" local rel_path abs_path binary binary_path config_file - local ranges help output status changed_output suppressed ignored_desc + local ranges help output status suppressed ignored_desc uses_native_regions + local max_findings floor_rank report_json scope_fields + local total err warn adv surfaced floored more [[ -n "$file_path" ]] || return 0 [[ "$file_path" =~ $SKIP_DIR_PATTERN ]] && return 0 @@ -526,6 +717,9 @@ process_file() { binary="$(variant_for_path "$rel_path" || true)" [[ -n "$binary" ]] || return 0 config_file="$root/.${binary}.yaml" + if [[ ! -f "$config_file" ]]; then + config_file="$root/.${binary}.yml" + fi [[ -f "$config_file" ]] || return 0 binary_path="$(discover_binary "$root" "$binary")" @@ -547,14 +741,18 @@ process_file() { printf 'gruff-code-quality: %s does not expose JSON output; changed-line filtering skipped\n' "$binary" >&2 return 0 fi + uses_native_regions=0 + if supports_native_changed_regions "$binary" "$help"; then + uses_native_regions=1 + fi set +e - output="$(run_gruff_json "$binary_path" "$help" "$rel_path")" + output="$(run_gruff_json "$binary_path" "$help" "$rel_path" "$binary" "$ranges")" status=$? set -e - if [[ "$status" -eq 124 ]]; then - printf 'gruff-code-quality: %s crashed or timed out\n' "$binary" >&2 + if [[ "$status" -eq 124 || "$status" -eq 137 ]]; then + printf 'gruff-code-quality: %s exceeded %ss or was killed; changed-line filtering skipped\n' "$binary" "$GRUFF_CODE_QUALITY_TIMEOUT_SECONDS" >&2 return 0 fi if [[ -z "$output" ]]; then @@ -573,7 +771,7 @@ process_file() { printf '%s\n' "$output" | awk 'NR <= 12 { print " " $0 }' return 0 fi - printf 'gruff-code-quality: %s produced non-JSON output; changed-line filtering skipped\n' "$binary" >&2 + printf 'gruff-code-quality: %s exited %s with non-JSON output; changed-line filtering skipped\n' "$binary" "$status" >&2 return 0 fi @@ -584,22 +782,50 @@ process_file() { # bypass `paths.ignore` for explicitly-passed files. ignored_desc="$(ignored_descriptor "$output" "$rel_path" "$abs_path")" if [[ -n "$ignored_desc" ]]; then - printf 'gruff-code-quality: skipped %s - %s; out of scope, do not modify to satisfy gruff.\n' "$rel_path" "$ignored_desc" + printf 'gruff-code-quality: skipped %s %s - %s; out of scope, do not modify to satisfy gruff.\n' "$binary" "$rel_path" "$ignored_desc" return 0 fi # MVP range model: enforce findings whose primary line intersects edited lines. # Wider function-block expansion is deferred unless an analyzer reports new - # method findings only on unchanged declaration lines. - changed_output="$(filter_findings "$output" "$rel_path" "$abs_path" "$ranges")" - suppressed="$(suppressed_count "$output" "$rel_path" "$abs_path" "$ranges")" - if [[ -n "$changed_output" ]]; then - printf '%s\n' "$changed_output" + # method findings only on unchanged declaration lines. Surfaced findings are + # severity-sorted (error first), floored at GRUFF_CODE_QUALITY_MIN_SEVERITY, and + # capped at GRUFF_CODE_QUALITY_MAX_FINDINGS. + max_findings="$GRUFF_CODE_QUALITY_MAX_FINDINGS" + [[ "$max_findings" =~ ^[0-9]+$ && "$max_findings" -ge 1 ]] || max_findings=20 + floor_rank="$(min_severity_rank "$GRUFF_CODE_QUALITY_MIN_SEVERITY")" + + report_json="$(changed_findings_report "$output" "$rel_path" "$abs_path" "$ranges" "$floor_rank" "$max_findings" "$uses_native_regions")" + [[ -n "$report_json" ]] || report_json='{"total":0,"e":0,"w":0,"a":0,"surfaced":0,"floored":0,"more":0,"lines":[]}' + if [[ "$uses_native_regions" -eq 1 ]]; then + suppressed="$(native_suppressed_count "$output")" + else + suppressed="$(suppressed_count "$output" "$rel_path" "$abs_path" "$ranges")" + fi + + scope_fields="$(printf '%s' "$report_json" | jq -r '[.total,.e,.w,.a,.surfaced,.floored,.more] | @tsv' 2>/dev/null || true)" + IFS=$'\t' read -r total err warn adv surfaced floored more <<< "$scope_fields" + [[ "$total" =~ ^[0-9]+$ ]] || total=0 + [[ "$surfaced" =~ ^[0-9]+$ ]] || surfaced=0 + [[ "$floored" =~ ^[0-9]+$ ]] || floored=0 + [[ "$more" =~ ^[0-9]+$ ]] || more=0 + + if [[ "$total" -gt 0 || ( "$suppressed" =~ ^[0-9]+$ && "$suppressed" -gt 0 ) ]]; then + print_scope_header "$binary" "$rel_path" "$ranges" "$total" "$err" "$warn" "$adv" + fi + if [[ "$surfaced" -gt 0 ]]; then + printf '%s' "$report_json" | jq -r '.lines[]' 2>/dev/null || true + fi + if [[ "$more" -gt 0 ]]; then + printf 'gruff-code-quality: (%s more on changed lines; raise GRUFF_CODE_QUALITY_MAX_FINDINGS to list them)\n' "$more" + fi + if [[ "$floored" -gt 0 ]]; then + printf 'gruff-code-quality: %s finding(s) below GRUFF_CODE_QUALITY_MIN_SEVERITY=%s not listed\n' "$floored" "${GRUFF_CODE_QUALITY_MIN_SEVERITY:-advisory}" fi if [[ "$suppressed" =~ ^[0-9]+$ && "$suppressed" -gt 0 ]]; then printf 'gruff-code-quality: suppressed %s pre-existing finding(s) outside changed lines\n' "$suppressed" fi - if [[ -n "$changed_output" ]]; then + if [[ "$surfaced" -gt 0 ]]; then printf '%s\n' "$FOOTER" fi return 0 @@ -608,6 +834,11 @@ process_file() { main() { local payload tool_name root file_path local -a file_paths + if [[ "${1:-}" == "--self-test=smoke" ]]; then + self_test + exit $? + fi + payload="$(read_stdin)" tool_name="$(json_tool_name "$payload")" [[ -n "$tool_name" ]] || tool_name="$(fallback_tool_name "$payload")" diff --git a/.claude/settings.json b/.claude/settings.json index 5ebca0f1..7ebc79ef 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -72,7 +72,7 @@ "hooks": [ { "type": "command", - "command": "gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\" || { printf 'BLOCKED: Guard cannot start: git repository root unavailable.\\n' >&2; exit 2; }; case \"$gcd\" in /*) root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel)\" ;; esac; bash \"$root/.claude/hooks/deny-dangerous.sh\"" + "command": "root=\"$(git rev-parse --show-toplevel 2>/dev/null)\" || { printf 'BLOCKED: Guard cannot start: git repository root unavailable.\\n' >&2; exit 2; }; bash \"$root/.claude/hooks/deny-dangerous.sh\"" } ] } @@ -83,7 +83,7 @@ "hooks": [ { "type": "command", - "command": "gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\" || { printf 'BLOCKED: Guard cannot start: git repository root unavailable.\\n' >&2; exit 2; }; case \"$gcd\" in /*) root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel)\" ;; esac; bash \"$root/.claude/hooks/gruff-code-quality.sh\"" + "command": "root=\"$(git rev-parse --show-toplevel 2>/dev/null)\" || { printf 'gruff-code-quality: git repository root unavailable; skipping\\n' >&2; exit 0; }; bash \"$root/.claude/hooks/gruff-code-quality.sh\"" } ] }, @@ -92,7 +92,7 @@ "hooks": [ { "type": "command", - "command": "gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\" || { printf 'BLOCKED: Guard cannot start: git repository root unavailable.\\n' >&2; exit 2; }; case \"$gcd\" in /*) root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel)\" ;; esac; bash \"$root/.claude/hooks/gruff-code-quality.sh\"" + "command": "root=\"$(git rev-parse --show-toplevel 2>/dev/null)\" || { printf 'gruff-code-quality: git repository root unavailable; skipping\\n' >&2; exit 0; }; bash \"$root/.claude/hooks/gruff-code-quality.sh\"" } ] }, @@ -101,7 +101,7 @@ "hooks": [ { "type": "command", - "command": "gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\" || { printf 'BLOCKED: Guard cannot start: git repository root unavailable.\\n' >&2; exit 2; }; case \"$gcd\" in /*) root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel)\" ;; esac; bash \"$root/.claude/hooks/gruff-code-quality.sh\"" + "command": "root=\"$(git rev-parse --show-toplevel 2>/dev/null)\" || { printf 'gruff-code-quality: git repository root unavailable; skipping\\n' >&2; exit 0; }; bash \"$root/.claude/hooks/gruff-code-quality.sh\"" } ] } diff --git a/.codex/hooks/gruff-code-quality.sh b/.codex/hooks/gruff-code-quality.sh index 7ed7d545..2cf90da6 100755 --- a/.codex/hooks/gruff-code-quality.sh +++ b/.codex/hooks/gruff-code-quality.sh @@ -10,7 +10,7 @@ # same file. # # Supported analyzers: -# - gruff-ts for .ts / .tsx / .js / .jsx +# - gruff-ts for .ts / .tsx / .mts / .cts / .js / .jsx / .mjs / .cjs # - gruff-php for .php # - gruff-go for .go # - gruff-rs for .rs @@ -20,8 +20,8 @@ # Payload is read from stdin as agent PostToolUse JSON. The hook prefers # an edited file path from the payload, then falls back to git-changed # supported files for runtimes that only expose the completed file tool -# event. It also needs a matching `.gruff-*.yaml` config at the repo root, -# a matching gruff binary, and `jq` for JSON filtering. Missing +# event. It also needs a matching `.gruff-*.yaml` or `.gruff-*.yml` config at +# the repo root, a matching gruff binary, and `jq` for JSON filtering. Missing # prerequisites fail soft: the edit is not blocked and whole-file gruff # output is not printed as a fallback. # @@ -30,12 +30,26 @@ # Otherwise parse `git diff --unified=0 -- ` for tracked files. # New/untracked files are treated as fully changed. If no range can be # derived, the hook exits quietly apart from a short stderr diagnostic. +# Analyzers with native changed-region support own the filtering: gruff-py is +# invoked with `--changed-ranges`, `--changed-scope symbol`, and `--no-baseline` +# so symbol-aware scope is used and adoption baselines do not hide agent +# feedback. All other analyzers use the portable primary-line fallback above. +# Either way the surfaced findings are severity-sorted, floored, and capped +# identically. # # Output: -# Prints `[severity] path:line rule - message` for findings whose -# primary reported line intersects the changed ranges, then one compact -# suppressed-count line for same-file findings outside those ranges. -# The playbook footer is printed only when at least one changed-line +# Prints a scope/tally header +# `gruff-code-quality: changed-lines=; on changed +# lines: error, warning, advisory`, then one canonical finding line +# per surfaced finding `- [severity] file:line ruleId - message` (matching +# CONTRACT.md's normative per-finding line so hook and native CLI output read +# identically). Findings on changed lines are sorted error -> warning -> +# advisory so the highest-value land first; they are floored at +# GRUFF_CODE_QUALITY_MIN_SEVERITY (default advisory) and capped at +# GRUFF_CODE_QUALITY_MAX_FINDINGS (default 20) with a "( more on changed +# lines)" note when the cap hides some. A trailing line reports findings dropped +# below the floor and the count of same-file findings outside the changed +# ranges. The playbook footer is printed only when at least one changed-line # finding is shown. If the analyzer reports the edited file as ignored by # its `paths.ignore` config, the hook instead prints a single # `skipped - out of scope` line and surfaces no findings, so the @@ -46,7 +60,15 @@ set -euo pipefail FOOTER="For triage: consult .goat-flow/skill-playbooks/gruff-code-quality.md" SUPPORTED_TOOLS=" edit write multiedit write_to_file replace_file_content multi_replace_file_content " -SKIP_DIR_PATTERN='(^|/)(node_modules|vendor|\.goat-flow|dist|build|coverage|\.git)(/|$)' +SKIP_DIR_PATTERN='(^|/)(node_modules|vendor|\.goat-flow|dist|build|coverage|\.git|target|\.venv|\.mypy_cache|\.pytest_cache|\.ruff_cache)(/|$)' +GRUFF_CODE_QUALITY_TIMEOUT_SECONDS="${GRUFF_CODE_QUALITY_TIMEOUT_SECONDS:-30}" +# Max changed-line findings listed per file before the rest are summarised as +# "( more on changed lines)". Keeps a large edit from flooding the agent. +GRUFF_CODE_QUALITY_MAX_FINDINGS="${GRUFF_CODE_QUALITY_MAX_FINDINGS:-20}" +# Lowest severity surfaced on changed lines (advisory|warning|error). Findings +# below it are counted, not listed - a project that only wants the agent pushed on +# warning+ sets this to `warning`. Default `advisory` keeps every finding visible. +GRUFF_CODE_QUALITY_MIN_SEVERITY="${GRUFF_CODE_QUALITY_MIN_SEVERITY:-advisory}" # Payload extraction stays jq-first for correctness but keeps small regex # fallbacks so unsupported tools and paths can still be skipped when jq is @@ -79,34 +101,52 @@ json_tool_name() { ' } -json_file_path() { +json_file_paths() { local input="$1" json_field "$input" ' - def path_from(value): + def string_path_fields(value): + if (value | type) == "object" then + [ + value.file_path?, + value.filePath?, + value.path?, + value.AbsolutePath?, + value.absolutePath?, + value.TargetFile?, + value.targetFile?, + value.FilePath?, + value.SearchPath?, + value.searchPath? + ] + else + [] + end; + def paths_from(value): if value == null then empty + elif (value | type) == "array" then + value[] | paths_from(.) elif (value | type) == "object" then - (value.file_path // value.path // value.AbsolutePath // value.TargetFile // value.FilePath // value.SearchPath // empty) + (string_path_fields(value)[]?), + (value.files? | paths_from(.)), + (value.paths? | paths_from(.)), + (value.edits? | paths_from(.)), + (value.changes? | paths_from(.)), + (value.operations? | paths_from(.)) elif (value | type) == "string" then - ((value | fromjson? // {}) - | if type == "object" then - (.file_path // .path // .AbsolutePath // .TargetFile // .FilePath // .SearchPath // empty) - else - empty - end) + (try (value | fromjson | paths_from(.)) catch value) else empty end; [ - .tool_input.file_path, - .tool_input.path, - path_from(.toolCall.args), - path_from(.toolArgs), - path_from(.tool_args), - .file_path, - .path - ] | map(select(type == "string" and length > 0)) | first + paths_from(.tool_input), + paths_from(.toolCall.args), + paths_from(.toolArgs), + paths_from(.tool_args), + paths_from(.result), + paths_from(.) + ] | map(select(type == "string" and length > 0)) | unique | .[] ' } @@ -121,12 +161,12 @@ fallback_tool_name() { fi } -fallback_file_path() { +fallback_file_paths() { local input="$1" if [[ "$input" =~ \"file_path\"[[:space:]]*:[[:space:]]*\"([^\"]+)\" ]]; then - printf '%s' "${BASH_REMATCH[1]}" + printf '%s\n' "${BASH_REMATCH[1]}" elif [[ "$input" =~ \"path\"[[:space:]]*:[[:space:]]*\"([^\"]+)\" ]]; then - printf '%s' "${BASH_REMATCH[1]}" + printf '%s\n' "${BASH_REMATCH[1]}" fi } @@ -164,7 +204,7 @@ absolute_path() { variant_for_path() { local file_path="$1" case "${file_path##*.}" in - ts|tsx|js|jsx) printf 'gruff-ts' ;; + ts|tsx|mts|cts|js|jsx|mjs|cjs) printf 'gruff-ts' ;; php) printf 'gruff-php' ;; go) printf 'gruff-go' ;; rs) printf 'gruff-rs' ;; @@ -187,6 +227,7 @@ git_changed_supported_paths() { local rel_path { git -C "$root" diff --name-only --diff-filter=ACMR -- 2>/dev/null || true + git -C "$root" diff --cached --name-only --diff-filter=ACMR -- 2>/dev/null || true git -C "$root" ls-files --others --exclude-standard -- 2>/dev/null || true } | while IFS= read -r rel_path; do if supported_candidate_path "$rel_path"; then @@ -198,11 +239,11 @@ git_changed_supported_paths() { file_paths_for_payload() { local payload="$1" local root="$2" - local file_path - file_path="$(json_file_path "$payload")" - [[ -n "$file_path" ]] || file_path="$(fallback_file_path "$payload")" - if [[ -n "$file_path" ]]; then - printf '%s\n' "$file_path" + local paths + paths="$(json_file_paths "$payload" || true)" + [[ -n "$paths" ]] || paths="$(fallback_file_paths "$payload")" + if [[ -n "$paths" ]]; then + printf '%s\n' "$paths" | awk 'length($0) && !seen[$0]++' return fi git_changed_supported_paths "$root" @@ -335,9 +376,87 @@ changed_ranges() { git_diff_ranges "$root" "$rel_path" "$abs_path" } +self_test() { + local payload paths ranges variant report_output report_json first_line + if ! command -v jq >/dev/null 2>&1; then + printf 'gruff-code-quality self-test: jq unavailable\n' >&2 + return 1 + fi + + payload='{"tool_name":"MultiEdit","tool_input":{"edits":[{"file_path":"src/a.mts"},{"path":"src/b.php"}],"changed_ranges":[{"startLine":2,"endLine":4}]}}' + paths="$(json_file_paths "$payload")" + [[ "$paths" == *"src/a.mts"* && "$paths" == *"src/b.php"* ]] || { + printf 'gruff-code-quality self-test: path extraction failed: %s\n' "$paths" >&2 + return 1 + } + ranges="$(payload_ranges "$payload")" + [[ "$ranges" == "2-4" ]] || { + printf 'gruff-code-quality self-test: range extraction failed: %s\n' "$ranges" >&2 + return 1 + } + variant="$(variant_for_path "src/a.mts")" + [[ "$variant" == "gruff-ts" ]] || { + printf 'gruff-code-quality self-test: variant mapping failed: %s\n' "$variant" >&2 + return 1 + } + + [[ "$(min_severity_rank warning)" == "2" && "$(min_severity_rank error)" == "3" && "$(min_severity_rank bogus)" == "1" ]] || { + printf 'gruff-code-quality self-test: min_severity_rank mapping failed\n' >&2 + return 1 + } + + report_output='{"findings":[{"severity":"advisory","line":2,"file":"x.ts","ruleId":"a.one","message":"m1"},{"severity":"error","line":3,"file":"x.ts","ruleId":"z.two","message":"m2"},{"severity":"warning","line":4,"file":"x.ts","ruleId":"m.three","message":"m3"}]}' + report_json="$(changed_findings_report "$report_output" "x.ts" "/tmp/x.ts" "2-4" 1 2)" + first_line="$(printf '%s' "$report_json" | jq -r '.lines[0]')" + [[ "$first_line" == "- [error] x.ts:3 z.two - m2" ]] || { + printf 'gruff-code-quality self-test: severity sort failed: %s\n' "$first_line" >&2 + return 1 + } + [[ "$(printf '%s' "$report_json" | jq -r '.total')" == "3" && "$(printf '%s' "$report_json" | jq -r '.more')" == "1" ]] || { + printf 'gruff-code-quality self-test: volume cap failed\n' >&2 + return 1 + } + report_json="$(changed_findings_report "$report_output" "x.ts" "/tmp/x.ts" "2-4" 2 20 0)" + [[ "$(printf '%s' "$report_json" | jq -r '.surfaced')" == "2" && "$(printf '%s' "$report_json" | jq -r '.floored')" == "1" ]] || { + printf 'gruff-code-quality self-test: severity floor failed\n' >&2 + return 1 + } + + # Native mode (analyzer owns scoping) surfaces a finding outside the literal + # changed range; the portable fallback filters that same finding out. + report_output='{"findings":[{"severity":"warning","line":99,"file":"x.ts","ruleId":"r.one","message":"m"}]}' + report_json="$(changed_findings_report "$report_output" "x.ts" "/tmp/x.ts" "2-4" 1 20 1)" + [[ "$(printf '%s' "$report_json" | jq -r '.total')" == "1" ]] || { + printf 'gruff-code-quality self-test: native scope bypass failed\n' >&2 + return 1 + } + report_json="$(changed_findings_report "$report_output" "x.ts" "/tmp/x.ts" "2-4" 1 20 0)" + [[ "$(printf '%s' "$report_json" | jq -r '.total')" == "0" ]] || { + printf 'gruff-code-quality self-test: fallback range filter failed\n' >&2 + return 1 + } + + printf 'gruff-code-quality self-test: ok\n' +} + +# An analyzer "owns" changed-region filtering when it can scope the scan itself. +# Only gruff-py advertises the symbol-aware trio (`--changed-ranges`, +# `--changed-scope`, `--no-baseline`); when present the hook delegates scoping to +# it instead of filtering by primary line. Any other binary uses the fallback. +supports_native_changed_regions() { + local binary="$1" + local help="$2" + [[ "$binary" == "gruff-py" ]] || return 1 + [[ "$help" == *"--changed-ranges"* ]] || return 1 + [[ "$help" == *"--changed-scope"* ]] || return 1 + [[ "$help" == *"--no-baseline"* ]] || return 1 +} + # Analyzer invocation adapts to the two flag families currently used by the # gruff CLIs: long GNU-style flags (`--format json`) and Go-style single-dash -# flags (`-format json`). Findings never cause a non-zero hook exit. +# flags (`-format json`). When the binary owns changed-region scoping the hook +# passes `--no-baseline --changed-ranges --changed-scope symbol`. +# Findings never cause a non-zero hook exit. analyse_help() { local binary_path="$1" "$binary_path" analyse --help 2>&1 || true @@ -352,21 +471,31 @@ run_gruff_json() { local binary_path="$1" local help="$2" local file_path="$3" - local args + local binary="$4" + local ranges="$5" + local args timeout_seconds args=(analyse) if [[ "$help" == *"--format"* ]]; then args+=(--format json) if [[ "$help" == *"--fail-on"* ]]; then args+=(--fail-on none) fi + if supports_native_changed_regions "$binary" "$help"; then + args+=(--no-baseline --changed-ranges "$ranges" --changed-scope symbol) + fi elif [[ "$help" == *"-format"* ]]; then args+=(-format json) else return 64 fi + timeout_seconds="$GRUFF_CODE_QUALITY_TIMEOUT_SECONDS" + if ! [[ "$timeout_seconds" =~ ^[0-9]+$ ]] || [[ "$timeout_seconds" -lt 1 ]]; then + timeout_seconds=30 + fi + if command -v timeout >/dev/null 2>&1; then - timeout 30 "$binary_path" "${args[@]}" "$file_path" 2>&1 + timeout "$timeout_seconds" "$binary_path" "${args[@]}" "$file_path" 2>&1 return $? fi "$binary_path" "${args[@]}" "$file_path" 2>&1 @@ -377,15 +506,36 @@ valid_gruff_json() { printf '%s' "$output" | jq -e 'type == "object" and (.findings | type == "array")' >/dev/null 2>&1 } -# Report filtering accepts the JSON shapes emitted across gruff-ts, gruff-go, -# gruff-php, gruff-py, and gruff-rs: path may be `filePath`, `file`, or -# `path`; line may be `line`, `location.line`, or `location.startLine`. -filter_findings() { +# Map a min-severity name to its rank (advisory=1, warning=2, error=3). Any +# unrecognised value (or empty) floors at advisory, the default - the hook never +# hides findings because of a typo in GRUFF_CODE_QUALITY_MIN_SEVERITY. +min_severity_rank() { + case "${1,,}" in + warning) printf '2' ;; + error) printf '3' ;; + *) printf '1' ;; + esac +} + +# Build a single JSON control object describing the changed-line findings: +# { total, e, w, a, surfaced, floored, more, lines } +# `total`/`e`/`w`/`a` count every finding whose primary line intersects the +# changed ranges, by severity. `lines` holds the canonical +# `- [severity] file:line ruleId - message` rows for the findings that survive the +# severity floor (rank >= $floor_rank), sorted error -> warning -> advisory then +# file/line/ruleId, capped at $max; `more` is how many surfaced findings the cap +# hid and `floored` how many were dropped below the floor. Accepts the JSON shapes +# emitted across all five ports: path may be `filePath`, `file`, or `path`; line +# may be `line`, `location.line`, or `location.startLine`. +changed_findings_report() { local output="$1" local rel_path="$2" local abs_path="$3" local ranges="$4" - printf '%s' "$output" | jq -r --arg rel "$rel_path" --arg abs "$abs_path" --arg ranges "$ranges" ' + local floor_rank="$5" + local max="$6" + local native="${7:-0}" + printf '%s' "$output" | jq -c --arg rel "$rel_path" --arg abs "$abs_path" --arg ranges "$ranges" --argjson floor_rank "$floor_rank" --argjson max "$max" --argjson native "$native" ' def normalize_path: tostring | gsub("\\\\"; "/") | sub("^\\./"; ""); def finding_path: @@ -414,12 +564,29 @@ filter_findings() { def in_changed_ranges($line): parsed_ranges as $parsed | any($parsed[]; $line >= .start and $line <= .end); + def sev_rank($s): + if $s == "error" then 3 elif $s == "warning" then 2 elif $s == "advisory" then 1 else 0 end; - (.findings // []) - | map(. as $finding | ($finding | line_or_null) as $line | select(($finding | same_file) and $line != null and in_changed_ranges($line))) - | .[] - | line_or_null as $line - | "[\(.severity // "unknown")] \(finding_path):\($line) \(.ruleId // "unknown-rule") - \(.message // "")" + [ (.findings // [])[] + | . as $finding + | ($finding | line_or_null) as $line + | select(($finding | same_file) and $line != null and ($native == 1 or in_changed_ranges($line))) + | { sev: (.severity // "unknown"), + rank: sev_rank(.severity // ""), + line: $line, + file: ($finding | finding_path), + ruleId: (.ruleId // "unknown-rule"), + message: (.message // "") } ] as $all + | ($all | sort_by([ (3 - .rank), .file, .line, .ruleId ])) as $sorted + | [ $sorted[] | select(.rank >= $floor_rank) ] as $surfaced + | { total: ($all | length), + e: ([ $all[] | select(.sev == "error") ] | length), + w: ([ $all[] | select(.sev == "warning") ] | length), + a: ([ $all[] | select(.sev == "advisory") ] | length), + surfaced: ($surfaced | length), + floored: (($all | length) - ($surfaced | length)), + more: (if ($surfaced | length) > $max then ($surfaced | length) - $max else 0 end), + lines: [ limit($max; $surfaced[]) | "- [\(.sev)] \(.file):\(.line) \(.ruleId) - \(.message)" ] } ' 2>/dev/null || true } @@ -469,6 +636,16 @@ suppressed_count() { ' 2>/dev/null || printf '0' } +# When the analyzer owns changed-region scoping, it reports how many findings it +# suppressed as out-of-scope in its own output; read that count rather than +# re-deriving it. Falls back to 0 when the field is absent. +native_suppressed_count() { + local output="$1" + printf '%s' "$output" | jq -r ' + (.suppressedCount? // .diff.suppressedCount? // 0) + ' 2>/dev/null || printf '0' +} + # When the analyzer reports the edited file as ignored by its config # (`paths.ignore`), return a short human descriptor (for example # "ignored by gruff config (matched *.css)") so the hook can tell the agent the @@ -496,9 +673,9 @@ ignored_descriptor() { or $n == ("./" + ($rel | normalize_path)) or ($n | endswith("/" + ($rel | normalize_path)))); - ((.paths.ignoredPaths? // .ignoredPaths? // .paths.skipped? // [])) + ((.paths.ignoredPaths? // []) + (.ignoredPaths? // []) + (.paths.skipped? // [])) | map(select(is_match(entry_path))) - | first + | ((map(select(entry_detail | length > 0)) | first) // first) | if . == null then empty else (entry_detail) as $d | if ($d | length) > 0 then "ignored by gruff config (matched \($d))" @@ -507,12 +684,26 @@ ignored_descriptor() { ' 2>/dev/null || true } +print_scope_header() { + local binary="$1" + local rel_path="$2" + local ranges="$3" + local total="$4" + local err="$5" + local warn="$6" + local adv="$7" + 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" +} + process_file() { local payload="$1" local root="$2" local file_path="$3" local rel_path abs_path binary binary_path config_file - local ranges help output status changed_output suppressed ignored_desc + local ranges help output status suppressed ignored_desc uses_native_regions + local max_findings floor_rank report_json scope_fields + local total err warn adv surfaced floored more [[ -n "$file_path" ]] || return 0 [[ "$file_path" =~ $SKIP_DIR_PATTERN ]] && return 0 @@ -526,6 +717,9 @@ process_file() { binary="$(variant_for_path "$rel_path" || true)" [[ -n "$binary" ]] || return 0 config_file="$root/.${binary}.yaml" + if [[ ! -f "$config_file" ]]; then + config_file="$root/.${binary}.yml" + fi [[ -f "$config_file" ]] || return 0 binary_path="$(discover_binary "$root" "$binary")" @@ -547,14 +741,18 @@ process_file() { printf 'gruff-code-quality: %s does not expose JSON output; changed-line filtering skipped\n' "$binary" >&2 return 0 fi + uses_native_regions=0 + if supports_native_changed_regions "$binary" "$help"; then + uses_native_regions=1 + fi set +e - output="$(run_gruff_json "$binary_path" "$help" "$rel_path")" + output="$(run_gruff_json "$binary_path" "$help" "$rel_path" "$binary" "$ranges")" status=$? set -e - if [[ "$status" -eq 124 ]]; then - printf 'gruff-code-quality: %s crashed or timed out\n' "$binary" >&2 + if [[ "$status" -eq 124 || "$status" -eq 137 ]]; then + printf 'gruff-code-quality: %s exceeded %ss or was killed; changed-line filtering skipped\n' "$binary" "$GRUFF_CODE_QUALITY_TIMEOUT_SECONDS" >&2 return 0 fi if [[ -z "$output" ]]; then @@ -573,7 +771,7 @@ process_file() { printf '%s\n' "$output" | awk 'NR <= 12 { print " " $0 }' return 0 fi - printf 'gruff-code-quality: %s produced non-JSON output; changed-line filtering skipped\n' "$binary" >&2 + printf 'gruff-code-quality: %s exited %s with non-JSON output; changed-line filtering skipped\n' "$binary" "$status" >&2 return 0 fi @@ -584,22 +782,50 @@ process_file() { # bypass `paths.ignore` for explicitly-passed files. ignored_desc="$(ignored_descriptor "$output" "$rel_path" "$abs_path")" if [[ -n "$ignored_desc" ]]; then - printf 'gruff-code-quality: skipped %s - %s; out of scope, do not modify to satisfy gruff.\n' "$rel_path" "$ignored_desc" + printf 'gruff-code-quality: skipped %s %s - %s; out of scope, do not modify to satisfy gruff.\n' "$binary" "$rel_path" "$ignored_desc" return 0 fi # MVP range model: enforce findings whose primary line intersects edited lines. # Wider function-block expansion is deferred unless an analyzer reports new - # method findings only on unchanged declaration lines. - changed_output="$(filter_findings "$output" "$rel_path" "$abs_path" "$ranges")" - suppressed="$(suppressed_count "$output" "$rel_path" "$abs_path" "$ranges")" - if [[ -n "$changed_output" ]]; then - printf '%s\n' "$changed_output" + # method findings only on unchanged declaration lines. Surfaced findings are + # severity-sorted (error first), floored at GRUFF_CODE_QUALITY_MIN_SEVERITY, and + # capped at GRUFF_CODE_QUALITY_MAX_FINDINGS. + max_findings="$GRUFF_CODE_QUALITY_MAX_FINDINGS" + [[ "$max_findings" =~ ^[0-9]+$ && "$max_findings" -ge 1 ]] || max_findings=20 + floor_rank="$(min_severity_rank "$GRUFF_CODE_QUALITY_MIN_SEVERITY")" + + report_json="$(changed_findings_report "$output" "$rel_path" "$abs_path" "$ranges" "$floor_rank" "$max_findings" "$uses_native_regions")" + [[ -n "$report_json" ]] || report_json='{"total":0,"e":0,"w":0,"a":0,"surfaced":0,"floored":0,"more":0,"lines":[]}' + if [[ "$uses_native_regions" -eq 1 ]]; then + suppressed="$(native_suppressed_count "$output")" + else + suppressed="$(suppressed_count "$output" "$rel_path" "$abs_path" "$ranges")" + fi + + scope_fields="$(printf '%s' "$report_json" | jq -r '[.total,.e,.w,.a,.surfaced,.floored,.more] | @tsv' 2>/dev/null || true)" + IFS=$'\t' read -r total err warn adv surfaced floored more <<< "$scope_fields" + [[ "$total" =~ ^[0-9]+$ ]] || total=0 + [[ "$surfaced" =~ ^[0-9]+$ ]] || surfaced=0 + [[ "$floored" =~ ^[0-9]+$ ]] || floored=0 + [[ "$more" =~ ^[0-9]+$ ]] || more=0 + + if [[ "$total" -gt 0 || ( "$suppressed" =~ ^[0-9]+$ && "$suppressed" -gt 0 ) ]]; then + print_scope_header "$binary" "$rel_path" "$ranges" "$total" "$err" "$warn" "$adv" + fi + if [[ "$surfaced" -gt 0 ]]; then + printf '%s' "$report_json" | jq -r '.lines[]' 2>/dev/null || true + fi + if [[ "$more" -gt 0 ]]; then + printf 'gruff-code-quality: (%s more on changed lines; raise GRUFF_CODE_QUALITY_MAX_FINDINGS to list them)\n' "$more" + fi + if [[ "$floored" -gt 0 ]]; then + printf 'gruff-code-quality: %s finding(s) below GRUFF_CODE_QUALITY_MIN_SEVERITY=%s not listed\n' "$floored" "${GRUFF_CODE_QUALITY_MIN_SEVERITY:-advisory}" fi if [[ "$suppressed" =~ ^[0-9]+$ && "$suppressed" -gt 0 ]]; then printf 'gruff-code-quality: suppressed %s pre-existing finding(s) outside changed lines\n' "$suppressed" fi - if [[ -n "$changed_output" ]]; then + if [[ "$surfaced" -gt 0 ]]; then printf '%s\n' "$FOOTER" fi return 0 @@ -608,6 +834,11 @@ process_file() { main() { local payload tool_name root file_path local -a file_paths + if [[ "${1:-}" == "--self-test=smoke" ]]; then + self_test + exit $? + fi + payload="$(read_stdin)" tool_name="$(json_tool_name "$payload")" [[ -n "$tool_name" ]] || tool_name="$(fallback_tool_name "$payload")" diff --git a/.goat-flow/architecture.md b/.goat-flow/architecture.md index 5c996fcf..a7eb136a 100644 --- a/.goat-flow/architecture.md +++ b/.goat-flow/architecture.md @@ -1,6 +1,6 @@ # Architecture - gruff-php -Last reviewed 2026-06-01. All claims map to a real file in `src/`, `tests/`, or top-level config; cross-check before broadening any of them. +Last reviewed 2026-06-03. All claims map to a real file in `src/`, `tests/`, or top-level config; cross-check before broadening any of them. ## System Overview @@ -57,7 +57,7 @@ Static finding baselines default to `gruff-baseline.json` at the project root: ` ## Rule Catalogue -The default registry-backed static rule set covers 11 emitted pillars (`Size`, `Complexity`, `Maintainability`, `DeadCode`, `Naming`, `Documentation`, `Modernisation`, `Security`, `SensitiveData`, `TestQuality`, `Design`) and currently exposes 132 rule ids through `list-rules --format json`. `waste.*` rule ids are historical names that emit either `DeadCode` or `Maintainability` findings. Infection ingestion can also emit `Mutation` pillar findings. All emitted rules are tier `v0.1`; `Coupling` and `Architecture` remain reserved. +The default registry-backed static rule set covers 11 emitted pillars (`Size`, `Complexity`, `Maintainability`, `DeadCode`, `Naming`, `Documentation`, `Modernisation`, `Security`, `SensitiveData`, `TestQuality`, `Design`) and currently exposes 133 rule ids through `list-rules --format json`. `waste.*` rule ids are historical names that emit either `DeadCode` or `Maintainability` findings. Infection ingestion can also emit `Mutation` pillar findings. All emitted rules are tier `v0.1`; `Coupling` and `Architecture` remain reserved. | Family | Rule ids | Notes | | --- | --- | --- | @@ -70,7 +70,7 @@ The default registry-backed static rule set covers 11 emitted pillars (`Size`, ` | Modernisation | `modernisation.constructor-promotion-candidate`, `modernisation.enum-candidate`, `modernisation.first-class-callable-candidate`, `modernisation.forbidden-global-access`, `modernisation.match-expression-candidate`, `modernisation.mixed-type-overuse`, `modernisation.named-argument-opportunity`, `modernisation.phpdoc-mixed-overuse`, `modernisation.public-property`, `modernisation.readonly-property-candidate` | PHP-version-gated opportunity checks where syntax support matters; no autofix behavior; `modernisation.phpdoc-mixed-overuse` covers PHPDoc contracts that signatures cannot express; `ModernisationNodeHelper` is shared infrastructure | | Security | `security.dangerous-function-call`, `security.disabled-ssl-verification`, `security.error-suppression`, `security.extract-compact-user-input`, `security.github-actions-risky-workflow`, `security.header-injection`, `security.insecure-random`, `security.path-traversal-file-access`, `security.process-command-construction`, `security.request-controlled-url`, `security.sensitive-data-logging`, `security.silent-catch`, `security.sql-concatenation`, `security.unsafe-archive-extraction`, `security.unsafe-xml-loading`, `security.unsafe-unserialize`, `security.variable-include`, `security.weak-crypto` | Mostly heuristic AST checks; `security.github-actions-risky-workflow` is a source-text workflow YAML check scoped to `.github/workflows`; `SecurityNodeHelper` is shared infrastructure | | SensitiveData | `sensitive-data.api-key-pattern`, `sensitive-data.aws-access-key`, `sensitive-data.database-url-password`, `sensitive-data.gcp-service-account-key`, `sensitive-data.hardcoded-env-value`, `sensitive-data.high-entropy-string`, `sensitive-data.jwt-token`, `sensitive-data.phi-pattern`, `sensitive-data.pii-test-fixture`, `sensitive-data.private-key`, `sensitive-data.url-credentials` | All implement `SourceTextRuleInterface`, so they also scan JSON/YAML/INI/.env-style files; provider/token findings carry deterministic redacted previews, and `SecretScannerHelper` is shared infrastructure | -| TestQuality | Source-test rules: `test-quality.no-assertions`, `test-quality.trivial-assertion`, `test-quality.conditional-logic`, `test-quality.loop-assertion-without-message`, `test-quality.test-longer-than-sut`, `test-quality.test-method-too-long`, `test-quality.eager-test`, `test-quality.mystery-guest`, `test-quality.excessive-mocking`, `test-quality.mock-only-test`, `test-quality.mock-without-expectation`, `test-quality.mocking-domain-object`, `test-quality.multiple-aaa-cycles`, `test-quality.unused-mock`, `test-quality.sleep-in-test`, `test-quality.naming-consistency`, `test-quality.magic-number-assertion`, `test-quality.private-reflection`, `test-quality.data-provider-annotation`, `test-quality.empty-data-provider`, `test-quality.trivial-snapshot`, `test-quality.sut-not-called`, `test-quality.setup-bloat`, `test-quality.skipped-without-reason`, `test-quality.extends-production-class`, `test-quality.tautological-type-assertion`, `test-quality.testdox-readability`, `test-quality.exception-type-only`, `test-quality.global-state-mutation`, `test-quality.repeated-structure-missing-data-provider`. `test-quality.mocking-domain-object` is enabled but emits only when `domainNamespaces` patterns are configured. Project-config rules (one finding per analyse run, read from `phpunit.xml`/`phpunit.xml.dist`/`phpunit.dist.xml`): `test-quality.phpunit-strict-flags-missing`, `test-quality.phpunit-deprecations-not-fatal`, `test-quality.phpunit-coverage-source-missing`. PHPUnit/Pest AST heuristics scoped to detected test methods or closures; confidence labels identify noisier smells; the `error` hard-gates are the "this test proves nothing" signals — `test-quality.no-assertions`, `test-quality.sut-not-called`, `test-quality.tautological-type-assertion`, `test-quality.empty-data-provider`, and `test-quality.extends-production-class` (ADR-022) — while the style/ceremony smells stay warning/advisory; `TestQualityNodeHelper` is shared infrastructure | +| TestQuality | Source-test rules: `test-quality.no-assertions`, `test-quality.trivial-assertion`, `test-quality.conditional-logic`, `test-quality.loop-assertion-without-message`, `test-quality.test-longer-than-sut`, `test-quality.test-method-too-long`, `test-quality.eager-test`, `test-quality.mystery-guest`, `test-quality.excessive-mocking`, `test-quality.mock-only-test`, `test-quality.mock-without-expectation`, `test-quality.mocking-domain-object`, `test-quality.multiple-aaa-cycles`, `test-quality.unused-mock`, `test-quality.sleep-in-test`, `test-quality.naming-consistency`, `test-quality.magic-number-assertion`, `test-quality.private-reflection`, `test-quality.data-provider-annotation`, `test-quality.empty-data-provider`, `test-quality.trivial-snapshot`, `test-quality.sut-not-called`, `test-quality.setup-bloat`, `test-quality.skipped-without-reason`, `test-quality.extends-production-class`, `test-quality.tautological-type-assertion`, `test-quality.static-analysis-redundant-test`, `test-quality.testdox-readability`, `test-quality.exception-type-only`, `test-quality.global-state-mutation`, `test-quality.repeated-structure-missing-data-provider`. `test-quality.mocking-domain-object` is enabled but emits only when `domainNamespaces` patterns are configured. Project-config rules (one finding per analyse run, read from `phpunit.xml`/`phpunit.xml.dist`/`phpunit.dist.xml`): `test-quality.phpunit-strict-flags-missing`, `test-quality.phpunit-deprecations-not-fatal`, `test-quality.phpunit-coverage-source-missing`. PHPUnit/Pest AST heuristics scoped to detected test methods or closures; confidence labels identify noisier smells; the `error` hard-gates are the "this test proves nothing" signals — `test-quality.no-assertions`, `test-quality.sut-not-called`, `test-quality.tautological-type-assertion`, `test-quality.empty-data-provider`, and `test-quality.extends-production-class` (ADR-022) — while shape-only candidate tests, style, and ceremony smells stay warning/advisory; `TestQualityNodeHelper` is shared infrastructure | | Design | `design.single-implementor-interface` | Project rule that flags internal interfaces with one implementor and no external type-hint usage | | Mutation | `mutation.survived-mutant`, `mutation.budget-exceeded`, `mutation.msi-regression` | Not registry-backed static rules; emitted only from optional Infection JSON ingestion | diff --git a/.goat-flow/code-map.md b/.goat-flow/code-map.md index 03fe2bb1..8fae6bf7 100644 --- a/.goat-flow/code-map.md +++ b/.goat-flow/code-map.md @@ -1,6 +1,6 @@ # Code Map - gruff-php -Last reviewed 2026-06-01. Captures the v0.3.0 surface as wired in `composer.json`, `bin/gruff-php`, `src/`, and `tests/`. Treat directory listings as authoritative for scope, but always re-grep before claiming behaviour. +Last reviewed 2026-06-03. Captures the v0.3.1 surface as wired in `composer.json`, `bin/gruff-php`, `src/`, and `tests/`. Treat directory listings as authoritative for scope, but always re-grep before claiming behaviour. ## Top-level layout @@ -246,6 +246,7 @@ src/ | | |-- SetupBloatRule.php = `test-quality.setup-bloat` | | |-- SkippedWithoutReasonRule.php = `test-quality.skipped-without-reason` | | |-- SleepInTestRule.php = `test-quality.sleep-in-test` (covers `sleep`/`usleep` family + `time`/`microtime` + `new DateTime('now')`/`DateTimeImmutable()`) +| | |-- StaticAnalysisRedundantTestRule.php = `test-quality.static-analysis-redundant-test` (advisory candidate for tests that assert same-file static declarations such as class_exists/method_exists/property_exists) | | |-- SutNotCalledRule.php = `test-quality.sut-not-called` (skips subprocess-execution tests; matches verb-without-trailing-`s` candidates so `testLoadsX` matches `load()`) | | |-- TautologicalTypeAssertionRule.php = `test-quality.tautological-type-assertion` (only when local static evidence proves the asserted type) | | |-- TestdoxReadabilityRule.php = `test-quality.testdox-readability` (`minWords` threshold) diff --git a/.goat-flow/decisions/ADR-022-test-quality-gate-parity.md b/.goat-flow/decisions/ADR-022-test-quality-gate-parity.md index e2ea2c16..5c85b9a4 100644 --- a/.goat-flow/decisions/ADR-022-test-quality-gate-parity.md +++ b/.goat-flow/decisions/ADR-022-test-quality-gate-parity.md @@ -3,7 +3,7 @@ **Status:** Implemented **Date:** 2026-05-30 **Author(s):** gruff maintainers -**Updated:** 2026-05-30 — amends ADR-010 (severity calibration); extends ADR-017 (mission corollary) +**Updated:** 2026-06-03 — amends ADR-010 (severity calibration); extends ADR-017 (mission corollary); records advisory static-analysis-redundant candidates ## Context @@ -49,6 +49,15 @@ over-fire: `mock-only-test`, `mock-without-expectation`, `trivial-assertion`, `excessive-mocking`, `setup-bloat`, `magic-number-assertion`, naming/readability). Forcing those would manufacture ceremony — the opposite of the mission. +Add `test-quality.static-analysis-redundant-test` as an **advisory** candidate rule, not a +hard gate. It flags direct static-shape assertions such as +`assertTrue(class_exists(Foo::class))` or `assertTrue(method_exists(Foo::class, 'bar'))` +only when the declaration is visible in the same parsed file. These findings must use +candidate language and recommend behavioral evidence, because public compatibility tests +and runtime contract probes can be legitimate even when they mention source shape. Promotion +to `error` would require the same false-positive-clean evidence standard used for +`test-quality.tautological-type-assertion`. + Severity is metadata, not schema: `gruff.analysis.v2` / `gruff.baseline.v1` are unchanged. The two stability snapshots (rule-definition digest, fixture-finding digest) are refreshed in the same change. diff --git a/.gruff-php.yaml b/.gruff-php.yaml index 439915fc..6ed6e1b0 100644 --- a/.gruff-php.yaml +++ b/.gruff-php.yaml @@ -490,6 +490,8 @@ rules: enabled: true test-quality.sleep-in-test: enabled: true + test-quality.static-analysis-redundant-test: + enabled: true test-quality.sut-not-called: enabled: true test-quality.tautological-type-assertion: diff --git a/composer.json b/composer.json index 8cc98b7a..32183aef 100644 --- a/composer.json +++ b/composer.json @@ -1,6 +1,6 @@ { "name": "blundergoat/gruff-php", - "description": "Opinionated PHP code-quality analyzer with 132 rules, SARIF output, baselines, and a local dashboard.", + "description": "Opinionated PHP code-quality analyzer with 133 rules, SARIF output, baselines, and a local dashboard.", "type": "library", "keywords": [ "php", diff --git a/src/Command/SummaryCommand.php b/src/Command/SummaryCommand.php index b941b083..bb9653cd 100644 --- a/src/Command/SummaryCommand.php +++ b/src/Command/SummaryCommand.php @@ -452,7 +452,7 @@ private function parseErrorCount(array $diagnostics): int private function renderText(SummaryReportData $summaryReportData): string { $lines = []; - $lines[] = sprintf('%s %s - summary', Application::NAME, Application::VERSION); + $lines[] = sprintf('%s %s summary', Application::NAME, Application::VERSION); $lines[] = ''; $lines[] = sprintf('Paths %s', $summaryReportData->paths === [] ? '(none)' : implode(', ', $summaryReportData->paths)); $lines[] = sprintf('Config %s', $summaryReportData->configPath ?? '(none)'); @@ -465,7 +465,14 @@ private function renderText(SummaryReportData $summaryReportData): string $summaryReportData->parseErrors ); $lines[] = ''; - $lines[] = sprintf('Composite %s (%.2f / 100)', $summaryReportData->score->composite->letter, $summaryReportData->score->composite->score); + $lines[] = sprintf('Composite: %s (%.2f / 100)', $summaryReportData->score->composite->letter, $summaryReportData->score->composite->score); + $lines[] = sprintf( + 'Findings: %d total · %d error · %d warning · %d advisory', + $summaryReportData->totals['total'], + $summaryReportData->totals['error'], + $summaryReportData->totals['warning'], + $summaryReportData->totals['advisory'], + ); $lines[] = sprintf('Scope %s', $summaryReportData->score->scope); $lines[] = sprintf('Score note %s', $summaryReportData->score->explanation); $lines[] = ''; @@ -527,15 +534,6 @@ private function renderText(SummaryReportData $summaryReportData): string } } - $lines[] = ''; - $lines[] = sprintf( - 'Totals %d findings (advisory=%d, warning=%d, error=%d)', - $summaryReportData->totals['total'], - $summaryReportData->totals['advisory'], - $summaryReportData->totals['warning'], - $summaryReportData->totals['error'], - ); - if ($summaryReportData->totals['total'] > 0) { $lines[] = ''; $lines[] = 'Baseline After review, `gruff-php analyse --generate-baseline` records current findings as known debt.'; diff --git a/src/Reporting/TextReporter.php b/src/Reporting/TextReporter.php index 2b0f7ad6..374da5ff 100644 --- a/src/Reporting/TextReporter.php +++ b/src/Reporting/TextReporter.php @@ -32,18 +32,28 @@ public function render(AnalysisReport $report): string { $counts = $report->findingCounts(); - $lines = [ - sprintf('%s %s', AnalysisReport::TOOL_NAME, $report->toolVersion), - sprintf('Format: %s', $report->format), - sprintf('Fail threshold: %s', $report->failOn), - '', - 'Files', - sprintf(' Discovered: %d', $report->filesDiscovered), - sprintf(' Parsed: %d', $report->filesParsed), - sprintf(' Ignored: %d', count($report->ignoredPaths)), - sprintf(' Missing: %d', count($report->missingPaths)), - sprintf(' Parse errors: %d', $report->parseErrorCount()), - ]; + $lines = [sprintf('%s %s analyse', AnalysisReport::TOOL_NAME, $report->toolVersion)]; + + if ($report->score !== null) { + $lines[] = sprintf('Composite: %s (%.2f / 100)', $report->score->composite->letter, $report->score->composite->score); + } + + $lines[] = sprintf( + 'Findings: %d total · %d error · %d warning · %d advisory', + $counts['total'], + $counts['error'], + $counts['warning'], + $counts['advisory'], + ); + $lines[] = sprintf('Format: %s', $report->format); + $lines[] = sprintf('Fail threshold: %s', $report->failOn); + $lines[] = ''; + $lines[] = 'Files'; + $lines[] = sprintf(' Discovered: %d', $report->filesDiscovered); + $lines[] = sprintf(' Parsed: %d', $report->filesParsed); + $lines[] = sprintf(' Ignored: %d', count($report->ignoredPaths)); + $lines[] = sprintf(' Missing: %d', count($report->missingPaths)); + $lines[] = sprintf(' Parse errors: %d', $report->parseErrorCount()); $this->appendPathSection($lines, 'Ignored paths', $report->ignoredPaths); $this->appendPathSection($lines, 'Missing paths', $report->missingPaths); @@ -57,13 +67,6 @@ public function render(AnalysisReport $report): string $lines[] = ''; $lines[] = 'Summary'; - $lines[] = sprintf( - ' Findings: %d (advisory: %d, warning: %d, error: %d)', - $counts['total'], - $counts['advisory'], - $counts['warning'], - $counts['error'], - ); $lines[] = sprintf(' Exit code: %d', $report->exitCode); if ($report->failureReason !== null) { @@ -224,11 +227,6 @@ private function appendScore(array &$lines, AnalysisReport $report): void $lines[] = ''; $lines[] = 'Score'; - $lines[] = sprintf( - ' Composite: %s (%.2f/100)', - $report->score->composite->letter, - $report->score->composite->score, - ); $lines[] = sprintf(' Scope: %s', $report->score->scope); $lines[] = sprintf(' Score drivers: %s', $report->score->explanation); diff --git a/src/Rule/RuleRegistry.php b/src/Rule/RuleRegistry.php index 24866ece..e2e4847e 100644 --- a/src/Rule/RuleRegistry.php +++ b/src/Rule/RuleRegistry.php @@ -122,6 +122,7 @@ use GruffPhp\Rule\TestQuality\SetupBloatRule; use GruffPhp\Rule\TestQuality\SkippedWithoutReasonRule; use GruffPhp\Rule\TestQuality\SleepInTestRule; +use GruffPhp\Rule\TestQuality\StaticAnalysisRedundantTestRule; use GruffPhp\Rule\TestQuality\SutNotCalledRule; use GruffPhp\Rule\TestQuality\TautologicalTypeAssertionRule; use GruffPhp\Rule\TestQuality\TestdoxReadabilityRule; @@ -297,6 +298,7 @@ public static function defaults(): self new SetupBloatRule(), new SkippedWithoutReasonRule(), new SleepInTestRule(), + new StaticAnalysisRedundantTestRule(), new SutNotCalledRule(), new TautologicalTypeAssertionRule(), new TestLongerThanSutRule(), diff --git a/src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php b/src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php new file mode 100644 index 00000000..61e50f76 --- /dev/null +++ b/src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php @@ -0,0 +1,352 @@ + 'Public API or compatibility contract where runtime existence is the behaviour under test.', + 'mitigation' => 'Keep the test when the runtime contract is intentional; gruff reports this as a candidate, not a deletion command.', + ], + ], + ); + } + + /** + * Find assertion calls that restate declarations already present in the parsed unit. + * + * @param AnalysisUnit $analysisUnit - Parsed unit to inspect. + * @param RuleContext $ruleContext - Rule context for this analysis pass. + * + * @return list - Findings for static-analysis-redundant test candidates. + */ + public function analyse(AnalysisUnit $analysisUnit, RuleContext $ruleContext): array + { + $declarations = $this->collectDeclarations($analysisUnit); + if ($declarations === []) { + return []; + } + + $findings = []; + + foreach (TestQualityNodeHelper::testScopes($analysisUnit) as $scope) { + foreach (TestQualityNodeHelper::assertionCalls($scope) as $assertionCall) { + if (TestQualityNodeHelper::callName($assertionCall) !== 'asserttrue') { + continue; + } + + $subjectCall = TestQualityNodeHelper::firstArgValue($assertionCall); + if (!$subjectCall instanceof Expr\FuncCall) { + continue; + } + + $candidate = $this->candidateFromSubjectCall($subjectCall, $declarations); + if ($candidate === null) { + continue; + } + + $findings[] = new Finding( + ruleId: self::ID, + message: sprintf( + '%s contains a static-analysis-redundant candidate: %s asserts %s, but %s.', + $scope->symbol, + $candidate['assertion'], + $candidate['evidenceSymbol'], + $candidate['staticFact'], + ), + filePath: $analysisUnit->file->displayPath, + line: $assertionCall->getStartLine(), + severity: Severity::Advisory, + pillar: Pillar::TestQuality, + tier: RuleTier::V01, + confidence: Confidence::High, + symbol: $scope->symbol, + remediation: 'Remove only the redundant assertion, or replace it with behavioral evidence that static analysis cannot prove.', + metadata: $candidate, + ); + } + } + + return $findings; + } + + /** + * Build a same-unit declaration index keyed by resolved and short class-like names. + * + * @param AnalysisUnit $analysisUnit - Parsed unit whose declarations should be indexed. + * + * @return array, properties: array}> - Declaration index. + */ + private function collectDeclarations(AnalysisUnit $analysisUnit): array + { + $declarations = []; + + foreach (NodeIndex::nodesOfAny( + $analysisUnit, + [Stmt\Class_::class, Stmt\Interface_::class, Stmt\Trait_::class, Stmt\Enum_::class], + ) as $node) { + if (!$node instanceof Stmt\ClassLike || $node->name === null) { + continue; + } + + $name = $this->classLikeName($node); + if ($name === null) { + continue; + } + + $record = [ + 'kind' => $this->classLikeKind($node), + 'name' => $name, + 'methods' => [], + 'properties' => [], + ]; + + foreach ($node->stmts as $statement) { + if ($statement instanceof Stmt\ClassMethod) { + $methodName = $statement->name->toString(); + $record['methods'][strtolower($methodName)] = $methodName; + continue; + } + + if ($statement instanceof Stmt\Property) { + foreach ($statement->props as $property) { + $propertyName = $property->name->toString(); + $record['properties'][strtolower($propertyName)] = $propertyName; + } + } + } + + foreach ($this->classLikeKeys($node, $name) as $key) { + $declarations[$key] = $record; + } + } + + return $declarations; + } + + /** + * Build a candidate metadata payload when a source declaration proves the subject call. + * + * @param Expr\FuncCall $subjectCall - Function call wrapped by assertTrue(). + * @param array, properties: array}> $declarations - Same-unit declaration index. + * + * @return array{variant: string, assertion: string, staticFact: string, evidenceSymbol: string, candidateConfidence: string}|null - Candidate evidence for a redundant static-fact assertion, or null when the assertion stays unowned. + */ + private function candidateFromSubjectCall(Expr\FuncCall $subjectCall, array $declarations): ?array + { + $assertion = TestQualityNodeHelper::functionName($subjectCall); + if ($assertion === null) { + return null; + } + + $symbolName = $this->classNameFromClassConst(TestQualityNodeHelper::firstArgValue($subjectCall)); + if ($symbolName === null) { + return null; + } + + $declaration = $declarations[strtolower($symbolName)] ?? null; + if ($declaration === null) { + return null; + } + + $expectedKind = $this->expectedKindForExistenceAssertion($assertion); + if ($expectedKind !== null) { + if ($declaration['kind'] !== $expectedKind) { + return null; + } + + return [ + 'variant' => $assertion, + 'assertion' => $assertion, + 'staticFact' => sprintf('%s %s is declared in the same parsed file', $declaration['kind'], $declaration['name']), + 'evidenceSymbol' => $declaration['name'], + 'candidateConfidence' => Confidence::High->value, + ]; + } + + if ($assertion === 'method_exists') { + return $this->memberCandidate($subjectCall, $declaration, 'methods', 'method'); + } + + if ($assertion === 'property_exists') { + return $this->memberCandidate($subjectCall, $declaration, 'properties', 'property'); + } + + return null; + } + + /** + * Build candidate metadata for method_exists() or property_exists() assertions. + * + * @param Expr\FuncCall $subjectCall - Existence check wrapped by assertTrue(). + * @param array{kind: string, name: string, methods: array, properties: array} $declaration - Same-unit declaration row. + * @param 'methods'|'properties' $memberBucket - Declaration member bucket to inspect. + * @param 'method'|'property' $memberKind - Human-readable member kind. + * + * @return array{variant: string, assertion: string, staticFact: string, evidenceSymbol: string, candidateConfidence: string}|null - Candidate evidence for a declared member existence assertion, or null when the member is not statically proven. + */ + private function memberCandidate(Expr\FuncCall $subjectCall, array $declaration, string $memberBucket, string $memberKind): ?array + { + $member = TestQualityNodeHelper::literalValue(TestQualityNodeHelper::argValue($subjectCall, 1)); + if (!is_string($member)) { + return null; + } + + $declaredName = $declaration[$memberBucket][strtolower($member)] ?? null; + if (!is_string($declaredName)) { + return null; + } + + $evidenceSymbol = $memberKind === 'property' + ? sprintf('%s::$%s', $declaration['name'], $declaredName) + : sprintf('%s::%s()', $declaration['name'], $declaredName); + + return [ + 'variant' => TestQualityNodeHelper::functionName($subjectCall) ?? $memberKind . '_exists', + 'assertion' => TestQualityNodeHelper::functionName($subjectCall) ?? $memberKind . '_exists', + 'staticFact' => sprintf('%s %s is declared in the same parsed file', $memberKind, $evidenceSymbol), + 'evidenceSymbol' => $evidenceSymbol, + 'candidateConfidence' => Confidence::High->value, + ]; + } + + /** + * Map source-level existence functions to the declaration kind they prove redundantly. + * + * @param string $assertion - Lowercase existence function name. + * + * @return string|null - Expected class-like kind, or null when the function checks a member. + */ + private function expectedKindForExistenceAssertion(string $assertion): ?string + { + return match ($assertion) { + 'class_exists' => 'class', + 'interface_exists' => 'interface', + 'trait_exists' => 'trait', + 'enum_exists' => 'enum', + default => null, + }; + } + + /** + * Resolve a ClassName::class expression to the parser-resolved class name. + * + * @param Expr|null $expr - Candidate first argument to an existence function. + * + * @return string|null - Resolved class name, or null when the expression is dynamic or not ::class. + */ + private function classNameFromClassConst(?Expr $expr): ?string + { + if (!$expr instanceof Expr\ClassConstFetch || !$expr->class instanceof Name) { + return null; + } + + if ($expr->class->isSpecialClassName()) { + return null; + } + + $name = $expr->name; + if (!$name instanceof Node\Identifier || strtolower($name->toString()) !== 'class') { + return null; + } + + $resolved = $expr->class->getAttribute('resolvedName'); + + return $resolved instanceof Name ? $resolved->toString() : $expr->class->toString(); + } + + /** + * Return the resolved display name for a class-like declaration. + * + * @param Stmt\ClassLike $classLike - Class-like declaration. + * + * @return string|null - Resolved declaration name, or null for anonymous classes. + */ + private function classLikeName(Stmt\ClassLike $classLike): ?string + { + if ($classLike->name === null) { + return null; + } + + $resolved = $classLike->namespacedName ?? null; + + return $resolved instanceof Name ? $resolved->toString() : $classLike->name->toString(); + } + + /** + * Build lookup keys for resolved and short class-like names. + * + * @param Stmt\ClassLike $classLike - Class-like declaration. + * @param string $resolvedName - Resolved class-like name. + * + * @return list - Lowercase lookup keys. + */ + private function classLikeKeys(Stmt\ClassLike $classLike, string $resolvedName): array + { + $keys = [strtolower($resolvedName)]; + + if ($classLike->name !== null) { + $keys[] = strtolower($classLike->name->toString()); + } + + return array_values(array_unique($keys)); + } + + /** + * Describe which PHP declaration kind a class-like node represents. + * + * @param Stmt\ClassLike $classLike - Class-like declaration. + * + * @return 'class'|'interface'|'trait'|'enum' - Declaration kind. + */ + private function classLikeKind(Stmt\ClassLike $classLike): string + { + return match (true) { + $classLike instanceof Stmt\Interface_ => 'interface', + $classLike instanceof Stmt\Trait_ => 'trait', + $classLike instanceof Stmt\Enum_ => 'enum', + default => 'class', + }; + } +} diff --git a/tests/Console/GruffCliSummaryTest.php b/tests/Console/GruffCliSummaryTest.php index 83ca1166..77f2eb0a 100644 --- a/tests/Console/GruffCliSummaryTest.php +++ b/tests/Console/GruffCliSummaryTest.php @@ -37,13 +37,16 @@ public function testSummaryRunsAndShowsDigestSections(): void self::assertSame(0, $process->getExitCode(), $process->getErrorOutput()); $output = $process->getOutput(); - self::assertStringContainsString('gruff-php 0.3.0 - summary', $output); + self::assertStringContainsString('gruff-php 0.3.0 summary', $output); self::assertStringContainsString('Paths tests/Fixtures/Source/mixed', $output); - self::assertStringContainsString('Composite', $output); + self::assertMatchesRegularExpression('/^Composite: [A-F] \(\d+\.\d{2} \/ 100\)$/m', $output); + self::assertMatchesRegularExpression( + '/^Findings: \d+ total · \d+ error · \d+ warning · \d+ advisory$/m', + $output, + ); self::assertStringContainsString('Score note Per-pillar scores start at 100', $output); self::assertStringContainsString('Pillars', $output); self::assertStringContainsString('Top', $output); - self::assertStringContainsString('Totals', $output); self::assertStringContainsString('gruff-php analyse --generate-baseline', $output); self::assertStringContainsString('known debt', $output); } @@ -66,11 +69,13 @@ public function testSummaryDoesNotEmitPerFindingLines(): void self::assertSame(0, $process->getExitCode()); - // The text reporter shows per-finding `[warning] rule.id` lines under "Findings". - // The summary digest must aggregate; it must not include those lines. - self::assertStringNotContainsString('[warning]', $process->getOutput()); - self::assertStringNotContainsString('[advisory]', $process->getOutput()); - self::assertStringNotContainsString('Findings', $process->getOutput()); + // The analyse text reporter shows per-finding `[warning] rule.id` lines under a bare + // "Findings" heading. The summary digest must aggregate; it must not include those lines. + // The canonical `Findings:` tally line (with colon) is expected and asserted elsewhere. + $output = $process->getOutput(); + self::assertStringNotContainsString('[warning]', $output); + self::assertStringNotContainsString('[advisory]', $output); + self::assertDoesNotMatchRegularExpression('/^Findings$/m', $output); } /** diff --git a/tests/Fixtures/Cli/Golden/text-warning.txt b/tests/Fixtures/Cli/Golden/text-warning.txt index 4e2a554f..9e3b15f9 100644 --- a/tests/Fixtures/Cli/Golden/text-warning.txt +++ b/tests/Fixtures/Cli/Golden/text-warning.txt @@ -1,4 +1,6 @@ -gruff-php 0.3.0 +gruff-php 0.3.0 analyse +Composite: A (96.10 / 100) +Findings: 4 total · 0 error · 2 warning · 2 advisory Format: text Fail threshold: error @@ -10,7 +12,6 @@ Files Parse errors: 0 Score - Composite: A (96.10/100) Scope: full-project Score drivers: Per-pillar scores start at 100 and subtract weighted finding penalties; correlated size and complexity findings on one symbol share a single penalty; the composite is the average of applicable pillar scores. Mutation is omitted when no Infection report is supplied. Pillars: @@ -40,5 +41,4 @@ Findings Method name "value" is generic and does not communicate clear intent. Summary - Findings: 4 (advisory: 2, warning: 2, error: 0) Exit code: 0 diff --git a/tests/Fixtures/TestQuality/static-analysis-redundant-test.php b/tests/Fixtures/TestQuality/static-analysis-redundant-test.php new file mode 100644 index 00000000..51845324 --- /dev/null +++ b/tests/Fixtures/TestQuality/static-analysis-redundant-test.php @@ -0,0 +1,87 @@ +label; + } +} + +interface ShapeContract +{ + public function handle(): void; +} + +trait ShapeTrait +{ + public function helper(): void + { + } +} + +enum ShapeStatus +{ + case Ready; +} + +final class ShapeFactory +{ + public static function build(): ShapeService + { + return new ShapeService(); + } +} + +final class StaticAnalysisRedundantCandidateTest extends TestCase +{ + public function testFlagsDirectClassLikeExistenceAssertions(): void + { + self::assertTrue(class_exists(ShapeService::class)); + self::assertTrue(interface_exists(ShapeContract::class)); + self::assertTrue(trait_exists(ShapeTrait::class)); + self::assertTrue(enum_exists(ShapeStatus::class)); + } + + public function testFlagsDirectMemberExistenceAssertions(): void + { + self::assertTrue(method_exists(ShapeService::class, 'label')); + self::assertTrue(property_exists(ShapeService::class, 'label')); + } + + public function testKeepsBehavioralValueAssertionClean(): void + { + $service = new ShapeService(); + + self::assertSame('shape', $service->label()); + } + + public function testKeepsDynamicSymbolNamesClean(): void + { + $className = ShapeService::class; + $methodName = 'label'; + + self::assertTrue(class_exists($className)); + self::assertTrue(method_exists(ShapeService::class, $methodName)); + } + + public function testKeepsExternalRuntimeContractClean(): void + { + self::assertTrue(class_exists(\DateTimeImmutable::class)); + } + + public function testKeepsFactoryReturnTypeAssertionDeferred(): void + { + $service = ShapeFactory::build(); + + self::assertInstanceOf(ShapeService::class, $service); + } +} diff --git a/tests/Rule/RuleRegistryTest.php b/tests/Rule/RuleRegistryTest.php index 7cb62102..a0db99b4 100644 --- a/tests/Rule/RuleRegistryTest.php +++ b/tests/Rule/RuleRegistryTest.php @@ -96,6 +96,7 @@ use GruffPhp\Rule\TestQuality\SetupBloatRule; use GruffPhp\Rule\TestQuality\SkippedWithoutReasonRule; use GruffPhp\Rule\TestQuality\SleepInTestRule; +use GruffPhp\Rule\TestQuality\StaticAnalysisRedundantTestRule; use GruffPhp\Rule\TestQuality\SutNotCalledRule; use GruffPhp\Rule\TestQuality\TestLongerThanSutRule; use GruffPhp\Rule\TestQuality\TestNamingConsistencyRule; @@ -171,6 +172,7 @@ public function testDefaultRegistryContainsStableRuleIds(): void MockOnlyTestRule::ID, MysteryGuestRule::ID, NoAssertionsRule::ID, PrivateReflectionRule::ID, SetupBloatRule::ID, SkippedWithoutReasonRule::ID, SleepInTestRule::ID, + StaticAnalysisRedundantTestRule::ID, SutNotCalledRule::ID, TestLongerThanSutRule::ID, TestNamingConsistencyRule::ID, TrivialAssertionRule::ID, TrivialSnapshotRule::ID, AverageMethodLengthRule::ID, @@ -326,9 +328,9 @@ public function testDefaultRuleDefinitionsStayStable(): void usort($definitions, static fn(array $left, array $right): int => $left['id'] <=> $right['id']); $json = json_encode($definitions, JSON_THROW_ON_ERROR); - self::assertCount(132, $definitions); + self::assertCount(133, $definitions); self::assertSame( - '5766c459c7516111c2b7' . 'b2d95ed45390cff1f3ffeec82d90de8327bdedb4a8ba', + 'e6458d471a959cc841760b' . '96bc46aa52d7cf8c8c90839971d7e16934275af700', hash('sha256', $json), ); } diff --git a/tests/Rule/RuleRegressionSnapshotTest.php b/tests/Rule/RuleRegressionSnapshotTest.php index 2f7d22a4..f148e999 100644 --- a/tests/Rule/RuleRegressionSnapshotTest.php +++ b/tests/Rule/RuleRegressionSnapshotTest.php @@ -51,10 +51,10 @@ public function testDefaultRuleRegistryFindingsStayStableAcrossFixtures(): void { [$units, $findings, $json] = $this->analysePaths(['tests/Fixtures']); - self::assertCount(169, $units); - self::assertCount(2410, $findings); + self::assertCount(170, $units); + self::assertCount(2439, $findings); self::assertSame( - 'ae32f2419065a7eb' . 'af9795dcd8eae2855e58ec4e93a6b896a35c6fd10fb8d93f', + '5cb43f1361b2feec' . '2a9697dfdda435146c692f30c46bcf70e087840491d9b8f5', hash('sha256', $json), ); } diff --git a/tests/Rule/TestQuality/TestQualityRulesTest.php b/tests/Rule/TestQuality/TestQualityRulesTest.php index 8cd3cec8..d101fc4c 100644 --- a/tests/Rule/TestQuality/TestQualityRulesTest.php +++ b/tests/Rule/TestQuality/TestQualityRulesTest.php @@ -7,7 +7,9 @@ use GruffPhp\Config\AnalysisConfig; use GruffPhp\Config\ConfigLoader; use GruffPhp\Config\RuleSettings; +use GruffPhp\Finding\Confidence; use GruffPhp\Finding\Finding; +use GruffPhp\Finding\Severity; use GruffPhp\Parser\AnalysisUnit; use GruffPhp\Parser\PhpFileParser; use GruffPhp\Rule\RuleContext; @@ -33,6 +35,7 @@ use GruffPhp\Rule\TestQuality\SetupBloatRule; use GruffPhp\Rule\TestQuality\SkippedWithoutReasonRule; use GruffPhp\Rule\TestQuality\SleepInTestRule; +use GruffPhp\Rule\TestQuality\StaticAnalysisRedundantTestRule; use GruffPhp\Rule\TestQuality\SutNotCalledRule; use GruffPhp\Rule\TestQuality\TautologicalTypeAssertionRule; use GruffPhp\Rule\TestQuality\TestdoxReadabilityRule; @@ -277,6 +280,7 @@ public function testNonCandidateCasesAreNotFlaggedBySelectedRules(): void self::assertRuleCount(LoopAssertionWithoutMessageRule::ID, 0, $findings); self::assertRuleCount(UnusedMockRule::ID, 0, $findings); self::assertRuleCount(ExceptionTypeOnlyRule::ID, 0, $findings); + self::assertRuleCount(StaticAnalysisRedundantTestRule::ID, 0, $findings); self::assertRuleCount(TautologicalTypeAssertionRule::ID, 0, $findings); self::assertRuleCount(GlobalStateMutationRule::ID, 0, $findings); self::assertRuleCount(MockWithoutExpectationRule::ID, 0, $findings); @@ -372,6 +376,70 @@ public function testTautologicalTypeAssertionDetectedAndCrossClassAssertionsAllo self::assertRuleCount(TautologicalTypeAssertionRule::ID, 2, $findings); } + /** + * Verify static-analysis-redundant candidates carry evidence and leave behavior clean. + * + * @return void + */ + public function testStaticAnalysisRedundantCandidatesDetectedWithEvidence(): void + { + $findings = array_values(array_filter( + $this->analysePath('tests/Fixtures/TestQuality/static-analysis-redundant-test.php'), + static fn(Finding $finding): bool => $finding->ruleId === StaticAnalysisRedundantTestRule::ID, + )); + + self::assertCount(6, $findings); + self::assertSame( + [ + 'class_exists', + 'enum_exists', + 'interface_exists', + 'method_exists', + 'property_exists', + 'trait_exists', + ], + $this->stringMetadataValues($findings, 'variant'), + ); + self::assertSame( + [ + 'Fixtures\TestQuality\StaticAnalysisRedundantTest\ShapeContract', + 'Fixtures\TestQuality\StaticAnalysisRedundantTest\ShapeService', + 'Fixtures\TestQuality\StaticAnalysisRedundantTest\ShapeService::$label', + 'Fixtures\TestQuality\StaticAnalysisRedundantTest\ShapeService::label()', + 'Fixtures\TestQuality\StaticAnalysisRedundantTest\ShapeStatus', + 'Fixtures\TestQuality\StaticAnalysisRedundantTest\ShapeTrait', + ], + $this->stringMetadataValues($findings, 'evidenceSymbol'), + ); + + foreach ($findings as $finding) { + self::assertSame(Severity::Advisory, $finding->severity); + self::assertSame(Confidence::High, $finding->confidence); + self::assertStringContainsString('static-analysis-redundant candidate', $finding->message); + self::assertSame('high', $finding->metadata['candidateConfidence'] ?? null); + } + } + + /** + * Verify static-analysis-redundant candidates do not duplicate neighbouring rule ownership. + * + * @return void + */ + public function testStaticAnalysisRedundantCandidatesRespectNeighbouringRules(): void + { + $tautologicalFindings = $this->analysePath('tests/Fixtures/TestQuality/tautological-type-assertion.php'); + self::assertRuleCount(StaticAnalysisRedundantTestRule::ID, 0, $tautologicalFindings); + self::assertRuleCount(TautologicalTypeAssertionRule::ID, 2, $tautologicalFindings); + + $exceptionFindings = $this->analysePath('tests/Fixtures/TestQuality/exception-type-only.php'); + self::assertRuleCount(StaticAnalysisRedundantTestRule::ID, 0, $exceptionFindings); + self::assertRuleCount(ExceptionTypeOnlyRule::ID, 1, $exceptionFindings); + + $mechanicsFindings = $this->analysePath('tests/Fixtures/TestQuality/phpunit-mechanics-smells.php'); + self::assertRuleCount(StaticAnalysisRedundantTestRule::ID, 0, $mechanicsFindings); + self::assertRuleCount(PrivateReflectionRule::ID, 3, $mechanicsFindings); + } + /** * Verify global state mutation detected and cleaned up class allowed. * @@ -546,6 +614,26 @@ private static function assertRuleCount(string $ruleId, int $expectedCount, arra ); } + /** + * Return sorted string metadata values for stable assertions. + * + * @param list $findings - Findings whose metadata should be inspected. + * @param string $key - Metadata key to read. + * + * @return list - String values sorted ascending. + */ + private function stringMetadataValues(array $findings, string $key): array + { + $values = array_values(array_filter( + array_map(static fn(Finding $finding): mixed => $finding->metadata[$key] ?? null, $findings), + 'is_string', + )); + + sort($values, SORT_STRING); + + return $values; + } + /** * Analyse test-quality fixtures and return findings for assertions. * From f55f5ee263752ba46695b0830b6f0594c1b679ff Mon Sep 17 00:00:00 2001 From: Matthew Hansen Date: Wed, 3 Jun 2026 07:02:24 +1000 Subject: [PATCH 2/5] Update rule counts and severity levels in rules.md --- docs/rules.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/rules.md b/docs/rules.md index af0e6f37..8fc505ae 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -12,7 +12,7 @@ to three near-match suggestions and exits with code 2. This rule catalogue is generated from `php bin/gruff-php list-rules --format json`. Use that command for the full machine-readable metadata, including thresholds and options. -Total rules: 132 +Total rules: 133 ## Summary By Pillar @@ -28,7 +28,7 @@ Total rules: 132 | `security` | 25 | | `sensitive-data` | 11 | | `size` | 7 | -| `test-quality` | 33 | +| `test-quality` | 34 | ## Rule Catalogue @@ -65,7 +65,7 @@ Total rules: 132 | --- | --- | --- | --- | --- | | `design.single-implementor-interface` | Single-implementor interface | `advisory` | `medium` | yes | -### `documentation` (14) +### `documentation` (15) | Rule ID | Name | Severity | Confidence | Enabled By Default | | --- | --- | --- | --- | --- | @@ -204,7 +204,7 @@ bag), `Collection` (single-leaf generic). | `size.property-count` | Property count | `error` | `high` | yes | | `size.public-method-count` | Public method count | `error` | `high` | yes | -### `test-quality` (33) +### `test-quality` (34) | Rule ID | Name | Severity | Confidence | Enabled By Default | | --- | --- | --- | --- | --- | @@ -224,7 +224,7 @@ bag), `Collection` (single-leaf generic). | `test-quality.multiple-aaa-cycles` | Multiple arrange-act-assert cycles | `advisory` | `low` | yes | | `test-quality.mystery-guest` | Mystery guest | `advisory` | `medium` | yes | | `test-quality.naming-consistency` | Test naming consistency | `advisory` | `high` | yes | -| `test-quality.no-assertions` | Test without assertions | `warning` | `medium` | yes | +| `test-quality.no-assertions` | Test without assertions | `error` | `medium` | yes | | `test-quality.phpunit-coverage-source-missing` | PHPUnit coverage source missing | `advisory` | `medium` | yes | | `test-quality.phpunit-deprecations-not-fatal` | PHPUnit deprecations not fatal | `warning` | `high` | yes | | `test-quality.phpunit-strict-flags-missing` | PHPUnit strict flags missing | `warning` | `high` | yes | @@ -233,8 +233,9 @@ bag), `Collection` (single-leaf generic). | `test-quality.setup-bloat` | Setup bloat | `advisory` | `medium` | yes | | `test-quality.skipped-without-reason` | Skipped test without reason | `warning` | `high` | yes | | `test-quality.sleep-in-test` | Sleep or wall-clock read in test | `warning` | `high` | yes | -| `test-quality.sut-not-called` | Test name mentions SUT that is not called | `advisory` | `low` | yes | -| `test-quality.tautological-type-assertion` | Tautological type assertion | `warning` | `high` | yes | +| `test-quality.static-analysis-redundant-test` | Static-analysis-redundant test candidate | `advisory` | `high` | yes | +| `test-quality.sut-not-called` | Test name mentions SUT that is not called | `error` | `low` | yes | +| `test-quality.tautological-type-assertion` | Tautological type assertion | `error` | `high` | yes | | `test-quality.test-longer-than-sut` | Test longer than apparent SUT | `advisory` | `low` | yes | | `test-quality.test-method-too-long` | Test method too long | `advisory` | `high` | yes | | `test-quality.testdox-readability` | Testdox readability | `advisory` | `low` | yes | From f6b30ae7e0392eb6f7045fa54562df2fe9e72422 Mon Sep 17 00:00:00 2001 From: Matthew Hansen Date: Thu, 4 Jun 2026 05:38:12 +1000 Subject: [PATCH 3/5] Bump version to 0.3.1, add Symfony YAML controller support, and update CHANGELOG --- CHANGELOG.md | 8 + composer.lock | 58 +++---- src/Console/Application.php | 2 +- src/Rule/DeadCode/DeadCodeProjectIndex.php | 152 ++++++++++++++++++ src/Rule/DeadCode/UnusedInternalClassRule.php | 4 +- src/Rule/ProjectSourceTextRuleAccumulator.php | 12 ++ src/Rule/RuleRegistry.php | 6 +- tests/Console/AnalyseCliTest.php | 2 +- tests/Console/GruffCliSummaryTest.php | 4 +- tests/Console/ListRulesCliTest.php | 2 +- tests/Fixtures/Cli/Golden/json-warning.json | 2 +- tests/Fixtures/Cli/Golden/text-warning.txt | 2 +- .../project-wide/config/routes/block.yml | 4 + .../project-wide/config/routes/inline.yaml | 3 + .../project-wide/config/routes/non-fqcn.yaml | 13 ++ .../project-wide/config/routes/quoted.yaml | 9 ++ .../src/Controller/RouteControllers.php | 33 ++++ .../DeadCode/ProjectDeadCodeRulesTest.php | 29 +++- tests/Rule/RuleRegressionSnapshotTest.php | 6 +- .../Rule/TestQuality/TestQualityRulesTest.php | 133 ++++++--------- 20 files changed, 354 insertions(+), 130 deletions(-) create mode 100644 src/Rule/ProjectSourceTextRuleAccumulator.php create mode 100644 tests/Fixtures/DeadCode/project-wide/config/routes/block.yml create mode 100644 tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml create mode 100644 tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml create mode 100644 tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml create mode 100644 tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 23f546b1..929da26f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ Notable user-facing changes to `gruff-php` are listed here. This project is still pre-1.0, so minor releases may break behaviour. Breaking changes are marked and include the action to take. +## 0.3.1 - 2026-06-04 + +0.3.1 adds one conservative test-quality rule, fixes a Symfony YAML route false positive in project-wide dead-code analysis, and moves the headline numbers to the top of text reports. No breaking changes; JSON schemas, config format, and baselines are unchanged. + +- **New rule `test-quality.static-analysis-redundant-test`** - Advisory rule that flags unit tests whose main assertion only restates a statically visible declaration: `class_exists`, `interface_exists`, `trait_exists`, `enum_exists`, `method_exists`, or `property_exists` on a type declared in the same file. Each finding names the static fact the assertion restates and recommends asserting behaviour instead of deleting the test; it does not duplicate the existing `test-quality.tautological-type-assertion` hard gate. On by default at advisory, so upgrading projects may see new advisory findings - they are candidates, not gate failures. +- **Symfony YAML route controllers count as live references** - `dead-code.unused-internal-class` now recognises internal `FQCN::method` values under Symfony YAML `_controller` keys, including block, inline, and quoted route defaults. Service-id and legacy non-FQCN controller strings are ignored, so projects with YAML routes no longer need to add those controllers to `entrypointSymbols` just to avoid this false positive. +- **Text reports lead with score and findings** - `analyse` and `summary` text output now show `Composite:` and `Findings: N total · N error · N warning · N advisory` at the top, and the header names the subcommand (for example `gruff-php ... analyse`). JSON output is unchanged. + ## 0.3.0 - 2026-05-31 0.3.0 focuses on agent-friendly CI: scan only changed code, respect ignored paths everywhere, and fail on newly introduced debt instead of old baseline debt. It also removes noisy complexity/design checks and tightens the rules that support human review of AI-written code. diff --git a/composer.lock b/composer.lock index 00cf0d12..3ab84f6e 100644 --- a/composer.lock +++ b/composer.lock @@ -1563,16 +1563,16 @@ }, { "name": "friendsofphp/php-cs-fixer", - "version": "v3.95.3", + "version": "v3.95.4", "source": { "type": "git", "url": "https://github.com/PHP-CS-Fixer/PHP-CS-Fixer.git", - "reference": "3d681493acc0e93283481b1c63c263737df78687" + "reference": "3f8f68856837a77e1f1d870354eca3c8747f2f72" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/PHP-CS-Fixer/PHP-CS-Fixer/zipball/3d681493acc0e93283481b1c63c263737df78687", - "reference": "3d681493acc0e93283481b1c63c263737df78687", + "url": "https://api.github.com/repos/PHP-CS-Fixer/PHP-CS-Fixer/zipball/3f8f68856837a77e1f1d870354eca3c8747f2f72", + "reference": "3f8f68856837a77e1f1d870354eca3c8747f2f72", "shasum": "" }, "require": { @@ -1656,7 +1656,7 @@ ], "support": { "issues": "https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/issues", - "source": "https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/tree/v3.95.3" + "source": "https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/tree/v3.95.4" }, "funding": [ { @@ -1664,7 +1664,7 @@ "type": "github" } ], - "time": "2026-05-29T20:35:26+00:00" + "time": "2026-06-03T18:02:44+00:00" }, { "name": "infection/abstract-testframework-adapter", @@ -2506,16 +2506,16 @@ }, { "name": "phpunit/php-code-coverage", - "version": "12.5.6", + "version": "12.5.7", "source": { "type": "git", "url": "https://github.com/sebastianbergmann/php-code-coverage.git", - "reference": "876099a072646c7745f673d7aeab5382c4439691" + "reference": "186dab580576598076de6818596d12b61801880e" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sebastianbergmann/php-code-coverage/zipball/876099a072646c7745f673d7aeab5382c4439691", - "reference": "876099a072646c7745f673d7aeab5382c4439691", + "url": "https://api.github.com/repos/sebastianbergmann/php-code-coverage/zipball/186dab580576598076de6818596d12b61801880e", + "reference": "186dab580576598076de6818596d12b61801880e", "shasum": "" }, "require": { @@ -2526,13 +2526,13 @@ "php": ">=8.3", "phpunit/php-text-template": "^5.0", "sebastian/complexity": "^5.0", - "sebastian/environment": "^8.0.3", - "sebastian/lines-of-code": "^4.0", + "sebastian/environment": "^8.1.2", + "sebastian/lines-of-code": "^4.0.1", "sebastian/version": "^6.0", "theseer/tokenizer": "^2.0.1" }, "require-dev": { - "phpunit/phpunit": "^12.5.1" + "phpunit/phpunit": "^12.5.28" }, "suggest": { "ext-pcov": "PHP extension that provides line coverage", @@ -2570,7 +2570,7 @@ "support": { "issues": "https://github.com/sebastianbergmann/php-code-coverage/issues", "security": "https://github.com/sebastianbergmann/php-code-coverage/security/policy", - "source": "https://github.com/sebastianbergmann/php-code-coverage/tree/12.5.6" + "source": "https://github.com/sebastianbergmann/php-code-coverage/tree/12.5.7" }, "funding": [ { @@ -2590,7 +2590,7 @@ "type": "tidelift" } ], - "time": "2026-04-15T08:23:17+00:00" + "time": "2026-06-01T13:24:19+00:00" }, { "name": "phpunit/php-file-iterator", @@ -3615,16 +3615,16 @@ }, { "name": "sanmai/di-container", - "version": "0.1.16", + "version": "0.1.17", "source": { "type": "git", "url": "https://github.com/sanmai/di-container.git", - "reference": "8b8a8859e992297259b220a92179439f9d185838" + "reference": "a901c4a8778c9212ef4d66607525281af2f787bd" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sanmai/di-container/zipball/8b8a8859e992297259b220a92179439f9d185838", - "reference": "8b8a8859e992297259b220a92179439f9d185838", + "url": "https://api.github.com/repos/sanmai/di-container/zipball/a901c4a8778c9212ef4d66607525281af2f787bd", + "reference": "a901c4a8778c9212ef4d66607525281af2f787bd", "shasum": "" }, "require": { @@ -3682,7 +3682,7 @@ ], "support": { "issues": "https://github.com/sanmai/di-container/issues", - "source": "https://github.com/sanmai/di-container/tree/0.1.16" + "source": "https://github.com/sanmai/di-container/tree/0.1.17" }, "funding": [ { @@ -3690,7 +3690,7 @@ "type": "github" } ], - "time": "2026-05-30T11:27:18+00:00" + "time": "2026-06-01T08:52:14+00:00" }, { "name": "sanmai/duoclock", @@ -4341,26 +4341,26 @@ }, { "name": "sebastian/global-state", - "version": "8.0.2", + "version": "8.0.3", "source": { "type": "git", "url": "https://github.com/sebastianbergmann/global-state.git", - "reference": "ef1377171613d09edd25b7816f05be8313f9115d" + "reference": "b164d3274d6537ab462591c5755f76a8f5b1aae9" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/sebastianbergmann/global-state/zipball/ef1377171613d09edd25b7816f05be8313f9115d", - "reference": "ef1377171613d09edd25b7816f05be8313f9115d", + "url": "https://api.github.com/repos/sebastianbergmann/global-state/zipball/b164d3274d6537ab462591c5755f76a8f5b1aae9", + "reference": "b164d3274d6537ab462591c5755f76a8f5b1aae9", "shasum": "" }, "require": { "php": ">=8.3", "sebastian/object-reflector": "^5.0", - "sebastian/recursion-context": "^7.0" + "sebastian/recursion-context": "^7.0.1" }, "require-dev": { "ext-dom": "*", - "phpunit/phpunit": "^12.0" + "phpunit/phpunit": "^12.5.28" }, "type": "library", "extra": { @@ -4391,7 +4391,7 @@ "support": { "issues": "https://github.com/sebastianbergmann/global-state/issues", "security": "https://github.com/sebastianbergmann/global-state/security/policy", - "source": "https://github.com/sebastianbergmann/global-state/tree/8.0.2" + "source": "https://github.com/sebastianbergmann/global-state/tree/8.0.3" }, "funding": [ { @@ -4411,7 +4411,7 @@ "type": "tidelift" } ], - "time": "2025-08-29T11:29:25+00:00" + "time": "2026-06-01T15:10:33+00:00" }, { "name": "sebastian/lines-of-code", diff --git a/src/Console/Application.php b/src/Console/Application.php index 8a03853b..ae6b1fa0 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -26,7 +26,7 @@ final class Application extends SymfonyApplication /** * Version displayed by the CLI. */ - public const VERSION = '0.3.0'; + public const VERSION = '0.3.1'; /** * Register the gruff-php CLI command surface with Symfony Console. diff --git a/src/Rule/DeadCode/DeadCodeProjectIndex.php b/src/Rule/DeadCode/DeadCodeProjectIndex.php index f6c89116..b857a7ff 100644 --- a/src/Rule/DeadCode/DeadCodeProjectIndex.php +++ b/src/Rule/DeadCode/DeadCodeProjectIndex.php @@ -26,6 +26,8 @@ use PhpParser\Node\Stmt\Property; use PhpParser\Node\Stmt\Trait_; use PhpParser\Node\UnionType; +use Symfony\Component\Yaml\Exception\ParseException; +use Symfony\Component\Yaml\Yaml; /** * Builds project-owned declaration/reference summaries for dead-code rules. @@ -176,6 +178,10 @@ public function unusedConstantDeclarations(): array */ private function recordDeclarations(AnalysisUnit $analysisUnit): void { + if (!$analysisUnit->file->isPhp()) { + return; + } + $scope = $this->scope(); $skipEntrypointDeclarations = $scope->isEntrypointPath($analysisUnit->file->displayPath); $classLikeTypes = [Class_::class, Interface_::class, Trait_::class, Enum_::class]; @@ -245,6 +251,11 @@ private function recordReferences(AnalysisUnit $analysisUnit): void { $isTestFile = $this->scope()->isTestPath($analysisUnit->file->displayPath); + if (!$analysisUnit->file->isPhp()) { + $this->recordSymfonyYamlControllerReferences($analysisUnit, $isTestFile); + return; + } + $this->recordExpressionClassReferences($analysisUnit, $isTestFile); $this->recordStructuralClassReferences($analysisUnit, $isTestFile); $this->recordTypeReferencesInUnit($analysisUnit, $isTestFile); @@ -252,6 +263,147 @@ private function recordReferences(AnalysisUnit $analysisUnit): void $this->recordConstantFetchReferences($analysisUnit, $isTestFile); } + /** + * Record Symfony YAML route controller callables as class references. + * + * @param AnalysisUnit $analysisUnit - YAML/YML text unit to inspect. + * @param bool $isTestFile - Whether the containing unit is a test file. + * + * @return void + */ + private function recordSymfonyYamlControllerReferences(AnalysisUnit $analysisUnit, bool $isTestFile): void + { + if (!$this->isYamlUnit($analysisUnit) || trim($analysisUnit->source) === '') { + return; + } + + try { + $decoded = Yaml::parse($analysisUnit->source); + } catch (ParseException) { + return; + } + + $this->recordSymfonyYamlControllerReferencesFromValue($decoded, $isTestFile); + } + + /** + * Walk parsed YAML and record values attached to `_controller` keys. + * + * @param mixed $value - Parsed YAML value. + * @param bool $isTestFile - Whether the containing unit is a test file. + * + * @return void + */ + private function recordSymfonyYamlControllerReferencesFromValue(mixed $value, bool $isTestFile): void + { + if (!is_array($value)) { + return; + } + + foreach ($value as $key => $childValue) { + if ($key === '_controller' && is_string($childValue)) { + $this->recordSymfonyControllerReferenceValue($childValue, $isTestFile); + } + + if (is_array($childValue)) { + $this->recordSymfonyYamlControllerReferencesFromValue($childValue, $isTestFile); + } + } + } + + /** + * Record the class part from an internal `FQCN::method` controller callable. + * + * @param string $controllerValue - Raw `_controller` scalar from YAML. + * @param bool $isTestFile - Whether the containing unit is a test file. + * + * @return void + */ + private function recordSymfonyControllerReferenceValue(string $controllerValue, bool $isTestFile): void + { + $fqn = $this->classFqnFromControllerCallable($controllerValue); + if ($fqn === null || !$this->scope()->isInternalFqn($fqn)) { + return; + } + + $this->classReferences[$fqn][] = new DeadCodeSymbolReference( + fqn: $fqn, + originSymbol: null, + isTestFile: $isTestFile, + ); + } + + /** + * Extract a PHP class FQN from a Symfony controller callable string. + * + * @param string $controllerValue - Candidate `_controller` value. + * + * @return string|null - Class FQN without leading slash, or null for non-FQCN controller shapes + */ + private function classFqnFromControllerCallable(string $controllerValue): ?string + { + $candidate = trim($controllerValue, " \t\n\r\0\x0B'\""); + if (!str_contains($candidate, '::')) { + return null; + } + + $parts = explode('::', $candidate, 2); + if (count($parts) !== 2) { + return null; + } + + $classPart = ltrim(trim($parts[0], " \t\n\r\0\x0B'\""), '\\'); + $methodPart = trim($parts[1], " \t\n\r\0\x0B'\""); + + if ($classPart === '' || $methodPart === '') { + return null; + } + + if (!$this->isPhpFqn($classPart) || !$this->isPhpMethodName($methodPart)) { + return null; + } + + return $classPart; + } + + /** + * Decide whether the unit is a YAML route/config source. + * + * @param AnalysisUnit $analysisUnit - Text unit to classify. + * + * @return bool - true for .yaml and .yml display paths + */ + private function isYamlUnit(AnalysisUnit $analysisUnit): bool + { + $extension = strtolower(pathinfo($analysisUnit->file->displayPath, PATHINFO_EXTENSION)); + + return $extension === 'yaml' || $extension === 'yml'; + } + + /** + * Decide whether a string has PHP FQN segment syntax. + * + * @param string $fqn - Candidate class FQN without a leading slash. + * + * @return bool - true when every namespace segment is a PHP identifier + */ + private function isPhpFqn(string $fqn): bool + { + return preg_match('/^[A-Za-z_][A-Za-z0-9_]*(?:\\\\[A-Za-z_][A-Za-z0-9_]*)*$/', $fqn) === 1; + } + + /** + * Decide whether a callable suffix has PHP method-name syntax. + * + * @param string $methodName - Candidate method name after `::`. + * + * @return bool - true when the method segment is a PHP identifier + */ + private function isPhpMethodName(string $methodName): bool + { + return preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $methodName) === 1; + } + /** * Record expression-level class references from one unit. * diff --git a/src/Rule/DeadCode/UnusedInternalClassRule.php b/src/Rule/DeadCode/UnusedInternalClassRule.php index aeb8e7ec..114df51e 100644 --- a/src/Rule/DeadCode/UnusedInternalClassRule.php +++ b/src/Rule/DeadCode/UnusedInternalClassRule.php @@ -4,10 +4,12 @@ namespace GruffPhp\Rule\DeadCode; +use GruffPhp\Rule\ProjectSourceTextRuleAccumulator; + /** * Reports project-owned class-like declarations with no supported references. */ -final class UnusedInternalClassRule extends AbstractUnusedInternalSymbolRule +final class UnusedInternalClassRule extends AbstractUnusedInternalSymbolRule implements ProjectSourceTextRuleAccumulator { /** * Stable rule identifier for unused internal class-like findings. diff --git a/src/Rule/ProjectSourceTextRuleAccumulator.php b/src/Rule/ProjectSourceTextRuleAccumulator.php new file mode 100644 index 00000000..84b3b8d7 --- /dev/null +++ b/src/Rule/ProjectSourceTextRuleAccumulator.php @@ -0,0 +1,12 @@ +hasParseErrors() || !$analysisUnit->file->isPhp()) { + if ($analysisUnit->hasParseErrors()) { return; } + $isPhp = $analysisUnit->file->isPhp(); foreach ($this->enabledRules($ruleContext->config) as $rule) { if (!$rule instanceof ProjectRuleAccumulator) { continue; } + if (!$isPhp && !$rule instanceof ProjectSourceTextRuleAccumulator) { + continue; + } if ($ruleRunnerObserver === null) { $rule->accumulate($analysisUnit, $ruleContext); diff --git a/tests/Console/AnalyseCliTest.php b/tests/Console/AnalyseCliTest.php index 979a9f26..25b1c399 100644 --- a/tests/Console/AnalyseCliTest.php +++ b/tests/Console/AnalyseCliTest.php @@ -32,7 +32,7 @@ public function testAnalyseCommandRunsAsNoOp(): void $process->run(); self::assertSame(0, $process->getExitCode(), $process->getErrorOutput()); - self::assertStringContainsString('gruff-php 0.3.0', $process->getOutput()); + self::assertStringContainsString('gruff-php 0.3.1', $process->getOutput()); self::assertStringContainsString('Discovered: 2', $process->getOutput()); self::assertStringContainsString('Ignored: 6', $process->getOutput()); self::assertStringContainsString('tests/Fixtures/Source/mixed/vendor/ignored.php', $process->getOutput()); diff --git a/tests/Console/GruffCliSummaryTest.php b/tests/Console/GruffCliSummaryTest.php index 77f2eb0a..4bf779e0 100644 --- a/tests/Console/GruffCliSummaryTest.php +++ b/tests/Console/GruffCliSummaryTest.php @@ -37,7 +37,7 @@ public function testSummaryRunsAndShowsDigestSections(): void self::assertSame(0, $process->getExitCode(), $process->getErrorOutput()); $output = $process->getOutput(); - self::assertStringContainsString('gruff-php 0.3.0 summary', $output); + self::assertStringContainsString('gruff-php 0.3.1 summary', $output); self::assertStringContainsString('Paths tests/Fixtures/Source/mixed', $output); self::assertMatchesRegularExpression('/^Composite: [A-F] \(\d+\.\d{2} \/ 100\)$/m', $output); self::assertMatchesRegularExpression( @@ -108,7 +108,7 @@ public function testSummaryJsonOutputMatchesSchema(): void $tool = $decoded['tool'] ?? null; self::assertIsArray($tool); self::assertSame('gruff-php', $tool['name'] ?? null); - self::assertSame('0.3.0', $tool['version'] ?? null); + self::assertSame('0.3.1', $tool['version'] ?? null); $scope = $decoded['scope'] ?? null; self::assertIsArray($scope); diff --git a/tests/Console/ListRulesCliTest.php b/tests/Console/ListRulesCliTest.php index f03ca418..a0bffd42 100644 --- a/tests/Console/ListRulesCliTest.php +++ b/tests/Console/ListRulesCliTest.php @@ -24,7 +24,7 @@ public function testVersionCommandRunsThroughBinary(): void self::assertSame(0, $process->getExitCode(), $process->getErrorOutput()); self::assertStringContainsString('gruff-php', $process->getOutput()); - self::assertStringContainsString('0.3.0', $process->getOutput()); + self::assertStringContainsString('0.3.1', $process->getOutput()); } /** diff --git a/tests/Fixtures/Cli/Golden/json-warning.json b/tests/Fixtures/Cli/Golden/json-warning.json index 936d43fc..7f717f51 100644 --- a/tests/Fixtures/Cli/Golden/json-warning.json +++ b/tests/Fixtures/Cli/Golden/json-warning.json @@ -2,7 +2,7 @@ "schemaVersion": "gruff.analysis.v2", "tool": { "name": "gruff-php", - "version": "0.3.0" + "version": "0.3.1" }, "run": { "format": "json", diff --git a/tests/Fixtures/Cli/Golden/text-warning.txt b/tests/Fixtures/Cli/Golden/text-warning.txt index 9e3b15f9..0f63a09e 100644 --- a/tests/Fixtures/Cli/Golden/text-warning.txt +++ b/tests/Fixtures/Cli/Golden/text-warning.txt @@ -1,4 +1,4 @@ -gruff-php 0.3.0 analyse +gruff-php 0.3.1 analyse Composite: A (96.10 / 100) Findings: 4 total · 0 error · 2 warning · 2 advisory Format: text diff --git a/tests/Fixtures/DeadCode/project-wide/config/routes/block.yml b/tests/Fixtures/DeadCode/project-wide/config/routes/block.yml new file mode 100644 index 00000000..60cd8a14 --- /dev/null +++ b/tests/Fixtures/DeadCode/project-wide/config/routes/block.yml @@ -0,0 +1,4 @@ +block_controller: + path: /block + defaults: + _controller: App\Controller\BlockController::index diff --git a/tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml b/tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml new file mode 100644 index 00000000..2be837fc --- /dev/null +++ b/tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml @@ -0,0 +1,3 @@ +inline_controller: + path: /inline + defaults: { _controller: App\Controller\InlineController::index } diff --git a/tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml b/tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml new file mode 100644 index 00000000..896e48af --- /dev/null +++ b/tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml @@ -0,0 +1,13 @@ +service_id_controller: + path: /service-id + defaults: + _controller: app.controller.service_id::index + +legacy_controller: + path: /legacy + defaults: { _controller: AppBundle:Legacy:index } + +other_key_controller: + path: /other-key + defaults: + not_controller: App\Controller\OtherKeyController::index diff --git a/tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml b/tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml new file mode 100644 index 00000000..721f5985 --- /dev/null +++ b/tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml @@ -0,0 +1,9 @@ +single_quoted_controller: + path: /single-quoted + defaults: + _controller: 'App\Controller\SingleQuotedController::index' + +double_quoted_controller: + path: /double-quoted + defaults: + _controller: "App\\Controller\\DoubleQuotedController::index" diff --git a/tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php b/tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php new file mode 100644 index 00000000..9659c14a --- /dev/null +++ b/tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php @@ -0,0 +1,33 @@ + */ private const FIXTURE_FILES = [ 'src/Symbols.php', + 'src/Controller/RouteControllers.php', 'src/references.php', 'tests/TestReferences.php', 'entrypoints/Entrypoints.php', 'src/FrameworkCommand.php', 'src/External/Vendored.php', + 'config/routes/inline.yaml', + 'config/routes/block.yml', + 'config/routes/quoted.yaml', + 'config/routes/non-fqcn.yaml', ]; /** @@ -72,6 +77,24 @@ public function testUnusedInternalClassLikeDeclarationsDetected(): void self::assertNotContains('App\\Tests\\FixtureTestCase', $symbols); } + /** + * Verify Symfony YAML `_controller` FQCN callables keep route controllers live. + * + * @return void + */ + public function testSymfonyYamlControllerReferencesKeepInternalClassesLive(): void + { + $symbols = $this->symbolsForRule(UnusedInternalClassRule::ID); + + self::assertNotContains('App\\Controller\\InlineController', $symbols); + self::assertNotContains('App\\Controller\\BlockController', $symbols); + self::assertNotContains('App\\Controller\\SingleQuotedController', $symbols); + self::assertNotContains('App\\Controller\\DoubleQuotedController', $symbols); + self::assertContains('App\\Controller\\UnreferencedController', $symbols); + self::assertContains('App\\Controller\\ServiceIdStyleController', $symbols); + self::assertContains('App\\Controller\\OtherKeyController', $symbols); + } + /** * Verify unused internal functions are reported while direct and test references count. * @@ -281,7 +304,9 @@ private function fixtureUnits(): array */ private function parseProjectFile(string $projectRoot, string $displayPath): AnalysisUnit { - return (new PhpFileParser())->parse(new SourceFile($projectRoot . '/' . $displayPath, $displayPath)); + $type = str_ends_with($displayPath, '.php') ? SourceFile::TYPE_PHP : SourceFile::TYPE_TEXT; + + return (new PhpFileParser())->parse(new SourceFile($projectRoot . '/' . $displayPath, $displayPath, $type)); } /** diff --git a/tests/Rule/RuleRegressionSnapshotTest.php b/tests/Rule/RuleRegressionSnapshotTest.php index f148e999..d9b2775c 100644 --- a/tests/Rule/RuleRegressionSnapshotTest.php +++ b/tests/Rule/RuleRegressionSnapshotTest.php @@ -51,10 +51,10 @@ public function testDefaultRuleRegistryFindingsStayStableAcrossFixtures(): void { [$units, $findings, $json] = $this->analysePaths(['tests/Fixtures']); - self::assertCount(170, $units); - self::assertCount(2439, $findings); + self::assertCount(175, $units); + self::assertCount(2454, $findings); self::assertSame( - '5cb43f1361b2feec' . '2a9697dfdda435146c692f30c46bcf70e087840491d9b8f5', + '07a17793d73788b' . '0239ca96cf9e5ec4d11110024eb239d42076724cabab8543b', hash('sha256', $json), ); } diff --git a/tests/Rule/TestQuality/TestQualityRulesTest.php b/tests/Rule/TestQuality/TestQualityRulesTest.php index d101fc4c..bb2c33ca 100644 --- a/tests/Rule/TestQuality/TestQualityRulesTest.php +++ b/tests/Rule/TestQuality/TestQualityRulesTest.php @@ -46,6 +46,7 @@ use GruffPhp\Rule\TestQuality\TrivialSnapshotRule; use GruffPhp\Rule\TestQuality\UnusedMockRule; use GruffPhp\Source\SourceFile; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; /** @@ -293,75 +294,37 @@ public function testNonCandidateCasesAreNotFlaggedBySelectedRules(): void } /** - * Verify extends production class detected and allows test case descendants. + * Verify each single-rule fixture emits exactly the expected finding count. * - * @return void - */ - public function testExtendsProductionClassDetectedAndAllowsTestCaseDescendants(): void - { - $findings = $this->analysePath('tests/Fixtures/TestQuality/extends-production.php'); - - self::assertRuleCount(ExtendsProductionClassRule::ID, 1, $findings); - } - - /** - * Verify test method too long detected and ignores whitespace lines. - * - * @return void - */ - public function testTestMethodTooLongDetectedAndIgnoresWhitespaceLines(): void - { - $findings = $this->analysePath('tests/Fixtures/TestQuality/test-method-too-long.php'); - - self::assertRuleCount(TestMethodTooLongRule::ID, 1, $findings); - } - - /** - * Verify empty data provider detected and yielding provider is allowed. - * - * @return void - */ - public function testEmptyDataProviderDetectedAndYieldingProviderIsAllowed(): void - { - $findings = $this->analysePath('tests/Fixtures/TestQuality/empty-data-provider.php'); - - self::assertRuleCount(EmptyDataProviderRule::ID, 2, $findings); - } - - /** - * Verify loop assertion without message detected and assertion with message allowed. - * - * @return void - */ - public function testLoopAssertionWithoutMessageDetectedAndAssertionWithMessageAllowed(): void - { - $findings = $this->analysePath('tests/Fixtures/TestQuality/loop-assertion-without-message.php'); - - self::assertRuleCount(LoopAssertionWithoutMessageRule::ID, 3, $findings); - } - - /** - * Verify unused mock detected and used mocks allowed. + * @param string $fixture - Fixture path under tests/Fixtures/TestQuality to analyse. + * @param string $ruleId - Rule identifier whose findings are counted. + * @param int $expectedCount - Exact number of findings the rule must emit on the fixture. * * @return void */ - public function testUnusedMockDetectedAndUsedMocksAllowed(): void + #[DataProvider('singleRuleFixtureProvider')] + public function testSingleRuleFixtureEmitsExpectedCount(string $fixture, string $ruleId, int $expectedCount): void { - $findings = $this->analysePath('tests/Fixtures/TestQuality/unused-mock.php'); - - self::assertRuleCount(UnusedMockRule::ID, 2, $findings); + self::assertRuleCount($ruleId, $expectedCount, $this->analysePath($fixture)); } /** - * Verify exception type only detected and paired assertions allowed. + * Provide single-rule fixtures paired with the exact finding count each rule must emit. Each exact count also + * pins the negative half of the fixture (the allowed shapes that must stay unflagged). * - * @return void + * @return array - Rows of fixture path, rule id, and expected finding count. */ - public function testExceptionTypeOnlyDetectedAndPairedAssertionsAllowed(): void + public static function singleRuleFixtureProvider(): array { - $findings = $this->analysePath('tests/Fixtures/TestQuality/exception-type-only.php'); - - self::assertRuleCount(ExceptionTypeOnlyRule::ID, 1, $findings); + return [ + 'extends-production flags its production-class parent' => ['tests/Fixtures/TestQuality/extends-production.php', ExtendsProductionClassRule::ID, 1], + 'test-method-too-long flags one oversized method' => ['tests/Fixtures/TestQuality/test-method-too-long.php', TestMethodTooLongRule::ID, 1], + 'empty-data-provider flags two empty providers' => ['tests/Fixtures/TestQuality/empty-data-provider.php', EmptyDataProviderRule::ID, 2], + 'loop-assertion-without-message flags three messageless loop assertions' => ['tests/Fixtures/TestQuality/loop-assertion-without-message.php', LoopAssertionWithoutMessageRule::ID, 3], + 'unused-mock flags two unused mocks' => ['tests/Fixtures/TestQuality/unused-mock.php', UnusedMockRule::ID, 2], + 'exception-type-only flags one type-only expectation' => ['tests/Fixtures/TestQuality/exception-type-only.php', ExceptionTypeOnlyRule::ID, 1], + 'global-state-mutation flags three leaks in the leaky class' => ['tests/Fixtures/TestQuality/global-state-mutation.php', GlobalStateMutationRule::ID, 3], + ]; } /** @@ -412,11 +375,11 @@ public function testStaticAnalysisRedundantCandidatesDetectedWithEvidence(): voi $this->stringMetadataValues($findings, 'evidenceSymbol'), ); - foreach ($findings as $finding) { - self::assertSame(Severity::Advisory, $finding->severity); - self::assertSame(Confidence::High, $finding->confidence); - self::assertStringContainsString('static-analysis-redundant candidate', $finding->message); - self::assertSame('high', $finding->metadata['candidateConfidence'] ?? null); + foreach ($findings as $index => $finding) { + self::assertSame(Severity::Advisory, $finding->severity, "finding {$index}"); + self::assertSame(Confidence::High, $finding->confidence, "finding {$index}"); + self::assertStringContainsString('static-analysis-redundant candidate', $finding->message, "finding {$index}"); + self::assertSame('high', $finding->metadata['candidateConfidence'] ?? null, "finding {$index}"); } } @@ -427,30 +390,9 @@ public function testStaticAnalysisRedundantCandidatesDetectedWithEvidence(): voi */ public function testStaticAnalysisRedundantCandidatesRespectNeighbouringRules(): void { - $tautologicalFindings = $this->analysePath('tests/Fixtures/TestQuality/tautological-type-assertion.php'); - self::assertRuleCount(StaticAnalysisRedundantTestRule::ID, 0, $tautologicalFindings); - self::assertRuleCount(TautologicalTypeAssertionRule::ID, 2, $tautologicalFindings); - - $exceptionFindings = $this->analysePath('tests/Fixtures/TestQuality/exception-type-only.php'); - self::assertRuleCount(StaticAnalysisRedundantTestRule::ID, 0, $exceptionFindings); - self::assertRuleCount(ExceptionTypeOnlyRule::ID, 1, $exceptionFindings); - - $mechanicsFindings = $this->analysePath('tests/Fixtures/TestQuality/phpunit-mechanics-smells.php'); - self::assertRuleCount(StaticAnalysisRedundantTestRule::ID, 0, $mechanicsFindings); - self::assertRuleCount(PrivateReflectionRule::ID, 3, $mechanicsFindings); - } - - /** - * Verify global state mutation detected and cleaned up class allowed. - * - * @return void - */ - public function testGlobalStateMutationDetectedAndCleanedUpClassAllowed(): void - { - $findings = $this->analysePath('tests/Fixtures/TestQuality/global-state-mutation.php'); - - // 3 mutations in the leaky class (superglobal write + putenv + ini_set), 0 in classes with local or inherited cleanup, 0 in the read-only class. - self::assertRuleCount(GlobalStateMutationRule::ID, 3, $findings); + $this->assertSmellOwnedSolelyByNeighbour('tests/Fixtures/TestQuality/tautological-type-assertion.php', TautologicalTypeAssertionRule::ID, 2); + $this->assertSmellOwnedSolelyByNeighbour('tests/Fixtures/TestQuality/exception-type-only.php', ExceptionTypeOnlyRule::ID, 1); + $this->assertSmellOwnedSolelyByNeighbour('tests/Fixtures/TestQuality/phpunit-mechanics-smells.php', PrivateReflectionRule::ID, 3); } /** @@ -614,6 +556,23 @@ private static function assertRuleCount(string $ruleId, int $expectedCount, arra ); } + /** + * Assert the static-analysis-redundant rule stays silent while a neighbouring rule solely owns the fixture's smell. + * + * @param string $fixture - Fixture path whose neighbouring-rule ownership is verified. + * @param string $ownerRuleId - Rule identifier expected to solely own the fixture's smell. + * @param int $ownerCount - Exact number of findings the owning rule must emit. + * + * @return void + */ + private function assertSmellOwnedSolelyByNeighbour(string $fixture, string $ownerRuleId, int $ownerCount): void + { + $findings = $this->analysePath($fixture); + + self::assertRuleCount(StaticAnalysisRedundantTestRule::ID, 0, $findings); + self::assertRuleCount($ownerRuleId, $ownerCount, $findings); + } + /** * Return sorted string metadata values for stable assertions. * From 1db79ca94b99c2e0e8c4b91655f5ca859e62c69e Mon Sep 17 00:00:00 2001 From: Matthew Hansen Date: Thu, 4 Jun 2026 06:35:19 +1000 Subject: [PATCH 4/5] Add tests for changed-region accounting and project-wide findings in analysis --- CHANGELOG.md | 5 +- docs/output-formats.md | 9 + src/Command/AnalyseCommand.php | 1 + src/Command/AnalysisFindingSupport.php | 25 ++ src/Command/AnalysisPipeline.php | 25 ++ src/Command/BranchReviewBuilder.php | 33 ++- src/Diff/DiffResult.php | 31 ++- src/Rule/DeadCode/DeadCodeProjectIndex.php | 50 ++-- src/Rule/RuleRegistry.php | 20 ++ tests/Console/AnalyseCliDiffTest.php | 294 ++++++++++++++++++++- 10 files changed, 456 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 929da26f..e3b015e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,12 @@ changes are marked and include the action to take. ## 0.3.1 - 2026-06-04 -0.3.1 adds one conservative test-quality rule, fixes a Symfony YAML route false positive in project-wide dead-code analysis, and moves the headline numbers to the top of text reports. No breaking changes; JSON schemas, config format, and baselines are unchanged. +0.3.1 adds one conservative test-quality rule, fixes Symfony YAML route and changed-region accounting edges in project-wide dead-code analysis, and moves the headline numbers to the top of text reports. No breaking changes; JSON schemas, config format, and baselines are unchanged. - **New rule `test-quality.static-analysis-redundant-test`** - Advisory rule that flags unit tests whose main assertion only restates a statically visible declaration: `class_exists`, `interface_exists`, `trait_exists`, `enum_exists`, `method_exists`, or `property_exists` on a type declared in the same file. Each finding names the static fact the assertion restates and recommends asserting behaviour instead of deleting the test; it does not duplicate the existing `test-quality.tautological-type-assertion` hard gate. On by default at advisory, so upgrading projects may see new advisory findings - they are candidates, not gate failures. - **Symfony YAML route controllers count as live references** - `dead-code.unused-internal-class` now recognises internal `FQCN::method` values under Symfony YAML `_controller` keys, including block, inline, and quoted route defaults. Service-id and legacy non-FQCN controller strings are ignored, so projects with YAML routes no longer need to add those controllers to `entrypointSymbols` just to avoid this false positive. -- **Text reports lead with score and findings** - `analyse` and `summary` text output now show `Composite:` and `Findings: N total · N error · N warning · N advisory` at the top, and the header names the subcommand (for example `gruff-php ... analyse`). JSON output is unchanged. +- **Changed-region suppression counts are scoped to changed files** - `suppressedCount` now reconciles with the findings anchored to the changed/requested files after project-wide rules have used whole-project context. The count is also mirrored as `diff.suppressedCount` in JSON reports. +- **Text reports lead with score and findings** - `analyse` and `summary` text output now show `Composite:` and `Findings: N total · N error · N warning · N advisory` at the top, and the header names the subcommand (for example `gruff-php ... analyse`). ## 0.3.0 - 2026-05-31 diff --git a/docs/output-formats.md b/docs/output-formats.md index 9503a6be..7efcee4f 100644 --- a/docs/output-formats.md +++ b/docs/output-formats.md @@ -35,6 +35,15 @@ Each finding carries two identifier fields: For baseline matching, use `fingerprint`. For line-shift-resilient diff tooling, use `stableIdentity`. +Changed-region reports (`--diff`, `--since`, or `--changed-ranges`) include a +top-level `suppressedCount`, mirrored as `diff.suppressedCount`, when diff +filtering is active. It counts findings anchored in the changed/requested files +that were produced by the analysis run and then removed because they were +outside the selected hunk or symbol. Project-wide rules still use whole-project +context before filtering, but project-rule findings anchored outside the +changed/requested files are outside the invocation scope and are not included in +the suppression total. + ## HTML Use `html` for archived human review or dashboard scan output: diff --git a/src/Command/AnalyseCommand.php b/src/Command/AnalyseCommand.php index b35de99b..21a6cf71 100644 --- a/src/Command/AnalyseCommand.php +++ b/src/Command/AnalyseCommand.php @@ -197,6 +197,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $diffFilterResult = (new DiffFindingFilter())->apply($findings, $diff, $sources->analysisUnits, $options->changedScope); $findings = $diffFilterResult->findings; $suppressedCount = $diffFilterResult->suppressedCount; + $diff = $diff->withSuppressedCount($suppressedCount); } $findings = $findingSupport->filterAllowedSecretPreviews($findings, $config); diff --git a/src/Command/AnalysisFindingSupport.php b/src/Command/AnalysisFindingSupport.php index 3c4e0640..0cda6461 100644 --- a/src/Command/AnalysisFindingSupport.php +++ b/src/Command/AnalysisFindingSupport.php @@ -65,6 +65,31 @@ public function filterFindingsToChangedFiles(array $findings, array $changedFile )); } + /** + * Keep project-rule findings inside the files requested by this invocation. + * + * @param list $findings - Findings to filter. + * @param list $projectRuleIds - Rule ids whose output came from project-wide context. + * @param list $filePaths - Project-relative display paths in the requested source set. + * + * @return list - Findings with out-of-scope project-rule rows removed. + */ + public function filterProjectRuleFindingsToFiles(array $findings, array $projectRuleIds, array $filePaths): array + { + if ($projectRuleIds === [] || $filePaths === []) { + return $findings; + } + + $projectRules = array_fill_keys($projectRuleIds, true); + $files = array_fill_keys($filePaths, true); + + return array_values(array_filter( + $findings, + static fn(Finding $finding): bool => !isset($projectRules[$finding->ruleId]) + || isset($files[$finding->filePath]), + )); + } + /** * Rewrite absolute finding paths to be relative to the requested base directory. * diff --git a/src/Command/AnalysisPipeline.php b/src/Command/AnalysisPipeline.php index c3d41960..25436b2d 100644 --- a/src/Command/AnalysisPipeline.php +++ b/src/Command/AnalysisPipeline.php @@ -18,6 +18,7 @@ use GruffPhp\Rule\RuleRegistry; use GruffPhp\Rule\RuleRunnerObserver; use Closure; +use GruffPhp\Source\SourceFile; use GruffPhp\Source\SourceDiscoveryResult; /** @@ -133,9 +134,13 @@ private function canStream( ?DiffResult $reviewDiff, RuleContext $ruleContext, ): bool { + $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 && $options->diffVs === null && $this->registry->supportsStreaming($ruleContext); } @@ -302,6 +307,11 @@ private function runLegacy( $ruleRunnerObserver, shouldReleaseUnitsAfterAnalysis: true, ); + $findings = (new AnalysisFindingSupport())->filterProjectRuleFindingsToFiles( + $findings, + $this->registry->enabledProjectRuleIds($config), + $this->sourceFilePaths($sources), + ); $analyseNs = hrtime(true) - $analyseStart; // Surface the resolved project-context units too so review flows can diff them against the base snapshot. @@ -313,4 +323,19 @@ private function runLegacy( 'projectContextUnits' => $projectContextUnits, ]; } + + /** + * Return display paths from the source set requested by this invocation. + * + * @param AnalysisSourceSet $sources - Loaded sources for the requested analysis paths. + * + * @return list - Project-relative source file paths in discovery order. + */ + private function sourceFilePaths(AnalysisSourceSet $sources): array + { + return array_map( + static fn(SourceFile $sourceFile): string => $sourceFile->displayPath, + $sources->discovery->files, + ); + } } diff --git a/src/Command/BranchReviewBuilder.php b/src/Command/BranchReviewBuilder.php index b2ad9fa1..431b7aa3 100644 --- a/src/Command/BranchReviewBuilder.php +++ b/src/Command/BranchReviewBuilder.php @@ -18,6 +18,7 @@ use GruffPhp\Rule\RuleContext; use GruffPhp\Rule\RuleRegistry; use GruffPhp\Scoring\ScoreCalculator; +use GruffPhp\Source\SourceFile; use RuntimeException; /** @@ -90,6 +91,11 @@ public function build( ? $this->baseProjectContextUnits($baseRoot, $options, $config) : $baseSources->analysisUnits; $baseFindings = $baseRegistry->analyse($baseSources->analysisUnits, new RuleContext($baseRoot, $config), $baseProjectContextUnits); + $baseFindings = (new AnalysisFindingSupport())->filterProjectRuleFindingsToFiles( + $baseFindings, + $baseRegistry->enabledProjectRuleIds($config), + $this->sourceFilePaths($baseSources), + ); $baseFindings = (new AnalysisFindingSupport())->filterAllowedSecretPreviews($baseFindings, $config); } @@ -189,14 +195,14 @@ private function baseSnapshotPaths( ): array { $support = new AnalysisFindingSupport(); - if (!$options->isChangedOnly) { - return $support->normaliseRequestedPaths($projectRoot, $options->paths); - } - if ($shouldLoadProjectContext) { return []; } + if (!$options->isChangedOnly) { + return $support->normaliseRequestedPaths($projectRoot, $options->paths); + } + if ($reviewDiff->changedFiles === []) { return []; } @@ -285,8 +291,27 @@ private function shouldLoadProjectContext( return true; } + if ($options->paths !== []) { + return true; + } + return $options->isChangedOnly && $reviewDiff instanceof DiffResult && $reviewDiff->changedFiles !== []; } + + /** + * Return display paths from a source set. + * + * @param AnalysisSourceSet $sources - Source set loaded for requested analysis paths. + * + * @return list - Project-relative display paths in discovery order. + */ + private function sourceFilePaths(AnalysisSourceSet $sources): array + { + return array_map( + static fn(SourceFile $sourceFile): string => $sourceFile->displayPath, + $sources->discovery->files, + ); + } } diff --git a/src/Diff/DiffResult.php b/src/Diff/DiffResult.php index e06d3c9d..fd085057 100644 --- a/src/Diff/DiffResult.php +++ b/src/Diff/DiffResult.php @@ -16,6 +16,7 @@ * @param array> $changedLines - Changed line ranges keyed by display path. * @param list $changedFiles - Display paths marked as changed. * @param string $message - Human-readable diff status message. + * @param int|null $suppressedCount - Findings removed by changed-region filtering, when known. */ public function __construct( public bool $active, @@ -24,6 +25,7 @@ public function __construct( public array $changedLines, public array $changedFiles, public string $message, + public ?int $suppressedCount = null, ) { } @@ -40,6 +42,26 @@ public static function inactive(): self return new self(false, 'full-project', null, [], [], 'Diff mode is disabled.'); } + /** + * Return a copy carrying the changed-region suppression count. + * + * @param int $suppressedCount - Findings excluded by changed-region filtering. + * + * @return self - Diff metadata with the count attached for report serialization. + */ + public function withSuppressedCount(int $suppressedCount): self + { + return new self( + active: $this->active, + mode: $this->mode, + base: $this->base, + changedLines: $this->changedLines, + changedFiles: $this->changedFiles, + message: $this->message, + suppressedCount: $suppressedCount, + ); + } + /** * Return changed line ranges for a display path. * @@ -62,6 +84,7 @@ public function rangesFor(string $filePath): array * base: string|null, * changedFiles: int, * message: string, + * suppressedCount?: int, * files: list}> * } - JSON-serialisable summary of the diff: changedFiles is a count (not the paths), with per-file paths and ranges nested under files */ @@ -81,7 +104,7 @@ public function toArray(): array // The wire shape intentionally diverges from the in-memory one: `changedFiles` is emitted as a // count while the per-file paths and ranges move under `files`, so consumers read a summary plus detail. - return [ + $payload = [ 'active' => $this->active, 'mode' => $this->mode, 'base' => $this->base, @@ -89,5 +112,11 @@ public function toArray(): array 'message' => $this->message, 'files' => $files, ]; + + if ($this->suppressedCount !== null) { + $payload['suppressedCount'] = $this->suppressedCount; + } + + return $payload; } } diff --git a/src/Rule/DeadCode/DeadCodeProjectIndex.php b/src/Rule/DeadCode/DeadCodeProjectIndex.php index b857a7ff..69d6ff3d 100644 --- a/src/Rule/DeadCode/DeadCodeProjectIndex.php +++ b/src/Rule/DeadCode/DeadCodeProjectIndex.php @@ -34,6 +34,16 @@ */ final class DeadCodeProjectIndex { + /** + * Pattern for PHP class FQNs: namespace segments separated by backslashes. + */ + private const PHP_CLASS_FQN_PATTERN = '/^[A-Za-z_][A-Za-z0-9_]*(?:\\\\[A-Za-z_][A-Za-z0-9_]*)*$/'; + + /** + * Pattern for a concrete PHP method identifier after a controller callable delimiter. + */ + private const PHP_METHOD_NAME_PATTERN = '/^[A-Za-z_][A-Za-z0-9_]*$/'; + /** * @var array */ @@ -289,18 +299,18 @@ private function recordSymfonyYamlControllerReferences(AnalysisUnit $analysisUni /** * Walk parsed YAML and record values attached to `_controller` keys. * - * @param mixed $value - Parsed YAML value. + * @param mixed $yamlNode - Parsed YAML value or nested mapping. * @param bool $isTestFile - Whether the containing unit is a test file. * * @return void */ - private function recordSymfonyYamlControllerReferencesFromValue(mixed $value, bool $isTestFile): void + private function recordSymfonyYamlControllerReferencesFromValue(mixed $yamlNode, bool $isTestFile): void { - if (!is_array($value)) { + if (!is_array($yamlNode)) { return; } - foreach ($value as $key => $childValue) { + foreach ($yamlNode as $key => $childValue) { if ($key === '_controller' && is_string($childValue)) { $this->recordSymfonyControllerReferenceValue($childValue, $isTestFile); } @@ -359,7 +369,13 @@ private function classFqnFromControllerCallable(string $controllerValue): ?strin return null; } - if (!$this->isPhpFqn($classPart) || !$this->isPhpMethodName($methodPart)) { + // Require a PHP class FQN: identifier segments separated by namespace separators. + if (preg_match(self::PHP_CLASS_FQN_PATTERN, $classPart) !== 1) { + return null; + } + + // Require a concrete method identifier after the Symfony controller delimiter. + if (preg_match(self::PHP_METHOD_NAME_PATTERN, $methodPart) !== 1) { return null; } @@ -380,30 +396,6 @@ private function isYamlUnit(AnalysisUnit $analysisUnit): bool return $extension === 'yaml' || $extension === 'yml'; } - /** - * Decide whether a string has PHP FQN segment syntax. - * - * @param string $fqn - Candidate class FQN without a leading slash. - * - * @return bool - true when every namespace segment is a PHP identifier - */ - private function isPhpFqn(string $fqn): bool - { - return preg_match('/^[A-Za-z_][A-Za-z0-9_]*(?:\\\\[A-Za-z_][A-Za-z0-9_]*)*$/', $fqn) === 1; - } - - /** - * Decide whether a callable suffix has PHP method-name syntax. - * - * @param string $methodName - Candidate method name after `::`. - * - * @return bool - true when the method segment is a PHP identifier - */ - private function isPhpMethodName(string $methodName): bool - { - return preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $methodName) === 1; - } - /** * Record expression-level class references from one unit. * diff --git a/src/Rule/RuleRegistry.php b/src/Rule/RuleRegistry.php index 49ed0d35..6d89e9f8 100644 --- a/src/Rule/RuleRegistry.php +++ b/src/Rule/RuleRegistry.php @@ -409,6 +409,26 @@ public function hasEnabledProjectRules(AnalysisConfig $config): bool return false; } + /** + * Return enabled rule ids whose findings come from project-wide analysis. + * + * @param AnalysisConfig $config - Config used to filter registered rules. + * + * @return list - Enabled ProjectRuleInterface ids in registry order. + */ + public function enabledProjectRuleIds(AnalysisConfig $config): array + { + $ruleIds = []; + + foreach ($this->enabledRules($config) as $rule) { + if ($rule instanceof ProjectRuleInterface) { + $ruleIds[] = $rule->definition()->id; + } + } + + return $ruleIds; + } + /** * Determine whether the enabled rule set is fully streaming-capable. * diff --git a/tests/Console/AnalyseCliDiffTest.php b/tests/Console/AnalyseCliDiffTest.php index dc2b558e..96cfe2c4 100644 --- a/tests/Console/AnalyseCliDiffTest.php +++ b/tests/Console/AnalyseCliDiffTest.php @@ -136,10 +136,142 @@ public function testAnalyseCommandDiffStdinParsesUnifiedDiff(): void } } + /** + * Verify changed-ranges accounting reconciles with full-file intra-file findings. + * + * @return void + * @throws JsonException + */ + public function testAnalyseCommandChangedRangesReconcilesIntraFileFindings(): void + { + $tempDir = $this->tempDir(); + + try { + file_put_contents($tempDir . '/gruff-test.yaml', $this->ruleSelectionConfig(['waste.empty-method'])); + file_put_contents($tempDir . '/Example.php', $this->multiMethodIntraFileSource()); + + $fullReport = $this->runJsonAnalyse($tempDir, [ + 'analyse', + 'Example.php', + '--config', + 'gruff-test.yaml', + '--no-baseline', + '--format', + 'json', + '--fail-on', + 'none', + ]); + $fullFindings = $this->findingRows($fullReport); + self::assertCount(2, $fullFindings); + + $scopedReport = $this->runJsonAnalyse($tempDir, [ + 'analyse', + 'Example.php', + '--config', + 'gruff-test.yaml', + '--no-baseline', + '--changed-ranges', + '5-5', + '--changed-scope', + 'symbol', + '--format', + 'json', + '--fail-on', + 'none', + ]); + + $findings = $this->findingRows($scopedReport); + self::assertCount(1, $findings); + self::assertSame(1, $this->suppressedCount($scopedReport)); + self::assertSame(1, $this->diffSuppressedCount($scopedReport)); + self::assertSame( + count($fullFindings), + count($findings) + $this->suppressedCount($scopedReport), + ); + self::assertSame('Example::first()', $findings[0]['symbol'] ?? null); + } finally { + $this->removeDir($tempDir); + } + } + + /** + * Verify project-wide rule findings in the changed file surface or count, while other project findings stay out of the file-scoped total. + * + * @return void + * @throws JsonException + */ + public function testAnalyseCommandChangedRangesReconcilesProjectWideFindings(): void + { + $tempDir = $this->tempDir(); + + try { + $this->writeProjectWideChangedRegionFixture($tempDir); + + $fullReport = $this->runJsonAnalyse($tempDir, [ + 'analyse', + 'src/ChangedUnused.php', + '--config', + 'gruff-test.yaml', + '--no-baseline', + '--format', + 'json', + '--fail-on', + 'none', + ]); + $fullFindings = $this->findingRows($fullReport); + self::assertSame(['App\\ChangedUnused'], $this->symbolsFromJsonFindings($fullFindings)); + + $inRangeReport = $this->runJsonAnalyse($tempDir, [ + 'analyse', + 'src/ChangedUnused.php', + '--config', + 'gruff-test.yaml', + '--no-baseline', + '--changed-ranges', + '5-5', + '--changed-scope', + 'symbol', + '--format', + 'json', + '--fail-on', + 'none', + ]); + self::assertSame(['App\\ChangedUnused'], $this->symbolsFromJsonFindings($this->findingRows($inRangeReport))); + self::assertSame(0, $this->suppressedCount($inRangeReport)); + self::assertSame(0, $this->diffSuppressedCount($inRangeReport)); + + $outOfRangeReport = $this->runJsonAnalyse($tempDir, [ + 'analyse', + 'src/ChangedUnused.php', + '--config', + 'gruff-test.yaml', + '--no-baseline', + '--changed-ranges', + '2-2', + '--changed-scope', + 'symbol', + '--format', + 'json', + '--fail-on', + 'none', + ]); + $outOfRangeFindings = $this->findingRows($outOfRangeReport); + self::assertSame([], $outOfRangeFindings); + self::assertSame(1, $this->suppressedCount($outOfRangeReport)); + self::assertSame(1, $this->diffSuppressedCount($outOfRangeReport)); + self::assertSame( + count($fullFindings), + count($outOfRangeFindings) + $this->suppressedCount($outOfRangeReport), + ); + } finally { + $this->removeDir($tempDir); + } + } + /** * Extract symbol strings from JSON finding rows. * - * @param list $findings - Finding rows decoded from the CLI JSON report. + * @param list> $findings - Finding rows decoded from the CLI JSON report. * * @return list - symbol names of findings that carry one, in finding order; entries without a string symbol are omitted */ @@ -156,6 +288,126 @@ private function symbolsFromJsonFindings(array $findings): array return $symbols; } + /** + * Return decoded finding rows after narrowing their mixed JSON type. + * + * @param array $report - Decoded JSON report. + * + * @return list> - Finding rows. + */ + private function findingRows(array $report): array + { + $findings = $report['findings'] ?? null; + self::assertIsArray($findings); + + $rows = []; + foreach ($findings as $finding) { + self::assertIsArray($finding); + $findingRow = []; + foreach ($finding as $key => $value) { + if (is_string($key)) { + $findingRow[$key] = $value; + } + } + + $rows[] = $findingRow; + } + + return $rows; + } + + /** + * Return the top-level changed-region suppression count from a decoded report. + * + * @param array $report - Decoded JSON report. + * + * @return int - Top-level suppressedCount value. + */ + private function suppressedCount(array $report): int + { + $suppressedCount = $report['suppressedCount'] ?? null; + self::assertIsInt($suppressedCount); + + return $suppressedCount; + } + + /** + * Return the diff-local changed-region suppression count from a decoded report. + * + * @param array $report - Decoded JSON report. + * + * @return int - diff.suppressedCount value. + */ + private function diffSuppressedCount(array $report): int + { + $diff = $report['diff'] ?? null; + self::assertIsArray($diff); + + $suppressedCount = $diff['suppressedCount'] ?? null; + self::assertIsInt($suppressedCount); + + return $suppressedCount; + } + + /** + * Run gruff in a fixture project and decode its JSON report. + * + * @param string $workingDirectory - Project root to run the command in. + * @param list $arguments - CLI arguments after the PHP binary and bin path. + * + * @return array - Decoded JSON report. + * @throws JsonException + */ + private function runJsonAnalyse(string $workingDirectory, array $arguments): array + { + $process = new Process(array_merge([PHP_BINARY, self::PROJECT_ROOT . '/bin/gruff-php'], $arguments), $workingDirectory); + $process->run(); + + self::assertSame(0, $process->getExitCode(), $process->getErrorOutput()); + + return $this->decodeJsonOutput($process); + } + + /** + * Build a minimal config selecting only the rules under test. + * + * @param list $ruleIds - Rule ids to include. + * + * @return string - YAML config content. + */ + private function ruleSelectionConfig(array $ruleIds): string + { + $lines = [ + 'schemaVersion: gruff-php.config.v0.1', + 'selection:', + ' rules:', + ]; + + foreach ($ruleIds as $ruleId) { + $lines[] = ' - ' . $ruleId; + } + + return implode("\n", $lines) . "\n"; + } + + /** + * Create a tiny project whose project-wide dead-code findings include one changed-file and one other-file symbol. + * + * @param string $projectRoot - Temporary project root to populate. + * + * @return void + */ + private function writeProjectWideChangedRegionFixture(string $projectRoot): void + { + self::assertTrue(mkdir($projectRoot . '/src', 0777, true)); + file_put_contents($projectRoot . '/composer.json', "{\"autoload\":{\"psr-4\":{\"App\\\\\":\"src/\"}}}\n"); + file_put_contents($projectRoot . '/gruff-test.yaml', $this->ruleSelectionConfig(['dead-code.unused-internal-class'])); + file_put_contents($projectRoot . '/src/ChangedUnused.php', $this->changedUnusedProjectSource()); + file_put_contents($projectRoot . '/src/OtherUnused.php', " Date: Thu, 4 Jun 2026 19:28:40 +1000 Subject: [PATCH 5/5] Fix project-rule finding filter to drop findings on empty file set and add related tests --- .claude/hooks/gruff-code-quality.sh | 71 +++++++++--- .codex/hooks/gruff-code-quality.sh | 71 +++++++++--- .goat-flow/footguns/commands.md | 12 +- .goat-flow/footguns/hooks.md | 26 +++++ .goat-flow/footguns/rules.md | 22 +++- src/Command/AnalysisFindingSupport.php | 16 ++- src/Command/AnalysisPipeline.php | 28 ++--- src/Command/AnalysisSourceSet.php | 14 +++ src/Command/BranchReviewBuilder.php | 18 +-- .../StaticAnalysisRedundantTestRule.php | 49 ++++++-- tests/Command/AnalysisFindingSupportTest.php | 109 ++++++++++++++++++ 11 files changed, 357 insertions(+), 79 deletions(-) create mode 100644 .goat-flow/footguns/hooks.md create mode 100644 tests/Command/AnalysisFindingSupportTest.php diff --git a/.claude/hooks/gruff-code-quality.sh b/.claude/hooks/gruff-code-quality.sh index 2cf90da6..52f7f6a2 100755 --- a/.claude/hooks/gruff-code-quality.sh +++ b/.claude/hooks/gruff-code-quality.sh @@ -358,7 +358,14 @@ git_diff_ranges() { [[ -f "$abs_path" ]] && all_file_range "$abs_path" return fi - diff_output="$(git -C "$root" diff --unified=0 -- "$rel_path" 2>/dev/null || true)" + # Diff against HEAD so staged-only edits are scoped too: discovery already includes + # `--cached` paths, so a file whose only changes are staged would otherwise yield no + # ranges and be skipped. Fall back to the index diff on an unborn branch with no HEAD. + if git -C "$root" rev-parse --verify --quiet HEAD >/dev/null 2>&1; then + diff_output="$(git -C "$root" diff HEAD --unified=0 -- "$rel_path" 2>/dev/null || true)" + else + diff_output="$(git -C "$root" diff --cached --unified=0 -- "$rel_path" 2>/dev/null || true)" + fi parse_diff_ranges "$diff_output" } @@ -367,11 +374,17 @@ changed_ranges() { local root="$2" local rel_path="$3" local abs_path="$4" + local file_count="${5:-1}" local ranges - ranges="$(payload_ranges "$payload")" - if [[ -n "$ranges" ]]; then - printf '%s' "$ranges" - return + # A payload's changed_ranges is a single flat list with no per-file attribution, so trust it only + # for a single-file edit. With several edited files, sharing one range set would mis-scope findings + # for every file but the one the ranges came from, so derive each file's ranges from git instead. + if [[ "$file_count" -le 1 ]]; then + ranges="$(payload_ranges "$payload")" + if [[ -n "$ranges" ]]; then + printf '%s' "$ranges" + return + fi fi git_diff_ranges "$root" "$rel_path" "$abs_path" } @@ -400,6 +413,25 @@ self_test() { return 1 } + # A single edited file trusts the payload's changed_ranges; several edited files must not share + # one range set, so changed_ranges falls back to per-file git ranges (empty under a bogus root). + [[ "$(changed_ranges "$payload" "/nonexistent" "src/a.mts" "/nonexistent/src/a.mts" 1)" == "2-4" ]] || { + printf 'gruff-code-quality self-test: single-file payload range failed\n' >&2 + return 1 + } + [[ -z "$(changed_ranges "$payload" "/nonexistent" "src/a.mts" "/nonexistent/src/a.mts" 2)" ]] || { + printf 'gruff-code-quality self-test: multi-file payload range sharing not suppressed\n' >&2 + return 1 + } + + # An invalid or sub-1 timeout floors at 30 so the value used and the value reported agree. + [[ "$(GRUFF_CODE_QUALITY_TIMEOUT_SECONDS=bogus normalized_timeout_seconds)" == "30" \ + && "$(GRUFF_CODE_QUALITY_TIMEOUT_SECONDS=0 normalized_timeout_seconds)" == "30" \ + && "$(GRUFF_CODE_QUALITY_TIMEOUT_SECONDS=45 normalized_timeout_seconds)" == "45" ]] || { + printf 'gruff-code-quality self-test: timeout normalization failed\n' >&2 + return 1 + } + [[ "$(min_severity_rank warning)" == "2" && "$(min_severity_rank error)" == "3" && "$(min_severity_rank bogus)" == "1" ]] || { printf 'gruff-code-quality self-test: min_severity_rank mapping failed\n' >&2 return 1 @@ -467,6 +499,17 @@ supports_json_format() { [[ "$help" == *"--format"* || "$help" == *"-format"* ]] } +# Resolve the analyzer timeout, flooring any non-numeric or sub-1 value at the +# 30-second default. Centralised so the value passed to `timeout` and the value +# named in the timeout/kill diagnostic are always the same number. +normalized_timeout_seconds() { + local timeout_seconds="${GRUFF_CODE_QUALITY_TIMEOUT_SECONDS:-}" + if ! [[ "$timeout_seconds" =~ ^[0-9]+$ ]] || [[ "$timeout_seconds" -lt 1 ]]; then + timeout_seconds=30 + fi + printf '%s' "$timeout_seconds" +} + run_gruff_json() { local binary_path="$1" local help="$2" @@ -489,10 +532,7 @@ run_gruff_json() { return 64 fi - timeout_seconds="$GRUFF_CODE_QUALITY_TIMEOUT_SECONDS" - if ! [[ "$timeout_seconds" =~ ^[0-9]+$ ]] || [[ "$timeout_seconds" -lt 1 ]]; then - timeout_seconds=30 - fi + timeout_seconds="$(normalized_timeout_seconds)" if command -v timeout >/dev/null 2>&1; then timeout "$timeout_seconds" "$binary_path" "${args[@]}" "$file_path" 2>&1 @@ -692,7 +732,7 @@ print_scope_header() { local err="$5" local warn="$6" local adv="$7" - printf 'gruff-code-quality: %s %s changed-lines=%s; %s on changed lines: %s error, %s warning, %s advisory\n' \ + printf 'gruff-code-quality: %s %s changed-lines=%s; %s in changed scope: %s error, %s warning, %s advisory\n' \ "$binary" "$rel_path" "$ranges" "$total" "$err" "$warn" "$adv" } @@ -700,6 +740,7 @@ process_file() { local payload="$1" local root="$2" local file_path="$3" + local file_count="${4:-1}" local rel_path abs_path binary binary_path config_file local ranges help output status suppressed ignored_desc uses_native_regions local max_findings floor_rank report_json scope_fields @@ -730,7 +771,7 @@ process_file() { return 0 fi - ranges="$(changed_ranges "$payload" "$root" "$rel_path" "$abs_path")" + ranges="$(changed_ranges "$payload" "$root" "$rel_path" "$abs_path" "$file_count")" if [[ -z "$ranges" ]]; then printf 'gruff-code-quality: no changed lines detected for %s; skipping gruff output\n' "$rel_path" >&2 return 0 @@ -752,7 +793,7 @@ process_file() { set -e if [[ "$status" -eq 124 || "$status" -eq 137 ]]; then - printf 'gruff-code-quality: %s exceeded %ss or was killed; changed-line filtering skipped\n' "$binary" "$GRUFF_CODE_QUALITY_TIMEOUT_SECONDS" >&2 + printf 'gruff-code-quality: %s exceeded %ss or was killed; changed-line filtering skipped\n' "$binary" "$(normalized_timeout_seconds)" >&2 return 0 fi if [[ -z "$output" ]]; then @@ -817,13 +858,13 @@ process_file() { printf '%s' "$report_json" | jq -r '.lines[]' 2>/dev/null || true fi if [[ "$more" -gt 0 ]]; then - printf 'gruff-code-quality: (%s more on changed lines; raise GRUFF_CODE_QUALITY_MAX_FINDINGS to list them)\n' "$more" + printf 'gruff-code-quality: (%s more in changed scope; raise GRUFF_CODE_QUALITY_MAX_FINDINGS to list them)\n' "$more" fi if [[ "$floored" -gt 0 ]]; then printf 'gruff-code-quality: %s finding(s) below GRUFF_CODE_QUALITY_MIN_SEVERITY=%s not listed\n' "$floored" "${GRUFF_CODE_QUALITY_MIN_SEVERITY:-advisory}" fi if [[ "$suppressed" =~ ^[0-9]+$ && "$suppressed" -gt 0 ]]; then - printf 'gruff-code-quality: suppressed %s pre-existing finding(s) outside changed lines\n' "$suppressed" + printf 'gruff-code-quality: suppressed %s pre-existing finding(s) outside changed scope\n' "$suppressed" fi if [[ "$surfaced" -gt 0 ]]; then printf '%s\n' "$FOOTER" @@ -849,7 +890,7 @@ main() { [[ "${#file_paths[@]}" -gt 0 ]] || exit 0 for file_path in "${file_paths[@]}"; do - process_file "$payload" "$root" "$file_path" + process_file "$payload" "$root" "$file_path" "${#file_paths[@]}" done exit 0 } diff --git a/.codex/hooks/gruff-code-quality.sh b/.codex/hooks/gruff-code-quality.sh index 2cf90da6..52f7f6a2 100755 --- a/.codex/hooks/gruff-code-quality.sh +++ b/.codex/hooks/gruff-code-quality.sh @@ -358,7 +358,14 @@ git_diff_ranges() { [[ -f "$abs_path" ]] && all_file_range "$abs_path" return fi - diff_output="$(git -C "$root" diff --unified=0 -- "$rel_path" 2>/dev/null || true)" + # Diff against HEAD so staged-only edits are scoped too: discovery already includes + # `--cached` paths, so a file whose only changes are staged would otherwise yield no + # ranges and be skipped. Fall back to the index diff on an unborn branch with no HEAD. + if git -C "$root" rev-parse --verify --quiet HEAD >/dev/null 2>&1; then + diff_output="$(git -C "$root" diff HEAD --unified=0 -- "$rel_path" 2>/dev/null || true)" + else + diff_output="$(git -C "$root" diff --cached --unified=0 -- "$rel_path" 2>/dev/null || true)" + fi parse_diff_ranges "$diff_output" } @@ -367,11 +374,17 @@ changed_ranges() { local root="$2" local rel_path="$3" local abs_path="$4" + local file_count="${5:-1}" local ranges - ranges="$(payload_ranges "$payload")" - if [[ -n "$ranges" ]]; then - printf '%s' "$ranges" - return + # A payload's changed_ranges is a single flat list with no per-file attribution, so trust it only + # for a single-file edit. With several edited files, sharing one range set would mis-scope findings + # for every file but the one the ranges came from, so derive each file's ranges from git instead. + if [[ "$file_count" -le 1 ]]; then + ranges="$(payload_ranges "$payload")" + if [[ -n "$ranges" ]]; then + printf '%s' "$ranges" + return + fi fi git_diff_ranges "$root" "$rel_path" "$abs_path" } @@ -400,6 +413,25 @@ self_test() { return 1 } + # A single edited file trusts the payload's changed_ranges; several edited files must not share + # one range set, so changed_ranges falls back to per-file git ranges (empty under a bogus root). + [[ "$(changed_ranges "$payload" "/nonexistent" "src/a.mts" "/nonexistent/src/a.mts" 1)" == "2-4" ]] || { + printf 'gruff-code-quality self-test: single-file payload range failed\n' >&2 + return 1 + } + [[ -z "$(changed_ranges "$payload" "/nonexistent" "src/a.mts" "/nonexistent/src/a.mts" 2)" ]] || { + printf 'gruff-code-quality self-test: multi-file payload range sharing not suppressed\n' >&2 + return 1 + } + + # An invalid or sub-1 timeout floors at 30 so the value used and the value reported agree. + [[ "$(GRUFF_CODE_QUALITY_TIMEOUT_SECONDS=bogus normalized_timeout_seconds)" == "30" \ + && "$(GRUFF_CODE_QUALITY_TIMEOUT_SECONDS=0 normalized_timeout_seconds)" == "30" \ + && "$(GRUFF_CODE_QUALITY_TIMEOUT_SECONDS=45 normalized_timeout_seconds)" == "45" ]] || { + printf 'gruff-code-quality self-test: timeout normalization failed\n' >&2 + return 1 + } + [[ "$(min_severity_rank warning)" == "2" && "$(min_severity_rank error)" == "3" && "$(min_severity_rank bogus)" == "1" ]] || { printf 'gruff-code-quality self-test: min_severity_rank mapping failed\n' >&2 return 1 @@ -467,6 +499,17 @@ supports_json_format() { [[ "$help" == *"--format"* || "$help" == *"-format"* ]] } +# Resolve the analyzer timeout, flooring any non-numeric or sub-1 value at the +# 30-second default. Centralised so the value passed to `timeout` and the value +# named in the timeout/kill diagnostic are always the same number. +normalized_timeout_seconds() { + local timeout_seconds="${GRUFF_CODE_QUALITY_TIMEOUT_SECONDS:-}" + if ! [[ "$timeout_seconds" =~ ^[0-9]+$ ]] || [[ "$timeout_seconds" -lt 1 ]]; then + timeout_seconds=30 + fi + printf '%s' "$timeout_seconds" +} + run_gruff_json() { local binary_path="$1" local help="$2" @@ -489,10 +532,7 @@ run_gruff_json() { return 64 fi - timeout_seconds="$GRUFF_CODE_QUALITY_TIMEOUT_SECONDS" - if ! [[ "$timeout_seconds" =~ ^[0-9]+$ ]] || [[ "$timeout_seconds" -lt 1 ]]; then - timeout_seconds=30 - fi + timeout_seconds="$(normalized_timeout_seconds)" if command -v timeout >/dev/null 2>&1; then timeout "$timeout_seconds" "$binary_path" "${args[@]}" "$file_path" 2>&1 @@ -692,7 +732,7 @@ print_scope_header() { local err="$5" local warn="$6" local adv="$7" - printf 'gruff-code-quality: %s %s changed-lines=%s; %s on changed lines: %s error, %s warning, %s advisory\n' \ + printf 'gruff-code-quality: %s %s changed-lines=%s; %s in changed scope: %s error, %s warning, %s advisory\n' \ "$binary" "$rel_path" "$ranges" "$total" "$err" "$warn" "$adv" } @@ -700,6 +740,7 @@ process_file() { local payload="$1" local root="$2" local file_path="$3" + local file_count="${4:-1}" local rel_path abs_path binary binary_path config_file local ranges help output status suppressed ignored_desc uses_native_regions local max_findings floor_rank report_json scope_fields @@ -730,7 +771,7 @@ process_file() { return 0 fi - ranges="$(changed_ranges "$payload" "$root" "$rel_path" "$abs_path")" + ranges="$(changed_ranges "$payload" "$root" "$rel_path" "$abs_path" "$file_count")" if [[ -z "$ranges" ]]; then printf 'gruff-code-quality: no changed lines detected for %s; skipping gruff output\n' "$rel_path" >&2 return 0 @@ -752,7 +793,7 @@ process_file() { set -e if [[ "$status" -eq 124 || "$status" -eq 137 ]]; then - printf 'gruff-code-quality: %s exceeded %ss or was killed; changed-line filtering skipped\n' "$binary" "$GRUFF_CODE_QUALITY_TIMEOUT_SECONDS" >&2 + printf 'gruff-code-quality: %s exceeded %ss or was killed; changed-line filtering skipped\n' "$binary" "$(normalized_timeout_seconds)" >&2 return 0 fi if [[ -z "$output" ]]; then @@ -817,13 +858,13 @@ process_file() { printf '%s' "$report_json" | jq -r '.lines[]' 2>/dev/null || true fi if [[ "$more" -gt 0 ]]; then - printf 'gruff-code-quality: (%s more on changed lines; raise GRUFF_CODE_QUALITY_MAX_FINDINGS to list them)\n' "$more" + printf 'gruff-code-quality: (%s more in changed scope; raise GRUFF_CODE_QUALITY_MAX_FINDINGS to list them)\n' "$more" fi if [[ "$floored" -gt 0 ]]; then printf 'gruff-code-quality: %s finding(s) below GRUFF_CODE_QUALITY_MIN_SEVERITY=%s not listed\n' "$floored" "${GRUFF_CODE_QUALITY_MIN_SEVERITY:-advisory}" fi if [[ "$suppressed" =~ ^[0-9]+$ && "$suppressed" -gt 0 ]]; then - printf 'gruff-code-quality: suppressed %s pre-existing finding(s) outside changed lines\n' "$suppressed" + printf 'gruff-code-quality: suppressed %s pre-existing finding(s) outside changed scope\n' "$suppressed" fi if [[ "$surfaced" -gt 0 ]]; then printf '%s\n' "$FOOTER" @@ -849,7 +890,7 @@ main() { [[ "${#file_paths[@]}" -gt 0 ]] || exit 0 for file_path in "${file_paths[@]}"; do - process_file "$payload" "$root" "$file_path" + process_file "$payload" "$root" "$file_path" "${#file_paths[@]}" done exit 0 } diff --git a/.goat-flow/footguns/commands.md b/.goat-flow/footguns/commands.md index c4670e50..60dad4b6 100644 --- a/.goat-flow/footguns/commands.md +++ b/.goat-flow/footguns/commands.md @@ -1,6 +1,6 @@ --- category: commands -last_reviewed: 2026-05-31 +last_reviewed: 2026-06-04 --- # CLI Command Footguns @@ -25,6 +25,16 @@ The default-applied `gruff-baseline.json` matches accepted-debt findings to live **Prevention:** When refactoring a file that carries baseline-suppressed findings, first run `grep gruff-baseline.json` to learn which findings it has accepted, then either (a) add the new code *below* every suppressed finding and keep any edit above them net-zero in line count — the trick used to keep `stripTopLevelNullUnion` from shifting `PhpDocMixedOveruseRule`'s baselined methods — or (b) fix the resurfaced finding for real, or (c) regenerate with `gruff-php analyse --generate-baseline gruff-baseline.json` after reviewing the movement diff. +## Footgun: Finding-scope filters must treat an empty target set as drop-all, not pass-through + +**Status:** active | **Created:** 2026-06-04 | **Evidence:** OBSERVED + +`src/Command/AnalysisFindingSupport.php` holds sibling finding filters with deliberately different — and easy-to-confuse — empty-set semantics. `filterFindingsToChangedFiles` (search: `intentional drop-all`) returns `[]` on an empty changed set because "nothing changed" means nothing qualifies. `filterProjectRuleFindingsToFiles` (search: `filterProjectRuleFindingsToFiles`) originally short-circuited `if ($projectRuleIds === [] || $filePaths === []) return $findings;`, returning ALL findings when the requested path set discovered zero files. Because the legacy pipeline runs project rules over the whole-tree context regardless of the narrow request (`src/Rule/RuleRegistry.php`, search: `$projectUnits ?? $units`), a scoped run whose path matched no source files (e.g. `analyse some/dir-with-no-php --diff-vs main` with a project rule like `dead-code.unused-internal-class`) leaked whole-repo project findings into a run the user scoped to nothing. + +**Evidence:** PR #8 review (CodeRabbit, "Don't treat an empty discovered-file set as unscoped"). Both callers pass `AnalysisSourceSet::displayPaths()` (search: `displayPaths`), which is empty exactly when the requested paths discovered no files. The fix drops project-rule findings on an empty set (search: `nothing is in scope`) while still returning unchanged when there are simply no project rules to scope. `tests/Command/AnalysisFindingSupportTest.php` (search: `WhenNoFilesDiscovered`) locks the behaviour. + +**Prevention:** For any filter whose job is "keep findings inside scope set S", an empty S means "nothing is in scope" → drop the in-scope-only findings, not "no filter" → keep everything. Only return the input unchanged when the FILTER itself is inactive (no rule ids, no allowlist) — a different condition from an empty scope set. When adding a finding filter, write the empty-scope-set case as an explicit test before the happy path; the `=== [] return $findings` shortcut reads as a harmless guard but silently inverts the filter. + ## Resolved Entries ## Footgun: Dispatching a sub-command loses the caller's project-root context diff --git a/.goat-flow/footguns/hooks.md b/.goat-flow/footguns/hooks.md new file mode 100644 index 00000000..29c3a440 --- /dev/null +++ b/.goat-flow/footguns/hooks.md @@ -0,0 +1,26 @@ +--- +category: hooks +last_reviewed: 2026-06-04 +--- + +# Hook Footguns + +## Footgun: Hook file-discovery and changed-range derivation must cover the same git states + +**Status:** active | **Created:** 2026-06-04 | **Evidence:** OBSERVED + +The gruff-code-quality hook discovers candidate files and then derives changed-line ranges for each through two independent git queries that must agree on which working states count as "changed". `git_changed_supported_paths` (search: `git_changed_supported_paths`) enumerates unstaged, staged (`git diff --cached --name-only`), and untracked paths, but range derivation in `git_diff_ranges` originally ran only `git diff --unified=0` (working-tree-vs-index, i.e. unstaged). A file with staged-only edits was therefore selected by discovery but produced empty ranges, so the hook skipped it with "no changed lines detected" — a silent false negative for exactly the files a pre-commit workflow stages. + +**Evidence:** PR #8 review (Cursor Bugbot, "Staged paths without staged ranges"). Reproduction: in a repo with a staged-only change, the pre-fix `git_diff_ranges` returned empty; the fix diffs against `HEAD` (search: `Diff against HEAD so staged-only edits are scoped`), falling back to `git diff --cached` on an unborn branch with no HEAD, and now returns the staged ranges. The hook's own `--self-test=smoke` (search: `self-test=smoke`) and `bash -n` both pass after the change. + +**Prevention:** Whenever you broaden which git states the hook *discovers* (staged, untracked, a specific ref), broaden range derivation to match in the same change, or files fall into a "selected but no ranges → skipped" gap. Diffing against `HEAD` covers staged+unstaged in one query and is the safe default; reserve `--cached`-only for the no-HEAD (unborn branch) case. Review the discovery query and the range query as a pair. + +## Footgun: `.claude` and `.codex` hook scripts are byte-identical duplicates that must move in lockstep + +**Status:** active | **Created:** 2026-06-04 | **Evidence:** OBSERVED + +`.claude/hooks/gruff-code-quality.sh` and `.codex/hooks/gruff-code-quality.sh` are maintained as byte-identical copies (verified with `diff` — no differences across the entire script), one per peer agent surface. They are intentionally NOT shared or symlinked — the project keeps `.claude/**` and `.codex/**` as distinct agent-owned surfaces — so any fix applied to one is invisible to the other unless mirrored. A change made to only one copy leaves the other agent running the old, buggy behaviour while tests and the self-test on the edited copy pass. + +**Evidence:** PR #8 review surfaced this two ways: CodeRabbit flagged the duplication as a maintenance nitpick, and Copilot independently raised the same scope-header wording finding against BOTH copies. Every behavioural fix in PR #8 (staged ranges, timeout-message normalisation, multi-file range attribution, scope-header wording, two new self-test assertions) had to be applied to both files; the lockstep was confirmed with `diff .claude/hooks/gruff-code-quality.sh .codex/hooks/gruff-code-quality.sh` reporting identical. + +**Prevention:** Treat the two hook scripts as one logical file with two locations. After editing one, copy it over the other (`cp .claude/hooks/gruff-code-quality.sh .codex/hooks/gruff-code-quality.sh`) and confirm `diff` reports identical before claiming the fix done; run `--self-test=smoke` against both. Do not consolidate them into a shared sourced module — runtime isolation between agent surfaces is deliberate — but never edit just one. `.codex/**` is also an "Ask First" peer surface per `CLAUDE.md`, so mirror the change as part of the same task and disclose it. diff --git a/.goat-flow/footguns/rules.md b/.goat-flow/footguns/rules.md index 42a8d606..63f19454 100644 --- a/.goat-flow/footguns/rules.md +++ b/.goat-flow/footguns/rules.md @@ -1,6 +1,6 @@ --- category: rules -last_reviewed: 2026-06-01 +last_reviewed: 2026-06-04 --- # Rule Footguns @@ -118,6 +118,26 @@ as malformed type syntax, not prose. When composing long PHPStan type aliases, a alias name from its type body across physical lines; `tests/Mutation/InfectionReportParserTest.php` (search: `InvalidReportNestedA`) uses smaller intermediate aliases instead. +## Footgun: Member-existence rules must honour PHP's split case rules — methods case-insensitive, properties case-sensitive + +**Status:** active | **Created:** 2026-06-04 | **Evidence:** OBSERVED + +PHP resolves method names case-insensitively but property names case-sensitively: `method_exists($c, 'RENDER')` is true for a declared `render()`, yet `property_exists($c, 'LABEL')` is false for a declared `$label`. A rule that indexes or looks up member names with a single `strtolower()` for both buckets mis-handles properties. `src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php` (search: `memberCandidate`) originally lowercased both the declaration key and the asserted member, so `assertTrue(property_exists(Foo::class, 'LABEL'))` against a `$label` property — a test that actually fails at runtime — was reported as a static-analysis-redundant candidate, steering users to delete a test that was catching a real case typo. + +**Evidence:** PR #8 review (Codex P2, "Preserve property-name case in redundant-test matching"). Reproduction: a fixture with `public string $label` plus `assertTrue(property_exists(Widget::class, 'LABEL'))` was flagged pre-fix and is not flagged post-fix, while `method_exists(Widget::class, 'RENDER')` stays flagged. The fix indexes properties by their declared name (search: `PHP property names are case-sensitive, so index by the declared name as-is`) and keeps methods lowercased (search: `PHP resolves method names case-insensitively`); `memberCandidate` chooses the lookup key by member kind. + +**Prevention:** Any rule that matches class members by name must split case handling by kind. Case-insensitive: methods, functions, class/interface/trait/enum names (lowercase both sides). Case-sensitive: properties, class constants, enum cases, variables (compare verbatim). When a member-matching rule lands, add a wrong-case fixture for every case-sensitive member kind it inspects and assert it is NOT matched, so a future single-`strtolower()` shortcut fails the test. + +## Footgun: `NodeIndex` enumerates declarations nested in functions and conditionals, not just top-level ones + +**Status:** active | **Created:** 2026-06-04 | **Evidence:** OBSERVED + +`NodeIndex::nodesOfAny`/`nodesOf` return matches from a full preorder walk of the whole unit — `src/Rule/NodeIndex.php` (search: `traverse($analysisUnit->statements)`) visits every descendant, so a query for `Stmt\Class_`/`Interface_`/`Trait_`/`Enum_` also returns class-likes declared inside functions, methods, `if` blocks, and other conditionals. PHP does not register those symbols until the enclosing path runs (`class_exists(Foo::class, false)` is false before a nested `class Foo {}` executes), so a rule that treats every indexed declaration as "statically guaranteed to exist" over-claims. `src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php` (search: `topLevelClassLikes`) hit this: a `class` declared inside `if (!class_exists(...)) { ... }` (a common polyfill shape) was treated as proven, so an `assertTrue(class_exists(...))` that genuinely tests the runtime branch was flagged as redundant. + +**Evidence:** PR #8 review (Codex P2, "Skip non-top-level declarations for redundant checks"). Reproduction: a conditionally-declared `Conditional` class plus `assertTrue(class_exists(Conditional::class))` was flagged pre-fix and is not flagged post-fix. The fix walks `$analysisUnit->statements` and only collects class-likes at file scope or directly inside a `Stmt\Namespace_` body (search: `topLevelClassLikes`), instead of using the full-AST `NodeIndex` enumeration. + +**Prevention:** When a rule needs declarations that PHP registers unconditionally (top-level symbols, "this type definitely exists"), do not enumerate them via `NodeIndex` — it has no scope filter. Collect them from `$analysisUnit->statements` plus each `Stmt\Namespace_::$stmts` directly, which excludes function/method/conditional bodies. `NodeIndex` stays correct for "find every node of this shape anywhere" queries (most rules); the trap is specifically assuming its results are top-level. + ## Resolved Entries ## Footgun: Project rules need full project context, not `--changed-only` diff --git a/src/Command/AnalysisFindingSupport.php b/src/Command/AnalysisFindingSupport.php index 0cda6461..2ce76906 100644 --- a/src/Command/AnalysisFindingSupport.php +++ b/src/Command/AnalysisFindingSupport.php @@ -70,18 +70,28 @@ public function filterFindingsToChangedFiles(array $findings, array $changedFile * * @param list $findings - Findings to filter. * @param list $projectRuleIds - Rule ids whose output came from project-wide context. - * @param list $filePaths - Project-relative display paths in the requested source set. + * @param list $filePaths - Project-relative display paths in the requested source set; an empty set drops every project-rule finding. * * @return list - Findings with out-of-scope project-rule rows removed. */ public function filterProjectRuleFindingsToFiles(array $findings, array $projectRuleIds, array $filePaths): array { - if ($projectRuleIds === [] || $filePaths === []) { + if ($projectRuleIds === []) { return $findings; } $projectRules = array_fill_keys($projectRuleIds, true); - $files = array_fill_keys($filePaths, true); + + if ($filePaths === []) { + // The invocation requested files but discovered none, so nothing is in scope. Drop every + // project-rule finding rather than leaking the whole-project context this run never loaded. + 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, diff --git a/src/Command/AnalysisPipeline.php b/src/Command/AnalysisPipeline.php index 25436b2d..a9a64ccf 100644 --- a/src/Command/AnalysisPipeline.php +++ b/src/Command/AnalysisPipeline.php @@ -18,7 +18,6 @@ use GruffPhp\Rule\RuleRegistry; use GruffPhp\Rule\RuleRunnerObserver; use Closure; -use GruffPhp\Source\SourceFile; use GruffPhp\Source\SourceDiscoveryResult; /** @@ -93,7 +92,7 @@ public function runAnalysis( ]; } - if ($this->canStream($options, $reviewDiff, $ruleContext)) { + if ($this->canStream($projectRoot, $options, $reviewDiff, $ruleContext)) { // Streaming is safe here, so take the low-peak-memory path that releases each unit after analysis. return $this->runStreaming( projectRoot: $projectRoot, @@ -122,6 +121,7 @@ public function runAnalysis( /** * Decide whether streaming parse → analyse → release is safe for this run. * + * @param string $projectRoot - Project root requested paths resolve against. * @param AnalyseCommandOptions $options - CLI options; changed-region and diff modes force the legacy path. * @param DiffResult|null $reviewDiff - Review diff metadata; an active review keeps the base snapshot. * @param RuleContext $ruleContext - Context whose enabled rules must all tolerate per-unit release. @@ -130,11 +130,16 @@ public function runAnalysis( * mode forces the legacy load-all path */ private function canStream( + string $projectRoot, AnalyseCommandOptions $options, ?DiffResult $reviewDiff, RuleContext $ruleContext, ): bool { - $hasNarrowProjectRuleContext = $options->paths !== [] + // An explicit project-root request ('.', './', or the root path itself) still covers the whole + // tree, so it can stream like a bare invocation; only a genuinely narrower path needs the legacy + // load-all flow that pulls whole-tree project context separately from the requested files. + $requestedPaths = (new AnalysisFindingSupport())->normaliseRequestedPaths($projectRoot, $options->paths); + $hasNarrowProjectRuleContext = $requestedPaths !== [] && $requestedPaths !== ['.'] && $this->registry->hasEnabledProjectRules($ruleContext->config); // Stream only when no review/diff retains the base snapshot and every enabled rule tolerates per-unit release. @@ -310,7 +315,7 @@ private function runLegacy( $findings = (new AnalysisFindingSupport())->filterProjectRuleFindingsToFiles( $findings, $this->registry->enabledProjectRuleIds($config), - $this->sourceFilePaths($sources), + $sources->displayPaths(), ); $analyseNs = hrtime(true) - $analyseStart; @@ -323,19 +328,4 @@ private function runLegacy( 'projectContextUnits' => $projectContextUnits, ]; } - - /** - * Return display paths from the source set requested by this invocation. - * - * @param AnalysisSourceSet $sources - Loaded sources for the requested analysis paths. - * - * @return list - Project-relative source file paths in discovery order. - */ - private function sourceFilePaths(AnalysisSourceSet $sources): array - { - return array_map( - static fn(SourceFile $sourceFile): string => $sourceFile->displayPath, - $sources->discovery->files, - ); - } } diff --git a/src/Command/AnalysisSourceSet.php b/src/Command/AnalysisSourceSet.php index aea0647b..91f7723f 100644 --- a/src/Command/AnalysisSourceSet.php +++ b/src/Command/AnalysisSourceSet.php @@ -7,6 +7,7 @@ use GruffPhp\Analysis\RunDiagnostic; use GruffPhp\Parser\AnalysisUnit; use GruffPhp\Source\SourceDiscoveryResult; +use GruffPhp\Source\SourceFile; /** * Carries parsed analysis units, diagnostics, and discovery metadata. @@ -34,6 +35,19 @@ public function __construct( $this->explicitParsedFileCount = $parsedFileCount; } + /** + * List the project-relative display paths of the discovered source files. + * + * @return list - Project-relative source file paths in discovery order. + */ + public function displayPaths(): array + { + return array_map( + static fn (SourceFile $sourceFile): string => $sourceFile->displayPath, + $this->discovery->files, + ); + } + /** * Count successfully parsed analysis units in the loaded source set. * diff --git a/src/Command/BranchReviewBuilder.php b/src/Command/BranchReviewBuilder.php index 431b7aa3..57edfc81 100644 --- a/src/Command/BranchReviewBuilder.php +++ b/src/Command/BranchReviewBuilder.php @@ -18,7 +18,6 @@ use GruffPhp\Rule\RuleContext; use GruffPhp\Rule\RuleRegistry; use GruffPhp\Scoring\ScoreCalculator; -use GruffPhp\Source\SourceFile; use RuntimeException; /** @@ -94,7 +93,7 @@ public function build( $baseFindings = (new AnalysisFindingSupport())->filterProjectRuleFindingsToFiles( $baseFindings, $baseRegistry->enabledProjectRuleIds($config), - $this->sourceFilePaths($baseSources), + $baseSources->displayPaths(), ); $baseFindings = (new AnalysisFindingSupport())->filterAllowedSecretPreviews($baseFindings, $config); } @@ -299,19 +298,4 @@ private function shouldLoadProjectContext( && $reviewDiff instanceof DiffResult && $reviewDiff->changedFiles !== []; } - - /** - * Return display paths from a source set. - * - * @param AnalysisSourceSet $sources - Source set loaded for requested analysis paths. - * - * @return list - Project-relative display paths in discovery order. - */ - private function sourceFilePaths(AnalysisSourceSet $sources): array - { - return array_map( - static fn(SourceFile $sourceFile): string => $sourceFile->displayPath, - $sources->discovery->files, - ); - } } diff --git a/src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php b/src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php index 61e50f76..0537b059 100644 --- a/src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php +++ b/src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php @@ -10,7 +10,6 @@ use GruffPhp\Finding\RuleTier; use GruffPhp\Finding\Severity; use GruffPhp\Parser\AnalysisUnit; -use GruffPhp\Rule\NodeIndex; use GruffPhp\Rule\RuleContext; use GruffPhp\Rule\RuleDefinition; use GruffPhp\Rule\RuleInterface; @@ -122,11 +121,8 @@ private function collectDeclarations(AnalysisUnit $analysisUnit): array { $declarations = []; - foreach (NodeIndex::nodesOfAny( - $analysisUnit, - [Stmt\Class_::class, Stmt\Interface_::class, Stmt\Trait_::class, Stmt\Enum_::class], - ) as $node) { - if (!$node instanceof Stmt\ClassLike || $node->name === null) { + foreach ($this->topLevelClassLikes($analysisUnit) as $node) { + if ($node->name === null) { continue; } @@ -144,6 +140,7 @@ private function collectDeclarations(AnalysisUnit $analysisUnit): array foreach ($node->stmts as $statement) { if ($statement instanceof Stmt\ClassMethod) { + // PHP resolves method names case-insensitively, so index by the lowercase name. $methodName = $statement->name->toString(); $record['methods'][strtolower($methodName)] = $methodName; continue; @@ -151,8 +148,9 @@ private function collectDeclarations(AnalysisUnit $analysisUnit): array if ($statement instanceof Stmt\Property) { foreach ($statement->props as $property) { + // PHP property names are case-sensitive, so index by the declared name as-is. $propertyName = $property->name->toString(); - $record['properties'][strtolower($propertyName)] = $propertyName; + $record['properties'][$propertyName] = $propertyName; } } } @@ -165,6 +163,38 @@ private function collectDeclarations(AnalysisUnit $analysisUnit): array return $declarations; } + /** + * Collect class-like declarations PHP registers unconditionally: those at the file top level or + * directly inside a namespace block. Declarations nested in functions, methods, or conditional + * blocks are only registered once that code path runs, so a static existence assertion against + * them is not provably redundant and must be excluded from the index. + * + * @param AnalysisUnit $analysisUnit - Parsed unit whose top-level declarations should be collected. + * + * @return list - Class-like declarations at file or namespace scope, in source order. + */ + private function topLevelClassLikes(AnalysisUnit $analysisUnit): array + { + $classLikes = []; + + foreach ($analysisUnit->statements as $statement) { + if ($statement instanceof Stmt\Namespace_) { + foreach ($statement->stmts as $namespaceStatement) { + if ($namespaceStatement instanceof Stmt\ClassLike) { + $classLikes[] = $namespaceStatement; + } + } + continue; + } + + if ($statement instanceof Stmt\ClassLike) { + $classLikes[] = $statement; + } + } + + return $classLikes; + } + /** * Build a candidate metadata payload when a source declaration proves the subject call. * @@ -233,7 +263,10 @@ private function memberCandidate(Expr\FuncCall $subjectCall, array $declaration, return null; } - $declaredName = $declaration[$memberBucket][strtolower($member)] ?? null; + // Methods resolve case-insensitively in PHP; properties do not. Look each up the way the + // language resolves it so a wrong-case property_exists() is not mistaken for a proven member. + $memberKey = $memberKind === 'property' ? $member : strtolower($member); + $declaredName = $declaration[$memberBucket][$memberKey] ?? null; if (!is_string($declaredName)) { return null; } diff --git a/tests/Command/AnalysisFindingSupportTest.php b/tests/Command/AnalysisFindingSupportTest.php new file mode 100644 index 00000000..a1c79cdf --- /dev/null +++ b/tests/Command/AnalysisFindingSupportTest.php @@ -0,0 +1,109 @@ +finding('dead-code.unused-internal-class', 'src/Other.php'), + $this->finding('test-quality.weak-assertion', 'src/Requested.php'), + ]; + + $filtered = $support->filterProjectRuleFindingsToFiles( + $findings, + ['dead-code.unused-internal-class'], + [], + ); + + self::assertSame( + ['test-quality.weak-assertion'], + array_map(static fn (Finding $finding): string => $finding->ruleId, $filtered), + ); + } + + /** + * Verify project-rule findings stay scoped to the requested files when files are discovered. + * + * @return void + */ + public function testFilterProjectRuleFindingsKeepsProjectFindingsInsideRequestedFiles(): void + { + $support = new AnalysisFindingSupport(); + $findings = [ + $this->finding('dead-code.unused-internal-class', 'src/Requested.php'), + $this->finding('dead-code.unused-internal-class', 'src/Other.php'), + $this->finding('test-quality.weak-assertion', 'src/Other.php'), + ]; + + $filtered = $support->filterProjectRuleFindingsToFiles( + $findings, + ['dead-code.unused-internal-class'], + ['src/Requested.php'], + ); + + self::assertSame( + ['src/Requested.php', 'src/Other.php'], + array_map(static fn (Finding $finding): string => $finding->filePath, $filtered), + ); + } + + /** + * Verify findings pass through untouched when no project rules are enabled. + * + * @return void + */ + public function testFilterProjectRuleFindingsReturnsAllWhenNoProjectRules(): void + { + $support = new AnalysisFindingSupport(); + $findings = [$this->finding('dead-code.unused-internal-class', 'src/Other.php')]; + + self::assertSame( + $findings, + $support->filterProjectRuleFindingsToFiles($findings, [], []), + ); + } + + /** + * Build a minimal advisory finding for the given rule id and file path. + * + * @param string $ruleId - Rule id to attach to the finding. + * @param string $filePath - File path to attach to the finding. + * + * @return Finding - Minimal finding used for filter assertions. + */ + private function finding(string $ruleId, string $filePath): Finding + { + return new Finding( + ruleId: $ruleId, + message: 'message', + filePath: $filePath, + line: 1, + severity: Severity::Advisory, + pillar: Pillar::TestQuality, + tier: RuleTier::V01, + confidence: Confidence::High, + ); + } +}