diff --git a/docs/README.skills.md b/docs/README.skills.md index 24473fa6f4..d979f57cbd 100644 --- a/docs/README.skills.md +++ b/docs/README.skills.md @@ -330,6 +330,7 @@ See [CONTRIBUTING.md](../CONTRIBUTING.md#adding-skills) for guidelines on how to | [premium-frontend-ui](../skills/premium-frontend-ui/SKILL.md)
`gh skills install github/awesome-copilot premium-frontend-ui` | A comprehensive guide for GitHub Copilot to craft immersive, high-performance web experiences with advanced motion, typography, and architectural craftsmanship. | None | | [project-workflow-analysis-blueprint-generator](../skills/project-workflow-analysis-blueprint-generator/SKILL.md)
`gh skills install github/awesome-copilot project-workflow-analysis-blueprint-generator` | Comprehensive technology-agnostic prompt generator for documenting end-to-end application workflows. Automatically detects project architecture patterns, technology stacks, and data flow patterns to generate detailed implementation blueprints covering entry points, service layers, data access, error handling, and testing approaches across multiple technologies including .NET, Java/Spring, React, and microservices architectures. | None | | [prompt-optimizer](../skills/prompt-optimizer/SKILL.md)
`gh skills install github/awesome-copilot prompt-optimizer` | Turn any rough prompt, half-formed idea, or task description into a finished, ready-to-send prompt optimized for any LLM model inside a chat interface — NOT the API. Use this skill whenever the user wants to write, rewrite, optimize, improve, sharpen, or polish a prompt for chat. Trigger phrases include "rewrite this prompt", "make this a better prompt", "optimize this prompt", "turn this into a prompt", "help me prompt this", "draft a prompt that...", "I want to ask...", or whenever the user pastes a draft prompt and asks for improvements. Also trigger when the user describes a task they plan to send to an LLM model and clearly wants a reusable, well-structured prompt rather than a direct answer. The output is always a single, copy-pasteable prompt in a code block that the user sends as-is — never a template with placeholders. | None | +| [protobuf-grpc-api-review](../skills/protobuf-grpc-api-review/SKILL.md)
`gh skills install github/awesome-copilot protobuf-grpc-api-review` | Review Protocol Buffer (.proto) and gRPC API changes for wire and JSON compatibility, safe schema evolution, rollout hazards, and RPC contract quality. Use when reviewing proto diffs, adding or changing messages and services, planning migrations, or diagnosing cross-version failures. | `references/grpc-contract-review.md`
`references/protobuf-compatibility.md` | | [publish-to-pages](../skills/publish-to-pages/SKILL.md)
`gh skills install github/awesome-copilot publish-to-pages` | Publish presentations and web content to GitHub Pages. Converts PPTX, PDF, HTML, or Google Slides to a live GitHub Pages URL. Handles repo creation, file conversion, Pages enablement, and returns the live URL. Use when the user wants to publish, deploy, or share a presentation or HTML file via GitHub Pages. | `scripts/convert-pdf.py`
`scripts/convert-pptx.py`
`scripts/publish.sh` | | [pytest-coverage](../skills/pytest-coverage/SKILL.md)
`gh skills install github/awesome-copilot pytest-coverage` | Run pytest tests with coverage, discover lines missing coverage, and increase coverage to 100%. | None | | [python-azure-iot-edge-modules](../skills/python-azure-iot-edge-modules/SKILL.md)
`gh skills install github/awesome-copilot python-azure-iot-edge-modules` | Build and operate Python Azure IoT Edge modules with robust messaging, deployment manifests, observability, and production readiness checks. | `references/python-edge-module-template.md`
`references/python-official-best-practices.md` | diff --git a/skills/protobuf-grpc-api-review/SKILL.md b/skills/protobuf-grpc-api-review/SKILL.md new file mode 100644 index 0000000000..f526b4d415 --- /dev/null +++ b/skills/protobuf-grpc-api-review/SKILL.md @@ -0,0 +1,118 @@ +--- +name: protobuf-grpc-api-review +description: 'Review Protocol Buffer (.proto) and gRPC API changes for wire and JSON compatibility, safe schema evolution, rollout hazards, and RPC contract quality. Use when reviewing proto diffs, adding or changing messages and services, planning migrations, or diagnosing cross-version failures.' +--- + +# Protobuf and gRPC API Review + +Review `.proto` and related gRPC changes as long-lived contracts. Distinguish what the wire format permits from what generated clients, JSON users, stored data, and mixed-version deployments can safely tolerate. + +## Start With the Compatibility Envelope + +Before deciding whether a change is safe, determine: + +- the old and new schema, not only the final file +- whether payloads use binary protobuf, ProtoJSON, text format, or more than one encoding +- whether messages persist in databases, queues, logs, caches, or events +- whether clients exist outside the repository or release independently +- the protobuf syntax or edition and the generated languages/runtime versions +- whether HTTP/JSON transcoding, reflection, service config, or schema registries expose the contract +- the deployment order, rollback window, and duration of mixed-version operation + +If context is missing, state the assumption and lower confidence. Do not call a change backward-compatible from the new schema alone. + +## Review Workflow + +### 1. Inventory Contract Changes + +Compare the old and new definitions by fully qualified symbol. Record: + +- message fields: number, name, type, cardinality, presence, `oneof`, defaults, and relevant options +- enums: value name, number, aliases, reservations, and zero value +- services: package, service, method, request and response types, and streaming mode +- generated API inputs: package options, outer class names, namespaces, and custom options + +Ignore formatting-only changes after confirming they do not alter descriptors or generated APIs. + +### 2. Evaluate Four Compatibility Dimensions + +Assess each affected symbol independently: + +1. **Binary wire** — can old and new readers parse both old and new bytes without corruption or loss? +2. **Named formats** — do ProtoJSON, text-format, REST-transcoded, or name-based consumers still work? +3. **Source and generated API** — will regenerated clients compile and preserve presence, enum, and accessor behavior? +4. **Behavior and operations** — do status codes, retry safety, deadlines, authorization, streaming, and resource bounds preserve the RPC contract? + +Read [protobuf compatibility rules](references/protobuf-compatibility.md) for field, enum, presence, `oneof`, and serialization changes. Read [gRPC contract review](references/grpc-contract-review.md) when services, methods, or runtime behavior change. + +### 3. Trace Mixed-Version Scenarios + +For every non-trivial change, reason through these paths: + +- old writer -> new reader +- new writer -> old reader +- old reader modifies and reserializes a new message +- rollback after new writers have emitted new values +- persisted old data read after the migration + +For conditionally compatible changes, identify the exact writer constraint and the point at which it may be relaxed. A safe rollout commonly requires deploying readers before writers and retaining the old field or method until rollback is no longer needed. + +### 4. Review Repository Evidence + +Use the repository's own tooling when available: + +- compile descriptors with the project's `protoc`, Buf, Gradle, Maven, Bazel, or language-specific task +- run configured breaking-change or lint checks +- inspect generated-code diffs only when they are committed by repository convention +- search call sites for exhaustive enum switches, presence assumptions, JSON field names, method paths, status handling, and retry configuration +- look for compatibility fixtures or descriptor baselines before proposing a new mechanism + +Do not claim a check passed unless you ran it. If a required tool or baseline is unavailable, name the unverified risk. + +### 5. Produce an Actionable Review + +Lead with one verdict: + +- **Compatible** — safe within the stated compatibility envelope +- **Rollout-dependent** — parseable, but safe only with explicit sequencing or value constraints +- **Breaking** — causes wire, named-format, source, or behavioral incompatibility +- **Insufficient context** — the old schema, encoding, consumers, or deployment model is unknown + +Then provide only evidence-backed findings. For each finding include: + +```text +[severity] Short title +Location: file and symbol or changed lines +Dimension: binary | JSON/text | source | behavior/operations +Change: old contract -> new contract +Impact: concrete failing mixed-version scenario +Remediation: smallest safe schema change or staged migration +``` + +Use **blocker** for corruption, unparsable data, tag reuse, or an unavoidable production break; **high** for likely cross-version data loss or unsafe RPC behavior; **medium** for bounded compatibility or operability risks; and **low** for maintainability issues that do not break the contract. Do not inflate style preferences into compatibility findings. + +Conclude with: + +- a compact compatibility matrix for changed symbols +- the rollout/rollback sequence if migration is required +- targeted tests that would prove the remaining assumptions + +## Default Safety Principles + +- Never reuse a field or enum number, even after deletion; reserve deleted numbers and usually names. +- Treat field-number changes and moving fields into an existing `oneof` as breaking. +- Treat type and cardinality changes as migrations, even when their binary wire types are compatible. +- Remember that adding a field or enum value can still break generated code or exhaustive consumers. +- Review ProtoJSON separately: names and unknown-field behavior make its compatibility envelope narrower than binary protobuf. +- Preserve unknown fields through read-modify-write paths when forward compatibility depends on them. +- Prefer additive evolution: add a new field or RPC, migrate readers and writers, deprecate the old contract, then remove it only after the compatibility window closes. +- Never recommend retries for a state-changing RPC without establishing idempotency or a deduplication mechanism. +- Require realistic client deadlines and cancellation-aware server work for production RPCs. + +## Avoid False Positives + +- Do not require every service to use streaming, retries, health checks, or HTTP transcoding. +- Do not flag a new optional field as breaking solely because old clients ignore it. +- Do not call a wire-compatible type change safe without checking value ranges and rollout order. +- Do not assume a renamed field is harmless when JSON, text format, reflection, or generated source APIs are consumers. +- Do not demand reservations for fields that never shipped; ask for release history when that distinction matters. diff --git a/skills/protobuf-grpc-api-review/references/grpc-contract-review.md b/skills/protobuf-grpc-api-review/references/grpc-contract-review.md new file mode 100644 index 0000000000..8422d93ffc --- /dev/null +++ b/skills/protobuf-grpc-api-review/references/grpc-contract-review.md @@ -0,0 +1,104 @@ +# gRPC Contract Review + +Use this reference when a change affects a service, method, request/response behavior, or client/server policy. Apply only the checks relevant to the change. + +## Method Identity and Shape + +The fully qualified package, service, and method form the RPC identity. Renaming or moving any of them changes the method path and breaks existing clients, routing, authorization policies, observability, and service configuration. + +Treat these as breaking contract changes: + +- removing or renaming a service or method +- changing request or response message type +- changing unary, client-streaming, server-streaming, or bidirectional-streaming mode +- moving a service to another protobuf package + +Prefer adding a new method, migrating callers, deprecating the old method, and removing it only after the compatibility window closes. Review request and response message changes with the protobuf compatibility rules. + +## Deadlines and Cancellation + +- Clients should set a realistic deadline; gRPC does not set one by default. +- The deadline must cover expected network and processing latency and be validated against production or load-test evidence. +- Servers must stop spawned work when the call is cancelled or its deadline expires. +- Downstream calls should receive the remaining deadline rather than starting a fresh full timeout. Confirm whether propagation is automatic or must be enabled in the implementation language. +- Do not convert a deadline or cancellation into a misleading application status. + +Flag new unbounded calls, swallowed cancellation, or downstream work that can outlive the originating request when they create resource or correctness risk. + +## Retries and Idempotency + +Before recommending or approving a retry policy, answer: + +1. Can the operation be repeated without duplicating a state change? +2. If the result is ambiguous, is there an idempotency key or server-side deduplication window? +3. Which status codes are retryable for this method? +4. Are attempts bounded by max attempts, exponential backoff, jitter, throttling, and the overall deadline? +5. Are retry attempts and final call outcomes observable? + +Retries are enabled in gRPC implementations, but there is no default general retry policy; transparent retries can still occur in limited cases. A response header commits the RPC and ends gRPC retry attempts. Never treat a mutating RPC as retry-safe merely because it returns `UNAVAILABLE` or `DEADLINE_EXCEEDED`: the server may already have applied the change. + +## Status Codes Are Part of the API + +Review status changes as observable behavior. Prefer the most specific stable code and keep error details free of secrets. + +- `INVALID_ARGUMENT`: invalid regardless of system state +- `FAILED_PRECONDITION`: retry only after state is explicitly fixed +- `ABORTED`: retry the larger transaction or read-modify-write sequence +- `UNAVAILABLE`: transient failure where retrying this call may be appropriate +- `RESOURCE_EXHAUSTED`: quota or capacity exhausted +- `UNAUTHENTICATED`: credentials are missing or invalid +- `PERMISSION_DENIED`: authenticated caller lacks permission +- `NOT_FOUND`: resource does not exist, or deliberate existence hiding is part of the authorization contract + +Avoid replacing domain failures with `UNKNOWN` or `INTERNAL`. Check client logic, retry policies, metrics, and alerts before changing an established code. + +## Streaming Contracts + +For streaming methods, establish and test: + +- message ordering and whether duplicates are possible +- half-close and completion semantics +- backpressure and bounded buffering +- per-message and total stream size limits +- cancellation and cleanup on both peers +- authentication lifetime for long-lived streams +- resume, replay, or checkpoint behavior after interruption + +Do not add streaming as a default improvement; require a concrete need and an explicit lifecycle contract. + +## Security and Resource Boundaries + +- Enforce authentication and authorization per method; update policy maps that enumerate paths whenever a method is added. +- Use transport security appropriate to the deployment, and avoid logging credentials or sensitive metadata. +- Bound request size, response size, concurrency, fan-out, and expensive repeated fields where input is untrusted. +- Validate before starting irreversible or costly work. +- Check that reflection, health services, and debug endpoints are exposed only as intended. + +## Behavioral Evolution + +Schema compatibility does not protect semantic contracts. Review changes to: + +- required business fields represented as optional protobuf fields +- pagination tokens, ordering, filters, and consistency guarantees +- idempotency and deduplication keys +- partial success and batch error semantics +- default limits, quotas, and server-side timeouts +- error details and redaction + +For a behavioral change, require documentation and tests that cover old clients during rollout and rollback. + +## Targeted Verification + +- invoke old generated clients against the new server and new clients against the old server when mixed versions are supported +- test the exact method path through proxies, gateways, and authorization middleware +- exercise deadlines, cancellation, and downstream propagation +- inject retryable and non-retryable failures, including an ambiguous failure after a state change +- test streaming cancellation, slow consumers, large messages, and interrupted streams where applicable +- verify metrics distinguish attempts from logical calls + +## Primary References + +- [gRPC deadlines](https://grpc.io/docs/guides/deadlines/) +- [gRPC retry](https://grpc.io/docs/guides/retry/) +- [gRPC status codes](https://grpc.io/docs/guides/status-codes/) +- [gRPC authentication](https://grpc.io/docs/guides/auth/) diff --git a/skills/protobuf-grpc-api-review/references/protobuf-compatibility.md b/skills/protobuf-grpc-api-review/references/protobuf-compatibility.md new file mode 100644 index 0000000000..69d2877815 --- /dev/null +++ b/skills/protobuf-grpc-api-review/references/protobuf-compatibility.md @@ -0,0 +1,106 @@ +# Protobuf Compatibility Rules + +Use this reference when a review changes messages, fields, enums, serialization, or generated APIs. Apply it to the actual encoding and runtimes in scope. + +## Classify the Change + +### Binary wire-unsafe + +Treat these as breaking unless all serialized data and every reader/writer can be migrated atomically: + +- changing an existing field number +- reusing a deleted or previously shipped field number +- reusing a deleted enum number +- moving fields into an existing `oneof` +- changing between types with different wire encodings, such as `string` and an integer + +Never rely on an apparently unused tag having no historical data. Old binaries, logs, queues, and rollback artifacts may still contain it. + +### Binary wire-safe, with other dimensions still to review + +- adding a field with a new number +- removing a field while permanently reserving its number; reserve its name when name reuse is also unsafe +- adding an enum value with a new number +- moving one explicit-presence field into a new `oneof` +- changing a single-field `oneof` to an explicit-presence field +- changing a field to an extension with the same number and type + +These can still break source code, business logic, JSON consumers, validation, or exhaustive enum handling. + +### Binary-compatible but rollout-dependent + +These pairs can parse the same wire representation but may change values or lose data. Prefer adding a new field. If a migration is unavoidable, keep writers within the old domain until every reader has upgraded and rollback is closed. + +| Change | Required constraint or risk | +| --- | --- | +| `int32`, `uint32`, `int64`, `uint64`, `bool` | Old readers may truncate or reinterpret values; boolean semantics are especially risky. | +| `sint32` <-> `sint64` | Values outside the narrower range change when read as `sint32`. These are not compatible with ordinary integer encodings. | +| `fixed32` <-> `sfixed32`; `fixed64` <-> `sfixed64` | Signedness changes application meaning. | +| `string` <-> `bytes` | Bytes must remain valid UTF-8 for `string` readers. Generated APIs change. | +| embedded message <-> `bytes` | Bytes must always contain that message encoding; generated APIs change. | +| enum <-> integer types | Unknown-enum representation and generated behavior vary by language. | +| singular <-> repeated for strings, bytes, or messages | A singular reader keeps the last primitive value or merges messages; information can be lost. | +| map <-> its repeated entry message | Map readers may reorder entries and discard duplicate keys. | + +Numeric repeated fields are normally packed and are not safely interchangeable with singular fields. Treat any cardinality change as a data migration rather than a cleanup. + +## Deletion and Replacement Pattern + +For a shipped field that must change type or meaning: + +1. Add a new field with a new number and a distinct name. +2. Deploy readers that understand both fields. +3. Deploy writers that populate the new field, and dual-write if rollback requires it. +4. Backfill persisted data where needed. +5. Stop reading and writing the old field only after old binaries and rollback are gone. +6. Remove the old field and reserve its number and name. + +Do not reserve the old number while the field is still declared. Do not reuse the old number for the replacement. + +## Presence, Defaults, and `oneof` + +- Changing implicit scalar presence to explicit `optional` presence is wire-compatible but changes generated APIs and the meaning of default values. Check merge, patch, equality, and serialization behavior. +- An implicit-presence scalar cannot distinguish "unset" from its default value. If zero, empty, or false is meaningful in an update API, use explicit presence or a field mask. +- Adding a field to a `oneof` is generally parseable, but old clients can clear an unknown member when they set a known member and reserialize. Review read-modify-write paths. +- Removing a `oneof` member makes it impossible for a new reader to know which removed member had been set. Preserve the number and plan the migration. +- Never add a required field. Older writers cannot populate it, and partial rollout or rollback becomes unsafe. +- Do not change explicit defaults in proto2; version-skewed readers can interpret the same absent field differently. + +## Enums + +- Keep a zero-valued `*_UNSPECIFIED` first value unless the established contract deliberately uses another zero value. +- Adding a value is binary-safe but may break exhaustive switches or older business logic. Search generated-language consumers. +- When deleting a value, reserve both its number and, where names are part of the contract, its name. +- For aliases, add the new alias after the old one. A safe rename requires staged parser and serializer rollout before removing the old name. + +## ProtoJSON and Other Named Formats + +Review name-based formats separately from the binary wire format: + +- field and enum names can be serialized contract data, so renaming may break readers +- ProtoJSON does not preserve unknown fields, narrowing forward compatibility +- adding fields or enum values can fail older parsers that reject unknown names +- changing `json_name`, integer/string representation, bytes encoding, or `oneof` shape can alter the public JSON schema +- HTTP transcoding can expose message fields and enum names even when the internal transport is binary gRPC + +Ask whether stored JSON exists. If it does, include historical payloads in compatibility tests. + +## Verification Matrix + +Use real generated runtimes where behavior can vary by language: + +| Writer | Reader | What to assert | +| --- | --- | --- | +| old | new | Values, presence, enum handling, and validation remain correct. | +| new | old | Unknown data is tolerated and known values are not reinterpreted. | +| old | new -> modify -> serialize -> old | Unknown fields survive and known fields are not cleared. | +| new | old -> modify -> serialize -> new | New fields or `oneof` members are not silently lost. | + +Also compile descriptors, run the repository's breaking-change checker, test boundary values for widened/narrowed numeric domains, and round-trip any supported JSON representation. + +## Primary References + +- [Proto3: Updating a Message Type](https://protobuf.dev/programming-guides/proto3/#updating) +- [Proto Best Practices](https://protobuf.dev/best-practices/dos-donts/) +- [ProtoJSON wire safety](https://protobuf.dev/programming-guides/json/#json-wire-safety) +- [Field presence](https://protobuf.dev/programming-guides/field_presence/)