-
Notifications
You must be signed in to change notification settings - Fork 0
chore: add pre-pr-review skill and update CLAUDE.md #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,353 @@ | ||||||||||||||||||||||
| --- | ||||||||||||||||||||||
| description: "Pre-PR review pipeline: automated checks + review agents + fixes + create PR" | ||||||||||||||||||||||
| argument-hint: "[quick] or [issue number]" | ||||||||||||||||||||||
| allowed-tools: | ||||||||||||||||||||||
| - Bash | ||||||||||||||||||||||
| - Read | ||||||||||||||||||||||
| - Edit | ||||||||||||||||||||||
| - Write | ||||||||||||||||||||||
| - Grep | ||||||||||||||||||||||
| - Glob | ||||||||||||||||||||||
| - Task | ||||||||||||||||||||||
| - AskUserQuestion | ||||||||||||||||||||||
| - mcp__github__create_pull_request | ||||||||||||||||||||||
| --- | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Pre-PR Review | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Automated pre-PR pipeline that runs checks, launches review agents, triages findings, implements fixes, and creates the PR — so the first push is already reviewed and clean. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Arguments:** "$ARGUMENTS" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| --- | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 0: Precondition Checks | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 1. **Check current branch and handle main:** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| git branch --show-current | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| - If NOT on main: proceed normally. | ||||||||||||||||||||||
| - If on main: do NOT abort. Continue to step 2 to detect changes first. If changes exist, ask the user for a branch name via AskUserQuestion (suggest one based on the changes, e.g. `feat/add-cost-tracking`). Then create and switch to that branch: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| git checkout -b <branch-name> | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
Comment on lines
+35
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolve markdownlint violations to keep doc checks clean. This file currently has lint warnings (MD031 fenced-block spacing and MD029 ordered-list prefixes). Please normalize these sections to avoid noisy/failing docs lint runs. Also applies to: 101-121, 174-177, 285-294 🧰 Tools🪛 markdownlint-cli2 (0.21.0)[warning] 33-33: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) [warning] 35-35: Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) 🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Uncommitted/staged/untracked changes carry over to the new branch automatically. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 2. **Check for changes.** Collect uncommitted, staged, untracked, and committed-but-unpushed changes: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| # Uncommitted changes (modified tracked files) | ||||||||||||||||||||||
| git diff --name-only | ||||||||||||||||||||||
| # Staged changes | ||||||||||||||||||||||
| git diff --staged --name-only | ||||||||||||||||||||||
| # Untracked files (new files not yet added to git) | ||||||||||||||||||||||
| git ls-files --others --exclude-standard | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Committed but not pushed (compared to main) | ||||||||||||||||||||||
| git diff main...HEAD --name-only | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Combine all four lists (deduplicated) as the full set of changed files. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| If no changes at all, abort with: "No changes detected. Nothing to review." | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 3. **Check if a PR already exists for this branch:** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| gh pr list --head "$(git branch --show-current)" --json number,title,url --jq '.[0]' | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| If a PR exists, ask the user via AskUserQuestion: | ||||||||||||||||||||||
| - Option A: "Run review + push to existing PR #N" | ||||||||||||||||||||||
| - Option B: "Redirect to /aurelio-review-pr (for external feedback triage)" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| If user picks B, stop and tell them to run `/aurelio-review-pr`. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 4. **Check if branch is behind main:** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| git rev-list --count HEAD..main | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| If behind, warn: "Branch is N commits behind main. Consider rebasing before review." | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 5. **Collect changed files into categories** from the combined list of all changes (uncommitted + staged + untracked + committed-but-unpushed): | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| - `src_py`: `.py` files in `src/` | ||||||||||||||||||||||
| - `test_py`: `.py` files in `tests/` | ||||||||||||||||||||||
| - `config`: `.toml`, `.yaml`, `.json`, `.cfg` files | ||||||||||||||||||||||
| - `docs`: `.md` files | ||||||||||||||||||||||
| - `other`: everything else | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 6. **Large diff warning.** If 50+ files changed, warn about token cost and ask user whether to proceed with all agents or select a subset. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 1: Quick Mode Detection | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Determine if agent review can be skipped: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| - If `$ARGUMENTS` contains `quick` -> skip agents, go to Phase 2 then Phase 8, then Phase 10 and Phase 11 | ||||||||||||||||||||||
| - **Auto-detect**: If ALL changed files are non-substantive (only `.md` docs, config formatting, typo-level edits with no logic changes), skip agents automatically | ||||||||||||||||||||||
| - Auto-skip examples: all changes are `.md` files; only `pyproject.toml` version bump; only `.yaml`/`.json` config with no Python changes | ||||||||||||||||||||||
| - Do NOT auto-skip: any `.py` file changed; config changes that affect runtime behavior; new dependencies added | ||||||||||||||||||||||
| - If auto-skipping, inform user: "Skipping agent review (no substantive code changes detected). Running automated checks only." | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 2: Automated Checks (always run) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Scoping:** If no `.py` files changed (only `.md`, `.yaml`, `.toml`, `.json`, etc.), skip steps 1-5 entirely — ruff, mypy, and pytest only operate on Python files and running them is unnecessary for docs/config-only changes. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Run these sequentially, fixing as we go: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 1. **Lint + auto-fix:** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| uv run ruff check src/ tests/ --fix | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 2. **Format:** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| uv run ruff format src/ tests/ | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 3. If steps 1-2 changed any files, stage them: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| git add -A | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
Comment on lines
+117
to
+121
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Clarify conditional to account for scoping rule. Line 101's scoping rule may skip steps 1-2 entirely when no 📝 Suggested clarification-3. If steps 1-2 changed any files, stage them:
+3. If steps 1-2 were run AND changed any files, stage them:📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 4. **Type-check:** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| uv run mypy src/ tests/ | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 5. **Test + coverage:** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| uv run pytest tests/ -n auto --cov=ai_company --cov-fail-under=80 | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Failure handling:** | ||||||||||||||||||||||
| - If mypy fails: fix the type errors, re-run mypy | ||||||||||||||||||||||
| - If pytest fails: fix failing tests, re-run pytest | ||||||||||||||||||||||
| - If something can't be auto-fixed: present the error to the user via AskUserQuestion, ask how to proceed (fix now / skip check / abort) | ||||||||||||||||||||||
| - After fixing, stage changes with `git add -A` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **If in quick mode:** After automated checks pass, skip directly to Phase 8 (Post-Fix Verification), then continue to Phase 10 (Commit + Push + Create PR) and Phase 11 (Summary). | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant automated checks in quick mode In quick mode the flow is: Phase 2 (automated checks) → Phase 8 (post-fix verification, which runs the same four commands) → Phase 10. Because agents are skipped in quick mode there are no fixes applied between Phase 2 and Phase 8, so Phase 8 is a pure duplicate run of exactly the same ruff/mypy/pytest commands that already passed. This doubles CI time for the most common "trivial changes" use-case. Consider short-circuiting Phase 8 in quick mode:
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: .claude/skills/pre-pr-review/SKILL.md
Line: 141
Comment:
**Redundant automated checks in quick mode**
In quick mode the flow is: Phase 2 (automated checks) → Phase 8 (post-fix verification, which runs the same four commands) → Phase 10. Because agents are skipped in quick mode there are no fixes applied between Phase 2 and Phase 8, so Phase 8 is a pure duplicate run of exactly the same ruff/mypy/pytest commands that already passed. This doubles CI time for the most common "trivial changes" use-case.
Consider short-circuiting Phase 8 in quick mode:
```suggestion
**If in quick mode:** After automated checks pass, skip directly to Phase 10 (Commit + Push + Create PR) and Phase 11 (Summary). Phase 8 is unnecessary because no agent-driven fixes were applied.
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+141
to
+142
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider whether Phase 8 is needed in quick mode. In quick mode, Phase 8 re-runs the same automated checks that already passed in Phase 2. Since no agent fixes are applied (agents were skipped), this verification step may be redundant. Consider documenting the rationale (e.g., defensive verification of auto-fixes) or skipping Phase 8 when in quick mode with no code changes applied. 🤖 Prompt for AI Agents |
||||||||||||||||||||||
| ## Phase 3: Determine Agent Roster | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Based on changed files and diff content, select which agents to launch. Stage all changes first, then get a unified diff against main: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| # Stage everything so the diff includes uncommitted + untracked changes | ||||||||||||||||||||||
| git add -A | ||||||||||||||||||||||
| # Unified diff: all changes (committed + staged) vs main | ||||||||||||||||||||||
| git diff --staged main | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
Comment on lines
+147
to
+152
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect diff base when main has advanced The command Phase 0 step 2 correctly uses the three-dot syntax (
Suggested change
This ensures review agents see exactly what the PR will introduce — nothing more, nothing less. Prompt To Fix With AIThis is a comment left during a code review.
Path: .claude/skills/pre-pr-review/SKILL.md
Line: 147-152
Comment:
**Incorrect diff base when main has advanced**
The command `git diff --staged main` compares the index against the *tip* of `main`, not against the common ancestor. When `main` has progressed since the feature branch was created (a scenario the skill explicitly warns about in Phase 0 step 4), every commit that landed on `main` after the branch point will appear as **deletions** in the diff fed to the review agents. This creates false signals — agents will believe the PR is removing code that was legitimately added to `main`.
Phase 0 step 2 correctly uses the three-dot syntax (`git diff main...HEAD --name-only`), which resolves to the merge-base. Phase 3 should do the same:
```suggestion
# Stage everything so the diff includes uncommitted + untracked changes
git add -A
# Unified diff: all changes (committed + staged) vs merge-base with main
git diff --staged $(git merge-base HEAD main)
```
This ensures review agents see exactly what the PR will introduce — nothing more, nothing less.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| This captures committed-but-unpushed changes AND any uncommitted/untracked work in a single diff. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| | Agent | Condition | subagent_type | | ||||||||||||||||||||||
| |---|---|---| | ||||||||||||||||||||||
| | **code-reviewer** | Any `src_py` or `test_py` | `pr-review-toolkit:code-reviewer` | | ||||||||||||||||||||||
| | **python-reviewer** | Any `src_py` or `test_py` | `everything-claude-code:python-reviewer` | | ||||||||||||||||||||||
| | **pr-test-analyzer** | `test_py` changed, OR `src_py` changed with no corresponding test changes | `pr-review-toolkit:pr-test-analyzer` | | ||||||||||||||||||||||
| | **silent-failure-hunter** | Diff contains `try`, `except`, `raise`, error handling patterns | `pr-review-toolkit:silent-failure-hunter` | | ||||||||||||||||||||||
| | **comment-analyzer** | Diff contains docstring changes (`"""`) or significant comment changes | `pr-review-toolkit:comment-analyzer` | | ||||||||||||||||||||||
| | **type-design-analyzer** | Diff contains `class ` definitions, `BaseModel`, `TypedDict`, type aliases | `pr-review-toolkit:type-design-analyzer` | | ||||||||||||||||||||||
| | **logging-audit** | Any `src_py` changed | `pr-review-toolkit:code-reviewer` (custom prompt below) | | ||||||||||||||||||||||
| | **resilience-audit** | Files in `src/ai_company/providers/` changed | `pr-review-toolkit:code-reviewer` (custom prompt below) | | ||||||||||||||||||||||
| | **security-reviewer** | Files in `src/ai_company/api/`, `src/ai_company/security/`, `src/ai_company/tools/`, `src/ai_company/config/` changed, OR diff contains `subprocess`, `eval`, `exec`, `pickle`, `yaml.load`, auth/credential patterns | `everything-claude-code:security-reviewer` | | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ### Logging-audit custom prompt | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| The logging-audit agent must check for these violations (from CLAUDE.md `## Logging`): | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Infrastructure violations (hard rules):** | ||||||||||||||||||||||
| 1. `import logging` + `logging.getLogger` in application source (CRITICAL) | ||||||||||||||||||||||
| 2. `print()` calls in application source (CRITICAL) | ||||||||||||||||||||||
| 3. Logger variable named `_logger` instead of `logger` (CRITICAL) | ||||||||||||||||||||||
| 4. Log calls using positional `%s` formatting instead of structured kwargs (CRITICAL) | ||||||||||||||||||||||
| 5. Log call event argument is a bare string literal, not an event constant (MAJOR) | ||||||||||||||||||||||
| 6. Business logic file missing a `logger = get_logger(__name__)` declaration (MAJOR) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Logging coverage suggestions (soft rules — mark as SUGGESTION, must be validated by user in triage):** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| For every function touched by the changes, analyze its logic and suggest missing logging where appropriate: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 1. Error/except paths that don't `logger.warning()` or `logger.error()` with context before raising or returning (SUGGESTION) | ||||||||||||||||||||||
| 2. State transitions (status changes, lifecycle events, mode switches) that don't `logger.info()` (SUGGESTION) | ||||||||||||||||||||||
| 3. Object creation, entry/exit of key functions, or important branching decisions that don't `logger.debug()` (SUGGESTION) | ||||||||||||||||||||||
| 4. Any other code path that would benefit from logging for debuggability or operational visibility (SUGGESTION) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Exclusions — do NOT flag these for coverage suggestions:** | ||||||||||||||||||||||
| - Pure data models, Pydantic `BaseModel` subclasses, enums, TypedDict definitions | ||||||||||||||||||||||
| - Re-export `__init__.py` files | ||||||||||||||||||||||
| - Simple property accessors, trivial getters/setters | ||||||||||||||||||||||
| - One-liner functions with no branching or side effects | ||||||||||||||||||||||
| - Test files | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ### Resilience-audit custom prompt | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| The resilience-audit agent must check for these violations (from CLAUDE.md `## Resilience`): | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Hard rules:** | ||||||||||||||||||||||
| 1. Driver subclass implements its own retry/backoff logic instead of relying on base class (CRITICAL) | ||||||||||||||||||||||
| 2. Calling code wraps provider calls in manual retry loops (CRITICAL) | ||||||||||||||||||||||
| 3. New `BaseCompletionProvider` subclass doesn't pass `retry_handler`/`rate_limiter` to `super().__init__()` (MAJOR) | ||||||||||||||||||||||
| 4. Retryable error type created without `is_retryable = True` (MAJOR) | ||||||||||||||||||||||
| 5. `asyncio.sleep` used for retry delays outside of `RetryHandler` (MAJOR) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Soft rules (SUGGESTION):** | ||||||||||||||||||||||
| 6. New provider error type missing `is_retryable` classification (SUGGESTION) | ||||||||||||||||||||||
| 7. Provider call site that catches `ProviderError` but doesn't account for `RetryExhaustedError` (SUGGESTION) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 4: Launch Review Agents (parallel) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Launch ALL selected agents **in parallel** using the Task tool. **Do NOT use `run_in_background`** — launch them as regular parallel Task calls so results arrive together. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Each agent receives: | ||||||||||||||||||||||
| - List of changed files | ||||||||||||||||||||||
| - The diff content for those files | ||||||||||||||||||||||
| - Relevant CLAUDE.md sections (Logging, Resilience, Code Conventions, Testing) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| If an agent times out or fails, proceed with findings from the agents that succeeded. Report the failed agent in the summary. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Collect all findings with their severity/confidence scores. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 5: Consolidate and Triage | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Build a single consolidated table of ALL findings from all agents. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| For each item, determine: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| - **Source**: Which agent found it | ||||||||||||||||||||||
| - **Severity**: Critical / Major / Medium / Minor | ||||||||||||||||||||||
| - Map confidence 91-100 to Critical, 80-90 to Major, 60-79 to Medium, below 60 to Minor | ||||||||||||||||||||||
| - **File:Line**: Where the issue is | ||||||||||||||||||||||
| - **Issue**: One-line summary of the problem | ||||||||||||||||||||||
| - **Valid?**: Your assessment — is this correct advice for this codebase? Check against CLAUDE.md rules and actual code | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Deduplication:** If multiple agents flag the same issue at the same location, merge into one item and note all sources. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Conflict detection:** If two agents contradict each other, flag both positions. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Sort by severity (Critical first, Minor last). | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 6: Present for User Approval | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Show the consolidated table with: | ||||||||||||||||||||||
| - Total count of items | ||||||||||||||||||||||
| - Count by source agent | ||||||||||||||||||||||
| - Any items recommended to skip (with reasoning) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Ask the user via AskUserQuestion: | ||||||||||||||||||||||
| - "Implement all" (Recommended) | ||||||||||||||||||||||
| - "Let me review the list first" | ||||||||||||||||||||||
| - "Skip some items" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| If the user wants to skip items, ask which ones by number. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 7: Implement Fixes | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| For each approved item, grouped by file (minimize context switches): | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 1. Read the file | ||||||||||||||||||||||
| 2. Apply the fix | ||||||||||||||||||||||
| 3. Move to the next fix in the same file before switching files | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| After all fixes: | ||||||||||||||||||||||
| - If a fix requires test changes, change the tests too | ||||||||||||||||||||||
| - If a fix introduces new code paths, add test coverage | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 8: Post-Fix Verification | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Run the full automated checks again: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 1. `uv run ruff check src/ tests/` | ||||||||||||||||||||||
| 2. `uv run ruff format src/ tests/` | ||||||||||||||||||||||
| 3. `uv run mypy src/ tests/` | ||||||||||||||||||||||
| 4. `uv run pytest tests/ -n auto --cov=ai_company --cov-fail-under=80` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| If anything fails, fix and re-run. Stage all changes after passing: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| git add -A | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 9: Polish Pass (code-simplifier) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| **Skip this phase if:** quick mode was used, OR no agent findings were implemented (nothing changed beyond Phase 2 auto-fixes). | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 1. Launch `pr-review-toolkit:code-simplifier` on all modified files | ||||||||||||||||||||||
| 2. If it suggests improvements, apply them | ||||||||||||||||||||||
| 3. Re-run `uv run ruff check src/ tests/` + `uv run ruff format src/ tests/` to ensure polish didn't break formatting | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ## Phase 10: Commit + Push + Create PR | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 1. **Stage all files:** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| git add -A | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 2. **Commit** with a descriptive message: | ||||||||||||||||||||||
| - Type based on changes (feat/fix/refactor/docs/etc.) | ||||||||||||||||||||||
| - If agents ran, add body: "Pre-reviewed by N agents, M findings addressed" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 3. **Push** with `-u` flag: | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||
| git push -u origin "$(git branch --show-current)" | ||||||||||||||||||||||
| ``` | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| 4. **If PR already exists** (detected in Phase 0): push only, do NOT create a new PR. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+310
to
+311
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Clarify step 4 wording to avoid confusion. Step 4 says "push only, do NOT create a new PR," but step 3 already pushed the branch. The intent is to skip step 5 (PR creation), not to perform an additional push action. Consider rewording: "If PR already exists (detected in Phase 0): skip PR creation. The push in step 3 updates the existing PR." 📝 Suggested clarification-4. **If PR already exists** (detected in Phase 0): push only, do NOT create a new PR.
+4. **If PR already exists** (detected in Phase 0): skip PR creation (the push in step 3 updates the existing PR).🤖 Prompt for AI Agents |
||||||||||||||||||||||
| 5. **If no PR exists**, create one using the **`mcp__github__create_pull_request`** tool (NOT `gh pr create` — that is blocked by hookify): | ||||||||||||||||||||||
|
||||||||||||||||||||||
| 5. **If no PR exists**, create one using the **`mcp__github__create_pull_request`** tool (NOT `gh pr create` — that is blocked by hookify): | |
| 5. **If no PR exists**, create one via the hookify-approved `/pre-pr-review` path using the **`mcp__github__create_pull_request`** tool. Do **not** run `gh pr create` manually — hookify blocks manual `gh` usage, but allows PRs created through this skill’s `/pre-pr-review` workflow: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Reposition failure handling closer to the failing step.
Step 6 discusses commit failure handling (from step 2) after describing the push and PR creation steps. For clarity, either move this guidance immediately after step 2, or add "If step 2 failed..." to make the reference explicit.
📝 Suggested repositioning
Move step 6 content to immediately follow step 2:
2. **Commit** with a descriptive message:
- Type based on changes (feat/fix/refactor/docs/etc.)
- If agents ran, add body: "Pre-reviewed by N agents, M findings addressed"
**If commit fails due to pre-commit hooks:** fix the issue and create a NEW commit. **NEVER use `--no-verify`.**
3. **Push** with `-u` flag:🧰 Tools
🪛 LanguageTool
[style] ~328-~328: Consider using a different verb for a more formal wording.
Context: ...commit fails due to pre-commit hooks:** fix the issue and create a NEW commit. **NE...
(FIX_RESOLVE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/pre-pr-review/SKILL.md around lines 328 - 329, The guidance
about handling commit failures (the sentence starting "If commit fails due to
pre-commit hooks:... NEVER use `--no-verify`.") is placed far from the commit
step and should be moved or clarified; either relocate that sentence so it
immediately follows step 2's "Commit" instructions, or modify it in place to
explicitly reference step 2 (e.g., "If step 2 fails..."). Update the SKILL.md
content to reference the unique step labels "Step 2" and "Step 6" accordingly so
readers clearly see the failure-handling guidance next to the commit
instructions.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -111,6 +111,14 @@ src/ai_company/ | |||||
| - **Branches**: `<type>/<slug>` from main | ||||||
| - **Pre-commit hooks**: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks | ||||||
|
|
||||||
| ## Pre-PR Review (MANDATORY) | ||||||
|
|
||||||
| - **NEVER create a PR directly** — `gh pr create` is blocked by hookify | ||||||
|
||||||
| - **NEVER create a PR directly** — `gh pr create` is blocked by hookify | |
| - **NEVER create a PR directly** — `gh pr create` is blocked by the repository's Git hook configuration |
Copilot
AI
Mar 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/commit-push-pr is described as blocked, but there’s no corresponding skill/command definition in .claude/skills/ (and no other repo references). Either add the command, or remove/replace this line so CLAUDE.md doesn’t document a nonexistent workflow.
| - The `/commit-push-pr` command is effectively blocked (it calls `gh pr create` internally) | |
| - Any direct PR-creation commands that call `gh pr create` internally are effectively blocked by hooks |
Uh oh!
There was an error while loading. Please reload this page.