mirror of
https://github.com/NVIDIA/OpenShell.git
synced 2026-10-02 07:34:45 +08:00
feat(ci): detect breaking protobuf changes
Compare the proto module against the PR or merge-group base and report Buf violations in Branch Checks. Add local reproduction and fixture coverage. Closes #3794 Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
This commit is contained in:
@@ -16,6 +16,9 @@ outputs:
|
||||
should_run:
|
||||
description: "true if the workflow should proceed, false otherwise"
|
||||
value: ${{ steps.gate.outputs.should_run }}
|
||||
base_sha:
|
||||
description: "Target branch commit SHA for a mirrored pull request, or empty for other events"
|
||||
value: ${{ steps.gate.outputs.base_sha }}
|
||||
labels_json:
|
||||
description: "JSON array of PR label names for push-triggered mirror runs, or [] otherwise"
|
||||
value: ${{ steps.gate.outputs.labels_json }}
|
||||
@@ -40,16 +43,19 @@ runs:
|
||||
if [ "$EVENT_NAME" != "push" ]; then
|
||||
echo "labels_json=[]" >> "$GITHUB_OUTPUT"
|
||||
echo "should_run=true" >> "$GITHUB_OUTPUT"
|
||||
echo "base_sha=" >> "$GITHUB_OUTPUT"
|
||||
exit 0
|
||||
fi
|
||||
|
||||
if [ "$GET_PR_INFO_OUTCOME" != "success" ]; then
|
||||
echo "labels_json=[]" >> "$GITHUB_OUTPUT"
|
||||
echo "should_run=false" >> "$GITHUB_OUTPUT"
|
||||
echo "base_sha=" >> "$GITHUB_OUTPUT"
|
||||
exit 0
|
||||
fi
|
||||
|
||||
head_sha="$(jq -r '.head.sha' <<< "$PR_INFO")"
|
||||
base_sha="$(jq -r '.base.sha // empty' <<< "$PR_INFO")"
|
||||
labels_json="$(jq -c '[.labels[].name]' <<< "$PR_INFO")"
|
||||
if [ -z "$REQUIRED_LABEL" ]; then
|
||||
has_label=true
|
||||
@@ -67,3 +73,4 @@ runs:
|
||||
|
||||
echo "labels_json=$labels_json" >> "$GITHUB_OUTPUT"
|
||||
echo "should_run=$should_run" >> "$GITHUB_OUTPUT"
|
||||
echo "base_sha=$base_sha" >> "$GITHUB_OUTPUT"
|
||||
|
||||
@@ -30,12 +30,65 @@ jobs:
|
||||
pull-requests: read
|
||||
outputs:
|
||||
should_run: ${{ steps.gate.outputs.should_run }}
|
||||
base_sha: ${{ steps.gate.outputs.base_sha }}
|
||||
steps:
|
||||
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
|
||||
|
||||
- id: gate
|
||||
uses: ./.github/actions/pr-gate
|
||||
|
||||
proto-breaking:
|
||||
name: Protobuf API compatibility
|
||||
needs: pr_metadata
|
||||
if: needs.pr_metadata.outputs.should_run == 'true'
|
||||
runs-on: linux-amd64-cpu8
|
||||
timeout-minutes: 15
|
||||
container:
|
||||
image: ghcr.io/nvidia/openshell/ci:latest
|
||||
credentials:
|
||||
username: ${{ github.actor }}
|
||||
password: ${{ secrets.GITHUB_TOKEN }}
|
||||
steps:
|
||||
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
|
||||
|
||||
- name: Mark workspace as safe for git
|
||||
run: git config --global --add safe.directory "$GITHUB_WORKSPACE"
|
||||
|
||||
- name: Resolve and fetch target branch baseline
|
||||
id: baseline
|
||||
env:
|
||||
PR_BASE_SHA: ${{ needs.pr_metadata.outputs.base_sha }}
|
||||
MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }}
|
||||
DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
|
||||
run: |
|
||||
set -euo pipefail
|
||||
if [ "$GITHUB_EVENT_NAME" = "merge_group" ]; then
|
||||
base_sha="$MERGE_GROUP_BASE_SHA"
|
||||
elif [ "$GITHUB_EVENT_NAME" = "push" ]; then
|
||||
base_sha="$PR_BASE_SHA"
|
||||
else
|
||||
git fetch --no-tags --depth=1 origin "refs/heads/$DEFAULT_BRANCH"
|
||||
base_sha="$(git rev-parse FETCH_HEAD)"
|
||||
fi
|
||||
|
||||
if [[ ! "$base_sha" =~ ^[0-9a-f]{40}$ ]]; then
|
||||
echo "::error::No valid target branch baseline SHA was available."
|
||||
exit 1
|
||||
fi
|
||||
if [ "$GITHUB_EVENT_NAME" != "workflow_dispatch" ]; then
|
||||
git fetch --no-tags --depth=1 origin "$base_sha"
|
||||
fi
|
||||
echo "sha=$base_sha" >> "$GITHUB_OUTPUT"
|
||||
echo "Comparing protobuf API with $base_sha"
|
||||
|
||||
- name: Install tools
|
||||
run: mise install --locked
|
||||
|
||||
- name: Check protobuf API compatibility
|
||||
env:
|
||||
PROTO_BREAKING_BASE_REF: ${{ steps.baseline.outputs.sha }}
|
||||
run: mise run proto:breaking
|
||||
|
||||
mise-lockfile:
|
||||
name: mise Lockfile
|
||||
needs: pr_metadata
|
||||
|
||||
@@ -18,6 +18,31 @@ This setting does not change the bot's automatic trust policy for ready PRs.
|
||||
|
||||
Merge queue validation is a second integration gate for `main`. After a PR has passed the required PR-head statuses, a maintainer adds it to the merge queue. GitHub creates a temporary merge-group branch that combines the latest `main`, the queued PR, and any earlier queued PRs. The same required `OpenShell / ...` status contexts are then published against the merge-group SHA before GitHub merges it.
|
||||
|
||||
### Protobuf API compatibility
|
||||
|
||||
The `Protobuf API compatibility` job in `Branch Checks` compares the candidate's
|
||||
`proto/` module with the PR target branch commit. This covers SDK and extension
|
||||
contracts. Merge-queue runs compare with the merge group's base commit. The job
|
||||
uses the `FILE` breaking policy in `buf.yaml` and fails with a file and symbol
|
||||
diagnostic for an incompatible change. It runs on every branch check so changes
|
||||
to imported messages cannot be missed by a changed-file filter. The gateway's
|
||||
storage-only protobuf module is outside this comparison and has separate
|
||||
durability checks.
|
||||
|
||||
Fetch the target branch and reproduce the comparison locally, replacing
|
||||
`origin/main` if the PR targets another branch:
|
||||
|
||||
```shell
|
||||
git fetch origin main
|
||||
PROTO_BREAKING_BASE_REF=origin/main mise run proto:breaking
|
||||
```
|
||||
|
||||
For an intentional incompatibility, record the Buf finding, linked issue,
|
||||
consumer impact, and migration plan in the PR. The check stays failed; a
|
||||
maintainer must explicitly decide whether to version the API or authorize an
|
||||
exception through repository merge policy. Do not suppress the finding by
|
||||
disabling the job or adding a broad Buf ignore rule.
|
||||
|
||||
Windows PR checks are opt-in: add `test:windows`, then select **Re-run all jobs**
|
||||
on the current Windows MSVC run. Subsequent mirrored commits run them automatically.
|
||||
Windows checks are not required for merging and do not run in merge queues.
|
||||
|
||||
@@ -679,6 +679,8 @@ Public API compatibility and storage compatibility are reviewed independently:
|
||||
|
||||
- Public compatibility is evaluated from public service descriptors and SDK
|
||||
generation inputs. Storage-only packages must never enter that closure.
|
||||
Branch CI compares the `proto/` module, including SDK and extension contracts,
|
||||
with the target branch commit under the repository's Buf `FILE` policy.
|
||||
- `openshell.storage.v1` is frozen. Its test fingerprint covers message names,
|
||||
field numbers, cardinality, scalar wire types, referenced types, map-entry
|
||||
shapes, and optional presence. Keep its decoder available and introduce a
|
||||
|
||||
@@ -81,6 +81,12 @@ identifies the service being exposed.
|
||||
not reuse either for a different meaning.
|
||||
- Review changes against both the public descriptor closure and durable stored
|
||||
protobuf closure described in [the gateway architecture](../architecture/gateway.md#protobuf-api-and-storage-boundaries).
|
||||
- Branch CI compares this module, including SDK and extension contracts,
|
||||
against the target branch with Buf's `FILE` breaking policy. To check locally
|
||||
after fetching the target branch, run
|
||||
`PROTO_BREAKING_BASE_REF=origin/main mise run proto:breaking` (replace the ref
|
||||
when the PR targets another branch). The storage-only module is checked by its
|
||||
separate durability tests, not by this API comparison.
|
||||
- Regenerate Rust, Python, Go, and TypeScript bindings after contract changes.
|
||||
Run `mise run pre-commit`, the affected SDK checks, and relevant server tests
|
||||
before submitting the change.
|
||||
|
||||
Executable
+24
@@ -0,0 +1,24 @@
|
||||
#!/usr/bin/env bash
|
||||
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
|
||||
# SPDX-License-Identifier: Apache-2.0
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
base_ref="${PROTO_BREAKING_BASE_REF:-}"
|
||||
if [ -z "$base_ref" ]; then
|
||||
echo "Set PROTO_BREAKING_BASE_REF to the commit or ref to compare against." >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
base_sha=$(git rev-parse --verify "$base_ref^{commit}")
|
||||
|
||||
error_format=text
|
||||
if [ "${GITHUB_ACTIONS:-}" = true ]; then
|
||||
error_format=github-actions
|
||||
fi
|
||||
|
||||
echo "Checking protobuf API compatibility against $base_sha"
|
||||
buf breaking proto \
|
||||
--against ".git#ref=$base_sha,subdir=proto" \
|
||||
--config buf.yaml \
|
||||
--error-format "$error_format"
|
||||
Executable
+120
@@ -0,0 +1,120 @@
|
||||
#!/usr/bin/env bash
|
||||
# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
|
||||
# SPDX-License-Identifier: Apache-2.0
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
|
||||
check_script="$script_dir/check-proto-breaking.sh"
|
||||
work_dir=$(mktemp -d)
|
||||
trap 'rm -rf "$work_dir"' EXIT
|
||||
|
||||
mkdir -p "$work_dir/proto" "$work_dir/crates/openshell-server/proto"
|
||||
cat > "$work_dir/buf.yaml" <<'EOF'
|
||||
version: v2
|
||||
modules:
|
||||
- path: proto
|
||||
- path: crates/openshell-server/proto
|
||||
breaking:
|
||||
use:
|
||||
- FILE
|
||||
EOF
|
||||
cat > "$work_dir/proto/openshell.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.v1;
|
||||
import "datamodel.proto";
|
||||
message GetRequest { openshell.datamodel.v1.Resource resource = 1; }
|
||||
service OpenShell { rpc Get(GetRequest) returns (GetRequest); }
|
||||
EOF
|
||||
cat > "$work_dir/proto/datamodel.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.datamodel.v1;
|
||||
message Resource { string name = 1; }
|
||||
EOF
|
||||
cat > "$work_dir/proto/compute_driver.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.compute.v1;
|
||||
message DriverRequest { string sandbox = 1; }
|
||||
service ComputeDriver { rpc Start(DriverRequest) returns (DriverRequest); }
|
||||
EOF
|
||||
cat > "$work_dir/crates/openshell-server/proto/storage.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.storage.v1;
|
||||
message StoredResource { string name = 1; }
|
||||
EOF
|
||||
|
||||
git -C "$work_dir" init -q
|
||||
git -C "$work_dir" add buf.yaml proto crates
|
||||
git -C "$work_dir" -c user.name='OpenShell Test' -c user.email='test@example.invalid' commit -qm 'test: baseline protobuf descriptors'
|
||||
base_sha=$(git -C "$work_dir" rev-parse HEAD)
|
||||
|
||||
run_check() {
|
||||
(cd "$work_dir" && PROTO_BREAKING_BASE_REF="$base_sha" "$check_script")
|
||||
}
|
||||
|
||||
if (cd "$work_dir" && PROTO_BREAKING_BASE_REF= "$check_script") > "$work_dir/missing-base.log" 2>&1; then
|
||||
echo "Expected a missing baseline to fail closed." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Additions to imported SDK descriptors are compatible.
|
||||
cat > "$work_dir/proto/datamodel.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.datamodel.v1;
|
||||
message Resource { string name = 1; string display_name = 2; }
|
||||
EOF
|
||||
run_check > "$work_dir/additive.log"
|
||||
|
||||
# A removed field in an imported descriptor must fail with a useful location.
|
||||
cat > "$work_dir/proto/datamodel.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.datamodel.v1;
|
||||
message Resource { string display_name = 2; }
|
||||
EOF
|
||||
if GITHUB_ACTIONS=true run_check > "$work_dir/breaking.log" 2>&1; then
|
||||
echo "Expected an imported protobuf field removal to fail." >&2
|
||||
exit 1
|
||||
fi
|
||||
if ! grep -q '^::error file=proto/datamodel.proto.*Previously present field "1"' "$work_dir/breaking.log"; then
|
||||
cat "$work_dir/breaking.log" >&2
|
||||
echo "Missing a diagnostic for the removed imported field." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Extension protocol changes in the proto module must also be checked.
|
||||
cat > "$work_dir/proto/datamodel.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.datamodel.v1;
|
||||
message Resource { string name = 1; }
|
||||
EOF
|
||||
cat > "$work_dir/proto/compute_driver.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.compute.v1;
|
||||
message DriverRequest { string renamed = 2; }
|
||||
service ComputeDriver { rpc Start(DriverRequest) returns (DriverRequest); }
|
||||
EOF
|
||||
if GITHUB_ACTIONS=true run_check > "$work_dir/extension.log" 2>&1; then
|
||||
echo "Expected an extension protobuf field removal to fail." >&2
|
||||
exit 1
|
||||
fi
|
||||
if ! grep -q '^::error file=proto/compute_driver.proto.*Previously present field "1"' "$work_dir/extension.log"; then
|
||||
cat "$work_dir/extension.log" >&2
|
||||
echo "Missing a diagnostic for the removed extension field." >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# A storage-only change is outside the proto module comparison.
|
||||
cat > "$work_dir/proto/compute_driver.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.compute.v1;
|
||||
message DriverRequest { string sandbox = 1; }
|
||||
service ComputeDriver { rpc Start(DriverRequest) returns (DriverRequest); }
|
||||
EOF
|
||||
cat > "$work_dir/crates/openshell-server/proto/storage.proto" <<'EOF'
|
||||
syntax = "proto3";
|
||||
package openshell.storage.v1;
|
||||
message StoredResource { string renamed = 2; }
|
||||
EOF
|
||||
run_check > "$work_dir/storage.log"
|
||||
|
||||
echo "Proto breaking checks passed: additive, SDK, extension, and storage-only cases."
|
||||
@@ -18,6 +18,7 @@ depends = [
|
||||
"test:e2e-parity",
|
||||
"test:packaging-assets",
|
||||
"test:qualification-summary",
|
||||
"test:proto-breaking",
|
||||
"test:codex-security-release-range",
|
||||
"test:docs-website",
|
||||
"test:docs-nav",
|
||||
@@ -74,6 +75,12 @@ run = "tasks/scripts/test-qualification-summary.sh"
|
||||
run_windows = "echo Skipping test:qualification-summary: the release workflow runs on Unix hosts."
|
||||
hide = true
|
||||
|
||||
["test:proto-breaking"]
|
||||
description = "Test public protobuf breaking-change detection"
|
||||
run = "bash tasks/scripts/test-proto-breaking.sh"
|
||||
run_windows = "echo Skipping test:proto-breaking: the Branch Checks workflow runs on Unix hosts."
|
||||
hide = true
|
||||
|
||||
["test:codex-security-release-range"]
|
||||
description = "Test Codex Security release-range resolution"
|
||||
run = "uv run --no-project --with pytest pytest -o \"python_files=*_test.py\" tasks/scripts/codex_security_range_test.py"
|
||||
|
||||
@@ -29,6 +29,10 @@ depends = ["sdk:ts:install"]
|
||||
run = "./sdk/typescript/node_modules/.bin/buf lint"
|
||||
run_windows = "sdk\\typescript\\node_modules\\.bin\\buf.cmd lint"
|
||||
|
||||
["proto:breaking"]
|
||||
description = "Compare proto/ contracts against PROTO_BREAKING_BASE_REF"
|
||||
run = "bash tasks/scripts/check-proto-breaking.sh"
|
||||
|
||||
["sdk:ts:typecheck"]
|
||||
description = "Type-check the TypeScript SDK"
|
||||
depends = ["sdk:ts:proto"]
|
||||
|
||||
Reference in New Issue
Block a user