Skip to content

Add pre-merge-check skill and improve PR review workflow#98

Merged
igerber merged 2 commits into
mainfrom
feature/improve-pr-workflow
Jan 22, 2026
Merged

Add pre-merge-check skill and improve PR review workflow#98
igerber merged 2 commits into
mainfrom
feature/improve-pr-workflow

Conversation

@igerber
Copy link
Copy Markdown
Owner

@igerber igerber commented Jan 22, 2026

Summary

  • Create /pre-merge-check skill for automated checks before submitting PRs
    • Pattern checks for NaN handling issues (if se > 0 else 0.0 anti-pattern)
    • Test coverage verification (methodology file → test file mapping)
    • Context-specific checklists based on changed files
  • Update PR review prompt with edge case checklist learned from PR Add base_period parameter to CallawaySantAnna for pre-treatment effects #97 analysis
    • Empty result set handling
    • NaN/Inf propagation through all inference fields
    • Parameter interaction verification with aggregation/bootstrap
    • Control group logic validation
    • Pattern consistency checks for bug fixes
  • Add Task Implementation Workflow section to CLAUDE.md
    • 4-phase workflow: Planning, Implementation, Pre-Merge Review, Submit
    • Quick reference grep commands for common pattern checks

Methodology references (required if estimator / math changes)

  • Method name(s): N/A - no methodology changes
  • Paper / source link(s): N/A
  • Any intentional deviations from the source (and why): None

Validation

  • Tests added/updated: No test changes (workflow/documentation only)
  • Backtest / simulation / notebook evidence (if applicable): N/A

Security / privacy

  • Confirm no secrets/PII in this PR: Yes

Generated with Claude Code

- Create /pre-merge-check skill for automated checks before submitting PRs
  - Pattern checks for NaN handling issues
  - Test coverage verification
  - Context-specific checklists based on changed files
- Update PR review prompt with edge case checklist from PR #97 analysis
  - Empty result set handling
  - NaN/Inf propagation checks
  - Parameter interaction verification
  - Control group logic validation
  - Pattern consistency checks
- Add Task Implementation Workflow section to CLAUDE.md
  - 4-phase workflow: Planning, Implementation, Pre-Merge Review, Submit
  - Quick reference commands for common pattern checks

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Overall Assessment

  • ⚠️ Needs changes

Executive Summary

  • P1: pre-merge change detection can miss committed PR changes, which can skip critical checks.
  • P2: NaN division pattern check likely never matches due to \b usage without PCRE.
  • No estimator/math/variance behavior changes; updates are workflow/docs only.

Methodology

  • No findings; no methodology or estimator code changes in this diff.

Code Quality

  • Severity: P1 | Impact: pre-merge check may ignore committed changes and return an empty file list on clean branches, skipping pattern/test checks | Fix: diff against the PR base (e.g., git diff --name-only <base>...HEAD or git diff --name-only $(git merge-base HEAD <base>) HEAD) and keep git ls-files --others --exclude-standard for untracked | Location: .claude/commands/pre-merge-check.md:L14-L20
  • Severity: P2 | Impact: grep -n "/ se\b" uses BRE where \b is a backspace, so the heuristic likely never matches and can miss issues | Fix: switch to grep -n -P "/\s*se\b" or grep -n -E "/[[:space:]]*se\>" | Location: .claude/commands/pre-merge-check.md:L38-L39

Performance

  • No findings.

Maintainability

  • No findings.

Tech Debt

  • No findings.

Security

  • No findings.

Documentation/Tests

  • No findings; doc-only changes are consistent with scope.

Change \b (interpreted as backspace in BRE) to \> (word-end boundary)
with -E flag for Extended Regular Expressions. Also use [[:space:]]*
for portable whitespace matching.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@igerber igerber merged commit fd7f10b into main Jan 22, 2026
@igerber igerber deleted the feature/improve-pr-workflow branch January 22, 2026 15:04
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