fix(os-summary): cache Darwin sw_vers product version across prompt callers (#95145)

Sanjays2402 · Sanjays2402 · commit 64677b034305 · 2026-06-19T22:58:53.000-07:00
Addresses the reviewer P1 from clawsweeper on PR #95189: resolveRuntimePromptOs() is invoked from embedded attempt, compaction, and CLI runtime-prompt callers on every prompt build, so the Darwin branch was synchronously spawning sw_vers once per prompt without reuse. resolveOsSummary() already caches its full summary by platform/release/arch tuple, but the underlying macosProductVersion() helper spawned sw_vers unconditionally on each call — so resolveRuntimePromptOs's direct call to macosProductVersion() bypassed the existing cache entirely. Add a module-level cachedMacosProductVersionByRelease map keyed by os.release(). The macOS marketing product version is stable for the life of a process for a given kernel release; once resolved, both resolveOsSummary() and resolveRuntimePromptOs() reuse the cached string and no longer re-spawn sw_vers. Non-Darwin output is unchanged. Also export a test-only __resetOsSummaryCachesForTests() helper that clears both module-level caches, and wire beforeEach() hooks into the existing test file. Without the reset, the new cache exposes a pre-existing test-isolation gap — the 'falls back to os.release on darwin when sw_vers returns blank' case reuses kernel release 25.5.0 from an earlier case that already cached '26.5.1', so the cache would return the earlier mock and the fallback case would never exercise its fallback. The reset hook makes each test case authoritative for its own mock. Two new cache regression tests: - reuses cached macOS product version across repeated calls (asserts spawnSync is called exactly once across 3 invocations) - runtime-prompt and os-summary share the cached Darwin product version (asserts resolveRuntimePromptOs called after resolveOsSummary for the same kernel release does not re-spawn sw_vers — the two helpers share the module-level cache) Verification: - node scripts/run-vitest.mjs src/infra/os-summary.test.ts src/agents/system-prompt.test.ts src/agents/embedded-agent-runner/run/attempt-system-prompt.test.ts → all 3 shards pass (11 + 1 + 4 tests) - pnpm tsgo:core → clean - pnpm tsgo:core:test → clean
diff --git a/src/infra/os-summary.test.ts b/src/infra/os-summary.test.ts
@@ -1,6 +1,6 @@
 // Tests operating system summary collection and normalization.
 import os from "node:os";
-import { afterEach, describe, expect, it, vi } from "vitest";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 
 const spawnSyncMock = vi.hoisted(() => vi.fn());
 
@@ -11,7 +11,11 @@ vi.mock("node:child_process", async () => {
   );
 });
 
-import { resolveOsSummary, resolveRuntimePromptOs } from "./os-summary.js";
+import {
+  __resetOsSummaryCachesForTests,
+  resolveOsSummary,
+  resolveRuntimePromptOs,
+} from "./os-summary.js";
 
 type OsSummaryCase = {
   name: string;
@@ -23,6 +27,9 @@ type OsSummaryCase = {
 };
 
 describe("resolveOsSummary", () => {
+  beforeEach(() => {
+    __resetOsSummaryCachesForTests();
+  });
   afterEach(() => {
     vi.restoreAllMocks();
   });
@@ -106,6 +113,9 @@ type RuntimePromptOsCase = {
 };
 
 describe("resolveRuntimePromptOs", () => {
+  beforeEach(() => {
+    __resetOsSummaryCachesForTests();
+  });
   afterEach(() => {
     vi.restoreAllMocks();
   });
@@ -166,3 +176,72 @@ describe("resolveRuntimePromptOs", () => {
     expect(resolveRuntimePromptOs()).toBe(expected);
   });
 });
+
+describe("resolveRuntimePromptOs caching", () => {
+  beforeEach(() => {
+    __resetOsSummaryCachesForTests();
+  });
+  afterEach(() => {
+    vi.restoreAllMocks();
+  });
+
+  it("reuses cached macOS product version across repeated calls (PR #95189 reviewer P1)", () => {
+    // Use a kernel release no other test case in this file uses, so the
+    // module-level cache miss is exclusively this test's responsibility.
+    vi.spyOn(os, "platform").mockReturnValue("darwin");
+    vi.spyOn(os, "release").mockReturnValue("25.7.0");
+    spawnSyncMock.mockReturnValue({
+      stdout: "26.7.0\n",
+      stderr: "",
+      pid: 1,
+      output: [],
+      status: 0,
+      signal: null,
+    });
+
+    const callsBefore = spawnSyncMock.mock.calls.length;
+    const first = resolveRuntimePromptOs();
+    const callsAfterFirst = spawnSyncMock.mock.calls.length;
+    const second = resolveRuntimePromptOs();
+    const third = resolveRuntimePromptOs();
+    const callsAfterThird = spawnSyncMock.mock.calls.length;
+
+    expect(first).toBe("macOS 26.7.0");
+    expect(second).toBe("macOS 26.7.0");
+    expect(third).toBe("macOS 26.7.0");
+    // First call must spawn sw_vers exactly once; subsequent calls must reuse
+    // the cached product version and not re-spawn the subprocess.
+    expect(callsAfterFirst - callsBefore).toBe(1);
+    expect(callsAfterThird - callsAfterFirst).toBe(0);
+  });
+
+  it("runtime-prompt and os-summary share the cached Darwin product version", () => {
+    // Same kernel release across both call paths: the second path must not
+    // spawn sw_vers again, because the cache is keyed by os.release().
+    vi.spyOn(os, "platform").mockReturnValue("darwin");
+    vi.spyOn(os, "release").mockReturnValue("25.8.0");
+    vi.spyOn(os, "arch").mockReturnValue("arm64");
+    spawnSyncMock.mockReturnValue({
+      stdout: "26.8.0\n",
+      stderr: "",
+      pid: 1,
+      output: [],
+      status: 0,
+      signal: null,
+    });
+
+    const callsBefore = spawnSyncMock.mock.calls.length;
+    const summaryLabel = resolveOsSummary().label;
+    const callsAfterSummary = spawnSyncMock.mock.calls.length;
+    const runtimeLabel = resolveRuntimePromptOs();
+    const callsAfterRuntime = spawnSyncMock.mock.calls.length;
+
+    expect(summaryLabel).toBe("macos 26.8.0 (arm64)");
+    expect(runtimeLabel).toBe("macOS 26.8.0");
+    expect(callsAfterSummary - callsBefore).toBe(1);
+    // resolveRuntimePromptOs called after resolveOsSummary for the same
+    // kernel release must not re-spawn sw_vers — the two helpers share the
+    // module-level cachedMacosProductVersionByRelease map.
+    expect(callsAfterRuntime - callsAfterSummary).toBe(0);
+  });
+});
diff --git a/src/infra/os-summary.ts b/src/infra/os-summary.ts
@@ -12,10 +12,39 @@ type OsSummary = {
 
 const cachedOsSummaryByKey = new Map<string, OsSummary>();
 
+/**
+ * Cache for the slow Darwin `sw_vers -productVersion` lookup, keyed by
+ * `os.release()` (the kernel release a given binary is observing). Once
+ * resolved for a given kernel release, the macOS marketing product version
+ * is stable for the lifetime of the process — there is no scenario where
+ * macOS changes its product version without the kernel release changing
+ * too — so re-spawning `sw_vers` per call only burns latency on every
+ * runtime prompt build (#95145 review feedback on PR #95189).
+ */
+const cachedMacosProductVersionByRelease = new Map<string, string>();
+
 function macosProductVersion(): string {
+  const release = os.release();
+  const cached = cachedMacosProductVersionByRelease.get(release);
+  if (cached !== undefined) {
+    return cached;
+  }
   const res = spawnSync("sw_vers", ["-productVersion"], { encoding: "utf-8" });
   const out = normalizeOptionalString(res.stdout) ?? "";
-  return out || os.release();
+  const resolved = out || release;
+  cachedMacosProductVersionByRelease.set(release, resolved);
+  return resolved;
+}
+
+/**
+ * Test-only: clear both module-level caches so each test case can mock a
+ * different `os.release()` without leaking the previous case's resolved
+ * Darwin marketing version. Not part of the public API — prefer keying tests
+ * by unique kernel releases when possible.
+ */
+export function __resetOsSummaryCachesForTests(): void {
+  cachedOsSummaryByKey.clear();
+  cachedMacosProductVersionByRelease.clear();
 }
 
 /** Resolves a compact OS label for diagnostics, logs, and environment summaries. */
