|
| 1 | +--- |
| 2 | +name: code-review |
| 3 | +description: "General repository code review for microsoft/mssql-python, based solely on .github/copilot-instructions.md. Use when reviewing pull requests or diffs across the Python API, native bindings, packaging, tests, CI, or documentation. Apply the repository's existing architecture, correctness, compatibility, testing, and credential-handling expectations without replacing normal Copilot review." |
| 4 | +--- |
| 5 | + |
| 6 | +# Repository code review |
| 7 | + |
| 8 | +The sole source of repository-specific rules for this skill is |
| 9 | +[copilot-instructions.md](../../copilot-instructions.md). |
| 10 | +Read that file before reviewing. The checklist below organizes its guidance |
| 11 | +for review; it does not introduce policies beyond the repository instructions or a performance-only review methodology. |
| 12 | +review methodology. If this summary drifts, use the source instructions. |
| 13 | + |
| 14 | +## Apply the instructions to the changed area |
| 15 | + |
| 16 | +1. **Architecture:** Identify the affected layer before judging the change: |
| 17 | + Python API, extension loader, C++/ODBC bindings, or Rust-backed bulk copy. |
| 18 | + Bulk copy uses `mssql_py_core` and TDS rather than the ODBC path. |
| 19 | +2. **Python API:** Preserve DB API 2.0 semantics, specific exception handling, |
| 20 | + and connection/cursor context-manager behavior. For public API changes, |
| 21 | + check that `__all__` and the `mssql_python.pyi` stubs stay consistent. |
| 22 | +3. **Native safety:** Inspect Python-reference shutdown ordering, initialization |
| 23 | + failures, and error translation. A failed initialization must not expose a |
| 24 | + half-built object. For hot paths, follow the source's raw-CPython guidance |
| 25 | + with correct refcounts and error checks. |
| 26 | +4. **Platforms and packaging:** Check all affected shipped architectures, not |
| 27 | + just the build host. In particular, universal2 dylib/rpath changes must cover |
| 28 | + arm64 and x86_64. Review wheel/platform tagging where it changes, and do not |
| 29 | + hand-edit bundled ODBC binaries. |
| 30 | +5. **Tests:** Check that fixes have regression coverage. Assert promised |
| 31 | + operation counts as well as returned values when the change claims fewer |
| 32 | + calls. Global type-mapping changes need the typed-NULL cases identified in |
| 33 | + the source instructions. Keep crash-prone and global-state cases in isolated |
| 34 | + subprocesses. |
| 35 | +6. **Credentials and examples:** Reject committed real credentials. Connection |
| 36 | + examples with `UID`/`PWD` use localhost and dummy values. Do not add `Driver=`; |
| 37 | + the bundled driver is selected automatically. Treat |
| 38 | + `TrustServerCertificate=yes` as local-development only. |
| 39 | +7. **Scope and contribution requirements:** Keep changes surgical and avoid |
| 40 | + unrelated edits, build artifacts, or virtual environments. Apply the source |
| 41 | + instructions' title-prefix, issue-reference, and summary requirements. |
| 42 | +8. **Evidence and context:** Understand the linked issue and existing review |
| 43 | + threads. Reproduce before asserting a driver bug or fix. Follow the source's |
| 44 | + no-duplicate-PR and no-unsolicited-comment rules; a review is not an instruction |
| 45 | + to create or publish changes. |
| 46 | + |
| 47 | +## Validation guidance |
| 48 | + |
| 49 | +Use the setup, native-build, test, and PR guides identified in |
| 50 | +[Development workflow](../../copilot-instructions.md#development-workflow). |
| 51 | +Build the native extension before running Python tests. Most tests need a live |
| 52 | +SQL Server through `DB_CONNECTION_STRING`; the dependency checks do not. |
| 53 | + |
| 54 | +Consult the actual pipeline matrix for supported combinations rather than |
| 55 | +inferring cross-platform coverage from a local run. Preserve the distinction |
| 56 | +between blocking checks and informational tools documented in |
| 57 | +[Validation gate](../../copilot-instructions.md#validation-gate-run-before-you-finish--this-mirrors-ci). |
| 58 | +A formatting recommendation from an informational tool is not automatically |
| 59 | +a failing merge requirement. |
| 60 | + |
| 61 | +Apply only the relevant repository checks alongside normal Copilot review. |
| 62 | +Do not force every change into a native-code or performance investigation. |
0 commit comments