Skip to content

fix(core): format generated artifacts through oxfmt's API, not a bun spawn - #6152

Closed
ym256-stanford wants to merge 1 commit into
heygen-com:mainfrom
ym256-stanford:fix/codegen-oxfmt-no-bun-spawn
Closed

ym256-stanford wants to merge 1 commit into
heygen-com:mainfrom
ym256-stanford:fix/codegen-oxfmt-no-bun-spawn

Conversation

@ym256-stanford

Copy link
Copy Markdown
Contributor

Summary

Fixes #5856. The codegen format step (formatGeneratedSource) shelled out to bun x oxfmt from a tsx/node process. On Windows, where PATH can carry only the extensionless bun shim, that spawn fails with ENOENT and the silent catch published unformatted artifacts — dirtying the force-tracked files under packages/core/src/generated/ and blocking git pull after every build.

The formatter now calls oxfmt's JS API (import("oxfmt") + format()), so formatting runs in-process and no longer depends on a spawnable bun. The repo's .oxfmtrc.json is passed when present (verified byte-identical to the CLI path). The fallback for environments without oxfmt stays, but warns with the file name and error instead of swallowing them.

buildInjectedArtifact and its three caller scripts become async; oxfmt is declared as a @hyperframes/core devDependency.

Verification

  • New unit tests cover the API path, the spawn-failure fallback (injected rejection ≙ the Windows ENOENT), and oxfmt-reported errors: 7/7 pass via node --import tsx --test.
  • All three artifacts rebuild byte-identical: check:position-edits-render, check:svg-selector-aliases, and build:audio-fx-runtime + git diff --exit-code.
  • Windows condition simulated by scrubbing bun from PATH (spawn fails with ENOENT, confirmed); codegen output still byte-identical.
  • oxlint, oxfmt --check, tsc --noEmit clean.

Closes #5856

…spawn

The codegen format step shelled out to `bun x oxfmt` from a tsx/node
process; on Windows, where PATH can carry only the extensionless bun
shim, that spawn fails with ENOENT and the silent catch published
unformatted artifacts, blocking git pull.

- formatGeneratedSource now uses oxfmt's JS API in-process, with the
  repo's .oxfmtrc.json when present
- the fallback stays but warns instead of swallowing the failure
- buildInjectedArtifact and its three caller scripts go async
- declare oxfmt as a core devDependency

Refs heygen-com#5856

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 8b97c2e26b035e316769d978d8174931d184676a. The three artifact callers await formatting before publication, and the focused formatter tests pass (7/7).

One config-parity defect remains at packages/core/scripts/writeGeneratedFile.ts:78: the lookup resolves to packages/.oxfmtrc.json, so the actual repository config is silently ignored. See the inline reproduction.

Audited: all seven changed files, formatter API boundary and all three callers. Local validation: seven tests, three artifact builds, two tracked artifacts byte-identical, and a non-default config control that fails on this head and passes with the corrected path. Trusting: no native Windows execution or full workspace typecheck/lint by this reviewer. Hosted snapshot contains only successful WIP and Graphite checks, not test/build execution. GitHub currently reports this head conflicting (dirty). Auto-merge was not enabled.

Verdict: REQUEST CHANGES
Reasoning: The in-process formatter does not read the root configuration as promised; current defaults happen to hide the mismatch. Correct the root and add a non-default config regression before merge.

— Magi

input: source,
encoding: "utf8",
});
const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), "..", "..");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Resolve the actual repository root before loading formatter options

This module lives in packages/core/scripts, so two .. segments reach packages, not the repository root. Consequently loadOxfmtConfig() catches ENOENT and always supplies defaults in the normal checkout. I reproduced this with the real formatter API in an isolated source copy: setting root .oxfmtrc.json to singleQuote: true still produces export const A = "x";, while directly passing that option produces single quotes. Current root settings match defaults, so the seven tests and current artifact byte comparisons do not expose it. Resolve the root one level higher (or locate it reliably) and pin a non-default root option; the same control passes with the extra parent step.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, and for the unusually thorough write up. Moving off the bun spawn is the right fix for the Windows ENOENT path, and the three callers all await correctly.

One thing blocks it. In packages/core/scripts/writeGeneratedFile.ts:78 the root resolves two levels up from packages/core/scripts, which lands on packages/core, not the repo root, so there is no .oxfmtrc.json there (it lives at the repo root). The read always throws, the catch returns undefined, and oxfmt runs on its defaults. It looks byte identical today only because the repo settings happen to match oxfmt's defaults, so the moment someone changes printWidth the generated files go out of sync again. Three levels up is what you want. A test with a non default option would keep it from regressing: all three current tests pass whatever that path resolves to.

Also the branch is conflicting and full CI has not run on this head, only the WIP and mergeability marks. A rebase should get the real jobs going.

@jrusso1020

Copy link
Copy Markdown
Collaborator

Thanks for the thorough write-up. The bug this fixes is already fixed on main, so I think this one can be closed.

#5859 (merged 2026-10-19 as d369159) replaced the bun x oxfmt spawn in packages/core/scripts/writeGeneratedFile.ts. It resolves oxfmt/bin/oxfmt with createRequire and runs it with process.execPath, so it no longer needs a spawnable bun on PATH, which was the Windows ENOENT in #5856. It also throws before publishing instead of silently writing unformatted artifacts, and because it goes through the oxfmt CLI it picks up the repo's .oxfmtrc.json the normal way. That last point also sidesteps the config-path issue Miguel flagged at line 78 here (the lookup resolved to packages/core instead of the repo root).

This branch now conflicts with main in exactly those two files (writeGeneratedFile.ts and its test), for the same reason. #5856 is still open on GitHub but is resolved by #5859 and can be closed with it.

Need: no, fixed by #5859 · Size: ok · Reuse: n/a · Simpler: n/a

— Rames

@jrusso1020 jrusso1020 closed this Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

build: codegen silently skips oxfmt when bun can't be spawned, leaving unformatted generated files that block git pull

3 participants