Skip to content

Commit 2577360

Browse files
jeanscherfjeanscherf
committed
feat(contrib): add PR review and module scaffold skills
Adds two Claude Code skills to .claude/skills/ for SDK contributors: - review-pr: reviews a PR against 23 criteria across 6 sections (process, security, code quality, API design, tests, documentation). Runs from the repo root via gh CLI. Outputs a structured table report with verdict and optional gh pr review posting. - scaffold-module: generates the full standard module layout (10 files + telemetry wiring) for a new BTP service integration. Includes a self-review phase that runs ruff and fixes issues before reporting. Both skills are compatible with Claude Code, Cline, Copilot, and other agentic tools via the tools/compatibility frontmatter fields.
1 parent 4f6aecb commit 2577360

2 files changed

Lines changed: 725 additions & 0 deletions

File tree

‎.claude/skills/review-pr/SKILL.md‎

Lines changed: 256 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,256 @@
1+
---
2+
name: review-pr
3+
description: Review a pull request against all cloud-sdk-python contribution standards. Use when you want to review a PR, check if a PR is ready to merge, or get structured feedback on pending changes.
4+
tools: Bash, Read
5+
compatibility: gh CLI ≥ 2.0, git, GitHub access to SAP/cloud-sdk-python
6+
---
7+
8+
# PR Review: SAP Cloud SDK for Python
9+
10+
Reviews a PR against 23 criteria across 6 sections. Run from the root of the `cloud-sdk-python` repository.
11+
12+
---
13+
14+
## Phase 1: Identify the PR
15+
16+
If the user provided a PR number, use it.
17+
18+
Otherwise list open PRs and ask the user to pick one:
19+
```bash
20+
gh pr list --state open --json number,title,author,headRefName
21+
```
22+
23+
---
24+
25+
## Phase 2: Gather Data
26+
27+
Run all five commands **in parallel**:
28+
29+
```bash
30+
gh pr view <number> --json number,title,body,state,labels,headRefName,baseRefName,author,commits,reviews,reviewRequests,files,additions,deletions
31+
```
32+
```bash
33+
gh pr diff <number>
34+
```
35+
```bash
36+
gh pr checks <number> 2>/dev/null || echo "CI checks not yet available"
37+
```
38+
```bash
39+
gh pr view <number> --comments
40+
```
41+
```bash
42+
gh pr view <number> --json commits --jq '.commits[].messageHeadline'
43+
```
44+
45+
---
46+
47+
## Phase 3: Evaluate 23 Criteria
48+
49+
Assign each: **✅ Pass** / **⚠️ Warning** / **❌ Fail** / **➖ N/A**
50+
51+
Always include `file:line` references from the diff as evidence.
52+
53+
If you need to verify a specific rule, read the authoritative source directly:
54+
- `CONTRIBUTING.md`
55+
- `docs/GUIDELINES.md`
56+
- `docs/DEVELOPMENT.md`
57+
- `.github/pull_request_template.md`
58+
- `.github/workflows/` (exact CI job names)
59+
60+
---
61+
62+
### Section A: Process & Compliance
63+
64+
**A1: PR template complete**
65+
Template requires: Description, Related Issue, Type of Change (one box ticked), How to Test (numbered steps), checklist of 9 items all ticked. Empty or placeholder body → ❌.
66+
67+
**A2: Conventional Commits**
68+
Every commit headline must match `type(scope): description`. Types: `feat`, `fix`, `chore`, `docs`, `refactor`, `test`, `ci`, `perf`, `style`, `build`, `revert`. PR title is also validated. Check `commit-validation` CI job. Quote failing commit titles.
69+
70+
**A3: Issue linked**
71+
PR body must contain `Closes #N`, `Fixes #N`, or `Resolves #N`.
72+
73+
**A4: AI-generated code disclosure**
74+
If the diff looks AI-generated, the PR description must explicitly disclose it and reference the [SAP AI contribution guideline](https://github.com/SAP/.github/blob/main/CONTRIBUTING_USING_GENAI.md). Required by `CONTRIBUTING.md`.
75+
76+
---
77+
78+
### Section B: Security & Sensitive Data
79+
80+
**B1: No sensitive data in code**
81+
Scan diff for: hardcoded credentials/tokens/API keys, SAP-internal URLs (non-public hostnames or internal tooling URLs that should not be in a public repo), tenant IDs as literals, customer names, environment-specific configs. Any hit → ❌.
82+
83+
**B2: No sensitive data in PR body**
84+
Check the PR body for the same categories as B1: account-specific URLs containing GUIDs or subaccount identifiers, real tenant IDs, internal email addresses, internal tooling references (e.g., Slack channels, internal issue trackers). The PR template disclaimer applies.
85+
86+
---
87+
88+
### Section C: Code Quality
89+
90+
**C1: CI checks passing**
91+
| Job name | Meaning |
92+
|---|---|
93+
| `Code Quality Checks` | ruff lint + ruff format + ty type check |
94+
| `Unit Tests with Coverage` | pytest, coverage ≥ 80% |
95+
| `Build SDK` | `uv build` produces `.whl` + `.tar.gz` |
96+
| `commit-validation` | commitlint on all commits |
97+
| `Enforce version bump when src/ is modified` | version must increase if `src/` changed |
98+
| `Verify generated proto code is up-to-date` | only for proto changes |
99+
| `Analyze (python)` | CodeQL security scan |
100+
101+
Any required job ❌ → overall ❌. Pending → ⚠️.
102+
103+
**C2: Version bump**
104+
If diff touches any `src/` file: `version` in `pyproject.toml` MUST be incremented (semver). Only `docs/`, `tests/`, `.github/`, `.claude/` changed → ➖ N/A.
105+
106+
**C3: Type hints**
107+
All new/modified public functions, methods, class attributes must have full annotations (params + return type). New modules need `py.typed`. Missing `Optional`, `Union`, or return type on a public method → ⚠️ or ❌.
108+
109+
**C4: No hardcoded values**
110+
No magic strings (e.g., `/etc/secrets/appfnd` inline) or magic numbers. Use module-level constants or enum values.
111+
112+
**C5: Import organization**
113+
Top-level imports preferred (PEP 8). Lazy imports inside functions are a smell without a documented circular-import reason. No `requirements-*.txt` for deps already in `pyproject.toml`.
114+
115+
**C6: Naming conventions**
116+
- Enum values: `SCREAMING_SNAKE_CASE = "snake_case_value"` (e.g., `AGENT_GATEWAY = "agent_gateway"`)
117+
- Enum members and module-level lists: alphabetical order
118+
- Private methods/attributes/modules: leading underscore
119+
- File names: `snake_case.py`
120+
- GitHub workflow files: `.yaml` extension, not `.yml`
121+
122+
**C7: No unused code**
123+
No unused imports, variables, or dead methods introduced.
124+
125+
**C8: No unjustified new dependencies**
126+
New runtime dep in `pyproject.toml`: must be minimal, justified, checked for CVEs. No duplicate `requirements-*.txt` alongside `pyproject.toml`.
127+
128+
**C9: Proto code freshness**
129+
If `.proto` files under `src/sap_cloud_sdk/core/auditlog_ng/proto/` changed: "Verify generated proto code is up-to-date" CI job must pass.
130+
131+
---
132+
133+
### Section D: API & Design
134+
135+
**D1: API future-proofing**
136+
New config/behavior options should be a `*Config` dataclass, not individual params. Enums over bare string constants. `create_client()` factory present; direct constructor warns users to use factory instead.
137+
138+
**D2: Public API hygiene**
139+
`__init__.py` and `__all__` expose only genuinely public symbols. Internal helpers prefixed `_`. No unnecessary wrapper classes (wrapping `requests.Session` 1:1 without adding value).
140+
141+
**D3: Breaking changes properly marked**
142+
Breaking = removing/renaming a public function/method/param, changing return type, making optional param required. If present and NOT marked with the "Breaking Changes" checkbox + migration section → ❌.
143+
144+
**D4: Pagination & tenant filtering consistency**
145+
New list/query operations: encapsulate pagination params like existing modules (see `destination`). Tenant-scoped operations: filter by tenant property for consistency.
146+
147+
**D5: Telemetry instrumentation**
148+
New client methods: `@record_metrics(Module.X, Operation.Y)` from `core/telemetry`. New module: constant added to `core/telemetry/module.py` and operations to `core/telemetry/operation.py`. If module is called by other SDK modules: `_telemetry_source: Optional[Module] = None` param present.
149+
150+
---
151+
152+
### Section E: Tests & Documentation
153+
154+
**E1: Tests added/updated**
155+
Every changed `src/` file → corresponding change in `tests/`. Unit: `tests/[module]/unit/test_*.py`. Integration (BDD): `tests/[module]/integration/*.feature` + `test_*_bdd.py`. Test names: `test_<functionality>_<condition>_<expected_result>`. New env vars for integration tests documented in `.env_integration_tests`.
156+
157+
**E2: Documentation quality**
158+
New modules: `user-guide.md` with overview, quick start, config examples, API examples, troubleshooting. Changed public APIs: docstrings updated (Google/NumPy style: `Args:`, `Returns:`, `Raises:`). Sub-audience features not mixed into the general user guide.
159+
160+
**E3: Module structure compliance**
161+
New modules follow:
162+
```
163+
src/sap_cloud_sdk/[module]/
164+
├── __init__.py (create_client(), __all__)
165+
├── client.py (or {service}_client.py)
166+
├── config.py (load_from_env_or_mount(), *Config dataclass)
167+
├── exceptions.py (exception hierarchy)
168+
├── _models.py (Pydantic models)
169+
├── py.typed (empty PEP 561 marker)
170+
└── user-guide.md
171+
172+
tests/[module]/unit/
173+
tests/[module]/integration/ (optional, BDD)
174+
```
175+
176+
---
177+
178+
## Phase 4: Report
179+
180+
```markdown
181+
## PR Review: #<number>: <title>
182+
183+
**Author**: <author> **Branch**: `<headRef>` → `<baseRef>`
184+
**Verdict**: ✅ Ready to Merge | ⚠️ Needs Minor Work | ❌ Blocked
185+
**Summary**: <one sentence>
186+
187+
---
188+
189+
### A: Process & Compliance
190+
| # | Criterion | Status | Finding |
191+
|---|-----------|--------|---------|
192+
| A1 | PR template complete | | |
193+
| A2 | Conventional Commits | | |
194+
| A3 | Issue linked | | |
195+
| A4 | AI-generated code disclosure | | |
196+
197+
### B: Security & Sensitive Data
198+
| # | Criterion | Status | Finding |
199+
|---|-----------|--------|---------|
200+
| B1 | No sensitive data in code | | |
201+
| B2 | No sensitive data in PR body | | |
202+
203+
### C: Code Quality
204+
| # | Criterion | Status | Finding |
205+
|---|-----------|--------|---------|
206+
| C1 | CI checks passing | | |
207+
| C2 | Version bump | | |
208+
| C3 | Type hints | | |
209+
| C4 | No hardcoded values | | |
210+
| C5 | Import organization | | |
211+
| C6 | Naming conventions | | |
212+
| C7 | No unused code | | |
213+
| C8 | No unjustified new dependencies | | |
214+
| C9 | Proto code freshness | | |
215+
216+
### D: API & Design
217+
| # | Criterion | Status | Finding |
218+
|---|-----------|--------|---------|
219+
| D1 | API future-proofing | | |
220+
| D2 | Public API hygiene | | |
221+
| D3 | Breaking changes marked | | |
222+
| D4 | Pagination & tenant filtering | | |
223+
| D5 | Telemetry instrumentation | | |
224+
225+
### E: Tests & Documentation
226+
| # | Criterion | Status | Finding |
227+
|---|-----------|--------|---------|
228+
| E1 | Tests added/updated | | |
229+
| E2 | Documentation quality | | |
230+
| E3 | Module structure compliance | | |
231+
232+
---
233+
234+
### ❌ Blocking Issues
235+
- **[C2]**: <specific finding with file:line>
236+
237+
### ⚠️ Non-Blocking Suggestions
238+
- **[C6]**: <suggestion>
239+
240+
### ✅ Things Done Well
241+
- <observation>
242+
```
243+
244+
Verdict: any ❌ → **Blocked** · any ⚠️ → **Needs Minor Work** · all ✅/➖ → **Ready to Merge**
245+
246+
---
247+
248+
## Phase 5 (Optional): Post Review
249+
250+
Ask: "Post as GitHub PR review? (comment / request-changes / approve / skip)"
251+
252+
```bash
253+
gh pr review <number> --comment --body "<report>"
254+
gh pr review <number> --request-changes --body "<report>"
255+
gh pr review <number> --approve --body "<report>"
256+
```

0 commit comments

Comments
 (0)