perf(api): I/O assíncrono no diff-validator + cache de versão (ENG-1667)

[limaronaldo]

Problema

Operações fs.writeFileSync/readFileSync bloqueavam o event loop do Bun — pior caso: writeFileSync dentro de loop em applyFileChanges (diff-validator.ts:255), bloqueando por arquivo modificado. version.ts lia package.json do disco a cada chamada.

Solução

• diff-validator.ts: mkdtemp/writeFile/unlink/rm/mkdir via fs.promises
• applyFileChanges agora é async com writes paralelos via Promise.all
• cleanupTempDir async, aguardado no finally de validateDiff
• version.ts: cache em memória — lê package.json no máximo 1x por processo (+ helper de reset para testes)
• Handler de cleanup em process.on("exit") mantido sync intencionalmente (exit handlers não podem await)

Testes

• tsc --noEmit limpo

Closes ENG-1667

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a492aaf381

ℹ️ 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".

[chatgpt-codex-connector]

Keep path-dependent file changes ordered

When a valid diff replaces a tracked file with a directory of the same name—for example, deleting foo and adding foo/bar.ts—these operations are dependent: parallel execution can run mkdir(foo) while foo is still a file, causing EEXIST. The rejection is caught by validateDiff as only a warning, so typechecking is skipped and the diff can still return valid: true; preserve ordering for path-conflicting changes or parallelize only independent paths.

AGENTS.md reference: AGENTS.md:L59-L59

Useful? React with 👍 / 👎.

[limaronaldo]

Light Review — PR #429 (ENG-1667): Sync-to-async I/O in diff-validator + version.ts cache

Verdict: changes-needed (one MEDIUM race condition worth addressing or explicitly accepting before merge)

Summary

packages/api/src/core/diff-validator.ts: cloneRepo's mkdtempSync/writeFileSync/unlinkSync calls are converted to their fs.promises equivalents with await. The singular cleanupTempDir(tempDir) helper is correctly converted to async, and both call sites (validateDiff's success and finally paths) are updated to await it. applyFileChanges is converted from a sequential for loop to await Promise.all(files.map(async (file) => {...})), using fs.promises.rm(fullPath, { force: true }) for deletions (dropping the old existsSync pre-check, which is fine since force: true already no-ops on a missing path) and fs.promises.mkdir(dir, { recursive: true }) + fs.promises.writeFile(...) for additions/modifications.

Critically, the plural tempDirs/cleanupTempDirs architecture — the process.on("exit", ...) handler that cleans up all tracked temp directories on process exit — is untouched by this diff and correctly remains synchronous. This is architecturally correct: Node's "exit" event handler cannot await async work (the event loop is already terminating), so this exit-time cleanup path must stay sync while the per-call cleanup (cleanupTempDir, singular, called from within async function validateDiff) can and should be async. This distinction is preserved correctly in the diff.

packages/api/src/lib/version.ts: adds a module-level cachedVersion variable populated on first getVersion() call, with a test-only __resetVersionCacheForTests() export to clear it. package.json's version is immutable at runtime (not hot-reloaded), so caching is semantically safe — no invalidation is needed because the underlying value cannot legitimately change without a process restart.

Findings

• [MEDIUM] packages/api/src/core/diff-validator.ts (applyFileChanges) — parallelized Promise.all writes race on path-type transitions (file↔directory) within a single diff.
  A syntactically valid diff can, in the same changeset, delete file a and add file a/b (converting a from a file to a directory), or the reverse (delete everything under directory a/ and add file a). Because all file operations in the files array now run concurrently via Promise.all with no ordering guarantee, fs.promises.mkdir(path.dirname("a/b"), { recursive: true }) could execute before fs.promises.rm("a", { force: true }) completes, causing mkdir to fail with EEXIST (a file exists where a directory is needed) — or the reverse ordering could leave a stale file when a directory should have replaced it. The prior sequential for loop was implicitly safe against this because it preserved diff-listing order (deletes before adds, if the diff generator emits them in that order) and each operation completed before the next began.
  This is a genuine behavior change (not just a performance one): correctness for a specific class of legitimately-producible diffs, not merely malformed input, now depends on operation completion order that Promise.all does not guarantee. Recommend one of: (a) run deletions in one Promise.all pass, then run creates/writes in a second Promise.all pass (preserves parallelism within each phase, fixes ordering across phases); or (b) detect path ancestor/descendant conflicts and serialize just those; or (c) if this scenario is considered out-of-scope/rare enough to accept, document that explicitly in a code comment and add a regression test asserting the current (possibly still-failing) behavior so a future change doesn't silently alter it further. Cross-model review (codex exec) independently identified this same race and suggested the equivalent regression tests (file→directory and directory→file transitions).

• [LOW] packages/api/src/lib/version.ts — cache has no error-path memoization; a failed first read is not cached, causing repeated fs.existsSync/readFileSync/JSON.parse on every call until success.
  This is arguably correct behavior (don't cache a failure so a subsequent fix without restart could recover), but worth a one-line comment confirming it's intentional, since silent perf degradation from an unexpectedly-broken package.json could otherwise look like a caching bug during triage.

Not found

• No missing await on any converted call — the two cleanupTempDir call sites, the applyFileChanges call site, and all fs.promises.* calls inside cloneRepo are all correctly awaited. No unhandled promise rejections introduced. The "exit"-handler sync path is correctly left untouched.

Cross-model review

codex exec -m gpt-5.6-terra reviewed the diff independently and returned one Medium-severity finding — the same path-type-transition race in applyFileChanges identified above — corroborating this as the primary substantive issue in the diff. Codex characterized the version.ts change as "internally sound... provided tests reset the cache when they alter the package metadata fixture," consistent with this review's LOW note.

[limaronaldo]

Fixed in 9266961.

applyFileChanges now groups file operations by path-prefix conflict (union-find over normalized paths where one path is an ancestor of another — e.g. foo and foo/bar.ts). Conflicting groups run sequentially with deletes ordered before creates; independent groups still run in parallel via Promise.all.

Also hardened the write path: if a stale (now-empty) directory is left at the exact write target after its last child was deleted within the same group, it's removed before writeFile — otherwise the dir→file replacement case still throws EISDIR.

Added packages/api/src/core/diff-validator.test.ts (new file, applyFileChanges exported for direct unit testing) covering:

• file → dir: delete foo, add foo/bar.ts in the same changeset
• dir → file: delete foo/bar.ts (last file under foo), add file at foo in the same changeset
• delete-before-create ordering holds regardless of input array order
• independent (non-conflicting) paths are unaffected by a conflicting group elsewhere in the changeset
• baseline coverage: parallel writes, deletes, and force-delete of a missing file

bun test in packages/api: 447 pass / 37 fail, all 8 new tests passing — the 37 failures are pre-existing Playwright/CUA environment failures (ActionExecutor, BrowserManager, Visual Regression suites, missing Playwright binary), unrelated to this change and identical to the baseline failure set before the fix. tsc --noEmit clean.