review-pr

# PR Review: SAP Cloud SDK for Python Reviews a PR against 25 criteria across 6 sections. Run from the root of the `cloud-sdk-python` repository. --- ## Phase 1: Identify the PR Determine `REPO` and `NUMBER` from what the user provided: - **Full GitHub URL** (e.g. `https://github.com/<owner>/cloud-sdk-python/pull/1`): parse `REPO=<owner>/cloud-sdk-python`, `NUMBER=1`. - **`owner/repo#number`** (e.g. `<owner>/cloud-sdk-python#1`): parse accordingly. - **Plain number** (e.g. `93`): `REPO=SAP/cloud-sdk-python`, `NUMBER=93`. - **Nothing provided**: list open PRs from the default repo and ask the user to pick one: ```bash gh pr list --repo SAP/cloud-sdk-python --state open --json number,title,author,headRefName ``` Use `REPO` and `NUMBER` in every subsequent command. --- ## Phase 2: Gather Data **Step 1** — Run all five commands **in parallel**: ```bash gh pr view <NUMBER> --repo <REPO> --json number,title,body,state,labels,headRefName,baseRefName,author,commits,reviews,reviewRequests,files,additions,deletions ``` ```bash gh pr diff <NUMBER> --repo <REPO> ``` ```bash gh pr checks <NUMBER> --repo <REPO> 2>/dev/null || echo "CI checks not yet available" ``` ```bash gh pr view <NUMBER> --repo <REPO> --comments ``` ```bash gh pr view <NUMBER> --repo <REPO> --json commits --jq '.commits[].messageHeadline' ``` **Step 2** — Fetch each changed file at the PR head ref for accurate line references. From the `files` array in Step 1, for each non-binary file run (in parallel): ```bash mkdir -p /tmp/pr<NUMBER>/$(dirname <path>) gh api "repos/<REPO>/contents/<path>?ref=<headCommitSHA>" \ -H "Accept: application/vnd.github.raw+json" \ > /tmp/pr<NUMBER>/<path> ``` Use the head **commit SHA** (not the branch name) as the ref — branch names are ambiguous for forks. Skip files larger than 500 KB or with binary extensions (`.png`, `.jpg`, `.whl`, `.gz`, etc.). This gives you the full file content at the exact PR state, which you will use in Phase 3 for all `file:line` citations. --- ## Phase 3: Evaluate 23 Criteria Assign each: **✅ Pass** / **⚠️ Warning** / **❌ Fail** / **➖ N/A** For every `file:line` reference: use `Read /tmp/pr<NUMBER>/<path>` on the files fetched in Phase 2. Line numbers in that output are exact. Do not derive line positions from diff hunk offsets — diff arithmetic is unreliable. If you need to verify a specific rule, read the authoritative source directly: - `CONTRIBUTING.md` - `docs/GUIDELINES.md` - `docs/DEVELOPMENT.md` - `.github/pull_request_template.md` - `.github/workflows/` (exact CI job names) --- ### Section A: Process & Compliance **A1: PR template complete** 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 → ❌. **A2: Conventional Commits** 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. **A3: Issue linked** PR body must contain `Closes #N`, `Fixes #N`, or `Resolves #N`. **A4: AI-generated code disclosure** 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`. --- ### Section B: Security & Sensitive Data **B1: No sensitive data in code** 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 → ❌. **B2: No sensitive data in PR body** 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. --- ### Section C: Code Quality **C1: CI checks passing** | Job name | Meaning | |---|---| | `Code Quality Checks` | ruff lint + ruff format + ty type check | | `Unit Tests with Coverage` | pytest, coverage ≥ 80% | | `Build SDK` | `uv build` produces `.whl` + `.tar.gz` | | `commit-validation` | commitlint on all commits | | `Enforce version bump when src/ is modified` | version must increase if `src/` changed | | `Verify generated proto code is up-to-date` | only for proto changes | | `Analyze (python)` | CodeQL security scan | Any required job ❌ → overall ❌. Pending → ⚠️. **C2: Version bump** If diff touches any `src/` file: `version` in `pyproject.toml` MUST be incremented (semver). Only `docs/`, `tests/`, `.github/`, `.claude/` changed → ➖ N/A. **C3: Type hints** 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 ❌. **C4: No hardcoded values** No magic strings (e.g., `/etc/secrets/appfnd` inline) or magic numbers. Use module-level constants or enum values. **C5: Import organization** 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`. **C6: Naming conventions** - Enum values: `SCREAMING_SNAKE_CASE = "snake_case_value"` (e.g., `AGENT_GATEWAY = "agent_gateway"`) - Enum members and module-level lists: alphabetical order - Private methods/attributes/modules: leading underscore - File names: `snake_case.py` - GitHub workflow files: `.yaml` extension, not `.yml` **C7: No unused code** No unused imports, variables, or dead methods introduced. **C8: No unjustified new dependencies** New runtime dep in `pyproject.toml`: must be minimal, justified, checked for CVEs. No duplicate `requirements-*.txt` alongside `pyproject.toml`. **C9: Proto code freshness** 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. --- ### Section D: API & Design **D1: API future-proofing** 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. **D2: Public API hygiene** `__init__.py` and `__all__` expose only genuinely public symbols. Internal helpers prefixed `_`. No unnecessary wrapper classes (wrapping `requests.Session` 1:1 without adding value). **D3: Breaking changes properly marked** 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 → ❌. **D4: Pagination & tenant filtering consistency** New list/query operations: encapsulate pagination params like existing modules (see `destination`). Tenant-scoped operations: filter by tenant property for consistency. **D5: Telemetry instrumentation** 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. **D6: Multi-tenancy support** Multi-tenancy is a cross-cutting concern for most BTP applications. For new service modules, consider the full scope of multi-tenancy — not just token routing, but also provisioning and tenant isolation: - **Provisioning:** Does the service require a subscription or onboarding step per tenant? Does the module need to support tenant lifecycle callbacks (subscribe/unsubscribe)? - **Tenant isolation:** Is data or configuration isolated per tenant at the service level? Does the module enforce or expose tenant boundaries correctly? - **Auth/routing:** Does the module need to route requests to a subscriber tenant context (e.g., XSUAA subdomain replacement, IAS `app_tid`, or Destination Service routing)? - **Infrastructure:** Is there any infrastructure work required (e.g., new service binding fields, SPII fragments, Subscription Manager integration)? This is a **nice-to-have** (⚠️ if absent, not ❌), but must be a conscious decision either way — the `user-guide.md` must document whether support is present, planned, or intentionally out of scope (see E2a). --- ### Section E: Tests & Documentation **E1: Tests added/updated** 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`. **E2: Documentation quality** 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. **E2a: Multi-tenancy documentation** Every new or modified `user-guide.md` must contain a `## Multi-tenancy` section with all four fields: - **Supported:** Yes / No / Not yet / N/A - **Authentication:** XSUAA / IAS / Other / — - **How to use:** explanation (or "Not applicable") - **Further reading:** link(s) or "—" If the module exposes XSUAA or IAS authentication, the section must describe the actual mechanism (e.g., `access_strategy` param, `tenant` param, token URL replacement). A placeholder or missing section → ⚠️. If the module is a new service module with no section at all → ❌. **E3: Module structure compliance** New modules follow: ``` src/sap_cloud_sdk/[module]/ ├── __init__.py (create_client(), __all__) ├── client.py (or {service}_client.py) ├── config.py (load_from_env_or_mount(), *Config dataclass) ├── exceptions.py (exception hierarchy) ├── _models.py (Pydantic models) ├── py.typed (empty PEP 561 marker) └── user-guide.md tests/[module]/unit/ tests/[module]/integration/ (optional, BDD) ``` --- ## Phase 4: Report ```markdown ## PR Review: #<number>: <title> **Author**: <author> **Branch**: `<headRef>` → `<baseRef>` **Verdict**: ✅ Ready to Merge | ⚠️ Needs Minor Work | ❌ Blocked **Summary**: <one sentence> --- ### A: Process & Compliance | # | Criterion | Status | Finding | |---|-----------|--------|---------| | A1 | PR template complete | | | | A2 | Conventional Commits | | | | A3 | Issue linked | | | | A4 | AI-generated code disclosure | | | ### B: Security & Sensitive Data | # | Criterion | Status | Finding | |---|-----------|--------|---------| | B1 | No sensitive data in code | | | | B2 | No sensitive data in PR body | | | ### C: Code Quality | # | Criterion | Status | Finding | |---|-----------|--------|---------| | C1 | CI checks passing | | | | C2 | Version bump | | | | C3 | Type hints | | | | C4 | No hardcoded values | | | | C5 | Import organization | | | | C6 | Naming conventions | | | | C7 | No unused code | | | | C8 | No unjustified new dependencies | | | | C9 | Proto code freshness | | | ### D: API & Design | # | Criterion | Status | Finding | |---|-----------|--------|---------| | D1 | API future-proofing | | | | D2 | Public API hygiene | | | | D3 | Breaking changes marked | | | | D4 | Pagination & tenant filtering | | | | D5 | Telemetry instrumentation | | | | D6 | Multi-tenancy support | | | ### E: Tests & Documentation | # | Criterion | Status | Finding | |---|-----------|--------|---------| | E1 | Tests added/updated | | |