fix(codegen): fail when the core sources are missing instead of emitting less - #10
Conversation
…ing less 0.6.3 shipped a .d.ts with 183 of its 297 type declarations gone, still declaring `queryUsync(query: UsyncQuery): Promise<UsyncResponse>` for types it no longer exported. Any TypeScript consumer of that release fails to compile. This crate does not depend on whatsapp-rust, so `cargo run` never fetches it — the sources have to already be on disk. On a runner with a cold cache, `gen` runs before anything downloads the core, every lookup misses, and `WalkDir` over a missing directory yields nothing rather than erroring. The generator wrote a nearly empty type file, which compiled, and the build stayed green all the way to the registry. It now refuses to run without the sources, and refuses to emit an empty file. Both workflows also diff the regenerated file against the committed one, so what ships is what was reviewed rather than whatever a runner reconstructed. No floor on the type count: the real number is 183, and a threshold near it breaks on any legitimate change to the core. The drift check compares against the committed file instead of guessing.
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b13e49ab4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| fn main() { | ||
| let root = find_whatsapp_rust_root(); | ||
| require_sources(&root); |
There was a problem hiding this comment.
Fetch the core before requiring its checkout
On a fresh clone or GitHub-hosted runner, bun run build invokes gen:bridge-types before any root-crate Cargo command can materialize the whatsapp-rust checkout, so this call panics when no sibling clone exists. In particular, the build-and-test job and the separate publish job go from bun install directly to this build; Swatinem/rust-cache caches Cargo's git database/build outputs rather than guaranteeing the working checkout expected here. Add an explicit cargo fetch/cargo metadata step before these builds, or make the generator populate the checkout itself, so the documented full-build command and release publish work on clean machines.
AGENTS.md reference: AGENTS.md:L97-L103
Useful? React with 👍 / 👎.
The previous commit made a missing checkout fail loudly instead of emitting a broken type file, which stops the bad publish but leaves the build unable to run anywhere the sources are not already on disk — a fresh clone, or any runner. `cargo fetch` materialises the git checkout the generator reads, and it belongs in the script rather than in the workflows: `bun run build` is the documented command, and it should work on a clean machine without the caller knowing this crate reads the core off disk. Verified on a fresh clone with an empty CARGO_HOME and no sibling checkout: 183 types parsed, output identical to the committed file.
|
Right, and the same point the reviewer raised: the first commit only stopped the bad publish. It did not make the build work — a fresh clone or a runner would now fail loudly instead of silently shipping less, which is better but still broken. Fixed in It belongs in the script rather than in the workflow steps: Verified on the exact failing scenarioFresh clone of this branch, empty And the output is byte-identical to the committed Also confirmed the premise before relying on it: So the three layers now are: fetch so it generates, panic if the sources are still absent, and diff against the committed file so what ships is what was reviewed. |
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The guard checked `wacore/src` alone while the generator also reads `src/features`, `src/types` and a `send` path. wacore contributes most of the types, so a checkout missing only `src/` would have produced a plausible file and passed both the guard and the emptiness check — the same silent-partial failure this branch exists to remove, one directory down. Every parsed path is now required, and the message names which one is missing. Writing that list surfaced a dead read: `send.rs` became `send/` in the core, so the hardcoded `parse_file` had been pointing at nothing. Nothing was lost, since the types it named are not `Serialize` and this generator only emits those, but the path now points at what exists. Output is unchanged: 183 types, byte-identical to the committed file.
|
One applied, one already fixed. Guard only covered Verified by building exactly that checkout — Writing the list surfaced something else: CI never fetches the core — already fixed in Proven on the failing scenario: fresh clone, empty One note on method: my check for the happy path first "passed" while the generator was actually failing, because |
0.6.3is broken on npm. Its.d.tslost 183 of 297 exported type declarations, while still declaring:for types it no longer exports. Any TypeScript consumer fails to compile — the baileyrs
dts-drifttest caught it on the first bump attempt.Root cause
codegendoes not depend onwhatsapp-rust(onlysyn,walkdir,quote), socargo runnever fetches it: the sources must already be on disk. It looks in Cargo's git checkout, then falls back to../../whatsapp-rust.On a runner with a cold cache,
genruns before anything downloads the core, so the checkout does not exist yet and there is no sibling clone.WalkDirover a missing directory yields nothing instead of erroring, the generator wrote a nearly empty type file, that compiled, and the build stayed green through publish.Locally it always worked, because both the checkout and a sibling clone exist here. That is why it survived review.
Ironically this was exposed by #6, which made CI and the release run the real
bun run build. Before that neither rangen, and the committed file was what shipped.Fix
require_sourcespanics with a message naming the path it expected, rather than parsing an empty tree.git diff --exit-code src/generated_types.rsafter the build, in verify and publish, so what ships is the reviewed file rather than whatever a runner reconstructed.No floor on the type count. I tried one and it was wrong: the real number is 183, and my initial threshold of 200 failed the legitimate build. A number close to the truth breaks on any change to the core; the drift check compares against the committed file and needs no calibration.
Verified, all three paths
Worth noting the second test caught a mistake in my own verification: an earlier run of it "passed" only because
gen:bridge-typeschainscargo run ... > tmp && mv, so a failing generator leaves the previous file in place anddifflooks clean. Checking the exit code, not just the diff, is what exposed it.After this
Publish
0.6.4and then bump baileyrs.0.6.3should be deprecated on npm — it is installable and broken for TypeScript consumers:Summary by cubic
Fail fast when
whatsapp-rustsources are missing and guard all parsed paths. Builds now fetch the core and CI/release check for type drift so the published.d.tsis the reviewed one.wacore/src,src/features,src/types,src/send), reads thesendmodule, prints the parsed type count, and refuses to emit an empty file.gen:bridge-typesrunscargo fetchbefore codegen so clean machines and CI work.src/generated_types.rsafterbun run build(including the publish path) to catch drift and ship the reviewed types.Written for commit f342875. Summary will update on new commits.