Skip to content

Add pr-best-practices skill#504

Open
Thanhphan1147 wants to merge 14 commits into
mainfrom
add_pr_best_practices_skill
Open

Add pr-best-practices skill#504
Thanhphan1147 wants to merge 14 commits into
mainfrom
add_pr_best_practices_skill

Conversation

@Thanhphan1147

@Thanhphan1147 Thanhphan1147 commented May 4, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Add a skill that when called will analyze the changes between the first and final iteration of a PR authored by an LLM. The result is a set of good and bad example maintained in .github/instructions/best-practices.instructions.md. For example:

## Handling Jinja2 templates

Keep rendering logic in charm-state dataclasses or helper builders so templates stay declarative.
State properties that produce ACL conditions should return bare condition strings; the template is responsible for wrapping them in `acl` directive syntax.

<examples>

Checklist

  • I followed the contributing guide
  • I added or updated the documentation (if applicable)
  • I updated docs/changelog.md with user-relevant changes
  • I added a change artifact for user-relevant changes in docs/release-notes/artifacts. If no change artifact is necessary, I tagged the PR with the label no-release-note.
  • I used AI to assist with preparing this PR
  • I added or updated tests as needed (unit and integration)
  • If integration test modules are used: I updated the workflow configuration
    (e.g., in .github/workflows/integration_tests.yaml, ensure the modules list is correct)
  • If this PR involves a Grafana dashboard: I added a screenshot of the dashboard
  • If this PR involves Terraform: terraform fmt passes and tflint reports no errors

@Thanhphan1147 Thanhphan1147 requested a review from a team as a code owner May 4, 2026 15:11
Thanhphan1147 and others added 3 commits May 4, 2026 17:12
- .github/skills/pr-best-practices/SKILL.md: skill that analyzes merged
  LLM-authored PRs, compares first vs final commit, and synthesizes do/don't
  rules with concrete code examples into best-practices.instructions.md
- .github/skills/pr-best-practices/evals/evals.json: 3 eval cases covering
  substantive PR, validation-fix PR, and trivial (non-significant) PR
- .github/instructions/best-practices.instructions.md: initial rules extracted
  from PR #475 (relation contracts, reconcile flow, testing) and PR #500
  (validation error handling) with before/after code examples
- .gitignore: ignore *-workspace/ directories (skill eval scratch data)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Clarify description: 'after human reviews and code changes' (was 'after human review')
- Replace generic file extension list with explicit repo-aware globs:
  **/*.py (charm/test code), haproxy-operator/*.j2 (HAProxy templates),
  **/*.yaml (charmcraft/snapcraft manifests)
- Drop .github/workflows/*.yaml — CI config doesn't yield coding best practices
- Fix typo **/*yaml → **/*.yaml; strip trailing whitespace

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Thanhphan1147 Thanhphan1147 changed the title remove file Add pr-best-practices skil May 4, 2026
@Thanhphan1147 Thanhphan1147 changed the title Add pr-best-practices skil Add pr-best-practices skill May 4, 2026
@Thanhphan1147 Thanhphan1147 added the no-release-note This PR does not require a change artifact label May 4, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Test results for commit 9b28b7d

Test coverage for 9b28b7d

Name                               Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------------
src/charm.py                          45      9      2      0    77%   65-91, 96-98
src/haproxy_spoe_auth_service.py      44     16      2      0    61%   56-64, 76-82, 93-117
src/state.py                          55     15      6      1    67%   64-66, 79, 125-146
------------------------------------------------------------------------------
TOTAL                                144     40     10      1    68%

Static code analysis report

Run started:2026-05-15 23:07:17.024833

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 409
  Total lines skipped (#nosec): 1
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 1

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

@@ -0,0 +1,108 @@
---
description: 'Accumulated best practices extracted from LLM-authored PR reviews'
applyTo: '**'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wonder if we could have a way to split best practicies into multiple instructions to avoid having a single one that applies too all files.
My worry here is that as we accumulate more and more practices, we will front-load a LOT of context that might not be needed by every agent, every prompt.

Skill is invoked explicitly so automated evals are not needed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Libraries: Out of sync no-release-note This PR does not require a change artifact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants