Skip to content

feat(ci): detect breaking protobuf API changes in branch checks #3794

Closed
Enhancement
#4049
Closed
feat(ci): detect breaking protobuf API changes in branch checks#3794
Enhancement
#4049
Labels
state:acceptedA maintainer decided OpenShell should pursue this issue

Description

@mrunalp
mrunalp
opened on Sep 28, 2026

User Story

As an OpenShell maintainer reviewing a protobuf change, I want CI to identify breaking changes against the target branch, so that I can catch unintended client incompatibilities before merging.

Problem Statement

The repository defines a Buf FILE breaking-change policy in buf.yaml, but Branch Checks run buf lint without running buf breaking against a previous revision. The Go SDK's proto check verifies that generated bindings are current; SDK builds and tests use the proposed schema. None of these checks compares the proposed protobuf contract with the existing one.

Impact / Why This Matters

A change can be well-formed, regenerate successfully, and pass in-tree tests while breaking existing gRPC or generated-SDK consumers. Reviewers must inspect schema diffs manually or run a local compatibility comparison. That workaround is easy to miss and does not provide a consistent, visible result for every pull request or merge-queue run.

Proposed Design

For changes to public protobuf contracts, Branch Checks should report compatibility against the appropriate target-branch baseline. Compatible additions should pass; a breaking change should produce an actionable CI failure that identifies the affected contract. The result should remain visible when a change enters the merge queue. If an intentional pre-stability break is permitted, its disposition should be explicit and reviewable rather than silently disabling the check.

Keep the public API boundary distinct from the gateway's storage-only protobuf module, which has separate durability rules.

Acceptance Criteria

  • A pull request that changes public .proto files receives a protobuf breaking-change result against the target branch; merge-queue runs receive an equivalent result.
  • A representative breaking change to an existing public message or RPC fails the check with a useful diagnostic, while a compatible additive change passes.
  • The comparison covers the public protobuf descriptor closure used by generated SDKs and does not accidentally classify storage-only declarations as public API.
  • The documented process for an intentional breaking change preserves a reviewer-visible compatibility result and records why the break is allowed.
  • Contributor documentation describes the check and how to reproduce it locally.

Alternatives Considered

  • Rely on Buf lint, generated-binding freshness, and SDK builds. These validate the proposed schema but do not compare it with an existing contract.
  • Rely on manual review or release qualification alone. This delays detection and produces no consistent signal on the pull request where the change is introduced.

Agent Investigation

  • buf.yaml declares the repo-level modules (proto/ and crates/openshell-server/proto/) and breaking: use: [FILE].
  • tasks/typescript.toml defines proto:lint as buf lint; sdk:ts:ci includes that task. .github/workflows/branch-checks.yml runs sdk:ts:ci and go:ci.
  • tasks/go.toml defines go:proto:check as a generated-file freshness check. No task or workflow invokes buf breaking.
  • architecture/gateway.md distinguishes public API compatibility from storage compatibility. rfc/0014-release-stability/release-qualification.md specifies a breaking API review, while the current qualification summary lists that review as a missing suite. This issue targets detection in branch CI.
  • Related API-stabilization tracker: Stabilize public API, SDK, and extension contracts for 0.1.0 #2565.

Activity

  1. mrunalp
    added the
    Enhancement
    issue type on Sep 28, 2026
  2. mrunalp
    added
    state:acceptedA maintainer decided OpenShell should pursue this issue
    on Sep 28, 2026

  3. mrunalp commented on Sep 28, 2026

    @mrunalp
    mrunalp
    on Sep 28, 2026
    CollaboratorAuthor

    🏗️ build-plan

    Implementation Plan

    Issue type: feat
    Complexity: Medium
    Confidence: High — the Buf comparison and CI baselines can be verified locally; intentional breaks require a maintainer policy decision.

    Summary

    Add a required Branch Checks job that compares the proto/ module, including SDK and extension contracts, with the exact target-branch commit. Use Buf's existing FILE policy and report actionable violations on both mirrored PR pushes and merge-group runs.

    Scope

    • tasks/scripts/check-proto-breaking.sh and a proto:breaking mise task: compare the current and baseline proto/ modules, leaving the storage-only module outside this API check.
    • .github/workflows/branch-checks.yml: resolve the PR base SHA or merge-group base SHA, fetch it, and run the check as a standard job.
    • A focused script test: verify additive changes pass, SDK and extension breaks fail with diagnostics, and storage-only changes are outside the API comparison.
    • CI.md, proto/README.md, and the existing architecture/gateway.md section: document the gate, local reproduction, and the public/storage boundary.
    • Review .agents/skills/watch-github-actions/SKILL.md for changed CI diagnostics and update it only if its guidance becomes inaccurate.

    Implementation Steps

    1. Implement and locally verify module comparison against an explicit baseline SHA.
    2. Add synthetic pass/fail and storage boundary checks for the comparison.
    3. Wire a visible Branch Checks job for mirrored PRs and merge groups using their exact base SHAs.
    4. Update contributor and architecture documentation; run targeted checks and pre-commit.

    Test Plan

    • Script tests: additive field passes; removed SDK and extension fields fail with useful Buf diagnostics; storage-only changes do not alter the proto/ module comparison.
    • CI validation: inspect the workflow syntax and confirm the job participates in the existing required Branch Checks aggregate.
    • E2E: N/A; no sandbox or e2e/ behavior changes.

    Risks & Open Questions

    • Intentional same-version breaks will fail the required job. The current main ruleset has no configured bypass actors. The PR must record the reason and linked issue, and maintainers must decide whether to version the API or approve a narrow policy exception; this implementation will not silently suppress the diagnostic.
    • The mirror workflow does not use the PR event payload, so the job must resolve its base SHA from PR metadata. Merge groups provide base_sha in their event payload.

    Documentation Impact

    • Update CI and protobuf contributor guidance plus the existing gateway architecture boundary. No published product docs, gateway configuration docs, or LSM changes.

    Revision 2 — cover all versioned proto/ contracts, including extension APIs
    Revision 1 — initial plan

  4. mrunalp commented on Sep 28, 2026

    @mrunalp
    mrunalp
    on Sep 28, 2026
    CollaboratorAuthor

    🏗️ build-from-issue-agent

    Implementation Complete

    PR: #3805

    What was built

    Branch Checks now compare the public proto/ module with the exact PR or merge-group base using Buf's existing FILE compatibility policy. The check reports incompatible changes as GitHub annotations; the gateway storage-only module remains outside the public API check.

    Tests

    • Regression fixture: 5 scenarios covering additive edits, SDK and extension breaks, a missing baseline, and the storage boundary.
    • Local validation: mise run pre-commit, mise run ci, and comparison against origin/main passed.
    • E2E: N/A for this CI-only change.

    Docs updated

    CI.md, proto/README.md, and architecture/gateway.md.

    The issue will auto-close when the PR is merged.

  5. SDAChess
    closed this as completedin #4049on Oct 1, 2026
  6. rajkrishnamurthy
    added a commit that references this issue on Oct 1, 2026

    fix(ci): qualify protobuf compatibility by release train (NVIDIA#4049)

    5698c4f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    state:acceptedA maintainer decided OpenShell should pursue this issue

    Type

    Enhancement

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions