القائمة
Use when reviewing PRs that add or modify API endpoints - verifying implementation completeness, test coverage, assertion patterns, and documentation updates
التثبيت باستخدام Codex أو Claude انسخ هذا Prompt والصقه في Codex أو Claude أو مساعد آخر ليراجع صفحة Skill ويثبّتها لك.
استنادا إلى تصنيف SOC المهني
Use when cutting a new release of the Smartsheet Python SDK — changelog update, tag, and GitHub Release
Use when adding or modifying API endpoints in smartsheet-python-sdk - implementing resource methods, handling request/response serialization, or updating endpoint parameters
| name | review-api-endpoint |
| description | Use when reviewing PRs that add or modify API endpoints - verifying implementation completeness, test coverage, assertion patterns, and documentation updates |
Systematic checklist for reviewing SDK endpoint implementations. Ensures all 4 required components exist: implementation, tests (5 types with correct assertions), WireMock mappings, and documentation. Critical: Verify implementation matches OpenAPI spec exactly.
GOLD STANDARD VERIFICATION: Compare implementation against tests/mock_api/reports/ during review. Tests should match these patterns exactly.
Required before review:
Cannot approve without spec AND mappings verification.
Work through each section systematically:
Required: OpenAPI spec URL or path
If any spec mismatch found: Request changes immediately - do not continue review until fixed.
Required: WireMock mappings location (GitHub repo or local path)
/{resource}/{method}/all-response-body-properties/{resource}/{method}/required-response-body-properties (if applicable)/errors/400-response, /errors/500-responseIf any mapping missing or spec mismatch found: Request changes immediately - do not continue review.
CRITICAL IMPORTANCE: Breaking changes in the public SDK contract break user code in production. This check is MANDATORY and NON-NEGOTIABLE.
Rule: Unless the PR author EXPLICITLY states "This PR contains breaking changes" with justification, ANY breaking change MUST be flagged and PR MUST be rejected.
What constitutes a breaking change:
Method Signature Changes (BREAKING):
str → int, List[str] → str, etc.sheet_id → sheetId breaks keyword argument callsReturn Type Changes (BREAKING):
Sheet → List[Sheet], Union[Sheet, Error] → SheetError from union - Breaks isinstance(response, Error) checksid: int → id: str in response modelBehavior Changes (BREAKING):
Non-Breaking Changes (ALLOWED):
def method(x, new_param=None) is safeReview Process:
Check PR description first:
Compare old vs new signatures:
If breaking change found without documentation:
If breaking change is documented:
Example Breaking Changes to Flag:
# BREAKING: Removed parameter
# Before
def get_sheet(self, sheet_id: int, include: List[str] = None)
# After
def get_sheet(self, sheet_id: int)
# BREAKING: Changed optional to required
# Before
def get_sheet(self, sheet_id: int, format: str = "json")
# After
def get_sheet(self, sheet_id: int, format: str)
# BREAKING: Changed return type
# Before
def get_sheet(self, sheet_id: int) -> Union[Sheet, Error]
# After
def get_sheet(self, sheet_id: int) -> Sheet
# BREAKING: Removed property from response model
# Before: Sheet has .permalink property
# After: Sheet doesn't have .permalink property
Common Rationalizations to REJECT:
| Excuse | Reality | Response |
|---|---|---|
| "Users probably aren't using that parameter" | You don't know that | REQUEST CHANGES |
| "It's a minor change" | Minor to you ≠ minor to users | REQUEST CHANGES |
| "Tests pass" | Tests don't catch breaking changes to public API | REQUEST CHANGES |
| "OpenAPI spec changed" | SDK still breaks existing user code | REQUEST CHANGES (requires explicit BREAKING CHANGE note) |
| "Better API design" | Breaking user code is not better | REQUEST CHANGES or deprecation path required |
| "Will document in CHANGELOG" | PR description must state it FIRST | REQUEST CHANGES until PR description updated |
Red Flags - Breaking Change Not Documented:
These thoughts mean STOP and REQUEST CHANGES immediately:
The Rule (Repeat for Emphasis):
NO BREAKING CHANGES WITHOUT EXPLICIT PR DESCRIPTION STATING "BREAKING CHANGE:" + JUSTIFICATION
If you find a breaking change and PR description doesn't explicitly mention it: IMMEDIATE REQUEST CHANGES. DO NOT PROCEED WITH REVIEW.
File: smartsheet/{resource}.py
fresh_operation() → prepare_request() → request()/resource/{id} not /resource/123Union[ExpectedType, Error]Reference: @ADVANCED.md "Resource Module Organization"
File: tests/mock_api/{resource}/test_{method_name}.py
CRITICAL: All 5 test types MUST exist. Tests MUST strictly follow @TESTING.md standards. Check test file has all 5 functions.
GOLD STANDARD COMPARISON: Open tests/mock_api/reports/test_create_report.py (or any report test) side-by-side. Implementation should match these patterns exactly - same assertion style, same structure, same helper usage.
Test data must match OpenAPI spec:
Request test data uses types/formats from spec
Response assertions match spec schema
Error tests match spec error responses
test_{method}_generated_url_is_correct exists
get_wiremock_request(request_id) to retrieve requesturlparse() and parse_qs()assert query == {...} (NOT property-by-property)assert not query test_{method}_all_response_properties exists
isinstance(response, ExpectedType)isinstance(response, Result) AND isinstance(response.result[0], InnerType)assert response.to_dict() == {...} (NOT properties)json.loads(wiremock_request["body"])assert request_body == {...}assert not request_body test_{method}_required_response_properties exists (if applicable)
assert not request_body test_{method}_error_4xx exists
/errors/400-response test scenarioisinstance(response, Error) test_{method}_error_5xx exists
/errors/500-response test scenarioisinstance(response, Error)Reference:
Common anti-patterns to catch:
No property-by-property assertions
assert response.id == 123; assert response.name == "test"assert response.to_dict() == {"id": 123, "name": "test"}No property-by-property query assertions
assert query["page"] == ["1"]; assert query["pageSize"] == ["100"]assert query == {"page": ["1"], "pageSize": ["100"]}Request body assertions for POST/PUT/PATCH
all_response_properties testget_wiremock_request(request_id) and json.loads()Type safety for wrapped responses
isinstance(response, Result)isinstance(response.result[0], Model)Repository: smartsheet-sdk-tests
Note: Cannot directly verify without checking external repo, but look for test scenario names:
/{resource}/{method}/all-response-body-properties/{resource}/{method}/required-response-body-properties (if applicable)/errors/400-response and /errors/500-responseIf tests reference non-standard scenarios, flag for WireMock mapping verification.
File: docs-source/smartsheet_api.rst (or appropriate .rst file)
.. automethod:: {Class}.{method}uv run sphinx-build -b html docs-source docs-source/_build/html)Reference: @CONTRIBUTING.md "Documentation" section and @ADVANCED.md for documentation requirements
tests/mock_api/{resource}/common_test_constants.py| Endpoint Type | Required Tests | Request Body Assertions |
|---|---|---|
| GET single | 4-5 (skip required if N/A) | No |
| GET list | 4-5 | No |
| POST | 5 | Yes - required |
| PUT | 5 | Yes - required |
| PATCH | 5 | Yes - required |
| DELETE | 5 | No |
Approve (Ready to Merge):
Request Changes (MUST block merge - non-negotiable):
Comment/Question:
These thoughts mean STOP - review is incomplete:
All of these mean: Review incomplete. Verify spec and mappings first, compare to gold standard, then work through checklist systematically.
| Mistake | Reality | Fix |
|---|---|---|
| Didn't check OpenAPI spec | Cannot verify correctness without it | Get spec, verify every detail |
| Didn't check for breaking changes | Breaking changes break user code | Compare old/new signatures explicitly |
| "No breaking changes mentioned" | You must verify, not trust | Check every signature change against breaking change list |
| "Change seems reasonable" | Reasonable ≠ non-breaking | Verify against explicit breaking change criteria |
| "Spec roughly matches" | Must match EXACTLY | Compare every parameter/field |
| "Tests pass, looks good" | Passing ≠ complete coverage | Check all 5 types exist |
| "Has error tests" | Only checking 1 of 2 error types | Both 4xx AND 5xx required |
| "Response assertions look fine" | Property-by-property missed | Must use .to_dict() == {...} |
| "POST test complete" | Missing request body assertion | Check TESTING.md line 136-141 |
| "Type checking present" | Only wrapper or only inner | Must check BOTH types |
| Missing query assertion pattern | Allows unexpected params | Must assert complete query object |
| "Docs aren't critical" | Sphinx won't discover method | Documentation required, not optional |
| Approving under time pressure | Quality > speed | Request changes, cite standards |
| "Tests look good enough" | Must strictly follow TESTING.md | Reference @TESTING.md standards |
| Didn't verify WireMock mappings | Tests may reference non-existent mappings | Check repo or local path |
| "Mappings probably exist" | Cannot assume - must verify | Verify actual mapping files exist |
| "Type hints are optional" | Type definitions MANDATORY for new endpoints | Request type defs |
| Didn't check Python version | Must support 3.7, no 3.8+ | Verify compatibility |
| Assumed docs needed | Check ADVANCED.md requirements | Verify docs actually needed |
For missing OpenAPI spec:
Cannot proceed with review. OpenAPI spec required for verification.
Please provide:
- Smartsheet public API spec: https://developers.smartsheet.com/_spec/api/smartsheet/openapi.json
- OR path to custom OpenAPI spec for this endpoint
Will verify implementation matches spec exactly before approving.
For breaking changes without documentation:
CRITICAL: This PR introduces breaking changes without explicit documentation.
Breaking changes found:
1. Removed optional parameter `include` - breaks calls using this parameter
2. Changed return type from `Union[Sheet, Error]` to `Sheet` - breaks error handling code
3. Changed parameter `format` from optional to required - breaks calls without this parameter
**Impact:** These changes will break existing user code in production.
**Required actions:**
- Either: Revert to backward-compatible design (keep optional params, maintain Union return type)
- Or: Update PR description with "BREAKING CHANGE:" header and provide:
- Justification for breaking changes
- Migration path for existing users
- Version bump strategy (major version)
Cannot approve until breaking changes are either removed or explicitly documented with justification.
For spec mismatch:
Request changes: Implementation doesn't match OpenAPI spec.
Issues found:
- Parameter `pageSize` type: spec says integer, implementation uses string
- Missing required parameter `objectType` from spec
- Response field `directId` not in spec schema
- Query parameter `includeAll` in implementation but not in spec
Please update implementation to match spec exactly at:
[spec path/section]
For missing test types:
Not ready to merge. Missing 3 of 5 required test types:
- test_{method}_required_response_properties (if applicable)
- test_{method}_error_4xx
- test_{method}_error_5xx
See TESTING.md "Mock API Test Standards" for complete requirements.
For property-by-property assertions:
Test assertion pattern needs fixing:
Current (line X):
assert response.id == 123
assert response.name == "test"
Required pattern (TESTING.md):
assert response.to_dict() == {
"id": 123,
"name": "test"
}
Whole-object assertions prevent unexpected properties from creeping in.
For missing request body assertions (POST/PUT/PATCH):
POST/PUT/PATCH endpoints require request body assertions (TESTING.md lines 136-141).
Add to test_{method}_all_response_properties:
wiremock_request = get_wiremock_request(request_id)
request_body = json.loads(wiremock_request["body"])
assert request_body == {
# expected request structure
}
This verifies serialization correctness (camelCase conversion, etc).
For missing type definitions:
Request changes: Missing type definitions for new endpoint.
New endpoints require type definitions in smartsheet/models/.
Required types:
- Request model (if POST/PUT/PATCH with body)
- Response model matching OpenAPI spec schema
All types must be Python 3.7 compatible (no 3.8+ syntax).
See existing models in smartsheet/models/ for patterns.
For Python 3.8+ compatibility issues:
Request changes: Code uses Python 3.8+ features.
This SDK must support Python 3.7. Found:
- Line X: Walrus operator := (requires 3.8)
- Line Y: positional-only parameter / (requires 3.8)
- Line Z: TypedDict without typing_extensions (requires 3.8)
Please update to Python 3.7 compatible syntax.
For tests not matching gold standard:
Tests don't match gold standard patterns. Please review tests/mock_api/reports/ and update:
Current issues:
- Property-by-property assertions (should be `.to_dict() ==`)
- Missing complete query parameter assertion
- Error tests don't follow reports pattern
- Missing request body assertions for POST
Compare your test structure against:
- tests/mock_api/reports/test_create_report.py (POST example)
- tests/mock_api/reports/test_add_report_columns.py (POST example)
Tests should match these patterns exactly.
Review is about spec compliance, gold standard pattern matching, and completeness verification.
Implementation can work perfectly but:
Review workflow:
Approve only when spec verified AND mappings verified AND patterns match gold standard AND all 4 components are complete and correct.
Approve only when spec verified AND all 4 components are complete and correct.