test(pg-core): assert build.yml's gate wiring so an unapplied patch fails loudly

[rubenhensen]

Part of #272.

Every gate in this repo lands in two halves — the code and tests in a PR, the workflow YAML applied by hand, because dobby-coder has no workflows: write. Nothing detected a missing second half. It has already happened twice (#265 merged pg-compat with no job calling it; postguard-js#137 patched one workflow of two), and the PR was green both times.

pg-pkg/tests/api_gate.rs closed that hole for api-diff.yml by reading the workflow back. pg-core/tests/ci_wiring.rs is the same instrument pointed at build.yml, which carries the three gates nothing machine-read until now.

what it asserts	why that line is load bearing
the job named Wire compat exists, is unique, and needs both halves	the main: required checks ruleset (bypass_actors: []) pins that literal string. A ruleset requiring a context no job produces blocks nothing, so renaming the aggregator disarms the gate instead of breaking it (#299)
wire-compat-rust's 7 filter paths, push override, seal + open commands, sealed output	a dropped filter path does not fail the gate, it stops it firing; the push override is what stopped #297's class of silent pass
wire-compat-js gates on that output and runs npm test in pg-compat-js	both readers must be held to the same bytes, or a non-deterministic sealer hides between them
semver-checks calls both scripts and reads the declaration off the PR title	a body-only BREAKING CHANGE: footer never reaches the squashed commit release-plz reads

Why pg-core and not next to each gate

Test workspace (pg-core) runs unconditionally. pg-compat's tests run inside wire-compat-rust — a guard living beside the gate would be skipped by exactly the deletion it exists to catch.

Verified by mutation, not by inspection

18 drifts applied to build.yml one at a time, all 18 red. The two worth naming: deleting the aggregator outright, and moving the name Wire compat onto wire-compat-rust so a single-half job satisfies the ruleset — both caught, the second with 2 jobs carry that name.

This PR touches no workflow file, so there is no second half to apply.

What it deliberately cannot reach

That the ruleset still names Wire compat with bypass_actors: [] needs gh api repos/encryption4all/postguard/rules/branches/main, which no test runner can call. That half stays manual, as #272's own comment says.

Also in here

COMPATIBILITY.md called wire-compat-rust "a required PR check" and listed #262 and postguard-js#131 under "planned consumers, each still an open issue". Both shipped; the required context has been Wire compat since #296. Corrected, and CLAUDE.md gains the edit-them-together rule.

[dobby-coder]

On it — I'll review this PR (part of #272) focusing on whether pg-core/tests/ci_wiring.rs correctly reads back build.yml and catches the Wire compat job, filter paths, and semver-checks wiring you described, then post one consolidated review.

[dobby-coder]

Rules + review, cycle 1 on 2045c90. 4 findings: 1 blocking, 3 nits.

Ran on the branch: cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream --test ci_wiring — 5 passed. cargo fmt --check clean.

This is the right instrument in the right place, and the argument for pg-core over pg-compat holds up: Test workspace (pg-core) has no if: and no path filter, so it really does run on every PR, while pg-compat's tests run inside wire-compat-rust and would be skipped by the deletion they'd be guarding against. The prefix guard in every_job_this_test_reads_is_still_found is a real trap avoided — job() matches " wire-compat:" exactly, so wire-compat and wire-compat-rust never alias.

The blocking one is a hole in the assertion the file's own headline case depends on: the aggregator test pins the name, the uniqueness, the needs and the if:, but never the step that reads those results. Four one-line drifts to build.yml leave the suite green while turning the repo's sole required check permanently green — verified locally. Inline, with a suggestion that turns all four red.

Checked and clean, so you don't re-derive it

• Docs cross-references all resolve. #251, #262 and postguard-js#131 are closed, postguard-e2e#21 and #25 are open — so demoting #262/postguard-js#131 out of "still planned, each an open issue" and dropping #251 is accurate, and no link definition is left dangling in COMPATIBILITY.md. #251 survives in pg-compat/README.md and pg-compat-js/README.md as parentage, which is still correct.
• path-filtered-job-cannot-be-a-required-check — both gates scope with a step-level dorny/paths-filter and both filters include .github/workflows/build.yml, so a change to a job re-runs it. The test pins both path sets exactly.
• actions-default-shell-has-no-pipefail — build.yml has exactly one shell pipe (echo "$SHA256 …" | sha256sum -c -), sha256sum is last, and that step declares shell: bash anyway.
• cargo-semver-checks-wasm-and-overload-traps — the 100-vs-101 mapping and RUSTFLAGS="--cap-lints=warn" both live in scripts/semver-checks.sh (lines 76 and 125), covered by semver-checks-test.sh, which the workflow calls and this test pins. The workflow's half is right.
• patch-in-comment-gate-is-not-live-yet — every job the new CLAUDE.md bullet and COMPATIBILITY.md describe exists in build.yml on this branch. Nothing is claimed present-tense that is still pending. Correctly, this PR touches no workflow file, so there is no second half to apply.

The three gates / four jobs wording in CLAUDE.md reads fine to me — the aggregator isn't a gate, it's what makes two gates into one context, which is exactly how the file's doc comment puts it.

[dobby-coder]

Blocking. The aggregator test never asserts that Wire compat acts on the results it needs. It pins the name, the uniqueness, the needs set and if: ${{ !cancelled() }} — but not the one step that turns a red half into a red required check.

All four of these drifts to build.yml leave the suite green (each applied on its own to this branch, suite re-run each time):

• exit 1 → exit 0 in the verdict step
• dropping "${{ needs.wire-compat-js.result }}" != "success" from the condition
• dropping the Rust half's clause the same way
• deleting the condition and exit 1, leaving only the two echos

Each one yields a Wire compat that keeps its name, needs both halves, runs on !cancelled() — and reports success however those halves ended. That is the repo's sole required check, with bypass_actors: [], permanently green. Strictly worse than the rename in #299 this file was written to catch: a rename at least disarms one ruleset visibly, this certifies a failure as a pass.

The doc comment scopes the threat as "a Wire compat that needs one half, or none". The case it misses is the aggregator that needs both and reads neither.

The suggestion below adds that assertion. Verified on this branch: compiles, cargo fmt --check clean, passes on the unmutated build.yml, and red on all four drifts above.

Suggested change

	}
	// `needs` only makes the halves' results *available*; what turns a red half
	// into a red required check is the step that reads them. Without this the
	// aggregator keeps its name, its `needs` and its `if:` while its script
	// stops failing -- a sole required check that is permanently green.
	let steps = steps(&aggregator);
	let verdict = step_with(&steps, "exit 1");
	for half in AGGREGATED_HALVES {
	assert!(
	verdict.contains(&format!(r#""${{{{ needs.{half}.result }}}}" != "success""#)),
	"the job named {REQUIRED_CHECK:?} does not fail on a red `{half}`, so the repo's \
	sole required check reports success however that half ended",
	);
	}
	}

[rubenhensen]

Right, and it was the one gap that mattered — taken in 933b450.

Reproduced all four drifts myself before fixing, then re-ran them after: exit 0, each clause dropped on its own, and the condition deleted leaving only the echoes. All four now red on the_required_check_name_still_names_the_aggregator.

Took the substance rather than the suggestion block verbatim: asserting against the job block instead of step_with(&steps, "exit 1"), so deleting the verdict step fails with a message about the required check rather than about the reader not finding a step. Same four drifts caught either way.

You put the diagnosis better than the file did, so it is now failure class 3 in the module doc: needs makes the results available; only the step that reads them turns a red half into a red check.

[dobby-coder]

Nit — one line, in a file this PR doesn't touch. split_once("\njobs:\n") here (and at line 159 in job_ids) is a multi-line anchor matched against a file read with fs::read_to_string. rustc normalises CRLF to LF inside string literals; read_to_string does not. So on a Windows checkout (core.autocrlf=true is the Git for Windows default) the file is \r\njobs:\r\n and the anchor misses.

Reproduced on this branch rather than reasoned about — sed -i 's/$/\r/' .github/workflows/build.yml, then the suite:

test result: FAILED. 0 passed; 5 failed
panicked at pg-core/tests/ci_wiring.rs:139:28: no top-level `jobs:` in .../build.yml

All five, on a clean tree, with a message blaming the workflow rather than the line endings. Every other reader in this file is already CRLF-safe (line.trim(), trim_end(), single-line contains) — only this anchor breaks, which is why it hides.

CI here is ubuntu-latest plus one macos-latest, no Windows runner, so the blast radius is local runs only and this can't redden a PR. Non-blocking for that reason. Same class and same fix as the two entries already in root .gitattributes (cryptify#203, #269):

# pg-core/tests/ci_wiring.rs locates the `jobs:` block in this workflow with a
# multi-line anchor. Same reasoning as the specs above.
.github/workflows/build.yml -text

[rubenhensen]

Confirmed — sed -i "" $'s/$/\r/' on the workflow reds all five on a clean tree, with the panic blaming the workflow rather than the line endings. Fixed in 933b450`, and a CRLF checkout now passes.

Went with normalising at read time (.replace("\r\n", "\n") in workflow()) rather than the .gitattributes entry. The two entries already there have no choice: their anchors are multi-line raw-string content copied out of the spec, which cannot be normalised away without changing what is being matched. Here every reader is line-oriented and line endings carry no meaning in YAML, so normalising holds however the tree was checked out — including a clone that predates the attribute, which the -text route does not. The reasoning is in the workflow() doc comment so the next person does not "fix" it back.

[dobby-coder]

Nit. The declaration is read off github.event.pull_request.title, and this test pins that — but not the trigger that makes a title edit re-run it. build.yml's on: block carries types: [opened, synchronize, reopened, edited] with a comment naming edited as load bearing, and nothing in this file reads the on: block, so dropping it back to a bare on: pull_request: passes the suite.

The default types are opened, synchronize, reopened — not edited. Two silent passes follow, both in the class this file exists to catch:

• A PR opened as feat!: runs the gate with release_type=major and goes green. Retitling to fix(pg-core): fires only edited, so the green stays attached to the unchanged head sha — and release-plz then cuts a patch release of a break the gate waved through, which is the exact scenario the id: declared comment argues against.
• A base retarget also fires only edited, so paths-filter's "nothing relevant changed" verdict survives against a base it never compared.

No suggestion block: where this belongs is a judgement call (it is about on:, not about a job, so it sits outside the job() helper). Something like this in whichever test you'd rather own it:

assert!(
    workflow().contains("types: [opened, synchronize, reopened, edited]"),
    "`edited` is no longer a pull_request trigger, so retitling a PR (or retargeting it) \
     leaves the gate's verdict attached to the title and base it was computed from",
);

[rubenhensen]

Taken in 933b450, as its own test — a_retitled_or_retargeted_pr_still_re_runs_the_gates. It is about on:, not about a job, and it covers both gates rather than only the semver one, so it did not belong inside either.

Asserts the types: list as a set against a pinned PULL_REQUEST_TYPES rather than contains on the literal line, so a reorder does not red it while a drop still does. Both drifts verified red: edited removed, and the whole types: line deleted back to the defaults.

The retitle scenario is the sharper half of this and is now written into the test doc, since it compounds: --release-type major skips every lint, so the green that survives the retitle verified nothing in the first place.

[dobby-coder]

Nit. npm ci isn't pinned, only npm test is. build.yml carries an explicit comment on why the install command matters — "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" — but rewriting it to npm install, or dropping its working-directory: pg-compat-js, keeps this suite green. Both verified on this branch.

Same silent-narrowing class as a dropped filter path: the gate still runs, still reports, and is measuring readers COMPATIBILITY.md never declared. Optional, but it is one line and it closes the gap the workflow comment already names.

Verified: compiles, fmt-clean, passes unmutated, red on both drifts.

Suggested change

	}
	// `ci`, not `install`: the readers are pinned by pg-compat-js's lockfile,
	// so a gate that quietly resolved a different reader would be measuring
	// something other than COMPATIBILITY.md's support window.
	let install = step_with(&steps, "run: npm ci");
	assert!(
	install.contains("working-directory: pg-compat-js"),
	"wire-compat-js no longer installs the pinned readers with `npm ci` in pg-compat-js, so \
	the gate measures whatever npm resolves rather than the declared support window",
	);
	}

[rubenhensen]

Taken in 933b450. Verified both drifts red: npm ci → npm install, and the step losing its working-directory.

Worth the line for the reason you gave — it is the same silent-narrowing class as a dropped filter path, where the gate keeps running and reporting while measuring something other than the declared support window. The workflow comment naming it and nothing enforcing it is exactly what this file exists to replace.

[dobby-coder]

Reviewed #316 and posted a REQUEST_CHANGES review with 4 inline comments (1 blocking, 3 nits).

The blocking one: ci_wiring.rs pins the Wire compat aggregator's name and its needs/if, but never checks that the step reading those results actually consumes them — so a one-line drift in build.yml could leave the suite green while the required check stays green for the wrong reason. That's exactly the class of silent-pass hole this test was meant to close, so worth tightening before merge.

I also updated the CRLF detection rule in memory, since its documented tell would have missed this case.