v0.3.1#8
Conversation
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds StaticAnalysisRedundantTestRule; refactors gruff-code-quality hooks for jq-first multi-path discovery, native changed-region delegation, and severity-ranked JSON reporting; extends DeadCode YAML _controller indexing; and updates reporters, tests, fixtures, docs, and hook wiring. ChangesStatic-analysis-redundant-test rule and hook script refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f55f5ee263
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return null; | ||
| } | ||
|
|
||
| $declaredName = $declaration[$memberBucket][strtolower($member)] ?? null; |
There was a problem hiding this comment.
For property_exists() candidates this lower-cases the asserted member name before lookup. PHP property names are case-sensitive, so property_exists(Foo::class, 'LABEL') is false when the declaration is only $label; with the new rule enabled, that typo/case regression is reported as a redundant static-fact assertion and can lead users to remove the test instead of fixing the property name. Keep method lookups case-insensitive, but compare properties with their original case.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.codex/hooks/gruff-code-quality.sh (1)
1-857: 💤 Low valueConsider extracting shared hook logic to reduce duplication.
This file is nearly identical to
.claude/hooks/gruff-code-quality.sh. Both scripts must be updated in lockstep, which creates maintenance risk. A shared script (e.g., in a common location likebin/or.goat-flow/hooks/) sourced or symlinked by both could reduce drift.That said, separate copies may be intentional for runtime isolation—defer if so.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.codex/hooks/gruff-code-quality.sh around lines 1 - 857, The hook is duplicated with .claude/hooks/gruff-code-quality.sh; extract the common logic into a single sourced module (e.g., bin/gruff-code-quality-common.sh) and have both scripts source it. Move shared functions and symbols such as read_stdin, json_file_paths, variant_for_path, discover_binary, changed_ranges, run_gruff_json, valid_gruff_json, changed_findings_report, suppressed_count, ignored_descriptor, process_file, and main-support helpers into the common file and leave only small wrapper scripts that set any runner-specific constants and call the shared main/process_file entrypoint; update both .codex/hooks/gruff-code-quality.sh and .claude/hooks/gruff-code-quality.sh to source the common file and delegate to the shared functions so future fixes are made once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.codex/hooks/gruff-code-quality.sh:
- Around line 1-857: The hook is duplicated with
.claude/hooks/gruff-code-quality.sh; extract the common logic into a single
sourced module (e.g., bin/gruff-code-quality-common.sh) and have both scripts
source it. Move shared functions and symbols such as read_stdin,
json_file_paths, variant_for_path, discover_binary, changed_ranges,
run_gruff_json, valid_gruff_json, changed_findings_report, suppressed_count,
ignored_descriptor, process_file, and main-support helpers into the common file
and leave only small wrapper scripts that set any runner-specific constants and
call the shared main/process_file entrypoint; update both
.codex/hooks/gruff-code-quality.sh and .claude/hooks/gruff-code-quality.sh to
source the common file and delegate to the shared functions so future fixes are
made once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d31c1fa3-5a4e-475b-92e0-84c53599b75a
📒 Files selected for processing (19)
.claude/hooks/gruff-code-quality.sh.claude/settings.json.codex/hooks/gruff-code-quality.sh.goat-flow/architecture.md.goat-flow/code-map.md.goat-flow/decisions/ADR-022-test-quality-gate-parity.md.gruff-php.yamlcomposer.jsondocs/rules.mdsrc/Command/SummaryCommand.phpsrc/Reporting/TextReporter.phpsrc/Rule/RuleRegistry.phpsrc/Rule/TestQuality/StaticAnalysisRedundantTestRule.phptests/Console/GruffCliSummaryTest.phptests/Fixtures/Cli/Golden/text-warning.txttests/Fixtures/TestQuality/static-analysis-redundant-test.phptests/Rule/RuleRegistryTest.phptests/Rule/RuleRegressionSnapshotTest.phptests/Rule/TestQuality/TestQualityRulesTest.php
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6b30ae7e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| foreach (NodeIndex::nodesOfAny( | ||
| $analysisUnit, | ||
| [Stmt\Class_::class, Stmt\Interface_::class, Stmt\Trait_::class, Stmt\Enum_::class], | ||
| ) as $node) { |
There was a problem hiding this comment.
When a test file contains a class/interface/trait/enum declared inside a function, method, or conditional block, PHP does not register that symbol until that code path executes; for example class_exists(Foo::class, false) before a nested declaration is still false. Because this indexes every class-like node returned by the full-AST NodeIndex walk, the new rule treats those runtime-dependent declarations as statically proven and can report a non-redundant existence assertion as something users should remove. Restrict the declaration index to top-level/namespace declarations, or otherwise exclude conditional/nested declarations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Console/GruffCliSummaryTest.php (1)
40-40: ⚡ Quick winUse
Application::VERSIONinstead of hardcoded release strings in assertions.This avoids touching multiple tests on every version bump.
Proposed refactor
+use GruffPhp\Console\Application; use JsonException; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Symfony\Component\Process\Process; @@ - self::assertStringContainsString('gruff-php 0.3.1 summary', $output); + self::assertStringContainsString('gruff-php ' . Application::VERSION . ' summary', $output); @@ - self::assertSame('0.3.1', $tool['version'] ?? null); + self::assertSame(Application::VERSION, $tool['version'] ?? null);Also applies to: 111-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Console/GruffCliSummaryTest.php` at line 40, Replace the hardcoded version string in the assertion with the app's version constant: instead of asserting 'gruff-php 0.3.1 summary', build the expected text using Application::VERSION (e.g., "gruff-php " . Application::VERSION . " summary") and use that in the assertStringContainsString call referring to $output; update all similar assertions (e.g., the one at line 111) so tests no longer rely on a pinned release string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Console/GruffCliSummaryTest.php`:
- Line 40: Replace the hardcoded version string in the assertion with the app's
version constant: instead of asserting 'gruff-php 0.3.1 summary', build the
expected text using Application::VERSION (e.g., "gruff-php " .
Application::VERSION . " summary") and use that in the
assertStringContainsString call referring to $output; update all similar
assertions (e.g., the one at line 111) so tests no longer rely on a pinned
release string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f2bb75c-576e-4501-8e8d-d21666e03f14
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
CHANGELOG.mdsrc/Console/Application.phpsrc/Rule/DeadCode/DeadCodeProjectIndex.phpsrc/Rule/DeadCode/UnusedInternalClassRule.phpsrc/Rule/ProjectSourceTextRuleAccumulator.phpsrc/Rule/RuleRegistry.phptests/Console/AnalyseCliTest.phptests/Console/GruffCliSummaryTest.phptests/Console/ListRulesCliTest.phptests/Fixtures/Cli/Golden/json-warning.jsontests/Fixtures/Cli/Golden/text-warning.txttests/Fixtures/DeadCode/project-wide/config/routes/block.ymltests/Fixtures/DeadCode/project-wide/config/routes/inline.yamltests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yamltests/Fixtures/DeadCode/project-wide/config/routes/quoted.yamltests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.phptests/Rule/DeadCode/ProjectDeadCodeRulesTest.phptests/Rule/RuleRegressionSnapshotTest.phptests/Rule/TestQuality/TestQualityRulesTest.php
✅ Files skipped from review due to trivial changes (7)
- tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml
- tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml
- tests/Console/AnalyseCliTest.php
- tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml
- CHANGELOG.md
- tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php
- tests/Fixtures/Cli/Golden/json-warning.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Command/AnalysisFindingSupport.php`:
- Around line 79-80: The early return currently uses the condition "if
($projectRuleIds === [] || $filePaths === []) { return $findings; }" which
treats an empty discovered-file set as unscoped; change the logic so an empty
$filePaths does not preserve all project-rule findings. Replace the OR with an
AND (i.e. only return when both $projectRuleIds and $filePaths are empty) or
alternatively, if $filePaths is empty, ensure project-level findings are
filtered out instead of returning $findings; update the check around
$projectRuleIds, $filePaths and the return so unrelated project-wide findings
are not kept.
In `@src/Command/AnalysisPipeline.php`:
- Around line 137-143: The check that sets $hasNarrowProjectRuleContext treats
any explicit paths (e.g. analyse .) as narrowing the project and disables
streaming; update the predicate in AnalysisPipeline.php so that only genuinely
narrow paths (paths that are not the project root) set
$hasNarrowProjectRuleContext. Concretely, change the condition using
$options->paths to detect and ignore a single root path (or "." equivalent) — or
add/use a helper on the options object (e.g. isRootOnly or similar) — so
BranchReviewBuilder::shouldLoadProjectContext() still recognizes full-project
invocations and avoids an extra full-tree parse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f125884b-c100-492c-a268-1ec4d2565468
📒 Files selected for processing (10)
CHANGELOG.mddocs/output-formats.mdsrc/Command/AnalyseCommand.phpsrc/Command/AnalysisFindingSupport.phpsrc/Command/AnalysisPipeline.phpsrc/Command/BranchReviewBuilder.phpsrc/Diff/DiffResult.phpsrc/Rule/DeadCode/DeadCodeProjectIndex.phpsrc/Rule/RuleRegistry.phptests/Console/AnalyseCliDiffTest.php
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Rule/DeadCode/DeadCodeProjectIndex.php
There was a problem hiding this comment.
Pull request overview
Release v0.3.1 updates the analyzer and tooling to improve test-quality detection, changed-region/diff accounting, dead-code indexing across Symfony YAML routes, and text output ergonomics—alongside the usual version/doc/test snapshot bumps.
Changes:
- Add advisory rule
test-quality.static-analysis-redundant-testand corresponding fixtures/tests. - Extend dead-code project indexing to treat Symfony YAML
_controller: FQCN::methodcallables as live references, including non-PHP source units. - Improve changed-region reporting (
suppressedCount/diff.suppressedCount) and lead text outputs with Composite/Findings tallies; update CLI fixtures/snapshots and agent hook scripts.
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Rule/TestQuality/TestQualityRulesTest.php | Refactors test-quality rule fixture assertions; adds coverage for the new static-analysis-redundant rule. |
| tests/Rule/RuleRegressionSnapshotTest.php | Updates regression snapshot counts/hash for new rules/fixtures. |
| tests/Rule/RuleRegistryTest.php | Registers the new rule in stable rule-id and definition snapshots. |
| tests/Rule/DeadCode/ProjectDeadCodeRulesTest.php | Adds project-wide fixtures/tests to ensure YAML _controller references keep controllers live; parses non-PHP units. |
| tests/Fixtures/TestQuality/static-analysis-redundant-test.php | New fixture exercising static declaration existence assertions and non-candidate cases. |
| tests/Fixtures/DeadCode/project-wide/src/Controller/RouteControllers.php | Adds controller classes referenced (or intentionally not referenced) by YAML routes. |
| tests/Fixtures/DeadCode/project-wide/config/routes/quoted.yaml | YAML route examples with quoted _controller values. |
| tests/Fixtures/DeadCode/project-wide/config/routes/non-fqcn.yaml | YAML route examples that should not be treated as FQCN controller references. |
| tests/Fixtures/DeadCode/project-wide/config/routes/inline.yaml | YAML inline mapping route example for _controller. |
| tests/Fixtures/DeadCode/project-wide/config/routes/block.yml | YAML block mapping route example for _controller. |
| tests/Fixtures/Cli/Golden/text-warning.txt | Updates golden text output (version + new top-of-report tallies). |
| tests/Fixtures/Cli/Golden/json-warning.json | Updates golden JSON output version. |
| tests/Console/ListRulesCliTest.php | Updates expected CLI version output. |
| tests/Console/GruffCliSummaryTest.php | Updates summary output assertions to match new formatting and version. |
| tests/Console/AnalyseCliTest.php | Updates analyse output assertions for version bump. |
| tests/Console/AnalyseCliDiffTest.php | Adds changed-region reconciliation tests and helper methods for JSON report assertions. |
| src/Rule/TestQuality/StaticAnalysisRedundantTestRule.php | Implements the new advisory rule for same-file *_exists-style static existence assertions. |
| src/Rule/RuleRegistry.php | Registers the new rule; adds enabled project-rule id helper; allows project accumulators to optionally consume text units. |
| src/Rule/ProjectSourceTextRuleAccumulator.php | Introduces marker interface for project accumulators that need non-PHP units. |
| src/Rule/DeadCode/UnusedInternalClassRule.php | Marks unused-internal-class rule as needing text units so YAML can contribute references. |
| src/Rule/DeadCode/DeadCodeProjectIndex.php | Adds Symfony YAML _controller parsing and reference recording for dead-code indexing. |
| src/Reporting/TextReporter.php | Moves composite/findings tallies to the top; simplifies score/summary sections. |
| src/Diff/DiffResult.php | Adds suppressedCount to diff metadata serialization. |
| src/Console/Application.php | Bumps CLI version to 0.3.1. |
| src/Command/SummaryCommand.php | Updates summary text formatting (header + Composite/Findings lines). |
| src/Command/BranchReviewBuilder.php | Filters project-rule findings to requested file set in base snapshot comparisons. |
| src/Command/AnalysisPipeline.php | Disables streaming when narrowed paths require project-rule context; filters project-rule findings to requested file set. |
| src/Command/AnalysisFindingSupport.php | Adds helper to scope project-rule findings to the invocation’s requested files. |
| src/Command/AnalyseCommand.php | Threads diff suppressedCount into DiffResult for reporting. |
| docs/rules.md | Updates rule catalogue counts and severities; documents the new rule. |
| docs/output-formats.md | Documents suppressedCount / diff.suppressedCount semantics for changed-region reports. |
| composer.lock | Dev dependency version bumps for the release. |
| composer.json | Updates package description rule count to 133. |
| CHANGELOG.md | Adds 0.3.1 release notes. |
| .gruff-php.yaml | Enables the new test-quality rule by default. |
| .goat-flow/decisions/ADR-022-test-quality-gate-parity.md | Records rationale for the new advisory candidate rule. |
| .goat-flow/code-map.md | Updates code map metadata and references the new rule. |
| .goat-flow/architecture.md | Updates architecture doc rule counts and test-quality rule list. |
| .codex/hooks/gruff-code-quality.sh | Enhances hook path/range extraction, severity sorting/capping/flooring, and native changed-region delegation. |
| .claude/settings.json | Simplifies repo-root detection and makes gruff hook fail-soft when git is unavailable. |
| .claude/hooks/gruff-code-quality.sh | Mirrors the codex hook improvements for Claude environments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private function sourceFilePaths(AnalysisSourceSet $sources): array | ||
| { | ||
| return array_map( | ||
| static fn(SourceFile $sourceFile): string => $sourceFile->displayPath, | ||
| $sources->discovery->files, |
| private function sourceFilePaths(AnalysisSourceSet $sources): array | ||
| { | ||
| return array_map( | ||
| static fn(SourceFile $sourceFile): string => $sourceFile->displayPath, | ||
| $sources->discovery->files, |
| printf 'gruff-code-quality: %s %s changed-lines=%s; %s on changed lines: %s error, %s warning, %s advisory\n' \ | ||
| "$binary" "$rel_path" "$ranges" "$total" "$err" "$warn" "$adv" |
| printf 'gruff-code-quality: %s %s changed-lines=%s; %s on changed lines: %s error, %s warning, %s advisory\n' \ | ||
| "$binary" "$rel_path" "$ranges" "$total" "$err" "$warn" "$adv" |
…d add related tests
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings.json:
- Line 75: The root-detection logic can yield the wrong repository root in
linked worktrees: in the PreToolUse inline command (the big "gcd=...; root=...;
case" statement in .claude/settings.json) and in resolve_goat_flow_root() inside
.claude/hooks/deny-dangerous.sh you should compute root using git rev-parse
--show-toplevel first (falling back to ${CLAUDE_PROJECT_DIR:-}) instead of using
the /*) root="$(dirname "$gcd")" branch that points at git common-dir; update
the /*) branch to set root="$(git rev-parse --show-toplevel 2>/dev/null ||
true)" or simply call git rev-parse --show-toplevel before the case, and in
resolve_goat_flow_root() ensure GOAT_FLOW_ROOT is assigned (e.g., set root then
export GOAT_FLOW_ROOT) so GOAT_HOOK_LIB_DIR resolves correctly and hooks remain
discoverable and executable.
In @.codex/config.toml:
- Around line 25-43: The deny globs in .codex/config.toml are too specific
(e.g., "**/.env.local", "**/credentials") and miss variants; update the deny
rules to mirror Claude’s broader policy by replacing or adding patterns like
"**/.env*" and "**/credentials*" (or equivalent expanded globs) so all .env*
suffixes and credentials* variants are covered; modify the existing entries that
mention "**/.env", "**/.env.local", "**/.env.production", and "**/credentials"
to use the wildcarded patterns and remove redundant specific entries to keep
Codex aligned with Claude.
In @.goat-flow/hook-lib/patterns-shell.sh:
- Around line 255-258: The xargs check only matches when the segment starts with
"xargs", so pipeline-fed cases (commands where xargs appears after a pipe) are
missed; update check_destructive_segment to detect xargs in any pipeline element
by splitting CMD_NORMALIZED on pipe ('|') boundaries and running
strip_xargs_payload_command on each trimmed pipeline component (or modify
strip_xargs_payload_command to search for a word-boundary '\bxargs\b' anywhere
in the input and extract its payload), then call rm_has_recursive on the
extracted payload and block if recursive rm is present; reference functions:
check_destructive_segment, strip_xargs_payload_command, rm_has_recursive, and
variable CMD_NORMALIZED.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc5a4f9e-92f8-4750-b35f-2d2d0833cb6d
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
.agents/skills/goat-critique/SKILL.md.agents/skills/goat-critique/references/rubric-examples.md.agents/skills/goat-critique/references/sub-agent-directives.md.agents/skills/goat-debug/SKILL.md.agents/skills/goat-plan/SKILL.md.agents/skills/goat-plan/references/issue-format.md.agents/skills/goat-plan/references/milestone-examples.md.agents/skills/goat-qa/SKILL.md.agents/skills/goat-review/SKILL.md.agents/skills/goat-review/references/automated-review.md.agents/skills/goat-review/references/examples.md.agents/skills/goat-review/references/refuter-spec.md.agents/skills/goat-security/SKILL.md.agents/skills/goat-security/references/common-threats.md.agents/skills/goat-security/references/file-upload-and-paths.md.agents/skills/goat-security/references/identity-and-data.md.agents/skills/goat-security/references/project-policy-template.md.agents/skills/goat-security/references/supply-chain-and-cicd.md.agents/skills/goat/SKILL.md.claude/hooks/deny-dangerous.sh.claude/settings.json.claude/skills/goat-critique/SKILL.md.claude/skills/goat-critique/references/rubric-examples.md.claude/skills/goat-critique/references/sub-agent-directives.md.claude/skills/goat-debug/SKILL.md.claude/skills/goat-plan/SKILL.md.claude/skills/goat-plan/references/issue-format.md.claude/skills/goat-plan/references/milestone-examples.md.claude/skills/goat-qa/SKILL.md.claude/skills/goat-review/SKILL.md.claude/skills/goat-review/references/automated-review.md.claude/skills/goat-review/references/examples.md.claude/skills/goat-review/references/refuter-spec.md.claude/skills/goat-security/SKILL.md.claude/skills/goat-security/references/common-threats.md.claude/skills/goat-security/references/file-upload-and-paths.md.claude/skills/goat-security/references/identity-and-data.md.claude/skills/goat-security/references/project-policy-template.md.claude/skills/goat-security/references/supply-chain-and-cicd.md.claude/skills/goat/SKILL.md.codex/config.toml.codex/hooks/deny-dangerous.sh.goat-flow/.gitignore.goat-flow/config.yaml.goat-flow/hook-lib/deny-dangerous-self-test.sh.goat-flow/hook-lib/patterns-shell.sh.goat-flow/hook-lib/patterns-writes.sh.goat-flow/lessons/workflow.md.goat-flow/logs/review/README.md.goat-flow/skill-playbooks/README.md.goat-flow/skill-playbooks/browser-use.md.goat-flow/skill-playbooks/changelog.md.goat-flow/skill-playbooks/code-comments.md.goat-flow/skill-playbooks/gruff-code-quality.md.goat-flow/skill-playbooks/observability.md.goat-flow/skill-playbooks/page-capture.md.goat-flow/skill-playbooks/release-notes.md.goat-flow/skill-playbooks/skill-quality-testing.md.goat-flow/skill-playbooks/skill-quality-testing/adversarial-framing.md.goat-flow/skill-playbooks/skill-quality-testing/deployment.md.goat-flow/skill-playbooks/skill-quality-testing/tdd-iteration.md.goat-flow/skill-reference/README.md.goat-flow/skill-reference/skill-conventions.md.goat-flow/skill-reference/skill-preamble.md
✅ Files skipped from review due to trivial changes (41)
- .goat-flow/config.yaml
- .claude/skills/goat-review/references/automated-review.md
- .claude/skills/goat-review/references/examples.md
- .claude/skills/goat-security/references/identity-and-data.md
- .agents/skills/goat-security/references/common-threats.md
- .goat-flow/skill-playbooks/skill-quality-testing/adversarial-framing.md
- .goat-flow/.gitignore
- .goat-flow/skill-reference/skill-conventions.md
- .agents/skills/goat-plan/references/milestone-examples.md
- .claude/skills/goat-plan/references/milestone-examples.md
- .agents/skills/goat-security/references/identity-and-data.md
- .claude/skills/goat-critique/references/sub-agent-directives.md
- .goat-flow/skill-playbooks/skill-quality-testing.md
- .claude/skills/goat-plan/references/issue-format.md
- .goat-flow/logs/review/README.md
- .agents/skills/goat-plan/references/issue-format.md
- .agents/skills/goat-security/references/project-policy-template.md
- .claude/skills/goat-critique/references/rubric-examples.md
- .agents/skills/goat-critique/references/rubric-examples.md
- .goat-flow/skill-playbooks/code-comments.md
- .goat-flow/skill-reference/README.md
- .claude/skills/goat-security/references/common-threats.md
- .goat-flow/skill-playbooks/release-notes.md
- .agents/skills/goat-critique/references/sub-agent-directives.md
- .agents/skills/goat-review/references/automated-review.md
- .claude/skills/goat-review/references/refuter-spec.md
- .goat-flow/skill-playbooks/observability.md
- .agents/skills/goat-security/references/file-upload-and-paths.md
- .goat-flow/skill-playbooks/README.md
- .agents/skills/goat-review/references/refuter-spec.md
- .claude/skills/goat-security/references/project-policy-template.md
- .agents/skills/goat-security/references/supply-chain-and-cicd.md
- .goat-flow/skill-playbooks/gruff-code-quality.md
- .agents/skills/goat-qa/SKILL.md
- .agents/skills/goat-review/references/examples.md
- .claude/skills/goat-security/references/file-upload-and-paths.md
- .goat-flow/skill-playbooks/changelog.md
- .goat-flow/skill-playbooks/browser-use.md
- .goat-flow/lessons/workflow.md
- .claude/skills/goat-security/references/supply-chain-and-cicd.md
- .goat-flow/skill-playbooks/page-capture.md
…ioning and rule catalogue changes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a1477216a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ($key === '_controller' && is_string($childValue)) { | ||
| $this->recordSymfonyControllerReferenceValue($childValue, $isTestFile); |
There was a problem hiding this comment.
When a Symfony route YAML file uses the common top-level controller: App\Controller\FooController::index form, this walker parses the file but ignores that key because it only records _controller. In those projects, route controllers referenced from config/routes.yaml are still reported by dead-code.unused-internal-class as unused even though they are live entrypoints; treat both _controller and controller as controller callable keys.
Useful? React with 👍 / 👎.
…ecks and installation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e21dce49b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $includeRules = $this->stringListOption($input, 'include-rule'); | ||
| $excludeRules = $this->stringListOption($input, 'exclude-rule'); | ||
| if ($includeRules !== [] || $excludeRules !== []) { | ||
| $config = $config->withRuleSelection(new RuleSelection(rules: $includeRules, excludeRules: $excludeRules)); |
There was a problem hiding this comment.
When a project config already narrows selection.rules, invoking hook --exclude-rule ... replaces that selection with a new RuleSelection whose include lists are empty, and empty includes mean every enabled rule is allowed before the exclude is applied. In that scenario a focused hook run unexpectedly widens to the whole configured rule set, producing unrelated findings instead of just removing the named rule from the existing selection.
Useful? React with 👍 / 👎.
| throw new RuntimeException(sprintf('Unable to read hook baseline: %s', $baselinePath)); | ||
| } | ||
|
|
||
| $decoded = json_decode($contents, true, 512, JSON_THROW_ON_ERROR); |
There was a problem hiding this comment.
If --baseline points at an existing but invalid JSON file, this json_decode(..., JSON_THROW_ON_ERROR) raises JsonException, which is not caught by the surrounding DiffException | RuntimeException handler in execute(). That breaks the hook contract for a common operational error by exiting through Symfony's exception path instead of returning the in-band empty hook report with config.error.
Useful? React with 👍 / 👎.
…oved readability and maintainability
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3f178d43d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tiers: $existing->tiers, | ||
| pillars: $existing->pillars, | ||
| rules: $includeRules !== [] ? $includeRules : $existing->rules, |
There was a problem hiding this comment.
When a project config already selects tiers or pillars, preserving those fields here makes hook --include-rule X run X plus every rule matched by the configured tier/pillar, because RuleSelection::allows() treats tiers, pillars, and rules as ORed include filters. That contradicts the hook help text (“Run only the named rule id”) and can surface unrelated findings in focused editor/agent hook runs; keep the existing selection for bare excludes, but when include rules are supplied the execution selection needs to be narrowed to those rule ids.
Useful? React with 👍 / 👎.
| 'file' => $finding->filePath, | ||
| 'symbol' => $finding->symbol, | ||
| 'qualifier' => self::qualifier($finding, $scope), | ||
| ]; |
There was a problem hiding this comment.
For line-scoped findings this identity omits the line and only includes rule/file/symbol plus a qualifier, so repeated same-rule diagnostics with the same message in one file collapse to one stableIdentity (for example two security.error-suppression @ uses have no symbol or metadata and the same message). In hook new-only mode, a baseline containing one such finding suppresses any newly added duplicate elsewhere in the same file, hiding fresh changed-code feedback from the agent.
Useful? React with 👍 / 👎.
…s and updating related classes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52cbbfbd25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "version": "1.9.0", | ||
| "resolved": "https://registry.npmjs.org/@blundergoat/goat-flow/-/goat-flow-1.9.0.tgz", | ||
| "integrity": "sha512-3LVOGfdOKtyuTdAjDQ7/kjlCjOysdTq0uadKhhPnIcuEzhMYjIx2rK3H47URB9/nIU0580BfsEeVJyTlrqSPmg==", | ||
| "version": "1.9.1", |
There was a problem hiding this comment.
With this commit .goat-flow/config.yaml and the committed skill metadata declare goat-flow 1.9.2, and scripts/dependency-install.sh now runs npm ci, which installs exactly the version recorded here. Because the lockfile still pins @blundergoat/goat-flow to 1.9.1, a fresh setup gets an older CLI/reference package than the repo configuration and skills expect, so goat-flow audit/hook syncs can run against the wrong version; regenerate the lockfile for 1.9.2 (and consider tightening package.json) so installs match the checked-in goat-flow surface.
Useful? React with 👍 / 👎.
… goat-flow version to 1.10.1
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1bc6814. Configure here.
| "hooks": [ | ||
| { | ||
| "type": "command", | ||
| "command": "bash -c 'gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\"; root=\"\"; case \"$gcd\" in */.git/modules/*|.git/modules/*) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; /*|[A-Za-z]:/*|[A-Za-z]:\\\\*) gcd=\"${gcd//\\\\//}\"; root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; esac; [ -f \"$root/.goat-flow/hooks/gruff-code-quality.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/gruff-code-quality.sh\" ] || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; cd \"$root\" || { printf '\\''{\"decision\":\"deny\",\"reason\":\"Policy hook unavailable: git repository root unavailable.\"}\\n'\\''; exit 0; }; bash \"$root/.goat-flow/hooks/gruff-code-quality.sh\"'", |
There was a problem hiding this comment.
Agents gruff hook denies when missing
Medium Severity
When .goat-flow/hooks/gruff-code-quality.sh is missing, the PostToolUse wrapper emits a JSON deny decision instead of skipping quietly. That treats an optional quality hook like a hard policy failure and can block or disrupt file edits even though the hook is designed to fail soft.
Reviewed by Cursor Bugbot for commit 1bc6814. Configure here.
| { | ||
| "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": "bash -c 'gcd=\"$(git rev-parse --git-common-dir 2>/dev/null)\"; root=\"\"; case \"$gcd\" in */.git/modules/*|.git/modules/*) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; /*|[A-Za-z]:/*|[A-Za-z]:\\\\*) gcd=\"${gcd//\\\\//}\"; root=\"$(dirname \"$gcd\")\" ;; *) root=\"$(git rev-parse --show-toplevel 2>/dev/null || true)\" ;; esac; [ -f \"$root/.goat-flow/hooks/gruff-code-quality.sh\" ] || root=\"${CLAUDE_PROJECT_DIR:-}\"; [ -f \"$root/.goat-flow/hooks/gruff-code-quality.sh\" ] || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; cd \"$root\" || { printf '\\''BLOCKED: Policy hook unavailable: git repository root unavailable.\\n'\\'' >&2; exit 2; }; bash \"$root/.goat-flow/hooks/gruff-code-quality.sh\"'" |
There was a problem hiding this comment.
Claude PostToolUse blocks missing gruff
Medium Severity
Edit and Write PostToolUse hooks now exit with code 2 and a BLOCKED message when .goat-flow/hooks/gruff-code-quality.sh is unavailable. That hard-fails the hook phase for an optional analyzer instead of allowing the edit to complete without gruff output.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1bc6814. Configure here.
| "MultiEdit(**/.pypirc)", | ||
| "MultiEdit(**/*.pfx)", | ||
| "MultiEdit(**/credentials*)", | ||
| "MultiEdit(**/.kube/config)", |
There was a problem hiding this comment.
MultiEdit secret denies removed
Medium Severity
The diff removes all MultiEdit(**/…) permission denies for secret and credential paths while keeping parallel Read, Edit, and Write rules. If Claude still exposes MultiEdit as a separate tool, those paths may no longer be blocked at the permissions layer.
Reviewed by Cursor Bugbot for commit 1bc6814. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bc6814d0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "Edit(**/.env*)", | ||
| "MultiEdit(**/.env*)", | ||
| "MultiEdit(**/secrets/**)", | ||
| "MultiEdit(**/*.pem)", | ||
| "MultiEdit(**/*.key)", | ||
| "MultiEdit(**/.ssh/**)", | ||
| "MultiEdit(**/.aws/**)", | ||
| "MultiEdit(**/.docker/config.json)", | ||
| "MultiEdit(**/.gnupg/**)", | ||
| "MultiEdit(**/.npmrc)", | ||
| "MultiEdit(**/.pypirc)", | ||
| "MultiEdit(**/*.pfx)", | ||
| "MultiEdit(**/credentials*)", | ||
| "MultiEdit(**/.kube/config)", | ||
| "Write(**/.env*)", |
There was a problem hiding this comment.
When Claude uses MultiEdit on protected paths such as .env, secrets/**, or *.pem, this config no longer blocks it: the MultiEdit(...) deny entries were removed, the only PreToolUse hook matcher is Bash, and the PostToolUse hook now covers only Edit/Write (I checked the current .claude/settings.json for MultiEdit). That leaves a common file-edit tool able to modify secret paths without the settings deny or the deny-dangerous policy hook, even though the hook script still knows how to evaluate multiedit; add the MultiEdit deny/hook coverage back for the same protected globs.
Useful? React with 👍 / 👎.
… new features and fixes
Note
High Risk
Relocates security-sensitive PreToolUse/PostToolUse hooks and rewrites paths every goat skill depends on; a bad root resolve or missing
.goat-flow/hooksinstall would weaken agent guardrails or break workflows.Overview
goat-flow 1.10.1 recenters agent policy on
.goat-flow/: shared docs move toskill-docs/, learning artifacts tolearning-loop/, milestone coordination toplans/, and review scratch output tologs/review/. All goat skills in.agents/,.claude/, and mirrored copies bump to 1.10.1 with path and behavior updates (e.g./goat-planwrites under.goat-flow/plans/, footgun grep targetslearning-loop/).Hooks are no longer duplicated under
.claude/hooks/; Claude, Codex, and new.agents/hooks.jsoninvoke.goat-flow/hooks/deny-dangerous.shandgruff-code-quality.shvia richer git-root resolution (submodules,CLAUDE_PROJECT_DIRfallback). Claude drops redundantMultiEditsecret denies and theMultiEditgruff hook; Codex points at the same canonical scripts and tightens filesystem denies inconfig.toml.Skill protocol tweaks include stricter
/goat-debugquick-path evidence,/goat-reviewoptional fetch (base-fetch-skipped), refuter artifacts underlogs/review/, ship verdictPENDING REFUTER/HUMAN,/goat-qacontinuing to Phase 3 on explicit test-plan intent,/goat-securitysummarizing withheld leads, and/goatrouting code exploration to Investigate mode.Reviewed by Cursor Bugbot for commit ef99d2a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation