From 7234608827c8cca2630bf799045babbeb105ec5a Mon Sep 17 00:00:00 2001 From: Chris Nussbaum Date: Sat, 1 Aug 2026 22:22:54 -0500 Subject: [PATCH] Add a repo-local pr-workflow skill; correct two stale command docs The `pr-workflow` skill that surfaces in this working directory belongs to the sibling dev_esphome/ checkout and describes contributing to upstream ESPHome core -- base on upstream/dev, fill in their PR template, prefix the title with [component], target esphome/esphome. Invoking it here produces the wrong workflow every time. commit_pr.md warned about this in prose, but nothing owned the name for this repo, so the name still resolved upstream. Add .claude/skills/pr-workflow/SKILL.md covering this repo's conventions: branch prefixes, base master, title and body style, the seven required checks and how long they take, and the squash-merge details. It defers the commit steps to /commit and the full cycle to /commit_pr rather than restating them. Also fixes two things in the existing command docs that were no longer true: - commit_pr.md said master has no required status checks and that a merge should go through immediately. Master now requires all seven CI checks with enforce_admins on, so a merge waits on CI. - commit.md said to run `python -m pre_commit` because the pre-commit CLI is not on PATH. The reverse is now true: pre-commit is installed via pipx and shimmed at ~/.local/bin/pre-commit, and the python on PATH has no pre_commit module, so the documented command fails outright. Co-Authored-By: Claude Opus 5 --- .claude/commands/commit.md | 2 +- .claude/commands/commit_pr.md | 4 +- .claude/skills/pr-workflow/SKILL.md | 123 ++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 .claude/skills/pr-workflow/SKILL.md diff --git a/.claude/commands/commit.md b/.claude/commands/commit.md index d2edc52..d832d83 100644 --- a/.claude/commands/commit.md +++ b/.claude/commands/commit.md @@ -5,7 +5,7 @@ description: Run pre-commit hooks to a clean state, commit the staged/modified c Get the working tree to a committable state, commit it, and push it. 1. Check the current branch (`git branch --show-current`). If it's `master`, create and switch to a new feature branch before doing anything else — don't commit on `master` (the `no-commit-to-branch` hook blocks it anyway). Name the branch from what's actually changing, following this repo's convention: `f/` for a feature/change, `b/` for a bug fix, `chore/` for maintenance/tooling (see existing branches for examples). Base it off the pending changes (`git status`/`git diff`), not off the eventual commit message wording verbatim. -2. Run `python -m pre_commit run --all-files` (the `pre-commit` CLI isn't on `PATH` in this environment, so use the Python module form, not the bare `pre-commit` command). If a hook modifies files (e.g. `ruff-format`), `git add` those files and re-run until every hook reports `Passed`. If a hook fails for a reason it can't auto-fix (e.g. `flake8`), stop and report the failure instead of committing — don't work around it with `--no-verify`. +2. Run `pre-commit run --all-files`. pre-commit is installed via pipx and shimmed at `~/.local/bin/pre-commit`, so the bare command works; `python -m pre_commit` does **not** — the `python` on `PATH` has no `pre_commit` module. If a hook modifies files (e.g. `ruff-format`), `git add` those files and re-run until every hook reports `Passed`. If a hook fails for a reason it can't auto-fix (e.g. `flake8`), stop and report the failure instead of committing — don't work around it with `--no-verify`. 3. Once hooks are clean, follow the normal commit workflow: review `git status` and `git diff --cached` (and unstaged changes, staging anything that belongs in this commit) for what's about to be committed, check `git log --oneline -10` for this repo's message style, draft a concise commit message focused on *why*, then create the commit with the `Co-Authored-By: Claude ` trailer as usual, and confirm with `git status` afterward. 4. Push the branch: `git push`, or `git push -u origin ` if it has no upstream yet (a brand-new feature branch created in step 1 won't). `push.followTags` is already configured locally, so any version tag the `tag-esphome-version` post-commit hook created will be pushed along with it automatically — no separate tag push needed. If the push is rejected (e.g. remote has diverged), stop and report it rather than force-pushing. 5. Never skip hooks. diff --git a/.claude/commands/commit_pr.md b/.claude/commands/commit_pr.md index 4858fc9..7e32154 100644 --- a/.claude/commands/commit_pr.md +++ b/.claude/commands/commit_pr.md @@ -13,13 +13,13 @@ Every step below operates on **this repo only** (`nuttytree/ESPHome-Devices`, de gh pr create --base master --title "" --body-file - ``` - > **Do not use the `pr-workflow` skill here, and never target `esphome/esphome`.** That skill lives in the sibling `dev_esphome/` checkout (upstream ESPHome core, wired in only for IntelliSense — see CLAUDE.md) and describes how to contribute *to upstream*: it says to base on `upstream/dev`, fill in ESPHome's `.github/PULL_REQUEST_TEMPLATE.md`, prefix the title with `[component]`, and run `gh pr create --repo esphome/esphome --base dev`. All of that is wrong for this repo. It surfaces as an available skill purely because `dev_esphome/` is an additional working directory. Ignore it; the conventions below are this repo's. + > **There are two skills named `pr-workflow`; only one is for this repo, and never target `esphome/esphome`.** This repo's own is `.claude/skills/pr-workflow/SKILL.md` and matches the conventions below — that one is fine to consult. The other lives in the sibling `dev_esphome/` checkout (upstream ESPHome core, wired in only for IntelliSense — see CLAUDE.md) and describes how to contribute *to upstream*: it says to base on `upstream/dev`, fill in ESPHome's `.github/PULL_REQUEST_TEMPLATE.md`, prefix the title with `[component]`, and run `gh pr create --repo esphome/esphome --base dev`. All of that is wrong here. It surfaces as an available skill purely because `dev_esphome/` is an additional working directory. If a `pr-workflow` skill tells you to base on `upstream/dev` or pass `--repo esphome/esphome`, you have the wrong one — stop and use the conventions below. This repo has no PR template and uses no labels. Match the style of recent merged PRs (`gh pr list --state merged --limit 5`, then `gh pr view <n> --json body`): - **Title**: sentence-case description of the change, no bracket/component prefix (e.g. "Fix pump anomaly detection: startup window, latching, drift, variance"). - **Body**: a prose statement of the problem and why it mattered, then a `Changes:` bullet list of what was actually done. Reference related PRs/issues with `#<n>` where relevant, and call out anything that silently changes behaviour on upgrade (e.g. persisted state being discarded). - Note how the change was verified (`esphome config <device>.yaml`, or an `esphome compile` if the C++ changed). -3. Merge the PR: this repo only allows squash merges (`allow_squash_merge: true`; `allow_merge_commit`/`allow_rebase_merge` are both `false`), so merge with `gh pr merge <number> --squash`. `master` has no required status checks configured, so the merge should go through immediately — if `gh` reports it can't merge (conflicts, an unexpected required check, etc.), stop and report rather than forcing it. Don't pass `--delete-branch`: the repo already has `delete_branch_on_merge` enabled, so GitHub deletes the remote branch itself on merge. +3. Wait for CI, then merge. `master` requires all seven checks to pass before a PR can merge — `pre-commit`, `esphome config`, and `compile pool` / `compile water-heater` / `compile hvac` / `compile pool-lights` / `compile master-bed` — and `enforce_admins` is on, so this applies to you too. Poll with `gh pr checks <number>`; budget ~5 minutes, since the five compiles run in parallel (esp32 ~4 min, esp8266 ~2 min). This repo only allows squash merges (`allow_squash_merge: true`; `allow_merge_commit`/`allow_rebase_merge` are both `false`), so merge with `gh pr merge <number> --squash`. If `gh` reports it can't merge (conflicts, a failing or non-reporting required check), stop and report rather than forcing it. Don't pass `--delete-branch`: the repo already has `delete_branch_on_merge` enabled, so GitHub deletes the remote branch itself on merge. For a branch with more than one commit, pass `--subject` and `--body` explicitly — the default squash body is `COMMIT_MESSAGES`, which concatenates every commit message rather than reading as one coherent change. See the `pr-workflow` skill for PR body conventions and for what to do if a required check wedges a PR. 4. Switch back to `master`: `git checkout master`. 5. Sync `master`: `git pull` (should fast-forward cleanly since nothing else is committed directly to `master`). 6. Delete the now-merged local branch: `git branch -D <branch>` — use `-D` (force), not `-d`. Because the merge was a squash, the local branch's commits aren't literally an ancestor of the new `master` commit, so `-d`'s "already merged" safety check will refuse to delete it even though the change is fully in `master`. Confirm first (`git log master -1` matches the change, `git status` clean) before force-deleting. diff --git a/.claude/skills/pr-workflow/SKILL.md b/.claude/skills/pr-workflow/SKILL.md new file mode 100644 index 0000000..1520c05 --- /dev/null +++ b/.claude/skills/pr-workflow/SKILL.md @@ -0,0 +1,123 @@ +--- +name: pr-workflow +description: Create a pull request in this ESPHome config repo (nuttytree/ESPHome-Devices). Use when opening a PR, submitting changes, or preparing contributions from this working directory. This repo's conventions differ from upstream esphome/esphome. +allowed-tools: Read, Bash, Glob, Grep +--- + +# PR Workflow (nuttytree/ESPHome-Devices) + +This is the PR workflow for **this repo**: a personal ESPHome device-config and +custom-component repo, default branch `master`. + +> **There is a second, unrelated `pr-workflow` skill in the sibling `dev_esphome/` +> checkout.** That one describes contributing to *upstream ESPHome core*: base on +> `upstream/dev`, fill in `.github/PULL_REQUEST_TEMPLATE.md`, prefix the title with +> `[component]`, `gh pr create --repo esphome/esphome --base dev`. **All of that is +> wrong here.** It only appears as an available skill because `dev_esphome/` is +> configured as an additional working directory for IntelliSense. `dev_esphome/` +> and `esphome.io/` are read-only reference checkouts of other people's projects — +> never push to them or open PRs against them. + +## Related commands + +- `/commit` — branch, run hooks clean, commit, push. Use it for the pre-PR steps. +- `/commit_pr` — the whole cycle: commit, push, PR, **merge**, and clean up. + +Use this skill when you want a PR *opened* but not merged. If the goal is to land +a change end to end, use `/commit_pr` instead. + +## 1. Branch and commit + +Follow `/commit` in full. Never commit on `master` — the `no-commit-to-branch` +hook blocks it. Branch naming, per this repo's history: + +| prefix | use | example | +|---|---|---| +| `f/` | feature or change | `f/pump-restart-resume` | +| `b/` | bug fix | `b/pump-anomaly-detection` | +| `chore/` | maintenance, tooling, docs | `chore/align-lint-with-upstream` | + +## 2. Open the PR + +Against **this repo only**, base `master`: + +```bash +gh pr create --base master --title "<title>" --body-file - +``` + +This repo has no PR template and uses no labels. Match recent merged PRs +(`gh pr list --state merged --limit 5`, then `gh pr view <n> --json body`): + +- **Title** — sentence-case description of the change, no bracket or component + prefix. E.g. "Distrust stale current readings; measure runtime from flow". +- **Body** — prose statement of the problem and why it mattered, then a + `Changes:` bullet list of what was actually done. Reference related PRs or + issues as `#<n>`. Call out anything that silently changes behaviour on upgrade, + such as persisted state being discarded. +- **Verification** — say how it was checked: `esphome config <device>.yaml`, or an + `esphome compile` if C++ changed. + +## 3. Wait for CI + +`master` requires **all seven checks** to pass before a PR can merge: + +``` +pre-commit esphome config +compile pool compile water-heater compile hvac +compile pool-lights compile master-bed +``` + +`enforce_admins` is on, so this applies to the repo owner too. Budget ~5 minutes: +the five compiles run in parallel, esp32 builds take ~4 min, esp8266 ~2 min. + +```bash +gh pr checks <number> +``` + +Two CI facts worth knowing when a check looks wrong: + +- **Compile jobs only run on `pull_request` and pushes to `master`.** Pushing a + branch with no PR open runs nothing at all. This is deliberate — see the comment + on the `on:` trigger in `.github/workflows/ci.yml`. +- **`devices/secrets.yaml` is gitignored**, so CI generates a placeholder one via + `scripts/generate_ci_secrets.py` before validating or compiling. A new `!secret` + reference is picked up automatically; no CI change needed. + +Some lint rules only fail on Linux. `ruff`'s `EXE001` (shebang on a +non-executable file) is skipped on Windows, so a local `pre-commit run +--all-files` can pass while CI fails. Fix with +`git update-index --chmod=+x <file>` rather than dropping the shebang. + +## 4. Merging (if asked) + +Squash only — `allow_merge_commit` and `allow_rebase_merge` are both `false`: + +```bash +gh pr merge <number> --squash --delete-branch +``` + +- The repo has `delete_branch_on_merge` enabled, so the remote branch goes either + way. `--delete-branch` additionally switches back to `master`, pulls, and + deletes the *local* branch — doing steps that would otherwise be manual. +- Default squash body is `COMMIT_MESSAGES`, which concatenates every commit on the + branch. For a multi-commit branch, pass `--subject` and `--body` explicitly so + the squashed commit reads as one coherent message in this repo's style. +- Deleting the local branch by hand needs `git branch -D`, not `-d`: after a + squash the branch's commits aren't ancestors of the new `master` commit, so + `-d`'s merged-check refuses. + +If `gh` reports it can't merge — conflicts, a failing required check — stop and +report rather than forcing it. + +### If a required check wedges a PR + +A required context that never reports leaves the PR unmergeable. To regain the +admin bypass without disturbing anything else: + +```bash +gh api -X DELETE repos/:owner/:repo/branches/master/protection/enforce_admins +``` + +Narrower `DELETE` endpoints also exist for `required_status_checks` and for the +whole `protection` object. Branch protection never blocks editing the protection +rule itself, and repo-owner admin can't be lost.