mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-06 02:01:07 +00:00
Five findings from a fifth (final scoped) max-effort review of the previous commit, all local: 1. writeConfig now writes atomically (pid-suffixed temp file + renameSync — rename within one directory is atomic on POSIX). This closes the real hazard behind the review's torn-read finding: readConfig's corrupted-file catch RESETS the config to defaults (telemetry re-enabled, anonymousId rotated, trial fields wiped), so a concurrent reader catching a non-atomic write mid-flight would silently destroy the user's config — and the previous commit's per-render readConfigFresh() at the arm site multiplied exposure to exactly that window. Verified against a real filesystem, not just the mocked unit tests. 2. writeConfig now returns whether the write landed (errors still swallowed — telemetry must never break the CLI). persistDeParallelRouterTrialFired uses it to stop immediately on a genuine fs failure (retrying an unwritable file is pointless) and reserve its retries for actual concurrent clobbers, instead of 3 blind write attempts + 4 disk reads. 3. The persistence-failure console.warn is now !quiet-gated like every other trial message — a quiet/batch-json render on an unwritable ~/.hyperframes no longer emits unexpected stderr that CI wrappers asserting empty stderr would misread as a render failure. The in-process latch already guarantees the safety behavior whether or not the warning prints. 4. The arm site short-circuits on the in-process fired latch BEFORE the fresh config read — post-fired batch rows no longer pay a per-row config read + parse + shared-cache invalidation for an answer module state already knows. 5. Replaced the new `as T` assertions in render.test.ts's config-state factory with an explicitly typed vi.hoisted return (repo TypeScript convention: no `as T`). config.test.ts: node:fs mock gains renameSync (faithful to the new atomic write); new test covers the success/failure return and asserts no temp file survives a write. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
119 lines
4.7 KiB
TypeScript
119 lines
4.7 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from "vitest";
|
|
|
|
// In-memory fake filesystem so these tests exercise the REAL config.ts
|
|
// module (parsing, caching, readConfigFresh's cache-bypass) without ever
|
|
// touching the developer/CI machine's actual ~/.hyperframes/config.json —
|
|
// homedir() is resolved once at config.ts's module-load time, so faking
|
|
// HOME via env var would only work in a fresh process, not inside a shared
|
|
// vitest worker.
|
|
const fsState = vi.hoisted(() => ({
|
|
files: new Map<string, string>(),
|
|
}));
|
|
|
|
vi.mock("node:fs", () => ({
|
|
existsSync: vi.fn((path: string) => fsState.files.has(path)),
|
|
mkdirSync: vi.fn(() => undefined),
|
|
readFileSync: vi.fn((path: string) => {
|
|
const content = fsState.files.get(path);
|
|
if (content === undefined) throw new Error(`ENOENT: ${path}`);
|
|
return content;
|
|
}),
|
|
writeFileSync: vi.fn((path: string, content: string) => {
|
|
fsState.files.set(path, content);
|
|
}),
|
|
// writeConfig writes atomically: temp file + rename over the target.
|
|
renameSync: vi.fn((from: string, to: string) => {
|
|
const content = fsState.files.get(from);
|
|
if (content === undefined) throw new Error(`ENOENT: ${from}`);
|
|
fsState.files.set(to, content);
|
|
fsState.files.delete(from);
|
|
}),
|
|
}));
|
|
|
|
describe("config.ts — readConfig / readConfigFresh / writeConfig (real module, faked fs)", () => {
|
|
let readConfig: typeof import("./config.js").readConfig;
|
|
let readConfigFresh: typeof import("./config.js").readConfigFresh;
|
|
let writeConfig: typeof import("./config.js").writeConfig;
|
|
let CONFIG_PATH: typeof import("./config.js").CONFIG_PATH;
|
|
|
|
beforeEach(async () => {
|
|
fsState.files.clear();
|
|
// Fresh module instance per test — config.ts's `cachedConfig` is
|
|
// module-scoped, so without this, a later test would silently inherit
|
|
// an earlier test's cached read.
|
|
vi.resetModules();
|
|
({ readConfig, readConfigFresh, writeConfig, CONFIG_PATH } = await import("./config.js"));
|
|
});
|
|
|
|
it("creates a default config with a fresh anonymousId when no file exists", () => {
|
|
const config = readConfig();
|
|
expect(config.telemetryEnabled).toBe(true);
|
|
expect(config.anonymousId).toBeTruthy();
|
|
expect(fsState.files.has(CONFIG_PATH)).toBe(true);
|
|
});
|
|
|
|
it("caches the read — a second readConfig() call does not see a file mutated out from under it", () => {
|
|
const first = readConfig();
|
|
fsState.files.set(CONFIG_PATH, JSON.stringify({ ...first, deParallelRouterTrialFired: true }));
|
|
const second = readConfig();
|
|
expect(second.deParallelRouterTrialFired).toBeUndefined();
|
|
});
|
|
|
|
it("readConfigFresh bypasses the cache and picks up a file written by another process", () => {
|
|
const first = readConfig();
|
|
fsState.files.set(CONFIG_PATH, JSON.stringify({ ...first, deParallelRouterTrialFired: true }));
|
|
const fresh = readConfigFresh();
|
|
expect(fresh.deParallelRouterTrialFired).toBe(true);
|
|
});
|
|
|
|
it("writeConfig updates the in-process cache so a subsequent readConfig() sees the write immediately", () => {
|
|
const config = readConfig();
|
|
config.deParallelRouterTrialRenderCount = 5;
|
|
writeConfig(config);
|
|
const reread = readConfig();
|
|
expect(reread.deParallelRouterTrialRenderCount).toBe(5);
|
|
});
|
|
|
|
it('treats a non-boolean deParallelRouterTrialFired (e.g. the JSON string "false") as unset, not truthy', () => {
|
|
const base = readConfig();
|
|
fsState.files.set(
|
|
CONFIG_PATH,
|
|
JSON.stringify({ ...base, deParallelRouterTrialFired: "false" }),
|
|
);
|
|
const fresh = readConfigFresh();
|
|
expect(fresh.deParallelRouterTrialFired).toBeUndefined();
|
|
});
|
|
|
|
it("treats a non-number deParallelRouterTrialRenderCount as unset", () => {
|
|
const base = readConfig();
|
|
fsState.files.set(
|
|
CONFIG_PATH,
|
|
JSON.stringify({ ...base, deParallelRouterTrialRenderCount: "5" }),
|
|
);
|
|
const fresh = readConfigFresh();
|
|
expect(fresh.deParallelRouterTrialRenderCount).toBeUndefined();
|
|
});
|
|
|
|
it("resets to defaults with a fresh anonymousId when the file is corrupted JSON", () => {
|
|
fsState.files.set(CONFIG_PATH, "{not valid json");
|
|
const config = readConfig();
|
|
expect(config.telemetryEnabled).toBe(true);
|
|
expect(config.anonymousId).toBeTruthy();
|
|
});
|
|
|
|
it("writeConfig reports success, leaves no temp file behind, and reports failure when the fs throws", async () => {
|
|
const config = readConfig();
|
|
expect(writeConfig(config)).toBe(true);
|
|
// Atomic write: the pid-suffixed temp file must have been renamed away.
|
|
for (const path of fsState.files.keys()) {
|
|
expect(path.endsWith(".tmp")).toBe(false);
|
|
}
|
|
|
|
const fs = await import("node:fs");
|
|
vi.mocked(fs.writeFileSync).mockImplementationOnce(() => {
|
|
throw new Error("EACCES: permission denied");
|
|
});
|
|
expect(writeConfig(config)).toBe(false);
|
|
});
|
|
});
|