mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 23:03:09 +00:00
fix(cli): atomic config writes, gated trial warning, and write-failure signal
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>
This commit is contained in:
co-authored by
Claude Fable 5
parent
2542e94277
commit
6172d79dc2
@@ -21,6 +21,13 @@ vi.mock("node:fs", () => ({
|
||||
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)", () => {
|
||||
@@ -93,4 +100,19 @@ describe("config.ts — readConfig / readConfigFresh / writeConfig (real module,
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs";
|
||||
import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { homedir } from "node:os";
|
||||
import { randomUUID } from "node:crypto";
|
||||
@@ -162,15 +162,32 @@ export function readConfigFresh(): HyperframesConfig {
|
||||
}
|
||||
|
||||
/**
|
||||
* Persist config to disk. Updates the in-memory cache.
|
||||
* Persist config to disk. Updates the in-memory cache on success.
|
||||
*
|
||||
* Atomic: writes to a pid-suffixed temp file and renames it over the config —
|
||||
* `rename(2)` within one directory is atomic on POSIX, so a concurrent
|
||||
* reader can never observe a partially-written file. That matters beyond
|
||||
* hygiene: `readConfig`'s corrupted-file catch RESETS the config to defaults
|
||||
* (new anonymousId, telemetry re-enabled, all optional fields wiped), so a
|
||||
* torn read of a non-atomic write would silently destroy the user's config
|
||||
* (review finding).
|
||||
*
|
||||
* Returns whether the write actually landed — errors are still swallowed
|
||||
* (telemetry must never break the CLI), but callers that need persistence
|
||||
* certainty (e.g. the DE parallel-router trial's off-switch) can react
|
||||
* instead of re-implementing read-back verification.
|
||||
*/
|
||||
export function writeConfig(config: HyperframesConfig): void {
|
||||
export function writeConfig(config: HyperframesConfig): boolean {
|
||||
try {
|
||||
mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 });
|
||||
writeFileSync(CONFIG_FILE, JSON.stringify(config, null, 2) + "\n", { mode: 0o600 });
|
||||
const tmpFile = `${CONFIG_FILE}.${process.pid}.tmp`;
|
||||
writeFileSync(tmpFile, JSON.stringify(config, null, 2) + "\n", { mode: 0o600 });
|
||||
renameSync(tmpFile, CONFIG_FILE);
|
||||
cachedConfig = { ...config };
|
||||
return true;
|
||||
} catch {
|
||||
// Non-fatal — telemetry should never break the CLI
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user