natsukium/felis
Watch
1
Code Issues 27 Pull requests 1 Projects Releases Packages Wiki Activity Actions

[v0.1/P0] Make protobuf compatibility checks fail against the CI base #19

New issue
Closed
opened 2026-09-03 16:18:02 +09:00 by natsukium · 1 comment
natsukium commented 2026-09-03 16:18:02 +09:00
Owner
Copy link

Parent: #12 (P0.4). Supersedes the CI-gate part of #5 and #9.

Why

buf breaking --against '.git#ref=HEAD' compares the schema with itself, while the Nix store path skips the check when .git is absent. A breaking wire edit can therefore pass CI.

Scope

  • Compare pull requests against the PR base revision.
  • Compare direct main pushes against the prior commit or latest release tag.
  • Require an explicit pre-release acknowledgement for intentional breaks, together with generated artifacts and design updates.
  • Define post-tag requirements for protocol-major breaks.
  • Keep semantic ledgers and row-codec vectors as separate gates because Buf cannot cover them.

Acceptance criteria

  • A deliberately breaking proto fixture fails the Forgejo workflow.
  • Additive changes pass and still require regenerated code.
  • Missing git history cannot silently turn the CI check into success.
  • Direct pushes and pull requests select the documented comparison base.
  • IPC/testing docs explain Buf's coverage and exclusions.
Parent: #12 (P0.4). Supersedes the CI-gate part of #5 and #9. ## Why `buf breaking --against '.git#ref=HEAD'` compares the schema with itself, while the Nix store path skips the check when `.git` is absent. A breaking wire edit can therefore pass CI. ## Scope - Compare pull requests against the PR base revision. - Compare direct `main` pushes against the prior commit or latest release tag. - Require an explicit pre-release acknowledgement for intentional breaks, together with generated artifacts and design updates. - Define post-tag requirements for protocol-major breaks. - Keep semantic ledgers and row-codec vectors as separate gates because Buf cannot cover them. ## Acceptance criteria - [ ] A deliberately breaking proto fixture fails the Forgejo workflow. - [ ] Additive changes pass and still require regenerated code. - [ ] Missing git history cannot silently turn the CI check into success. - [ ] Direct pushes and pull requests select the documented comparison base. - [ ] IPC/testing docs explain Buf's coverage and exclusions.
natsukium commented 2026-09-03 20:35:12 +09:00
Author
Owner
Copy link

Triage plan (2026-09-03)

Source-grounded triage against main at 69076d42, reviewed through seven rounds of an independent reviewer (pi sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision.

Claim check

Accurate, and slightly worse than stated.

  • buf breaking --against '.git#ref=HEAD': flake.nix:384-396 defines the buf-breaking pre-commit hook with exactly that --against. As a pre-commit hook on a developer's machine it compares the staged tree with HEAD, which is meaningful. As a CI check it is not: pr.yml:22-29 runs nix flake check, whose pre-commit check evaluates a store copy of the tree with no .git, so the guard on flake.nix:390 (git rev-parse --is-inside-work-tree || exit 0) exits 0 — the check is skipped, silently, in the only CI job that runs it. The comment at flake.nix:379-383 acknowledges this. Even if .git were present, the checked-out tree is HEAD in CI, so it would compare the schema with itself. Either way a breaking edit passes.
  • CI relies on it anyway: pr.yml:45-52 (proto job) says wire-compat "run[s] as pre-commit hooks, so the flake-check job is their CI gate", and docs/reference/testing.md:1075-1078 repeats that nix flake check covers buf lint / buf breaking. The proto job itself only regenerates and diffs (pr.yml:58-63); just proto (justfile:157-160) runs buf lint + buf generate, never buf breaking. So the documented gate does not exist.
  • Shallow clones: no fetch-depth in pr.yml (only bench.yml:68 sets fetch-depth: 0), so even a corrected --against '.git#ref=<base>' would fail to resolve the base until the workflow fetches it — this is the "missing git history cannot silently turn into success" criterion, and today the failure mode would be a hard error from buf (good) if the guard did not skip first (bad).
  • Rules: buf.yaml:12-14 uses WIRE_JSON, the right category for a protobuf-binary wire plus the JSON bridge (docs/reference/ipc.md:1624-1630).
  • What Buf cannot cover: the minor ledger (docs/reference/ipc.md:1738-1752) is prose with no code check that PROTOCOL_MINOR (crates/felis-protocol/src/preface.rs:57) matches the last row; the row-codec golden vectors are unit tests in crates/felis-grid/src/wire.rs:959-985 and the carrier block vector in preface.rs:614, which run under cargo nextest in the build job — these are already gates but nothing names them as the compatibility gate. The issue's "keep them as separate gates" is accurate.
  • Intentional breaks: the acknowledgement path today is SKIP=buf-breaking at commit time (flake.nix:378,387), invisible to CI. docs/reference/ipc.md:1685-1687 states pre-freeze breaks ride major 1 unbumped, which is the policy the acknowledgement must encode.

Verdict

accept, with one addition and one clarification.

Addition: the hook's git rev-parse guard must not be the CI mechanism at all. Move the CI-facing check into a dedicated pr.yml job that runs buf breaking inside nix develop with an explicit base, and leave the pre-commit hook as the developer-side early warning (its HEAD comparison is correct there). Do not try to make nix flake check do this: the sandbox has no history by design.

Clarification on "explicit pre-release acknowledgement": use a file, not a commit-message trailer or a SKIP env, because a file is visible to the PR diff, to git log -p, and to the release gate (#18). Recommended: crates/felis-protocol/proto/BREAKING.md (or a breaking: field in a small proto/compat.toml) that names the base revision the break is acknowledged against; the CI job passes when either buf is clean or the file names github.event.pull_request.base.sha/github.event.before exactly. On tag (#18) the file must be absent or reference a revision before the previous tag, and post-tag a break additionally requires PROTOCOL_MAJOR to have moved — the #30 rule.

Principles: pure process/CI, no product surface. Pass.

Approach

pr.yml — new proto-compat job (runs on nix)

proto-compat:
  name: wire schema is compatible with the base
  runs-on: nix
  steps:
    - uses: actions/checkout@…   # same pin as pr.yml:28
      with: { fetch-depth: 0 }   # buf needs the base object; see bench.yml:68 for the precedent
    - run: nix develop -c scripts/proto/compat.sh

scripts/proto/compat.sh (new; the scripts/ tree already holds bench/ and make-macos-app.sh):

  1. Resolve BASE: pull_request → github.event.pull_request.base.sha; push to main → github.event.before, falling back to the latest v* tag when before is the null sha (first push / force) — and once a tag exists, prefer git describe --tags --abbrev=0 HEAD~1 so the comparison is against the released baseline, which is what the issue's "prior commit or latest release tag" asks for. Fail with a named error if BASE cannot be resolved or git cat-file -e "$BASE^{commit}" fails (this is the "missing history cannot become success" criterion: set -eu, no || exit 0).
  2. buf breaking --against ".git#ref=$BASE"; on failure, accept only if the acknowledgement file names $BASE (exact 40-hex match) and the workflow is not a tag build.
  3. Print which base was used so the run log shows the documented selection.

Also amend the proto job comment at pr.yml:45-52 and the hook comment at flake.nix:376-383 so neither claims nix flake check is the CI gate.

justfile

  • just proto-compat [base]: same script, defaulting to main's merge-base locally (git merge-base HEAD origin/main), so the local runner mirrors CI 1:1 as the header at justfile:1-10 promises.

Fixtures / tests

  • The "deliberately breaking proto fixture fails the workflow" criterion is best proven once, not kept: open a throwaway PR that deletes a field from felis.proto and record the red run URL in the issue; a permanent negative fixture would need a second buf module and an expect-fail harness that Forgejo Actions cannot express cleanly. Alternatively, scripts/proto/compat_test.sh can build a temp git repo with two commits (additive vs breaking) and assert the script's exit codes — cheap, hermetic, and runs in just check; recommend this.
  • Additive changes "still require regenerated code": already enforced by the proto job's git diff --cached --exit-code (pr.yml:63); no new work, just cite it.
  • Ledger gate: add a unit test in felis-protocol that parses docs/reference/ipc.md's ledger table (or a mirrored const list in preface.rs) and asserts the highest row equals PROTOCOL_MINOR. Keeping the ledger in prose only means a minor bump with no row cannot be caught; a const MINOR_LEDGER: &[(u16, &str)] in preface.rs with a doc-sync test (same pattern as the config schema sync test at crates/felis-client-core/src/config.rs:2476) is the smallest form. If #30 drops the ledger for the 2.0 baseline, this becomes a one-row table; keep the test.
  • Row-codec vectors: already wire.rs:959-985; name them in testing.md as the codec half of the compatibility gate.

Docs cascade

  • docs/reference/testing.md:1073-1078 and the pipeline list: describe proto-compat (base selection, acknowledgement file, tag behavior) and list the three compatibility gates (buf WIRE_JSON, ledger sync test, row-codec/carrier golden vectors) with what each covers.
  • docs/reference/ipc.md:1674-1687 "Versioning": one paragraph on how a pre-freeze break is acknowledged (the file) and what changes after the first tag (major bump mandatory).
  • docs/explanation/architecture/ipc.md (schema-evolution section around :896-920): the why — Buf checks field-level wire compatibility, not semantics (renamed meaning, kinds/variants fatal rule at ipc.md:1699-1706), which is why the ledger and vectors stay separate; rejected alternatives (SKIP= env: invisible to CI; commit trailer: not diffable; nix flake check: no history in sandbox); "Revisit if" Buf gains a Rust-aware semantic plugin or the wire moves to a registry.
  • .claude/skills/extend-ipc/SKILL.md step for proto edits: add "run just proto-compat; for an intended break write the acknowledgement file".
  • CHANGELOG.md: not user-affecting; no entry.
  • skills/felis: no change.

Dependencies

  • None hard; the tracker's order (#19 with #45, before #17) still holds and is the right sequence because #17 and #30 both edit felis.proto and should be the first changes the new gate sees.
  • #30 decides the post-tag rule text ("protocol-major breaks after v0.1.0"); write the doc paragraph so #30 fills in the major number rather than restating the policy.
  • #18 consumes the tag-mode behavior (acknowledgement file must not permit a break on a tag build).

Risk/effort

S–M. One workflow job, one script with a hermetic test, one const + sync test, docs. Main risk: base resolution edge cases on Forgejo (github.event.before on the first push after a force, or a PR from a fork where base.sha is stale after the base branch moves — use the merge-base of base.sha and HEAD to be safe). Secondary: fetch-depth: 0 cost on a runner with a warm checkout is negligible for this repo's size.

Labels

Keep priority/P0, release/v0.1.0. Suggest adding an acceptance line: "the pre-commit hook's .git guard is no longer the only CI path; nix flake check is not claimed as the compatibility gate anywhere in docs/ or workflow comments".

Review amendments (round 4)

  • The compatibility baseline is committed release state, not "latest v* tag". A tag exists even when its release run failed, and rehearsal tags are pushed deliberately. The base for proto-compat on a tag build is the schema recorded in a committed baseline file crates/felis-protocol/proto/baseline/felis.proto plus baseline/RELEASE (the final release version and commit it was published from), which #18's release job updates (commits to main) only after a final (non-prerelease) release publishes successfully. PR/push builds keep the merge-base / event.before base. When the baseline file is absent (before the first final release) the workflow requires the committed first-release acknowledgment instead; that acknowledgment is tied to the absence of the baseline, not to tag ordinality, so rc tags before v0.1.0 do not consume it. Prerelease tags never update the baseline.

Review amendments (round 5)

  • Baseline lives outside the Buf module. buf.yaml roots the module at crates/felis-protocol/proto, so a copy inside it would be a duplicate declaration. Store the baseline at repo root proto-baseline/felis.proto + proto-baseline/RELEASE (outside every Buf module; buf breaking --against takes it as a separate input directory). Add a test to scripts/proto/compat.sh's self-test that buf lint on the module and the baseline comparison both pass with the committed layout.
  • Baseline absence is not proof of no release. Before every final tag, verify-tag queries the Forgejo release API: (a) if any final release exists, proto-baseline/RELEASE must name the latest one and its schema must equal that release's attached felis.proto (byte compare of the release asset), otherwise fail with "baseline behind the published release; reconcile first"; (b) first-release is accepted only when the API reports no final release. The baseline update is therefore reconciled before publication, and a failed post-publish baseline commit blocks the next tag rather than silently narrowing the check.
## Triage plan (2026-09-03) Source-grounded triage against `main` at `69076d42`, reviewed through seven rounds of an independent reviewer (`pi` sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision. ## Claim check Accurate, and slightly worse than stated. - **`buf breaking --against '.git#ref=HEAD'`**: `flake.nix:384-396` defines the `buf-breaking` pre-commit hook with exactly that `--against`. As a *pre-commit* hook on a developer's machine it compares the staged tree with HEAD, which is meaningful. As a *CI* check it is not: `pr.yml:22-29` runs `nix flake check`, whose pre-commit check evaluates a store copy of the tree with no `.git`, so the guard on `flake.nix:390` (`git rev-parse --is-inside-work-tree || exit 0`) exits 0 — the check is skipped, silently, in the only CI job that runs it. The comment at `flake.nix:379-383` acknowledges this. Even if `.git` were present, the checked-out tree *is* HEAD in CI, so it would compare the schema with itself. Either way a breaking edit passes. - **CI relies on it anyway**: `pr.yml:45-52` (`proto` job) says wire-compat "run[s] as pre-commit hooks, so the flake-check job is their CI gate", and `docs/reference/testing.md:1075-1078` repeats that `nix flake check` covers `buf lint / buf breaking`. The `proto` job itself only regenerates and diffs (`pr.yml:58-63`); `just proto` (`justfile:157-160`) runs `buf lint` + `buf generate`, never `buf breaking`. So the documented gate does not exist. - **Shallow clones**: no `fetch-depth` in `pr.yml` (only `bench.yml:68` sets `fetch-depth: 0`), so even a corrected `--against '.git#ref=<base>'` would fail to resolve the base until the workflow fetches it — this is the "missing git history cannot silently turn into success" criterion, and today the failure mode would be a hard error from buf (good) *if* the guard did not skip first (bad). - **Rules**: `buf.yaml:12-14` uses `WIRE_JSON`, the right category for a protobuf-binary wire plus the JSON bridge (`docs/reference/ipc.md:1624-1630`). - **What Buf cannot cover**: the minor ledger (`docs/reference/ipc.md:1738-1752`) is prose with no code check that `PROTOCOL_MINOR` (`crates/felis-protocol/src/preface.rs:57`) matches the last row; the row-codec golden vectors are unit tests in `crates/felis-grid/src/wire.rs:959-985` and the carrier block vector in `preface.rs:614`, which run under `cargo nextest` in the `build` job — these are already gates but nothing names them as the compatibility gate. The issue's "keep them as separate gates" is accurate. - **Intentional breaks**: the acknowledgement path today is `SKIP=buf-breaking` at commit time (`flake.nix:378,387`), invisible to CI. `docs/reference/ipc.md:1685-1687` states pre-freeze breaks ride major 1 unbumped, which is the policy the acknowledgement must encode. ## Verdict **accept**, with one addition and one clarification. Addition: the hook's `git rev-parse` guard must not be the CI mechanism at all. Move the CI-facing check into a dedicated `pr.yml` job that runs `buf breaking` inside `nix develop` with an explicit base, and leave the pre-commit hook as the developer-side early warning (its `HEAD` comparison is correct *there*). Do not try to make `nix flake check` do this: the sandbox has no history by design. Clarification on "explicit pre-release acknowledgement": use a file, not a commit-message trailer or a `SKIP` env, because a file is visible to the PR diff, to `git log -p`, and to the release gate (#18). Recommended: `crates/felis-protocol/proto/BREAKING.md` (or a `breaking:` field in a small `proto/compat.toml`) that names the base revision the break is acknowledged against; the CI job passes when either buf is clean or the file names `github.event.pull_request.base.sha`/`github.event.before` exactly. On tag (#18) the file must be absent or reference a revision *before* the previous tag, and post-tag a break additionally requires `PROTOCOL_MAJOR` to have moved — the #30 rule. Principles: pure process/CI, no product surface. Pass. ## Approach ### `pr.yml` — new `proto-compat` job (runs on `nix`) ```yaml proto-compat: name: wire schema is compatible with the base runs-on: nix steps: - uses: actions/checkout@… # same pin as pr.yml:28 with: { fetch-depth: 0 } # buf needs the base object; see bench.yml:68 for the precedent - run: nix develop -c scripts/proto/compat.sh ``` `scripts/proto/compat.sh` (new; the `scripts/` tree already holds `bench/` and `make-macos-app.sh`): 1. Resolve `BASE`: `pull_request` → `github.event.pull_request.base.sha`; `push` to `main` → `github.event.before`, falling back to the latest `v*` tag when `before` is the null sha (first push / force) — and once a tag exists, prefer `git describe --tags --abbrev=0 HEAD~1` so the comparison is against the released baseline, which is what the issue's "prior commit or latest release tag" asks for. Fail with a named error if `BASE` cannot be resolved or `git cat-file -e "$BASE^{commit}"` fails (this is the "missing history cannot become success" criterion: `set -eu`, no `|| exit 0`). 2. `buf breaking --against ".git#ref=$BASE"`; on failure, accept only if the acknowledgement file names `$BASE` (exact 40-hex match) *and* the workflow is not a tag build. 3. Print which base was used so the run log shows the documented selection. Also amend the `proto` job comment at `pr.yml:45-52` and the hook comment at `flake.nix:376-383` so neither claims `nix flake check` is the CI gate. ### `justfile` - `just proto-compat [base]`: same script, defaulting to `main`'s merge-base locally (`git merge-base HEAD origin/main`), so the local runner mirrors CI 1:1 as the header at `justfile:1-10` promises. ### Fixtures / tests - The "deliberately breaking proto fixture fails the workflow" criterion is best proven once, not kept: open a throwaway PR that deletes a field from `felis.proto` and record the red run URL in the issue; a permanent negative fixture would need a second buf module and an `expect-fail` harness that Forgejo Actions cannot express cleanly. Alternatively, `scripts/proto/compat_test.sh` can build a temp git repo with two commits (additive vs breaking) and assert the script's exit codes — cheap, hermetic, and runs in `just check`; recommend this. - Additive changes "still require regenerated code": already enforced by the `proto` job's `git diff --cached --exit-code` (`pr.yml:63`); no new work, just cite it. - Ledger gate: add a unit test in `felis-protocol` that parses `docs/reference/ipc.md`'s ledger table (or a mirrored const list in `preface.rs`) and asserts the highest row equals `PROTOCOL_MINOR`. Keeping the ledger in prose only means a minor bump with no row cannot be caught; a `const MINOR_LEDGER: &[(u16, &str)]` in `preface.rs` with a doc-sync test (same pattern as the config schema sync test at `crates/felis-client-core/src/config.rs:2476`) is the smallest form. If #30 drops the ledger for the 2.0 baseline, this becomes a one-row table; keep the test. - Row-codec vectors: already `wire.rs:959-985`; name them in testing.md as the codec half of the compatibility gate. ### Docs cascade - `docs/reference/testing.md:1073-1078` and the pipeline list: describe `proto-compat` (base selection, acknowledgement file, tag behavior) and list the three compatibility gates (buf WIRE_JSON, ledger sync test, row-codec/carrier golden vectors) with what each covers. - `docs/reference/ipc.md:1674-1687` "Versioning": one paragraph on how a pre-freeze break is acknowledged (the file) and what changes after the first tag (major bump mandatory). - `docs/explanation/architecture/ipc.md` (schema-evolution section around `:896-920`): the why — Buf checks field-level wire compatibility, not semantics (renamed meaning, kinds/variants fatal rule at `ipc.md:1699-1706`), which is why the ledger and vectors stay separate; rejected alternatives (`SKIP=` env: invisible to CI; commit trailer: not diffable; `nix flake check`: no history in sandbox); "Revisit if" Buf gains a Rust-aware semantic plugin or the wire moves to a registry. - `.claude/skills/extend-ipc/SKILL.md` step for proto edits: add "run `just proto-compat`; for an intended break write the acknowledgement file". - `CHANGELOG.md`: not user-affecting; no entry. - `skills/felis`: no change. ## Dependencies - None hard; the tracker's order (#19 with #45, before #17) still holds and is the right sequence because #17 and #30 both edit `felis.proto` and should be the first changes the new gate sees. - **#30** decides the post-tag rule text ("protocol-major breaks after v0.1.0"); write the doc paragraph so #30 fills in the major number rather than restating the policy. - **#18** consumes the tag-mode behavior (acknowledgement file must not permit a break on a tag build). ## Risk/effort **S–M.** One workflow job, one script with a hermetic test, one const + sync test, docs. Main risk: base resolution edge cases on Forgejo (`github.event.before` on the first push after a force, or a PR from a fork where `base.sha` is stale after the base branch moves — use the merge-base of `base.sha` and `HEAD` to be safe). Secondary: `fetch-depth: 0` cost on a runner with a warm checkout is negligible for this repo's size. ## Labels Keep `priority/P0`, `release/v0.1.0`. Suggest adding an acceptance line: "the pre-commit hook's `.git` guard is no longer the only CI path; `nix flake check` is not claimed as the compatibility gate anywhere in `docs/` or workflow comments". ## Review amendments (round 4) - **The compatibility baseline is committed release state, not "latest `v*` tag".** A tag exists even when its release run failed, and rehearsal tags are pushed deliberately. The base for `proto-compat` on a tag build is the schema recorded in a committed baseline file `crates/felis-protocol/proto/baseline/felis.proto` plus `baseline/RELEASE` (the final release version and commit it was published from), which #18's `release` job updates (commits to `main`) only after a *final* (non-prerelease) release publishes successfully. PR/push builds keep the merge-base / `event.before` base. When the baseline file is absent (before the first final release) the workflow requires the committed `first-release` acknowledgment instead; that acknowledgment is tied to the absence of the baseline, not to tag ordinality, so rc tags before v0.1.0 do not consume it. Prerelease tags never update the baseline. ## Review amendments (round 5) - **Baseline lives outside the Buf module.** `buf.yaml` roots the module at `crates/felis-protocol/proto`, so a copy inside it would be a duplicate declaration. Store the baseline at repo root `proto-baseline/felis.proto` + `proto-baseline/RELEASE` (outside every Buf module; `buf breaking --against` takes it as a separate input directory). Add a test to `scripts/proto/compat.sh`'s self-test that `buf lint` on the module and the baseline comparison both pass with the committed layout. - **Baseline absence is not proof of no release.** Before every final tag, `verify-tag` queries the Forgejo release API: (a) if any final release exists, `proto-baseline/RELEASE` must name the latest one and its schema must equal that release's attached `felis.proto` (byte compare of the release asset), otherwise fail with "baseline behind the published release; reconcile first"; (b) `first-release` is accepted only when the API reports no final release. The baseline update is therefore reconciled before publication, and a failed post-publish baseline commit blocks the next tag rather than silently narrowing the check.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
ci/darwin-smoke
No results found.
Labels
Clear labels   
priority/P0
Must be resolved before the v0.1.0 tag

  
priority/P1
Must be settled before the first public contract freezes

  
priority/P2
Explicitly deferred until after v0.1.0

  
release/v0.1.0
Work tracked for the first tagged release

  
status/blocked
Cannot start now: waits on an upstream release, another issue, or a recorded revisit trigger

  
status/planned
Triaged with a source-grounded plan and a passed pi review; ready to implement

  
type/bug
Shipped behavior that contradicts the docs or the code's own intent; needs a product code change

  
type/design
An open scope or trade-off question; needs a recorded decision in the owning explanation doc before code

  
type/test-gap
Documented or reviewed behavior that no test pins; usually split from a review finding, needs no product code change

  
type/tracker
Parent issue that tracks child work

No labels
priority/P0
priority/P1
priority/P2
release/v0.1.0
status/blocked
status/planned
type/bug
type/design
type/test-gap
type/tracker
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
natsukium/felis#19
No description provided.