fix(cli): stale-cache arm reads, retry double-count, and unwritable-config re-arm in DE trial

Three root causes from a fourth max-effort review (15 raw findings deduped;
the synthesize step died on a session limit so they arrived unmerged):

1. The previous commit's telemetryEnabled fix was ineffective: the arm site
   passed readConfig() — the process-lifetime cache — into
   isDeParallelRouterTrialBlocked, making it exactly as stale as the
   shouldTrack() memoization it claimed to bypass. A mid-batch
   `hyperframes telemetry off` (or another process persisting fired=true)
   was never observed. Now reads readConfigFresh() at the arm site; the
   test mock previously hid this because readConfig/readConfigFresh were
   behaviorally identical views over one shared object.

2. The verify-and-retry write loop double-counted a render whenever OUR
   write landed but a concurrent writer advanced the file before our
   verify read — the retry re-applied the increment on top (two renders
   → three counts), tripping the 25-render exposure cap early and
   permanently killing the trial with less telemetry than the cap was
   designed to allow. Reworked: the render COUNTER is written exactly
   once, unverified (a lost increment under-counts by one — benign); only
   the FIRED flag is verified and re-asserted, which is idempotent, so
   retries can no longer corrupt anything
   (persistDeParallelRouterTrialFired).

3. writeConfig swallows all fs errors, so on an unwritable ~/.hyperframes
   a reverted outcome could never persist — the trial would re-arm and
   re-fail on every subsequent render forever, silently. Added an
   in-process fired latch (set at decision time, before persistence is
   attempted) consulted by the blocked-check, plus a one-time console
   warning when persistence exhausts its attempts. Later processes still
   re-arm (disk is the only cross-process channel), but each process now
   stops after at most one failure it couldn't record.

Test infrastructure fix enabling all of the above to be tested: the config
mock now models disk vs cache SEPARATELY (readConfig serves the cache,
readConfigFresh re-reads "disk", writeConfig updates both) with a
failWrites hook simulating the real writeConfig's silent error swallowing.
The old single-shared-object mock made cached-vs-fresh mis-routing and
retry iterations untestable by construction.

3 new regression tests: mid-batch opt-out observed through the cache;
fired flag re-asserted after a lost write WITHOUT re-counting the render;
unwritable-config latch blocking re-arm. 56 tests total across
render.test.ts + config.test.ts.

Not fixed (by design): the widened pinned-fallback retry paying a doubled
render on deterministic mid-stream failures (e.g. ENOSPC) — the accepted
tradeoff of the fallback design; cancellation and OOM are special-cased.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

packages/cli/src/commands/render.ts

@@ -66,12 +66,7 @@ import {
   trackRenderPreflightRejected,
 } from "../telemetry/events.js";
 import { maybePromptRenderFeedback } from "../telemetry/feedback.js";
-import {
-  readConfig,
-  readConfigFresh,
-  writeConfig,
-  type HyperframesConfig,
-} from "../telemetry/config.js";
+import { readConfigFresh, writeConfig, type HyperframesConfig } from "../telemetry/config.js";
 import { shouldTrack } from "../telemetry/client.js";
 import { renderJobObservabilityTelemetryPayload } from "../telemetry/renderObservability.js";
 import { normalizeSkillSlug } from "../telemetry/skill.js";
@@ -1655,32 +1650,51 @@ const DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS = 25;
 let deParallelRouterTrialManagedByUs = false;
 
 /**
- * Test-only reset for `deParallelRouterTrialManagedByUs` — a real CLI
- * process only ever runs one `--batch` sequence, so this state never needs
+ * In-process latch mirroring `deParallelRouterTrialFired`: set the moment we
+ * DECIDE the trial is over, independent of whether persisting that decision
+ * to `~/.hyperframes/config.json` succeeds. `writeConfig` swallows all fs
+ * errors (by design — telemetry must never break the CLI), so on an
+ * unwritable config (root-owned file, disk full) the fired flag can never
+ * stick on disk; without this latch the trial would silently re-arm and
+ * re-fail on every subsequent render in this process forever (review
+ * finding). Later processes still re-arm — disk is the only cross-process
+ * channel — but each process now stops after at most one failure it
+ * couldn't record.
+ */
+let deParallelRouterTrialFiredThisProcess = false;
+
+/**
+ * Test-only reset for the module-level trial state — a real CLI process
+ * only ever runs one `--batch` sequence, so this state never needs
  * resetting outside a test process where many independent test cases share
  * one imported module instance.
  */
 // fallow-ignore-next-line unused-export
 export function __resetDeParallelRouterTrialStateForTests(): void {
   deParallelRouterTrialManagedByUs = false;
+  deParallelRouterTrialFiredThisProcess = false;
 }
 
 /**
- * True once the trial should stop offering itself: already failed, hit the
- * render-count backstop, or telemetry isn't actually recordable right now.
+ * True once the trial should stop offering itself: already failed (on disk
+ * or via this process's in-memory latch), hit the render-count backstop, or
+ * telemetry isn't actually recordable right now.
  *
  * Checks BOTH `shouldTrack()` and `config.telemetryEnabled` directly, not
  * `shouldTrack()` alone: `shouldTrack()` (`../telemetry/client.js`) memoizes
  * its verdict once per process and never invalidates, so during a long
  * `--batch` run (all rows share one process) a `hyperframes telemetry off`
- * issued from another terminal mid-batch would never be observed —
- * `config.telemetryEnabled` is read fresh from `readConfig()` on every call
- * here instead, closing that gap (review finding).
+ * issued from another terminal mid-batch would never be observed. The
+ * caller must pass a `readConfigFresh()` snapshot for the same reason —
+ * `readConfig()` serves a process-lifetime cache that is exactly as stale
+ * as the `shouldTrack()` memoization this check exists to bypass (review
+ * finding).
  */
 function isDeParallelRouterTrialBlocked(config: HyperframesConfig): boolean {
   const overRenderCap =
     (config.deParallelRouterTrialRenderCount ?? 0) >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS;
   return (
+    deParallelRouterTrialFiredThisProcess ||
     Boolean(config.deParallelRouterTrialFired) ||
     overRenderCap ||
     !config.telemetryEnabled ||
@@ -1731,7 +1745,11 @@ function maybeEnableDeParallelRouterTrial(quiet: boolean, disabled: boolean): bo
     process.env.HF_DE_PARALLEL_ROUTER !== undefined && !deParallelRouterTrialManagedByUs;
   if (userSetIt) return false;
 
-  if (isDeParallelRouterTrialBlocked(readConfig())) {
+  // readConfigFresh, NOT readConfig: the cached read is exactly as stale as
+  // the shouldTrack() memoization the blocked-check exists to bypass — a
+  // mid-batch `hyperframes telemetry off` (or another process persisting
+  // fired=true) would never be observed through the cache (review finding).
+  if (isDeParallelRouterTrialBlocked(readConfigFresh())) {
     stopManagingDeParallelRouterTrial();
     return false;
   }
@@ -1751,6 +1769,47 @@ function maybeEnableDeParallelRouterTrial(quiet: boolean, disabled: boolean): bo
   return true;
 }
 
+/**
+ * The router outcome for this render, or undefined when the router never
+ * engaged. `perfSummary.drawElement.parallelRouter` is NEVER undefined on
+ * the success path — aggregateDrawElement (perfSummary.ts) defaults it to
+ * the string "none" for every render, whether or not drawElement/the router
+ * ever engaged. Normalizing "none" to undefined here is required, not
+ * optional: without it, ordinary renders below the router's own frame
+ * threshold (the common case) would tick the render-count backstop on every
+ * single render and trip DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS after 25
+ * completely unrelated renders that never touched the router (review
+ * finding).
+ */
+function resolveDeParallelRouterOutcome(job: RenderJob): string | undefined {
+  const outcome =
+    job.perfSummary?.drawElement?.parallelRouter ??
+    job.errorDetails?.observability?.capture.deParallelRouter;
+  return outcome === "none" ? undefined : outcome;
+}
+
+/**
+ * Persist `deParallelRouterTrialFired: true`, verifying against a fresh
+ * disk read that it actually stuck, and re-asserting if a concurrent
+ * writer's stale snapshot clobbered it. ONLY the fired flag is retried —
+ * re-asserting a boolean is idempotent, so retries can't corrupt anything,
+ * unlike the render counter (a re-applied increment double-counts the
+ * render when our write landed but a later concurrent write raced our
+ * verify read — review finding). Returns false when every attempt failed
+ * to stick (e.g. `writeConfig` silently swallowing fs errors on an
+ * unwritable `~/.hyperframes`).
+ */
+function persistDeParallelRouterTrialFired(): boolean {
+  const MAX_ATTEMPTS = 3;
+  for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) {
+    const config = readConfigFresh();
+    if (config.deParallelRouterTrialFired) return true;
+    config.deParallelRouterTrialFired = true;
+    writeConfig(config);
+  }
+  return Boolean(readConfigFresh().deParallelRouterTrialFired);
+}
+
 /**
  * After a trial-armed render, persist that the router's OWN bet actually
  * failed — its self-verify/generic-failure safety net fired
@@ -1770,59 +1829,40 @@ function maybeEnableDeParallelRouterTrial(quiet: boolean, disabled: boolean): bo
  * router never became eligible for this render (e.g. too few frames): the
  * trial stays available for a future run either way, uncounted.
  *
- * Re-reads the config fresh from disk immediately before writing (bypassing
- * the in-process read cache) and verifies the write actually stuck against
- * another fresh read, retrying against whatever a concurrent writer left
- * behind if not (up to a few attempts) — this repo has no real cross-process
- * file locking, so it's still possible for two truly simultaneous writers to
- * race past each other, but it closes the common case where a concurrently-
- * running CLI process (another terminal, a parallel script) would otherwise
- * silently revert this write with its own stale snapshot (review finding).
+ * Cross-process race semantics (no file locking exists here): the render
+ * COUNTER is written exactly once, unverified — a lost increment under a
+ * concurrent-writer race just under-counts the exposure cap by one
+ * (benign), whereas retrying it would double-count this render whenever our
+ * write actually landed but another writer raced the verify read (trips the
+ * cap early, killing the trial prematurely — review finding). The FIRED
+ * flag is the safety-critical bit and IS verified/re-asserted — see
+ * `persistDeParallelRouterTrialFired`.
  */
-/**
- * Apply this render's outcome to a fresh config snapshot: increment the
- * render-count backstop, and flip `deParallelRouterTrialFired` (+ stop
- * managing the env var) if the router's own safety net fired or the cap was
- * reached. Pure mutation, no I/O — kept separate from the retry loop below
- * so each stays simple enough for the repo's own complexity gate.
- */
-function applyDeParallelRouterOutcome(config: HyperframesConfig, outcome: string): void {
-  const renderCount = (config.deParallelRouterTrialRenderCount ?? 0) + 1;
-  config.deParallelRouterTrialRenderCount = renderCount;
-  if (outcome === "reverted" || renderCount >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS) {
-    config.deParallelRouterTrialFired = true;
-    stopManagingDeParallelRouterTrial();
-  }
-}
-
 function maybeConsumeDeParallelRouterTrial(trialArmed: boolean, job: RenderJob): void {
   if (!trialArmed) return;
-  const outcome =
-    job.perfSummary?.drawElement?.parallelRouter ??
-    job.errorDetails?.observability?.capture.deParallelRouter;
-  // perfSummary.drawElement.parallelRouter is NEVER undefined on the success
-  // path — aggregateDrawElement (perfSummary.ts) defaults it to the string
-  // "none" for every render, whether or not drawElement/the router ever
-  // engaged. Treating "none" the same as undefined here is required, not
-  // optional: without it, ordinary renders below the router's own frame
-  // threshold (the common case) would tick the render-count backstop on
-  // every single render and trip DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS after
-  // 25 completely unrelated renders that never touched the router at all
-  // (review finding).
-  if (outcome === undefined || outcome === "none") return;
+  const outcome = resolveDeParallelRouterOutcome(job);
+  if (outcome === undefined) return;
 
-  const MAX_WRITE_ATTEMPTS = 3;
-  for (let attempt = 0; attempt < MAX_WRITE_ATTEMPTS; attempt++) {
-    const config = readConfigFresh();
-    applyDeParallelRouterOutcome(config, outcome);
-    writeConfig(config);
-    const verify = readConfigFresh();
-    const stuck =
-      verify.deParallelRouterTrialRenderCount === config.deParallelRouterTrialRenderCount &&
-      verify.deParallelRouterTrialFired === config.deParallelRouterTrialFired;
-    if (stuck) return;
-    // A concurrent writer landed between our write and this verify read —
-    // retry against whatever they left, re-applying our own mutation on top.
+  const config = readConfigFresh();
+  const renderCount = (config.deParallelRouterTrialRenderCount ?? 0) + 1;
+  config.deParallelRouterTrialRenderCount = renderCount;
+  const fired = outcome === "reverted" || renderCount >= DE_PARALLEL_ROUTER_TRIAL_MAX_RENDERS;
+  if (fired) {
+    config.deParallelRouterTrialFired = true;
+    // Latch BEFORE attempting persistence — the decision holds for this
+    // process even if the disk write never sticks (unwritable config).
+    deParallelRouterTrialFiredThisProcess = true;
+    stopManagingDeParallelRouterTrial();
+  }
+  writeConfig(config);
+  if (fired && !persistDeParallelRouterTrialFired()) {
+    console.warn(
+      c.warn(
+        "  Could not persist the parallel drawElement trial's off-switch to " +
+          "~/.hyperframes/config.json (unwritable?). The experiment stays off for this " +
+          "process; future runs may retry it. Set HF_DE_PARALLEL_ROUTER=false to opt out.",
+      ),
+    );
   }
 }