build: pin flatc and check flatbuffers binding drift in CI by sanity · Pull Request #110 · freenet/freenet-stdlib

sanity · 2026-09-01T14:11:04Z

Prep for #106. No schema, wire format, or hand-written code changes — this makes the next change (adding a request_id field) a small reviewable diff instead of an unreviewable one.

Problem

The flatbuffers bindings are checked in, but rust/build.rs shelled out to whatever flatc was on PATH and rewrote them in place on every build — and did nothing at all when there was no flatc to find. Both halves cause real problems.

Churn. Different flatc releases format their output differently, so a contributor whose compiler didn't match got thousands of lines of unrelated diff on every build, whether or not they'd touched a schema. Not hypothetical: commit 8070e93, "revert: undo accidental flatbuffers regen from #72", exists because it reached main.

The silent fallback is worse. CI doesn't install flatc, so CI never regenerated anything — it compiled the checked-in bindings and reported success. A schema edit that was never regenerated would pass CI while the bindings on disk still described the previous schema. Nothing anywhere compared the two.

The bindings had in fact drifted from every flatc release available: they matched neither 2.0.8, 24.3.25, nor 25.9.23 output, even after formatting.

Approach

Regeneration is explicit (FREENET_REGEN_FLATBUFFERS=1) and refuses to run under a flatc other than the pinned one.
A new CI job flatbuffers_drift regenerates with the pinned compiler and fails if the result differs from what's committed. That job, not the build, is what enforces the schema↔bindings relationship.
The job verifies the flatc version before comparing, because a silently-different compiler would make it compare the wrong thing and pass — the exact failure mode it exists to prevent.

Why 24.3.25

Chosen on evidence, not preference:

It matches the flatbuffers = "24.3" crate version already in rust/Cargo.toml.
Regenerating the TypeScript bindings with it is a no-op against what's committed — so it's already the de-facto compiler for that half.
The Rust bindings it produces pass the byte-frozen wire-format tests unchanged, which is the evidence that the wire format is untouched and only generated-code style moves.

Commits

0cbeca8 — the mechanism (build.rs + CI job). This is the commit with the argument in it.
45523cc — the regenerated output. Mechanical; doesn't need line-by-line reading.

Testing

Regeneration is idempotent — identical diff on a second run, so the drift check is stable.
A plain cargo build leaves a deliberately-reverted binding file reverted, confirming the guard works in both directions.
56 Rust tests pass, including all three byte-frozen wire-format pins; clippy clean; 78 TypeScript tests pass.

Note for reviewers

There is a related gap this PR does not close: the request side has three byte-frozen wire-format pin tests, but the response side has none — nothing currently fails CI if HostResponse's bincode layout moves. Given #106 is specifically about steering around that break class, it should get a pin. Kept separate to keep this PR mechanical.

Refs #106

[AI-assisted - Claude]

…ift in CI The flatbuffers bindings are checked in, but `build.rs` shelled out to whatever `flatc` was on PATH and rewrote them in place on every build -- and did nothing at all when there was no flatc to find. Both halves caused real problems. Different flatc releases format their output differently, so anyone whose compiler did not match the one the bindings came from got thousands of lines of unrelated churn mixed into their diff, on every build, whether or not they had touched a schema. Not hypothetical: commit 8070e93, "revert: undo accidental flatbuffers regen from #72", exists because it reached main. The silent fallback is the worse half. CI does not install flatc, so CI never regenerated anything -- it compiled the checked-in bindings and reported success. A schema edit that was never regenerated therefore passed CI while the bindings on disk still described the previous schema, and nothing anywhere compared the two. So regeneration is now explicit (FREENET_REGEN_FLATBUFFERS=1) and refuses to run under a flatc other than the pinned one, and a new CI job regenerates with that compiler and fails if the result differs from what is committed. That job, not the build, is what enforces the relationship between schemas and bindings. Pinned to 24.3.25 on evidence rather than preference: it matches the `flatbuffers` crate version already in Cargo.toml, regenerating the TypeScript bindings with it is a no-op against what is committed, and the Rust bindings it produces pass the byte-frozen wire-format tests unchanged -- so the wire format is untouched and only the generated code style moves. The version check in the CI job is deliberate: a silently-different compiler would make the whole job compare the wrong thing and pass, which is the failure mode it exists to prevent. Verified: regeneration is idempotent (identical diff on a second run), a plain build leaves a reverted binding file reverted, 56 Rust tests pass including all three wire-format pins, clippy is clean, and 78 TypeScript tests pass. Refs #106 [AI-assisted - Claude]

Mechanical. No schema changed and no hand-written code changed; this is the output of the regeneration path added in the previous commit, so that the committed bindings match what the pinned compiler produces and the new drift check passes on a clean tree. The bindings had drifted from every flatc release available: they matched neither 2.0.8, 24.3.25, nor 25.9.23 output, even after formatting. That is the accumulated residue of being rewritten by whichever compiler each contributor happened to have. The wire format does not change. The three byte-frozen wire-format tests in client_api::client_events pass unchanged, which is the evidence that this is a formatting-and-imports change rather than a protocol one. Reviewers: this diff is generated output and does not need reading line by line. The previous commit is the one with the argument in it. Refs #106 [AI-assisted - Claude]

…cked bindings as drift Both from external review of this PR. An explicitly requested regeneration that quietly regenerates nothing was the same silent no-op this change exists to remove. It was worse here than in the code it replaced: cargo caches a SUCCESSFUL build-script run keyed on the declared inputs, so warn-and-return would be recorded as success, and installing the right compiler and re-running the documented command could legitimately not execute the script again -- leaving the bindings stale while the developer believed they had regenerated. Every failure on the regeneration path now panics, which is not cached and cannot be missed. Verified: with flatc 2.0.8 first on PATH the build fails naming both versions and where to get the right one. The staleness check now reads `git status --porcelain` rather than `git diff --exit-code`, because a diff ignores untracked files. A new .fbs generates entirely NEW binding files, so the loudest possible drift -- a schema merging with no committed bindings at all -- was the one case that sailed past the check. The pinned flatc download also moves to RUNNER_TEMP so the archive cannot itself register as untracked drift. [AI-assisted - Claude]

…tc, harden the drift job Second review round. The first finding is a regression this PR introduced and is the reason for the round. **The rerun-if-changed paths made every downstream build recompile.** `cargo package` ships only files under `rust/`, so in a registry checkout `../schemas/` does not exist -- and cargo treats a rerun-if-changed path that does not exist as permanently dirty. Declaring the schemas unconditionally therefore rebuilt freenet-stdlib, and everything downstream of it, on every `cargo build`. Measured ~1s per no-op build for this crate alone before its reverse-dependency cone; freenet-core and River would both have paid it. The directives now come after the opt-in early return, so they are declared only from a source checkout where the path really exists. Verified by building twice with `schemas/` moved away: cached both times, no recompile. **Re-pinned 24.3.25 -> 24.12.23, because the stated reason for 24.3.25 was wrong.** The pin was justified as matching the crate, but `flatbuffers = "24.3"` is a caret range and the package actually resolves to 24.12.23; `Cargo.lock` is untracked, so nothing held it anywhere near 24.3. Upstream ships flatc and the crate from the same tag, so pairing them nine minor releases apart was not the coupling the comment claimed. The two compilers emit byte-identical output for these schemas -- checked directly -- so this changes no generated file and every earlier verification carries over: Rust bindings unchanged, TypeScript regeneration still a no-op, 56 tests still pass. What changes is that the comment is now true. The residual is documented rather than asserted away: the runtime still floats within 24.x and nothing detects it drifting from the pin. **The schema list is discovered, not hardcoded.** It was a fixed array of three while the TypeScript side globs the directory, so a new .fbs generated TypeScript bindings and no Rust ones -- and the drift job reported success having compared a file set that excluded it. **The drift job no longer trusts the build script to have run.** It deletes the generated bindings first, so a regeneration that quietly does nothing leaves deletions, which the staleness check sees. Previously a no-op left a clean tree that read as success, and that only did not happen because this is the one Rust job without a cargo cache -- an implicit property nothing enforced. Also: the flatc download is checksummed (a substituted release asset would self-report the expected version, pass the gate, and then dictate what this repo must commit); the version is read from build.rs instead of being a second copy that can drift; the redundant `npm install` is gone; and a schema newer than its bindings now produces a cargo warning, since opt-in regeneration otherwise fails silently for someone editing a schema locally. Corrected two claims rather than leaving them: the `wire_format_is_frozen` tests cited earlier are bincode freezes that never touch src/generated -- the tests that actually exercise the regenerated code are the `test_build_contract_*_from_fbs` decoders -- and this job is advisory until it is added to branch protection, which `main` does not currently require. [AI-assisted - Claude]

CI clippy caught it: `is_none_or` is stable since 1.82 while Cargo.toml declares `rust-version = "1.80"`, so the `incompatible_msrv` lint failed the build script on both clippy targets. Replaced with `matches!`, which needs nothing newer than the MSRV. My earlier local clippy run predated this code -- I rewrote build.rs afterwards and did not re-run it. Re-run and clean now. [AI-assisted - Claude]

…vice naming the old pin Both from external re-review. The stale-schema warning could never fire. After one ordinary build cargo had recorded only `rerun-if-env-changed`, so editing a schema did not re-run the build script at all -- the warning was unreachable in exactly the situation it was added for. A check that cannot go off is worse than no check, because it reads as coverage. The previous commit had removed the path directives wholesale to fix the always-dirty downstream rebuild, which overcorrected: declaring paths that do not exist makes cargo rebuild forever, and declaring nothing makes it never re-run. The condition that separates those is whether the schema directory is actually there, so that is now the condition. A source checkout declares the schemas it can see; a published crate declares none and falls back to cargo's default heuristic, which caches correctly. `discover_schemas` returns None instead of panicking on a missing directory for the same reason -- it runs on every downstream build, where a panic would be far worse than the problem it reports. Verified all three ways: the warning now fires on a touched schema and names the right compiler; two consecutive in-repo builds still cache; and with schemas/ moved away, two consecutive builds cache with no recompile. The drift job's failure advice still said flatc 24.3.25 after the pin moved to 24.12.23, so a contributor following it would install a compiler the build script now rejects. It reads the version from the same step output as the install rather than repeating it. [AI-assisted - Claude]

…op an orphaned binding From the re-review, and the first item is a defect in the check this PR added rather than in the code it guards. `warn_if_schemas_look_newer` fired on every fresh clone. Git writes checkout files in path order, so `rust/src/generated/` lands before `schemas/`, making every schema strictly newer than every binding the moment a contributor clones. It emitted fifteen warnings across five jobs in this PR's own green CI run -- the same run where the drift job proved the bindings matched the schemas exactly. A warning that is already going off is not a warning: by the time it was true, nobody could have told it apart from the noise, which is the same "signal that cannot fail" the rest of this PR exists to remove. It now compares CONTENT. The regeneration path records an FNV-1a fingerprint of the schema bytes next to the bindings, and an ordinary build warns only when the current schemas hash differently. Verified both directions: touching a schema without editing it (the fresh-clone case) produces no warning, and appending a line to one does. Deliberately not DefaultHasher, whose output is not stable across Rust releases -- that would have swapped a false positive keyed on clone order for one keyed on the contributor's toolchain. Also removes typescript/src/host-response/application-message.ts, an orphan from when ApplicationMessage moved to common.fbs. Nothing imports it -- every reference resolves to common/application-message.js -- and flatc no longer emits it, which is exactly why the drift job could not see it: a generated file for a table that no longer exists is never regenerated, never modified, and so never shows up as drift. The general gap remains for the TypeScript side, which has no equivalent of the delete-before-regenerate step the Rust side uses. Corrected a comment the review disproved: emitting any directive opts out of cargo's default packaged-file scan, so a checkout without the schemas re-runs only on the env var rather than falling back to that scan. Right for a registry checkout, whose sources cannot change, but not what the comment claimed. [AI-assisted - Claude]

Two more from external review, both cases where drift stays invisible. **Orphans.** When a table is deleted, renamed, or moves namespace, flatc stops emitting its old .ts file but does not remove it. Regenerating in place leaves that tracked file untouched, so it is never modified and the tree reads clean -- the drift gate approves bindings that no longer match the schemas. The orphan removed in the previous commit is exactly this shape and sat green through several runs of the new job. The TypeScript half now gets the same delete-before-regenerate treatment as the Rust half, keyed on flatc's own header comment. That marker is what makes deletion-by-pattern safe rather than reckless: all 71 generated files carry it and none of the hand-written sources in typescript/src do (index.ts, streaming.ts, websocket-interface.ts all verified clean). An orphan therefore stays deleted through regeneration and surfaces as drift. **Added schemas.** Watching only the individual schema files gave cargo no input that changes when a schema is ADDED, so on an already-built checkout the script would not re-run: no stale-schema warning, and a regeneration already enabled could stay cached and silently omit the new binding. Watching the directory too closes that, and stays inside the only-when-present rule that keeps downstream builds cached. Verified the full CI sequence locally in both languages: deleting all 71 generated TypeScript files and all three Rust ones, then regenerating, returns a byte-clean tree. Caching re-checked after adding the directory watch -- two consecutive builds cache in-repo, and still cache with schemas/ moved away. [AI-assisted - Claude]

Small, and none of them a correctness risk -- recorded so the review is addressed rather than cherry-picked. The flatc-version step's friendly error was unreachable: GitHub Actions runs `bash -e`, so a failed grep aborted the step before the message could explain itself. It failed closed either way; now it also says why. The download checksum is the one value that cannot follow PINNED_FLATC automatically, since it has to be pinned by hand. Moving the pin without it fails with "computed checksum did NOT match", which reads as a supply-chain alarm rather than a forgotten hash, so the coupling is now stated at the call site along with the fact that the asset filename is not stable across flatc releases either. The TypeScript glob was `../schemas/flatbuffers/*` while the Rust side filters on `.fbs`, so a README dropped in that directory would break TypeScript generation and not Rust. Both now agree on `*.fbs`. Verified the narrowed glob regenerates byte-identical output. Not fixed, deliberately: flatc is pinned exactly while rustfmt is not, and build.rs shells to `cargo fmt` after regenerating -- so a contributor regenerating on a different toolchain could produce formatting the drift job rejects while blaming flatc. The fix is a repo-wide rust-toolchain.toml pinning the formatter for every contributor and every job, which is a larger decision than this PR should make on its own. Worth doing; worth doing deliberately. [AI-assisted - Claude]

sanity

Review summary: #110

Tier: Full (CI / build configuration is a high-risk surface).
Lenses: skeptical ×2 (separate heads) + Codex ×3 as the external non-Claude pass.
Rounds: four. Every round found something real, all of it in code added by this PR.
Final HEAD: f64109a1, CI green including flatbuffers_drift.

Fixed

Sev	Finding
High	`rerun-if-changed` declared `../schemas/...` unconditionally. Those paths do not exist in a registry checkout, and cargo treats a missing one as permanently dirty — so freenet-stdlib and its whole reverse-dependency cone recompiled on every build (~1s per no-op for this crate alone). A regression introduced by this PR.
High	The pin's justification was false: `flatbuffers = "24.3"` is a caret range resolving to 24.12.23, and `Cargo.lock` is untracked. Re-pinned to 24.12.23. The two compilers emit byte-identical output for these schemas (verified independently by the reviewer), so no generated file changed.
Med	`build.rs` silently succeeded when explicit regeneration could not run — worse than the code it replaced, since cargo caches a successful build-script run. Now panics.
Med	The drift check used `git diff --exit-code`, which ignores untracked files — so a new `.fbs` generating brand-new bindings, the loudest possible drift, sailed past it. Now `git status --porcelain`.
Med	The stale-schema warning fired on every fresh clone (git writes `rust/src/generated/` before `schemas/`, so schemas are always strictly newer). It emitted 15 warnings across 5 jobs in this PR's own green run — pre-desensitised before it ever meant anything. Replaced with an FNV-1a content fingerprint. Verified: touching a schema warns nothing, editing one warns; 0 warnings in CI, down from 15.
Med	Orphaned bindings were invisible: when a table moves namespace, flatc stops emitting its old file but does not delete it, so it is never modified and reads as clean. `typescript/src/host-response/application-message.ts` was exactly this and sat green through several runs. Removed, and the TS side now gets the same delete-before-regenerate treatment as Rust, keyed on flatc's own header comment.
Med	Watching only individual schema files gave cargo no input that changes when a schema is added. Now watches the directory too.
Med	The schema list was a hardcoded array of three while the TS side globbed, so a new schema would get TS bindings and no Rust ones — with the drift job reporting success over a file set that excluded it. Now discovered.
Low	Drift-failure advice still named the old pin after re-pinning, so following it installed a compiler the build script rejects. Now derived from the pin.
Low	MSRV: `is_none_or` is 1.82, crate is 1.80. Caught by CI clippy, not by me.
Low	flatc download unchecksummed; version duplicated in CI; `npm install` unnecessary; a comment about cargo's default file scan that the reviewer empirically disproved; TS glob `*` vs Rust `.fbs`; unreachable error message under `bash -e`.

Not fixed, with reasons

rustfmt is not pinned while flatc is. build.rs shells to cargo fmt after regenerating, so a contributor on a different toolchain could produce formatting the drift job rejects while the message blames flatc. The fix is a repo-wide rust-toolchain.toml affecting every contributor and job — a larger decision than this PR should make alone. Worth doing deliberately.
main requires no status checks, so flatbuffers_drift reports drift rather than preventing its merge. Adding it to branch protection is a repo-settings change, noted in build.rs rather than asserted away.
The bindings lose unsafe_op_in_unsafe_fn hygiene by pinning to a 24.x compiler, which will need revisiting before an edition-2024 migration. Public item set is identical to main across all three files (reviewer-verified), so no API break.

Verified, not asserted

Regeneration is idempotent; a plain build leaves a reverted binding file reverted; deleting all 71 TypeScript and all 3 Rust generated files and regenerating returns a byte-clean tree; two consecutive builds cache both in-repo and with schemas/ moved away; the wrong-compiler panic fires with both versions named; 56 Rust tests, 78 TypeScript tests, clippy clean.

Verdict: Ready to merge.

[AI-assisted - Claude]

sanity added 9 commits September 1, 2026 09:10

sanity commented Sep 2, 2026

View reviewed changes

sanity merged commit 08e77ad into main Sep 2, 2026
11 checks passed

sanity deleted the feat/request-id-correlation branch September 2, 2026 02:08

Navigation Menu

Uh oh!

build: pin flatc and check flatbuffers binding drift in CI - #110

build: pin flatc and check flatbuffers binding drift in CI#110
sanity merged 9 commits into
mainfrom
feat/request-id-correlation

sanity commented Sep 1, 2026

Uh oh!

sanity left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

sanity commented Sep 1, 2026

Problem

Approach

Why 24.3.25

Commits

Testing

Note for reviewers

Uh oh!

sanity left a comment

Choose a reason for hiding this comment

Review summary: #110

Fixed

Not fixed, with reasons

Verified, not asserted

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant