ci: run the wire-compat gate (published pg-core opens HEAD-sealed samples)

[rubenhensen]

The workflow half of #265, which merged without it (the App can't push workflows), leaving pg-compat inert — same pattern as the dotnet pin. This is dobby's YAML from the #265 comment with two changes: dorny/paths-filter bumped v3→v4.0.2 and SHA-pinned, since that step decides whether the gate's teeth engage.

Design notes carried over from the comment: the path filter is a step, not on: paths:, so the job always reports a status and can become a required check (#262) without wedging non-wire PRs; the artifact upload happens before the reader runs, so a red gate still leaves the bytes that broke it for #261's JS half.

Self-testing: this PR touches build.yml, which is in the filter, so the full gate runs here — expect Wire compat (published pg-core) green below.

Refs #260 #251. Part of #247 (workstream C).

[dobby-coder]

Got it — I'm on it. I'll review the dorny/paths-filter bump/pin and the rest of the gate wiring in this PR now, including the self-test expectation that build.yml's change trips the filter and turns Wire compat (published pg-core) green.

[dobby-coder]

Verified against HEAD 394ef02 and its own CI run (30338358017), where Wire compat (published pg-core) is green in 1m17s — the self-test in the PR body holds. The shape is right and the design notes are accurate: the filter is a step so the job always reports (this is exactly what the reason it was written this way asks for, and it means #262 can make this required without wedging non-wire PRs), the workflow file is in its own filter so a change to the job re-runs it, and the upload genuinely precedes the reader — I confirmed the artifact exists on this run.

Requesting changes on one thing, plus a second one-liner in the same file. Both are in the trigger/filter, not in the gate's body.

Blocking — the filter omits the root lockfile. Details inline at the filter block. Short version: pg-core has no Cargo.lock of its own (I checked — only the root has one, pg-compat/Cargo.lock is the other), and the seal step runs cargo run --locked, so the bytes HEAD produces are a function of the root Cargo.lock. A cargo update/dependabot PR bumping ibe, ibs, serde or bincode-next touches only that file, wire evaluates false, every step skips, and the gate reports green having sealed and opened nothing — on exactly the class of change CLAUDE.md singles out as wire-critical (bincode_next::config::legacy() byte compatibility, and "a bincode-next rc bump" specifically). Two lines fix it.

Also worth folding in — on: pull_request: has no types:, so no edited. Can't anchor this inline; the trigger block at the top of build.yml is outside the diff. Per GitHub's documented default, a bare on: pull_request: subscribes to opened/synchronize/reopened only. A retarget — by hand, or GitHub's automatic one when a stacked base is deleted on merge — fires edited, which nothing here listens for. paths-filter derives its verdict from the base, so after a retarget the last run's wire verdict stays attached to the unchanged head sha and GitHub reports it as current. For a check slated to become required, a stale green is a passing required check on a comparison never made against the actual merge target. Since this PR is the one introducing the first base-dependent job in build.yml:

on:
  pull_request:
    types: [opened, synchronize, reopened, edited]
    branches: [main]

Docs now understate the gate. Two lines describe this job as unlanded, and after this PR they sit in the same tree as the job itself — our own note on patch-in-comment gates says that tightening belongs in this PR, not a follow-up, because the pending wording sends a later agent run looking for a patch comment that is spent:

• CLAUDE.md:8 — "The wire-compat-rust job that wires the two together is NOT yet a CI gate: the YAML sits in a comment on PR #265 waiting for a maintainer to apply it, so nothing stops a non-additive wire change until then". False once this merges. The same line also says pg-compat "is covered by none of the per-crate test/fmt/clippy matrices, so run [fmt and clippy] by hand" — this PR is what starts covering them, so that clause needs the same pass.
• pg-compat/README.md:144 — "The proposed wire-compat-rust job seals and uploads only when the PR touches the wire surface". Drop "proposed"; the rest of that paragraph's advice to #261 stays correct and is worth keeping.

Three non-blocking notes are inline (missing permissions:, the pg-compat lint sitting in front of the gate, and the artifact name recording the merge commit rather than the head commit — verified: this run's artifact is wire-compat-artifacts-821c3bc2… while HEAD is 394ef022…, which makes pg-compat/README.md's "CI records which commit sealed a set in the uploaded artifact's name" not quite true as written).

Checked and clean: no piped run: line, so the pipefail trap isn't in play (seal and the reader declare shell: bash anyway); title passes the pr-title.yml Conventional Commit check; pg-core/examples/seal-samples and PG_COMPAT_ARTIFACTS both exist at HEAD; steps.seal.outcome == 'success' correctly keeps the upload out of the skipped path.

[dobby-coder]

Blocking. The wire filter omits the root Cargo.lock (and root Cargo.toml).

pg-core has no lockfile of its own — I checked the tree at HEAD: only the repo root has a Cargo.lock, plus the one inside the out-of-workspace pg-compat. pg-core is a workspace member, so it resolves versions from the root lockfile, and the seal step below runs cargo run --locked. The bytes HEAD produces are therefore a function of the root Cargo.lock.

So: a cargo update or dependabot PR that bumps ibe, ibs, serde or bincode-next touches only Cargo.lock, steps.changes.outputs.wire evaluates to false, every step below skips on its if, and the job reports green having sealed and opened nothing. That is precisely the class of change CLAUDE.md flags as wire-critical — the bincode_next::config::legacy() byte-compatibility property, and the explicit warning that a bincode-next rc bump is wire-relevant. The gate would be silent on the one PR shape it most needs to catch, and once #262 makes it required that silence is a passing required check.

Suggested change

	- '.github/workflows/build.yml'
	- 'pg-core/**'
	- 'pg-wasm/**'
	- 'pg-compat/**'
	- 'Cargo.lock'
	- 'Cargo.toml'
	- '.github/workflows/build.yml'

Root Cargo.toml earns its place too: it holds the workspace dependency table and the exclude list that keeps pg-compat pointed at crates.io pg-core rather than ../pg-core. CLAUDE.md calls that exclusion load-bearing, and a change to it would quietly turn the gate into a self-comparison.

[dobby-coder]

No permissions: block on the job. dorny/paths-filter defaults token to ${{ github.token }} and, on pull_request, fetches the changed-file list from the REST API — its README lists pull-requests: read as required. It works here because the repo default is permissive, but this is the first GITHUB_TOKEN-consuming step in build.yml (I grepped: zero permissions: keys in this file, versus 7 in delivery.yml). For a check slated to become required, worth pinning:

    permissions:
      contents: read
      pull-requests: read

Good news on the failure mode: the action throws rather than falling back, so a missing permission reds the job instead of silently returning zero changed files. Non-blocking.

[dobby-coder]

Format pg-compat and Clippy pg-compat sit in front of the gate in the same job. Filling the lint gap for the out-of-workspace crate is the right call — nothing else covers it — but coupling it to the wire verdict has two costs: (a) clippy -D warnings under dtolnay/rust-toolchain@stable reds this check whenever a new stable rustc adds a lint, and (b) when it does, the steps that actually answer "do published readers still open our bytes?" never run, because the lint precedes them and the implicit success() on their if skips them.

Once #262 makes Wire compat (published pg-core) a required check, that is a required check going red for reasons unrelated to what its name claims. Either split the pg-compat lint into its own job, or move these two steps after the seal/read steps. Non-blocking, but cheaper to decide now than after it is required.

[dobby-coder]

wire-compat-artifacts-${{ github.sha }} records the merge commit on pull_request events, not the head commit. Verified on this PR's own run: the artifact is wire-compat-artifacts-821c3bc26f8e7710cb776c2e9479f6bf1abbba28 while the PR head is 394ef0228042548e8d550d979ce525fb6b0c00e2.

pg-compat/README.md says "CI records which commit sealed a set in the uploaded artifact's name", and leans on that because the manifest deliberately carries no commit hash. But that merge sha belongs to no branch and is GC'd, so the name does not identify a tree anyone can re-seal from.

Suggested change

	name: wire-compat-artifacts-${{ github.sha }}
	name: wire-compat-artifacts-${{ github.event.pull_request.head.sha || github.sha }}

Keeps the intra-run handoff to #261 working (same run, same expression on both sides) and makes the name resolvable afterwards. Nit.

[dobby-coder]

Nit on the pinning rationale, not the pin. The comment above pins paths-filter because "this step decides whether the gate's teeth engage" — but dtolnay/rust-toolchain@stable here is a mutable branch ref, which is weaker than the retagged-tag case the pin defends against, and it supplies the compiler that both seals the samples and builds the reader. It has at least as much say in the verdict. actions/checkout@v4 and actions/upload-artifact@v4 are likewise floating.

Consistent with the rest of build.yml, so fine to leave as-is — but the stated reasoning does not survive being applied to only one of the four actions. Either pin all four or soften the comment.

[dobby-coder]

Reviewed #266 and requested changes — review here. Confirmed the self-test worked: Wire compat (published pg-core) ran green in 1m17s and the artifact uploaded, so the step-based filter design holds up for #262.

One blocker: the wire: filter doesn't watch the root Cargo.lock (or Cargo.toml). Since pg-core has no lockfile of its own and cargo run --locked seals against the root one, a dependency-bump PR would skip every step and report a false-green gate — right on the class of change this workflow exists to catch.

Also flagged: the pull_request trigger is missing types: [..., edited], so a base retarget leaves a stale wire verdict attached; two docs lines (CLAUDE.md:8, pg-compat/README.md:144) still describe the gate as unlanded even though this PR lands it; and three non-blocking nits (missing permissions: block, lint ordering ahead of the gate, artifact name using the merge commit instead of HEAD).

Everything else checked out clean — Conventional Commit title, no piped run:, and the skip logic around steps.seal.outcome.

[rubenhensen]

/dobby all six addressed:

• Blocking: root Cargo.lock + Cargo.toml added to the filter, with an inline comment carrying your reasoning — the dependabot-bumps-bincode-next-and-the-gate-stays-silent scenario was exactly the failure mode this gate exists to prevent. Good catch.
• types: [opened, synchronize, reopened, edited] on the trigger, with a comment on why edited matters for base-dependent verdicts.
• permissions: contents: read, pull-requests: read pinned on both jobs.
• Lint split into its own pg-compat-lint job — a new stable clippy lint can no longer red a check named after wire compatibility or pre-empt its verdict.
• Artifact name uses pull_request.head.sha || github.sha, so the README's claim about resolvable commit names is true again.
• Pin-rationale comment softened to the consistency framing.

Docs updated in the same pass: CLAUDE.md's 'NOT yet a CI gate / run by hand' clauses replaced with the landed reality; 'proposed' dropped in pg-compat/README.md.

The push re-runs the gate (build.yml is in the filter). Please re-review.

[dobby-coder]

Thanks for the detailed rundown — I'm on it. Kicking off a fresh review of PR #266 now that all six points are addressed and the gate has re-run.

[dobby-coder]

All six from the last round check out, and the gate is live rather than proposed: run 30339618753 on 62f2585 sealed the sample set with HEAD, uploaded wire-compat-artifacts-62f2585093d5a679dfc144fc9046e24b9e9bc102, and opened it with published pg-core. Every step green, Lint pg-compat included. CLAUDE.md's present-tense rewrite is now backed by a workflow file in the same tree, which is the direction that wording is supposed to travel.

One thing left, and it is the same class of defect the head-sha change was made to fix. pg-compat/README.md still tells #261 to download wire-compat-artifacts-${{ github.sha }}. On a pull_request event that is the merge commit (4bbbbdb on this run), not the head sha the job actually used (62f2585), so a wire-compat-js job written against that sentence 404s on every PR. The description says the README's claim about resolvable commit names is true again; that one expression is what still makes it false. Mirror head.sha || github.sha there and this is done.

Three nits inline, none of them blocking.

Checked and cleared, so you do not have to re-litigate them:

• edited earns its place. on: pull_request: with no types: key subscribes to opened, synchronize and reopened only. A base retarget fires edited and nothing else, whether a human does it or GitHub does it automatically when a stacked base is deleted on merge, so a paths-filter verdict would otherwise stay attached to the old base while GitHub reports the check as current. We watched that happen on cryptify#201, where an automatic retarget fired no run at all. The comment above the key is accurate.
• workflow_dispatch does not silently misreport. dorny/paths-filter defaults base to the repository default branch on events that are neither push nor pull_request, so a manual dispatch from a branch that touches the wire surface does run the seal and read. Only a dispatch on main itself compares against the previous commit and no-ops. If you want a force-run from main, || github.event_name == 'workflow_dispatch' on the conditional steps buys it, but nothing is broken today.
• The step-level filter is the right shape for something destined to be a required check, and build.yml sits in both filter lists, so a change to the jobs re-runs them.

[dobby-coder]

Docs still name the old artifact, three lines below this one: wire-compat-artifacts-${{ github.sha }}.

The job now uploads wire-compat-artifacts-${{ github.event.pull_request.head.sha || github.sha }} (build.yml:153). On a pull_request event github.sha is the merge commit, not the head sha, and this very run proves it: the artifact landed as wire-compat-artifacts-62f2585093d5a679dfc144fc9046e24b9e9bc102 (head) while the PR's merge commit is 4bbbbdb. A wire-compat-js job written against this sentence therefore 404s on every PR, which is the silent-gate failure mode the head-sha change was made to avoid.

Mirror the head.sha || github.sha expression here. The advice that follows, about repeating the path condition on the download or sealing locally in the JS job, stays correct.

[dobby-coder]

Keep edited — it is doing real work. A base retarget fires edited and none of the three default types, so without it this job's paths-filter verdict would stay pinned to the old base while GitHub reports the check as current. The comment is right.

The cost is the other half of edited: it also fires on every title and description edit, and build.yml has no concurrency group, so each edit re-runs the whole workflow (4x Test, 4x Clippy, 4x Format, plus 3 wasm browser jobs including a macOS runner) with nothing cancelling the superseded run. Worth pairing with:

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

Pre-existing gap rather than something this PR introduced; edited just makes it cheap to hit. Non-blocking.

[dobby-coder]

Leftover from the lint split: this job no longer runs fmt or clippy, so components: clippy, rustfmt installs two components nothing in it uses. A bare - uses: dtolnay/rust-toolchain@stable (keeping the if:) is enough for the seal and read steps. The components belong in pg-compat-lint, where they already are.

[dobby-coder]

The two filters: blocks now duplicate Cargo.lock, Cargo.toml and .github/workflows/build.yml (lines 179-184 here and 121-132 in wire-compat-rust). For a pair of gates whose whole value is not going silent, drift between the two lists would be invisible, so a one-line comment on each saying they are kept in step would earn its keep. Non-blocking.

[dobby-coder]

Re-reviewed #266 — all six items from last round check out, and the gate itself is live now: run 30339618753 on 62f2585 went green end to end, including the new pg-compat-lint job.

One thing still needs a tweak, so I requested changes: pg-compat/README.md still tells readers to download wire-compat-artifacts-${{ github.sha }}, but the job now uploads under head.sha || github.sha. On a pull_request event github.sha is the merge commit, not the head sha the artifact actually landed under (62f2585… vs merge commit 4bbbbdb…), so following the README literally 404s. Just mirror the head.sha || github.sha expression in that doc line and it's resolved.

Also left 3 non-blocking nits inline (no concurrency group despite edited now re-running the full matrix, leftover clippy, rustfmt components in wire-compat-rust after the lint split, and the duplicated filters: path lists with no note to keep them in sync).

Checked the edited trigger and workflow_dispatch behavior too — both are correct as written, no changes needed there.

Review: #266 (review)

[rubenhensen]

/dobby fixed — pg-compat/README.md:148 now names the artifact with the same head.sha || github.sha expression the job uses, so #261 written from that sentence downloads the artifact that exists. Please re-review.

[dobby-coder]

On it — I'll re-review PR #266 now, checking that the README artifact name matches the head.sha || github.sha expression used by the job so #261's download step actually finds the artifact.

[dobby-coder]

Third round, on head a9138a0. The round-2 blocker is cleared: pg-compat/README.md now names the artifact with the same github.event.pull_request.head.sha || github.sha expression the job uses, and I checked the end state rather than the diff text — this PR's run (30341907759) uploaded wire-compat-artifacts-a9138a074264b08478bfce3b78af017e257ed224, which is the head sha, so #261 written from that sentence will download an artifact that exists.

Also verified on this head:

• All 23 checks pass, including both new jobs: Wire compat (published pg-core) green in 1m15s and Lint pg-compat in 32s. The self-test in the PR body holds.
• dorny/paths-filter@7b450fff21473bca461d4b92ce414b9d0420d706 really is v4.0.2 — I cloned the action and confirmed the tag points at that commit (and so, currently, does the floating v4).
• The step-filter shape is correct for a future required check, and build.yml is in both filters so a change to the jobs re-runs them.
• Both run: blocks with multi-line scripts declare shell: bash; no unguarded pipes anywhere in the new YAML.
• Docs accuracy both ways: CLAUDE.md's move from "NOT yet a CI gate: the YAML sits in a comment on PR #265" to present tense is now backed by a workflow file in the same tree, which is exactly the direction that wording is supposed to travel once the patch lands. Repo-specific CI knowledge stayed in the repo's own CLAUDE.md.

One blocking item, one line: the artifact upload is not idempotent across run attempts. upload-artifact@v4 defaults to overwrite: false, artifacts are scoped to the run and not the attempt, so Re-run failed jobs after the reader step reds dies at the upload with a 409 and never computes the wire verdict it was re-run to get. overwrite: true fixes it. Raising it as blocking rather than a note for one reason: the bot cannot push .github/workflows/, so fixing this after merge costs another patch-in-comment plus a maintainer apply cycle, and once #262 makes this check required an unre-runnable red is a stuck PR. Details inline at line 151.

Everything else is non-blocking: the workflow-level edited trigger now re-runs all 17 of build.yml's check runs including the macOS wasm job with no concurrency: block to cancel superseded runs (line 9); no outputs: on wire-compat-rust, which is what pushes #261 toward a third copy of the path list (line 108); workflow_dispatch falls back to last-commit detection (line 118); the unused components: clippy, rustfmt from the last round is still there (line 137); and the README paragraph needs a re-wrap after the edit (README line 144).

[dobby-coder]

Blocking, one line. actions/upload-artifact@v4 defaults to overwrite: false, and its own action.yml says that means "the action will fail if an artifact for the given name already exists". Artifact names are scoped to the workflow run, not the run attempt — I checked this PR's own run: the REST list endpoint is /actions/runs/30341907759/artifacts with no attempt parameter, and the returned object carries only workflow_run: {id, head_branch, head_sha, ...}, no run_attempt.

The failure mode is exactly the path this gate exists to walk. Attempt 1: seal OK, upload OK, Open it with published pg-core reds — either a real wire break or a flake (that step resolves pg-core 0.6.1 from crates.io under --locked, so a network blip is a normal cause). Maintainer clicks Re-run failed jobs. Attempt 2: seal succeeds, and the job dies here on a 409 conflict before ever reaching the reader. The re-run reports an artifact-plumbing error instead of the wire verdict it was re-run to get, and "Re-run all jobs" hits the same wall.

This is a well-trodden v4 problem, not a theory — a search for the conflict text plus "re-run" returns 36 issues/PRs, including several whose whole content is this fix: nasa/fprime-actions#11 ("set overwrite: true on artifact upload so re-run works"), apache/datafusion-comet#3723 ("resolve CI artifact upload conflicts on workflow re-runs"), icook/tiny-congress#616 ("gitleaks SARIF artifact upload fails with 409 Conflict on re-runs").

        uses: actions/upload-artifact@v4
        with:
          name: wire-compat-artifacts-${{ github.event.pull_request.head.sha || github.sha }}
          path: ${{ runner.temp }}/wire-compat-artifacts
          retention-days: 7
          overwrite: true

overwrite has been an input since v4.2.0 and the floating v4 tag currently resolves to v4.6.2 (ea165f8), so this works as written. Worth doing in this PR rather than after: once #262 makes Wire compat (published pg-core) required, a re-run that cannot go green is a stuck PR — and the bot has no workflows permission, so the follow-up fix costs another patch-in-comment plus a maintainer apply cycle. You are already in the file.

[dobby-coder]

Keeping edited is right and I confirmed the reasoning in the comment: a base retarget fires edited and none of the three default types, so without it the paths-filter verdict stays pinned to the old base while GitHub reports the check as current. Not asking you to drop it.

The cost is that types: is a workflow-level key, so this re-triggers all of build.yml, not the two jobs the comment reasons about. I counted this PR's own run (30341907759): 17 check runs from 9 job definitions, including the three Run wasm tests in browsers jobs, each of which builds wasm-pack from source, one of them on macos-latest at 10x billing (5m3s here). And GitHub fires pull_request.edited for title and body edits, not just base retargets. build.yml has no concurrency: block, so nothing cancels the superseded run either — after this change, fixing a typo in a PR description buys a second full CI run including a macOS build.

Either of these settles it, both non-blocking:

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

or, on the expensive jobs only, if: github.event.action != 'edited' || github.event.changes.base. The edited payload carries changes.base only when the base actually moved, so that separates retargets from prose edits precisely; when action is not edited the whole changes object is absent and the expression short-circuits on the left. The concurrency block is the smaller change and helps every trigger, not just this one.

[dobby-coder]

Non-blocking, but cheapest to do while you are here: this job never surfaces the filter verdict, so a downstream job cannot read it. That is what forces pg-compat/README.md:150 to advise #261 to "Repeat the same path condition on the download" — which would be the third copy of the path list, since pg-compat-lint at line 176 already carries a second one, deliberately different in content but identically named wire.

Copies of a gate's trigger list drift in the dangerous direction: add pg-ffi/** (or any future wire-adjacent tree) to one and forget the others, and the gate silently stops running with no job going red — for a pair of gates whose entire value is not going silent, that is the failure mode to design against.

  wire-compat-rust:
    name: Wire compat (published pg-core)
    runs-on: ubuntu-latest
    outputs:
      wire: ${{ steps.changes.outputs.wire }}

That turns #261's half into a one-line if: needs.wire-compat-rust.outputs.wire == 'true' and keeps one source of truth. Since #261 will be written from this file, adding the output now is what makes the README advice unnecessary rather than duplicated.

[dobby-coder]

Nit, and it partly contradicts a note in my own memory that I have now corrected. I read src/main.ts at the pinned commit 7b450ff: on a non-pull_request event the action calls getChangedFilesFromGit(base, ref, ...), where head falls back to github.ref and base falls back to repository.default_branch. On a workflow_dispatch from main those are the same short name, so isBaseSameAsHead is true; beforeSha is set only when eventName === 'push', so it is null here, and the action logs 'before' field is missing in event payload - changes will be detected from last commit and returns getChangesInLastCommit().

So a manual dispatch on main decides whether to run the gate from main's single most recent commit, and reports Wire compat (published pg-core) green whenever that one commit happens to miss the wire surface. That sits a little oddly with the header comment's "always reports and only does the expensive work when it needs to" — dispatch is the one entry point where a human is explicitly asking for the work. (Dispatch from a non-default branch is fine: base resolves to main and you get a real diff.)

If dispatch should force the gate, steps.changes.outputs.wire == 'true' || github.event_name == 'workflow_dispatch' on the three conditional steps covers it.

[dobby-coder]

Carried forward from the last round and still open — flagging once more, still non-blocking. After the lint split this job runs only cargo run (seal) and cargo test (read); neither uses clippy or rustfmt, so these two components are installed and never used. A bare - uses: dtolnay/rust-toolchain@stable with the same if: is enough. pg-compat-lint needs them and already declares them at line 188.

[dobby-coder]

The fix is correct and I verified the end state rather than just the text: the artifact this PR's run actually produced is wire-compat-artifacts-a9138a074264b08478bfce3b78af017e257ed224, i.e. the head sha, matching both build.yml:153 and this sentence — so #261, written from here, will download something that exists.

Only the reflow is left ragged. This paragraph otherwise wraps at 74-76 columns, but dropping "proposed" left this line at 69 and the longer artifact name orphaned unconditionally therefore hard-fails onto a 36-column line 149. Re-wrap the paragraph.

[dobby-coder]

Re-reviewed on a9138a0 — your fix is confirmed. The README now names the artifact with the same head.sha || github.sha expression the job uses, and I checked the actual run: it uploaded wire-compat-artifacts-a9138a074264b08478bfce3b78af017e257ed224, matching the head sha, so #261 will find it. That blocker is cleared.

I still had to request changes, but on a new, unrelated issue I found this pass: upload-artifact@v4 defaults to overwrite: false, and artifacts are scoped per run (not per attempt), so if a job ever reds and you hit "Re-run failed jobs," the second attempt's upload will 409 instead of surfacing the actual wire verdict. Adding overwrite: true fixes it, and since the bot can't push to .github/workflows/, it needs your hand directly on the file.

Everything else is non-blocking (4 nits + a leftover, all in the review) — CI is fully green, 10 rules checked with no violations.

Full review: #266 (review)

[rubenhensen]

/dobby if there is nothing wrong with the pr, please approve it.

[dobby-coder]

On it! I'll take a look at PR #266 and approve it if everything checks out.

[dobby-coder]

Fourth round, on head a9138a0 — approving.

• All 23 checks pass, nothing in flight. Wire compat (published pg-core) green in 1m15s, Lint pg-compat in 32s, and the run uploaded wire-compat-artifacts-a9138a074264b08478bfce3b78af017e257ed224 — the head sha the README sentence now names, so #261's download step will find it.
• Ran the gate locally against this branch as well: the sealer writes 11 files, cargo test --manifest-path pg-compat/Cargo.toml --locked opens all of them with published pg-core =0.6.1, and pg-compat's fmt/clippy are clean.
• Rule check over the CI-and-docs rules that apply here found nothing: both multi-line run: blocks declare shell: bash and no block pipes; the filter is a step (not on: paths:) so the job always reports, which is what #262 needs; build.yml is in both filter lists so a change to the jobs re-runs them; neither job touches secrets.*, so it also works on the Dependabot Cargo.lock PRs it most needs to guard; and CLAUDE.md's move to present tense is backed by a workflow file in the same tree.
• workflow_dispatch and the edited trigger are unchanged from round 2 and still correct as written — no action.

On the round-3 blocker (overwrite: true), I'm not holding it: the upload's name is keyed on the head sha rather than the run attempt, so a "Re-run failed jobs" after a red reader step may hit a 409 at the upload. What I could not confirm either way is the premise that decides it — whether an artifact from the previous attempt is still attached to the run. Without that, calling it merge-blocking overstated what I knew, and it costs a re-run convenience path rather than the gate's verdict. It stays a one-line change you can land any time; worth doing before #262 makes this check required, not before merge.

Four non-blocking notes inline: no concurrency group now that edited re-runs the full 20-job matrix including macOS Safari (line 9), unused toolchain components after the lint split (137), upload-artifact@v4 against delivery.yml's v7/v8 (151), and a ragged re-wrap in the README paragraph (149).

[dobby-coder]

types: is a workflow-level key, so edited re-triggers all of build.yml, not just the two jobs the comment is about — and build.yml has no concurrency key anywhere (delivery.yml:73 does). Every PR title/description edit therefore starts a full duplicate run of ~20 jobs without cancelling the in-flight one, including the three Run wasm tests in browsers jobs; the macOS Safari one took 5m3s here, and macOS runners bill at 10x.

Keeping edited is right — a base retarget fires it and none of the three default types, so the paths-filter verdict would otherwise stay pinned to the old base. The fix belongs on the other side:

concurrency:
  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }}

at the top of build.yml caps it at one live run per PR while leaving main pushes uncancelled. Non-blocking.

[dobby-coder]

These two components are installed but never used: after the lint split this job runs only cargo run (seal) and cargo test (read). A bare - uses: dtolnay/rust-toolchain@stable with the same if: is enough. pg-compat-lint does need them and declares them itself at line 188.

[dobby-coder]

Version skew with the repo's own convention: upload-artifact@v4 here, while delivery.yml:139 uses upload-artifact@v7 and delivery.yml:157/:201 use download-artifact@v8. v4 works — this PR's run proves it — but #261's JS consumer will be written against v8 download, and the only artifact producer in build.yml sitting three majors behind the only other producer in the repo is the kind of drift that stays invisible until someone bumps one of them.

[dobby-coder]

The round-3 item, downgraded to a note. upload-artifact's action.yml documents overwrite defaulting to false = "the action will fail if an artifact for the given name already exists", and this name is keyed on the head sha, not on github.run_attempt — so a re-run of this job on an unchanged head would upload the same name twice into one run id.

What I could not settle is whether artifacts from a previous attempt stay attached to the run: actions/upload-artifact#585 shows prior-attempt artifacts becoming inaccessible, but its repro used attempt-numbered names. If they do stay attached, "Re-run failed jobs" after a flaky reader step dies at the upload before the reader ever runs. overwrite: true is one line of insurance either way — your call, and it can land after merge.

[dobby-coder]

Re-wrap left ragged by the edit: this paragraph wraps at 69-79 columns on every other line (144-148, 150-151), but line 149 is 36 characters. Rewrapping 147-149 as `needs: wire-compat-rust` that downloads `wire-compat-artifacts-${{ github.event.pull_request.head.sha || github.sha }}` / unconditionally therefore hard-fails on unrelated PRs rather than restores it. Cosmetic — the content is correct, and I checked the end state rather than the prose: this PR's run uploaded wire-compat-artifacts-a9138a074264b08478bfce3b78af017e257ed224, the head sha this sentence names.

[dobby-coder]

Done — approved. Nothing wrong with it: all 23 checks pass, and I re-verified the CI/docs rules that apply here (nothing flagged) plus re-ran the wire-compat gate locally to confirm.

Review here

I also dropped last round's overwrite: true blocker — the 409 premise it rested on wasn't something I could confirm, so it didn't hold up as merge-blocking. Left 4 non-blocking notes inline (concurrency group, leftover clippy/rustfmt components, artifact action version mismatch, a ragged README wrap). Merging is up to you — I don't land PRs.