Skip to content

chore: add create-pr skill#978

Merged
mergify[bot] merged 3 commits into
sigp:unstablefrom
diegomrsantos:chore/create-pr-skill
Apr 29, 2026
Merged

chore: add create-pr skill#978
mergify[bot] merged 3 commits into
sigp:unstablefrom
diegomrsantos:chore/create-pr-skill

Conversation

@diegomrsantos
Copy link
Copy Markdown
Member

Problem, Evidence, and Context (Required)

  • The create-pr workflow existed locally but was hidden by Git exclude rules, so it was not available from a clean checkout.
  • This is worth tracking now because PR creation should use the same repo-local guidance as the rest of the .claude workflow tree.
  • Evidence: this PR adds the canonical source skill at .claude/skills/create-pr/SKILL.md. The generated Codex mirrors remain local-only, as described in AGENTS.md.
  • Relevant links: N/A.

Change Overview (Required)

  • Adds the create-pr skill under .claude/skills/.
  • Reviewers can read the front matter first, then the pre-flight, PR-writing, and posting sections in order.
  • This does not add generated .agents/ or .codex/ mirror files and does not change runtime behavior.

Risks, Trade-offs, and Mitigations (Required)

  • The blast radius is limited to repository workflow guidance.
  • The main risk is stale or unclear PR-writing guidance. The skill is kept in the canonical .claude tree so future edits can follow the repo's normal authoring flow.
  • No runtime trade-off is introduced.

Validation (Required)

  • Confirmed the local Git exclude rules no longer hide .claude/skills/create-pr/SKILL.md.
  • Verified the branch diff is one new skill file and has no whitespace errors.
  • The commit hook completed the repo's formatting, clippy, and Cargo.toml sorting checks.
  • No runtime tests were run because this is a workflow prompt change only.

Rollback (Required for behavior or runtime changes; optional otherwise)

  • Revert the commit to remove the skill from the tracked .claude tree.
  • No config, data, or operational rollback is needed.

Blockers / Dependencies (Optional)

N/A.

Additional Info / Next Steps (Optional)

N/A.

Comment thread .claude/skills/create-pr/SKILL.md Outdated
@shane-moore
Copy link
Copy Markdown
Member

Nice skill. Two additions from my pass on #966 that I'd fold in:

1. Hard length budget on the visible body (~150 words). The "make it easy to review" principles don't fight verbosity on their own without a number. If the body can't fit in the budget, that's usually a signal to split the PR, not to grow the description. Enforcement rule: "cut, don't hide."

2. <details> collapsed section for bot-readable depth. Two audiences: a senior reviewer skimming wants the minimum; a future Claude reviewing the PR wants spec file:line anchors, "why was the old code wrong" archaeology, and intentionally-unchanged callouts. Putting all of that in the visible body bloats it; deleting it loses context a bot can't re-derive.

Pattern that worked on #966:

<details>
<summary>Background: spec references, why X was wrong</summary>

### Why X was wrong
### Spec references
- `file:line`: what it says
### Intentionally unchanged
- <preempts scope-creep follow-ups>

</details>

Visible body stays skimmable; depth is one click away.

- Drop hardcoded PR section list; defer to .github/PULL_REQUEST_TEMPLATE.md
  loaded in pre-flight so the skill stays in sync with template edits.
- Remove --repo sigp/anchor from gh pr create so the command works from
  forks and lets gh infer the upstream target.
@diegomrsantos
Copy link
Copy Markdown
Member Author

diegomrsantos commented Apr 25, 2026

Thanks, good points to think through.

On the 150-word budget: I like the principle but not the specific number. The repo template already requires five sections (Problem/Evidence/Context, Change Overview, Risks/Trade-offs, Validation, Rollback), and ~30 words per section pushes authors toward cryptic rather than clear. I'd rather frame it as "if the visible body doesn't fit on a screen, that's a signal to split, not to grow the description" and keep "cut, don't hide" as the enforcement rule.

On the <details> block for bot-readable depth: interesting idea, and I sat with it for a bit before landing on some concerns:

  • Context that gets collapsed tends to be missed by humans too. If spec file:line anchors matter, they matter for the human reviewer, not just a future bot.
  • "Intentionally unchanged" is already covered by principle 4 in the visible body. Duplicating it into a collapsed block is the kind of drift we're trying to avoid.
  • It introduces a second structure on top of the template. I'd rather skill guidance stay template-compatible than orthogonal to it.
  • If the underlying problem is "bots lose archaeology context," commit messages and linked issues feel like the better place for that than hidden HTML in the PR body.

Happy to fold the length principle (without the hard number) into the skill. Leaning toward leaving <details> as an individual-author choice rather than a skill rule, but open to pushback.

@diegomrsantos
Copy link
Copy Markdown
Member Author

@shane-moore are you ok to approve it?

Copy link
Copy Markdown
Member

@shane-moore shane-moore left a comment

Choose a reason for hiding this comment

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

great! also perhaps mention this in mattermost or something that it can be useful for creating pr's or even just to make the description for a pr you create manually

@diegomrsantos diegomrsantos self-assigned this Apr 29, 2026
@mergify mergify Bot merged commit 377969b into sigp:unstable Apr 29, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants