fix(cli): keep a set-but-empty router env var breaker-managed (review)

Ownership detection classified ANY defined HF_DE_PARALLEL_ROUTER as a user
choice, but both parsers read empty/whitespace as "unset -> default ON".
Launching with `HF_DE_PARALLEL_ROUTER=` therefore routed the render (empty
parses as ON) while exempting the install from its circuit breaker: after a
verified fallback applyDeParallelRouterBreaker() no-op'd, so the install kept
retrying the failing router instead of latching off. That is the exact
first-fallback protection this PR exists to provide, lost on a documented
default path. Ownership now uses the same normalization as the parsers.

Also: only announce a trip the breaker could act on. With an explicit user
opt-in the breaker is deliberately a no-op, so "now off for this install" was
factually wrong — and reprinted on every later revert, since the user's value
keeps the router active.

Tests: set-but-empty and whitespace both latch off and persist the fired flag
(fault-injection verified — restoring the old check fails both); explicit
"true" survives a fallback.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: vanceingalls

packages/cli/src/commands/render.test.ts

@@ -795,6 +795,51 @@ describe("renderLocal — DE parallel-router circuit breaker", () => {
     expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
   });
 
+  for (const emptyish of ["", "   "]) {
+    it(`treats a set-but-empty env var (${JSON.stringify(emptyish)}) as default, not a user choice`, async () => {
+      // Both parsers read empty/whitespace as "unset → default ON", so the
+      // producer routes. If ownership instead treated any defined value as a
+      // user choice, the breaker would no-op and this install would keep
+      // retrying a failing router forever — losing the first-fallback
+      // protection that is the point of the breaker.
+      configState.disk = {
+        telemetryEnabled: true,
+        deParallelRouterTrialFired: false,
+        telemetryNoticeShown: true,
+      };
+      process.env.HF_DE_PARALLEL_ROUTER = emptyish;
+      producerState.executeImpl = async (job) => {
+        job.perfSummary = {
+          resolution: { width: 100, height: 100 },
+          drawElement: { parallelRouter: "reverted" },
+        };
+      };
+      await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
+      expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("false");
+      expect(configState.writeConfigCalls).toContainEqual(
+        expect.objectContaining({ deParallelRouterTrialFired: true }),
+      );
+    });
+  }
+
+  it("does not override an explicit user opt-in even after a fallback", async () => {
+    // "Explicit user choice wins in both directions" — the opt-in half.
+    configState.disk = {
+      telemetryEnabled: true,
+      deParallelRouterTrialFired: false,
+      telemetryNoticeShown: true,
+    };
+    process.env.HF_DE_PARALLEL_ROUTER = "true";
+    producerState.executeImpl = async (job) => {
+      job.perfSummary = {
+        resolution: { width: 100, height: 100 },
+        drawElement: { parallelRouter: "reverted" },
+      };
+    };
+    await renderLocal("/tmp/project", "/tmp/out.mp4", baseOptions);
+    expect(process.env.HF_DE_PARALLEL_ROUTER).toBe("true");
+  });
+
   it("keeps the router on for a telemetry opt-out — analytics choice must not cost performance", async () => {
     // The old trial refused to arm without recordable telemetry (no point
     // running an experiment you can't measure). Now that the router is a

packages/cli/src/commands/render.ts

@@ -68,7 +68,6 @@ import {
   writeConfig,
   type HyperframesConfig,
 } from "../telemetry/config.js";
-import { shouldTrack } from "../telemetry/client.js";
 import { renderJobObservabilityTelemetryPayload } from "../telemetry/renderObservability.js";
 import { bytesToMb } from "../telemetry/system.js";
 import { VERSION } from "../version.js";
@@ -1184,8 +1183,16 @@ function applyDeParallelRouterBreaker(): void {
 function applyDeParallelRouterCircuitBreaker(quiet: boolean): boolean {
   // Latch the user's own choice on first observation, BEFORE the breaker can
   // write the var itself and make the two indistinguishable.
+  //
+  // Ownership uses the SAME normalization as the two parsers: a set-but-empty
+  // (or whitespace) value means "unset / default ON", so it is NOT a user
+  // choice and must stay breaker-managed. Treating any defined value as
+  // user-managed would let `HF_DE_PARALLEL_ROUTER=` route the render (empty
+  // parses as ON) while exempting that install from the breaker — it would
+  // keep retrying a failing router forever, losing exactly the first-fallback
+  // protection this PR exists to provide (review finding).
   if (!deParallelRouterUserManagedResolved) {
-    deParallelRouterUserManaged = process.env.HF_DE_PARALLEL_ROUTER !== undefined;
+    deParallelRouterUserManaged = (process.env.HF_DE_PARALLEL_ROUTER ?? "").trim() !== "";
     deParallelRouterUserManagedResolved = true;
   }
   if (deParallelRouterUserManaged) {
@@ -1315,7 +1322,11 @@ function maybeConsumeDeParallelRouterTrial(
     applyDeParallelRouterBreaker();
   }
   writeConfig(config);
-  if (fired) reportDeParallelRouterBreakerTrip(quiet);
+  // Only announce a trip the breaker could actually act on. With an explicit
+  // user opt-in the breaker is a no-op, so "now off for this install" would
+  // be false — and would reprint on every subsequent revert, since the user's
+  // value keeps the router active (review finding).
+  if (fired && !deParallelRouterUserManaged) reportDeParallelRouterBreakerTrip(quiet);
 }
 
 /**