From 71232d20294abe1342f64a97092cc90b7a8434f3 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 20:22:02 -0400 Subject: [PATCH 1/5] feat(trogonstack-protobuf): keep protobuf contracts readable and evolvable without the schema Signed-off-by: Yordis Prieto --- .claude-plugin/marketplace.json | 6 + .cursor-plugin/marketplace.json | 5 + .github/release-please-config.json | 3 + .github/release-please-manifest.json | 3 +- .../.claude-plugin/plugin.json | 9 ++ .../.cursor-plugin/plugin.json | 25 +++ plugins/trogonstack-protobuf/README.md | 7 + plugins/trogonstack-protobuf/plugin.json | 19 +++ .../skills/protobuf-design-messages/SKILL.md | 65 ++++++++ .../references/enums-and-presets.md | 89 +++++++++++ .../references/event-sourced-contracts.md | 112 +++++++++++++ .../references/example-project-schema.md | 149 ++++++++++++++++++ .../references/json-field-naming.md | 78 +++++++++ .../references/naming-collisions.md | 35 ++++ .../references/presence-and-validation.md | 61 +++++++ .../references/review-checklist.md | 47 ++++++ .../references/value-objects.md | 138 ++++++++++++++++ .../skills/protobuf-evolve-schemas/SKILL.md | 59 +++++++ .../references/ci-and-governance.md | 54 +++++++ .../references/compatibility-surfaces.md | 38 +++++ .../references/example-schema-evolution.md | 98 ++++++++++++ .../references/package-versioning.md | 50 ++++++ .../references/renames-and-reserved.md | 76 +++++++++ .../references/review-checklist.md | 28 ++++ 24 files changed, 1253 insertions(+), 1 deletion(-) create mode 100644 plugins/trogonstack-protobuf/.claude-plugin/plugin.json create mode 100644 plugins/trogonstack-protobuf/.cursor-plugin/plugin.json create mode 100644 plugins/trogonstack-protobuf/README.md create mode 100644 plugins/trogonstack-protobuf/plugin.json create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/event-sourced-contracts.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/json-field-naming.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/presence-and-validation.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/value-objects.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/package-versioning.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index a032986..541d5c1 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -66,6 +66,12 @@ "description": "Elixir skills for designing and reviewing OTP processes such as GenServers", "source": "./plugins/trogonstack-elixir", "category": "development" + }, + { + "name": "trogonstack-protobuf", + "description": "Protobuf skills for designing messages and fields and for evolving schemas without breaking consumers", + "source": "./plugins/trogonstack-protobuf", + "category": "development" } ] } diff --git a/.cursor-plugin/marketplace.json b/.cursor-plugin/marketplace.json index 9747c8c..c9a5026 100644 --- a/.cursor-plugin/marketplace.json +++ b/.cursor-plugin/marketplace.json @@ -56,6 +56,11 @@ "name": "trogonstack-elixir", "source": "plugins/trogonstack-elixir", "description": "Elixir skills for designing and reviewing OTP processes such as GenServers" + }, + { + "name": "trogonstack-protobuf", + "source": "plugins/trogonstack-protobuf", + "description": "Protobuf skills for designing messages and fields and for evolving schemas without breaking consumers" } ] } diff --git a/.github/release-please-config.json b/.github/release-please-config.json index c390924..9a292eb 100644 --- a/.github/release-please-config.json +++ b/.github/release-please-config.json @@ -63,6 +63,9 @@ }, "plugins/trogonstack-elixir": { "component": "trogonstack-elixir" + }, + "plugins/trogonstack-protobuf": { + "component": "trogonstack-protobuf" } }, "plugins": [ diff --git a/.github/release-please-manifest.json b/.github/release-please-manifest.json index ac2e551..c4724fc 100644 --- a/.github/release-please-manifest.json +++ b/.github/release-please-manifest.json @@ -8,5 +8,6 @@ "plugins/trogonstack-eventmodeling": "0.1.2", "plugins/trogonstack-eda": "0.1.2", "plugins/trogonstack-product-requirements": "0.1.1", - "plugins/trogonstack-elixir": "0.1.0" + "plugins/trogonstack-elixir": "0.1.0", + "plugins/trogonstack-protobuf": "0.0.1" } diff --git a/plugins/trogonstack-protobuf/.claude-plugin/plugin.json b/plugins/trogonstack-protobuf/.claude-plugin/plugin.json new file mode 100644 index 0000000..7872076 --- /dev/null +++ b/plugins/trogonstack-protobuf/.claude-plugin/plugin.json @@ -0,0 +1,9 @@ +{ + "name": "trogonstack-protobuf", + "description": "Protobuf skills for designing messages and fields and for evolving schemas without breaking consumers", + "version": "0.0.1", + "author": { + "name": "TrogonStack", + "url": "https://github.com/TrogonStack" + } +} diff --git a/plugins/trogonstack-protobuf/.cursor-plugin/plugin.json b/plugins/trogonstack-protobuf/.cursor-plugin/plugin.json new file mode 100644 index 0000000..7de6259 --- /dev/null +++ b/plugins/trogonstack-protobuf/.cursor-plugin/plugin.json @@ -0,0 +1,25 @@ +{ + "name": "trogonstack-protobuf", + "displayName": "TrogonStack Protobuf", + "description": "Protobuf skills for designing messages and fields and for evolving schemas without breaking consumers", + "version": "0.0.1", + "author": { + "name": "TrogonStack" + }, + "homepage": "https://github.com/TrogonStack/agentskills/tree/main/plugins/trogonstack-protobuf", + "repository": "https://github.com/TrogonStack/agentskills", + "license": "MIT", + "keywords": [ + "trogonstack", + "protobuf", + "buf", + "schema-evolution" + ], + "category": "developer-tools", + "tags": [ + "protobuf", + "buf", + "schema-evolution" + ], + "skills": "./skills/" +} diff --git a/plugins/trogonstack-protobuf/README.md b/plugins/trogonstack-protobuf/README.md new file mode 100644 index 0000000..564b6c7 --- /dev/null +++ b/plugins/trogonstack-protobuf/README.md @@ -0,0 +1,7 @@ +# trogonstack-protobuf + +Protobuf skills for designing messages and fields and for evolving schemas without breaking consumers. + +```bash +claude plugin install trogonstack-protobuf@trogonstack +``` diff --git a/plugins/trogonstack-protobuf/plugin.json b/plugins/trogonstack-protobuf/plugin.json new file mode 100644 index 0000000..89d8c29 --- /dev/null +++ b/plugins/trogonstack-protobuf/plugin.json @@ -0,0 +1,19 @@ +{ + "$schema": "https://agent-plugins.org/schemas/1.0.0/plugin.schema.json", + "name": "trogonstack-protobuf", + "version": "0.0.1", + "description": "Protobuf skills for designing messages and fields and for evolving schemas without breaking consumers", + "author": { + "name": "TrogonStack", + "url": "https://github.com/TrogonStack" + }, + "homepage": "https://github.com/TrogonStack/agentskills/tree/main/plugins/trogonstack-protobuf", + "repository": "https://github.com/TrogonStack/agentskills", + "license": "MIT", + "keywords": [ + "trogonstack", + "protobuf", + "buf", + "schema-evolution" + ] +} diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md new file mode 100644 index 0000000..86ca1dc --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md @@ -0,0 +1,65 @@ +--- +name: protobuf-design-messages +description: "Design or review protobuf message and field definitions: field naming for JSON consumers, identifier and reference value objects, enums, presence, and validation. Use when writing a new .proto message, enum, or field, or reviewing one for naming and shape. Do not use for schema evolution, breaking-change detection, or buf lint/breaking CI setup (use protobuf-evolve-schemas)." +allowed-tools: + - AskUserQuestion + - Read + - Write + - Shell +--- + +# Design or Review Protobuf Messages and Fields + +Shape messages and fields so the schema itself carries the meaning: what a field points at, what a zero value does, and what the schema can and cannot guarantee. + +## Core Principle + +A `.proto` file is read by more than the compiler. ProtoJSON consumers see only field names, never types; reviewers see only names and comments, never runtime behavior; everyone who did not write the schema sees only what it says. A field, message, or enum that relies on tribal knowledge to interpret correctly is a defect, not a style nit. + +## Principles + +Apply them in order. A design that fails an earlier principle makes the later ones moot. Each one names the references that show it in practice; read them when the situation matches, not up front. + +### 1. Names Must Survive Schema-less JSON Readers + +ProtoJSON keys default to the lowerCamelCase of the field name (`json_name` overrides it, and parsers accept both forms). Many consumers see only the JSON, never the `.proto`, so the field name is the only signal they get. A field whose value points at something stored or owned elsewhere keeps a suffix in its name even when its type already says so. + +Read [references/json-field-naming.md](references/json-field-naming.md) when naming a field that is an identifier, a structured reference, or a hierarchy position, or when reviewing a schema for names that read as inline content but are not. + +### 2. No Primitive Obsession + +Identifiers are single-field wrapper messages, not a bare `string` whose only clue is the field name. References are value objects. Bytes stored out of line pair a storage locator with the digest it is verified against, in one message. Units live in names, and money and instants use the field name patterns the rest of the ecosystem already uses. + +Read [references/value-objects.md](references/value-objects.md) when defining an identifier, a reference to another resource, a pointer to out-of-line content, a quantity, a monetary amount, or an instant in time. + +### 3. Enums Encode an Exhaustive Choice, Not a Growable Boolean + +The zero value is always `_UNSPECIFIED`, and code must reject it rather than resolve it to a default. An omitted choice must never silently widen behavior. Prefer an enum, or a `oneof`, over a boolean that is one feature request away from needing a third state. + +Read [references/enums-and-presets.md](references/enums-and-presets.md) when defining an enum, naming its values, or designing a preset/option field such as an access or visibility level. + +### 4. Know What the Schema Can and Cannot Guarantee + +Editions presence (`EXPLICIT`, `IMPLICIT`, `LEGACY_REQUIRED`) and `protovalidate` constraints are schema-level tools, and each has a limit: `LEGACY_REQUIRED` makes removing the field later a breaking change; `buf.validate` annotations are documentation unless something in the stack actually runs protovalidate. Some invariants cannot be checked from the schema at all because they need live state. + +Read [references/presence-and-validation.md](references/presence-and-validation.md) when deciding whether a field should be required, writing a `buf.validate` constraint, or distinguishing a schema-checkable invariant from one that belongs in the service. + +### 5. Event-Sourced Contracts Separate Commands, Events, State, and Faults + +When the package models a decider (command handler plus event-sourced aggregate), commands are imperative and events are past tense. Facts that exist only at creation time belong on the creation event, even when another system owns everything that happens afterward. An aggregate does not get an event for a fact it does not own. + +Read [references/event-sourced-contracts.md](references/event-sourced-contracts.md) when designing commands, events, state, or faults for an event-sourced resource. Skip this reference entirely for plain CRUD-shaped or RPC request/response schemas; it does not apply to them. + +### 6. Rename Early When Words Collide With Product Vocabulary + +A name that is fine in isolation can collide with a term the product already uses for something else. The cheapest moment to fix a name is before anything generates code from it; a package still under design can be excluded from code generation so the churn costs nothing. + +Read [references/naming-collisions.md](references/naming-collisions.md) when a field, message, or package name echoes a term used elsewhere in the product for a different concept. + +## Worked Example + +Read [references/example-project-schema.md](references/example-project-schema.md) to see every principle applied in one complete `.proto` package: a `Project` resource with an identifier, a hierarchy position, an access preset enum, and the commands and events that create, rename, and archive it. + +## Reviewing an Existing Schema + +Read [references/review-checklist.md](references/review-checklist.md) when reviewing or auditing message and field definitions. Each item is a symptom; a failing item is a question about the principle behind it, answered in the linked reference. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md new file mode 100644 index 0000000..abe5b6a --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md @@ -0,0 +1,89 @@ +# Enums: Exhaustive Choice, Not a Growable Boolean + +Use this reference when defining an enum, naming its values, or designing a preset/option field such as an access or visibility level. + +## Zero Value Is Unspecified, and Unspecified Is a Rejection + +The zero value of every enum must end in `_UNSPECIFIED` (buf lint rule `ENUM_ZERO_VALUE_SUFFIX`). That value exists because every enum field that is never explicitly set reads as its zero value on the wire; naming it `_UNSPECIFIED` makes that silent default visible instead of letting it masquerade as a real choice. + +Enum value names are prefixed with the enum's name (buf lint rule `ENUM_VALUE_PREFIX`), which also keeps C++ scoping, where enum value names share their enclosing scope, from colliding across enums in the same file: + +```protobuf +// Avoid: unprefixed values collide with another enum's values in the same package, +// and the zero value reads as a real default. +enum Visibility { + PRIVATE = 0; + PUBLIC = 1; +} + +// Prefer: prefixed values, explicit unspecified zero value. +enum Visibility { + VISIBILITY_UNSPECIFIED = 0; + VISIBILITY_PRIVATE = 1; + VISIBILITY_PUBLIC = 2; +} +``` + +## An Omitted Choice Must Not Silently Widen Behavior + +`_UNSPECIFIED` is a value the wire format can produce without anyone intending it: an old client built before the field existed, a caller who forgot to set it, a bug. The service must reject that value rather than resolve it to a convenient default, especially when the convenient default is the permissive one. + +Consider an access preset on a resource: + +```protobuf +enum AccessPreset { + ACCESS_PRESET_UNSPECIFIED = 0; + ACCESS_PRESET_PRIVATE = 1; + ACCESS_PRESET_ORGANIZATION = 2; + ACCESS_PRESET_PUBLIC = 3; +} + +message CreateProject { + AccessPreset access_preset = 1; +} +``` + +If the service treats `ACCESS_PRESET_UNSPECIFIED` as `ACCESS_PRESET_PUBLIC` "so something reasonable happens," an old client, a client with a bug, or a caller who genuinely forgot the field just made the resource public by omission. The creator never chose that, and the resource the creator meant to keep private is now exposed. The only safe reading of `UNSPECIFIED` for a field like this is "reject the request and ask the caller to choose," never "pick the widest option on their behalf." + +Contrast that with a field where `UNSPECIFIED` defaulting to the narrowest, most conservative option would be safe. The service-level decision depends entirely on what gets exposed by resolving the default one way or the other; the schema's job is to make `UNSPECIFIED` a visible, named state so that decision has to be made on purpose rather than falling out of whatever the zero value happens to be. + +```protobuf +// Avoid: service code that treats UNSPECIFIED as a convenient permissive default. +switch (request.access_preset()) { + case AccessPreset::ACCESS_PRESET_UNSPECIFIED: + case AccessPreset::ACCESS_PRESET_PUBLIC: + return CreateWithPreset(AccessPreset::ACCESS_PRESET_PUBLIC); + // ... +} + +// Prefer: reject UNSPECIFIED explicitly; see references/presence-and-validation.md +// for enforcing this at the schema layer with protovalidate. +if (request.access_preset() == AccessPreset::ACCESS_PRESET_UNSPECIFIED) { + return InvalidArgumentError("access_preset is required"); +} +``` + +## Prefer an Enum or a `oneof` Over a Boolean That Will Grow + +A boolean field locks a concept into exactly two states forever; adding a third state means adding a second boolean and reconciling the combinations it creates. If there is any chance the concept has more than two states eventually, model it as an enum from the start: + +```protobuf +// Avoid: a boolean that is one feature request away from needing a third state. +bool is_archived = 1; + +// Prefer: an enum that can grow. +enum ProjectState { + PROJECT_STATE_UNSPECIFIED = 0; + PROJECT_STATE_ACTIVE = 1; + PROJECT_STATE_ARCHIVED = 2; +} +``` + +When two or more fields are mutually exclusive ways of saying the same thing (a result is either a success payload or a fault, never both), use `oneof` instead of a pair of optional fields plus a discriminator boolean; see [references/event-sourced-contracts.md](event-sourced-contracts.md) for a `oneof` used to model a fault. + +## Review Questions + +- Does every enum's zero value end in `_UNSPECIFIED`, and are all values prefixed with the enum name? +- Is there service code anywhere that treats an enum's `UNSPECIFIED` value as a usable default rather than rejecting it? +- Would resolving `UNSPECIFIED` to a default ever widen access, cost, or scope beyond what the caller explicitly chose? +- Is there a boolean field describing something that could plausibly need a third state? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/event-sourced-contracts.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/event-sourced-contracts.md new file mode 100644 index 0000000..290d88c --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/event-sourced-contracts.md @@ -0,0 +1,112 @@ +# Event-Sourced and Decider Contracts + +Use this reference when designing commands, events, state, or faults for an event-sourced resource (a decider: command in, events out, state folded from events). Skip it for plain CRUD-shaped or RPC request/response schemas; it does not apply to them. + +## Name by Tense + +Commands are imperative, because a command is a request that something happen and might be refused. Events are past tense, because an event is a fact that already happened and cannot be refused after the fact: + +```protobuf +// Commands: imperative, a request to act. +message CreateProject { ... } +message RenameProject { ... } +message ArchiveProject { ... } + +// Events: past tense, a fact that already happened. +message ProjectCreated { ... } +message ProjectRenamed { ... } +message ProjectArchived { ... } +``` + +## Record Creation-Time Facts on the Creation Event, Even When Another System Owns What Happens Later + +A resource's creation event is the one place certain facts can ever be recorded as "true at the moment this came into existence": who created it, where it was placed at birth, what access level it started with. Even if another system owns moves, renames, or access changes afterward, the creation event still owns the initial values, because nothing else will ever again be in a position to say what they were at creation time. + +```protobuf +message ProjectCreated { + ProjectId project_id = 1; + string name = 2; + UserId created_by = 3; + NodeId parent = 4; // where it was placed at birth + AccessPreset initial_access_preset = 5; // access choice at creation, even if access changes later move elsewhere + google.protobuf.Timestamp created_at = 6; +} +``` + +## Do Not Add an Event for a Fact the Aggregate Does Not Own + +If moves are owned by a hierarchy/placement service and access changes are owned by an access-control service, the project aggregate does not get a `ProjectMoved` or `ProjectAccessChanged` event just because those facts are interesting to the project. Emitting an event for a fact you do not own creates two systems of record for the same fact and invites them to disagree. + +```protobuf +// Avoid: the project aggregate does not own placement; emitting this event +// gives placement two sources of truth (the hierarchy service, and this event). +message ProjectMoved { + ProjectId project_id = 1; + NodeId new_parent = 2; +} + +// Prefer: the project aggregate only emits events for facts it owns. +// Consumers that need current placement read it from the hierarchy service, +// or from a read model built off the hierarchy service's own events. +``` + +The exception is the creation-time snapshot above: recording "where it was placed at birth" on `ProjectCreated` is not a claim about current placement, only about the fact of creation, which the aggregate does own. + +## State and Faults + +The folded state message is named after the resource, not the aggregate pattern: + +```protobuf +message Project { + ProjectId project_id = 1; + string name = 2; + AccessPreset access_preset = 3; + ProjectState state = 4; +} +``` + +Faults (the decider's rejection outcomes) are a `oneof`, not a generic error string, so a caller can match on the specific reason without parsing text: + +```protobuf +message RenameProjectFault { + oneof fault { + ProjectNotFound not_found = 1; + ProjectArchivedFault archived = 2; + InvalidName invalid_name = 3; + } +} + +message ProjectNotFound { + ProjectId project_id = 1; +} + +message ProjectArchivedFault { + ProjectId project_id = 1; +} + +message InvalidName { + string reason = 1; +} +``` + +## Comments Document Invariants, Not Mechanics + +Keep comments to the rule a reader cannot infer from the schema (an invariant, a non-obvious lifecycle rule), not a restatement of what the field declaration already says: + +```protobuf +// Avoid: restates the type. +// The project id. +ProjectId project_id = 1; + +// Prefer: states an invariant the schema cannot express. +// Immutable once set; renaming a project does not change this id. +ProjectId project_id = 1; +``` + +## Review Questions + +- Are commands named imperatively and events named in the past tense? +- Does the creation event carry every fact that is only knowable at creation time, even if later changes to those facts are owned elsewhere? +- Does any event represent a fact the aggregate does not own, duplicating another system's source of truth? +- Are faults modeled as a `oneof` of specific types rather than a generic error string or code? +- Do comments add information the schema cannot express, instead of restating field names? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md new file mode 100644 index 0000000..9685d1e --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md @@ -0,0 +1,149 @@ +# Worked Example: a Project Resource + +A complete, small `.proto` package for a generic `Project` resource, applying every principle in [SKILL.md](../SKILL.md): wrapped identifiers, a hierarchy position, a bare-vs-qualified parent distinction, an access preset enum that rejects its unspecified value, a money value object, and a decider's commands, events, state, and faults. + +```protobuf +edition = "2023"; + +package example.projects.v1alpha1; + +import "buf/validate/validate.proto"; +import "google/protobuf/timestamp.proto"; + +// --- Identifiers and references (principle 2: no primitive obsession) --- + +message ProjectId { + string value = 1 [(buf.validate.field).string.min_len = 1]; +} + +message UserId { + string value = 1 [(buf.validate.field).string.min_len = 1]; +} + +// A hierarchy position. Bare `parent` names a role (principle 1's exception): +// a position can point at a node of any kind, which is the point of the field. +message NodeId { + string value = 1 [(buf.validate.field).string.min_len = 1]; +} + +// --- Value objects (principle 2) --- + +message Money { + int64 amount_micros = 1; + string currency_code = 2 [(buf.validate.field).string.len = 3]; // ISO 4217 +} + +// --- Enums (principle 3: exhaustive choice, UNSPECIFIED is rejected) --- + +enum AccessPreset { + ACCESS_PRESET_UNSPECIFIED = 0; + ACCESS_PRESET_PRIVATE = 1; + ACCESS_PRESET_ORGANIZATION = 2; + ACCESS_PRESET_PUBLIC = 3; +} + +enum ProjectState { + PROJECT_STATE_UNSPECIFIED = 0; + PROJECT_STATE_ACTIVE = 1; + PROJECT_STATE_ARCHIVED = 2; +} + +// --- State (folded from events) --- + +message Project { + ProjectId project_id = 1; + string name = 2; + NodeId parent = 3; + AccessPreset access_preset = 4; + ProjectState state = 5; + Money budget = 6; +} + +// --- Commands: imperative (principle 5) --- + +message CreateProject { + string name = 1 [(buf.validate.field).string.min_len = 1]; + NodeId parent = 2; + AccessPreset access_preset = 3 [ + (buf.validate.field).enum.defined_only = true, + (buf.validate.field).enum = {not_in: [0]} + ]; + Money budget = 4; +} + +message RenameProject { + ProjectId project_id = 1; + string name = 2 [(buf.validate.field).string.min_len = 1]; +} + +message ArchiveProject { + ProjectId project_id = 1; +} + +// --- Events: past tense (principle 5) --- + +message ProjectCreated { + ProjectId project_id = 1; + string name = 2; + UserId created_by = 3; + // Placement and access at birth. Later moves and access changes are owned + // by other systems; this event only records the facts true at creation. + NodeId parent = 4; + AccessPreset initial_access_preset = 5; + Money initial_budget = 6; + google.protobuf.Timestamp created_at = 7; +} + +message ProjectRenamed { + ProjectId project_id = 1; + string name = 2; + google.protobuf.Timestamp renamed_at = 3; +} + +message ProjectArchived { + ProjectId project_id = 1; + google.protobuf.Timestamp archived_at = 2; +} + +// Deliberately absent: ProjectMoved, ProjectAccessChanged. Placement and +// access are owned elsewhere; this aggregate does not duplicate their +// source of truth with its own events for those facts. + +// --- Faults --- + +message ProjectNotFound { + ProjectId project_id = 1; +} + +message ProjectAlreadyArchived { + ProjectId project_id = 1; +} + +message InvalidProjectName { + string reason = 1; +} + +message RenameProjectFault { + oneof fault { + ProjectNotFound not_found = 1; + ProjectAlreadyArchived already_archived = 2; + InvalidProjectName invalid_name = 3; + } +} +``` + +## What Each Choice Demonstrates + +| Choice | Principle | +|---|---| +| `ProjectId`, `UserId`, `NodeId` as wrapper messages | No primitive obsession | +| `NodeId parent` bare, no `_id` suffix | `parent` names hierarchy position, never qualified | +| `Money` with `amount_micros` + `currency_code` | Units and currency travel together, never a bare float | +| `AccessPreset` zero value rejected via `buf.validate.field.enum` | An omitted choice cannot silently widen access | +| `CreateProject.name` marked `required` via `string.min_len = 1` | Schema-checkable invariant expressed portably | +| `ProjectCreated` carries `parent` and `initial_access_preset` | Creation-time facts recorded even though later changes are owned elsewhere | +| No `ProjectMoved` or `ProjectAccessChanged` event | The aggregate does not emit events for facts it does not own | +| `RenameProjectFault` as a `oneof` | Specific, matchable fault types instead of a generic error | +| `package example.projects.v1alpha1` | Pre-release versioning; see [protobuf-evolve-schemas](../../protobuf-evolve-schemas/SKILL.md) for when and how this graduates to `v1beta1` and `v1` | + +Note: `example.projects.v1alpha1` does not refer to any real service; it is a placeholder package name for this worked example. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/json-field-naming.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/json-field-naming.md new file mode 100644 index 0000000..5483987 --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/json-field-naming.md @@ -0,0 +1,78 @@ +# Field Names for Schema-less JSON Readers + +Use this reference when naming a field that is an identifier, a structured reference to another resource, or a hierarchy position, or when reviewing a schema for names that read as inline content but point elsewhere. + +## Why the Field Name Carries the Signal + +Protobuf's JSON mapping (ProtoJSON) converts a field name to lowerCamelCase for the JSON key by default; the `json_name` option overrides that, and a conformant parser accepts both the JSON name and the original proto field name on input. See [protobuf.dev: JSON Mapping](https://protobuf.dev/programming-guides/json/). + +The type system is invisible to anyone reading only the wire format. A gRPC-Gateway caller, a webhook consumer, a log line, or a frontend that deserializes into a generic object sees `{"project": {"value": "proj_123"}}` and has no way to tell, from that alone, whether `project` is an inline `Project` object or an opaque pointer to one stored elsewhere. The field's type answers that question in the `.proto` file; the field's name has to answer it everywhere else. + +## The Suffix Rules + +| Shape | Suffix | Example | +|---|---|---| +| Single opaque identifier | `_id` | `ProjectId project_id` -> JSON `projectId` | +| Structured reference (a typed pointer, possibly with more than one field) | `_ref` | `ArtifactRef result_ref`, `CheckoutRef checkout_ref` | +| Hierarchy position (the node a resource is attached to) | none; bare `parent` | `NodeId parent` | +| Kinship (a relation to another resource) | type name plus `_id`; never `parent_id` | `ProjectId project_id`, `SessionId parent_session_id` | + +A bare `result` or `project` field reads as inline content. Even though the type (`ArtifactRef`, `ProjectId`) already says it is a pointer, a reader who sees only the JSON key does not see the type, so the key must carry that signal on its own. + +```protobuf +// Avoid: the key `project` looks like an inline Project to anyone reading only JSON. +message Task { + ProjectId project = 1; +} + +// Prefer: the key `projectId` tells a schema-less reader this is a pointer, not content. +message Task { + ProjectId project_id = 1; +} +``` + +```protobuf +// Avoid: `result` could be inline content or a pointer; the name does not say. +message RunJobResponse { + ArtifactRef result = 1; +} + +// Prefer: the _ref suffix survives even without the schema. +message RunJobResponse { + ArtifactRef result_ref = 1; +} +``` + +## The Parent Exception + +Hierarchy position is the one case that stays bare. A `parent` field names a role, not a type, because the whole point of a position field is that it can point at a node of any kind. Qualifying it with a type would contradict that: + +```protobuf +// Avoid: compounds that do not improve on bare parent. +NodeId parent_id = 1; // redundant Id suffix on a role name +NodeId parent_node = 1; // the name repeats what the type already says + +// Prefer: bare parent for hierarchy position. +NodeId parent = 1; +``` + +Kinship between resources is a different relationship and is always qualified with the type, never left bare and never spelled `parent_id`: + +```protobuf +// Avoid: ambiguous with hierarchy position, and parent_id has no type. +string parent_id = 1; + +// Prefer: the type name says this is kinship, not position. +SessionId parent_session_id = 1; +``` + +When the identifier is a wrapper message rather than a bare `string`, the `_id` suffix stays on the field name even though the type is already `SessionId`. Consumers of the JSON form often have no schema and see only the key (`parentSessionId`); the key itself has to say the value points at a session. Position needs no such signal, because a parent is never inline content. + +This rule and its rationale come from the public straw-hat-team ADR on hierarchy naming: [ADR 6310044131, "Hierarchy position is referenced by a bare parent field"](https://github.com/straw-hat-team/adr/blob/main/src/adrs/6310044131/README.md). Its rule 5 was amended specifically to keep the `_id` suffix on typed kinship fields, for the JSON-reader reason above; read the full ADR for the cross-platform evidence (Google Cloud, AWS, Azure, Kubernetes) behind the bare `parent` convention. + +## Review Questions + +- Does any field name read as inline content (`project`, `result`, `owner`) while its type is an id or ref wrapper? Rename it with the matching suffix. +- Does any `_id` or `_ref` field have a type that is a bare `string` instead of a wrapper message? See [references/value-objects.md](value-objects.md). +- Is `parent` used for anything other than hierarchy position? Rename the kinship use to `parent__id`. +- Does any field spell kinship as `parent_id` or `parentId` with no type qualifier? That spelling is disallowed under this convention; it reads as a role with no type. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md new file mode 100644 index 0000000..254304a --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md @@ -0,0 +1,35 @@ +# Renaming Early When Words Collide + +Use this reference when a field, message, or package name echoes a term the product already uses for a different concept. + +## A Name That Is Fine Alone Can Still Collide + +A name can read perfectly well in isolation and still be the wrong name, because the product already uses that word for something else. A session's source checkout being called `workspace` is a reasonable name on its own, until the product also has a hierarchy concept called `Workspace`; at that point every reader has to disambiguate which `workspace` a given line means, and the schema itself cannot tell them. Renaming the checkout concept to `CheckoutRef` removes the ambiguity entirely instead of asking every future reader to resolve it from context. + +```protobuf +// Avoid: "workspace" already means something else in this product's hierarchy. +message StartSession { + string workspace = 1; +} + +// Prefer: a name with no collision anywhere else in the product's vocabulary. +message StartSession { + CheckoutRef checkout_ref = 1; +} +``` + +## Check New Names Against the Product's Existing Vocabulary, Not Just the Local File + +The collision rarely shows up by reading the file the new field lives in; it shows up by checking the new name against every other place the product already uses that word. Before naming a new message, field, or package, search the existing schemas (and the product's domain language more broadly) for the candidate name. A hit does not automatically rule the name out, but it does mean the two concepts need to be distinguishable at a glance, which a shared bare word cannot do. + +## The Cheapest Moment to Rename Is Before Anything Generates Code + +Once generated code exists, a rename is a breaking change (see [protobuf-evolve-schemas](../../protobuf-evolve-schemas/SKILL.md) for why a rename breaks JSON and generated source even when it is wire-compatible). Before that point, a rename costs nothing: it is a text edit in a file nobody depends on yet. + +This is one of the reasons a package still under active design is worth excluding from code generation entirely (for example, via `buf.gen.yaml` exclusions or simply not wiring the package into the build) until its shape has settled. While a package generates no code, every name in it can change for free; the moment it does, every rename has a cost that scales with how many consumers pulled in the generated bindings. + +## Review Questions + +- Does a new name reuse a word the product already assigns to a different concept elsewhere? +- Has the name been checked against the broader schema set, not just the file it lives in? +- Is the package still pre-codegen, meaning names in it can still be fixed for free? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/presence-and-validation.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/presence-and-validation.md new file mode 100644 index 0000000..15a25ab --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/presence-and-validation.md @@ -0,0 +1,61 @@ +# Presence and Validation + +Use this reference when deciding whether a field should be required, writing a `buf.validate` constraint, or distinguishing a schema-checkable invariant from one that belongs in the service. + +## Presence in Editions + +Protobuf Editions (2023, 2024) replace proto2's `required`/`optional` labels and proto3's implicit-presence-by-default with a per-field feature, `features.field_presence` (see [protobuf.dev: Editions Features](https://protobuf.dev/editions/features/)): + +| Value | Behavior | +|---|---| +| `EXPLICIT` | The field tracks whether it was explicitly set; an explicitly-set value serializes even if it equals the default. | +| `IMPLICIT` | No presence tracking; the default value never serializes, even if it was explicitly set to it. | +| `LEGACY_REQUIRED` | The field is required for parsing and serialization; this is proto2's `required` behavior carried into Editions for migration. | + +## `LEGACY_REQUIRED` Is proto2 `required`, and proto2 `required` Is Discouraged + +`LEGACY_REQUIRED` exists so proto2 files with `required` fields can convert to Editions syntax without changing wire behavior. It is not a recommended way to express a new invariant. Protobuf's own guidance on `required` fields states it plainly ([protobuf.dev: Dos and Don'ts](https://protobuf.dev/programming-guides/dos-donts/)): + +> "Required fields are considered harmful by so many they were removed from proto3 completely. Make all fields optional or repeated. You never know how long a message type is going to last and whether someone will be forced to fill in your required field with an empty string or zero in four years when it's no longer logically required but the proto still says it is." + +The mechanical reason is schema evolution: once a field is wire-required, removing it (or ceasing to populate it) breaks every consumer that still expects it. A field is never required for its entire service's lifetime; it is required until the day it is not, and `LEGACY_REQUIRED`/`required` makes that day a breaking change instead of a deprecation. + +**The trade-off, stated honestly:** some codebases use `LEGACY_REQUIRED` deliberately in pre-release packages (`v1alpha1`, `v1beta1`) specifically to document "this must be set" while the schema is still being designed and removal is cheap. That is a defensible, narrow use. It stops being defensible once the package stabilizes, because removing the field then becomes exactly the breaking change the guidance warns about. Prefer never reaching for it in a stable package. + +## `protovalidate` Is the Portable Way to Express Invariants + +[protovalidate](https://protovalidate.com) (the `buf.validate` annotations plus a runtime library for Go, JavaScript/TypeScript, Java, Python, and C++) expresses "must be set," range, and cross-field invariants as schema annotations that any language's validator enforces identically: + +```protobuf +import "buf/validate/validate.proto"; + +message CreateProject { + string name = 1 [(buf.validate.field).required = true]; + + AccessPreset access_preset = 2 [ + (buf.validate.field).enum = { defined_only: true, not_in: [0] } + ]; +} +``` + +`required` means something different depending on the field's presence: on a field with explicit presence, it checks that the field was set at all, even to its default value; on a field with implicit presence (plain proto3 scalars), it checks that the field is not the default value, since there is no other way to observe "was it set." `enum.defined_only` rejects any integer not defined on the enum; combining it with `not_in: [0]` (or `in` listing only the allowed non-zero values) is how to reject `_UNSPECIFIED` explicitly, enforcing principle 3 in [SKILL.md](../SKILL.md) at the schema layer. Message-level cross-field invariants are expressed with CEL expressions under `(buf.validate.message).cel`. + +**The caveat that matters:** `buf.validate` annotations are declarations, not enforcement. They do nothing on their own; a `buf.validate.field` constraint is only checked where something in the stack actually constructs and runs a protovalidate validator (typically as an interceptor in a gRPC/Connect server, or an explicit call in application code). A client that never wires up protovalidate can still construct and send a message that violates every constraint in the file. Treat `buf.validate` annotations as documentation of intent that becomes an enforced invariant only where the runtime is actually plugged in, and verify that it is plugged in on every path that accepts untrusted input, not just the one it was first added to. + +## Not Every Invariant Belongs in the Schema + +Some rules need live state the schema cannot see: "this parent must be a node of type workspace," "this user must have write access to the target project," "this slug must be unique." Those are admission-time rules, checked by the service against the current state of the system, not schema-checkable constraints. Trying to express them as `buf.validate` annotations either fails outright (CEL has no access to a database) or produces a constraint that lies about what it actually checked. + +| Checkable in the schema | Needs live state (belongs in the service) | +|---|---| +| A string matches a length or pattern | A slug is unique across existing resources | +| A number falls in a range | A referenced id actually exists | +| A oneof has exactly one field set | The caller has permission to perform the action | +| An enum value is not `UNSPECIFIED` | A parent reference points at a node of the required type | + +## Review Questions + +- Does any field use `LEGACY_REQUIRED` (or proto2 `required`) in a package that is no longer pre-release? +- Does every invariant that protovalidate can express (presence, range, pattern, enum exclusion, cross-field CEL) have a `buf.validate` annotation instead of living only in a comment? +- For every `buf.validate` annotation, is there a validator actually running on every code path that accepts the message, including from untrusted callers? +- Is any rule that needs live state (uniqueness, existence, authorization) mistakenly expressed, or attempted, as a schema-level constraint instead of a service-level check? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md new file mode 100644 index 0000000..a4545d9 --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md @@ -0,0 +1,47 @@ +# Review Checklist + +Each item is a symptom of a principle, not a rule on its own. When an item fails, read the linked reference and judge the schema by the principle behind it; a justified exception passes, a compliant schema that misses the point does not. + +## JSON Field Naming ([json-field-naming.md](json-field-naming.md)) + +- [ ] No field name reads as inline content (`project`, `result`, `owner`) while its type is an id or ref wrapper +- [ ] Single opaque identifiers are suffixed `_id`; structured references are suffixed `_ref` +- [ ] `parent` is used only for hierarchy position; kinship is qualified with a type (`parent_session_id`), never spelled `parent_id` + +## Value Objects ([value-objects.md](value-objects.md)) + +- [ ] No identifier is a bare `string` or numeric type; each is a single-field wrapper message +- [ ] No reference scatters its fields across siblings instead of one value object +- [ ] Any out-of-line content pointer carries its locator and its digest together in one message +- [ ] Numeric fields with a unit carry that unit in the field name +- [ ] Monetary fields pair a fixed-point amount with an ISO 4217 currency code, never a bare float +- [ ] Instants use `google.protobuf.Timestamp` with an `_at` suffix + +## Enums and Presets ([enums-and-presets.md](enums-and-presets.md)) + +- [ ] Every enum's zero value is named `_UNSPECIFIED` +- [ ] Every enum value is prefixed with the enum's name +- [ ] Service code rejects `UNSPECIFIED` rather than resolving it to a default, especially a permissive one +- [ ] No boolean field models a concept that could plausibly need a third state + +## Presence and Validation ([presence-and-validation.md](presence-and-validation.md)) + +- [ ] No field uses `LEGACY_REQUIRED` (or proto2 `required`) in a stable, non-pre-release package +- [ ] Schema-checkable invariants (presence, range, pattern, enum exclusion, cross-field rules) carry `buf.validate` annotations +- [ ] Every `buf.validate` annotation is backed by a validator actually running on every path that accepts the message +- [ ] No invariant that needs live state (uniqueness, existence, authorization) is expressed only as a schema-level constraint + +## Event-Sourced Contracts ([event-sourced-contracts.md](event-sourced-contracts.md)) + +Skip this section for plain CRUD/RPC schemas. + +- [ ] Commands are imperative; events are past tense +- [ ] The creation event carries every fact only knowable at creation time +- [ ] No event duplicates a fact owned by another system +- [ ] Faults are a `oneof` of specific types, not a generic error string +- [ ] Comments state invariants, not restatements of field names + +## Naming Collisions ([naming-collisions.md](naming-collisions.md)) + +- [ ] New names were checked against the product's existing vocabulary, not only the local file +- [ ] Any discovered collision was renamed before the package generates code diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/value-objects.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/value-objects.md new file mode 100644 index 0000000..0795ff9 --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/value-objects.md @@ -0,0 +1,138 @@ +# Value Objects Instead of Primitives + +Use this reference when defining an identifier, a reference to another resource, a pointer to content stored out of line, a quantity, a monetary amount, or an instant in time. + +## Identifiers Are Wrapper Messages + +A bare `string` or `int64` identifier carries its meaning only in the field name. Once that string crosses a function boundary, a log line, or a generic container, the meaning is gone and the compiler can no longer stop a `UserId` from being passed where a `ProjectId` is expected. + +```protobuf +// Avoid: primitive obsession. Nothing stops a swap between two string fields. +message TransferOwnership { + string project_id = 1; + string new_owner_id = 2; +} + +// Prefer: single-field wrapper messages. A ProjectId cannot be passed where a UserId belongs. +message ProjectId { + string value = 1; +} + +message UserId { + string value = 1; +} + +message TransferOwnership { + ProjectId project_id = 1; + UserId new_owner_id = 2; +} +``` + +The field name still carries the `_id` suffix even though the type already says "identifier." See [references/json-field-naming.md](json-field-naming.md) for why: a schema-less JSON reader sees only the key. + +## References Are Value Objects, Not Bare Strings + +The same reasoning applies to anything that points at a resource with more than one piece of identifying information: pair the fields in a named message instead of scattering them. + +```protobuf +// Avoid: two separate fields that only mean something together. +message RunJobResponse { + string result_bucket = 1; + string result_key = 2; +} + +// Prefer: a reference value object. +message ArtifactRef { + string bucket = 1; + string key = 2; +} + +message RunJobResponse { + ArtifactRef result_ref = 1; +} +``` + +## Claim Checks: a Locator and a Digest, Together + +Content stored out of line (blob storage, an artifact store, a large payload moved out of the message body) is the [claim check pattern](https://www.enterpriseintegrationpatterns.com/ClaimCheck.html): the message carries a claim, not the content. The claim is only useful if it says both where the content is and what it should hash to. Keep both fields in one value object; a locator in one message and a digest in a sibling field invites the two being paired wrong after an edit or a partial copy. + +```protobuf +// Avoid: locator and digest live in different messages, so nothing stops +// a locator from one artifact pairing with a digest from another. +message StartRestore { + string storage_locator = 1; +} + +message RestoreManifest { + string expected_digest = 1; +} + +// Prefer: the claim check is one value object with both fields. +message Digest { + string algorithm = 1; // e.g. "sha256" + bytes value = 2; +} + +message ClaimCheck { + string storage_locator = 1; + Digest digest = 2; +} + +message StartRestore { + ClaimCheck claim_check = 1; +} +``` + +## Units Live in the Name + +A bare numeric field answers "how much" but not "of what unit." Suffix the field name with the unit so the number is self-describing wherever it travels, including into JSON and logs. This mirrors [Google AIP-141](https://google.aip.dev/141): "Quantities with a clear unit of measurement (such as bytes, miles, and so on) must include the unit of measurement as the suffix." + +```protobuf +// Avoid: ambiguous unit. +int64 size = 1; +int32 timeout = 2; + +// Prefer: the unit travels with the value. +int64 size_bytes = 1; +int32 timeout_seconds = 2; +``` + +## Money Is Amount Plus Currency, Never a Float + +A bare amount is incomplete without its currency, and floating point loses cents at scale. Keep the amount as a fixed-point integer, name it with its unit, and pair it with an ISO 4217 currency code, in one message: + +```protobuf +// Avoid: no currency, and floating point drifts under repeated arithmetic. +double price = 1; + +// Prefer: fixed-point amount, explicit unit, explicit currency. +message Money { + int64 amount_micros = 1; + string currency_code = 2; // ISO 4217, e.g. "USD" +} +``` + +`google.type.Money` (units plus nanos, also paired with an ISO 4217 `currency_code`) is the widely used precedent for this shape; the fields above follow the same principle with micros instead of nanos as the fractional unit. Either precision works as long as the amount, its fractional unit, and the currency travel together in one message. + +## Instants Are Timestamps, Named With `_at` + +Use `google.protobuf.Timestamp` for a point in time, not an `int64` of ambiguous epoch and unit, and suffix the field name `_at` to mark it as an instant rather than a duration or a date-only value: + +```protobuf +import "google/protobuf/timestamp.proto"; + +// Avoid: ambiguous unit (seconds? millis?) and epoch. +int64 created = 1; + +// Prefer: self-describing type, _at suffix. +google.protobuf.Timestamp created_at = 1; +``` + +## Review Questions + +- Is any identifier a bare `string` or `int64` instead of a single-field wrapper message? +- Does any reference scatter its fields across sibling fields instead of one value object? +- Does any out-of-line content pointer carry a locator without the digest it is verified against, or vice versa, in separate messages? +- Does any numeric field omit its unit from the name? +- Does any monetary field use `float`/`double`, or omit the currency? +- Does any instant use a raw integer instead of `google.protobuf.Timestamp`, or omit the `_at` suffix? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md new file mode 100644 index 0000000..51a9aef --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md @@ -0,0 +1,59 @@ +--- +name: protobuf-evolve-schemas +description: "Evolve an existing protobuf schema without breaking consumers: field renames, type changes, reserved numbers and names, buf breaking detection in CI, and pre-release package versioning. Use when changing fields, enums, or packages in a .proto file that already has consumers, or setting up breaking-change CI. Do not use for designing new messages or field naming from scratch (use protobuf-design-messages)." +allowed-tools: + - AskUserQuestion + - Read + - Write + - Shell +--- + +# Evolve Protobuf Schemas Without Breaking Consumers + +A `.proto` file has audiences that can each break independently: the binary wire, JSON consumers, and generated source code. A change that is safe for one can be a break for another. + +## Core Principle + +"Non-breaking" has no meaning until you say non-breaking for whom. Check every change against every compatibility surface that applies to the package's actual consumers, not just the one that is easiest to verify. + +## Principles + +Apply them in order. A design that fails an earlier principle makes the later ones moot. Each one names the references that show it in practice; read them when the situation matches, not up front. + +### 1. Know Every Compatibility Surface + +Binary wire, JSON, and generated source code each have their own rules for what counts as breaking. `buf breaking` groups its checks into categories (`FILE`, `PACKAGE`, `WIRE_JSON`, `WIRE`) that map onto these surfaces. If anyone consumes JSON, `WIRE` alone is not enough; `FILE` and `PACKAGE` already include every `WIRE_JSON` check. + +Read [references/compatibility-surfaces.md](references/compatibility-surfaces.md) when deciding which `buf breaking` category to run, or when a proposed change needs to be checked against more than the wire format. + +### 2. A Rename Keeps the Field Number and Type + +Renaming a field is binary-compatible (the wire format encodes field numbers, not names) but breaks JSON keys and generated code identifiers. Renaming an enum value breaks JSON too, because enum values serialize by name. + +Read [references/renames-and-reserved.md](references/renames-and-reserved.md) when renaming a field, a message, or an enum value, or when reviewing a diff that does. + +### 3. A Type Change Never Reuses a Field Number + +Changing a field's type allocates a new field number and reserves the old one; the old name is reserved too unless it keeps pointing at the new number. A reserved number or name is never reused, ever. + +Read [references/renames-and-reserved.md](references/renames-and-reserved.md) for the `reserved` syntax and worked examples. + +### 4. Run `buf breaking` in CI, With an Explicit Opt-Out for Intentional Breaks + +Detect breaks automatically rather than by review alone. When a break is intentional in a pre-release package, make the exception a reviewable, visible decision (for example a PR label that skips the breaking job for that PR) instead of permanently weakening the `buf breaking` configuration for the whole package. + +Read [references/ci-and-governance.md](references/ci-and-governance.md) for the `buf breaking` invocation, the local git-hook caveat, and how to scope an intentional-break exception. + +### 5. Pre-Release Packages Version Instead of Breaking Silently + +A package name's last component signals its stability (`v1alpha1` -> `v1beta1` -> `v1`, per buf's `PACKAGE_VERSION_SUFFIX` lint rule). A stable package (`v1`) that needs a breaking change gets a new package version, not a break inside the existing one. + +Read [references/package-versioning.md](references/package-versioning.md) for the versioning progression and when a breaking change forces a new package version versus living inside the current pre-release one. + +## Worked Example + +Read [references/example-schema-evolution.md](references/example-schema-evolution.md) to see a field rename, a type change, and a package version bump applied to one schema, each handled the compatible way. + +## Reviewing a Proposed Schema Change + +Read [references/review-checklist.md](references/review-checklist.md) when reviewing a diff to an existing `.proto` file. Each item is a symptom; a failing item is a question about the principle behind it, answered in the linked reference. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md new file mode 100644 index 0000000..b8c6fe1 --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md @@ -0,0 +1,54 @@ +# Running `buf breaking` in CI, With a Reviewable Opt-Out + +Use this reference when setting up or reviewing breaking-change detection in CI, or when a break is genuinely intentional and needs a way through. + +## The Command + +Compare the working tree against the base branch using buf's git input syntax (see [buf docs: Breaking usage](https://buf.build/docs/breaking/usage/) and [buf docs: Inputs](https://buf.build/docs/reference/inputs/)): + +```bash +buf breaking --against '.git#branch=main' +``` + +Run this in CI on every pull request, against whatever branch the PR targets, so a break is caught before merge rather than discovered by a downstream consumer after release. + +## Local Tip: Breaking Global Git Config + +Running `buf breaking` against a git ref clones into a temporary directory and can trip over global git hooks or config that assume a normal working tree (commit signing hooks, credential helpers, or custom templates). If the local run fails for reasons that look like git configuration rather than an actual schema break, isolate it from global config: + +```bash +GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null buf breaking --against '.git#branch=main' +``` + +This is a local troubleshooting step, not something to bake into CI, where the environment is already clean. + +## When a Break Is Intentional + +A pre-release package (`v1alpha1`, see [references/package-versioning.md](package-versioning.md)) is explicitly allowed to break while its shape is being designed. The failure mode to avoid is making that exception permanent and invisible: weakening the `buf breaking` configuration for the package (removing it from the checked inputs, or lowering its category) silences every future break in that package too, intentional or not, and nobody reviewing a later PR will know the guardrail is gone. + +Prefer an opt-out that is explicit and scoped to the one PR that needs it: a PR label (for example `breaking-change-acknowledged`) that the CI breaking-change job checks for and skips only when present, so: + +- The exception is visible in the PR itself, not buried in a config file diff. +- A reviewer has to see and approve the label before the break ships. +- The next PR to this package is checked normally again; nothing was permanently turned off. + +```text +# CI job pseudocode: skip only this run, only when a human applied the label. +if pr.has_label("breaking-change-acknowledged"): + skip buf breaking +else: + run buf breaking --against '.git#branch=main' +``` + +The exact mechanism (a label, a required commit trailer, a specific reviewer's approval) matters less than the property it has to preserve: the opt-out is a per-change, reviewed decision, not a standing exemption. + +## Pre-Codegen Packages Cost Nothing to Break + +A package that is excluded from code generation entirely (nothing in the build consumes its generated bindings yet) has no consumers to break, so `buf breaking` findings against it are not really "breaks," they are just diff noise. It is reasonable to exclude such a package from the breaking-change check altogether until it is wired into code generation for the first time; at that point, treat its current shape as the new baseline and start enforcing `buf breaking` on it like any other package. + +## Review Questions + +- Does CI run `buf breaking` against the correct base branch on every pull request? +- Is any package's breaking-change check permanently disabled or weakened, rather than opted out of per-change? +- When a break was intentional, is there a visible, reviewed record of that decision on the PR, rather than a silent config change? +- Is a package excluded from breaking-change checks actually pre-codegen, or does it already have consumers? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md new file mode 100644 index 0000000..9157d1f --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md @@ -0,0 +1,38 @@ +# Compatibility Surfaces + +Use this reference when deciding which `buf breaking` category to run, or when a proposed change needs to be checked against more than the wire format. + +## Why "Non-Breaking" Needs an Audience + +A protobuf schema has independent consumers of "did this change break compatibility": + +- **Binary wire format**: field numbers and wire types, decoded without field names. +- **JSON (ProtoJSON)**: field names (or `json_name`) as object keys, enum values by name. +- **Generated source code**: identifiers in every target language, which care about names, not numbers. + +A change can be safe for one surface and a break for another. [buf's breaking change categories](https://buf.build/docs/breaking/rules/) map directly onto these surfaces, from strictest to most permissive: + +| Category | What it checks | Breaks when | +|---|---|---| +| `FILE` (buf's default) | Generated source code, per file | Code moves between files; some languages (Python) generate per-file bindings that break when a type moves files, even within the same package | +| `PACKAGE` | Generated source code, per package | Looser than `FILE`: types can move between files as long as they stay in the same package | +| `WIRE_JSON` | Wire and JSON encoding together | A change breaks binary decoding, or breaks JSON because JSON serializes by field/enum name, not number | +| `WIRE` | Wire encoding only | A change breaks binary decoding (field number/type changes, incompatible field number reuse) | + +## Pick `WIRE_JSON` as the Minimum if Anyone Consumes JSON + +`WIRE` alone only protects binary gRPC consumers. If the schema is exposed through a JSON-speaking transport (gRPC-Gateway, Connect, a webhook payload, a REST-ish proxy, or any client library that round-trips through JSON instead of binary), `WIRE_JSON` is the floor, not `WIRE`. A field rename passes a `WIRE`-only check and fails `WIRE_JSON`, because the wire format never cared about the name but the JSON key is the name. + +```text +# A rename is wire-compatible, JSON-incompatible. +buf breaking --against '.git#branch=main' (category: WIRE) -> passes +buf breaking --against '.git#branch=main' (category: WIRE_JSON) -> fails: field name change +``` + +`FILE` and `PACKAGE` are supersets of `WIRE_JSON` (buf's docs say so in the `FIELD_SAME_JSON_NAME` rule), so when generated source code is a first-class consumer, which it is in most codebases, pick `FILE` or `PACKAGE` instead of `WIRE_JSON`; you keep every JSON check and add the generated-code ones. Pick one category per module rather than combining them. + +## Review Questions + +- Does the `buf breaking` configuration include `WIRE_JSON` (or stricter) for any package with a JSON-speaking consumer? +- Was the proposed change checked against the category that actually matches this package's consumers, not just whichever category happens to pass? +- For a cross-language codebase, is the category `FILE` or `PACKAGE`, so generated-source breaks are caught along with wire and JSON breaks? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md new file mode 100644 index 0000000..a569388 --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md @@ -0,0 +1,98 @@ +# Worked Example: Evolving a Project Schema + +The same kind of `Project` resource as the [protobuf-design-messages worked example](../../protobuf-design-messages/references/example-project-schema.md), carried through real changes: a rename, a type change, and a breaking change in a stable package. + +## Starting Point + +```protobuf +edition = "2023"; + +package example.projects.v1alpha1; + +message ProjectId { + string value = 1; +} + +message Project { + ProjectId project_id = 1; + string title = 2; + string owner_id = 3; +} +``` + +## Change 1: a Rename, Handled as a Breaking Change + +`title` should have been `name` from the start. Renaming it is wire-compatible but breaks JSON keys and generated accessors (see [references/renames-and-reserved.md](renames-and-reserved.md)). Because the package is still `v1alpha1`, this is an acceptable break, made visible rather than silent: + +```protobuf +package example.projects.v1alpha1; + +message Project { + ProjectId project_id = 1; + string name = 2; // renamed from `title`; breaking, acceptable pre-v1 + string owner_id = 3; +} +``` + +In CI, this PR carries the `breaking-change-acknowledged` label (or whatever a given repository's equivalent is) so the `buf breaking` job's finding is reviewed and consciously accepted rather than the check being disabled for the package; see [references/ci-and-governance.md](ci-and-governance.md). + +## Change 2: a Type Change, Handled With `reserved` + +`owner_id` becomes a wrapper message (`UserId`) instead of a bare string, per [protobuf-design-messages](../../protobuf-design-messages/SKILL.md)'s no-primitive-obsession principle. The new field gets a new number; the old number and name are reserved: + +```protobuf +package example.projects.v1alpha1; + +message UserId { + string value = 1; +} + +message Project { + reserved 3; + reserved "owner_id"; + ProjectId project_id = 1; + string name = 2; + UserId owner = 4; +} +``` + +Field 4 (`owner`) is deliberately not named `owner_id`: since it is now a typed reference rather than an identifier, `owner_ref` or a bare role name would both be defensible depending on whether `UserId` is treated as an id-wrapper or a richer reference; the point demonstrated here is the mechanic (new number, reserved old number and name), not the specific name chosen. + +## Change 3: the Package Stabilizes, Then Needs a Breaking Change + +The package graduates: `v1alpha1` -> `v1beta1` -> `v1`, and ships as `example.projects.v1`. Later, `name` needs to become two fields (`display_name` and a separate `slug`). That is a breaking change to a stable package, so it does not land inside `v1`; it lands in a new `v2` package, and `v1` is left untouched for its existing consumers (see [references/package-versioning.md](package-versioning.md)): + +```protobuf +// example/projects/v1/project.proto: untouched, still serving v1 consumers. +package example.projects.v1; + +message Project { + ProjectId project_id = 1; + string name = 2; + UserId owner = 4; + reserved 3; + reserved "owner_id"; +} +``` + +```protobuf +// example/projects/v2/project.proto: the breaking change lives here. +package example.projects.v2; + +message Project { + ProjectId project_id = 1; + string display_name = 2; + string slug = 3; + UserId owner = 4; +} +``` + +## What Each Change Demonstrates + +| Change | Principle | +|---|---| +| `title` -> `name` | A rename is wire-safe, JSON/codegen-breaking; acceptable pre-`v1`, made visible via a reviewed CI opt-out | +| `owner_id` (string) -> `owner` (`UserId`) | A type change gets a new field number; the old number and name are `reserved`, never reused | +| `v1alpha1` -> `v1beta1` -> `v1` -> `v2` | A stable package's breaking change produces a new package version instead of mutating the one consumers already depend on | + +Note: `example.projects.*` does not refer to any real service; it is a placeholder package name for this worked example. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/package-versioning.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/package-versioning.md new file mode 100644 index 0000000..e9527db --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/package-versioning.md @@ -0,0 +1,50 @@ +# Pre-Release Package Versioning + +Use this reference for the versioning progression a package's name signals, and when a breaking change needs a new package version rather than living inside the current one. + +## The Progression + +buf's `PACKAGE_VERSION_SUFFIX` lint rule requires the last component of a package name to be a version in one of these forms: `v\d+`, `v\d+test.*`, `v\d+(alpha|beta)\d*`, or `v\d+p\d+(alpha|beta)\d*` (see [buf docs: Lint rules](https://buf.build/docs/lint/rules/)). In practice, most packages move through: + +```text +example.projects.v1alpha1 -> example.projects.v1beta1 -> example.projects.v1 +``` + +| Suffix | Signals | +|---|---| +| `v1alpha1`, `v1alpha2`, ... | Actively changing shape; breaking changes expected and do not require a new package | +| `v1beta1`, `v1beta2`, ... | Converging on a stable shape; breaking changes are costlier and should be rarer | +| `v1` | Stable; a breaking change here is not a continuation of `v1`, it is `v2` | + +## A Breaking Change in a Stable Package Means a New Package Version + +Once a package has shipped as `v1`, consumers depend on its current shape. A breaking change to `v1` does not get folded into `v1` after the fact; it produces a new package, `v2`, that coexists with `v1` for as long as `v1` still has consumers: + +```protobuf +// example/projects/v1/project.proto keeps its current, stable shape. +package example.projects.v1; + +message Project { + ProjectId project_id = 1; + string name = 2; +} +``` + +```protobuf +// example/projects/v2/project.proto carries the breaking change. +// v1 is untouched and keeps serving its existing consumers. +package example.projects.v2; + +message Project { + ProjectId project_id = 1; + string display_name = 2; // renamed from `name`, which is why this is v2 +} +``` + +This is the same principle as the reserved-number rule in [references/renames-and-reserved.md](renames-and-reserved.md) applied at the package level: the old shape (`v1`) is never mutated out from under its consumers; a new, separately versioned shape (`v2`) exists alongside it instead. + +## Review Questions + +- Does the package name's version suffix match its actual stability (alpha/beta churn versus a frozen `v1`)? +- Does a proposed breaking change target a pre-release package (fine, within reason) or a stable one (needs a new package version instead)? +- If a new major package version was introduced, is the old one left untouched rather than also being modified? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md new file mode 100644 index 0000000..4e1d01c --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md @@ -0,0 +1,76 @@ +# Renames, Type Changes, and Reserved Fields + +Use this reference when renaming a field, message, or enum value, when changing a field's type, or when reviewing a diff that does either. + +## A Field Rename Keeps the Number and Type + +The binary wire format encodes field numbers, not field names; a decoder that has never seen the new name still decodes the message correctly as long as the number and wire type are unchanged. JSON and generated code are not so forgiving: the JSON key is the field name (or `json_name`), and generated identifiers are derived from the field name in every target language. + +```protobuf +// Before +message Project { + string title = 1; +} + +// After: renamed title -> name. Binary still decodes (field 1 unchanged). +// JSON breaks: a client sending {"title": "..."} now sets nothing, and a +// client reading the response no longer finds a "title" key. +// Generated code breaks: callers of project.title() / project.Title no longer compile. +message Project { + string name = 1; +} +``` + +A rename is a `WIRE_JSON`-category and `FILE`/`PACKAGE`-category break even though it is a `WIRE`-category no-op; see [references/compatibility-surfaces.md](compatibility-surfaces.md). If a rename is still necessary (the old name was wrong, or collided with other vocabulary per [protobuf-design-messages](../../protobuf-design-messages/SKILL.md)), treat it as a breaking change requiring the same coordination as any other: a new package version, or a deliberate opt-out in CI for a pre-release package (see [references/ci-and-governance.md](ci-and-governance.md)). + +## An Enum Value Rename Breaks JSON Too + +Enum values serialize to JSON by name, not by number, so renaming an enum value is a JSON break even though the underlying integer is unchanged: + +```protobuf +// Before +enum ProjectState { + PROJECT_STATE_UNSPECIFIED = 0; + PROJECT_STATE_DONE = 1; +} + +// After: DONE -> COMPLETED. The integer 1 is unchanged, but a JSON consumer +// that was matching the string "PROJECT_STATE_DONE" now sees +// "PROJECT_STATE_COMPLETED" and silently fails to match. +enum ProjectState { + PROJECT_STATE_UNSPECIFIED = 0; + PROJECT_STATE_COMPLETED = 1; +} +``` + +## A Type Change Never Reuses the Old Field Number + +Changing a field's type (for example, a `string` identifier becoming a wrapper message, or an `int32` becoming an `int64` outside the compatible-widening cases documented by protobuf) is a wire-level break if the new wire type differs. Give the new value a new field number, and mark the old number (and, unless the old name is being reused for the new field at the new number, the old name too) `reserved` so nothing ever reuses it: + +```protobuf +// Before +message Project { + string owner_id = 9; +} + +// After: owner_id becomes a wrapper message instead of a bare string. +// The old field 9 is reserved, never reused. The name owner_id is not +// reserved because the new field reuses it; reserving it would not compile. +message Project { + reserved 9; + UserId owner_id = 10; +} +``` + +A future reader (or a future engineer who does not know this history) who adds a new field will see the `reserved` statement and knows not to pick number 9 for anything unrelated. Without it, a new field at number 9 would be silently misinterpreted by any old binary still holding a message serialized under the old schema, which is exactly the class of bug `reserved` exists to prevent. + +## Never Reuse a Reserved Number, Even Much Later + +A `reserved` statement is permanent. Removing it to "free up" the number for an unrelated field defeats the entire purpose: any message serialized under the old schema that still exists anywhere (a queue, a log, a backup, a long-lived client) will be misinterpreted as having the new field's meaning at the old number. + +## Review Questions + +- Does the diff rename a field, message, or enum value without reserving the old name/number and without a plan for the resulting JSON/generated-code break? +- Does the diff change a field's type at the same field number, instead of allocating a new number and reserving the old one? +- Does the diff remove a `reserved` statement to reuse a number or name? +- Does the diff rename an enum value, and has it been checked against `WIRE_JSON`, not just `WIRE`? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md new file mode 100644 index 0000000..f96090a --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md @@ -0,0 +1,28 @@ +# Review Checklist + +Each item is a symptom of a principle, not a rule on its own. When an item fails, read the linked reference and judge the change by the principle behind it; a justified exception passes, a compliant change that misses the point does not. + +## Compatibility Surfaces ([compatibility-surfaces.md](compatibility-surfaces.md)) + +- [ ] The change was checked against the `buf breaking` category that matches this package's actual consumers (binary-only vs. JSON vs. generated source) +- [ ] `WIRE_JSON` or stricter is the floor for any package with a JSON-speaking consumer + +## Renames and Reserved Fields ([renames-and-reserved.md](renames-and-reserved.md)) + +- [ ] No field, message, or enum value was renamed without accounting for the JSON/generated-code break that follows +- [ ] No field's type changed at its existing field number; a new number was allocated instead +- [ ] The old field number is `reserved`, and the old name is `reserved` unless it stays attached to the new number +- [ ] No `reserved` statement was removed to reuse a number or name + +## CI and Governance ([ci-and-governance.md](ci-and-governance.md)) + +- [ ] `buf breaking` runs in CI against the correct base branch +- [ ] No package's breaking-change check was permanently disabled or weakened to let one change through +- [ ] An intentional break carries a visible, per-change, reviewed opt-out (e.g. a PR label), not a standing exemption +- [ ] Any package excluded from breaking-change checks is actually pre-codegen, with no consumers yet + +## Package Versioning ([package-versioning.md](package-versioning.md)) + +- [ ] The package's version suffix (`v1alpha1`, `v1beta1`, `v1`, ...) matches its actual stability +- [ ] A breaking change to a stable (`v1`+) package produced a new package version instead of mutating the existing one +- [ ] The previous package version was left untouched after a new one was introduced From 06d813dc4730576449e48f31b2cb0644fc9a816f Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 20:55:56 -0400 Subject: [PATCH 2/5] refactor(trogonstack-protobuf): load only the guidance the task at hand needs Signed-off-by: Yordis Prieto --- .../skills/protobuf-design-messages/SKILL.md | 72 +++------ .../references/claim-checks.md | 38 +++++ .../references/event-sourced-contracts.md | 9 ++ .../references/example-project-schema.md | 9 +- .../references/identifiers-and-references.md | 58 ++++++++ .../references/json-field-naming.md | 2 +- .../references/naming-collisions.md | 2 +- .../references/review-checklist.md | 10 +- .../references/units-money-and-time.md | 54 +++++++ .../references/value-objects.md | 138 ------------------ .../skills/protobuf-evolve-schemas/SKILL.md | 62 +++----- .../references/example-schema-evolution.md | 14 +- .../references/renames-and-reserved.md | 2 +- .../references/review-checklist.md | 2 + 14 files changed, 232 insertions(+), 240 deletions(-) create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/claim-checks.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/identifiers-and-references.md create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/units-money-and-time.md delete mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/value-objects.md diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md index 86ca1dc..3d1806a 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md @@ -10,56 +10,28 @@ allowed-tools: # Design or Review Protobuf Messages and Fields -Shape messages and fields so the schema itself carries the meaning: what a field points at, what a zero value does, and what the schema can and cannot guarantee. - -## Core Principle - -A `.proto` file is read by more than the compiler. ProtoJSON consumers see only field names, never types; reviewers see only names and comments, never runtime behavior; everyone who did not write the schema sees only what it says. A field, message, or enum that relies on tribal knowledge to interpret correctly is a defect, not a style nit. +A `.proto` file is read by more than the compiler: ProtoJSON consumers see only field names, never types, and reviewers see only names and comments, never runtime behavior. A field, message, or enum that relies on tribal knowledge to interpret correctly is a defect, not a style nit. Apply the principles below in order; a design that fails an earlier one makes the later ones moot. ## Principles -Apply them in order. A design that fails an earlier principle makes the later ones moot. Each one names the references that show it in practice; read them when the situation matches, not up front. - -### 1. Names Must Survive Schema-less JSON Readers - -ProtoJSON keys default to the lowerCamelCase of the field name (`json_name` overrides it, and parsers accept both forms). Many consumers see only the JSON, never the `.proto`, so the field name is the only signal they get. A field whose value points at something stored or owned elsewhere keeps a suffix in its name even when its type already says so. - -Read [references/json-field-naming.md](references/json-field-naming.md) when naming a field that is an identifier, a structured reference, or a hierarchy position, or when reviewing a schema for names that read as inline content but are not. - -### 2. No Primitive Obsession - -Identifiers are single-field wrapper messages, not a bare `string` whose only clue is the field name. References are value objects. Bytes stored out of line pair a storage locator with the digest it is verified against, in one message. Units live in names, and money and instants use the field name patterns the rest of the ecosystem already uses. - -Read [references/value-objects.md](references/value-objects.md) when defining an identifier, a reference to another resource, a pointer to out-of-line content, a quantity, a monetary amount, or an instant in time. - -### 3. Enums Encode an Exhaustive Choice, Not a Growable Boolean - -The zero value is always `_UNSPECIFIED`, and code must reject it rather than resolve it to a default. An omitted choice must never silently widen behavior. Prefer an enum, or a `oneof`, over a boolean that is one feature request away from needing a third state. - -Read [references/enums-and-presets.md](references/enums-and-presets.md) when defining an enum, naming its values, or designing a preset/option field such as an access or visibility level. - -### 4. Know What the Schema Can and Cannot Guarantee - -Editions presence (`EXPLICIT`, `IMPLICIT`, `LEGACY_REQUIRED`) and `protovalidate` constraints are schema-level tools, and each has a limit: `LEGACY_REQUIRED` makes removing the field later a breaking change; `buf.validate` annotations are documentation unless something in the stack actually runs protovalidate. Some invariants cannot be checked from the schema at all because they need live state. - -Read [references/presence-and-validation.md](references/presence-and-validation.md) when deciding whether a field should be required, writing a `buf.validate` constraint, or distinguishing a schema-checkable invariant from one that belongs in the service. - -### 5. Event-Sourced Contracts Separate Commands, Events, State, and Faults - -When the package models a decider (command handler plus event-sourced aggregate), commands are imperative and events are past tense. Facts that exist only at creation time belong on the creation event, even when another system owns everything that happens afterward. An aggregate does not get an event for a fact it does not own. - -Read [references/event-sourced-contracts.md](references/event-sourced-contracts.md) when designing commands, events, state, or faults for an event-sourced resource. Skip this reference entirely for plain CRUD-shaped or RPC request/response schemas; it does not apply to them. - -### 6. Rename Early When Words Collide With Product Vocabulary - -A name that is fine in isolation can collide with a term the product already uses for something else. The cheapest moment to fix a name is before anything generates code from it; a package still under design can be excluded from code generation so the churn costs nothing. - -Read [references/naming-collisions.md](references/naming-collisions.md) when a field, message, or package name echoes a term used elsewhere in the product for a different concept. - -## Worked Example - -Read [references/example-project-schema.md](references/example-project-schema.md) to see every principle applied in one complete `.proto` package: a `Project` resource with an identifier, a hierarchy position, an access preset enum, and the commands and events that create, rename, and archive it. - -## Reviewing an Existing Schema - -Read [references/review-checklist.md](references/review-checklist.md) when reviewing or auditing message and field definitions. Each item is a symptom; a failing item is a question about the principle behind it, answered in the linked reference. +1. **Names must survive schema-less JSON readers.** A field whose value points at something stored or owned elsewhere keeps a suffix in its name even when its type already says so. +2. **No primitive obsession.** Identifiers, references, and out-of-line content pointers are value objects, not bare strings; units, money, and instants follow the ecosystem's naming patterns. +3. **Enums encode an exhaustive choice, not a growable boolean.** The zero value is always `_UNSPECIFIED`, code must reject it, and a boolean that might need a third state should be an enum instead. +4. **Know what the schema can and cannot guarantee.** Editions presence and `protovalidate` constraints are schema-level tools with real limits; some invariants need live state and belong in the service. +5. **Event-sourced contracts separate commands, events, state, and faults.** Commands are imperative, events are past tense, and an aggregate never gets an event for a fact it does not own. +6. **Rename early when words collide with product vocabulary.** The cheapest moment to fix a name is before anything generates code from it. + +## Load a Reference + +| When you are... | Load | +|---|---| +| Naming a field that is an identifier, structured reference, or hierarchy position | [references/json-field-naming.md](references/json-field-naming.md) | +| Defining an identifier, or a reference to another resource | [references/identifiers-and-references.md](references/identifiers-and-references.md) | +| Defining a pointer to content stored out of line | [references/claim-checks.md](references/claim-checks.md) | +| Defining a quantity, a monetary amount, or an instant in time | [references/units-money-and-time.md](references/units-money-and-time.md) | +| Defining an enum, naming its values, or designing a preset/option field | [references/enums-and-presets.md](references/enums-and-presets.md) | +| Deciding whether a field should be required, or writing a `buf.validate` constraint | [references/presence-and-validation.md](references/presence-and-validation.md) | +| Designing commands, events, state, or faults for an event-sourced resource | [references/event-sourced-contracts.md](references/event-sourced-contracts.md) | +| A field, message, or package name echoes a term used elsewhere for a different concept | [references/naming-collisions.md](references/naming-collisions.md) | +| Wanting to see every principle applied in one complete `.proto` package | [references/example-project-schema.md](references/example-project-schema.md) | +| Reviewing or auditing an existing schema | [references/review-checklist.md](references/review-checklist.md) | diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/claim-checks.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/claim-checks.md new file mode 100644 index 0000000..1a8599a --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/claim-checks.md @@ -0,0 +1,38 @@ +# Claim Checks for Content Stored Out of Line + +Use this reference when defining a pointer to content stored out of line. + +## A Locator and a Digest, Together + +Content stored out of line (blob storage, an artifact store, a large payload moved out of the message body) is the [claim check pattern](https://www.enterpriseintegrationpatterns.com/ClaimCheck.html): the message carries a claim, not the content. The claim is only useful if it says both where the content is and what it should hash to. Keep both fields in one value object; a locator in one message and a digest in a sibling field invites the two being paired wrong after an edit or a partial copy. + +```protobuf +// Avoid: locator and digest live in different messages, so nothing stops +// a locator from one artifact pairing with a digest from another. +message StartRestore { + string storage_locator = 1; +} + +message RestoreManifest { + string expected_digest = 1; +} + +// Prefer: the claim check is one value object with both fields. +message Digest { + string algorithm = 1; // e.g. "sha256" + bytes value = 2; +} + +message ClaimCheck { + string storage_locator = 1; + Digest digest = 2; +} + +message StartRestore { + ClaimCheck claim_check = 1; +} +``` + +## Review Questions + +- Does any out-of-line content pointer carry a locator without the digest it is verified against, or vice versa, in separate messages? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/event-sourced-contracts.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/event-sourced-contracts.md index 290d88c..09a3b60 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/event-sourced-contracts.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/event-sourced-contracts.md @@ -2,6 +2,15 @@ Use this reference when designing commands, events, state, or faults for an event-sourced resource (a decider: command in, events out, state folded from events). Skip it for plain CRUD-shaped or RPC request/response schemas; it does not apply to them. +## Contents + +- Name by Tense +- Record Creation-Time Facts on the Creation Event, Even When Another System Owns What Happens Later +- Do Not Add an Event for a Fact the Aggregate Does Not Own +- State and Faults +- Comments Document Invariants, Not Mechanics +- Review Questions + ## Name by Tense Commands are imperative, because a command is a request that something happen and might be refused. Events are past tense, because an event is a fact that already happened and cannot be refused after the fact: diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md index 9685d1e..1ced328 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md @@ -1,6 +1,11 @@ # Worked Example: a Project Resource -A complete, small `.proto` package for a generic `Project` resource, applying every principle in [SKILL.md](../SKILL.md): wrapped identifiers, a hierarchy position, a bare-vs-qualified parent distinction, an access preset enum that rejects its unspecified value, a money value object, and a decider's commands, events, state, and faults. +Use this reference when you want to see every principle in [SKILL.md](../SKILL.md) applied together in one complete, small `.proto` package for a generic `Project` resource: wrapped identifiers, a hierarchy position, a bare-vs-qualified parent distinction, an access preset enum that rejects its unspecified value, a money value object, and a decider's commands, events, state, and faults. + +## Contents + +- The `.proto` package, in full +- What Each Choice Demonstrates ```protobuf edition = "2023"; @@ -144,6 +149,6 @@ message RenameProjectFault { | `ProjectCreated` carries `parent` and `initial_access_preset` | Creation-time facts recorded even though later changes are owned elsewhere | | No `ProjectMoved` or `ProjectAccessChanged` event | The aggregate does not emit events for facts it does not own | | `RenameProjectFault` as a `oneof` | Specific, matchable fault types instead of a generic error | -| `package example.projects.v1alpha1` | Pre-release versioning; see [protobuf-evolve-schemas](../../protobuf-evolve-schemas/SKILL.md) for when and how this graduates to `v1beta1` and `v1` | +| `package example.projects.v1alpha1` | Pre-release versioning; the protobuf-evolve-schemas skill covers when and how this graduates to `v1beta1` and `v1` | Note: `example.projects.v1alpha1` does not refer to any real service; it is a placeholder package name for this worked example. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/identifiers-and-references.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/identifiers-and-references.md new file mode 100644 index 0000000..00002ca --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/identifiers-and-references.md @@ -0,0 +1,58 @@ +# Identifiers and References Instead of Primitives + +Use this reference when defining an identifier or a reference to another resource. + +## Identifiers Are Wrapper Messages + +A bare `string` or `int64` identifier carries its meaning only in the field name. Once that string crosses a function boundary, a log line, or a generic container, the meaning is gone and the compiler can no longer stop a `UserId` from being passed where a `ProjectId` is expected. + +```protobuf +// Avoid: primitive obsession. Nothing stops a swap between two string fields. +message TransferOwnership { + string project_id = 1; + string new_owner_id = 2; +} + +// Prefer: single-field wrapper messages. A ProjectId cannot be passed where a UserId belongs. +message ProjectId { + string value = 1; +} + +message UserId { + string value = 1; +} + +message TransferOwnership { + ProjectId project_id = 1; + UserId new_owner_id = 2; +} +``` + +The field name still carries the `_id` suffix even though the type already says "identifier." See [references/json-field-naming.md](json-field-naming.md) for why: a schema-less JSON reader sees only the key. + +## References Are Value Objects, Not Bare Strings + +The same reasoning applies to anything that points at a resource with more than one piece of identifying information: pair the fields in a named message instead of scattering them. + +```protobuf +// Avoid: two separate fields that only mean something together. +message RunJobResponse { + string result_bucket = 1; + string result_key = 2; +} + +// Prefer: a reference value object. +message ArtifactRef { + string bucket = 1; + string key = 2; +} + +message RunJobResponse { + ArtifactRef result_ref = 1; +} +``` + +## Review Questions + +- Is any identifier a bare `string` or `int64` instead of a single-field wrapper message? +- Does any reference scatter its fields across sibling fields instead of one value object? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/json-field-naming.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/json-field-naming.md index 5483987..d96ea2d 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/json-field-naming.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/json-field-naming.md @@ -73,6 +73,6 @@ This rule and its rationale come from the public straw-hat-team ADR on hierarchy ## Review Questions - Does any field name read as inline content (`project`, `result`, `owner`) while its type is an id or ref wrapper? Rename it with the matching suffix. -- Does any `_id` or `_ref` field have a type that is a bare `string` instead of a wrapper message? See [references/value-objects.md](value-objects.md). +- Does any `_id` or `_ref` field have a type that is a bare `string` instead of a wrapper message? See [references/identifiers-and-references.md](identifiers-and-references.md). - Is `parent` used for anything other than hierarchy position? Rename the kinship use to `parent__id`. - Does any field spell kinship as `parent_id` or `parentId` with no type qualifier? That spelling is disallowed under this convention; it reads as a role with no type. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md index 254304a..1c0ec60 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md @@ -24,7 +24,7 @@ The collision rarely shows up by reading the file the new field lives in; it sho ## The Cheapest Moment to Rename Is Before Anything Generates Code -Once generated code exists, a rename is a breaking change (see [protobuf-evolve-schemas](../../protobuf-evolve-schemas/SKILL.md) for why a rename breaks JSON and generated source even when it is wire-compatible). Before that point, a rename costs nothing: it is a text edit in a file nobody depends on yet. +Once generated code exists, a rename is a breaking change (the protobuf-evolve-schemas skill covers why a rename breaks JSON and generated source even when it is wire-compatible). Before that point, a rename costs nothing: it is a text edit in a file nobody depends on yet. This is one of the reasons a package still under active design is worth excluding from code generation entirely (for example, via `buf.gen.yaml` exclusions or simply not wiring the package into the build) until its shape has settled. While a package generates no code, every name in it can change for free; the moment it does, every rename has a cost that scales with how many consumers pulled in the generated bindings. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md index a4545d9..9ed1ba6 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md @@ -1,5 +1,7 @@ # Review Checklist +Use this reference when reviewing or auditing message and field definitions. + Each item is a symptom of a principle, not a rule on its own. When an item fails, read the linked reference and judge the schema by the principle behind it; a justified exception passes, a compliant schema that misses the point does not. ## JSON Field Naming ([json-field-naming.md](json-field-naming.md)) @@ -8,11 +10,17 @@ Each item is a symptom of a principle, not a rule on its own. When an item fails - [ ] Single opaque identifiers are suffixed `_id`; structured references are suffixed `_ref` - [ ] `parent` is used only for hierarchy position; kinship is qualified with a type (`parent_session_id`), never spelled `parent_id` -## Value Objects ([value-objects.md](value-objects.md)) +## Identifiers and References ([identifiers-and-references.md](identifiers-and-references.md)) - [ ] No identifier is a bare `string` or numeric type; each is a single-field wrapper message - [ ] No reference scatters its fields across siblings instead of one value object + +## Claim Checks ([claim-checks.md](claim-checks.md)) + - [ ] Any out-of-line content pointer carries its locator and its digest together in one message + +## Units, Money, and Time ([units-money-and-time.md](units-money-and-time.md)) + - [ ] Numeric fields with a unit carry that unit in the field name - [ ] Monetary fields pair a fixed-point amount with an ISO 4217 currency code, never a bare float - [ ] Instants use `google.protobuf.Timestamp` with an `_at` suffix diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/units-money-and-time.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/units-money-and-time.md new file mode 100644 index 0000000..f8fb67d --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/units-money-and-time.md @@ -0,0 +1,54 @@ +# Units, Money, and Time Instead of Bare Numbers + +Use this reference when defining a quantity, a monetary amount, or an instant in time. + +## Units Live in the Name + +A bare numeric field answers "how much" but not "of what unit." Suffix the field name with the unit so the number is self-describing wherever it travels, including into JSON and logs. This mirrors [Google AIP-141](https://google.aip.dev/141): "Quantities with a clear unit of measurement (such as bytes, miles, and so on) must include the unit of measurement as the suffix." + +```protobuf +// Avoid: ambiguous unit. +int64 size = 1; +int32 timeout = 2; + +// Prefer: the unit travels with the value. +int64 size_bytes = 1; +int32 timeout_seconds = 2; +``` + +## Money Is Amount Plus Currency, Never a Float + +A bare amount is incomplete without its currency, and floating point loses cents at scale. Keep the amount as a fixed-point integer, name it with its unit, and pair it with an ISO 4217 currency code, in one message: + +```protobuf +// Avoid: no currency, and floating point drifts under repeated arithmetic. +double price = 1; + +// Prefer: fixed-point amount, explicit unit, explicit currency. +message Money { + int64 amount_micros = 1; + string currency_code = 2; // ISO 4217, e.g. "USD" +} +``` + +`google.type.Money` (units plus nanos, also paired with an ISO 4217 `currency_code`) is the widely used precedent for this shape; the fields above follow the same principle with micros instead of nanos as the fractional unit. Either precision works as long as the amount, its fractional unit, and the currency travel together in one message. + +## Instants Are Timestamps, Named With `_at` + +Use `google.protobuf.Timestamp` for a point in time, not an `int64` of ambiguous epoch and unit, and suffix the field name `_at` to mark it as an instant rather than a duration or a date-only value: + +```protobuf +import "google/protobuf/timestamp.proto"; + +// Avoid: ambiguous unit (seconds? millis?) and epoch. +int64 created = 1; + +// Prefer: self-describing type, _at suffix. +google.protobuf.Timestamp created_at = 1; +``` + +## Review Questions + +- Does any numeric field omit its unit from the name? +- Does any monetary field use `float`/`double`, or omit the currency? +- Does any instant use a raw integer instead of `google.protobuf.Timestamp`, or omit the `_at` suffix? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/value-objects.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/value-objects.md deleted file mode 100644 index 0795ff9..0000000 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/value-objects.md +++ /dev/null @@ -1,138 +0,0 @@ -# Value Objects Instead of Primitives - -Use this reference when defining an identifier, a reference to another resource, a pointer to content stored out of line, a quantity, a monetary amount, or an instant in time. - -## Identifiers Are Wrapper Messages - -A bare `string` or `int64` identifier carries its meaning only in the field name. Once that string crosses a function boundary, a log line, or a generic container, the meaning is gone and the compiler can no longer stop a `UserId` from being passed where a `ProjectId` is expected. - -```protobuf -// Avoid: primitive obsession. Nothing stops a swap between two string fields. -message TransferOwnership { - string project_id = 1; - string new_owner_id = 2; -} - -// Prefer: single-field wrapper messages. A ProjectId cannot be passed where a UserId belongs. -message ProjectId { - string value = 1; -} - -message UserId { - string value = 1; -} - -message TransferOwnership { - ProjectId project_id = 1; - UserId new_owner_id = 2; -} -``` - -The field name still carries the `_id` suffix even though the type already says "identifier." See [references/json-field-naming.md](json-field-naming.md) for why: a schema-less JSON reader sees only the key. - -## References Are Value Objects, Not Bare Strings - -The same reasoning applies to anything that points at a resource with more than one piece of identifying information: pair the fields in a named message instead of scattering them. - -```protobuf -// Avoid: two separate fields that only mean something together. -message RunJobResponse { - string result_bucket = 1; - string result_key = 2; -} - -// Prefer: a reference value object. -message ArtifactRef { - string bucket = 1; - string key = 2; -} - -message RunJobResponse { - ArtifactRef result_ref = 1; -} -``` - -## Claim Checks: a Locator and a Digest, Together - -Content stored out of line (blob storage, an artifact store, a large payload moved out of the message body) is the [claim check pattern](https://www.enterpriseintegrationpatterns.com/ClaimCheck.html): the message carries a claim, not the content. The claim is only useful if it says both where the content is and what it should hash to. Keep both fields in one value object; a locator in one message and a digest in a sibling field invites the two being paired wrong after an edit or a partial copy. - -```protobuf -// Avoid: locator and digest live in different messages, so nothing stops -// a locator from one artifact pairing with a digest from another. -message StartRestore { - string storage_locator = 1; -} - -message RestoreManifest { - string expected_digest = 1; -} - -// Prefer: the claim check is one value object with both fields. -message Digest { - string algorithm = 1; // e.g. "sha256" - bytes value = 2; -} - -message ClaimCheck { - string storage_locator = 1; - Digest digest = 2; -} - -message StartRestore { - ClaimCheck claim_check = 1; -} -``` - -## Units Live in the Name - -A bare numeric field answers "how much" but not "of what unit." Suffix the field name with the unit so the number is self-describing wherever it travels, including into JSON and logs. This mirrors [Google AIP-141](https://google.aip.dev/141): "Quantities with a clear unit of measurement (such as bytes, miles, and so on) must include the unit of measurement as the suffix." - -```protobuf -// Avoid: ambiguous unit. -int64 size = 1; -int32 timeout = 2; - -// Prefer: the unit travels with the value. -int64 size_bytes = 1; -int32 timeout_seconds = 2; -``` - -## Money Is Amount Plus Currency, Never a Float - -A bare amount is incomplete without its currency, and floating point loses cents at scale. Keep the amount as a fixed-point integer, name it with its unit, and pair it with an ISO 4217 currency code, in one message: - -```protobuf -// Avoid: no currency, and floating point drifts under repeated arithmetic. -double price = 1; - -// Prefer: fixed-point amount, explicit unit, explicit currency. -message Money { - int64 amount_micros = 1; - string currency_code = 2; // ISO 4217, e.g. "USD" -} -``` - -`google.type.Money` (units plus nanos, also paired with an ISO 4217 `currency_code`) is the widely used precedent for this shape; the fields above follow the same principle with micros instead of nanos as the fractional unit. Either precision works as long as the amount, its fractional unit, and the currency travel together in one message. - -## Instants Are Timestamps, Named With `_at` - -Use `google.protobuf.Timestamp` for a point in time, not an `int64` of ambiguous epoch and unit, and suffix the field name `_at` to mark it as an instant rather than a duration or a date-only value: - -```protobuf -import "google/protobuf/timestamp.proto"; - -// Avoid: ambiguous unit (seconds? millis?) and epoch. -int64 created = 1; - -// Prefer: self-describing type, _at suffix. -google.protobuf.Timestamp created_at = 1; -``` - -## Review Questions - -- Is any identifier a bare `string` or `int64` instead of a single-field wrapper message? -- Does any reference scatter its fields across sibling fields instead of one value object? -- Does any out-of-line content pointer carry a locator without the digest it is verified against, or vice versa, in separate messages? -- Does any numeric field omit its unit from the name? -- Does any monetary field use `float`/`double`, or omit the currency? -- Does any instant use a raw integer instead of `google.protobuf.Timestamp`, or omit the `_at` suffix? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md index 51a9aef..e736b1d 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md @@ -10,50 +10,24 @@ allowed-tools: # Evolve Protobuf Schemas Without Breaking Consumers -A `.proto` file has audiences that can each break independently: the binary wire, JSON consumers, and generated source code. A change that is safe for one can be a break for another. - -## Core Principle - -"Non-breaking" has no meaning until you say non-breaking for whom. Check every change against every compatibility surface that applies to the package's actual consumers, not just the one that is easiest to verify. +A `.proto` file has audiences that can each break independently: the binary wire, JSON consumers, and generated source code. "Non-breaking" has no meaning until you say non-breaking for whom, so check every change against every compatibility surface that applies to the package's actual consumers, not just the one that is easiest to verify. ## Principles -Apply them in order. A design that fails an earlier principle makes the later ones moot. Each one names the references that show it in practice; read them when the situation matches, not up front. - -### 1. Know Every Compatibility Surface - -Binary wire, JSON, and generated source code each have their own rules for what counts as breaking. `buf breaking` groups its checks into categories (`FILE`, `PACKAGE`, `WIRE_JSON`, `WIRE`) that map onto these surfaces. If anyone consumes JSON, `WIRE` alone is not enough; `FILE` and `PACKAGE` already include every `WIRE_JSON` check. - -Read [references/compatibility-surfaces.md](references/compatibility-surfaces.md) when deciding which `buf breaking` category to run, or when a proposed change needs to be checked against more than the wire format. - -### 2. A Rename Keeps the Field Number and Type - -Renaming a field is binary-compatible (the wire format encodes field numbers, not names) but breaks JSON keys and generated code identifiers. Renaming an enum value breaks JSON too, because enum values serialize by name. - -Read [references/renames-and-reserved.md](references/renames-and-reserved.md) when renaming a field, a message, or an enum value, or when reviewing a diff that does. - -### 3. A Type Change Never Reuses a Field Number - -Changing a field's type allocates a new field number and reserves the old one; the old name is reserved too unless it keeps pointing at the new number. A reserved number or name is never reused, ever. - -Read [references/renames-and-reserved.md](references/renames-and-reserved.md) for the `reserved` syntax and worked examples. - -### 4. Run `buf breaking` in CI, With an Explicit Opt-Out for Intentional Breaks - -Detect breaks automatically rather than by review alone. When a break is intentional in a pre-release package, make the exception a reviewable, visible decision (for example a PR label that skips the breaking job for that PR) instead of permanently weakening the `buf breaking` configuration for the whole package. - -Read [references/ci-and-governance.md](references/ci-and-governance.md) for the `buf breaking` invocation, the local git-hook caveat, and how to scope an intentional-break exception. - -### 5. Pre-Release Packages Version Instead of Breaking Silently - -A package name's last component signals its stability (`v1alpha1` -> `v1beta1` -> `v1`, per buf's `PACKAGE_VERSION_SUFFIX` lint rule). A stable package (`v1`) that needs a breaking change gets a new package version, not a break inside the existing one. - -Read [references/package-versioning.md](references/package-versioning.md) for the versioning progression and when a breaking change forces a new package version versus living inside the current pre-release one. - -## Worked Example - -Read [references/example-schema-evolution.md](references/example-schema-evolution.md) to see a field rename, a type change, and a package version bump applied to one schema, each handled the compatible way. - -## Reviewing a Proposed Schema Change - -Read [references/review-checklist.md](references/review-checklist.md) when reviewing a diff to an existing `.proto` file. Each item is a symptom; a failing item is a question about the principle behind it, answered in the linked reference. +1. **Know every compatibility surface.** Binary wire, JSON, and generated source code each have their own rules for what counts as breaking; if anyone consumes JSON, `WIRE` alone is not enough. +2. **A rename keeps the field number and type.** It is binary-compatible but breaks JSON keys, generated code identifiers, and, for an enum value, the JSON serialization of the enum too. +3. **A type change never reuses a field number.** It allocates a new number and reserves the old one; a reserved number or name is never reused, ever. +4. **Run `buf breaking` in CI, with an explicit opt-out for intentional breaks.** Scope the exception to the one change that needs it instead of weakening the configuration permanently. +5. **Pre-release packages version instead of breaking silently.** A stable package that needs a breaking change gets a new package version, not a break inside the existing one. + +## Load a Reference + +| When you are... | Load | +|---|---| +| Deciding which `buf breaking` category to run | [references/compatibility-surfaces.md](references/compatibility-surfaces.md) | +| Renaming a field, message, or enum value | [references/renames-and-reserved.md](references/renames-and-reserved.md) | +| Changing a field's type | [references/renames-and-reserved.md](references/renames-and-reserved.md) | +| Setting up or reviewing breaking-change detection in CI | [references/ci-and-governance.md](references/ci-and-governance.md) | +| Deciding whether a breaking change needs a new package version | [references/package-versioning.md](references/package-versioning.md) | +| Wanting to see a rename, a type change, and a version bump applied to one schema | [references/example-schema-evolution.md](references/example-schema-evolution.md) | +| Reviewing a diff to an existing `.proto` file | [references/review-checklist.md](references/review-checklist.md) | diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md index a569388..5653f06 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md @@ -1,6 +1,16 @@ # Worked Example: Evolving a Project Schema -The same kind of `Project` resource as the [protobuf-design-messages worked example](../../protobuf-design-messages/references/example-project-schema.md), carried through real changes: a rename, a type change, and a breaking change in a stable package. +Use this reference when you want to see a field rename, a type change, and a package version bump applied to one schema, each handled the compatible way. + +The same kind of `Project` resource as the worked example in the protobuf-design-messages skill, carried through real changes: a rename, a type change, and a breaking change in a stable package. + +## Contents + +- Starting Point +- Change 1: a Rename, Handled as a Breaking Change +- Change 2: a Type Change, Handled With `reserved` +- Change 3: the Package Stabilizes, Then Needs a Breaking Change +- What Each Change Demonstrates ## Starting Point @@ -38,7 +48,7 @@ In CI, this PR carries the `breaking-change-acknowledged` label (or whatever a g ## Change 2: a Type Change, Handled With `reserved` -`owner_id` becomes a wrapper message (`UserId`) instead of a bare string, per [protobuf-design-messages](../../protobuf-design-messages/SKILL.md)'s no-primitive-obsession principle. The new field gets a new number; the old number and name are reserved: +`owner_id` becomes a wrapper message (`UserId`) instead of a bare string, per the protobuf-design-messages skill's no-primitive-obsession principle. The new field gets a new number; the old number and name are reserved: ```protobuf package example.projects.v1alpha1; diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md index 4e1d01c..e06fa77 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md @@ -21,7 +21,7 @@ message Project { } ``` -A rename is a `WIRE_JSON`-category and `FILE`/`PACKAGE`-category break even though it is a `WIRE`-category no-op; see [references/compatibility-surfaces.md](compatibility-surfaces.md). If a rename is still necessary (the old name was wrong, or collided with other vocabulary per [protobuf-design-messages](../../protobuf-design-messages/SKILL.md)), treat it as a breaking change requiring the same coordination as any other: a new package version, or a deliberate opt-out in CI for a pre-release package (see [references/ci-and-governance.md](ci-and-governance.md)). +A rename is a `WIRE_JSON`-category and `FILE`/`PACKAGE`-category break even though it is a `WIRE`-category no-op; see [references/compatibility-surfaces.md](compatibility-surfaces.md). If a rename is still necessary (the old name was wrong, or collided with other vocabulary per the protobuf-design-messages skill), treat it as a breaking change requiring the same coordination as any other: a new package version, or a deliberate opt-out in CI for a pre-release package (see [references/ci-and-governance.md](ci-and-governance.md)). ## An Enum Value Rename Breaks JSON Too diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md index f96090a..9478bb3 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md @@ -1,5 +1,7 @@ # Review Checklist +Use this reference when reviewing a diff to an existing `.proto` file. + Each item is a symptom of a principle, not a rule on its own. When an item fails, read the linked reference and judge the change by the principle behind it; a justified exception passes, a compliant change that misses the point does not. ## Compatibility Surfaces ([compatibility-surfaces.md](compatibility-surfaces.md)) From ba8eba26cc41df243098464c591724c8d4bf96a1 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 21:05:03 -0400 Subject: [PATCH 3/5] feat(trogonstack-protobuf): make a type findable from its file name Signed-off-by: Yordis Prieto --- .../skills/protobuf-design-messages/SKILL.md | 4 +- .../references/example-project-schema.md | 2 + .../references/file-layout.md | 46 +++++++++++++++++++ .../references/review-checklist.md | 7 +++ 4 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/file-layout.md diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md index 3d1806a..a12fee5 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md @@ -1,6 +1,6 @@ --- name: protobuf-design-messages -description: "Design or review protobuf message and field definitions: field naming for JSON consumers, identifier and reference value objects, enums, presence, and validation. Use when writing a new .proto message, enum, or field, or reviewing one for naming and shape. Do not use for schema evolution, breaking-change detection, or buf lint/breaking CI setup (use protobuf-evolve-schemas)." +description: "Design or review protobuf message and field definitions: field naming for JSON consumers, identifier and reference value objects, enums, presence, validation, and file and service layout. Use when writing a new .proto file, service, message, enum, or field, or reviewing one for naming and shape. Do not use for schema evolution, breaking-change detection, or buf lint/breaking CI setup (use protobuf-evolve-schemas)." allowed-tools: - AskUserQuestion - Read @@ -20,6 +20,7 @@ A `.proto` file is read by more than the compiler: ProtoJSON consumers see only 4. **Know what the schema can and cannot guarantee.** Editions presence and `protovalidate` constraints are schema-level tools with real limits; some invariants need live state and belong in the service. 5. **Event-sourced contracts separate commands, events, state, and faults.** Commands are imperative, events are past tense, and an aggregate never gets an event for a fact it does not own. 6. **Rename early when words collide with product vocabulary.** The cheapest moment to fix a name is before anything generates code from it. +7. **A file is named after what it defines.** One top-level definition per file in `lower_snake_case`, a service in `_service.proto` named `Service`, and its own `Request` and `Response` for every RPC. ## Load a Reference @@ -33,5 +34,6 @@ A `.proto` file is read by more than the compiler: ProtoJSON consumers see only | Deciding whether a field should be required, or writing a `buf.validate` constraint | [references/presence-and-validation.md](references/presence-and-validation.md) | | Designing commands, events, state, or faults for an event-sourced resource | [references/event-sourced-contracts.md](references/event-sourced-contracts.md) | | A field, message, or package name echoes a term used elsewhere for a different concept | [references/naming-collisions.md](references/naming-collisions.md) | +| Creating a `.proto` file, adding a `service`, or naming RPC request and response messages | [references/file-layout.md](references/file-layout.md) | | Wanting to see every principle applied in one complete `.proto` package | [references/example-project-schema.md](references/example-project-schema.md) | | Reviewing or auditing an existing schema | [references/review-checklist.md](references/review-checklist.md) | diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md index 1ced328..d3ece93 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md @@ -7,6 +7,8 @@ Use this reference when you want to see every principle in [SKILL.md](../SKILL.m - The `.proto` package, in full - What Each Choice Demonstrates +The package is shown as one listing so it reads top to bottom. On disk, principle 7 puts each top-level definition in its own file under `example/projects/v1alpha1/` (`project_id.proto`, `create_project.proto`, `project_created.proto`, and so on). + ```protobuf edition = "2023"; diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/file-layout.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/file-layout.md new file mode 100644 index 0000000..2f08daf --- /dev/null +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/file-layout.md @@ -0,0 +1,46 @@ +# File and Service Layout + +Use this reference when creating a new `.proto` file, adding a `service`, naming RPC request and response messages, or deciding which file a definition belongs in. + +## One Top-Level Definition Per File, Named After It + +Give each top-level `message`, `enum`, and `service` its own file, and name the file after the definition in `lower_snake_case`. A reader looking for `DecideRequest` opens `decide_request.proto`; nobody has to grep to find where a type lives, and a diff that touches one type touches one file. + +```txt +proto/acme/decider/v1/ +├── decider_service.proto # service DeciderService +├── decide_request.proto # message DecideRequest +├── decide_response.proto # message DecideResponse +├── decided_event.proto # message DecidedEvent +└── faults.proto # a closed family of fault messages kept together +``` + +A small family of types that only make sense together (a fault `oneof` and its arms, a value object and the enum it alone uses) may share a file named for the family. Keep that the exception: once a file holds unrelated definitions, its name stops telling the reader what is inside. + +Google's [AIP-191](https://google.aip.dev/191) takes a different default and keeps a service's request and response messages in the same file as the service. Either layout is defensible; pick one per package and apply it to every file in it, so the file tree stays predictable. + +## A Service Lives in `_service.proto` and Is Named `Service` + +The service name ends in `Service` (buf's `SERVICE_SUFFIX` lint rule, part of `STANDARD`), and its file is that name in `lower_snake_case`. The suffix keeps the service from colliding with the resource it serves: `Project` is the message, `ProjectService` is the API over it, and `project.proto` and `project_service.proto` sit side by side without ambiguity. + +```protobuf +// decider_service.proto +service DeciderService { + rpc Decide(DecideRequest) returns (DecideResponse); +} +``` + +## Every RPC Gets Its Own Request and Response + +Name them after the method, `Request` and `Response` (buf's `RPC_REQUEST_STANDARD_NAME` and `RPC_RESPONSE_STANDARD_NAME`), and never share one between RPCs (`RPC_REQUEST_RESPONSE_UNIQUE`), even when today's fields happen to match. A shared request message couples the evolution of two methods: a field one method needs becomes a field the other must ignore forever. Avoid returning `google.protobuf.Empty` for the same reason; an empty `Response` costs nothing and leaves room to grow. + +## The Directory Mirrors the Package + +Every file in package `acme.billing.v1` lives in `acme/billing/v1/`, and every file in that directory declares that package (buf's `PACKAGE_DIRECTORY_MATCH` and `PACKAGE_SAME_DIRECTORY`). The last segment is the version (`PACKAGE_VERSION_SUFFIX`). Never name a file after the version (`v1.proto`); [AIP-191](https://google.aip.dev/191) calls out that it produces confusing imports in generated client libraries. + +## Review Questions + +- Does each file hold one top-level definition, named after it in `lower_snake_case`, or a deliberately grouped family named for the family? +- Does every `service` end in `Service` and live in `_service.proto`? +- Does every RPC have its own `Request` and `Response`, shared with no other RPC? +- Does the directory path match the package, ending in the version segment, with no file named after the version? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md index 9ed1ba6..381ab47 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md @@ -53,3 +53,10 @@ Skip this section for plain CRUD/RPC schemas. - [ ] New names were checked against the product's existing vocabulary, not only the local file - [ ] Any discovered collision was renamed before the package generates code + +## File and Service Layout ([file-layout.md](file-layout.md)) + +- [ ] Each file holds one top-level definition named after it in `lower_snake_case`, or a deliberately grouped family named for the family +- [ ] Every `service` ends in `Service` and lives in `_service.proto` +- [ ] Every RPC has its own `Request` and `Response`, shared with no other RPC +- [ ] The directory path matches the package and ends in the version segment From c70af0b4e841dd92a82a612b47d0b72dfb6fdce0 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 21:08:42 -0400 Subject: [PATCH 4/5] fix(trogonstack-protobuf): keep the guidance consistent with its own rules Signed-off-by: Yordis Prieto --- .../references/enums-and-presets.md | 4 ++-- .../references/example-project-schema.md | 17 ++++++++++------ .../references/naming-collisions.md | 8 ++++---- .../references/ci-and-governance.md | 14 ++++++------- .../references/compatibility-surfaces.md | 20 +++++++++++++++---- .../references/example-schema-evolution.md | 19 +++++++++--------- .../references/renames-and-reserved.md | 8 +++++++- .../references/review-checklist.md | 3 ++- 8 files changed, 59 insertions(+), 34 deletions(-) diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md index abe5b6a..61efb81 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md @@ -45,9 +45,9 @@ message CreateProject { If the service treats `ACCESS_PRESET_UNSPECIFIED` as `ACCESS_PRESET_PUBLIC` "so something reasonable happens," an old client, a client with a bug, or a caller who genuinely forgot the field just made the resource public by omission. The creator never chose that, and the resource the creator meant to keep private is now exposed. The only safe reading of `UNSPECIFIED` for a field like this is "reject the request and ask the caller to choose," never "pick the widest option on their behalf." -Contrast that with a field where `UNSPECIFIED` defaulting to the narrowest, most conservative option would be safe. The service-level decision depends entirely on what gets exposed by resolving the default one way or the other; the schema's job is to make `UNSPECIFIED` a visible, named state so that decision has to be made on purpose rather than falling out of whatever the zero value happens to be. +The rule holds even when a narrow default looks harmless: `UNSPECIFIED` is rejected, never resolved. A default the service picks is a default nobody can see in the schema, and the next reader cannot tell a caller who chose `PRIVATE` from one who sent nothing. If a field genuinely has a safe default, make it an explicit value the caller sends, and let the client library fill it in where a human would otherwise forget. -```protobuf +```cpp // Avoid: service code that treats UNSPECIFIED as a convenient permissive default. switch (request.access_preset()) { case AccessPreset::ACCESS_PRESET_UNSPECIFIED: diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md index d3ece93..aefb94f 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/example-project-schema.md @@ -37,7 +37,9 @@ message NodeId { message Money { int64 amount_micros = 1; - string currency_code = 2 [(buf.validate.field).string.len = 3]; // ISO 4217 + // ISO 4217 alphabetic code. The pattern checks the shape only; membership in + // the ISO 4217 code list is checked by the service, which owns that list. + string currency_code = 2 [(buf.validate.field).string.pattern = "^[A-Z]{3}$"]; } // --- Enums (principle 3: exhaustive choice, UNSPECIFIED is rejected) --- @@ -57,13 +59,15 @@ enum ProjectState { // --- State (folded from events) --- +// Only facts this aggregate's own events keep current. Placement and access +// are recorded once on ProjectCreated, but their later changes are owned +// elsewhere, so folding them in here would freeze a stale copy. A reader +// that needs them joins the owning systems in a read model. message Project { ProjectId project_id = 1; string name = 2; - NodeId parent = 3; - AccessPreset access_preset = 4; - ProjectState state = 5; - Money budget = 6; + ProjectState state = 3; + Money budget = 4; } // --- Commands: imperative (principle 5) --- @@ -145,10 +149,11 @@ message RenameProjectFault { |---|---| | `ProjectId`, `UserId`, `NodeId` as wrapper messages | No primitive obsession | | `NodeId parent` bare, no `_id` suffix | `parent` names hierarchy position, never qualified | -| `Money` with `amount_micros` + `currency_code` | Units and currency travel together, never a bare float | +| `Money` with `amount_micros` + `currency_code` | Units and currency travel together, never a bare float; the schema checks the code's shape, the service checks it is a real ISO 4217 code | | `AccessPreset` zero value rejected via `buf.validate.field.enum` | An omitted choice cannot silently widen access | | `CreateProject.name` marked `required` via `string.min_len = 1` | Schema-checkable invariant expressed portably | | `ProjectCreated` carries `parent` and `initial_access_preset` | Creation-time facts recorded even though later changes are owned elsewhere | +| `Project` state omits `parent` and access | Folded state holds only facts the aggregate's own events keep current | | No `ProjectMoved` or `ProjectAccessChanged` event | The aggregate does not emit events for facts it does not own | | `RenameProjectFault` as a `oneof` | Specific, matchable fault types instead of a generic error | | `package example.projects.v1alpha1` | Pre-release versioning; the protobuf-evolve-schemas skill covers when and how this graduates to `v1beta1` and `v1` | diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md index 1c0ec60..e1afcc1 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/naming-collisions.md @@ -22,14 +22,14 @@ message StartSession { The collision rarely shows up by reading the file the new field lives in; it shows up by checking the new name against every other place the product already uses that word. Before naming a new message, field, or package, search the existing schemas (and the product's domain language more broadly) for the candidate name. A hit does not automatically rule the name out, but it does mean the two concepts need to be distinguishable at a glance, which a shared bare word cannot do. -## The Cheapest Moment to Rename Is Before Anything Generates Code +## The Cheapest Moment to Rename Is Before Anything Depends on the Name -Once generated code exists, a rename is a breaking change (the protobuf-evolve-schemas skill covers why a rename breaks JSON and generated source even when it is wire-compatible). Before that point, a rename costs nothing: it is a text edit in a file nobody depends on yet. +Once anything depends on the schema, a rename is a breaking change (the protobuf-evolve-schemas skill covers why a rename breaks JSON and generated source even when it is wire-compatible). Dependence is not only generated code: a client that reads the JSON form depends on the field names without ever seeing the schema. Before anything depends on the names, a rename costs nothing: it is a text edit in a file nobody reads yet. -This is one of the reasons a package still under active design is worth excluding from code generation entirely (for example, via `buf.gen.yaml` exclusions or simply not wiring the package into the build) until its shape has settled. While a package generates no code, every name in it can change for free; the moment it does, every rename has a cost that scales with how many consumers pulled in the generated bindings. +This is one of the reasons a package still under active design is worth excluding from code generation entirely (for example, via `buf.gen.yaml` exclusions or simply not wiring the package into the build) until its shape has settled. Excluding codegen keeps generated bindings from depending on the names, but it does not prove nothing else does: confirm no service serves the package over JSON or binary and no client reads its payloads. Only then can every name change for free; the moment any consumer appears, every rename has a cost that scales with how many depend on it. ## Review Questions - Does a new name reuse a word the product already assigns to a different concept elsewhere? - Has the name been checked against the broader schema set, not just the file it lives in? -- Is the package still pre-codegen, meaning names in it can still be fixed for free? +- Is the package still free of consumers on every surface (generated code, JSON, binary), meaning names in it can still be fixed for free? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md index b8c6fe1..3d9599f 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md @@ -7,17 +7,17 @@ Use this reference when setting up or reviewing breaking-change detection in CI, Compare the working tree against the base branch using buf's git input syntax (see [buf docs: Breaking usage](https://buf.build/docs/breaking/usage/) and [buf docs: Inputs](https://buf.build/docs/reference/inputs/)): ```bash -buf breaking --against '.git#branch=main' +buf breaking --against ".git#branch=${BASE_BRANCH}" ``` -Run this in CI on every pull request, against whatever branch the PR targets, so a break is caught before merge rather than discovered by a downstream consumer after release. +Set `BASE_BRANCH` to the branch the pull request targets (in GitHub Actions, `github.base_ref`), never a hard-coded `main`: a PR into a release or feature branch must be compared against that branch. The checkout also needs that branch's history fetched (a shallow clone of only the PR head has nothing to compare against). Run this in CI on every pull request, so a break is caught before merge rather than discovered by a downstream consumer after release. Locally, substitute the branch you will merge into. ## Local Tip: Breaking Global Git Config Running `buf breaking` against a git ref clones into a temporary directory and can trip over global git hooks or config that assume a normal working tree (commit signing hooks, credential helpers, or custom templates). If the local run fails for reasons that look like git configuration rather than an actual schema break, isolate it from global config: ```bash -GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null buf breaking --against '.git#branch=main' +GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null buf breaking --against '.git#branch=main' # main: the branch you will merge into ``` This is a local troubleshooting step, not something to bake into CI, where the environment is already clean. @@ -37,18 +37,18 @@ Prefer an opt-out that is explicit and scoped to the one PR that needs it: a PR if pr.has_label("breaking-change-acknowledged"): skip buf breaking else: - run buf breaking --against '.git#branch=main' + run buf breaking --against ".git#branch=${pr.base_branch}" ``` The exact mechanism (a label, a required commit trailer, a specific reviewer's approval) matters less than the property it has to preserve: the opt-out is a per-change, reviewed decision, not a standing exemption. -## Pre-Codegen Packages Cost Nothing to Break +## Packages With No Consumers Cost Nothing to Break -A package that is excluded from code generation entirely (nothing in the build consumes its generated bindings yet) has no consumers to break, so `buf breaking` findings against it are not really "breaks," they are just diff noise. It is reasonable to exclude such a package from the breaking-change check altogether until it is wired into code generation for the first time; at that point, treat its current shape as the new baseline and start enforcing `buf breaking` on it like any other package. +A package with no consumers on any compatibility surface has nobody to break, so `buf breaking` findings against it are diff noise rather than breaks. Excluding it from code generation is not evidence of that on its own: a service can already serve it as JSON, or a client can already read its binary payloads, without any generated bindings in this build. Exclude a package from the breaking-change check only after confirming it has no consumers on any surface, and record that confirmation where reviewers will see it. The moment it gains its first consumer (it is wired into code generation, served, or published), treat its current shape as the new baseline and start enforcing `buf breaking` on it like any other package. ## Review Questions - Does CI run `buf breaking` against the correct base branch on every pull request? - Is any package's breaking-change check permanently disabled or weakened, rather than opted out of per-change? - When a break was intentional, is there a visible, reviewed record of that decision on the PR, rather than a silent config change? -- Is a package excluded from breaking-change checks actually pre-codegen, or does it already have consumers? +- Is a package excluded from breaking-change checks confirmed to have no consumers on any surface (generated code, JSON, binary)? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md index 9157d1f..1adf6f0 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md @@ -23,12 +23,24 @@ A change can be safe for one surface and a break for another. [buf's breaking ch `WIRE` alone only protects binary gRPC consumers. If the schema is exposed through a JSON-speaking transport (gRPC-Gateway, Connect, a webhook payload, a REST-ish proxy, or any client library that round-trips through JSON instead of binary), `WIRE_JSON` is the floor, not `WIRE`. A field rename passes a `WIRE`-only check and fails `WIRE_JSON`, because the wire format never cared about the name but the JSON key is the name. -```text -# A rename is wire-compatible, JSON-incompatible. -buf breaking --against '.git#branch=main' (category: WIRE) -> passes -buf breaking --against '.git#branch=main' (category: WIRE_JSON) -> fails: field name change +`buf breaking` takes its category from `breaking.use` in `buf.yaml`, and an unset `breaking` section means `FILE`. The same rename gives a different result under each configuration: + +```yaml +# buf.yaml: a field rename passes. Only binary decoding is checked. +breaking: + use: + - WIRE +``` + +```yaml +# buf.yaml: the same rename fails with FIELD_SAME_JSON_NAME. +breaking: + use: + - WIRE_JSON ``` +Under the default (`FILE`) the rename fails too, on both the JSON check and the generated-code name check. + `FILE` and `PACKAGE` are supersets of `WIRE_JSON` (buf's docs say so in the `FIELD_SAME_JSON_NAME` rule), so when generated source code is a first-class consumer, which it is in most codebases, pick `FILE` or `PACKAGE` instead of `WIRE_JSON`; you keep every JSON check and add the generated-code ones. Pick one category per module rather than combining them. ## Review Questions diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md index 5653f06..87a8609 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md @@ -39,6 +39,7 @@ package example.projects.v1alpha1; message Project { ProjectId project_id = 1; + reserved title; string name = 2; // renamed from `title`; breaking, acceptable pre-v1 string owner_id = 3; } @@ -48,7 +49,7 @@ In CI, this PR carries the `breaking-change-acknowledged` label (or whatever a g ## Change 2: a Type Change, Handled With `reserved` -`owner_id` becomes a wrapper message (`UserId`) instead of a bare string, per the protobuf-design-messages skill's no-primitive-obsession principle. The new field gets a new number; the old number and name are reserved: +`owner_id` becomes a wrapper message (`UserId`) instead of a bare string, per the protobuf-design-messages skill's no-primitive-obsession principle. The new field gets a new number and the old number is reserved. The name stays `owner_id`, because a reference keeps its suffix for JSON readers who never see the type, so the name is not reserved (reserving a name the message still uses does not compile): ```protobuf package example.projects.v1alpha1; @@ -59,14 +60,14 @@ message UserId { message Project { reserved 3; - reserved "owner_id"; + reserved title; ProjectId project_id = 1; string name = 2; - UserId owner = 4; + UserId owner_id = 4; } ``` -Field 4 (`owner`) is deliberately not named `owner_id`: since it is now a typed reference rather than an identifier, `owner_ref` or a bare role name would both be defensible depending on whether `UserId` is treated as an id-wrapper or a richer reference; the point demonstrated here is the mechanic (new number, reserved old number and name), not the specific name chosen. +The JSON key stays `ownerId`, but its value changes from a string to an object (`{"value": "..."}`), so JSON consumers still break; the new field number protects binary consumers, not JSON ones. That is acceptable here for the same reason as the rename: the package is pre-release and the break is acknowledged in CI. ## Change 3: the Package Stabilizes, Then Needs a Breaking Change @@ -78,10 +79,10 @@ package example.projects.v1; message Project { ProjectId project_id = 1; - string name = 2; - UserId owner = 4; reserved 3; - reserved "owner_id"; + reserved title; + string name = 2; + UserId owner_id = 4; } ``` @@ -93,7 +94,7 @@ message Project { ProjectId project_id = 1; string display_name = 2; string slug = 3; - UserId owner = 4; + UserId owner_id = 4; } ``` @@ -102,7 +103,7 @@ message Project { | Change | Principle | |---|---| | `title` -> `name` | A rename is wire-safe, JSON/codegen-breaking; acceptable pre-`v1`, made visible via a reviewed CI opt-out | -| `owner_id` (string) -> `owner` (`UserId`) | A type change gets a new field number; the old number and name are `reserved`, never reused | +| `string owner_id` -> `UserId owner_id` | A type change gets a new field number and the old number is `reserved`; the name stays, because a reference keeps its suffix | | `v1alpha1` -> `v1beta1` -> `v1` -> `v2` | A stable package's breaking change produces a new package version instead of mutating the one consumers already depend on | Note: `example.projects.*` does not refer to any real service; it is a placeholder package name for this worked example. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md index e06fa77..c0539e5 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md @@ -16,13 +16,17 @@ message Project { // JSON breaks: a client sending {"title": "..."} now sets nothing, and a // client reading the response no longer finds a "title" key. // Generated code breaks: callers of project.title() / project.Title no longer compile. +// The old name is reserved so nothing reintroduces "title" with another meaning. message Project { + reserved title; string name = 1; } ``` A rename is a `WIRE_JSON`-category and `FILE`/`PACKAGE`-category break even though it is a `WIRE`-category no-op; see [references/compatibility-surfaces.md](compatibility-surfaces.md). If a rename is still necessary (the old name was wrong, or collided with other vocabulary per the protobuf-design-messages skill), treat it as a breaking change requiring the same coordination as any other: a new package version, or a deliberate opt-out in CI for a pre-release package (see [references/ci-and-governance.md](ci-and-governance.md)). +Reserved names in these examples use editions syntax, a bare identifier. In proto2 and proto3 files the name is quoted (`reserved "title";`). Field and enum-value names can be reserved; message names cannot. + ## An Enum Value Rename Breaks JSON Too Enum values serialize to JSON by name, not by number, so renaming an enum value is a JSON break even though the underlying integer is unchanged: @@ -38,6 +42,7 @@ enum ProjectState { // that was matching the string "PROJECT_STATE_DONE" now sees // "PROJECT_STATE_COMPLETED" and silently fails to match. enum ProjectState { + reserved PROJECT_STATE_DONE; PROJECT_STATE_UNSPECIFIED = 0; PROJECT_STATE_COMPLETED = 1; } @@ -70,7 +75,8 @@ A `reserved` statement is permanent. Removing it to "free up" the number for an ## Review Questions -- Does the diff rename a field, message, or enum value without reserving the old name/number and without a plan for the resulting JSON/generated-code break? +- Does the diff rename a field or enum value without reserving the old name, or without a plan for the resulting JSON/generated-code break? +- Does the diff rename a message? Message names cannot be reserved, so the plan has to cover every consumer that names the type: generated code, `google.protobuf.Any` type URLs, and anything that stores the full name. - Does the diff change a field's type at the same field number, instead of allocating a new number and reserving the old one? - Does the diff remove a `reserved` statement to reuse a number or name? - Does the diff rename an enum value, and has it been checked against `WIRE_JSON`, not just `WIRE`? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md index 9478bb3..8cdb581 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md @@ -12,6 +12,7 @@ Each item is a symptom of a principle, not a rule on its own. When an item fails ## Renames and Reserved Fields ([renames-and-reserved.md](renames-and-reserved.md)) - [ ] No field, message, or enum value was renamed without accounting for the JSON/generated-code break that follows +- [ ] A renamed field or enum value has its old name `reserved` (message names cannot be reserved) - [ ] No field's type changed at its existing field number; a new number was allocated instead - [ ] The old field number is `reserved`, and the old name is `reserved` unless it stays attached to the new number - [ ] No `reserved` statement was removed to reuse a number or name @@ -21,7 +22,7 @@ Each item is a symptom of a principle, not a rule on its own. When an item fails - [ ] `buf breaking` runs in CI against the correct base branch - [ ] No package's breaking-change check was permanently disabled or weakened to let one change through - [ ] An intentional break carries a visible, per-change, reviewed opt-out (e.g. a PR label), not a standing exemption -- [ ] Any package excluded from breaking-change checks is actually pre-codegen, with no consumers yet +- [ ] Any package excluded from breaking-change checks is confirmed to have no consumers on any surface (generated code, JSON, binary) ## Package Versioning ([package-versioning.md](package-versioning.md)) From 505aeaa5ccb467cdecf60074f81edf4e00b57899 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Sat, 10 Oct 2026 21:25:47 -0400 Subject: [PATCH 5/5] refactor(trogonstack-protobuf): leave mechanical checks to buf Signed-off-by: Yordis Prieto --- .../skills/protobuf-design-messages/SKILL.md | 4 ++ .../references/enums-and-presets.md | 3 +- .../references/file-layout.md | 36 ++++------- .../references/presence-and-validation.md | 4 +- .../references/review-checklist.md | 18 +++--- .../skills/protobuf-evolve-schemas/SKILL.md | 4 ++ .../references/ci-and-governance.md | 62 ++++++++++--------- .../references/compatibility-surfaces.md | 4 +- .../references/example-schema-evolution.md | 2 +- .../references/package-versioning.md | 2 +- .../references/renames-and-reserved.md | 57 ++++++----------- .../references/review-checklist.md | 18 +++--- 12 files changed, 97 insertions(+), 117 deletions(-) diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md index a12fee5..482026b 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/SKILL.md @@ -12,6 +12,10 @@ allowed-tools: A `.proto` file is read by more than the compiler: ProtoJSON consumers see only field names, never types, and reviewers see only names and comments, never runtime behavior. A field, message, or enum that relies on tribal knowledge to interpret correctly is a defect, not a style nit. Apply the principles below in order; a design that fails an earlier one makes the later ones moot. +## Run buf First + +Run `buf format -w` and `buf lint` (with `STANDARD`, buf's default) before anything below, and fix every finding the tool reports. Naming case, file/package/directory layout, enum zero-value and prefix conventions, service and RPC naming, and element ordering are mechanical checks a linter makes faster and more reliably than a manual pass; do not re-check them by hand. The principles below cover what `buf lint` has no way to judge: whether a name means what it says, whether a value is a primitive in disguise, and whether a shape fits the domain. + ## Principles 1. **Names must survive schema-less JSON readers.** A field whose value points at something stored or owned elsewhere keeps a suffix in its name even when its type already says so. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md index 61efb81..32734aa 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/enums-and-presets.md @@ -83,7 +83,8 @@ When two or more fields are mutually exclusive ways of saying the same thing (a ## Review Questions -- Does every enum's zero value end in `_UNSPECIFIED`, and are all values prefixed with the enum name? +(Zero-value naming and value prefixing are checked by `buf lint`'s `ENUM_ZERO_VALUE_SUFFIX`/`ENUM_VALUE_PREFIX`; run the tool instead of eyeballing a diff.) + - Is there service code anywhere that treats an enum's `UNSPECIFIED` value as a usable default rather than rejecting it? - Would resolving `UNSPECIFIED` to a default ever widen access, cost, or scope beyond what the caller explicitly chose? - Is there a boolean field describing something that could plausibly need a third state? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/file-layout.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/file-layout.md index 2f08daf..ce90fb8 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/file-layout.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/file-layout.md @@ -2,9 +2,15 @@ Use this reference when creating a new `.proto` file, adding a `service`, naming RPC request and response messages, or deciding which file a definition belongs in. +## Let `buf lint` Handle Naming and Directory Layout + +Enable `STANDARD` (buf's default lint categories) and let it enforce the mechanics: `SERVICE_SUFFIX` (every service ends in `Service`), `RPC_REQUEST_STANDARD_NAME` and `RPC_RESPONSE_STANDARD_NAME` (every RPC has its own `Request`/`Response`), `RPC_REQUEST_RESPONSE_UNIQUE` (no RPC shares one with another), `FILE_LOWER_SNAKE_CASE`, and `PACKAGE_DIRECTORY_MATCH`/`PACKAGE_SAME_DIRECTORY`/`PACKAGE_VERSION_SUFFIX` (directory mirrors the package, ending in the version segment). Run `buf lint` instead of checking these by hand. + +`RPC_REQUEST_RESPONSE_UNIQUE` and `RPC_RESPONSE_STANDARD_NAME` already rule out returning bare `google.protobuf.Empty`: a response type must be named `Response` and belong to one RPC alone, so every RPC gets a real, growable response message for free. The judgment call that remains is just not leaving that message artificially thin out of habit; it costs nothing to add fields later if it already exists. + ## One Top-Level Definition Per File, Named After It -Give each top-level `message`, `enum`, and `service` its own file, and name the file after the definition in `lower_snake_case`. A reader looking for `DecideRequest` opens `decide_request.proto`; nobody has to grep to find where a type lives, and a diff that touches one type touches one file. +Give each top-level `message`, `enum`, and `service` its own file, named after the definition. A reader looking for `DecideRequest` opens `decide_request.proto`; nobody has to grep to find where a type lives, and a diff that touches one type touches one file. ```txt proto/acme/decider/v1/ @@ -17,30 +23,10 @@ proto/acme/decider/v1/ A small family of types that only make sense together (a fault `oneof` and its arms, a value object and the enum it alone uses) may share a file named for the family. Keep that the exception: once a file holds unrelated definitions, its name stops telling the reader what is inside. -Google's [AIP-191](https://google.aip.dev/191) takes a different default and keeps a service's request and response messages in the same file as the service. Either layout is defensible; pick one per package and apply it to every file in it, so the file tree stays predictable. - -## A Service Lives in `_service.proto` and Is Named `Service` - -The service name ends in `Service` (buf's `SERVICE_SUFFIX` lint rule, part of `STANDARD`), and its file is that name in `lower_snake_case`. The suffix keeps the service from colliding with the resource it serves: `Project` is the message, `ProjectService` is the API over it, and `project.proto` and `project_service.proto` sit side by side without ambiguity. - -```protobuf -// decider_service.proto -service DeciderService { - rpc Decide(DecideRequest) returns (DecideResponse); -} -``` - -## Every RPC Gets Its Own Request and Response - -Name them after the method, `Request` and `Response` (buf's `RPC_REQUEST_STANDARD_NAME` and `RPC_RESPONSE_STANDARD_NAME`), and never share one between RPCs (`RPC_REQUEST_RESPONSE_UNIQUE`), even when today's fields happen to match. A shared request message couples the evolution of two methods: a field one method needs becomes a field the other must ignore forever. Avoid returning `google.protobuf.Empty` for the same reason; an empty `Response` costs nothing and leaves room to grow. - -## The Directory Mirrors the Package - -Every file in package `acme.billing.v1` lives in `acme/billing/v1/`, and every file in that directory declares that package (buf's `PACKAGE_DIRECTORY_MATCH` and `PACKAGE_SAME_DIRECTORY`). The last segment is the version (`PACKAGE_VERSION_SUFFIX`). Never name a file after the version (`v1.proto`); [AIP-191](https://google.aip.dev/191) calls out that it produces confusing imports in generated client libraries. +Google's [AIP-191](https://google.aip.dev/191) takes a different default and keeps a service's request and response messages in the same file as the service. Either layout is defensible; pick one per package and apply it to every file in it, so the file tree stays predictable. `buf lint` enforces the naming either way; this choice is about where the file boundary falls, which the tool has no opinion on. ## Review Questions -- Does each file hold one top-level definition, named after it in `lower_snake_case`, or a deliberately grouped family named for the family? -- Does every `service` end in `Service` and live in `_service.proto`? -- Does every RPC have its own `Request` and `Response`, shared with no other RPC? -- Does the directory path match the package, ending in the version segment, with no file named after the version? +- Does each file hold one top-level definition, or a deliberately grouped family named for the family, rather than an arbitrary mix? +- Was a layout (one-definition-per-file or AIP-191) chosen once and applied consistently across the package? +- Does `buf lint` pass with `STANDARD` enabled, so naming and directory layout are confirmed by the tool, not by eye? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/presence-and-validation.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/presence-and-validation.md index 15a25ab..0eb8d5c 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/presence-and-validation.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/presence-and-validation.md @@ -14,7 +14,7 @@ Protobuf Editions (2023, 2024) replace proto2's `required`/`optional` labels and ## `LEGACY_REQUIRED` Is proto2 `required`, and proto2 `required` Is Discouraged -`LEGACY_REQUIRED` exists so proto2 files with `required` fields can convert to Editions syntax without changing wire behavior. It is not a recommended way to express a new invariant. Protobuf's own guidance on `required` fields states it plainly ([protobuf.dev: Dos and Don'ts](https://protobuf.dev/programming-guides/dos-donts/)): +`buf lint`'s `FIELD_NOT_REQUIRED` rule (part of `BASIC`/`STANDARD`) flags every `required`/`LEGACY_REQUIRED` field automatically; run `buf lint` instead of grepping for it. The judgment call the tool cannot make is whether a flagged field is acceptable: `LEGACY_REQUIRED` exists so proto2 files with `required` fields can convert to Editions syntax without changing wire behavior, and it is not a recommended way to express a new invariant. Protobuf's own guidance on `required` fields states it plainly ([protobuf.dev: Dos and Don'ts](https://protobuf.dev/programming-guides/dos-donts/)): > "Required fields are considered harmful by so many they were removed from proto3 completely. Make all fields optional or repeated. You never know how long a message type is going to last and whether someone will be forced to fill in your required field with an empty string or zero in four years when it's no longer logically required but the proto still says it is." @@ -38,7 +38,7 @@ message CreateProject { } ``` -`required` means something different depending on the field's presence: on a field with explicit presence, it checks that the field was set at all, even to its default value; on a field with implicit presence (plain proto3 scalars), it checks that the field is not the default value, since there is no other way to observe "was it set." `enum.defined_only` rejects any integer not defined on the enum; combining it with `not_in: [0]` (or `in` listing only the allowed non-zero values) is how to reject `_UNSPECIFIED` explicitly, enforcing principle 3 in [SKILL.md](../SKILL.md) at the schema layer. Message-level cross-field invariants are expressed with CEL expressions under `(buf.validate.message).cel`. +`required` means something different depending on the field's presence: on a field with explicit presence, it checks that the field was set at all, even to its default value; on a field with implicit presence (plain proto3 scalars), it checks that the field is not the default value, since there is no other way to observe "was it set." `enum.defined_only` rejects any integer not defined on the enum; combining it with `not_in: [0]` (or `in` listing only the allowed non-zero values) is how to reject `_UNSPECIFIED` explicitly, enforcing principle 3 in [SKILL.md](../SKILL.md) at the schema layer. Message-level cross-field invariants are expressed with CEL expressions under `(buf.validate.message).cel`. `buf lint`'s `PROTOVALIDATE` rule (part of `STANDARD`) checks at lint time that these annotations are themselves well-formed; it says nothing about whether a validator actually runs them, which is the judgment call below. **The caveat that matters:** `buf.validate` annotations are declarations, not enforcement. They do nothing on their own; a `buf.validate.field` constraint is only checked where something in the stack actually constructs and runs a protovalidate validator (typically as an interceptor in a gRPC/Connect server, or an explicit call in application code). A client that never wires up protovalidate can still construct and send a message that violates every constraint in the file. Treat `buf.validate` annotations as documentation of intent that becomes an enforced invariant only where the runtime is actually plugged in, and verify that it is plugged in on every path that accepts untrusted input, not just the one it was first added to. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md index 381ab47..b0dc008 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-design-messages/references/review-checklist.md @@ -2,7 +2,9 @@ Use this reference when reviewing or auditing message and field definitions. -Each item is a symptom of a principle, not a rule on its own. When an item fails, read the linked reference and judge the schema by the principle behind it; a justified exception passes, a compliant schema that misses the point does not. +- [ ] `buf format -d --exit-code` and `buf lint` (`STANDARD`) both pass + +Each remaining item is a symptom of a principle `buf lint` cannot check, not a rule on its own. When an item fails, read the linked reference and judge the schema by the principle behind it; a justified exception passes, a compliant schema that misses the point does not. ## JSON Field Naming ([json-field-naming.md](json-field-naming.md)) @@ -27,14 +29,14 @@ Each item is a symptom of a principle, not a rule on its own. When an item fails ## Enums and Presets ([enums-and-presets.md](enums-and-presets.md)) -- [ ] Every enum's zero value is named `_UNSPECIFIED` -- [ ] Every enum value is prefixed with the enum's name - [ ] Service code rejects `UNSPECIFIED` rather than resolving it to a default, especially a permissive one - [ ] No boolean field models a concept that could plausibly need a third state +(Zero-value naming and value prefixing are `ENUM_ZERO_VALUE_SUFFIX`/`ENUM_VALUE_PREFIX`, caught by `buf lint`.) + ## Presence and Validation ([presence-and-validation.md](presence-and-validation.md)) -- [ ] No field uses `LEGACY_REQUIRED` (or proto2 `required`) in a stable, non-pre-release package +- [ ] Any `LEGACY_REQUIRED`/`required` field flagged by `buf lint`'s `FIELD_NOT_REQUIRED` is a deliberate, visible pre-release decision, not an oversight - [ ] Schema-checkable invariants (presence, range, pattern, enum exclusion, cross-field rules) carry `buf.validate` annotations - [ ] Every `buf.validate` annotation is backed by a validator actually running on every path that accepts the message - [ ] No invariant that needs live state (uniqueness, existence, authorization) is expressed only as a schema-level constraint @@ -56,7 +58,7 @@ Skip this section for plain CRUD/RPC schemas. ## File and Service Layout ([file-layout.md](file-layout.md)) -- [ ] Each file holds one top-level definition named after it in `lower_snake_case`, or a deliberately grouped family named for the family -- [ ] Every `service` ends in `Service` and lives in `_service.proto` -- [ ] Every RPC has its own `Request` and `Response`, shared with no other RPC -- [ ] The directory path matches the package and ends in the version segment +- [ ] Each file holds one top-level definition, or a deliberately grouped family named for the family, rather than an arbitrary mix +- [ ] One layout (one-definition-per-file or AIP-191) is applied consistently across the package + +(Service suffix, RPC request/response naming and uniqueness, file naming case, and package/directory match are `buf lint` `STANDARD` rules; see [file-layout.md](file-layout.md) for the rule IDs.) diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md index e736b1d..36eb3e5 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/SKILL.md @@ -12,6 +12,10 @@ allowed-tools: A `.proto` file has audiences that can each break independently: the binary wire, JSON consumers, and generated source code. "Non-breaking" has no meaning until you say non-breaking for whom, so check every change against every compatibility surface that applies to the package's actual consumers, not just the one that is easiest to verify. +## Run buf First + +Run `buf format -w`, `buf lint` (`STANDARD`), and `buf breaking` against the base branch, using the category that matches this package's actual consumers (see [references/compatibility-surfaces.md](references/compatibility-surfaces.md)). Fix every finding the tool reports; do not re-derive by hand what `buf breaking` already flags (a rename, a type change, a deleted field, a removed `reserved`). The principles below cover what the tool cannot decide: which category matches your consumers, whether a flagged break is actually acceptable, and when a break means a new package version instead of an opt-out. + ## Principles 1. **Know every compatibility surface.** Binary wire, JSON, and generated source code each have their own rules for what counts as breaking; if anyone consumes JSON, `WIRE` alone is not enough. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md index 3d9599f..3e97514 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/ci-and-governance.md @@ -2,45 +2,51 @@ Use this reference when setting up or reviewing breaking-change detection in CI, or when a break is genuinely intentional and needs a way through. -## The Command +## On GitHub, Use `bufbuild/buf-action` + +[`bufbuild/buf-action@v1`](https://github.com/bufbuild/buf-action) runs build, format, lint, and breaking checks in one step, and already implements the reviewable opt-out below as a built-in PR label, so there is nothing to hand-roll: + +```yaml +name: buf +on: + pull_request: + types: [opened, synchronize, reopened, labeled, unlabeled] +permissions: + contents: read + pull-requests: write +jobs: + buf: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: bufbuild/buf-action@v1 +``` -Compare the working tree against the base branch using buf's git input syntax (see [buf docs: Breaking usage](https://buf.build/docs/breaking/usage/) and [buf docs: Inputs](https://buf.build/docs/reference/inputs/)): +`breaking_against` defaults to the pull request's base branch (or the commit before the push outside a PR), so it does not need to be set for the common case. Omit `token` (the BSR authentication input) entirely unless this workflow also publishes to the Buf Schema Registry; it has no role in lint, format, or breaking checks. -```bash -buf breaking --against ".git#branch=${BASE_BRANCH}" -``` +### The Opt-Out Is the `buf skip breaking` Label, Not a Custom One -Set `BASE_BRANCH` to the branch the pull request targets (in GitHub Actions, `github.base_ref`), never a hard-coded `main`: a PR into a release or feature branch must be compared against that branch. The checkout also needs that branch's history fetched (a shallow clone of only the PR head has nothing to compare against). Run this in CI on every pull request, so a break is caught before merge rather than discovered by a downstream consumer after release. Locally, substitute the branch you will merge into. +`buf-action` skips breaking-change detection on any pull request carrying the label `buf skip breaking`. That already has the property a hand-written opt-out has to be engineered for: the exception is visible on the PR itself, a reviewer has to apply the label before the break ships, and the next PR to the same package is checked normally again because nothing in config was weakened. Do not invent a repository-specific label or a config-level exclusion for this; use the one the action already recognizes. -## Local Tip: Breaking Global Git Config +## Local Use and Non-GitHub CI -Running `buf breaking` against a git ref clones into a temporary directory and can trip over global git hooks or config that assume a normal working tree (commit signing hooks, credential helpers, or custom templates). If the local run fails for reasons that look like git configuration rather than an actual schema break, isolate it from global config: +Outside `buf-action` (a local check, or a CI system other than GitHub Actions), run the command directly against the base branch using buf's git input syntax (see [buf docs: Breaking usage](https://buf.build/docs/breaking/usage/) and [buf docs: Inputs](https://buf.build/docs/reference/inputs/)): ```bash -GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null buf breaking --against '.git#branch=main' # main: the branch you will merge into +buf breaking --against ".git#branch=${BASE_BRANCH}" ``` -This is a local troubleshooting step, not something to bake into CI, where the environment is already clean. - -## When a Break Is Intentional - -A pre-release package (`v1alpha1`, see [references/package-versioning.md](package-versioning.md)) is explicitly allowed to break while its shape is being designed. The failure mode to avoid is making that exception permanent and invisible: weakening the `buf breaking` configuration for the package (removing it from the checked inputs, or lowering its category) silences every future break in that package too, intentional or not, and nobody reviewing a later PR will know the guardrail is gone. +Set `BASE_BRANCH` to the branch being merged into, never a hard-coded `main`, and make sure that branch's history is actually fetched (a shallow clone of only the PR head has nothing to compare against). -Prefer an opt-out that is explicit and scoped to the one PR that needs it: a PR label (for example `breaking-change-acknowledged`) that the CI breaking-change job checks for and skips only when present, so: +### Local Tip: Breaking Global Git Config -- The exception is visible in the PR itself, not buried in a config file diff. -- A reviewer has to see and approve the label before the break ships. -- The next PR to this package is checked normally again; nothing was permanently turned off. +Running `buf breaking` against a git ref clones into a temporary directory and can trip over global git hooks or config that assume a normal working tree (commit signing hooks, credential helpers, or custom templates). If a local run fails for reasons that look like git configuration rather than an actual schema break, isolate it from global config: -```text -# CI job pseudocode: skip only this run, only when a human applied the label. -if pr.has_label("breaking-change-acknowledged"): - skip buf breaking -else: - run buf breaking --against ".git#branch=${pr.base_branch}" +```bash +GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null buf breaking --against '.git#branch=main' # main: the branch you will merge into ``` -The exact mechanism (a label, a required commit trailer, a specific reviewer's approval) matters less than the property it has to preserve: the opt-out is a per-change, reviewed decision, not a standing exemption. +This is a local troubleshooting step, not something to bake into CI, where the environment is already clean. ## Packages With No Consumers Cost Nothing to Break @@ -48,7 +54,7 @@ A package with no consumers on any compatibility surface has nobody to break, so ## Review Questions -- Does CI run `buf breaking` against the correct base branch on every pull request? -- Is any package's breaking-change check permanently disabled or weakened, rather than opted out of per-change? -- When a break was intentional, is there a visible, reviewed record of that decision on the PR, rather than a silent config change? +- Does CI run breaking-change detection (`buf-action` or the CLI) against the correct base branch on every pull request? +- Is any package's breaking-change check permanently disabled or weakened, rather than opted out of per-change via the `buf skip breaking` label? +- When a break was intentional, did a reviewer apply the label and approve it on the PR, rather than a silent config change? - Is a package excluded from breaking-change checks confirmed to have no consumers on any surface (generated code, JSON, binary)? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md index 1adf6f0..f371613 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/compatibility-surfaces.md @@ -33,13 +33,13 @@ breaking: ``` ```yaml -# buf.yaml: the same rename fails with FIELD_SAME_JSON_NAME. +# buf.yaml: the same rename fails with FIELD_SAME_NAME. breaking: use: - WIRE_JSON ``` -Under the default (`FILE`) the rename fails too, on both the JSON check and the generated-code name check. +Under the default (`FILE`) the rename fails too, on both the JSON check and the generated-code name check. (A `json_name` override alone, with the field name unchanged, is what `FIELD_SAME_JSON_NAME` catches instead.) `FILE` and `PACKAGE` are supersets of `WIRE_JSON` (buf's docs say so in the `FIELD_SAME_JSON_NAME` rule), so when generated source code is a first-class consumer, which it is in most codebases, pick `FILE` or `PACKAGE` instead of `WIRE_JSON`; you keep every JSON check and add the generated-code ones. Pick one category per module rather than combining them. diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md index 87a8609..fea0237 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/example-schema-evolution.md @@ -45,7 +45,7 @@ message Project { } ``` -In CI, this PR carries the `breaking-change-acknowledged` label (or whatever a given repository's equivalent is) so the `buf breaking` job's finding is reviewed and consciously accepted rather than the check being disabled for the package; see [references/ci-and-governance.md](ci-and-governance.md). +In CI, this PR carries the `buf skip breaking` label so `buf-action`'s breaking-change finding is reviewed and consciously accepted rather than the check being disabled for the package; see [references/ci-and-governance.md](ci-and-governance.md). ## Change 2: a Type Change, Handled With `reserved` diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/package-versioning.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/package-versioning.md index e9527db..efbcd12 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/package-versioning.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/package-versioning.md @@ -4,7 +4,7 @@ Use this reference for the versioning progression a package's name signals, and ## The Progression -buf's `PACKAGE_VERSION_SUFFIX` lint rule requires the last component of a package name to be a version in one of these forms: `v\d+`, `v\d+test.*`, `v\d+(alpha|beta)\d*`, or `v\d+p\d+(alpha|beta)\d*` (see [buf docs: Lint rules](https://buf.build/docs/lint/rules/)). In practice, most packages move through: +`buf lint`'s `PACKAGE_VERSION_SUFFIX` rule (part of `STANDARD`) already requires the package name to end in a valid version segment; run the tool instead of checking the format by hand. What it cannot tell you is which stage a package should be at, or when to move to the next one. In practice, most packages move through: ```text example.projects.v1alpha1 -> example.projects.v1beta1 -> example.projects.v1 diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md index c0539e5..80f901f 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/renames-and-reserved.md @@ -4,79 +4,58 @@ Use this reference when renaming a field, message, or enum value, when changing ## A Field Rename Keeps the Number and Type -The binary wire format encodes field numbers, not field names; a decoder that has never seen the new name still decodes the message correctly as long as the number and wire type are unchanged. JSON and generated code are not so forgiving: the JSON key is the field name (or `json_name`), and generated identifiers are derived from the field name in every target language. +`buf breaking`'s `FIELD_SAME_NAME` (in `FILE`, `PACKAGE`, and `WIRE_JSON`) flags any field rename; run it instead of tracing renames by eye. What the tool cannot tell you is what to do once it fires: a rename is binary-compatible (the wire format encodes field numbers, not names) but breaks JSON keys and generated-code identifiers, so whether to proceed is a judgment call, not a lint failure to silence. -```protobuf -// Before -message Project { - string title = 1; -} +The part the tool does not check at all: reserving the old name. A rename is not a deletion, so nothing forces this, but leaving the old name free lets a later field reuse it for a different meaning, confusing anyone reading the history or anything that stored the old name: -// After: renamed title -> name. Binary still decodes (field 1 unchanged). -// JSON breaks: a client sending {"title": "..."} now sets nothing, and a -// client reading the response no longer finds a "title" key. -// Generated code breaks: callers of project.title() / project.Title no longer compile. -// The old name is reserved so nothing reintroduces "title" with another meaning. +```protobuf +// After: renamed title -> name. The old name is reserved so nothing +// reintroduces "title" with another meaning, even though buf breaking +// has no rule requiring this (the field still exists, just renamed). message Project { reserved title; string name = 1; } ``` -A rename is a `WIRE_JSON`-category and `FILE`/`PACKAGE`-category break even though it is a `WIRE`-category no-op; see [references/compatibility-surfaces.md](compatibility-surfaces.md). If a rename is still necessary (the old name was wrong, or collided with other vocabulary per the protobuf-design-messages skill), treat it as a breaking change requiring the same coordination as any other: a new package version, or a deliberate opt-out in CI for a pre-release package (see [references/ci-and-governance.md](ci-and-governance.md)). +If a rename is still necessary (the old name was wrong, or collided with other vocabulary per the protobuf-design-messages skill), treat the `FIELD_SAME_NAME` finding as a breaking change requiring the same coordination as any other: a new package version, or a deliberate opt-out in CI for a pre-release package (see [references/ci-and-governance.md](ci-and-governance.md)). -Reserved names in these examples use editions syntax, a bare identifier. In proto2 and proto3 files the name is quoted (`reserved "title";`). Field and enum-value names can be reserved; message names cannot. +Reserved names in these examples use editions syntax, a bare identifier. In proto2 and proto3 files the name is quoted (`reserved "title";`). Field and enum-value names can be reserved; message names cannot, which matters for the review question below. ## An Enum Value Rename Breaks JSON Too -Enum values serialize to JSON by name, not by number, so renaming an enum value is a JSON break even though the underlying integer is unchanged: +Enum values serialize to JSON by name, not by number. `ENUM_VALUE_SAME_NAME` (`FILE`, `PACKAGE`, `WIRE_JSON`) flags this the same way `FIELD_SAME_NAME` flags a field rename; the same reserve-the-old-name judgment call applies: ```protobuf -// Before -enum ProjectState { - PROJECT_STATE_UNSPECIFIED = 0; - PROJECT_STATE_DONE = 1; -} - -// After: DONE -> COMPLETED. The integer 1 is unchanged, but a JSON consumer -// that was matching the string "PROJECT_STATE_DONE" now sees -// "PROJECT_STATE_COMPLETED" and silently fails to match. enum ProjectState { reserved PROJECT_STATE_DONE; PROJECT_STATE_UNSPECIFIED = 0; - PROJECT_STATE_COMPLETED = 1; + PROJECT_STATE_COMPLETED = 1; // renamed from DONE } ``` ## A Type Change Never Reuses the Old Field Number -Changing a field's type (for example, a `string` identifier becoming a wrapper message, or an `int32` becoming an `int64` outside the compatible-widening cases documented by protobuf) is a wire-level break if the new wire type differs. Give the new value a new field number, and mark the old number (and, unless the old name is being reused for the new field at the new number, the old name too) `reserved` so nothing ever reuses it: +`FIELD_SAME_TYPE` (`FILE`, `PACKAGE`) flags a field whose type changed at its existing number. The fix is mechanical and the tool verifies it: allocate a new field number for the new type, and reserve the old number so `FIELD_NO_DELETE_UNLESS_NUMBER_RESERVED` (`WIRE`, `WIRE_JSON`) does not also fire for silently dropping the old one. Reserve the old name too, unless the new field reuses it at the new number (reserving a name still in use does not compile): ```protobuf -// Before -message Project { - string owner_id = 9; -} - // After: owner_id becomes a wrapper message instead of a bare string. -// The old field 9 is reserved, never reused. The name owner_id is not -// reserved because the new field reuses it; reserving it would not compile. message Project { reserved 9; UserId owner_id = 10; } ``` -A future reader (or a future engineer who does not know this history) who adds a new field will see the `reserved` statement and knows not to pick number 9 for anything unrelated. Without it, a new field at number 9 would be silently misinterpreted by any old binary still holding a message serialized under the old schema, which is exactly the class of bug `reserved` exists to prevent. +Without the reservation, a future field at number 9 would be silently misinterpreted by any old binary still holding a message serialized under the old schema, which is exactly the class of bug `reserved` exists to prevent, and exactly what `FIELD_NO_DELETE_UNLESS_NUMBER_RESERVED` is there to catch before it ships. ## Never Reuse a Reserved Number, Even Much Later -A `reserved` statement is permanent. Removing it to "free up" the number for an unrelated field defeats the entire purpose: any message serialized under the old schema that still exists anywhere (a queue, a log, a backup, a long-lived client) will be misinterpreted as having the new field's meaning at the old number. +`RESERVED_MESSAGE_NO_DELETE` and `RESERVED_ENUM_NO_DELETE` (all four categories) flag removing a `reserved` statement. Removing it to "free up" the number for an unrelated field defeats the entire purpose: any message serialized under the old schema that still exists anywhere (a queue, a log, a backup, a long-lived client) will be misinterpreted as having the new field's meaning at the old number. There is no judgment call here beyond "don't"; treat a finding on these rules as a bug in the change, not a candidate for an opt-out. + +(Run `buf breaking` first: it catches the rename, type change, deletion, and removed-`reserved` findings above. The questions below are what it cannot check.) ## Review Questions -- Does the diff rename a field or enum value without reserving the old name, or without a plan for the resulting JSON/generated-code break? -- Does the diff rename a message? Message names cannot be reserved, so the plan has to cover every consumer that names the type: generated code, `google.protobuf.Any` type URLs, and anything that stores the full name. -- Does the diff change a field's type at the same field number, instead of allocating a new number and reserving the old one? -- Does the diff remove a `reserved` statement to reuse a number or name? -- Does the diff rename an enum value, and has it been checked against `WIRE_JSON`, not just `WIRE`? +- Does a flagged rename or type change have the old name or number actually `reserved`, not just a plan to deal with the break later? +- Does the diff rename a message? Message names cannot be reserved, so nothing in `buf breaking` will catch a dangling reference; the plan has to cover every consumer that names the type by hand: generated code, `google.protobuf.Any` type URLs, and anything that stores the full name. +- Is a flagged break actually acceptable here, given this package's real consumers and its versioning stage? diff --git a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md index 8cdb581..2ed68dc 100644 --- a/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md +++ b/plugins/trogonstack-protobuf/skills/protobuf-evolve-schemas/references/review-checklist.md @@ -2,26 +2,24 @@ Use this reference when reviewing a diff to an existing `.proto` file. -Each item is a symptom of a principle, not a rule on its own. When an item fails, read the linked reference and judge the change by the principle behind it; a justified exception passes, a compliant change that misses the point does not. +- [ ] `buf format -d --exit-code`, `buf lint` (`STANDARD`), and `buf breaking` (against the category matching this package's actual consumers) all pass + +Each remaining item is a symptom of a principle the tools cannot check, not a rule on its own. When an item fails, read the linked reference and judge the change by the principle behind it; a justified exception passes, a compliant change that misses the point does not. ## Compatibility Surfaces ([compatibility-surfaces.md](compatibility-surfaces.md)) -- [ ] The change was checked against the `buf breaking` category that matches this package's actual consumers (binary-only vs. JSON vs. generated source) -- [ ] `WIRE_JSON` or stricter is the floor for any package with a JSON-speaking consumer +- [ ] The `buf breaking` category actually matches this package's consumers (binary-only vs. JSON vs. generated source), not just whichever category happens to pass ## Renames and Reserved Fields ([renames-and-reserved.md](renames-and-reserved.md)) -- [ ] No field, message, or enum value was renamed without accounting for the JSON/generated-code break that follows -- [ ] A renamed field or enum value has its old name `reserved` (message names cannot be reserved) -- [ ] No field's type changed at its existing field number; a new number was allocated instead -- [ ] The old field number is `reserved`, and the old name is `reserved` unless it stays attached to the new number -- [ ] No `reserved` statement was removed to reuse a number or name +- [ ] A rename or type change flagged by `buf breaking` has its old name or number actually `reserved`, not just a plan to deal with it later +- [ ] A renamed message has every consumer that names it by hand covered (message names cannot be `reserved`, so `buf breaking` cannot catch a dangling reference): generated code, `google.protobuf.Any` type URLs, anything storing the full name +- [ ] A flagged break is actually acceptable here, given the package's real consumers and versioning stage ## CI and Governance ([ci-and-governance.md](ci-and-governance.md)) -- [ ] `buf breaking` runs in CI against the correct base branch - [ ] No package's breaking-change check was permanently disabled or weakened to let one change through -- [ ] An intentional break carries a visible, per-change, reviewed opt-out (e.g. a PR label), not a standing exemption +- [ ] An intentional break carries the `buf skip breaking` label, reviewed and applied per PR, not a standing exemption - [ ] Any package excluded from breaking-change checks is confirmed to have no consumers on any surface (generated code, JSON, binary) ## Package Versioning ([package-versioning.md](package-versioning.md))