ci: run the wire-compat gate unconditionally on pushes to main - #315
Conversation
On a PR the path filter answers "does this change touch the wire surface". On a push it answers something else entirely -- it diffs only the push that triggered it -- so a commit that reached main without the gate ever running is never re-checked, and the job still reports green because "filter said no" and "gate passed" are the same success. #297 merged during the 2026-08-06 Actions outage with no run against any of its SHAs, changing seven files under pg-compat/**. The three main runs after it (506144c, c1682ba, 1238389) each reported "Wire compat: success" with the seal and open steps skipped, so the wire surface it touched has never been through the gate it belongs to. Route both halves through one gate step so the push override cannot be applied to the seal and forgotten on the open. PR behaviour is unchanged: the filter still skips the expensive work, and the job still always reports. Refs #299
|
Thanks for the writeup — I'm on it. I'll review the |
…h-filter blind spot The old text said required checks come from classic protection "not from the main ruleset", which is now false twice over: the ruleset it dismissed matched no refs and enforced nothing, and enforcement now genuinely lives in both places. Only the ruleset binds an admin. Records what was measured rather than assumed: --admin is rejected by a ruleset required check that is absent, not merely failing; a ruleset with an empty ref_name.include looks active and enforces nothing; and rules/branches/main is the endpoint that reports what actually binds. Also the general lesson behind the build.yml change: a path filter asks a different question on push than on a PR, and a green from a skipped filter is indistinguishable from a green from a passing gate. Refs #299
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
The diagnosis holds up under checking, and so does every factual claim in it.
I re-verified the evidence rather than taking it on trust: 506144c2, c1682ba7 and 1238389b each report Wire compat: success on main (gh api .../commits/<sha>/check-runs), and ruleset 20607291 (main: required checks) is enforcement: active with bypass_actors: [] and exactly one context, Wire compat — so the comment's warning that renaming the job silently disarms the gate is literally true, and it is the right place for that warning to live. Routing both halves through one gate step rather than duplicating the override on each if is the part that will still be correct in a year.
Approving. Three notes below, none blocking, one of which has a one-click suggestion.
The one worth taking: workflow_dispatch is a trigger on this workflow and still routes through the path filter, so the manual "Run workflow" button — the natural escape hatch during the next outage — keeps reporting the skipped green this PR exists to remove. Details inline at build.yml:171.
Docs, checked at HEAD: the second commit (40a6009) already fixes what I would otherwise have raised about CLAUDE.md — the old "required checks come from classic protection, not the main ruleset ... ask a maintainer rather than inferring" text is gone, replaced by the two-system bullet, and the new push-filter bullet documents the behaviour change including the honest note that pg-compat-lint still carries the plain filter. One residual: the pg-compat bullet further down (line 11, not in this diff) still describes the gate as running "on any PR touching the wire surface", which is now half the story. Not worth its own commit; worth folding in the next time that bullet is touched.
Also not blocking, and not anchorable (it sits between the diff's two hunks): with the gate in place, the filter's output is dead on push — computed, then ignored — and pull-requests: read is likewise only needed for PR events. An if: github.event_name == 'pull_request' on the dorny/paths-filter step at build.yml:146 would drop a few seconds and, more usefully, remove a failure surface that no longer buys anything: a force-push whose before has been gc'd errors that step and reds the required Wire compat for a reason unrelated to wire compatibility. Note this only works together with the build.yml:171 change — skipping the step leaves WIRE_CHANGED empty, which under the current == "push" test yields run=false on a dispatch. Under the suggested != "pull_request" test it yields run=true, which is what you want.
| env: | ||
| WIRE_CHANGED: ${{ steps.changes.outputs.wire }} | ||
| run: | | ||
| if [[ "$WIRE_CHANGED" == "true" || "$GITHUB_EVENT_NAME" == "push" ]]; then |
There was a problem hiding this comment.
workflow_dispatch is in this workflow's on: block, and it is neither push nor a PR — so it takes the else branch and still depends on the filter.
That matters because dispatch is the entry point a human reaches for precisely in the scenario this PR is about. Our own notes on this action (read from src/main.ts at 7b450ff, v4.0.2, while reviewing #266) spell out the fallback: for a non-pull_request event it calls getChangedFilesFromGit(base, ref), where head falls back to github.ref and base falls back to the default branch. Dispatching from main makes those the same name, isBaseSameAsHead is true, and beforeSha is populated only when eventName === 'push' — so it logs 'before' field is missing in event payload - changes will be detected from last commit and diffs only the last commit.
So a maintainer who dispatches this workflow to force a re-check after an incident gets wire=false unless main's single most recent commit happened to touch the wire surface: Seal/Open skipped, Wire compat: success, nothing actually opened. That is the same indistinguishable green, on the one entry point where somebody explicitly asked for the work.
The filter is only the right instrument on a PR — which is what the comment at line 114 already argues. Inverting the test says exactly that, and covers any trigger added later (schedule, repository_dispatch, …):
The narrower form our notes prescribe, || github.event_name == 'workflow_dispatch', also works if you would rather enumerate. Either way the comment at line 127 ("on a push the gate always does the real work") wants widening to match — "on anything but a PR".
| if [[ "$WIRE_CHANGED" == "true" || "$GITHUB_EVENT_NAME" == "push" ]]; then | |
| if [[ "$WIRE_CHANGED" == "true" || "$GITHUB_EVENT_NAME" != "pull_request" ]]; then |
| # success. That is not hypothetical: #297 changed seven files under | ||
| # `pg-compat/**` and merged during the 2026-08-06 Actions outage with no run |
There was a problem hiding this comment.
Nit, and the off-by-one is the interesting file. #297 changed seven files, but only six are under pg-compat/**; the seventh is root COMPATIBILITY.md, which is not in the wire filter at all (the filter lists pg-core/**, pg-wasm/**, pg-compat/**, pg-compat-js/**, root Cargo.lock/Cargo.toml, and build.yml). Confirmed with gh pr view 297 --json files.
Doesn't weaken the argument — six is still squarely inside the filter — but the rest of this block is precise to the SHA (I checked 506144c2, c1682ba7, 1238389b: all real, all on main, all reporting Wire compat: success), so it is worth keeping exact.
| # success. That is not hypothetical: #297 changed seven files under | |
| # `pg-compat/**` and merged during the 2026-08-06 Actions outage with no run | |
| # success. That is not hypothetical: #297 changed seven files, six under | |
| # `pg-compat/**`, and merged during the 2026-08-06 Actions outage with no run |
| - Workspace members: `pg-core` (lib), `pg-ffi` (C ABI), `pg-pkg` (PKG service), `pg-cli`, `cryptify` (file-transfer service, merged in 2026-07-31). `pg-wasm` is a sibling crate the root `Cargo.toml` lists under `exclude`, so it is not part of the workspace and is built separately with wasm-pack (see Release & configuration). Sub-crates share workspace files. Build the workspace from repo root with `cargo build`. Two test invocations are worth knowing, and the obvious reading of each is wrong. **`cargo test --workspace` passes** — 296 tests over 10 targets (9 binaries plus `pg-core`'s doc-tests), verified 2026-08-09 — but only by accident, so don't read it as the workspace being self-consistent. `pg-core`'s tests really are gated behind its `test` feature (also `rust`/`stream`), and `cargo test -p pg-core` alone still fails exactly as it always did, with `unresolved import crate::test` on `crate::test::TestSetup`. What rescues the workspace build is feature unification: `cryptify`'s dev-dependency asks for `pg-core` with `features = ["rust", "stream", "test"]` (`cryptify/Cargo.toml`), so the feature is on for every member — confirm with `cargo tree -e features -p pg-core --workspace`. That has only been true since `cryptify` joined the workspace on 2026-07-31; it is a side effect of one member's dev-deps rather than a property of the workspace, and it reverts to the old compile failure the moment that line changes. **`cargo test --workspace --all-features` does not compile at all**, on any host: it switches on `pg-core`'s `web`, whose `compile_error!` fires off wasm32 — `error: "web" feature should only be enabled on wasm32 targets`, `pg-core/src/client/web/mod.rs:19`. The axis is whether `pg-core` is in the selected set, **not** how the invocation is scoped: `cargo test -p pg-core --all-features` fails identically, so this is not a workspace-wide problem that scoping fixes. `--all-features` is correct only for the members that declare no features of their own — `pkg`, `cli` and `ffi` in the CI matrices, and the api_gate suite further down, which lives in `pg-pkg` — where it is a no-op. `pg-core` is the sole member with a `[features]` section, so it is the sole member `--all-features` can break. CI (`.github/workflows/build.yml`) runs tests per crate: `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` for core, `--all-targets` for `cryptify` (it declares no features of its own, so that is the equivalent invocation), `--all-features` for `pkg`/`cli`/`ffi`. None of these cover `pg-wasm`. The `test`/`format`/`clippy` matrices key on the crate **directory** rather than a `pg-` suffix, because `cryptify` doesn't carry that prefix — so the check contexts are `Test workspace (pg-core)`, not `Test workspace (core)`. Nothing required points at them today, but whatever adds to the required list later must use the current names. Where that list lives is not obvious and an agent cannot read it: required checks come from **classic branch protection** (`gh api repos/encryption4all/postguard/branches/main/protection`, currently exactly `Wire compat`), *not* from the `main` ruleset, which carries only `deletion`/`non_fast_forward`/`pull_request` and no `required_status_checks` rule at all. `dobby-coder` gets 403 on the protection endpoint, so reading the ruleset alone concludes nothing is required, which is wrong — ask a maintainer rather than inferring. `pg-core` uses CGWKV + MKEM for multi-recipient encryption (production feature set `["cgwkv", "mkem"]`). [#262](https://github.com/encryption4all/postguard/issues/262) (merged `0343369`, #295) replaced the two per-language `Wire compat (published pg-core)` / `Wire compat (published pg-wasm/pg-js)` required checks with this single `wire-compat` job (`needs: [wire-compat-rust, wire-compat-js]`, `permissions: contents: read`); the two old names are no longer required and the underlying per-language jobs still exist and still run, just aren't required individually anymore. | ||
| - Workspace members: `pg-core` (lib), `pg-ffi` (C ABI), `pg-pkg` (PKG service), `pg-cli`, `cryptify` (file-transfer service, merged in 2026-07-31). `pg-wasm` is a sibling crate the root `Cargo.toml` lists under `exclude`, so it is not part of the workspace and is built separately with wasm-pack (see Release & configuration). Sub-crates share workspace files. Build the workspace from repo root with `cargo build`. Two test invocations are worth knowing, and the obvious reading of each is wrong. **`cargo test --workspace` passes** — 296 tests over 10 targets (9 binaries plus `pg-core`'s doc-tests), verified 2026-08-09 — but only by accident, so don't read it as the workspace being self-consistent. `pg-core`'s tests really are gated behind its `test` feature (also `rust`/`stream`), and `cargo test -p pg-core` alone still fails exactly as it always did, with `unresolved import crate::test` on `crate::test::TestSetup`. What rescues the workspace build is feature unification: `cryptify`'s dev-dependency asks for `pg-core` with `features = ["rust", "stream", "test"]` (`cryptify/Cargo.toml`), so the feature is on for every member — confirm with `cargo tree -e features -p pg-core --workspace`. That has only been true since `cryptify` joined the workspace on 2026-07-31; it is a side effect of one member's dev-deps rather than a property of the workspace, and it reverts to the old compile failure the moment that line changes. **`cargo test --workspace --all-features` does not compile at all**, on any host: it switches on `pg-core`'s `web`, whose `compile_error!` fires off wasm32 — `error: "web" feature should only be enabled on wasm32 targets`, `pg-core/src/client/web/mod.rs:19`. The axis is whether `pg-core` is in the selected set, **not** how the invocation is scoped: `cargo test -p pg-core --all-features` fails identically, so this is not a workspace-wide problem that scoping fixes. `--all-features` is correct only for the members that declare no features of their own — `pkg`, `cli` and `ffi` in the CI matrices, and the api_gate suite further down, which lives in `pg-pkg` — where it is a no-op. `pg-core` is the sole member with a `[features]` section, so it is the sole member `--all-features` can break. CI (`.github/workflows/build.yml`) runs tests per crate: `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` for core, `--all-targets` for `cryptify` (it declares no features of its own, so that is the equivalent invocation), `--all-features` for `pkg`/`cli`/`ffi`. None of these cover `pg-wasm`. The `test`/`format`/`clippy` matrices key on the crate **directory** rather than a `pg-` suffix, because `cryptify` doesn't carry that prefix — so the check contexts are `Test workspace (pg-core)`, not `Test workspace (core)`. Nothing required points at them today, but whatever adds to the required list later must use the current names. Where that list lives is not obvious and is now in **two** places at once — see the branch-protection bullet below, which is the authority; the short version is that `Wire compat` is required by both classic protection and a ruleset, and only the ruleset actually binds an admin. `pg-core` uses CGWKV + MKEM for multi-recipient encryption (production feature set `["cgwkv", "mkem"]`). [#262](https://github.com/encryption4all/postguard/issues/262) (merged `0343369`, #295) replaced the two per-language `Wire compat (published pg-core)` / `Wire compat (published pg-wasm/pg-js)` required checks with this single `wire-compat` job (`needs: [wire-compat-rust, wire-compat-js]`, `permissions: contents: read`); the two old names are no longer required and the underlying per-language jobs still exist and still run, just aren't required individually anymore. | ||
| - **`main` is protected by two systems at once, and only one of them binds an admin.** **Classic protection** (`gh api repos/encryption4all/postguard/branches/main/protection`) requires `Wire compat` and one approving review with `enforce_admins: false`, so `gh pr merge --admin` walks past all of it. That bypass is the *normal* path here, not an emergency one: 11 of the 15 merges before 2026-08-09 carried zero approving reviews, because `dobby-coder` cannot self-approve and neither can a maintainer authoring their own PR. Leaving it that way is deliberate — it is what keeps solo merges moving while still making `dobby-coder`'s PRs need a human approval, since the App is not an admin and cannot use `--admin`. **Rulesets** (`gh api repos/encryption4all/postguard/rulesets`) carry the half that must *not* be bypassable: `main: required checks` (id `20607291`, `bypass_actors: []`) requires exactly `Wire compat`. Repo admins do **not** implicitly bypass a ruleset and `--admin` does not reach one — measured on #315 with the check *absent* rather than failing: `gh pr merge --squash --admin` → `GraphQL: Repository rule violations found` / `Required status check "Wire compat" is expected.` Absent-not-failing is the whole point (#299): classic protection waves a required check through whenever it has nothing to report, which is how #297 merged during the 2026-08-06 Actions outage with zero runs against any of its three SHAs. Read what actually binds with `gh api repos/encryption4all/postguard/rules/branches/main`; it returns the *effective* rules, and returns `[]` for a ruleset matching no refs, which the rulesets list itself will not tell you (whether that endpoint is readable by `dobby-coder`, which gets 403 on the protection endpoint, is untested). Three consequences. `Wire compat` is now **a name with teeth** — the ruleset pins that literal context string, so renaming the `wire-compat` job disarms the gate silently instead of breaking it loudly; rename both in one change. A ruleset that matches no refs **enforces nothing while looking active**: the old `main` ruleset (`conditions.ref_name.include: []`, deleted 2026-08-09, backup in that PR's thread) looked like the enforcement story and was inert, which is what an earlier version of this file got wrong — always confirm against `rules/branches/main`. And when Actions is down and something genuinely must merge, the escape hatch is deliberate and on the record rather than a silent `--admin`: `gh api -X PUT repos/encryption4all/postguard/rulesets/20607291 -f enforcement=disabled`, merge, then the same call with `enforcement=active`. Ruleset changes land in the audit log; the next push to `main` re-verifies for real, so the unverified commit does not stay unverified. | ||
| - **A path filter answers a different question on `push` than on `pull_request`, and the two failure modes look identical from the outside.** On a PR, `dorny/paths-filter` asks "does this change touch the surface"; on a push it diffs only the push that triggered it, so a commit that reached `main` *without* its gate ever running is never re-checked — and the job still reports green, because "filter said no" and "gate passed" are the same `success`. Measured: #297 changed seven files under `pg-compat/**` and merged with no run at all, then `506144c2`, `c1682ba7` and `1238389b` each reported `Wire compat: success` with `Seal the sample set with HEAD=skipped, Open it with published pg-core=skipped`. Three green reports, zero executions against the change that slipped through. `wire-compat-rust` therefore bypasses its filter on `push` (one `gate` step feeding every conditional, so the override cannot be applied to the seal and forgotten on the open); `wire-compat-js` needed no change because it keys off `needs.wire-compat-rust.outputs.sealed`. `pg-compat-lint` still carries the plain filter and has the same blind spot, which is fine for a lint and not for a gate. Any new always-reports gate on this repo should do the same: cheap filter on the PR, real work on the push. |
There was a problem hiding this comment.
Same nit as build.yml:123, carried into this bullet: "#297 changed seven files under pg-compat/**" — seven files changed, six of them under pg-compat/**, the seventh being root COMPATIBILITY.md.
No suggestion attached because this bullet is one long line and a block replacement here would be a poor thing to apply blind; the two words are seven files, six under.
The rest of the bullet checks out against the live repo, including the parts that are easy to get wrong: ruleset 20607291 really does have bypass_actors: [] and exactly one required context, Wire compat (gh api repos/encryption4all/postguard/rulesets/20607291), and pg-compat-lint really does still carry the plain filter with per-step if: steps.changes.outputs.wire == 'true' — good call flagging that it has the same blind spot and that this is fine for a lint and not for a gate.
|
Reviewed and approved #315 — nothing here blocks merge, it's a strict improvement over what I re-verified the diagnosis against HEAD before posting, and left three non-blocking notes:
Two findings handed to me turned out stale — a later commit ( Nice fix for the detect half of #299. |
1 participant
The detect half of #299. The prevent half is a branch-ruleset change, which lives in no file — it is recorded on the ticket.
The bug
On a PR,
dorny/paths-filteranswers "does this change touch the wire surface?". On apushit answers something else: it diffs only the push that triggered it. So a commit that reachedmainwithout the gate ever running is never re-checked afterwards — and the job still reports green, because "filter said no" and "gate passed" are the samesuccess.That is live today, not hypothetical:
pg-compat/**, squarely inside the wire filter.mainrun since reportsWire compat: successwithSeal the sample set with HEAD=skipped, Open it with published pg-core=skipped:506144c2,c1682ba7,1238389b.Three green reports, zero executions against the change that slipped through. The gate this map spent #251/#260/#261/#262 building has been vouching for a wire surface it never opened.
The fix
On
push, always do the real work; on a PR, keep the cheap filter. Both halves route through onegatestep, so the override cannot be applied to the seal and forgotten on the open —wire-compat-jsneeds no change, since it already keys offneeds.wire-compat-rust.outputs.sealed.Whatever is on
mainis now what gets sealed and opened, however it got there. A commit that lands unverified — outage, admin merge, anything — is verified on the next push instead of hiding behind a skipped green.Also corrects a stale comment on the
wire-compatjob: the repoint it says is "left for a maintainer" happened in #296.Note on the gate's own name
Wire compatis now the sole context of themain: required checksruleset, whosebypass_actorsis empty. Renaming this job silently disarms that ruleset — a ruleset requiring a context that no job produces blocks nothing once the old name stops appearing. The comment inbuild.ymlsays so at the point of the hazard.