Skip to content

test(compat): published pg-wasm/pg-js readers open HEAD-sealed samples - #267

Merged
rubenhensen merged 11 commits into
mainfrom
test/wire-compat-js-readers
Jul 28, 2026
Merged

test(compat): published pg-wasm/pg-js readers open HEAD-sealed samples#267
rubenhensen merged 11 commits into
mainfrom
test/wire-compat-js-readers

Conversation

@dobby-coder

@dobby-coder dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Closes #261. Part of #251, epic #247 (workstream C).

pg-compat opens the HEAD-sealed sample set with published pg-core releases. This adds pg-compat-js, which does the same job for the npm side of the support window in COMPATIBILITY.md:

reader opens how
@e4a/pg-wasm 0.6.1 all 5 cases Unsealer and StreamUnsealer directly
@e4a/pg-js 2.3.3 the 3 stream* cases PostGuard#open().inspect() then .decrypt()
@e4a/pg-js 1.11.0 the 3 stream* cases same

That is 11 case/reader pairs, each opened with both recipients' keys, so 22 unseals per run. The three versions are installed side by side through npm: aliases (several versions of one package have to coexist in one node_modules) and pinned by a committed lockfile; CI installs with npm ci.

Each case runs in its own process, bin/pg-compat-case.mjs, for the reason the Rust half does it: a reader handed a shifted header can die on the allocation instead of returning an error, and one dead case should not take the run down.

What it checks per case

Recovering the right bytes, plus the things a consumer acts on once a container opens: the recipient list the reader read out of the header, the public signing policy it verified the header signature against, and the private signing policy for the *-privsig cases. pg-compat's reporting style is kept, including the first-differing-offset message, so a red run in either half reads the same in a CI log.

Two SDK limitations, declared rather than hidden

Both were found while building this, and both are the shape of thing that turns into a gate that passes without reading anything.

pg-js is stream mode only, in both directions. Its decrypt path builds a StreamUnsealer whichever input mode it is given, so a memory-mode container comes back as mode is not supported: InMemory { size: N }, and toBytes() seals with sealStream rather than seal. So the memory-mode cases are the pg-wasm reader's alone. This also corrects pg-compat/README.md, which had memory mode as "what pg-js toBytes produces".

pg-js 1.11.0 never surfaces a private signing policy: it discards what StreamUnsealer.unseal() returns and reports public_identity() instead, which only sees the header. 2.3.3 keeps the unseal result. That is a fixed limitation of the 1.x line, not something a wire change can move, so the reader declares it and the gate then requires the private policy to be absent for that reader. A 1.x that suddenly reported one is a mismatch rather than a quiet pass.

Neither is a silent skip. The run prints which reader opened which case, and fails if a case ends up opened by no reader or a reader opens no case:

not covered — @e4a/pg-js@2.3.3 × mem (memory): pg-js decrypts through StreamUnsealer in both input modes, ...
mem: opened by @e4a/pg-wasm@0.6.1
stream: opened by @e4a/pg-wasm@0.6.1, @e4a/pg-js@2.3.3, @e4a/pg-js@1.11.0

Does it actually go red

I checked both directions against pg-core HEAD rather than trusting the green run.

A field inserted before algo in Header (non-additive) turns all 22 unseals red, per case and per reader:

@e4a/pg-wasm@0.6.1: mem/alice: unseal: Bincode decode error: OtherString("invalid value: integer `3687607010`, expected variant index 0 <= i < 2")
@e4a/pg-js@1.11.0: stream-multi-segment/bob: unseal: Bincode decode error: OtherString("invalid value: integer `1534289520`, expected variant index 0 <= i < 2")

The same field appended at the end of Header (additive, trailing bytes ignored) stays green. Both probes were reverted; there is no pg-core change in this PR.

test/gate-teeth.test.mjs commits the cheap half of that: it hands the readers a damaged copy of the set (flipped payload byte, truncated container, bumped wireVersion, a case file the set does not have) and requires a failure naming the case. It is what catches a harness that has stopped reading, since "good bytes open" is an assertion a broken harness also satisfies.

The support window is checked against the document that declares it

COMPATIBILITY.md's Reader list is the normative window and src/readers.mjs is this package's copy of the npm half of it, so src/support-window.mjs parses the fenced <registry> <package> <versions...> block and test/manifest.test.mjs compares the npm rows against readers(). Adding a version to the document and not to the gate is a red run, which is the drift direction that matters: the window growing where it is declared while the gate goes on testing the set it always tested. The parser refuses a document with no such block, one with no npm row, and an npm row with no version, because an empty list would compare equal to nothing.

Each reader's declared version is also compared against the package.json of the alias npm resolved, so the two-place edit README.md documents cannot end up running one version while labelling every message with another.

Verified locally

  • pg-compat-js: 22 tests pass, 0 failed, 0 cancelled, exit 0 (PG_COMPAT_ARTIFACTS=... npm --prefix pg-compat-js test).
  • cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream: pass.
  • PG_COMPAT_ARTIFACTS=... cargo test --manifest-path pg-compat/Cargo.toml --locked: pass, so one sealed set still feeds both halves.
  • cargo test for pg-pkg, pg-cli, pg-ffi with --all-features: 69 tests, all pass.
  • cargo fmt --all -- --check and the same for pg-compat: clean. No Rust source changed.

The CI job needs a maintainer

dobby-coder[bot] has no workflows: write on this repo, so the wire-compat-js job is in a PR comment for you to apply, as with #266. It needs two lines added to the wire-compat-rust job in #266 as well: outputs.sealed: ${{ steps.seal.outcome }}, which is already the exact condition the artifact upload is behind, plus pg-compat-js/** in that job's path filter. Re-running the filter in the JS job instead would mean two copies of a six-pattern list that have to agree forever. Details, and what I checked, are in the comment.

Until that is applied nothing runs this in CI, the same caveat #266 carries.

The Node half of the wire-compat gate (#261). `pg-compat-js` opens the
sample set the Rust half seals with the npm readers in COMPATIBILITY.md's
support window: `@e4a/pg-wasm` 0.6.1 through both of its unsealers, and
`@e4a/pg-js` 2.3.3 and 1.11.0 through `PostGuard#open`, the entry point
consumers actually use. Versions are installed side by side as `npm:`
aliases and pinned by a committed lockfile.

Each case runs in its own process for the reason the Rust half does it: a
reader handed a shifted header can die on the allocation rather than
returning an error, and one dead case should not take the run down.

Two limitations of the published SDKs are declared rather than hidden,
because both would otherwise look like a gate that reads nothing. pg-js
is stream-mode only in both directions, so the memory-mode cases are the
pg-wasm reader's alone; and pg-js 1.x never surfaces a private signing
policy, so for that reader the gate requires it absent. The run prints
which reader opened which case, and fails when a case is opened by no
reader or a reader opens no case.

Also corrects `pg-compat/README.md`, which had memory mode as what pg-js
`toBytes` produces — it seals with `sealStream`.

The `wire-compat-js` job that runs this in CI cannot be pushed by the
app; the YAML is in a PR comment.

Refs #247, #251
Closes #261
@dobby-coder
dobby-coder Bot requested a review from rubenhensen July 28, 2026 07:45
@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Workflow YAML for a maintainer to apply

dobby-coder[bot] has no workflows: write on this repo, so this cannot ride the PR. Written against ci/wire-compat-rust-job as it stands now (the github.event.pull_request.head.sha || github.sha artifact name and the split-out pg-compat-lint job), not against the first revision of #266. Two parts, both in .github/workflows/build.yml.

Part 1: two lines of outputs on wire-compat-rust

The JS job needs to know whether an artifact exists. Two ways to answer that, and only one of them stays right:

Re-running dorny/paths-filter in the JS job would mean two copies of a six-pattern filter that have to agree forever. They would not. A PR touching only Cargo.lock trips one list, someone adds a pattern to the other, and the JS job downloads an artifact nobody produced.

So publish the answer from the job that owns it. steps.seal.outcome is already the exact condition the upload is behind, so it is also the exact condition "there is an artifact" is behind:

  wire-compat-rust:
    name: Wire compat (published pg-core)
    runs-on: ubuntu-latest
    permissions:
      contents: read
      pull-requests: read # paths-filter reads the PR's changed files
    # wire-compat-js (#261) opens the same bytes with the published npm readers.
    # It downloads the artifact the seal step below produces, so it gates on the
    # same outcome the upload gates on rather than re-running the path filter and
    # drifting out of step with it.
    outputs:
      sealed: ${{ steps.seal.outcome }}
    steps:

Then add pg-compat-js/** to the wire: filter in that job, so a change to the JS gate itself runs the gate:

            wire:
              - 'pg-core/**'
              - 'pg-wasm/**'
              - 'pg-compat/**'
              - 'pg-compat-js/**'
              # pg-core has no lockfile of its own: the bytes HEAD seals are a
              # function of the ROOT lockfile (a bincode-next/ibe/serde bump
              # touches only these two files). Root Cargo.toml also holds the
              # exclude list that keeps pg-compat on crates.io pg-core.
              - 'Cargo.lock'
              - 'Cargo.toml'
              - '.github/workflows/build.yml'

Nothing else in wire-compat-rust or pg-compat-lint changes.

Part 2: the new job

Append after pg-compat-lint:

  # ---------------------------------------------------------------------------
  # The Node half of the same gate (#261): the containers wire-compat-rust
  # sealed must also open with the published npm readers in COMPATIBILITY.md's
  # support window. It downloads rather than re-seals, so both halves are held
  # to the same bytes and a non-deterministic sealer cannot hide between them.
  #
  # `if: !cancelled()` rather than the default: a red wire-compat-rust is
  # exactly when it is worth knowing whether the JS readers broke the same way,
  # and the artifact is uploaded before that job's read step for this reason.
  # Without it, `needs` would skip this job on the run where it has the most to
  # say.
  # ---------------------------------------------------------------------------
  wire-compat-js:
    name: Wire compat (published pg-wasm/pg-js)
    runs-on: ubuntu-latest
    needs: wire-compat-rust
    if: ${{ !cancelled() }}
    permissions:
      contents: read
    steps:
      - uses: actions/checkout@v4

      # Every step below is behind the seal outcome, not just the download.
      # `outcome` is 'skipped' when the path filter said no (nothing to open,
      # report success) and 'failure' when sealing itself broke (wire-compat-rust
      # owns that failure; do not report it twice).
      - if: needs.wire-compat-rust.outputs.sealed == 'success'
        uses: actions/setup-node@v6
        with:
          node-version: '24'
          cache: npm
          cache-dependency-path: pg-compat-js/package-lock.json

      - name: Download the sample set
        if: needs.wire-compat-rust.outputs.sealed == 'success'
        uses: actions/download-artifact@v4
        with:
          name: wire-compat-artifacts-${{ github.event.pull_request.head.sha || github.sha }}
          path: ${{ runner.temp }}/wire-compat-artifacts

      # ci, not install: the readers are pinned by pg-compat-js/package-lock.json,
      # and a gate that quietly resolved a different reader would be measuring
      # something other than the support window.
      - name: Install the published readers
        if: needs.wire-compat-rust.outputs.sealed == 'success'
        shell: bash
        working-directory: pg-compat-js
        run: npm ci

      - name: Open the set with published pg-wasm/pg-js
        if: needs.wire-compat-rust.outputs.sealed == 'success'
        shell: bash
        working-directory: pg-compat-js
        env:
          PG_COMPAT_ARTIFACTS: ${{ runner.temp }}/wire-compat-artifacts
        run: npm test

The artifact name has to stay character-for-character identical to the upload's, including the github.event.pull_request.head.sha || github.sha fallback. On a pull_request event those two shas are different (head vs merge commit), so a plain github.sha here would look for an artifact that does not exist.

Notes

Node 24 matches delivery.yml's publish-wasm job. @e4a/pg-js declares engines: node >=22, so 22 also works if you would rather pin lower.

shell: bash on both run: steps is deliberate. Actions' default run: shell is bash -e {0}, which has -e but no pipefail; an explicit shell: bash gets -eo pipefail. Neither step pipes today, but the next line someone adds might.

No JS lint job to go with pg-compat-lint. The repo has no prettier or eslint config and pg-compat-js adds none, so there is nothing to run that would not be a new convention this PR has no mandate to introduce.

The gate itself takes about 15 seconds of wall clock locally: 11 case/reader pairs, one process each, plus a wasm init per process. npm ci and the download dominate the job.

retention-days: 7 on the upload bounds how long a red run can be re-examined. After that, seal locally instead; the sealer is deterministic, so the same commit gives the same bytes:

cargo run -p pg-core --features stream --example seal-samples -- target/wire-compat/artifacts
npm --prefix pg-compat-js ci
PG_COMPAT_ARTIFACTS="$PWD/target/wire-compat/artifacts" npm --prefix pg-compat-js test

Checked, not just written

I applied both parts to ci/wire-compat-rust-job's current build.yml and checked the result: it parses to 7 jobs, wire-compat-js has 5 steps with the seal-outcome condition on 4 of them, the two artifact-name expressions are byte-identical, and actionlint on the assembled file reports nothing. What I cannot check from here is a real run, since the bot cannot push the file.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: request-changes

Rules Dobby 2 — cycle 1

Rule sweep over the diff plus review findings from the previous pass, merged. Three blocking, four non-blocking. Both blocking code/docs claims were reproduced rather than argued.

What is good here: the gate-teeth test is the right instinct — "good bytes open" is an assertion a broken harness also satisfies, and committing the damaged-set half is what stops this becoming another green check that reads nothing. Both SDK limitations are declared and asserted on (1.x's absent private policy is required absent, not skipped) rather than papered over. The per-case child process, the coverage invariant, and the npm ci pin are all pointed at the same failure mode. The CI job is correctly phrased as pending in CLAUDE.md and the PR body.

Blocking

  1. bin/pg-compat-case.mjs:57 — a missing artifact reports Node's version banner instead of the error. verifyCase runs outside any try/catch, and loadArtifacts reads the ciphertext/plaintext/usk files before the first guarded block. Reproduced with a manifest naming a stream.bin that is not on disk:

    @e4a/pg-wasm@0.6.1: stream: reader exited with status 1 before it could report: Node.js v22.23.1
    

    The ENOENT and the path are both gone: stdout is empty, so childFailures falls through to stderrTail, which drops every at frame and lands on the last surviving line — Node's banner. This is exactly the state the new CI job makes reachable (a partial actions/download-artifact, or a manifest naming a file the sealer did not write), and it is the one path where a maintainer reading the log has nothing to go on.

  2. COMPATIBILITY.md:111 and pg-compat-js/README.md:84 promise a drift check that does not exist. Reproduced: adding 2.4.0 to the npm @e4a/pg-js line in COMPATIBILITY.md, leaving readers.mjs untouched, still gives # pass 5 / # fail 0. test/manifest.test.mjs:47 compares readers() against a literal hardcoded in the test file and never opens COMPATIBILITY.md. The drift direction that matters — the support window growing in the normative doc while the gate keeps testing the old set — goes undetected. Worth fixing rather than softening: the fenced block at COMPATIBILITY.md:88-94 is already machine-readable (npm @e4a/pg-js 2.3.3 1.11.0), so parse the npm rows and compare against readers().

  3. pg-compat/README.md:153 describes a job that does not exist, doing something the proposed job does not do. The added text states that pg-compat-js's job "repeats the same dorny/paths-filter step and gates every expensive step on it". The YAML in the workflow comment does the opposite, deliberately and with an argument for it: it publishes outputs.sealed: ${{ steps.seal.outcome }} from wire-compat-rust and gates on needs.wire-compat-rust.outputs.sealed precisely to avoid two copies of a six-pattern filter. Two defects in one sentence — present tense for a job no workflow file defines, and a mechanism that contradicts the patch this PR ships. Phrase it as pending and describe the outputs.sealed approach the comment actually proposes.

Non-blocking

  1. src/readers.mjs:43 — the declared version and the npm: alias in package.json are independent strings with nothing comparing them. Installed versions are correct today; the trap is the documented two-place edit. load() already imports from specifier, so reading ${specifier}/package.json's version closes it in a line or two.
  2. bin/pg-compat-case.mjs:58process.stdout.write immediately followed by process.exit(). Node's pipe writes are synchronous on Linux and Windows but asynchronous on macOS, so a hand-run on a maintainer's Mac (documented at README.md:38-41) can lose the per-recipient detail. CI is ubuntu, so the gate itself is unaffected. process.exitCode = 1 lets the writes drain.
  3. test/wire-compat.test.mjs:44openedBy is pushed whether or not runCase returned failures, so the coverage invariant below it is satisfied by a reader that failed to open the case. Harmless today, but it does not check what the comment above it says.
  4. CLAUDE.md:8 (pre-existing line, not touched by this PR, but the bullet right below it is) — "the YAML sits in a comment on PR #265" is stale: #265 is merged and the wire-compat-rust YAML is on #266. CLAUDE.md is auto-loaded agent context, so a later run follows that pointer to a merged PR looking for a spent patch comment. Your new bullet gets this right for #267; worth fixing the neighbour in the same commit.

Checked and clean

Conventional-commit PR title (the Conventional Commit check passes), the patch-in-comment pending phrasing in CLAUDE.md and the PR body, shell: bash on both run: steps in the proposed YAML (no unguarded pipes), cross-repo link format, prose style, draft state with the reason stated. CI on this branch is green. standardized-readmes does not apply — pg-compat-js/README.md is a package README inside the monorepo, not a repo root.

Findings 1-3 are worth a push. 4-7 are yours to take or leave; if you fold them in, none needs more than a couple of lines.

Comment thread pg-compat-js/bin/pg-compat-case.mjs Outdated
Comment thread COMPATIBILITY.md Outdated
Comment thread pg-compat/README.md Outdated
Comment thread pg-compat-js/src/readers.mjs
Comment thread pg-compat-js/bin/pg-compat-case.mjs Outdated
Comment thread pg-compat-js/test/wire-compat.test.mjs Outdated
Comment thread pg-compat-js/README.md Outdated
…ATIBILITY.md

Review follow-ups on the Node half of the wire-compat gate.

An error raised before the first reader call left `verifyCase` as an unhandled
rejection: the child exited 1 with an empty stdout, and `childFailures` fell
through to the last stderr line that is not a stack frame, which for Node's
uncaught-error dump is its own version banner. A missing `stream.bin` reported
`reader exited with status 1 before it could report: Node.js v22.23.1`, with the
errno and the path gone. That is the state a partial artifact download in CI
reaches. It is reported on stdout now like every other failure, and
`gate-teeth.test.mjs` holds it there.

`COMPATIBILITY.md` and the package README both promised that
`test/manifest.test.mjs` fails when `src/readers.mjs` and the document's reader
list drift. It did not: the test compared `readers()` against a literal in the
test file, so adding a version to the document alone stayed green, which is the
drift direction that matters. `src/support-window.mjs` parses the fenced
`<registry> <package> <versions...>` block instead and the test compares the
`npm` rows, refusing a document with no block and one with no npm row rather
than comparing against nothing.

Also: each reader's declared `version` is compared against the `package.json`
of the alias npm resolved, so the two-place edit cannot run one version while
labelling it another; the coverage invariant counts readers pointed at a case
rather than readers that pushed a name whether or not the case opened;
`process.exitCode` replaces `process.exit()` so a hand-run on macOS, where pipe
writes are asynchronous, cannot lose the failure lines it is being read for; and
`pg-compat/README.md` describes the JS job as pending and gating on the seal
outcome the sealing job publishes, which is what the proposed YAML does, rather
than as a job that re-runs the path filter.

CLAUDE.md's pointer to the `wire-compat-rust` YAML follows it from the merged
#265 to #266.

Refs #261, #251, #247.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Review cycle 1 addressed, all seven threads

All three blocking findings are fixed and pushed as 7caa296; the four non-blocking ones are folded in too. Each thread has a reply with what changed and how I checked it; this is the short version.

The two that were reproduced as broken are reproduced as fixed:

# before: a set whose stream.bin is absent
@e4a/pg-wasm@0.6.1: stream: reader exited with status 1 before it could report: Node.js v22.23.1
# after
@e4a/pg-wasm@0.6.1: stream: ENOENT: no such file or directory, open '/tmp/broken-set/stream.bin'

# before: 2.4.0 added to COMPATIBILITY.md's npm @e4a/pg-js row, readers.mjs untouched
# pass 5 / # fail 0
# after
not ok 4 - the support window is the npm reader list in COMPATIBILITY.md
    src/readers.mjs and the reader list in COMPATIBILITY.md have drifted; the document decides

Both are committed checks, not scratch runs. The first is a new gate-teeth case; I confirmed it has teeth by reverting the try/catch, at which point it is the only one of that file's four cases that fails, with the version banner as its actual value. The second is test/manifest.test.mjs reading COMPATIBILITY.md through the new src/support-window.mjs, which refuses a document with nothing to compare against rather than passing on an empty list.

pg-compat/README.md now describes the JS job as pending and gating on needs.wire-compat-rust.outputs.sealed, which is what the YAML in the workflow comment does. CLAUDE.md's pointer to the wire-compat-rust patch comment follows it from the merged 265 to 266, and gained the two things a later run would otherwise have to rediscover: that the Reader list block is machine-read, and that package.json is not in either published package's exports.

One deviation from a suggestion, on the version cross-check: read off disk rather than imported, because import('pg-js-2-3-3/package.json') is ERR_PACKAGE_PATH_NOT_EXPORTED. Reason is in a comment on the function.

Verified after the changes: 22 tests pass in pg-compat-js, the Rust half still opens the same sealed set (cargo test --manifest-path pg-compat/Cargo.toml --locked), and cargo fmt --all -- --check plus the pg-compat equivalent are clean. No Rust source changed in this round. The workflow YAML in the comment above is unchanged and still needs a maintainer to apply it.

The installed-version check closed the version half of the two-place edit
and left the package half open: an alias resolving to the right version of
the wrong package passed while every message carried the declared package
name, which is the mislabelled support window the check exists to catch.
`installedPackage` returns `{name, version}` and the test asserts both.

Two docs pointers with it. `pg-compat/README.md` called #261 a PR; the
work item is issue #261 and the YAML is a comment on PR #267.
`COMPATIBILITY.md` claimed the whole `crates.io` line is spelled out in
`pg-compat/Cargo.toml`, which pins 0.6.1 only; 0.5.10 is declared in the
window and opened by no gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: approve

Rules Dobby 2 — cycle 2

Re-swept the delta 5a22923..7caa296 against the rules that apply to a Node/CI diff, merged with the reviewer's four findings. Zero blocking issues. All three cycle-1 blockers are genuinely fixed, three of the four new nits I fixed on the branch myself (a605734), and the fourth is a latent hazard the reviewer explicitly asked not to "fix" — it is recorded below and inline instead.

Verified, not taken on trust

The PR body claims 22 passing tests. I sealed a set with HEAD and ran it rather than believing the summary line, because # fail 0 is not the same as green — one unresolved await reports as # cancelled and still exits 1:

# tests 22
# pass 22
# fail 0
# cancelled 0
EXIT=0

The widened alias check was reproduced as catching what the old one missed. Adding "pg-js-0-6-1": "npm:@e4a/pg-wasm@0.6.1" — an alias with the version the entry declares and the wrong package — passed the version-only check and fails now:

not ok 6 - every reader is labelled with the package npm installed for it
  +   name: '@e4a/pg-wasm',
  -   name: '@e4a/pg-js',

Fixed on the branch (a605734)

Nits, so they were cheaper to push than to hand back:

  1. src/readers.mjsinstalledVersion closed the version half of the documented two-place edit and left the package half open, so a right-version/wrong-package alias passed while every failure message carried the declared package name. That is the mislabelled support window the function exists to prevent. Now installedPackage returns {name, version} and the test asserts both, with CLAUDE.md and pg-compat-js/README.md updated to match.
  2. pg-compat/README.md:153 — called #261 a PR. #261 is an issue (compat gate: published pg-wasm/pg-js old-reader step (Node)) and the link ref resolves to /issues/261, so a maintainer following the pointer for the YAML landed on an issue that has none. Now reads "([#261], YAML in a comment on PR #267 and not applied yet)", matching what CLAUDE.md already said two paragraphs away.
  3. COMPATIBILITY.md:114 — asserted the whole crates.io line is spelled out in pg-compat/Cargo.toml. That manifest pins one version (=0.6.1, one reader!); 0.5.10 sits in the declared window and is opened by no gate. Qualified rather than papered over, since adding the Rust-side check is out of this PR's scope.

Non-blocking, left alone deliberately

  1. src/readers/pkg-stub.mjs — the process.exitCode fix means the child now exits when the event loop drains, and the stub is a listening server with no unref(). Not reachable today: startPkgStub is the last fallible statement in the pg-js reader's load(), so verify.mjs's early return on load failure cannot leak a live server, and its finally always runs unload. I confirmed both. Flagging the hazard, not a live bug — a server.unref() would make the drain fix robust against a later change to that ordering, but it is not worth a commit for a failure that cannot currently happen.

The crates.io drift check (0.5.10 in the window, no gate opening it) is worth a follow-up on #247 rather than scope creep here.

Checked and clean

shell: bash on both run: steps in the proposed YAML, and no pipes in either — the default bash -e {0} has no pipefail, so this matters and the comment gets it right and says why. The job cannot be path-filtered out of reporting: it is needs: + if: ${{ !cancelled() }} with the seal outcome gating the expensive steps, so it always reports and can become a required check. No pull_request.base.* dependency, so the trigger does not need edited. The patch-in-comment phrasing is pending-tense in CLAUDE.md, pg-compat/README.md and the PR body, and no doc claims a gate that no workflow file defines. Conventional-commit title passes. Prose style holds: 1 em dash in 603 words of added prose across the delta, against base files that sit at 0–1. standardized-readmes does not apply to a package README inside the monorepo. CI on 7caa296 was green across all 20 checks.

Two SDK limitations are declared and asserted on rather than skipped, and 1.x's absent private policy is required absent. That is the right instinct for a gate, and the gate-teeth test is what keeps it honest.

Comment thread pg-compat-js/src/readers/pkg-stub.mjs
@dobby-coder
dobby-coder Bot marked this pull request as ready for review July 28, 2026 08:46
@rubenhensen

Copy link
Copy Markdown
Contributor

/dobby workflow half applied verbatim (both parts: the outputs.sealed hand-off + filter addition on wire-compat-rust, and the wire-compat-js job). This push touches build.yml, so both halves run on this PR — the Rust gate is already a required check as of #266's merge; once this lands I'll add 'Wire compat (published pg-wasm/pg-js)' as required alongside it (#262). Please re-review the applied commit.

@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Got it — I'm on it. I'll re-review the latest commit on #267, checking that both the wire-compat-rust hand-off/filter change and the new wire-compat-js job in build.yml look correct.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: request-changes

Re-reviewed 40af2e9. Part 2 landed verbatim — the wire-compat-js job is byte-identical to the YAML in the comment. Part 1 did not land: the commit is 57 insertions(+), 0 deletions(-) and touches no existing line, so neither the outputs.sealed declaration nor the pg-compat-js/** filter entry reached wire-compat-rust. Without them the new job is a green no-op, which is the failure mode #262 would then make required.

Three things to fix, all in .github/workflows/build.yml, plus three doc lines this commit made false. Details inline; the short version:

  1. outputs.sealed is undeclared. git show origin/main:.github/workflows/build.yml | grep -n 'outputs:' matches nothing. Actions does not error on an undeclared output — needs.wire-compat-rust.outputs.sealed is the empty string, == 'success' is false on every run, and all four steps skip. Wire compat (published pg-wasm/pg-js) reports success having downloaded nothing and opened no container.
  2. pg-compat-js/** is not in the wire: filter. A PR touching only the gate's own harness leaves wire == 'false', so sealed is 'skipped' and the JS gate no-ops. The gate would not run on changes to itself.
  3. The branch is not rebased. mergeable: CONFLICTING / DIRTY: #266 appended wire-compat-rust + pg-compat-lint to the end of this file as 1237573, this branch appends wire-compat-js at the same place from base 2ba47a1. Consequences: gh pr checks 267 reports no checks and the head sha has zero check-runs, so neither half has actually run on this PR yet; and needs: wire-compat-rust names a job absent from this branch's own build.yml, so the file is invalid when evaluated from the head commit rather than a merge commit.

Rebase onto main, then apply Part 1 on main's wire-compat-rust job — two edits, both between runs-on: and permissions: / inside the existing filters: block:

  wire-compat-rust:
    name: Wire compat (published pg-core)
    runs-on: ubuntu-latest
    outputs:
      sealed: ${{ steps.seal.outcome }}
    permissions:
            wire:
              - 'pg-core/**'
              - 'pg-wasm/**'
              - 'pg-compat/**'
              - 'pg-compat-js/**'

What is right about the applied job

The parts that are easy to get wrong are correct. The artifact name matches the upload character for character, including the github.event.pull_request.head.sha || github.sha fallback — a plain github.sha would look for an artifact that does not exist on a pull_request event. if: ${{ !cancelled() }} with the upload placed before wire-compat-rust's own read step means a red Rust half still leaves the bytes and still lets the JS half report. Every expensive step is behind the seal outcome rather than a second copy of the path filter, so there is one filter to keep honest instead of two. shell: bash on both run: steps gets -eo pipefail instead of the default bash -e {0}. npm ci with cache-dependency-path pinned to pg-compat-js/package-lock.json means the readers are the lockfile's, not whatever resolves. setup-node@v6 matches delivery.yml's publish-wasm, and download-artifact@v4 pairs with the existing upload-artifact@v4.

Test run at HEAD

npm ci && npm test in pg-compat-js: 22 tests, 17 pass, 5 fail, 0 cancelled, and all five failures are the artifact-dependent cases reporting ENOENT ... manifest.json / Seal the sample set first — which is what they should do with no sealed set present. The support-window check (ok 18) parses the real COMPATIBILITY.md and agrees with readers.mjs, and ok 20 confirms each npm alias resolved to the declared package and version.

One forward note for #262

Before adding Wire compat (published pg-wasm/pg-js) as required, note that a workflow_dispatch from main is the one entry point where dorny/paths-filter decides from main's single most recent commit rather than a real diff (isBaseSameAsHead is true, beforeSha is null outside push, so it falls back to getChangesInLastCommit()). That reports the gate green whenever that one commit misses the filtered surface — a false pass on the entry point a human explicitly asked for. It is inherited from #266's already-merged job, not introduced here; || github.event_name == 'workflow_dispatch' on each conditional step's if closes it if you want the dispatch to force both halves.

Comment thread .github/workflows/build.yml
Comment thread .github/workflows/build.yml
Comment thread .github/workflows/build.yml
Comment thread CLAUDE.md Outdated
Comment thread COMPATIBILITY.md
Comment thread pg-compat/README.md Outdated
Comment thread .github/workflows/build.yml
@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Re-reviewed 40af2e9changes requested: #267 (review)

Part 2 (wire-compat-js) landed verbatim, but Part 1 didn't make it into wire-compat-rust:

  • No outputs.sealed declared on the job, so needs.wire-compat-rust.outputs.sealed is always empty and every downstream step in the JS job skips — it'd report success without downloading anything or opening a container.
  • pg-compat-js/** isn't in the wire: filter, so a PR touching only the harness leaves the gate skipped too.
  • The branch itself is unrebased — mergeable: CONFLICTING, zero check-runs on the head sha, and needs: wire-compat-rust points at a job that doesn't exist in this branch's own build.yml.

Left 7 inline comments with the exact YAML for both fixes, plus 3 doc lines (CLAUDE.md, pg-compat/README.md, COMPATIBILITY.md) that are now stale since #266 merged. What did land is solid — artifact naming, if: ${{ !cancelled() }} placement, filter dedup, npm ci pinning all check out, and 17/22 tests pass at HEAD (the 5 failures are expected with no sealed set present).

Once rebased with those two edits applied, this should be ready to go.

@rubenhensen

Copy link
Copy Markdown
Contributor

/dobby all blocking items addressed, properly this time:

  • Merged main in and resolved all three conflicts — the branch is mergeable again and CI actually runs.
  • Part 1 applied for real, with assertion-checked edits: outputs.sealed declared on wire-compat-rust, pg-compat-js/** in the wire filter. You were right that my previous commit's replaces silently matched nothing — a 57-insertions-0-deletions no-op is exactly the silent-failure class this gate exists to catch, and my apply process now asserts each anchor matched exactly once.
  • Docs trued up in the same pass: CLAUDE.md's two wire-compat bullets are present-tense (rust bullet from main + your pg-compat-js bullet updated), pg-compat/README.md keeps your outputs.sealed paragraph with 'proposed/not applied yet' dropped and the head-sha artifact name, COMPATIBILITY.md's reader-list paragraph now says both halves enforce it.
  • pkg-stub note left alone as requested; the wasm-browser flake on main is noted as pre-existing.

This push trips the filter, so both halves run here — the JS gate should now actually download and open the set (not the no-op green). Please re-review.

@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Got it, thanks for the detailed rundown! I'm on it — kicking off a fresh review of #267 now that main is merged in, the outputs.sealed/pg-compat-js gate fix is applied for real, and the docs are trued up. Will report back shortly.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: approve

Re-reviewed at 1016d8b4. All three blocking items from the last cycle are genuinely addressed this time — I verified each against the tree and the run rather than the description.

The gate is not a no-op green. wire-compat-rust declares outputs.sealed: ${{ steps.seal.outcome }} and the filter carries pg-compat-js/**, both on the branch (2e91728, 40af2e9). More to the point, Wire compat (published pg-wasm/pg-js) ran every step on this head sha — download, npm ci, npm test all success, tests 22 / pass 22 / fail 0 / cancelled 0 — and the log shows the set actually opening: mem and mem-privsig by @e4a/pg-wasm@0.6.1, the three stream cases by all three readers. That is five cases read, not a skipped job reporting green.

Branch is MERGEABLE. Docs match the tree: CLAUDE.md is present-tense with the workflow file in the same commit range, pg-compat/README.md drops the proposed/not-applied framing and carries the head-sha artifact name.

Rule check clean on the ones that bear on this diff:

  • actions-default-shell-has-no-pipefail — both new run: steps declare shell: bash.
  • path-filtered-job-cannot-be-a-required-check — the filter stays step-level, build.yml is in it, and if: ${{ !cancelled() }} means wire-compat-js always reports. I walked the four skip paths (filter false, seal fails, wire-compat-rust dies before seal, dispatch/push): each leaves sealed at something other than 'success', every step skips, the job reports. Fit for a required check.
  • pull-request-default-types-exclude-editededited is in the trigger, so a retarget re-runs the base-dependent filter.
  • patch-in-comment-gate-is-not-live-yet — this was the previous blocker and it is resolved in the right direction: the workflow and the present-tense prose now sit in the same tree.
  • node-test-cancelled-is-not-green — checked cancelled 0 in the CI log, not just fail 0.

Six non-blocking notes inline: one reachable diagnostic drop in stderrTail, one docs sentence that contradicts the caveat six lines below it, one remaining member of the silent-green class, one sibling guard, and two wrap-width nits.

The only one I would fix before merge is the COMPATIBILITY.md sentence — it overstates gate coverage in the normative document, which is the exact class this PR exists to close, and it is a one-phrase edit. The rest can ride or land as a follow-up.

Comment thread COMPATIBILITY.md Outdated
Comment thread pg-compat-js/src/failures.mjs
Comment thread pg-compat-js/test/wire-compat.test.mjs
Comment thread pg-compat-js/src/manifest.mjs
Comment thread pg-compat-js/test/manifest.test.mjs Outdated
Comment thread pg-compat/README.md Outdated
@dobby-coder

dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Re-reviewed at 1016d8b4 and this checks out — approve. I verified all three blocking items against the actual tree and CI run rather than taking the description at face value: the JS gate genuinely ran (22/22, and I traced the log to confirm it opened mem/mem-privsig plus the three stream cases, not a no-op green), outputs.sealed and the pg-compat-js/** filter are both really on the branch, and the docs match the tree. Branch is mergeable.

I posted the review as a COMMENT (GitHub won't let me APPROVE my own PR) with VERDICT: approve on the first line: #267 (review)

Six non-blocking notes inline — none are merge blockers, but I'd fix the first one before merging since it's a one-phrase edit: COMPATIBILITY.md:85 says "enforces the crates.io rows," which contradicts the caveat six lines below and overstates gate coverage. The rest (stderrTail banner drop, a manifest-driven coverage invariant, a missing-recipients guard, and two line-wrap nits) are minor and can wait.

@rubenhensen

Copy link
Copy Markdown
Contributor

COMPATIBILITY.md fixed — you were right that it overstated the crates.io half, and the fix went further than the one phrase because the surrounding lines had the same problem:

Left alone as you asked: the pkg-stub unref() hazard. The wasm-browser flake on main is pre-existing.

Ready to merge from my side.

- stderrTail kept the LAST non-frame stderr line, which for a Node crash dump is the 'Node.js vX' banner — the real error was dropped on exactly the run where the message matters most (a child that died before it could report). Now takes the first informative line and filters the banner/preamble too.
- readManifest refused a set with no cases but not a case with no recipients; verify.mjs loops over recipients, so an empty list reported as opened cleanly without checkOpened ever running.
- The coverage invariants were computed from the manifest, so a sealer that stopped emitting the memory-mode cases kept them satisfied and both halves went green having lost the only coverage memory mode has. The case-name set is now asserted explicitly.

Verified against a freshly sealed set: 22/22 pass, and removing the mem cases from the manifest now fails the assertion (it previously passed).
@rubenhensen

Copy link
Copy Markdown
Contributor

Went back through every open thread, verified each against HEAD, and resolved the ten that are genuinely fixed. Three of them needed new work — all in the silent-green class this PR exists to close, so I fixed them rather than deferring:

  • stderrTail dropped the real error (your repro was right): it kept the last non-frame line, which on a Node crash dump is the Node.js vX banner. Now takes the first informative line and filters the node:events/throw er;/^/banner preamble — so a child that dies before it can report (partial npm ci, unhandled rejection) actually says why.
  • readManifest now rejects a case with no recipients. Your reasoning held: verify.mjs loops over recipients, so an empty list reported as opened cleanly with checkOpened never running.
  • The case set is pinned explicitly. Your walk-through was exactly right, and I verified the failure mode rather than trusting it: with the assertion removed, deleting mem/mem-privsig from the manifest leaves both coverage invariants satisfied and the suite green. With it, that now fails. Note the real name is stream-multi-segment (I checked sample_set.rs rather than guessing).

Verified against a freshly sealed set: cargo run --example seal-samples22/22 pass, exit 0, all five cases opened by their expected readers.

Left open deliberately, both yours:

The wasm-browser flake you flagged on main is pre-existing and matches the chronic ChromeDriver flakiness already tracked in this repo — not fallout from here, as you said.

Ten measured lines plus the one prose line I left over 80 when editing pg-compat/README.md in place. No behaviour change; the suite still passes 22/22. Cheaper now than as churn when a formatter lands.
@rubenhensen

Copy link
Copy Markdown
Contributor

Both wrap nits fixed — the ten code lines you measured, plus the README prose line, which was my own leftover from editing that paragraph in place. No behaviour change; suite still 22/22 against a freshly sealed set, and nothing in the package now exceeds 100 chars.

I also resolved your pre-existing wasm-browser-flake thread: it is real but tracked as chronic ChromeDriver flakiness in this repo, and as you said it is not fallout from this branch.

That leaves only the pkg-stub.mjs unref() thread open, deliberately — you asked me not to touch it, and it stands as the recorded latent hazard.

… case process

The child ends with `process.exitCode = ...` rather than process.exit(), because pipe writes are async on macOS and exiting outright truncates the failure lines the gate exists to print. So it exits only when the loop drains, and a listening server is a live handle: a stub that outlived its case would hold the child open until runCase's 300s spawnSync timeout, turning a one-line failure into a stall.

Today nothing reaches that: startPkgStub is the last fallible statement in the pg-js reader's load(), and verify.mjs always unloads. But that invariant lives in two other files and nothing stated or enforced it, which is what made this worth closing rather than just recording.

Three parts: unref() so the leak cannot hang anything, a comment at the one place the invariant can be violated, and a test that spawns a child leaking a stub and asserts it still exits.

Verified: the probe exits in 0s (it would previously have run to the timeout), and the full gate still passes 23/23 against a freshly sealed set with all five cases opened — unref does not stop the stub serving, since the in-flight request and pg-js's own client socket keep the loop alive.
@rubenhensen

Copy link
Copy Markdown
Contributor

Closed the pkg-stub hazard after all — you asked me not to fix it on your account, but the maintainer asked for it directly, so here it is.

Fixed the shape of the problem rather than just the symptom. unref() alone makes a leak survivable but leaves the invariant invisible, and the invariant is the actual risk: startPkgStub must stay the last fallible statement in the pg-js reader's load(), because verify.mjs returns early on a load throw and never calls unload — and that dependency spans two files with nothing stating it. So:

  1. server.unref() in startPkgStub, with the reasoning (the exitCode-not-exit() choice is what makes a live handle load-bearing).
  2. A comment at the one line that can violate it, in load().
  3. A regression test that spawns a child which leaks a stub and asserts it still exits — because a comment does not survive a refactor.

Verified both directions: the leak probe exits in 0s (previously it would have run to runCase's 300s timeout), and the full gate still passes 23/23 against a freshly sealed set with all five cases opened by their expected readers — so unref does not stop the stub serving. It cannot: the in-flight request and the client socket pg-js opens from the same process both keep the loop alive on their own.

That was the last open thread on this PR.

@rubenhensen
rubenhensen merged commit 4e6e2c2 into main Jul 28, 2026
28 checks passed
@dobby-coder
dobby-coder Bot deleted the test/wire-compat-js-readers branch July 29, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compat gate: published pg-wasm/pg-js old-reader step (Node)

1 participant