Repository navigation
fix(core): format generated artifacts through oxfmt's API, not a bun spawn - #6152
ym256-stanford wants to merge 1 commit into
Conversation
…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
left a comment
There was a problem hiding this comment.
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)), "..", ".."); |
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
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.
|
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 This branch now conflicts with main in exactly those two files ( Need: no, fixed by #5859 · Size: ok · Reuse: n/a · Simpler: n/a — Rames |
Summary
Fixes #5856. The codegen format step (
formatGeneratedSource) shelled out tobun x oxfmtfrom a tsx/node process. On Windows, where PATH can carry only the extensionless bun shim, that spawn fails withENOENTand the silentcatchpublished unformatted artifacts — dirtying the force-tracked files underpackages/core/src/generated/and blockinggit pullafter 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.jsonis 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.buildInjectedArtifactand its three caller scripts becomeasync;oxfmtis declared as a@hyperframes/coredevDependency.Verification
ENOENT), and oxfmt-reported errors: 7/7 pass vianode --import tsx --test.check:position-edits-render,check:svg-selector-aliases, andbuild:audio-fx-runtime+git diff --exit-code.bunfrom PATH (spawn fails withENOENT, confirmed); codegen output still byte-identical.oxlint,oxfmt --check,tsc --noEmitclean.Closes #5856