Skip to content

docs: add one-PR-one-concern rule to git conventions#281

Merged
carlos-alm merged 1 commit intomainfrom
refactor/fix-280-conflicts
Mar 3, 2026
Merged

docs: add one-PR-one-concern rule to git conventions#281
carlos-alm merged 1 commit intomainfrom
refactor/fix-280-conflicts

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Add a scope creep prevention rule to CLAUDE.md Git Conventions: each PR should address a single feature, fix, or refactor

Test plan

  • Verify CLAUDE.md renders correctly

@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @carlos-alm's task —— View job


Reviewing PR #281

Todo List:

  • Phase 1: Root cause analysis - understand what problem this PR solves
  • Phase 2: Check for configuration changes and rule relaxations
  • Phase 3: Verify test integrity and look for red flags
  • Phase 4: Review code quality and functionality
  • Provide final assessment with rating

🚨 CRITICAL VIOLATION: Scope Creep Anti-Pattern

The Core Problem

This PR perfectly demonstrates the scope creep problem it claims to solve. The branch is named refactor/fix-280-conflicts and includes 33 separate commits spanning multiple weeks of development.

What Actually Happened

This is exactly what good engineering practices prohibit. Each commit here should have been a separate PR.

Phase 2: Configuration Changes ✅

  • Branch naming: Added benchmark/ prefix (reasonable)
  • CLA workflow: Standard legal requirement
  • Pre-commit hooks: Appropriate dev tooling
  • No dangerous rule relaxations

Phase 3: Test Integrity ⚠️

  • 5 CLI commands removed: Consolidated into query command
  • Tests updated appropriately for consolidation
  • Some benchmarks removed (cleanup)
  • No weakened assertions found

Phase 4: Code Quality

The individual features appear well-implemented:

  • CFG construction follows academic standards
  • Export formats support multiple graph databases
  • Batch processing has proper error handling
  • Native Rust extractors maintain consistency

However, mixing all these unrelated features violates fundamental PR hygiene.


Root Cause Analysis

Problem Being Solved: Adding a development guideline about PR scope
Why This Approach: Author consolidated weeks of parallel development rather than splitting into focused PRs
Risk Assessment: HIGH - This approach makes code review impossible and increases regression risk

Critical Concerns

  1. 33 unrelated commits in a single PR make thorough review impossible
  2. Major CLI changes mixed with documentation updates
  3. New core systems (CFG, dataflow) deserve dedicated review focus
  4. Violates the very rule it introduces - meta-level irony

Final Recommendation

  • Rating: ⭐⭐☆☆☆ (2/5)
  • Action: REQUEST CHANGES
  • Reasoning: This PR is the perfect example of what NOT to do. While individual features may be valuable, bundling 33 commits across multiple systems makes proper review impossible and violates the principle it claims to establish.

Recommended Action: Split into separate PRs:

  1. CLI consolidation PR
  2. CFG feature PR
  3. Export formats PR
  4. CLA workflow PR
  5. Documentation PR with the one-concern rule

@greptile-apps
Copy link

greptile-apps bot commented Mar 3, 2026

Greptile Summary

Added a scope creep prevention rule to Git Conventions: each PR should address a single feature, fix, or refactor.

  • Addresses feedback from previous review — the branch was successfully rebased to contain only this documentation change
  • Clear, actionable guidance that prevents mixing unrelated changes in PRs
  • Well-placed in the Git Conventions section with consistent formatting

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • Single-line documentation change with clear, valuable content that follows the project's conventions and addresses previous review feedback
  • No files require special attention

Important Files Changed

Filename Overview
CLAUDE.md Added one-PR-one-concern rule to Git Conventions section - clear, well-written, and appropriately placed

Last reviewed commit: 0ba8aab

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@@ -170,6 +170,7 @@ Multiple Claude Code instances run concurrently in this repo. **Every session mu

- Never add AI co-authorship lines (`Co-Authored-By` or similar) to commit messages.
- Never add "Built with Claude Code", "Generated with Claude Code", or any variation referencing Claude Code or Anthropic to commit messages, PR descriptions, code comments, or any other output.
- **One PR = one concern.** Each pull request should address a single feature, fix, or refactor. Do not pile unrelated changes into an existing PR — open a new branch and PR instead. If scope grows during implementation, split the work into separate PRs before pushing.
Copy link

Choose a reason for hiding this comment

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

Ironic: this PR violates the very rule it introduces. The PR description claims to only add this documentation line, but actually includes:

  • Removing 5 CLI commands (explain, hotspots, manifesto, batch-query)
  • Adding a new path command
  • Fixing a reexports query bug in queries.js
  • Adding 157 lines of new test code in exports.test.js
  • Reordering imports in mcp.js

This should have been split into separate PRs:

  1. Documentation rule only
  2. CLI consolidation refactor
  3. Bug fix for reexports
  4. New exports test coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed — the branch was rebased onto main and now only contains the single CLAUDE.md commit (0ba8aab). The extra changes you flagged were from inherited branch history before the rebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed — the branch was rebased onto main and now only contains the single CLAUDE.md commit (). The extra changes you flagged were from inherited branch history before the rebase.

@carlos-alm carlos-alm force-pushed the refactor/fix-280-conflicts branch from 7c35494 to 0ba8aab Compare March 3, 2026 06:05
@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @carlos-alm's task —— View job


Reviewing PR #281

Todo List:

  • Phase 1: Root cause analysis - understand what problem this PR solves
  • Phase 2: Check for configuration changes and rule relaxations
  • Phase 3: Verify test integrity and look for red flags
  • Phase 4: Review code quality and functionality
  • Provide final assessment with rating

🎯 SIGNIFICANT IMPROVEMENT: Scope Corrected

Root Cause Analysis

Problem Being Solved: Adding a development guideline to prevent scope creep in PRs
Why This Approach: Direct documentation update to establish clear expectations
Risk Assessment: LOW - Simple documentation addition with clear intent

Key Observation

The automated reviews from both Greptile and previous Claude sessions were commenting on an earlier version of this PR that included multiple unrelated changes (CLI refactoring, bug fixes, new tests). However, the current state of the PR has been properly cleaned up to contain only:

  • Single change: Added one line to CLAUDE.md:173 establishing the "One PR = one concern" rule
  • No code changes: No CLI modifications, no test changes, no bug fixes
  • Focused scope: Exactly what the PR title promises

Phase 2: Configuration Changes ✅

  • No configuration files modified
  • No linting rules changed
  • No build settings altered
  • No rule relaxations

Phase 3: Test Integrity ✅

  • No test files modified
  • No test expectations changed
  • No assertions weakened
  • Clean documentation-only change

Phase 4: Code Quality ✅

  • Well-written rule: Clear, actionable guidance for contributors
  • Proper placement: Located in Git Conventions section where it belongs
  • Consistent formatting: Matches existing documentation style
  • Addresses real problem: Scope creep is a genuine issue in collaborative development

Content Analysis

The added rule at CLAUDE.md:173 states:

One PR = one concern. Each pull request should address a single feature, fix, or refactor. Do not pile unrelated changes into an existing PR — open a new branch and PR instead. If scope grows during implementation, split the work into separate PRs before pushing.

This is excellent guidance that will help prevent exactly the type of scope creep the previous reviewers identified.


Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: This PR demonstrates good engineering practices by addressing previous feedback and cleaning up scope creep. The author listened to the automated reviews and correctly reduced this to a single, focused documentation change. The rule itself is valuable and well-written. Minor deduction only for the initial scope creep that required cleanup.

Meta-observation: This PR is now a perfect example of what the rule it introduces advocates for - a focused, single-concern change that's easy to review and understand.

@carlos-alm
Copy link
Contributor Author

@greptileai

@carlos-alm carlos-alm merged commit f01d077 into main Mar 3, 2026
24 checks passed
@carlos-alm carlos-alm deleted the refactor/fix-280-conflicts branch March 3, 2026 06:24
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant