fix: harden stable package payloads - #238
Conversation
|
This pull request is part of a Mergify stack:
|
Update GraphQL Code Generator and esbuild to supported releases without changing generated output. Trim Proof's npm payload to its bins and built files.\n\nTest plan: pnpm verify; package audits; Proof pack and CLI smoke checks. Change-Id: I69eee15141a4d6332f504bee70f4b533d076e077
2d66900 to
a9543f2
Compare
d50f852 to
034f313
Compare
Revision history
|
There was a problem hiding this comment.
Stale comment
Review verdict
REQUEST_CHANGES — consensus HIGH on the unverified public “byte-identical files” CHANGELOG contract after multi-major
@graphql-codegen/*bumps (also two independent HIGHs onceCODEGEN_OUTPUT_VERSION/ cache invalidation is included).Chunk-bound feedback
CHANGELOG.md:43-50 — severity: HIGH — flagged-by: dependency-runtime-surface, release-discipline, docs-and-positioning, dx-and-examples
- Risk: The note asserts that the same schema still generates byte-identical files across cli 5→6 / plugin majors and an esbuild 0.15→0.25 pin jump, with no golden checksum, fixture, or example regen in-tree. Major codegen lines document emission changes, and bundling the claim with esbuild invites readers to infer config
.mjsbyte-stability too.- Minimal fix: Split into two bullets (codegen vs esbuild); drop “byte-identical” / blanket “nothing changes” unless a committed golden (or example regen) proves it—otherwise say compatible / smoke-tested / semantically equivalent, scoped per surface.
packages/codegen/package.json:37-40 — severity: HIGH — flagged-by: dependency-runtime-surface, dx-and-examples
- Risk: Four
@graphql-codegen/*majors ship while Flatbread’s invoke path stays API-compatible, but cache keys still ignore the new generator line (CODEGEN_OUTPUT_VERSIONstays at2; resolved plugin versions are not hashed), so existing.flatbread-codegen-cache.jsoncan skip regen after upgrade.- Minimal fix: Bump
CODEGEN_OUTPUT_VERSIONinpackages/codegen/src/hash.ts(or hash resolved codegen package versions) in this release; stop asserting byte-identity unless a golden compare proves it.packages/config/package.json:43-43 — severity: MED — flagged-by: dependency-runtime-surface, dx-and-examples
- Risk: Exact
esbuild0.15.1→0.25.0 is a large 0.x jump on the only consumer;load.test.tssmokes a minimal config, but everyflatbread start/codegenpath bundles real configs with no assert on those edges.- Minimal fix: Keep the exact pin; add a load fixture mirroring
examples/nextjs/flatbread.config.js.packages/proof/package.json:38-38 — severity: LOW — flagged-by: release-discipline, docs-and-positioning, dx-and-examples
- Risk: Dropping
src/scriptsfromfilesis the right publish trim, butfilesstill lists unused root*.d.ts, and publishedscriptsstill advertise monorepo paths under unpackaged dirs.- Minimal fix: Remove the unused
*.d.tsentry; drop or relocate monorepo-only script entries from the published manifest.Coverage plan
packages/codegen/src/__tests__/e2e.test.ts— positive — same fixture schema/documents regenerate byte-identical (or explicitly versioned) output vs a committed golden; fail CI if bytes drift without aCODEGEN_OUTPUT_VERSIONbump.packages/codegen/src/__tests__/*cache*/ hash unit — positive — after simulated@graphql-codegen/*(orCODEGEN_OUTPUT_VERSION) bump, cache miss forces regen.packages/config/src/load.test.ts— edge:nextjs-shaped config — loads a fixture with externalized@flatbread/resolver-svimg, relative imports, and overrides under esbuild 0.25.0.packages/config/src/load.test.ts— edge:.tsconfig / externalize — if supported, assert transform + externalize still succeeds post-pin.Reviewer scoreboard
- dependency-runtime-surface: 3 findings, 2 coverage gaps, signal:HIGH
- release-discipline: 3 findings, 1 coverage gaps, signal:HIGH
- docs-and-positioning: 3 findings, 2 coverage gaps, signal:HIGH
- dx-and-examples: 4 findings, 2 coverage gaps, signal:HIGH
Perspectives: dependency-runtime-surface, release-discipline, docs-and-positioning, dx-and-examples (release/docs/deps-only path; no
packages/*/src). Models: grok-4.5 high / composer-2.5.Sent by Cursor Automation: Flatbread PR Review
| - `@flatbread/codegen` moves its four GraphQL Code Generator packages (`cli`, | ||
| `typescript`, `typescript-operations`, and `typed-document-node`) onto | ||
| supported major lines, and `@flatbread/config` moves its pinned `esbuild` | ||
| from 0.15.1 to 0.25.0. Nothing you get out changes: the same schema still | ||
| generates byte-identical files. |
There was a problem hiding this comment.
HIGH — “Nothing you get out changes: the same schema still generates byte-identical files” is a falsifiable public contract after multi-major @graphql-codegen/* bumps (and an esbuild 0.15→0.25 pin in the same breath), with no golden checksum, fixture, or example regen in this PR.
Minimal fix: split codegen vs esbuild into two bullets; drop “byte-identical” / blanket “nothing changes” unless a committed golden (or example regen) proves it—otherwise say compatible / smoke-tested / semantically equivalent, scoped per surface.
| "@graphql-codegen/cli": "^6.3.1", | ||
| "@graphql-codegen/typed-document-node": "^6.1.8", | ||
| "@graphql-codegen/typescript": "^5.0.10", | ||
| "@graphql-codegen/typescript-operations": "^5.1.0", |
There was a problem hiding this comment.
HIGH — Four @graphql-codegen/* majors ship here, but CODEGEN_OUTPUT_VERSION in packages/codegen/src/hash.ts stays at 2 and resolved generator versions are not hashed. Existing .flatbread-codegen-cache.json can skip regen after upgrade while the CHANGELOG claims identity.
Minimal fix: bump CODEGEN_OUTPUT_VERSION (or hash resolved codegen package versions) in this release; stop asserting byte-identity unless a golden compare proves it.
| "dependencies": { | ||
| "@flatbread/core": "workspace:*", | ||
| "esbuild": "0.15.1" | ||
| "esbuild": "0.25.0" |
There was a problem hiding this comment.
MED — Exact pin esbuild 0.15.1→0.25.0 is a large 0.x jump on the only consumer. load.test.ts smokes a minimal config, but every flatbread start / codegen path bundles real configs (externalized workspace packages, relative imports, overrides) with no assert on those edges.
Minimal fix: keep the exact pin; add a load fixture mirroring examples/nextjs/flatbread.config.js.
| "files": [ | ||
| "bin", | ||
| "dist", | ||
| "src", | ||
| "scripts", | ||
| "*.d.ts" |
There was a problem hiding this comment.
LOW — Dropping src/scripts from files is the right publish trim (bin/dist still ship). Residual hygiene: files still lists unused root *.d.ts (pack ships dist/*.d.ts only), and published scripts still advertise monorepo paths under unpackaged dirs.
Minimal fix: remove the unused *.d.ts entry; drop or relocate monorepo-only script entries from the published manifest.
There was a problem hiding this comment.
Review verdict
REQUEST_CHANGES — Two independent HIGHs (dependency-runtime-surface, docs-and-positioning) plus 4/4 consensus that CHANGELOG’s “byte-identical” / “Nothing you get out changes” claim overstates codegen (and conflates it with the esbuild pin) without a golden lock. Proof files[] hardening is sound for CLI/dist but silently breaks --restart-on-runner-change for npm installs.
Perspectives: dependency-runtime-surface, release-discipline, docs-and-positioning, dx-and-examples → judge. Models: grok-4.5 (high) / composer-2.5.
Consensus findings
- HIGH — CHANGELOG “byte-identical” / “Nothing you get out changes” overclaims across four
@graphql-codegen/*majors (+ blankets esbuild). Soften wording and/or add a golden before release. - MED — codegen majors land cleanly on imports, but lack an output-stability lock (substring e2e only).
- MED — dropping
src/scriptsfrom prooffiles[]is correct for the published CLI/diststory, but makes--restart-on-runner-changea silent no-op for npm/tarball installs. - LOW — esbuild 0.15.1→0.25.0 under-tested for non-trivial
loadConfig()fixtures.
Coverage plan
packages/codegene2e — golden hash/snapshot of fullgraphql.tsfor a fixed schema; empty-variables document assert on*QueryVariablesshape (reject silent{}→Record<PropertyKey, never>drift).packages/configload test — fixture with relative/local import under esbuild 0.25externalize-deps.packages/proof— pack assertsbin/+dist/present andsrc/+scripts/absent; dist-only--restart-on-runner-changemust warn/exit, never silent empty snapshot.
Suggested follow-ups
- Edit CHANGELOG: drop or qualify “byte-identical”; split codegen vs esbuild bullets.
- Add codegen golden (+ empty-vars) before any residual stability language.
- Fix or document proof supervisor restart under dist-only npm installs.
- Optional: esbuild fixture or “default configs only” note.
Reviewer scoreboard
- dependency-runtime-surface: REQUEST_CHANGES — strong (concrete
typescript-operations@5empty-object break) - release-discipline: COMMENT — strong (pack/
bin/exports+ restart silent no-op) - docs-and-positioning: REQUEST_CHANGES — strong (HIGH overclaim + codegen/esbuild conflation)
- dx-and-examples: COMMENT — good (restart + pack DX)
Sent by Cursor Automation: Flatbread PR Review
| - `@flatbread/codegen` moves its four GraphQL Code Generator packages (`cli`, | ||
| `typescript`, `typescript-operations`, and `typed-document-node`) onto | ||
| supported major lines, and `@flatbread/config` moves its pinned `esbuild` | ||
| from 0.15.1 to 0.25.0. Nothing you get out changes: the same schema still | ||
| generates byte-identical files. |
There was a problem hiding this comment.
severity: HIGH — flagged-by: dependency-runtime-surface, docs-and-positioning, release-discipline, dx-and-examples
risk: This bullet asserts “Nothing you get out changes” and “byte-identical files” across four @graphql-codegen/* majors (cli 5→6, typed-document-node 2→6, typescript 4→5, typescript-operations 4→5) and also blankets the @flatbread/config esbuild 0.15.1→0.25.0 pin. Tests only check substrings/TS compile, not full-file hashes. typescript-operations@5 documents empty-object emission {} → Record<PropertyKey, never> when documents exist.
minimal fix: Soften to schema-/behaviorally compatible (and note document-path typing may change), split codegen vs esbuild into separate bullets, or add a pinned-schema (+ empty-vars documents) golden/hash test and cite it before keeping any identity claim.
| "@graphql-codegen/cli": "^6.3.1", | ||
| "@graphql-codegen/typed-document-node": "^6.1.8", | ||
| "@graphql-codegen/typescript": "^5.0.10", | ||
| "@graphql-codegen/typescript-operations": "^5.1.0", |
There was a problem hiding this comment.
severity: MED — flagged-by: dependency-runtime-surface, release-discipline, docs-and-positioning, dx-and-examples
risk: Coordinated majors still resolve on import/generate/CodegenConfig (no source adapter needed), but they enable the operations typing break above with only substring e2e coverage and caret drift within the new majors — no output-contract lock.
minimal fix: Keep the bumps; add a schema-only (+ empty-variables documents) golden/hash assertion in packages/codegen e2e before asserting stability in release notes.
| "bin", | ||
| "dist", | ||
| "src", | ||
| "scripts", | ||
| "*.d.ts" |
There was a problem hiding this comment.
severity: MED — flagged-by: release-discipline, dx-and-examples, docs-and-positioning
(Attached to adjacent RIGHT-side context: pure-deletion hunk removed src/scripts above *.d.ts.)
risk: Dropping src/scripts from files[] is correct for CLI/dist/exports (pack inspection agrees), but proof-supervisor’s --restart-on-runner-change still snapshots ../src/*.ts and skips missing files silently — so npm/tarball installs get an undetectable no-op. Published package.json still lists scripts that invoke tsx src/... / scripts/... which are no longer in the tarball.
minimal fix: Snapshot dist/ when src/ is absent, or warn when the snapshot is empty with the flag set; optionally note monorepo-only scripts / supervisor self-hosting in CHANGELOG.
| "dependencies": { | ||
| "@flatbread/core": "workspace:*", | ||
| "esbuild": "0.15.1" | ||
| "esbuild": "0.25.0" |
There was a problem hiding this comment.
severity: LOW — flagged-by: release-discipline, dx-and-examples
risk: esbuild 0.15.1→0.25.0 drives every loadConfig() bundle via externalize-deps; existing AVA cases cover only trivial inline configs, not nested/node_modules or workspace-import paths.
minimal fix: Add a fixture config that imports a sibling/local package, or qualify the CHANGELOG esbuild line as smoke-tested on default configs only.
There was a problem hiding this comment.
CI Root Cause
Status: third-party-failure
Failing checks:
integration-nextjs (20.x, windows-latest):pnpm install --frozen-lockfilefailed duringsharp@0.30.7libvips download (socket hang up)
Evidence:
- Job never left install. Log shows
sharp: Downloading https://github.com/lovell/sharp-libvips/releases/download/v8.12.2/libvips-8.12.2-win32-x64.tar.brthensharp: Installation error: socket hang upandELIFECYCLEexit 1. - Same run also logged
sqlite3prebuildsocket hang up(secondary network flake). - Sibling matrix cells on SHA
a9543f2passed, includingintegration-nextjs (20.x, ubuntu-latest),integration-nextjs (20.x, macos-latest),integration-nextjs (22.x, windows-latest), andintegration-sveltekit (20.x, windows-latest). - This PR commit only touches codegen/esbuild/proof package manifests, CHANGELOG, and lockfile; it does not change install scripts that would uniquely break Windows Node 20.
Resolution steps:
- Re-run the failed job (or the whole Pipeline workflow) for SHA
a9543f2. - If sharp-libvips downloads keep failing across retries, check GitHub Releases / runner egress health; this is not a branch code defect.
- No branch code change is warranted unless a retry reaches the Next.js integration steps and fails there.
No code changes were made because this failure is not caused by the branch.
Sent by Cursor Automation: Flatbread - Fix CI


Update GraphQL Code Generator and esbuild to supported releases without changing generated output. Trim Proof's npm payload to its bins and built files.\n\nTest plan: pnpm verify; package audits; Proof pack and CLI smoke checks.
Depends-On: #237