Skip to content

feat: conflict_markers check + 'good practices' hygiene pack from CLAUDE.md (good-practices phase 1)#51

Merged
DataDave-Dev merged 2 commits into
mainfrom
feat/good-practices-hygiene
Jul 1, 2026
Merged

feat: conflict_markers check + 'good practices' hygiene pack from CLAUDE.md (good-practices phase 1)#51
DataDave-Dev merged 2 commits into
mainfrom
feat/good-practices-hygiene

Conversation

@DataDave-Dev

@DataDave-Dev DataDave-Dev commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Why

Phase 1 of "every commit follows good practices". "Good practices" isn't one
rule — it's a bundle. This phase delivers the deterministic hygiene half (the
part becwright can guarantee): drop the enforceable slices into place and let a
broad CLAUDE.md phrase pull them in.

What

  • New conflict_markers check — fails on leftover git merge markers
    (<<<<<<<, >>>>>>>, |||||||). Skips the ambiguous ======= (a legit
    Markdown underline) since the angle markers already catch every real conflict.
  • --from-claude-md expands "good practices" — a broad phrase ("follow good
    practices", "buenas prácticas", "commit hygiene", …) now expands to the
    deterministic hygiene set: hardcoded_secrets, dangerous_eval,
    debug_remnants (py), debugger/console.log (js/ts), and conflict_markers.
    Language-aware, and de-duplicated against any specific mention (a rule named
    explicitly keeps its specific trigger label).
Todos los commits deben seguir buenas prácticas.
→ From CLAUDE.md: 6 enforceable rule(s) derived —
    no-hardcoded-secrets · no-dangerous-eval · no-debug-remnants ·
    no-debugger-js · no-console-log-js · no-conflict-markers

Opinionated/narrow signals (wildcard_imports, no_token_in_logs) are not
pulled in by the broad phrase — they still require their own mention, to keep the
expansion honest.

Scope

Deterministic only — no new deps, no engine change. This is the first of three
phases for "good practices"; next: commit-msg hook (message rules) and an
advisory tier (LLM design review that warns, never blocks).

Test plan

  • conflict_markers: flags angle/pipe markers, ignores a Markdown ======= underline, main
  • "good practices" expands to the 6 hygiene rules, language-gated
  • Explicit "merge conflict markers" mention adds just that rule
  • Dedup: a specific mention wins the trigger label, no duplicate rule
  • Full suite: 210 passed; dogfooded

Summary by CodeRabbit

  • New Features
    • Added a new blocking check that detects leftover git merge conflict markers in files.
    • Expanded repo setup guidance so broad “good practices” phrases now map to a consistent set of code-hygiene rules.
  • Documentation
    • Updated the English and Spanish README and usage docs to describe the new conflict-marker check and the revised rule interpretation behavior.
  • Tests
    • Added coverage for conflict-marker detection and for rule expansion from broad guidance text.

…UDE.md

Phase 1 of enforcing 'commits follow good practices'. Adds a conflict_markers
check (leftover <<<<<<< / >>>>>>> / ||||||| markers; skips the ambiguous =======)
and teaches --from-claude-md to expand a broad 'good practices' / 'buenas
practicas' phrase into the deterministic hygiene set (secrets, eval, debug
leftovers, conflict markers), language-aware and de-duplicated against any
specific mentions. The narrow/opinionated signals are left out unless named.
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds a new conflict_markers check module that detects leftover git merge conflict markers, integrates it into CLI check descriptions and CLAUDE.md-based rule derivation (expanding broad "good practices" phrasing into a deterministic hygiene ruleset), adds corresponding tests, and updates documentation and changelog.

Changes

Conflict Markers Check

Layer / File(s) Summary
Check implementation
src/becwright/checks/conflict_markers.py
New module with PATTERN regex, find_violations(paths) scanning files line-by-line, main() reading paths from stdin and printing violations, and a __main__ entry point.
CLI integration and rule derivation
src/becwright/cli.py
Adds conflict_markers to _CHECK_DESCRIPTIONS; extends _rules_from_claude_md with new hygiene-signal and conflict-marker trigger constants, deriving no-conflict-markers and other hygiene rules from broad "good practices" phrasing or explicit conflict-marker mentions.
Tests
tests/test_metric_naming_checks.py, tests/test_init.py
Adds tests for angle-marker detection, markdown underline exclusion, main() CLI behavior, and multiple _rules_from_claude_md expansion/de-duplication scenarios.
Documentation and changelog
README.md, README.es.md, documentation/usage.md, documentation/usage.es.md, CHANGELOG.md
Documents the new conflict_markers check in checks tables, updates init --from-claude-md usage descriptions, and adds a CHANGELOG entry.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

  • DataDave-Dev/becwright#6: Both PRs touch src/becwright/cli.py's becwright init flow, with this PR extending rule-derivation logic to wire in the new checks.
  • DataDave-Dev/becwright#14: The new find_violations() logic integrates the shared is_ignored helper introduced in the retrieved PR.

Poem

A rabbit hopped through merge-torn files,
Spotting <<<<<<< in tangled piles,
"No conflicts here!" it thumped with glee,
Deriving rules from CLAUDE.md's decree,
Clean commits now, for miles and miles. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the two main changes: the new conflict_markers check and CLAUDE.md hygiene expansion.
Description check ✅ Passed The description covers the why, what, scope, and test plan, but it does not use the exact template headings or a separate checklist section.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/good-practices-hygiene

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@DataDave-Dev
DataDave-Dev merged commit 50106cc into main Jul 1, 2026
3 of 4 checks passed
@DataDave-Dev
DataDave-Dev deleted the feat/good-practices-hygiene branch July 1, 2026 18:20

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
tests/test_init.py (1)

340-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider covering the remaining trigger phrases.

Only "good practices" and "merge conflict markers" are exercised. The other _GOOD_PRACTICES_TRIGGERS variants ("buenas prácticas", "commit hygiene", "clean commit", "mejores practicas") and _CONFLICT_TRIGGERS variants ("conflict marker", "marcador de conflicto", "conflicto de merge") have no dedicated assertions, so a future typo in those literals wouldn't be caught by tests.

🤖 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/test_init.py` around lines 340 - 346, Add targeted assertions in
test_rules_from_claude_md_good_practices_keeps_specific_trigger to cover the
remaining trigger literals used by cli._rules_from_claude_md. Create test cases
that exercise each _GOOD_PRACTICES_TRIGGERS variant (“buenas prácticas”, “commit
hygiene”, “clean commit”, “mejores practicas”) and each _CONFLICT_TRIGGERS
variant (“conflict marker”, “marcador de conflicto”, “conflicto de merge”), and
assert they map to the expected rule IDs/labels so typos in those trigger
strings are caught.
🤖 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/test_init.py`:
- Around line 340-346: Add targeted assertions in
test_rules_from_claude_md_good_practices_keeps_specific_trigger to cover the
remaining trigger literals used by cli._rules_from_claude_md. Create test cases
that exercise each _GOOD_PRACTICES_TRIGGERS variant (“buenas prácticas”, “commit
hygiene”, “clean commit”, “mejores practicas”) and each _CONFLICT_TRIGGERS
variant (“conflict marker”, “marcador de conflicto”, “conflicto de merge”), and
assert they map to the expected rule IDs/labels so typos in those trigger
strings are caught.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: aa7a4c8e-50c1-4d2f-8dfc-8b6f425e7af7

📥 Commits

Reviewing files that changed from the base of the PR and between beae056 and 1ce08ca.

📒 Files selected for processing (9)
  • CHANGELOG.md
  • README.es.md
  • README.md
  • documentation/usage.es.md
  • documentation/usage.md
  • src/becwright/checks/conflict_markers.py
  • src/becwright/cli.py
  • tests/test_init.py
  • tests/test_metric_naming_checks.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant