docs: true up CLAUDE.md with the cryptify workspace merge - #287
Conversation
Root CLAUDE.md still described a four-crate workspace with cryptify as a separate repo, and mentioned cryptify zero times, though ba380a1 (#277) merged it as a fifth member on 2026-07-31. Found while reconciling the #247 map tree (#281). What was stale or missing: - Workspace members, the per-crate CI invocations (cryptify has no features of its own, so --all-targets), and the check-context rename from Test workspace (core) to Test workspace (pg-core) that came with keying the matrices on the crate directory. - The standing sqlx<->rusqlite coupling the merge deliberately bought: one links = "sqlite3" claimant per graph, optional deps in the graph whether enabled or not, so sqlx 0.9 + rusqlite 0.39 is the only intersecting pair and every rusqlite bump waits on sqlx's ceiling. - api-diff.yml is now a two-spec matrix, and its single fail-on and include-checks literals are load-bearing: both pg-pkg/tests/api_gate.rs and cryptify's in-crate api_gate_tests grep for exactly one of each. - Two Docker builds, cryptify's built from the repo root, both on rust:1.96.1-slim-trixie. - cryptify's release-plz entry, and the hand-placed cryptify-v0.1.27 floor tag that stops release-plz changelogging the whole import. - cryptify/CLAUDE.md exists, so root is no longer the single home. - cryptify keeps a renamed rand08 dev-dependency because pg-core's Sealer/TestSetup are still on rand 0.8 while the crate runs 0.10. Two facts the reconciliation itself established: - pg-core 0.6.2 is the first published release off the abandoned irma 0.2.1 and reqwest 0.11 (#263), which is why pg-compat's =0.6.1 pin resolves a tree unlike the workspace's -- and that pin plus COMPATIBILITY.md's crates.io reader row are now a release behind, with no gate reading them (#268). - A history-preserving import silently closes issues here: the cryptify re-attach carried 18 closing keywords resolved against this repo's numbering and closed live issue #146. Includes the audit command and the timeline query that distinguishes a keyword close from a hand one.
|
Thanks for opening this, @rubenhensen — I'm on it. I'll review PR #287 for correctness and compliance with the repo's rules, then post one consolidated review with a clear verdict. |
There was a problem hiding this comment.
Docs-only, and the direction is right — the file's most basic fact was wrong for four days and this fixes it. I verified the added claims against the repo rather than taking them on trust; most hold up exactly. Four comments, two of them blocking.
Blocking
- The file ships self-contradicting. Line 7 documents the rekeying onto the crate directory; line 13 still says
cargo fmt --manifest-path pg-<crate>/..., which expands topg-cryptifyand is not a directory. Line 13 sits in the gap between the two hunks, so it needs its own edit. - The keyword-audit numbers in the import bullet are off by one in three places, and that bullet is the one whose command the next agent runs:
ba380a1carries 17 closing references, not 18;#273is not among them (it comes froma55c6a0, #274); so 16 were no-ops, not 17. #146 also stood for four days, not five, and is OPEN again since2026-08-04T12:11:58Z.
Non-blocking
links = "sqlite3"is declared bylibsqlite3-sys, notrusqlite— confirmed withcargo metadata. Suggestion inline.- The cryptify image bullet reads present-tense, but all three image jobs are gated on
vars.PUBLISH_CRYPTIFY_IMAGE == 'true'and skip with it unset.
What I checked and found accurate: the --all-targets/--all-features split and the crate: matrix in build.yml; api-diff.yml as a two-spec matrix with exactly one fail-on: and one include-checks:; both Dockerfiles on rust:1.96.1-slim-trixie with cryptify built from the repo root; the release-plz cryptify entry (publish = false, git_only = true) and the cryptify-v0.1.27 floor tag on ba380a1; the sqlx/rusqlite resolve-graph mechanism itself; and every section line 3 attributes to cryptify/CLAUDE.md (Config, Upload flow and state lifetime, Token chain, CORS, Metrics, Integration test harness). Rule sweep found nothing further — PR title is conventional-commit compliant, the one cross-repo reference uses the owner/repo#NN auto-link form, and the typo sweep is clean.
Every fix here is a one-line edit on the branch.
|
|
||
| - Workspace members: `pg-core` (lib), `pg-ffi` (C ABI), `pg-pkg` (PKG service), `pg-cli`. `pg-wasm` is a sibling crate the root `Cargo.toml` lists under `exclude`, so it is not part of the workspace and is built separately with wasm-pack (see Release & configuration). Sub-crates share workspace files. Build the workspace from repo root with `cargo build`. A bare `cargo test --workspace` FAILS to compile: `pg-core`'s tests are gated behind its `test` feature (also `rust`/`stream`), so the item is configured out and imports like `crate::test::TestSetup` don't resolve. CI (`.github/workflows/build.yml`) runs tests per crate: `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` for core, `--all-features` for `pkg`/`cli`/`ffi`. None of these cover `pg-wasm`. `pg-core` uses CGWKV + MKEM for multi-recipient encryption (production feature set `["cgwkv", "mkem"]`). | ||
| - `pg-compat` is a second excluded sibling crate (root `Cargo.toml` `exclude`), and the exclusion is load-bearing: it depends on `pg-core` from **crates.io** (`=0.6.1`), not on `../pg-core`, so it can open bytes sealed by this tree with published readers. It has its own `Cargo.lock` (run it with `--locked`). Its input comes from `cargo run -p pg-core --features stream --example seal-samples -- <dir>`, a deterministic sealer whose output layout is documented in `pg-compat/README.md`. CI wires the two together: `wire-compat-rust` in `build.yml` seals with HEAD and opens with published pg-core on any PR touching the wire surface (pg-core/pg-wasm/pg-compat/pg-compat-js trees, the ROOT `Cargo.lock`/`Cargo.toml` — pg-core resolves from the root lockfile — and build.yml itself); `pg-compat-lint` covers the crate's fmt/clippy, which the per-crate matrices don't. | ||
| - Workspace members: `pg-core` (lib), `pg-ffi` (C ABI), `pg-pkg` (PKG service), `pg-cli`, `cryptify` (file-transfer service, merged in 2026-07-31). `pg-wasm` is a sibling crate the root `Cargo.toml` lists under `exclude`, so it is not part of the workspace and is built separately with wasm-pack (see Release & configuration). Sub-crates share workspace files. Build the workspace from repo root with `cargo build`. A bare `cargo test --workspace` FAILS to compile: `pg-core`'s tests are gated behind its `test` feature (also `rust`/`stream`), so the item is configured out and imports like `crate::test::TestSetup` don't resolve. CI (`.github/workflows/build.yml`) runs tests per crate: `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` for core, `--all-targets` for `cryptify` (it declares no features of its own, so that is the equivalent invocation), `--all-features` for `pkg`/`cli`/`ffi`. None of these cover `pg-wasm`. The `test`/`format`/`clippy` matrices key on the crate **directory** rather than a `pg-` suffix, because `cryptify` doesn't carry that prefix — so the check contexts are `Test workspace (pg-core)`, not `Test workspace (core)`. Nothing required points at them today (only the two `Wire compat` contexts are required), but a ruleset added later must use the current names. `pg-core` uses CGWKV + MKEM for multi-recipient encryption (production feature set `["cgwkv", "mkem"]`). |
There was a problem hiding this comment.
Two things in this bullet, one blocking.
The file now contradicts itself. This bullet documents the rekeying onto the crate directory, but line 13 — five lines down, in the gap between the two hunks — still carries the pre-merge form:
CI's
Format workspacematrix runscargo fmt --manifest-path pg-<crate>/Cargo.toml --all -- --checkper crate
build.yml:61 is cargo fmt --manifest-path ${{ matrix.crate }}/Cargo.toml --all -- --check over crate: [pg-core, pg-pkg, pg-cli, pg-ffi, cryptify], so pg-<crate> expands to pg-cryptify, which is not a directory. (pg-${{ matrix.workspace }} really was the old form — the matrix key at 361cdeb was workspace with short names, which is also what makes the Test workspace (core) -> Test workspace (pg-core) rename here correct.) Line 13 is outside the diff, so it needs its own edit rather than a suggestion — but it is the same fact this bullet exists to fix, and leaving the file self-contradicting undercuts the PR.
Nit on the required-checks parenthetical. "Nothing required points at them today (only the two Wire compat contexts are required)" does not match what I can read. The main ruleset (id 9496106) has no required_status_checks rule at all — its rules are deletion, non_fast_forward, pull_request — so on that evidence nothing is required, not two things. Caveat worth stating: /branches/main/protection is 403 for dobby-coder, so a second source of required checks cannot be ruled out from here. Since the point of the sentence is to orient whoever writes the next ruleset, worth a maintainer confirming which it is. The same claim also sits in build.yml's comment block above the test matrix, so if it is stale it is stale in two places.
| - Workspace members: `pg-core` (lib), `pg-ffi` (C ABI), `pg-pkg` (PKG service), `pg-cli`. `pg-wasm` is a sibling crate the root `Cargo.toml` lists under `exclude`, so it is not part of the workspace and is built separately with wasm-pack (see Release & configuration). Sub-crates share workspace files. Build the workspace from repo root with `cargo build`. A bare `cargo test --workspace` FAILS to compile: `pg-core`'s tests are gated behind its `test` feature (also `rust`/`stream`), so the item is configured out and imports like `crate::test::TestSetup` don't resolve. CI (`.github/workflows/build.yml`) runs tests per crate: `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` for core, `--all-features` for `pkg`/`cli`/`ffi`. None of these cover `pg-wasm`. `pg-core` uses CGWKV + MKEM for multi-recipient encryption (production feature set `["cgwkv", "mkem"]`). | ||
| - `pg-compat` is a second excluded sibling crate (root `Cargo.toml` `exclude`), and the exclusion is load-bearing: it depends on `pg-core` from **crates.io** (`=0.6.1`), not on `../pg-core`, so it can open bytes sealed by this tree with published readers. It has its own `Cargo.lock` (run it with `--locked`). Its input comes from `cargo run -p pg-core --features stream --example seal-samples -- <dir>`, a deterministic sealer whose output layout is documented in `pg-compat/README.md`. CI wires the two together: `wire-compat-rust` in `build.yml` seals with HEAD and opens with published pg-core on any PR touching the wire surface (pg-core/pg-wasm/pg-compat/pg-compat-js trees, the ROOT `Cargo.lock`/`Cargo.toml` — pg-core resolves from the root lockfile — and build.yml itself); `pg-compat-lint` covers the crate's fmt/clippy, which the per-crate matrices don't. | ||
| - Workspace members: `pg-core` (lib), `pg-ffi` (C ABI), `pg-pkg` (PKG service), `pg-cli`, `cryptify` (file-transfer service, merged in 2026-07-31). `pg-wasm` is a sibling crate the root `Cargo.toml` lists under `exclude`, so it is not part of the workspace and is built separately with wasm-pack (see Release & configuration). Sub-crates share workspace files. Build the workspace from repo root with `cargo build`. A bare `cargo test --workspace` FAILS to compile: `pg-core`'s tests are gated behind its `test` feature (also `rust`/`stream`), so the item is configured out and imports like `crate::test::TestSetup` don't resolve. CI (`.github/workflows/build.yml`) runs tests per crate: `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` for core, `--all-targets` for `cryptify` (it declares no features of its own, so that is the equivalent invocation), `--all-features` for `pkg`/`cli`/`ffi`. None of these cover `pg-wasm`. The `test`/`format`/`clippy` matrices key on the crate **directory** rather than a `pg-` suffix, because `cryptify` doesn't carry that prefix — so the check contexts are `Test workspace (pg-core)`, not `Test workspace (core)`. Nothing required points at them today (only the two `Wire compat` contexts are required), but a ruleset added later must use the current names. `pg-core` uses CGWKV + MKEM for multi-recipient encryption (production feature set `["cgwkv", "mkem"]`). | ||
| - `cryptify` was a separate repo until `ba380a1` (#277) merged it as a workspace member, history-preserving, the deliberate yes decided in #255/#285. What that bought: one lockfile, which closes a real drift class — the two trees previously built `pg-core`'s container serializer against two different `bincode-next` versions with nothing comparing them. What it costs is a **standing dependency coupling, and it does not go away**: `pg-pkg` reaches `libsqlite3-sys` through `sqlx`, `cryptify` reaches it through `rusqlite`, which sets `links = "sqlite3"`. Only one package per resolve graph may claim a given `links` value, and optional dependencies sit in the graph whether their feature is enabled or not — so `sqlx-sqlite` is locked even though `pg-pkg` enables only `postgres`. The newest intersecting pair is **sqlx 0.9 + rusqlite 0.39** (`libsqlite3-sys 0.37` in the root lockfile), so every future `rusqlite` bump waits on sqlx's `libsqlite3-sys` ceiling. Don't bump either half alone; check the intersection first. |
There was a problem hiding this comment.
links = "sqlite3" is declared by libsqlite3-sys, not by rusqlite. In "cryptify reaches it through rusqlite, which sets links = \"sqlite3\"" the relative clause attaches to rusqlite, naming the wrong claimant. Confirmed with cargo metadata: libsqlite3-sys 0.37.0 has links = 'sqlite3'; rusqlite 0.39.0, sqlx 0.9.0 and sqlx-sqlite 0.9.0 all have links = None.
The mechanism the bullet describes is otherwise right, and it holds up empirically: a root cargo build compiles sqlx 0.9.0 and rusqlite 0.39.0 in one graph against a single libsqlite3-sys 0.37.0, and sqlx-sqlite 0.9.0 is in the root lockfile even though pg-pkg enables only postgres. Since this is the bullet someone will cite in a dep-bump argument, the claimant should be named correctly.
| - `cryptify` was a separate repo until `ba380a1` (#277) merged it as a workspace member, history-preserving, the deliberate yes decided in #255/#285. What that bought: one lockfile, which closes a real drift class — the two trees previously built `pg-core`'s container serializer against two different `bincode-next` versions with nothing comparing them. What it costs is a **standing dependency coupling, and it does not go away**: `pg-pkg` reaches `libsqlite3-sys` through `sqlx`, `cryptify` reaches it through `rusqlite`, which sets `links = "sqlite3"`. Only one package per resolve graph may claim a given `links` value, and optional dependencies sit in the graph whether their feature is enabled or not — so `sqlx-sqlite` is locked even though `pg-pkg` enables only `postgres`. The newest intersecting pair is **sqlx 0.9 + rusqlite 0.39** (`libsqlite3-sys 0.37` in the root lockfile), so every future `rusqlite` bump waits on sqlx's `libsqlite3-sys` ceiling. Don't bump either half alone; check the intersection first. | |
| - `cryptify` was a separate repo until `ba380a1` (#277) merged it as a workspace member, history-preserving, the deliberate yes decided in #255/#285. What that bought: one lockfile, which closes a real drift class — the two trees previously built `pg-core`'s container serializer against two different `bincode-next` versions with nothing comparing them. What it costs is a **standing dependency coupling, and it does not go away**: `pg-pkg` reaches `libsqlite3-sys` through `sqlx`, `cryptify` reaches it through `rusqlite`, and it is `libsqlite3-sys` that declares `links = "sqlite3"`. Only one package per resolve graph may claim a given `links` value, and optional dependencies sit in the graph whether their feature is enabled or not — so `sqlx-sqlite` is locked even though `pg-pkg` enables only `postgres`. The newest intersecting pair is **sqlx 0.9 + rusqlite 0.39** (`libsqlite3-sys 0.37` in the root lockfile), so every future `rusqlite` bump waits on sqlx's `libsqlite3-sys` ceiling. Don't bump either half alone; check the intersection first. |
| - release-plz owns the version numbers, so the PR making a breaking change cannot bump the crate to match (bumping `pg-core` alone doesn't even resolve: `pg-cli` requires `^0.6.1`). What the semver gate accepts as the declaration is the conventional-commit `!` in the PR title, and only that; CI turns it into `SEMVER_RELEASE_TYPE=major`, which the script passes as `--release-type major`. A `BREAKING CHANGE:` footer in the PR body is not accepted and must not be: this repo's `squash_merge_commit_message` is `COMMIT_MESSAGES`, so the body never reaches the squashed commit, and release-plz reading a bare `fix(pg-core):` subject would cut a patch release of a break the gate had already waved through. Two consequences of the merge settings worth knowing when you declare a break. `squash_merge_commit_title` is `COMMIT_OR_PR_TITLE`, which is the PR title on a multi-commit PR but the commit's subject when the PR has exactly one commit — so on a single-commit PR put the `!` in the commit subject too, or the gate goes green off the PR title while release-plz cuts a patch. And `--release-type major` doesn't merely permit a bigger bump: every lint exists to demand a bump the declaration already grants, so all of them skip and the run checks nothing (`0 checks: 0 pass, 253 skip`) on both surfaces at once. A green gate on a `!` PR verified nothing; a `!` added for a pg-wasm break also passes any unrelated pg-core break in the same PR. | ||
| - The Docker build (`Dockerfile`, `FROM rust:<version>-slim`) pins an older or different Rust than the `Test workspace`/`Format workspace` jobs' `dtolnay/rust-toolchain@stable`. A change can pass every workspace test and still fail Docker Build on a type-inference difference that doesn't reproduce on host stable (e.g. a slice-element-type unification difference across rustc versions). Check the Dockerfile's current pin, and run `cargo build --profile edge --bin pg-pkg` locally before pushing any `Cargo.toml` dependency bump; for a true repro, build the Docker image. | ||
| - `pg-pkg/api-description.yaml` is the pinned v2 HTTP contract and is additive-only (see `COMPATIBILITY.md`). Its breaking-change gate is `.github/workflows/api-diff.yml`, job `API breaking changes (oasdiff)`, which runs on every PR; `pg-pkg/tests/api_gate.rs` is its executable spec. That test is the executable record of what the gate stops; it mutates the spec 19 ways and asserts each verdict. The verdict test skips when `oasdiff` is not on `PATH`, which is the case in CI; the other two run everywhere, one checking that every mutation still finds its anchor and one reading `fail-on`/`include-checks`/the pinned action ref back out of the workflow YAML, so editing the step and the constants apart fails the suite instead of quietly weakening the gate. The action ref is pinned because it is what selects the engine version (`v0.1.10` is `FROM tufin/oasdiff:v1.26.1`), so bumping the action silently re-measures every verdict. Those anchors are multi-line raw strings matched against the spec read off disk, which is why root `.gitattributes` marks `pg-pkg/api-description.yaml -text`: rustc normalises CRLF inside raw strings but `read_to_string` does not, so without it a Windows checkout fails `every_mutation_still_applies` on a clean tree. Run it (and read it) before touching the gate's settings: `go install github.com/oasdiff/oasdiff@v1.26.1 && cargo test --manifest-path pg-pkg/Cargo.toml --all-features --test api_gate`. | ||
| - There are **two** Docker builds, `Dockerfile` (pg-pkg) and `cryptify/Dockerfile`, each with its own image name and independent version output in `delivery.yml`. cryptify's builds with the **repo root** as context (`file: cryptify/Dockerfile`), because the crate is a workspace member and needs the root manifest and lockfile; it publishes to the same `ghcr.io/<owner>/cryptify` name the old repo did, so nothing downstream repoints. Both pin the same Rust today (`FROM rust:1.96.1-slim-trixie`), which is older or otherwise different from the `Test workspace`/`Format workspace` jobs' `dtolnay/rust-toolchain@stable`. A change can pass every workspace test and still fail a Docker Build on a type-inference difference that doesn't reproduce on host stable (e.g. a slice-element-type unification difference across rustc versions). Check the current pins, and run `cargo build --profile edge --bin pg-pkg` locally before pushing any `Cargo.toml` dependency bump; for a true repro, build the image. |
There was a problem hiding this comment.
"it publishes to the same ghcr.io/<owner>/cryptify name the old repo did, so nothing downstream repoints" reads as a statement about what happens today, but all three cryptify image jobs are gated off: build-cryptify, scan-cryptify and finalize-cryptify each carry if: vars.PUBLISH_CRYPTIFY_IMAGE == 'true' (delivery.yml:267, 322, 364), and delivery.yml's own comment is explicit — the GHCR package must first grant this repo Write, and "with it unset they skip, so merging this changes nothing that runs today". dobby-coder gets 403 on the repo-variables API, so I cannot tell whether the variable has since been set.
As written, an agent reading this bullet concludes the cryptify image ships on every main push. One clause fixes it: the name is claimed, but the jobs stay off until PUBLISH_CRYPTIFY_IMAGE is true. CLAUDE.md is auto-loaded agent context, so a present-tense claim about a job that does not run is the expensive kind of wrong.
Everything else in the bullet checks out: both Dockerfiles are FROM rust:1.96.1-slim-trixie, and cryptify's build really does use context: . with file: cryptify/Dockerfile.
| - There are **two** Docker builds, `Dockerfile` (pg-pkg) and `cryptify/Dockerfile`, each with its own image name and independent version output in `delivery.yml`. cryptify's builds with the **repo root** as context (`file: cryptify/Dockerfile`), because the crate is a workspace member and needs the root manifest and lockfile; it publishes to the same `ghcr.io/<owner>/cryptify` name the old repo did, so nothing downstream repoints. Both pin the same Rust today (`FROM rust:1.96.1-slim-trixie`), which is older or otherwise different from the `Test workspace`/`Format workspace` jobs' `dtolnay/rust-toolchain@stable`. A change can pass every workspace test and still fail a Docker Build on a type-inference difference that doesn't reproduce on host stable (e.g. a slice-element-type unification difference across rustc versions). Check the current pins, and run `cargo build --profile edge --bin pg-pkg` locally before pushing any `Cargo.toml` dependency bump; for a true repro, build the image. | ||
| - `pg-pkg/api-description.yaml` is the pinned v2 HTTP contract and is additive-only (see `COMPATIBILITY.md`). Its breaking-change gate is `.github/workflows/api-diff.yml`, which runs on every PR as a **two-spec matrix** — `pg-pkg/api-description.yaml` (versioned `/v2` routes) and `cryptify/api-description.yaml` (unversioned) — so the check names carry the spec path. The matrix is not cosmetic: `pg-pkg/tests/api_gate.rs` and cryptify's in-crate `mod api_gate_tests` both grep this file for exactly **one** `fail-on:` and one `include-checks:` line, so two separate oasdiff steps would give two of each and fail both suites. Vary only the spec paths; moving `fail-on` into the matrix removes the literal both tests look for. `pg-pkg/tests/api_gate.rs` is the executable record of what the gate stops on the pg-pkg side: it mutates the spec 19 ways and asserts each verdict. The verdict test skips when `oasdiff` is not on `PATH`, which is the case in CI; the other two run everywhere, one checking that every mutation still finds its anchor and one reading `fail-on`/`include-checks`/the pinned action ref back out of the workflow YAML, so editing the step and the constants apart fails the suite instead of quietly weakening the gate. The action ref is pinned because it is what selects the engine version (`v0.1.10` is `FROM tufin/oasdiff:v1.26.1`), so bumping the action silently re-measures every verdict. Those anchors are multi-line raw strings matched against the spec read off disk, which is why root `.gitattributes` marks `pg-pkg/api-description.yaml -text`: rustc normalises CRLF inside raw strings but `read_to_string` does not, so without it a Windows checkout fails `every_mutation_still_applies` on a clean tree. Run it (and read it) before touching the gate's settings: `go install github.com/oasdiff/oasdiff@v1.26.1 && cargo test --manifest-path pg-pkg/Cargo.toml --all-features --test api_gate`. | ||
| - The oasdiff gate's settings are **not** self-evident and `--fail-on ERR` alone fails open. `fail-on: WARN` is deliberate: oasdiff rates removing or renaming an *optional* response property, and removing a request parameter, as WARN, and this spec marks only `status` as `required`, so at ERR the gate silently passed a removed `key` (the IBE user secret key `/v2/request/key` exists to return), a renamed `proofStatus` and a dropped `timestamp` parameter. Two more, a changed non-success status (401 to 403) and a dropped response enum value, rate ERR but are **opt-in**, so they only run when named in `include-checks`. Of the 31 WARN checks, `response-property-enum-value-added` is the only one that fires on something `COMPATIBILITY.md` does not already forbid, which is why that document now names a new response enum value as non-additive too. Reproduce a verdict with the exact flags the action's entrypoint builds (`oasdiff/oasdiff-action/breaking@v0.1.10` is `FROM tufin/oasdiff:v1.26.1`, so the pinned tag is what makes a local run authoritative): `git show origin/main:pg-pkg/api-description.yaml > /tmp/base.yaml && oasdiff breaking /tmp/base.yaml pg-pkg/api-description.yaml --allow-external-refs=false --composed=false --fail-on WARN --include-checks response-non-success-status-removed,response-property-enum-value-removed`. Two traps: `oasdiff --version` prints `oasdiff version main` after a `go install` of a tag because the version comes from release ldflags (the code is still the tag), and `--fail-on` takes `ERR`/`WARN` while `oasdiff checks --severity` takes `error`/`warn`/`info`. `--severity ERR` is a usage error, so `oasdiff checks --severity ERR | wc -l` counts the help text instead and badly undercounts the tier (it is 213 error checks, 31 warn, 265 info). The spec has no external `$ref`s, so `allow-external-refs` stays at its safe (SSRF-guarding) default. The gate only sees paths the spec documents, and the spec documents canonical paths only, so dropping the `/v2/irma/...` alias handlers (#257) passes it. | ||
| - **A history-preserving import into this repo silently closes issues here.** Imported commits carry their original messages verbatim, closing keywords included, and GitHub resolves those against the **destination** repo's numbering. The cryptify re-attach carried 18 of them (`Closes #38 #45 #52 #54 #123 #125 #134 #142 #146 #153 #155 #157 #159 #167 #186 #191 #194 #273`); 17 pointed at numbers already closed here, and one — #146, a live unimplemented feature request — was closed by `ba380a14` four seconds after the merge landed. Nothing warned, and it stood for five days. Audit before merging *and* after, from the import branch: `git log origin/main..HEAD --pretty=%B | grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | sort -u`, then check each number here. Defusing costs every imported SHA (`git filter-repo`), so auditing and reopening is usually the better trade — but decide it deliberately, and do the audit either way. Reading close *state* cannot tell you which happened; read the close **event**, where a keyword-close carries a `commit_id` and a hand-close does not: `gh api repos/OWNER/REPO/issues/N/timeline --paginate -q '.[] | select(.event=="closed") | .created_at+" "+(.commit_id//"-")'`. Same failure class as the unapplied-workflow half below (#272); the guard lives in encryption4all/postguard-js#139. |
There was a problem hiding this comment.
Three number drifts in this bullet. It matters more here than elsewhere because the audit command in this bullet is what the next agent runs, and its output does not match the count documented beside it.
17 closing references, not 18, and #273 is not one of them. Closes #273 comes from a55c6a0 ("ci: unignore the tracked JWT rotation fixtures so release-plz runs", #274), an ordinary postguard commit; ba380a1's message does not contain the string 273 at all. I ran this bullet's own command over the imported side (git log 4df67fa --not 361cdeb --pretty=%B | grep -oiE ...) and over git log -1 --format=%B ba380a1; both return exactly #38 #45 #52 #54 #123 #125 #134 #142 #146 #153 #155 #157 #159 #167 #186 #191 #194 and nothing else, under the documented regex and a looser one allowing Closes: #N. Issue #273 ("release-plz PR job has been failing on main since #241") was closed legitimately by its own fix on 2026-07-30, the day before the merge.
So the follow-on count is 16, not 17. I checked all 16 and every closed_at predates 2026-07-31.
#146 stood for four days, not five, and it is OPEN again. Closed 2026-07-31T07:07:34Z by ba380a1, reopened 2026-08-04T12:11:58Z — 4d 5h, the same span this PR's own description calls "four days" for CLAUDE.md. The bullet does not mention the reopen; a reader who checks the issue finds it open and starts doubting the rest of the account.
Everything else is exact — merged 07:07:30, closed 07:07:34, so "four seconds" is literally right, and the timeline query works as documented: the close event carries commit_id ba380a14... and the reopen carries none, which is the distinction the bullet is teaching.
|
Reviewed this and posted a consolidated review: changes requested, with four inline comments on The direction is right — the file did describe the four-crate world for four days after cryptify landed — but two things block before merge:
Two non-blocking notes are attached inline too: Everything else checked out — the |
Review fixes on #287, all four verified against the repo first. - Line 13 still said `cargo fmt --manifest-path pg-<crate>/...`, which expands to pg-cryptify now that the matrix keys on the crate directory. The file contradicted the bullet six lines above it. - The import bullet over-counted: ba380a1 carries 17 closing references, not 18. #273 came from a55c6a0 (#274), an ordinary commit the day before, so the original range e642786..origin/main swept in keywords the import never carried. 16 no-ops, not 17. Records the range mistake itself, since the bullet's command is what the next agent runs. - #146 stood four days, not five, and the bullet now says it was reopened -- a reader who checks and finds it open doubts the rest. - links = "sqlite3" is declared by libsqlite3-sys, not rusqlite; the relative clause named the wrong claimant. Confirmed with cargo metadata: rusqlite, sqlx and sqlx-sqlite all declare none. The error came in from #285's body, so it was inherited rather than invented. - The cryptify image bullet read present-tense, but build/scan/finalize are all gated on vars.PUBLISH_CRYPTIFY_IMAGE and skip while it is unset. CLAUDE.md is auto-loaded, so a present-tense claim about a job that does not run is the expensive kind of wrong. The required-checks parenthetical held up, and is now stronger for being challenged: branch protection on main requires exactly the two Wire compat contexts. That list lives in classic branch protection, NOT in the main ruleset (which has no required_status_checks rule), and dobby-coder gets 403 on the protection endpoint -- so an agent reading the ruleset alone concludes nothing is required. Worth documenting, so it is.
|
All four fixed in Blocking
Non-blocking
On the required-checks nit — your read was reasonable and the claim was right, and the disagreement produced something better than either version. I could check what you couldn't: Everything remains docs-only. |
|
/dobby all four points are fixed in Detail is in the comment above, but the short version:
One thing to double-check on your side, because it went the other way: the required-checks parenthetical was correct. |
|
On it — re-reviewing PR #287 now that |
There was a problem hiding this comment.
Re-reviewed at eff1929. All four points from the last round are fixed, and I re-derived each one rather than reading the diff for intent. Two findings remain, both in the closing-keywords bullet, and both are wrong statements of fact rather than wording — worth one more push because CLAUDE.md is auto-loaded agent context, so a wrong mechanism here is a wrong mechanism in every future run.
Your required-checks point: you were right, and I can confirm it from my side. Ruleset 9496106 carries exactly deletion, non_fast_forward, pull_request and no required_status_checks rule, and branches/main/protection 403s for dobby-coder, so the ruleset really is the only thing I can read and it really does say "nothing required". There is also independent confirmation inside the repo I had missed: build.yml:32 says "only the two Wire compat contexts are required". Documenting the endpoint, the two contexts and the 403 trap is the right fix — that bullet is better than what I asked for. Related and also fine: both Wire compat jobs put the path filter in a step (dorny/paths-filter at build.yml:135/241), so they always report and are legitimately requirable.
Verified fixed:
- Line 13 —
<crate-dir>matchesbuild.yml:61(cargo fmt --manifest-path ${{ matrix.crate }}/Cargo.toml) overcrate: [pg-core, pg-pkg, pg-cli, pg-ffi, cryptify]. The file no longer contradicts itself. - The counts — 17 unique refs, and they are exactly the ones listed. All 16 others closed between 2025-10-30 and 2026-06-09, so all were no-ops. #146 closed
2026-07-31T07:07:34Z, reopened2026-08-04T12:11:58Z: four days. The file is right on every number. libsqlite3-sys— correct, and no in-repoCargo.tomldeclareslinksat all, so thecargo metadatapointer is the right instruction. Lockfile agrees:libsqlite3-sys 0.37.0,rusqlite 0.39.0,sqlx 0.9.0.- The image bullet —
build-cryptify,scan-cryptifyandfinalize-cryptifyall carryif: vars.PUBLISH_CRYPTIFY_IMAGE == 'true'(delivery.yml:267/322/364), and the root-context claim matchesfile: cryptify/Dockerfileat:300.
Also spot-checked and accurate: both Dockerfiles on rust:1.96.1-slim-trixie; api-diff.yml is a two-spec matrix with exactly one real fail-on: (:116) and one include-checks: (:120); cryptify-v0.1.27 points at ba380a1 with cryptify-v0.1.28 following; release-plz.toml has cryptify at publish = false + git_only = true; cryptify/CLAUDE.md and the rand08 dev-dependency both exist as described.
One non-blocking nit, out of the diff so it has no inline thread:
- The PR description is now stale against the file. It still says "18 closing keywords", "17 were no-ops" and "#146 … stood for five days", while the file correctly says 17 / 16 / four days. The file is the one that is right. This is a
PATCH /repos/encryption4all/postguard/pulls/287on the body — no push, no CI rerun.
A rule sweep over the docs-accuracy, prose and CI-facts rules that apply to a docs-only diff returned nothing.
| - There are **two** Docker builds, `Dockerfile` (pg-pkg) and `cryptify/Dockerfile`, each with its own image name and independent version output in `delivery.yml`. cryptify's builds with the **repo root** as context (`file: cryptify/Dockerfile`), because the crate is a workspace member and needs the root manifest and lockfile; it claims the same `ghcr.io/<owner>/cryptify` name the old repo published, so nothing downstream has to repoint. It is not publishing yet: `build-cryptify`, `scan-cryptify` and `finalize-cryptify` all carry `if: vars.PUBLISH_CRYPTIFY_IMAGE == 'true'` and skip while that repo variable is unset, which it is until the existing GHCR package grants this repo Write. Don't read the bullet as "the image ships on every main push". Both Dockerfiles pin the same Rust today (`FROM rust:1.96.1-slim-trixie`), which is older or otherwise different from the `Test workspace`/`Format workspace` jobs' `dtolnay/rust-toolchain@stable`. A change can pass every workspace test and still fail a Docker Build on a type-inference difference that doesn't reproduce on host stable (e.g. a slice-element-type unification difference across rustc versions). Check the current pins, and run `cargo build --profile edge --bin pg-pkg` locally before pushing any `Cargo.toml` dependency bump; for a true repro, build the image. | ||
| - `pg-pkg/api-description.yaml` is the pinned v2 HTTP contract and is additive-only (see `COMPATIBILITY.md`). Its breaking-change gate is `.github/workflows/api-diff.yml`, which runs on every PR as a **two-spec matrix** — `pg-pkg/api-description.yaml` (versioned `/v2` routes) and `cryptify/api-description.yaml` (unversioned) — so the check names carry the spec path. The matrix is not cosmetic: `pg-pkg/tests/api_gate.rs` and cryptify's in-crate `mod api_gate_tests` both grep this file for exactly **one** `fail-on:` and one `include-checks:` line, so two separate oasdiff steps would give two of each and fail both suites. Vary only the spec paths; moving `fail-on` into the matrix removes the literal both tests look for. `pg-pkg/tests/api_gate.rs` is the executable record of what the gate stops on the pg-pkg side: it mutates the spec 19 ways and asserts each verdict. The verdict test skips when `oasdiff` is not on `PATH`, which is the case in CI; the other two run everywhere, one checking that every mutation still finds its anchor and one reading `fail-on`/`include-checks`/the pinned action ref back out of the workflow YAML, so editing the step and the constants apart fails the suite instead of quietly weakening the gate. The action ref is pinned because it is what selects the engine version (`v0.1.10` is `FROM tufin/oasdiff:v1.26.1`), so bumping the action silently re-measures every verdict. Those anchors are multi-line raw strings matched against the spec read off disk, which is why root `.gitattributes` marks `pg-pkg/api-description.yaml -text`: rustc normalises CRLF inside raw strings but `read_to_string` does not, so without it a Windows checkout fails `every_mutation_still_applies` on a clean tree. Run it (and read it) before touching the gate's settings: `go install github.com/oasdiff/oasdiff@v1.26.1 && cargo test --manifest-path pg-pkg/Cargo.toml --all-features --test api_gate`. | ||
| - The oasdiff gate's settings are **not** self-evident and `--fail-on ERR` alone fails open. `fail-on: WARN` is deliberate: oasdiff rates removing or renaming an *optional* response property, and removing a request parameter, as WARN, and this spec marks only `status` as `required`, so at ERR the gate silently passed a removed `key` (the IBE user secret key `/v2/request/key` exists to return), a renamed `proofStatus` and a dropped `timestamp` parameter. Two more, a changed non-success status (401 to 403) and a dropped response enum value, rate ERR but are **opt-in**, so they only run when named in `include-checks`. Of the 31 WARN checks, `response-property-enum-value-added` is the only one that fires on something `COMPATIBILITY.md` does not already forbid, which is why that document now names a new response enum value as non-additive too. Reproduce a verdict with the exact flags the action's entrypoint builds (`oasdiff/oasdiff-action/breaking@v0.1.10` is `FROM tufin/oasdiff:v1.26.1`, so the pinned tag is what makes a local run authoritative): `git show origin/main:pg-pkg/api-description.yaml > /tmp/base.yaml && oasdiff breaking /tmp/base.yaml pg-pkg/api-description.yaml --allow-external-refs=false --composed=false --fail-on WARN --include-checks response-non-success-status-removed,response-property-enum-value-removed`. Two traps: `oasdiff --version` prints `oasdiff version main` after a `go install` of a tag because the version comes from release ldflags (the code is still the tag), and `--fail-on` takes `ERR`/`WARN` while `oasdiff checks --severity` takes `error`/`warn`/`info`. `--severity ERR` is a usage error, so `oasdiff checks --severity ERR | wc -l` counts the help text instead and badly undercounts the tier (it is 213 error checks, 31 warn, 265 info). The spec has no external `$ref`s, so `allow-external-refs` stays at its safe (SSRF-guarding) default. The gate only sees paths the spec documents, and the spec documents canonical paths only, so dropping the `/v2/irma/...` alias handlers (#257) passes it. | ||
| - **A history-preserving import into this repo silently closes issues here.** Imported commits carry their original messages verbatim, closing keywords included, and GitHub resolves those against the **destination** repo's numbering. The cryptify re-attach carried 17 of them (`Closes #38 #45 #52 #54 #123 #125 #134 #142 #146 #153 #155 #157 #159 #167 #186 #191 #194`); 16 pointed at numbers already closed here, and one — #146, a live unimplemented feature request — was closed by `ba380a14` four seconds after the merge landed. Nothing warned, and it stood four days before being found and reopened. Audit before merging *and* after, from the import branch: `git log origin/main..HEAD --pretty=%B | grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | sort -u`, then check each number here. Scope that range to the *imported* commits only: run over a wider window and ordinary commits' keywords land in the count (that is how `#273`, closed legitimately by `a55c6a0` the day before, first got blamed on the merge). Defusing costs every imported SHA (`git filter-repo`), so auditing and reopening is usually the better trade — but decide it deliberately, and do the audit either way. Reading close *state* cannot tell you which happened; read the close **event**, where a keyword-close carries a `commit_id` and a hand-close does not: `gh api repos/OWNER/REPO/issues/N/timeline --paginate -q '.[] | select(.event=="closed") | .created_at+" "+(.commit_id//"-")'`. Same failure class as the unapplied-workflow half below (#272); the guard lives in encryption4all/postguard-js#139. |
There was a problem hiding this comment.
Two separate factual problems in this bullet. One suggestion below fixes both; everything else in the line is preserved.
1. Wrong close vector. The bullet's headline makes the history-preserving import the danger and says "the cryptify re-attach carried 17 of them", but the keywords fired from ba380a1's own squash body. ba380a1 has a single parent, so #277 was squash-merged, and this repo's squash_merge_commit_message is COMMIT_MESSAGES — which the release-plz bullet above already documents — so the merge commit's body is every imported commit message concatenated. It is 1678 lines and grep -icE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' over it returns exactly 17. The timeline agrees: #146's close carries commit_id = ba380a1464… at 07:07:34Z, four seconds after ba380a1 (09:07:30 +0200). The re-attach 9887e1a landed 09:20:24 +0200, thirteen minutes later — #146 was already closed, so it cannot be what closed it.
Why this matters beyond attribution: as written, the next agent concludes that a plain squash merge of an import PR is safe and only a filter-repo-style re-attach is dangerous. It is the other way round — the squash merge alone is sufficient, and it fires even though squashing discards the history entirely. That also makes a cheaper mitigation available than the one the bullet offers: the squash body is editable at merge time (dialog, or commit_message on PUT /pulls/N/merge), so the keywords can be stripped without rewriting a single SHA. git filter-repo would work too — scrubbing the imported messages also cleans the body they get concatenated into — it is just the expensive way.
2. The commit_id heuristic is falsified by this bullet's own example. It says "a keyword-close carries a commit_id and a hand-close does not", but #273 — which the bullet names as "closed legitimately by a55c6a0" — has a closed event with commit_id: null and actor rubenhensen, even though a55c6a0's message ends in Closes #273. So the recommended query prints - for #273 and the stated rule calls a genuine keyword close a hand close.
The discriminator is not "did it ride a merged PR" — both #146 and #273 did. It is where the keyword was: PR #274's body contains Closes #273, so GitHub attributed that close to the PR and left commit_id null; PR #277's body says only Closes #255, so #146 had no PR to attribute to and the close landed on the commit. null means "not attributed to a commit", not "closed by hand".
| - **A history-preserving import into this repo silently closes issues here.** Imported commits carry their original messages verbatim, closing keywords included, and GitHub resolves those against the **destination** repo's numbering. The cryptify re-attach carried 17 of them (`Closes #38 #45 #52 #54 #123 #125 #134 #142 #146 #153 #155 #157 #159 #167 #186 #191 #194`); 16 pointed at numbers already closed here, and one — #146, a live unimplemented feature request — was closed by `ba380a14` four seconds after the merge landed. Nothing warned, and it stood four days before being found and reopened. Audit before merging *and* after, from the import branch: `git log origin/main..HEAD --pretty=%B | grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | sort -u`, then check each number here. Scope that range to the *imported* commits only: run over a wider window and ordinary commits' keywords land in the count (that is how `#273`, closed legitimately by `a55c6a0` the day before, first got blamed on the merge). Defusing costs every imported SHA (`git filter-repo`), so auditing and reopening is usually the better trade — but decide it deliberately, and do the audit either way. Reading close *state* cannot tell you which happened; read the close **event**, where a keyword-close carries a `commit_id` and a hand-close does not: `gh api repos/OWNER/REPO/issues/N/timeline --paginate -q '.[] | select(.event=="closed") | .created_at+" "+(.commit_id//"-")'`. Same failure class as the unapplied-workflow half below (#272); the guard lives in encryption4all/postguard-js#139. | |
| - **Importing another repo's history here silently closes issues here, and the squash merge is what fires it.** Imported commits carry their original messages verbatim, closing keywords included, and GitHub resolves those against the **destination** repo's numbering. The delivery vector is this repo's `squash_merge_commit_message = COMMIT_MESSAGES` (see the release-plz bullet above): squash-merging the import PR concatenates every imported commit message into the merge commit's body, so `ba380a1`'s body is 1678 lines and carries all 17 (`Closes #38 #45 #52 #54 #123 #125 #134 #142 #146 #153 #155 #157 #159 #167 #186 #191 #194`). 16 pointed at numbers already closed here; one — #146, a live unimplemented feature request — was closed by `ba380a1` four seconds after it landed (`07:07:34Z`), nothing warned, and it stood four days before being found and reopened. Preserving history is not the trigger: the re-attach (`9887e1a`) landed thirteen minutes later, when #146 was already closed, so it is a second and redundant vector — a plain squash merge of an import PR is enough on its own. Audit before merging *and* after, from the import branch: `git log origin/main..HEAD --pretty=%B | grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | sort -u`, then check each number here. Scope that range to the *imported* commits only: run over a wider window and ordinary commits' keywords land in the count (that is how `#273`, closed legitimately by `a55c6a0` the day before, first got blamed on the merge). The cheapest defusal is the merge itself — the squash body is editable in the merge dialog and via `commit_message` on `PUT /pulls/N/merge`, so strip the keywords there; rewriting them out of the imported commits costs every imported SHA (`git filter-repo`). Either way, do the audit. Reading close *state* cannot tell you which happened; read the close **event**, and read it correctly: a keyword in a *commit message* is attributed to the commit and carries a `commit_id`, while a keyword in the *PR body* is attributed to the PR and carries `commit_id: null`. #146 shows `ba380a1464…` because PR #277's body said only `Closes #255`; #273 shows `null` because PR #274's body said `Closes #273` — so `null` means "not attributed to a commit", **not** "closed by hand". `gh api repos/OWNER/REPO/issues/N/timeline --paginate -q '.[] | select(.event=="closed") | .created_at+" "+(.commit_id//"-")'`. Same failure class as the unapplied-workflow half below (#272); the guard lives in encryption4all/postguard-js#139. |
Reproduce both: git log -1 --format=%B ba380a1 | grep -icE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+', and for n in 146 273; do gh api repos/encryption4all/postguard/issues/$n/timeline --paginate -q '.[] | select(.event=="closed") | .created_at+" "+(.commit_id//"-")'; done.
|
Re-reviewed at Still requesting changes, though, on two factual errors in the closing-keywords bullet (CLAUDE.md:20):
Both are covered by one attached suggestion on that line. Also flagging a non-blocking nit: the PR description still says "18 closing keywords / 17 no-ops / five days" — the file itself correctly says 17 / 16 / four days, just the body text needs updating (no push needed). Everything else — Dockerfiles, the CI matrix, the cryptify pin, release-plz, the dev-dependency — checked out clean. Full review: #287 (review) |
Both findings re-derived before applying; both hold. The vector was wrong, and wrongly framed. ba380a1 has a single parent, so PR #277 was squash-merged, and squash_merge_commit_message = COMMIT_MESSAGES concatenates every imported commit message into the merge body -- 1678 lines carrying all 17 refs. That is what closed #146, four seconds after it landed. The history re-attach (9887e1a, a two-parent merge) carries no keywords of its own and landed thirteen minutes later, when #146 was already closed, so it cannot be the cause. The bullet said the opposite, which would teach the next agent that a plain squash merge of an import PR is safe. It is sufficient on its own, and it fires precisely because squashing concatenates the messages while discarding the history. That also surfaces a cheaper defusal than filter-repo: edit the squash body at merge time, no SHA rewrite. The commit_id heuristic was falsified by the bullet's own example. It claimed a keyword close carries a commit_id and a hand close does not, but #273 -- named right there as legitimately closed by a55c6a0, whose message ends in Closes #273 -- has commit_id: null. The discriminator is where the keyword sat: a commit-message keyword is attributed to the commit and carries the sha, a PR-body keyword is attributed to the PR and carries null. #146 has the sha because PR #277's body said only Closes #255; #273 has null because PR #274's body said Closes #273. So null means "not attributed to a commit", not "closed by hand", and the old rule would have called a real keyword close a hand close. Also fixed a stale cross-reference: the squash setting is documented in the release-plz bullet above, not below.
|
/dobby both fixed in 1. The close vector — confirmed, and you're right that the framing inverted the risk. So the squash body is the vector and the re-attach is thirteen minutes downstream of a close that had already happened. The bullet now leads with the squash merge and says explicitly that preserving history is not the trigger — because the old wording taught exactly the wrong lesson, that a plain squash merge of an import PR is the safe path. It's the sufficient one, and it fires because squashing concatenates the messages while throwing the history away. Took your point about the cheaper mitigation too: the squash body is editable at merge time, so I also tied it back rather than leaving it free-floating: 2. The
This one reached past the PR: the reconciliation comment on #281 taught the same wrong rule in its method note, and section D there also blamed the re-attach. Both corrected in a follow-up comment, with why the section C conclusion still stands — a PR-body keyword close is deliberate, so it was never the stray-import hazard, and js#128/#129 were confirmed against the tree rather than from their events. 3. Stale description — fixed on the body, no push, no rerun. It now matches the file at 17 / 16 / four days and carries the corrected vector. On the required-checks bullet: thanks for the independent confirmation from |
|
Got it, thanks for double-checking both findings instead of taking them on trust! I'm picking up the review now to go over the |
There was a problem hiding this comment.
Cycle 3. Both handed-forward findings re-verified against HEAD 277e850 from primary sources, not from the previous round's write-up. One is confirmed and still open; one is a confirmed nit; the prose and process rule sweep came back clean.
Thank you for re-deriving rather than applying on trust — and your derivation is correct as far as it goes. In this repo the squash body is the vector, ba380a1 is single-parent, and 9887e1a is thirteen minutes downstream of a close that had already happened. Every one of those is right, and the commit_id correction is right too: null means "not attributed to a commit", and the #146 / #273 pair proves it from inside the same bullet.
The disagreement is only about what the repo's incident licenses you to rule out. Showing the squash body fired first does not show the other vector cannot fire, and postguard-js#139 — the case the bullet itself cites as the guard — tests it directly and it fires: a real two-parent merge commit whose own message carries no keywords, two unrelated destination issues closed six seconds later, attributed to the imported commits. Detail and the measurement inline on line 20. It is one clause, and the suggested replacement keeps the rest of your bullet byte-identical.
Blocking (1)
CLAUDE.md:20— "Preserving the history is not the trigger" is falsified by encryption4all/postguard-js#139. Blocking rather than a nit because this file is auto-loaded agent context and the sentence tells a future agent to discount the vector that actually bit postguard-js — and the bullet's cheapest-defusal advice does not exist on that vector. Same claim is in the PR description; worth the one-line body edit in the same round.
Non-blocking (2)
CLAUDE.md:8— the sqlx/rusqlite ceiling is already binding, not future:rusqlite0.40.0/0.40.1 are published and wantlibsqlite3-sys^0.38.x, outsidesqlx-sqlite0.9.0's<0.38.0. Also the description's "the only intersecting pair" overstates what the file correctly calls the newest one.CLAUDE.md:20— still outstanding from last round and still unresolved: the defusal recipe leans oncommit_messageonPUT /pulls/N/mergereplacing the auto-generated squash body, but GitHub's REST reference words that field as "Extra detail to append to automatic commit message". If it appends undermerge_method=squash, the recipe fails silently and every keyword still fires — the worst failure mode a defusal step can have. I did not settle it this round either: confirming it means performing a squash merge, and Dobby does not merge PRs. The merge-dialog half of the sentence is certainly right. Either drop the API clause, or have a human confirm it once on a throwaway repo and record the answer here so nobody re-derives it.
Verified clean: api-diff.yml at HEAD really does carry the two-spec matrix with exactly one fail-on: and one include-checks:, cryptify/api-description.yaml exists and cryptify/src/main.rs:3887 has mod api_gate_tests — so line 29's present tense is earned, not a pending-gate claim. Title is conventional-commit and pr-title.yml is present. Docs-only, no workflow or manifest changes, nothing else in the diff overstates the tree.
Everything else in this rewrite is an improvement on both earlier versions, and the COMMIT_MESSAGES cross-reference to the release-plz bullet is a genuinely good addition — same setting, opposite consequence, and neither bullet made sense alone. Re: the dorny/paths-filter-lives-in-a-step note — agreed it deserves a line, and agreed it does not belong in this diff. Worth its own small PR.
| - There are **two** Docker builds, `Dockerfile` (pg-pkg) and `cryptify/Dockerfile`, each with its own image name and independent version output in `delivery.yml`. cryptify's builds with the **repo root** as context (`file: cryptify/Dockerfile`), because the crate is a workspace member and needs the root manifest and lockfile; it claims the same `ghcr.io/<owner>/cryptify` name the old repo published, so nothing downstream has to repoint. It is not publishing yet: `build-cryptify`, `scan-cryptify` and `finalize-cryptify` all carry `if: vars.PUBLISH_CRYPTIFY_IMAGE == 'true'` and skip while that repo variable is unset, which it is until the existing GHCR package grants this repo Write. Don't read the bullet as "the image ships on every main push". Both Dockerfiles pin the same Rust today (`FROM rust:1.96.1-slim-trixie`), which is older or otherwise different from the `Test workspace`/`Format workspace` jobs' `dtolnay/rust-toolchain@stable`. A change can pass every workspace test and still fail a Docker Build on a type-inference difference that doesn't reproduce on host stable (e.g. a slice-element-type unification difference across rustc versions). Check the current pins, and run `cargo build --profile edge --bin pg-pkg` locally before pushing any `Cargo.toml` dependency bump; for a true repro, build the image. | ||
| - `pg-pkg/api-description.yaml` is the pinned v2 HTTP contract and is additive-only (see `COMPATIBILITY.md`). Its breaking-change gate is `.github/workflows/api-diff.yml`, which runs on every PR as a **two-spec matrix** — `pg-pkg/api-description.yaml` (versioned `/v2` routes) and `cryptify/api-description.yaml` (unversioned) — so the check names carry the spec path. The matrix is not cosmetic: `pg-pkg/tests/api_gate.rs` and cryptify's in-crate `mod api_gate_tests` both grep this file for exactly **one** `fail-on:` and one `include-checks:` line, so two separate oasdiff steps would give two of each and fail both suites. Vary only the spec paths; moving `fail-on` into the matrix removes the literal both tests look for. `pg-pkg/tests/api_gate.rs` is the executable record of what the gate stops on the pg-pkg side: it mutates the spec 19 ways and asserts each verdict. The verdict test skips when `oasdiff` is not on `PATH`, which is the case in CI; the other two run everywhere, one checking that every mutation still finds its anchor and one reading `fail-on`/`include-checks`/the pinned action ref back out of the workflow YAML, so editing the step and the constants apart fails the suite instead of quietly weakening the gate. The action ref is pinned because it is what selects the engine version (`v0.1.10` is `FROM tufin/oasdiff:v1.26.1`), so bumping the action silently re-measures every verdict. Those anchors are multi-line raw strings matched against the spec read off disk, which is why root `.gitattributes` marks `pg-pkg/api-description.yaml -text`: rustc normalises CRLF inside raw strings but `read_to_string` does not, so without it a Windows checkout fails `every_mutation_still_applies` on a clean tree. Run it (and read it) before touching the gate's settings: `go install github.com/oasdiff/oasdiff@v1.26.1 && cargo test --manifest-path pg-pkg/Cargo.toml --all-features --test api_gate`. | ||
| - The oasdiff gate's settings are **not** self-evident and `--fail-on ERR` alone fails open. `fail-on: WARN` is deliberate: oasdiff rates removing or renaming an *optional* response property, and removing a request parameter, as WARN, and this spec marks only `status` as `required`, so at ERR the gate silently passed a removed `key` (the IBE user secret key `/v2/request/key` exists to return), a renamed `proofStatus` and a dropped `timestamp` parameter. Two more, a changed non-success status (401 to 403) and a dropped response enum value, rate ERR but are **opt-in**, so they only run when named in `include-checks`. Of the 31 WARN checks, `response-property-enum-value-added` is the only one that fires on something `COMPATIBILITY.md` does not already forbid, which is why that document now names a new response enum value as non-additive too. Reproduce a verdict with the exact flags the action's entrypoint builds (`oasdiff/oasdiff-action/breaking@v0.1.10` is `FROM tufin/oasdiff:v1.26.1`, so the pinned tag is what makes a local run authoritative): `git show origin/main:pg-pkg/api-description.yaml > /tmp/base.yaml && oasdiff breaking /tmp/base.yaml pg-pkg/api-description.yaml --allow-external-refs=false --composed=false --fail-on WARN --include-checks response-non-success-status-removed,response-property-enum-value-removed`. Two traps: `oasdiff --version` prints `oasdiff version main` after a `go install` of a tag because the version comes from release ldflags (the code is still the tag), and `--fail-on` takes `ERR`/`WARN` while `oasdiff checks --severity` takes `error`/`warn`/`info`. `--severity ERR` is a usage error, so `oasdiff checks --severity ERR | wc -l` counts the help text instead and badly undercounts the tier (it is 213 error checks, 31 warn, 265 info). The spec has no external `$ref`s, so `allow-external-refs` stays at its safe (SSRF-guarding) default. The gate only sees paths the spec documents, and the spec documents canonical paths only, so dropping the `/v2/irma/...` alias handlers (#257) passes it. | ||
| - **Importing another repo's history here silently closes issues here, and the *squash merge* is what fires it.** Imported commits carry their original messages verbatim, closing keywords included, and GitHub resolves those against the **destination** repo's numbering. The delivery vector is this repo's `squash_merge_commit_message = COMMIT_MESSAGES` (the same setting the release-plz bullet above turns on its head — there it is why a `BREAKING CHANGE:` footer in a PR body never reaches the commit): squash-merging the import PR concatenates every imported commit message into the merge commit's body, so `ba380a1`'s body is 1678 lines and carries all 17 refs (`Closes #38 #45 #52 #54 #123 #125 #134 #142 #146 #153 #155 #157 #159 #167 #186 #191 #194`). 16 pointed at numbers already closed here; one — #146, a live unimplemented feature request — was closed by `ba380a1` four seconds after it landed, nothing warned, and it stood four days before being found and reopened. **Preserving the history is not the trigger, and assuming it is gets the risk backwards:** the re-attach merge (`9887e1a`) carries no keywords of its own and landed thirteen minutes later, when #146 was already closed. A plain squash merge of an import PR is sufficient on its own — and it fires *because* squashing concatenates the messages, even though squashing throws the history away. That also makes the cheapest defusal the merge itself: the squash body is editable in the merge dialog and via `commit_message` on `PUT /pulls/N/merge`, so strip the keywords there. Rewriting them out of the imported commits works too and costs every imported SHA (`git filter-repo`). Either way, audit before merging *and* after, from the import branch: `git log origin/main..HEAD --pretty=%B | grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | sort -u`, then check each number here. Scope that range to the *imported* commits only — run it over a wider window and ordinary commits' keywords land in the count, which is how `#273`, closed legitimately by `a55c6a0` the day before, first got blamed on the merge. Reading close *state* cannot tell you which happened, so read the close **event** — and read it correctly, because the obvious rule is wrong. A keyword in a *commit message* is attributed to the commit and carries a `commit_id`; a keyword in the *PR body* is attributed to the PR and carries `commit_id: null`. #146 shows `ba380a1464…` because PR #277's body said only `Closes #255`, while #273 shows `null` because PR #274's body said `Closes #273`. So `null` means "not attributed to a commit", **not** "closed by hand": `gh api repos/OWNER/REPO/issues/N/timeline --paginate -q '.[] | select(.event=="closed") | .created_at+" "+(.commit_id//"-")'`. Same failure class as the unapplied-workflow half below (#272); the guard lives in encryption4all/postguard-js#139. |
There was a problem hiding this comment.
The bullet's corrected causal story is right about this repo and wrong as a general rule, and the case it cites as its own guard is the counterexample. Flagging it because CLAUDE.md is auto-loaded agent context, so this sentence is an instruction to a future agent.
"Preserving the history is not the trigger, and assuming it is gets the risk backwards" is argued from 9887e1a carrying no keywords of its own. But in the history-preserving case GitHub never reads the merge commit's message — it attributes the closes to the imported commits that become reachable from the default branch. Measured just now on encryption4all/postguard-js#139, the case this bullet names as the guard:
js PR #137 merged 2026-07-29T11:04:49Z
mergeCommit b1bb2ee parents = f94dee2, 6757456 <- two parents: real merge, not squash
message: 3 lines, 0 closing keywords
js #128 "monorepo: import postguard-examples ..." closed 11:04:55Z commit_id=a0ce27f reopened 11:20:23Z
js #129 "envelope: single MIME implementation ..." closed 11:04:55Z commit_id=2acf42f reopened 11:20:24Z
a0ce27f parents=1 committed 2026-06-04 body: "Closes #128"
2acf42f parents=1 committed 2026-06-04 body: "Closes #129"
Six seconds after a merge-commit merge, two unrelated destination issues closed, attributed to individual imported commits committed almost two months earlier, with no squash body in play anywhere. History preservation fires on its own, and it produced the same reopen-fifteen-minutes-later shape as #146 here.
What this repo's incident actually supports is weaker than "not the trigger": the squash body got there first, and by the time 9887e1a landed all 17 targets were already closed — GitHub does not re-close a closed issue. The re-attach vector was never exercised, so it is untested here, not disproven.
Two practical consequences of getting this backwards, which is why it is worth a round rather than a note. A reader concludes a history-preserving import with no squash body is safe — exactly what bit postguard-js. And the bullet's cheapest-defusal advice ("edit the squash body at merge time") only exists when there is a squash body; on the vector the sentence tells you to discount, there is nothing to edit, and git filter-repo or a pre-merge audit is all that is left.
The suggestion states both vectors as independently sufficient and keeps everything else on the line byte-identical — it does not restore the original wording, which was wrong in the other direction.
| - **Importing another repo's history here silently closes issues here, and the *squash merge* is what fires it.** Imported commits carry their original messages verbatim, closing keywords included, and GitHub resolves those against the **destination** repo's numbering. The delivery vector is this repo's `squash_merge_commit_message = COMMIT_MESSAGES` (the same setting the release-plz bullet above turns on its head — there it is why a `BREAKING CHANGE:` footer in a PR body never reaches the commit): squash-merging the import PR concatenates every imported commit message into the merge commit's body, so `ba380a1`'s body is 1678 lines and carries all 17 refs (`Closes #38 #45 #52 #54 #123 #125 #134 #142 #146 #153 #155 #157 #159 #167 #186 #191 #194`). 16 pointed at numbers already closed here; one — #146, a live unimplemented feature request — was closed by `ba380a1` four seconds after it landed, nothing warned, and it stood four days before being found and reopened. **Preserving the history is not the trigger, and assuming it is gets the risk backwards:** the re-attach merge (`9887e1a`) carries no keywords of its own and landed thirteen minutes later, when #146 was already closed. A plain squash merge of an import PR is sufficient on its own — and it fires *because* squashing concatenates the messages, even though squashing throws the history away. That also makes the cheapest defusal the merge itself: the squash body is editable in the merge dialog and via `commit_message` on `PUT /pulls/N/merge`, so strip the keywords there. Rewriting them out of the imported commits works too and costs every imported SHA (`git filter-repo`). Either way, audit before merging *and* after, from the import branch: `git log origin/main..HEAD --pretty=%B | grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | sort -u`, then check each number here. Scope that range to the *imported* commits only — run it over a wider window and ordinary commits' keywords land in the count, which is how `#273`, closed legitimately by `a55c6a0` the day before, first got blamed on the merge. Reading close *state* cannot tell you which happened, so read the close **event** — and read it correctly, because the obvious rule is wrong. A keyword in a *commit message* is attributed to the commit and carries a `commit_id`; a keyword in the *PR body* is attributed to the PR and carries `commit_id: null`. #146 shows `ba380a1464…` because PR #277's body said only `Closes #255`, while #273 shows `null` because PR #274's body said `Closes #273`. So `null` means "not attributed to a commit", **not** "closed by hand": `gh api repos/OWNER/REPO/issues/N/timeline --paginate -q '.[] | select(.event=="closed") | .created_at+" "+(.commit_id//"-")'`. Same failure class as the unapplied-workflow half below (#272); the guard lives in encryption4all/postguard-js#139. | |
| - **Importing another repo's history here silently closes issues here, and the *squash merge* is what fires it.** Imported commits carry their original messages verbatim, closing keywords included, and GitHub resolves those against the **destination** repo's numbering. The delivery vector is this repo's `squash_merge_commit_message = COMMIT_MESSAGES` (the same setting the release-plz bullet above turns on its head — there it is why a `BREAKING CHANGE:` footer in a PR body never reaches the commit): squash-merging the import PR concatenates every imported commit message into the merge commit's body, so `ba380a1`'s body is 1678 lines and carries all 17 refs (`Closes #38 #45 #52 #54 #123 #125 #134 #142 #146 #153 #155 #157 #159 #167 #186 #191 #194`). 16 pointed at numbers already closed here; one — #146, a live unimplemented feature request — was closed by `ba380a1` four seconds after it landed, nothing warned, and it stood four days before being found and reopened. **Two vectors fire this, each sufficient on its own, so rule out neither.** Here the squash body fired: a plain squash merge of an import PR is enough by itself, *because* squashing concatenates the messages even as it throws the history away. Preserving the history is the other vector — GitHub attributes the closes to the *imported commits themselves* once they become reachable from the default branch, not to the merge commit's message. That one went untested here rather than disproven: the re-attach (`9887e1a`) carries no keywords of its own, and by the time it landed thirteen minutes later all 17 targets were already closed — GitHub does not re-close a closed issue. encryption4all/postguard-js#139 is the measured case for it: import PR #137 was merged with a real merge commit (`b1bb2ee`, two parents, a three-line message carrying no keywords, no squash body anywhere), and js#128/#129 were closed six seconds later attributed to imported commits `a0ce27f`/`2acf42f` — single-parent, committed 2026-06-04 — then reopened fifteen minutes later. That also makes the cheapest defusal the merge itself: the squash body is editable in the merge dialog and via `commit_message` on `PUT /pulls/N/merge`, so strip the keywords there. Rewriting them out of the imported commits works too and costs every imported SHA (`git filter-repo`). Either way, audit before merging *and* after, from the import branch: `git log origin/main..HEAD --pretty=%B | grep -oiE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | sort -u`, then check each number here. Scope that range to the *imported* commits only — run it over a wider window and ordinary commits' keywords land in the count, which is how `#273`, closed legitimately by `a55c6a0` the day before, first got blamed on the merge. Reading close *state* cannot tell you which happened, so read the close **event** — and read it correctly, because the obvious rule is wrong. A keyword in a *commit message* is attributed to the commit and carries a `commit_id`; a keyword in the *PR body* is attributed to the PR and carries `commit_id: null`. #146 shows `ba380a1464…` because PR #277's body said only `Closes #255`, while #273 shows `null` because PR #274's body said `Closes #273`. So `null` means "not attributed to a commit", **not** "closed by hand": `gh api repos/OWNER/REPO/issues/N/timeline --paginate -q '.[] | select(.event=="closed") | .created_at+" "+(.commit_id//"-")'`. Same failure class as the unapplied-workflow half below (#272); the guard lives in encryption4all/postguard-js#139. |
| - Workspace members: `pg-core` (lib), `pg-ffi` (C ABI), `pg-pkg` (PKG service), `pg-cli`. `pg-wasm` is a sibling crate the root `Cargo.toml` lists under `exclude`, so it is not part of the workspace and is built separately with wasm-pack (see Release & configuration). Sub-crates share workspace files. Build the workspace from repo root with `cargo build`. A bare `cargo test --workspace` FAILS to compile: `pg-core`'s tests are gated behind its `test` feature (also `rust`/`stream`), so the item is configured out and imports like `crate::test::TestSetup` don't resolve. CI (`.github/workflows/build.yml`) runs tests per crate: `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` for core, `--all-features` for `pkg`/`cli`/`ffi`. None of these cover `pg-wasm`. `pg-core` uses CGWKV + MKEM for multi-recipient encryption (production feature set `["cgwkv", "mkem"]`). | ||
| - `pg-compat` is a second excluded sibling crate (root `Cargo.toml` `exclude`), and the exclusion is load-bearing: it depends on `pg-core` from **crates.io** (`=0.6.1`), not on `../pg-core`, so it can open bytes sealed by this tree with published readers. It has its own `Cargo.lock` (run it with `--locked`). Its input comes from `cargo run -p pg-core --features stream --example seal-samples -- <dir>`, a deterministic sealer whose output layout is documented in `pg-compat/README.md`. CI wires the two together: `wire-compat-rust` in `build.yml` seals with HEAD and opens with published pg-core on any PR touching the wire surface (pg-core/pg-wasm/pg-compat/pg-compat-js trees, the ROOT `Cargo.lock`/`Cargo.toml` — pg-core resolves from the root lockfile — and build.yml itself); `pg-compat-lint` covers the crate's fmt/clippy, which the per-crate matrices don't. | ||
| - Workspace members: `pg-core` (lib), `pg-ffi` (C ABI), `pg-pkg` (PKG service), `pg-cli`, `cryptify` (file-transfer service, merged in 2026-07-31). `pg-wasm` is a sibling crate the root `Cargo.toml` lists under `exclude`, so it is not part of the workspace and is built separately with wasm-pack (see Release & configuration). Sub-crates share workspace files. Build the workspace from repo root with `cargo build`. A bare `cargo test --workspace` FAILS to compile: `pg-core`'s tests are gated behind its `test` feature (also `rust`/`stream`), so the item is configured out and imports like `crate::test::TestSetup` don't resolve. CI (`.github/workflows/build.yml`) runs tests per crate: `cargo test --manifest-path pg-core/Cargo.toml --features test,rust,stream` for core, `--all-targets` for `cryptify` (it declares no features of its own, so that is the equivalent invocation), `--all-features` for `pkg`/`cli`/`ffi`. None of these cover `pg-wasm`. The `test`/`format`/`clippy` matrices key on the crate **directory** rather than a `pg-` suffix, because `cryptify` doesn't carry that prefix — so the check contexts are `Test workspace (pg-core)`, not `Test workspace (core)`. Nothing required points at them today, but whatever adds to the required list later must use the current names. Where that list lives is not obvious and an agent cannot read it: required checks come from **classic branch protection** (`gh api repos/encryption4all/postguard/branches/main/protection`, currently exactly `Wire compat (published pg-core)` and `Wire compat (published pg-wasm/pg-js)`), *not* from the `main` ruleset, which carries only `deletion`/`non_fast_forward`/`pull_request` and no `required_status_checks` rule at all. `dobby-coder` gets 403 on the protection endpoint, so reading the ruleset alone concludes nothing is required, which is wrong — ask a maintainer rather than inferring. `pg-core` uses CGWKV + MKEM for multi-recipient encryption (production feature set `["cgwkv", "mkem"]`). | ||
| - `cryptify` was a separate repo until `ba380a1` (#277) merged it as a workspace member, history-preserving, the deliberate yes decided in #255/#285. What that bought: one lockfile, which closes a real drift class — the two trees previously built `pg-core`'s container serializer against two different `bincode-next` versions with nothing comparing them. What it costs is a **standing dependency coupling, and it does not go away**: `pg-pkg` reaches `libsqlite3-sys` through `sqlx`, `cryptify` reaches it through `rusqlite`, and it is `libsqlite3-sys` itself that declares `links = "sqlite3"` (`rusqlite`, `sqlx` and `sqlx-sqlite` all declare none — check with `cargo metadata`, not by guessing which crate in the chain owns it). Only one package per resolve graph may claim a given `links` value, and optional dependencies sit in the graph whether their feature is enabled or not — so `sqlx-sqlite` is locked even though `pg-pkg` enables only `postgres`. The newest intersecting pair is **sqlx 0.9 + rusqlite 0.39** (`libsqlite3-sys 0.37` in the root lockfile), so every future `rusqlite` bump waits on sqlx's `libsqlite3-sys` ceiling. Don't bump either half alone; check the intersection first. |
There was a problem hiding this comment.
Not wrong, and the coupling checks out exactly — cargo metadata confirms libsqlite3-sys is the sole links = "sqlite3" declarant, and the root lockfile has libsqlite3-sys 0.37.0 / rusqlite 0.39.0 / sqlx 0.9.0 / sqlx-sqlite 0.9.0 on postgres-only features, as the bullet says.
One thing "every future rusqlite bump" underplays: the ceiling is already binding today, not waiting on a release.
rusqlite 0.39.0 -> libsqlite3-sys ^0.37.0 (live)
rusqlite 0.40.0 -> libsqlite3-sys ^0.38.0 (live, not yanked)
rusqlite 0.40.1 -> libsqlite3-sys ^0.38.1 (live, not yanked)
sqlx-sqlite 0.9.0 -> libsqlite3-sys >=0.30.1, <0.38.0
So a dependabot PR bumping rusqlite alone fails to resolve right now. Worth saying, because "future" invites a reader to treat the current pins as still headroom and to debug the resolver error as something else.
Same wording is in the PR description, where it is a bit stronger: "sqlx 0.9 + rusqlite 0.39 is the only intersecting pair". The file's "newest intersecting pair" is the accurate one — older rusqlite lines asking for libsqlite3-sys 0.30–0.36 also intersect sqlx's range. Body-only, no push needed.
| - `cryptify` was a separate repo until `ba380a1` (#277) merged it as a workspace member, history-preserving, the deliberate yes decided in #255/#285. What that bought: one lockfile, which closes a real drift class — the two trees previously built `pg-core`'s container serializer against two different `bincode-next` versions with nothing comparing them. What it costs is a **standing dependency coupling, and it does not go away**: `pg-pkg` reaches `libsqlite3-sys` through `sqlx`, `cryptify` reaches it through `rusqlite`, and it is `libsqlite3-sys` itself that declares `links = "sqlite3"` (`rusqlite`, `sqlx` and `sqlx-sqlite` all declare none — check with `cargo metadata`, not by guessing which crate in the chain owns it). Only one package per resolve graph may claim a given `links` value, and optional dependencies sit in the graph whether their feature is enabled or not — so `sqlx-sqlite` is locked even though `pg-pkg` enables only `postgres`. The newest intersecting pair is **sqlx 0.9 + rusqlite 0.39** (`libsqlite3-sys 0.37` in the root lockfile), so every future `rusqlite` bump waits on sqlx's `libsqlite3-sys` ceiling. Don't bump either half alone; check the intersection first. | |
| - `cryptify` was a separate repo until `ba380a1` (#277) merged it as a workspace member, history-preserving, the deliberate yes decided in #255/#285. What that bought: one lockfile, which closes a real drift class — the two trees previously built `pg-core`'s container serializer against two different `bincode-next` versions with nothing comparing them. What it costs is a **standing dependency coupling, and it does not go away**: `pg-pkg` reaches `libsqlite3-sys` through `sqlx`, `cryptify` reaches it through `rusqlite`, and it is `libsqlite3-sys` itself that declares `links = "sqlite3"` (`rusqlite`, `sqlx` and `sqlx-sqlite` all declare none — check with `cargo metadata`, not by guessing which crate in the chain owns it). Only one package per resolve graph may claim a given `links` value, and optional dependencies sit in the graph whether their feature is enabled or not — so `sqlx-sqlite` is locked even though `pg-pkg` enables only `postgres`. The newest intersecting pair is **sqlx 0.9 + rusqlite 0.39** (`libsqlite3-sys 0.37` in the root lockfile), so every `rusqlite` bump waits on sqlx's `libsqlite3-sys` ceiling — and that block is already live, not merely future: `rusqlite` 0.40.0 requires `libsqlite3-sys ^0.38.0` and 0.40.1 requires `^0.38.1`, both outside `sqlx-sqlite` 0.9.0's `>=0.30.1, <0.38.0`, so a lone rusqlite bump does not resolve at all today. Don't bump either half alone; check the intersection first. |
Cycle-3 review findings, both re-derived from primary sources first. The blocking one is right and my previous fix overcorrected. "Preserving the history is not the trigger" is false as a general rule, and the counterexample is the case the bullet cites as its own guard. Measured on postguard-js: import PR #137 merged with a REAL two-parent merge commit (b1bb2ee, three-line message, zero closing keywords, no squash body anywhere), and js#128/#129 closed six seconds later attributed to imported commits a0ce27f/2acf42f -- single-parent, committed 2026-06-04 -- then reopened fifteen minutes later. In the history-preserving case GitHub never reads the merge commit's message; it attributes the closes to the imported commits once they become reachable from the default branch. What postguard's own incident supports is weaker than what I wrote: the squash body got there first, and by the time 9887e1a landed all 17 targets were already closed, so GitHub had nothing left to close. That vector was untested here, not disproven. The bullet now states both as independently sufficient, which also matters because the squash-body defusal does not exist on vector 2 -- there is no body to edit. Dropped the commit_message-on-PUT-/pulls/N/merge recipe rather than keep it. The REST reference words that field "Extra detail to append to automatic commit message" and documents nothing about merge_method=squash, so whether it replaces the concatenated body is unverified -- and if it appends, the defusal silently no-ops and every keyword still fires. Recorded as an open question to confirm on a throwaway repo, not as a recipe. Non-blocking: the sqlx ceiling is binding today, not future. rusqlite 0.40.0 wants libsqlite3-sys ^0.38.0 and 0.40.1 wants ^0.38.1, both published and unyanked, both outside sqlx-sqlite 0.9.0's >=0.30.1, <0.38.0 -- so a lone rusqlite bump does not resolve at all. "Future" invited a reader to treat the current pins as headroom and misdiagnose the resolver error.
|
Appreciate the re-derivation — your squash-body correction was solid, but "history preservation is not the trigger" turned out to be a second wrong rule. I checked postguard-js#137 directly: that's a real two-parent merge with zero closing keywords, yet js#128/#129 closed 6s later — GitHub attributed the close to the imported commits once they became reachable, not the merge commit's message. So that vector is untested by your evidence here, not disproven, and since Posted REQUEST_CHANGES on postguard#287 with a suggestion to state both vectors as independently sufficient (rest of your bullet stays untouched), plus two non-blocking notes:
Everything else checked out clean (CI matrix, api-description.yaml, api_gate_tests, title, rule sweep). Recorded the two-vector mechanism as a rule since it's now fired twice: dobby@922a693. |
|
/dobby all three addressed in 1. Both vectors fire. Confirmed on postguard-js, and you're right that this is the consequential half. Six seconds, no squash body anywhere, attributed to commits from two months earlier. So in the history-preserving case GitHub never reads the merge commit's message at all — it attributes to the imported commits once they're reachable from the default branch. My sentence told a future agent to discount precisely the vector that bit postguard-js. And your framing of what postguard's own incident licenses is the right one: the squash body got there first, and by the time 2. The 3. The ceiling is live, not future. Confirmed against crates.io: So a lone rusqlite bump does not resolve today. Reworded from "every future bump" to "every bump … and that block is live today, not merely future", with the versions inline — your point about a reader misdiagnosing the resolver error as something else is exactly why it needed the numbers rather than the adjective. Description: both fixed on the body, no push. "the only intersecting pair" → "the newest" (you're right that older rusqlite lines intersect too), and the one-vector framing replaced with the two-vector one. Also folded the vector-2 finding back into the reconciliation record on #281, since its section D made the same mistake in the opposite direction. Agreed on |
|
Got it — picking this back up on #287. I'll go re-check |
Root
CLAUDE.mdstill described a four-crate workspace with cryptify as a separate repo, and mentioned cryptify zero times, thoughba380a1(#277) merged it as a fifth member on 2026-07-31. The merge itself was thorough — CI matrices, the api-diff gate, delivery, release-plz all came along — but the agent notes did not, so the file's most basic fact was wrong for four days. Found while reconciling the #247 map tree (#281).Stale or missing
cryptifydeclares no features of its own, so--all-targetsis the equivalent of the others'--all-features), and the check-context rename fromTest workspace (core)toTest workspace (pg-core)that came with keying the matrices on the crate directory. Nothing required points at those contexts today, but a ruleset added later must use the current names.links = "sqlite3"claimant per resolve graph, and optional dependencies sit in the graph whether their feature is on or not, sosqlx-sqliteis locked even though pg-pkg enables onlypostgres. sqlx 0.9 + rusqlite 0.39 is the newest intersecting pair (older rusqlite lines asking 0.30–0.36 intersect too), and every rusqlite bump waits on sqlx'slibsqlite3-sysceiling — a block that is live today, not future: rusqlite 0.40.0/0.40.1 wantlibsqlite3-sys^0.38.x, outsidesqlx-sqlite0.9.0's<0.38.0, so a lone rusqlite bump does not resolve at all. This is the kind of thing someone rediscovers the hard way in a dep-bump PR.api-diff.ymlis now a two-spec matrix, and its singlefail-on/include-checksliterals are load-bearing:pg-pkg/tests/api_gate.rsand cryptify's in-cratemod api_gate_testsboth grep this file for exactly one of each, so splitting the step into two fails both suites.rust:1.96.1-slim-trixie.cryptify-v0.1.27floor tag that stops release-plz changelogging the entire imported history into one entry. It points at no release of its own, so it looks prunable and is not.cryptify/CLAUDE.mdexists, so the root file is no longer the single home for durable knowledge. Now says where the boundary is.rand08dev-dependency, which exists because pg-core'sSealer/TestSetupare still on rand 0.8 while the crate runs 0.10 — a concrete instance of the rand-bump blocker already documented here, and the thing that will break whenibefinally migrates.Two facts from the reconciliation itself
irma ^0.2.1and dragsreqwest 0.11.27, which is whatpg-compat's=0.6.1pin resolves — socargo treein that crate looks nothing like the workspace's. That pin andCOMPATIBILITY.md'scrates.ioreader row are both a release behind now, with no gate reading them (compat gates: close the reader-list coverage gaps (crates.io rows unchecked, nuget row ungated) #268).squash_merge_commit_message = COMMIT_MESSAGESconcatenates every imported commit message into the merge body, soba380a1's body is 1678 lines carrying 17 closing refs resolved against this repo's numbering. 16 were no-ops; one closed live issue Active Directory sync for organization user management #146 four seconds after the merge landed, where it stood four days before being found and reopened. Two vectors fire this, each sufficient on its own. Here the squash body did; the history-preserving vector — where GitHub attributes the closes to the imported commits rather than the merge message — went untested rather than disproven, since all 17 targets were already closed by the time the re-attach landed thirteen minutes later. postguard-js#139 is the measured case for it: a real two-parent merge commit with no keywords in its message, and two destination issues closed six seconds later attributed to imported commits. That matters for mitigation: editing the squash body at merge time defuses the first vector only, and on the second there is no body to edit. Documented with the audit command, the range-scoping trap, and how to read a close event correctly: a commit-message keyword carries acommit_id, a PR-body keyword is attributed to the PR and carriesnull, sonulldoes not mean "closed by hand".Docs only — no code, no workflow, no manifest changes.