fix(gitops): scope credential walker, harden state and retries

Ports the bug fixes landed in gitops-mudflap to this repo. The push
pipeline had a class of bugs that surfaced as cryptic POST validation
errors and silent state corruption.

src/credentials.ts
  Rewrite credential resolution to walk only `credentialId` and
  `credentialIds` keys instead of every string in the payload. The old
  walker swapped any string matching a credential-name slug with its
  UUID, which collided with provider enum values like `openai`,
  `langfuse`, `11labs`, `cartesia` — those appear in `model.provider`,
  `voice.provider`, `voice.fallbackPlan.voices[].provider`, and
  `observabilityPlan.provider`, all of which the API enforces as enums
  on POST. PATCH silently tolerated the UUID-replaced fields, which
  hid the bug for resources that already existed.

  Also add a plain-object guard (Date/Map/Set/Buffer pass through) and
  a WeakSet to short-circuit cycles.

src/state.ts
  Atomic write via tmp file + rename so a crash mid-write leaves the
  prior state intact instead of truncating it. Loud throw on JSON
  parse error: silently falling back to an empty state would cause
  the next push to recreate every remote resource as new, duplicating
  the entire org.

src/push.ts
  Wrap the apply body in try/finally so saveState always runs. If a
  5xx kills the run after a few resources got UUIDs from the API, we
  still record them locally — otherwise the next run creates duplicates.

src/cleanup.ts
  Double-gate destructive cleanup: --force now also requires
  `--confirm <env>` matching VAPI_ENV. Refuse to run if the state
  file has zero tracked resources (almost always a fresh clone or
  bootstrap-not-run, where deletion would wipe the org).

src/pull.ts
  Preserve `null` values in cleanResource — `null` is the API's way
  to express "this field is intentionally cleared", which is different
  from "field absent". Stripping it caused round-trip drift.

  Switch git status parsing to `--porcelain -z` so filenames with
  spaces, quotes, or newlines parse correctly.

src/api.ts
  Extend the retry loop to cover 5xx responses (transient gateway
  errors, deploys in progress) in addition to 429. A single 502
  shouldn't force the operator to re-run the whole push.

src/resources.ts
  Sort directory entries before iteration so push order is stable
  across filesystems (APFS sorts, ext4 doesn't). Non-deterministic
  ordering makes duplicate-resourceId bugs hard to reproduce.

.gitignore
  Add .claude/ (local agent lock state).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: dhruva-vapi

.gitignore

@@ -23,3 +23,6 @@ Thumbs.db
 
 # Logs
 *.log
+
+# Local agent state
+.claude/

src/api.ts

@@ -46,6 +46,13 @@ async function throttle(): Promise<void> {
   lastRequestTime = Date.now();
 }
 
+// 429 = rate limit. 5xx = transient server error (gateway timeout, upstream
+// hiccup, deploy in progress). Both are worth retrying with backoff; surfacing
+// a 502 as a hard failure forces the operator to re-run the entire push.
+function shouldRetry(status: number): boolean {
+  return status === 429 || (status >= 500 && status < 600);
+}
+
 export async function vapiRequest<T = VapiResponse>(
   method: "POST" | "PATCH",
   endpoint: string,
@@ -68,10 +75,10 @@ export async function vapiRequest<T = VapiResponse>(
       return response.json() as Promise<T>;
     }
 
-    // Handle rate limit with retry
-    if (response.status === 429 && attempt < MAX_RETRIES) {
+    if (shouldRetry(response.status) && attempt < MAX_RETRIES) {
       const delay = INITIAL_DELAY_MS * Math.pow(2, attempt);
-      console.log(`  ⏳ Rate limited, retrying in ${delay / 1000}s (attempt ${attempt + 1}/${MAX_RETRIES})...`);
+      const reason = response.status === 429 ? "Rate limited" : `Server error ${response.status}`;
+      console.log(`  ⏳ ${reason}, retrying in ${delay / 1000}s (attempt ${attempt + 1}/${MAX_RETRIES})...`);
       await sleep(delay);
       continue;
     }
@@ -99,10 +106,10 @@ export async function vapiDelete(endpoint: string): Promise<void> {
       return;
     }
 
-    // Handle rate limit with retry
-    if (response.status === 429 && attempt < MAX_RETRIES) {
+    if (shouldRetry(response.status) && attempt < MAX_RETRIES) {
       const delay = INITIAL_DELAY_MS * Math.pow(2, attempt);
-      console.log(`  ⏳ Rate limited, retrying in ${delay / 1000}s (attempt ${attempt + 1}/${MAX_RETRIES})...`);
+      const reason = response.status === 429 ? "Rate limited" : `Server error ${response.status}`;
+      console.log(`  ⏳ ${reason}, retrying in ${delay / 1000}s (attempt ${attempt + 1}/${MAX_RETRIES})...`);
       await sleep(delay);
       continue;
     }

src/cleanup.ts

@@ -68,8 +68,19 @@ interface VapiResource {
   name?: string;
 }
 
+function readConfirmToken(argv: string[]): string | undefined {
+  // Accept either `--confirm <env>` or `--confirm=<env>`.
+  for (let i = 0; i < argv.length; i++) {
+    const arg = argv[i];
+    if (arg === "--confirm") return argv[i + 1];
+    if (arg?.startsWith("--confirm=")) return arg.slice("--confirm=".length);
+  }
+  return undefined;
+}
+
 async function main(): Promise<void> {
   const dryRun = !process.argv.includes("--force");
+  const confirmToken = readConfirmToken(process.argv);
 
   console.log(
     "═══════════════════════════════════════════════════════════════",
@@ -83,6 +94,20 @@ async function main(): Promise<void> {
     "═══════════════════════════════════════════════════════════════\n",
   );
 
+  // Destructive cleanup must be double-gated. `--force` alone is not enough
+  // because it is easy to set habitually or copy from another command where
+  // it has a different meaning. Require `--confirm <env>` so the caller has
+  // to name the environment they intend to wipe.
+  if (!dryRun && confirmToken !== VAPI_ENV) {
+    console.error(
+      `❌ Refusing to run destructive cleanup without explicit confirmation.`,
+    );
+    console.error(
+      `   Re-run with: npm run cleanup:${VAPI_ENV} -- --force --confirm ${VAPI_ENV}`,
+    );
+    process.exit(1);
+  }
+
   const state = loadState();
   const stateIds = new Set([
     ...Object.values(state.assistants),
@@ -95,6 +120,20 @@ async function main(): Promise<void> {
     ...Object.values(state.simulationSuites),
   ]);
 
+  // A state file with zero tracked resources is almost always a fresh clone,
+  // a corrupted state, or a bootstrap that has not written yet. Deleting from
+  // that baseline would wipe the org. Block it explicitly.
+  if (!dryRun && stateIds.size === 0) {
+    console.error(
+      `❌ Refusing to run destructive cleanup: state file has 0 tracked resources.`,
+    );
+    console.error(
+      `   This usually means the state was never bootstrapped. Run ` +
+        `\`npm run pull:${VAPI_ENV}:bootstrap\` first, then retry.`,
+    );
+    process.exit(1);
+  }
+
   console.log(`📄 State file has ${stateIds.size} resource IDs to keep\n`);
 
   const toDelete: {

src/credentials.ts

@@ -6,6 +6,12 @@ import type { StateFile } from "./types.ts";
 // Credentials are pulled from the API and stored in state (name-slug → UUID).
 // Resource files store credential NAMES (e.g., "roofr-server-credential").
 // Push resolves names → UUIDs. Pull resolves UUIDs → names.
+//
+// Replacement is scoped to `credentialId` / `credentialIds` fields only.
+// Credential slugs like `openai`, `langfuse`, `11labs` collide with enum
+// values for fields such as `model.provider`, `voice.provider`,
+// `observabilityPlan.provider`. A generic string-level walk would swap those
+// enum values with UUIDs and break POST validation.
 // ─────────────────────────────────────────────────────────────────────────────
 
 // Build UUID → name reverse map from state.credentials
@@ -27,28 +33,54 @@ export function credentialForwardMap(state: StateFile): Map<string, string> {
 }
 
 // ─────────────────────────────────────────────────────────────────────────────
-// Deep walk: replace string values matching the map keys
-// Works at any depth in any object/array structure
+// Scoped walker: replace values only at `credentialId` / `credentialIds` keys.
+// Works at any depth in any object/array structure.
 // ─────────────────────────────────────────────────────────────────────────────
 
-export function deepReplaceValues<T>(obj: T, replacements: Map<string, string>): T {
+export function replaceCredentialRefs<T>(
+  obj: T,
+  replacements: Map<string, string>,
+): T {
   if (replacements.size === 0) return obj;
-  return walk(obj, replacements) as T;
+  return walk(obj, replacements, new WeakSet()) as T;
 }
 
-function walk(value: unknown, replacements: Map<string, string>): unknown {
-  if (typeof value === "string") {
-    return replacements.get(value) ?? value;
-  }
+function isPlainObject(value: unknown): value is Record<string, unknown> {
+  if (value === null || typeof value !== "object") return false;
+  const proto = Object.getPrototypeOf(value);
+  // Only walk into objects produced by JSON parsing or object literals.
+  // Date/Map/Set/Buffer/etc. should pass through unchanged — recursing into
+  // them via Object.entries silently drops their prototype methods.
+  return proto === Object.prototype || proto === null;
+}
 
+function walk(
+  value: unknown,
+  replacements: Map<string, string>,
+  seen: WeakSet<object>,
+): unknown {
   if (Array.isArray(value)) {
-    return value.map((item) => walk(item, replacements));
+    if (seen.has(value)) return value;
+    seen.add(value);
+    return value.map((item) => walk(item, replacements, seen));
   }
 
-  if (value !== null && typeof value === "object") {
+  if (isPlainObject(value)) {
+    if (seen.has(value)) return value;
+    seen.add(value);
     const result: Record<string, unknown> = {};
-    for (const [key, val] of Object.entries(value as Record<string, unknown>)) {
-      result[key] = walk(val, replacements);
+    for (const [key, val] of Object.entries(value)) {
+      if (key === "credentialId" && typeof val === "string") {
+        result[key] = replacements.get(val) ?? val;
+      } else if (key === "credentialIds" && Array.isArray(val)) {
+        result[key] = val.map((item) =>
+          typeof item === "string"
+            ? (replacements.get(item) ?? item)
+            : walk(item, replacements, seen),
+        );
+      } else {
+        result[key] = walk(val, replacements, seen);
+      }
     }
     return result;
   }

src/pull.ts

@@ -14,7 +14,7 @@ import {
   BOOTSTRAP_SYNC,
 } from "./config.ts";
 import { loadState, saveState } from "./state.ts";
-import { credentialReverseMap, deepReplaceValues } from "./credentials.ts";
+import { credentialReverseMap, replaceCredentialRefs } from "./credentials.ts";
 import type { StateFile, ResourceType } from "./types.ts";
 
 // ─────────────────────────────────────────────────────────────────────────────
@@ -94,20 +94,28 @@ function gitHasCommits(): boolean {
   }
 }
 
-// Returns relative paths of all locally modified, deleted, or untracked files
+// Returns relative paths of all locally modified, deleted, or untracked files.
+// Uses `-z` (null-terminated) format so filenames containing spaces, newlines,
+// or quotes parse correctly without the ad-hoc quote/arrow stripping that the
+// plain porcelain format requires. With `-z`, renames emit two separate
+// null-terminated records: `XY new\0old\0` — we want `new`, so we consume
+// the record after any `R`/`C` status.
 function getLocallyChangedFiles(): Set<string> {
-  const status = gitCmd("status --porcelain");
+  const status = gitCmd("status --porcelain -z");
   const files = new Set<string>();
-  for (const line of status.split("\n")) {
-    if (!line.trim()) continue;
-    // format: XY filename  (or XY "filename" for special chars)
-    let filePath = line.slice(3);
-    // Handle renames: "old -> new"
-    const arrowIdx = filePath.indexOf(" -> ");
-    if (arrowIdx !== -1) filePath = filePath.slice(arrowIdx + 4);
-    // Strip quotes if present
-    filePath = filePath.replace(/^"|"$/g, "").trim();
-    files.add(filePath);
+  const records = status.split("\0");
+  for (let i = 0; i < records.length; i++) {
+    const record = records[i];
+    if (!record) continue;
+    // Each record is `XY <path>`. For R/C statuses, the NEXT record is the
+    // original path, which we skip.
+    const x = record[0];
+    const filePath = record.slice(3);
+    if (filePath) files.add(filePath);
+    if (x === "R" || x === "C") {
+      // Skip the following "from" path record
+      i++;
+    }
   }
   return files;
 }
@@ -323,12 +331,12 @@ function removeUuidMappings(
 function cleanResource(resource: VapiResource): Record<string, unknown> {
   const cleaned: Record<string, unknown> = {};
 
+  // Preserve `null` values: the API uses `null` to represent an intentionally
+  // cleared field (e.g. `voicemailMessage: null`), which is semantically
+  // different from an absent field. Stripping it on pull would cause the next
+  // push to drop the clear and re-apply any prior value still on the server.
   for (const [key, value] of Object.entries(resource)) {
-    if (
-      !EXCLUDED_FIELDS.includes(key) &&
-      value !== null &&
-      value !== undefined
-    ) {
+    if (!EXCLUDED_FIELDS.includes(key) && value !== undefined) {
       cleaned[key] = value;
     }
   }
@@ -708,7 +716,7 @@ export async function pullResourceType(
     // Clean, resolve resource references, and replace credential UUIDs with names
     const cleaned = cleanResource(resource);
     const resolved = resolveReferencesToResourceIds(cleaned, state);
-    const withCredNames = deepReplaceValues(resolved, credReverse);
+    const withCredNames = replaceCredentialRefs(resolved, credReverse);
 
     // Mark platform defaults so apply skips them
     if (isPlatformDefault) {

src/push.ts

@@ -14,7 +14,7 @@ import {
   resolveAssistantIds,
   extractReferencedIds,
 } from "./resolver.ts";
-import { credentialForwardMap, deepReplaceValues } from "./credentials.ts";
+import { credentialForwardMap, replaceCredentialRefs } from "./credentials.ts";
 import { deleteOrphanedResources } from "./delete.ts";
 import type {
   ResourceFile,
@@ -816,6 +816,11 @@ async function main(): Promise<void> {
     simulationSuites: 0,
   };
 
+  // From here on, any path out of the function (success OR thrown error) must
+  // flush the state file. If an early 5xx kills the apply after a few resources
+  // have been created on the remote, we still need their UUIDs recorded locally
+  // — otherwise the next run creates duplicates.
+  try {
   // Load all resources (we need them for reference resolution and filtering)
   console.log("\n📂 Loading resources...\n");
   const allToolsRaw = await loadResources<Record<string, unknown>>("tools");
@@ -860,7 +865,7 @@ async function main(): Promise<void> {
     resources: ResourceFile<T>[],
   ): ResourceFile<T>[] =>
     resources.map((r) => {
-      const resolved = deepReplaceValues(r.data, credMap);
+      const resolved = replaceCredentialRefs(r.data, credMap);
       warnUnresolvedCredentials(
         r.resourceId,
         resolved as Record<string, unknown>,
@@ -1147,9 +1152,6 @@ async function main(): Promise<void> {
     await updateStructuredOutputAssistantRefs(allAppliedOutputs, state);
   }
 
-  // Save updated state
-  await saveState(state);
-
   console.log(
     "\n═══════════════════════════════════════════════════════════════",
   );
@@ -1192,6 +1194,19 @@ async function main(): Promise<void> {
       `   Simulation Suites: ${Object.keys(state.simulationSuites).length}`,
     );
   }
+  } finally {
+    // Always flush state, even on partial failure — resources that already
+    // received UUIDs from the API must be recorded so the next run does not
+    // re-create them.
+    try {
+      await saveState(state);
+    } catch (saveError) {
+      console.error(
+        "\n⚠️  Failed to persist state file after apply:",
+        saveError instanceof Error ? saveError.message : saveError,
+      );
+    }
+  }
 }
 
 // Run the apply engine

src/resources.ts

@@ -67,7 +67,12 @@ function parseFrontmatter(content: string): {
  * Warns about unsupported files found in resource directories
  */
 async function scanDirectory(dir: string, baseDir: string): Promise<string[]> {
-  const entries = await readdir(dir);
+  // Sort entries so iteration order is identical across filesystems/CI runners.
+  // readdir() returns entries in OS-dependent order (APFS sorts, ext4 doesn't),
+  // and downstream push order affects which resource is created first when
+  // multiple files declare the same resourceId — non-determinism makes that
+  // bug class hard to reproduce.
+  const entries = (await readdir(dir)).slice().sort();
   const files: string[] = [];
 
   for (const entry of entries) {

src/state.ts

@@ -1,5 +1,5 @@
 import { existsSync, readFileSync } from "fs";
-import { writeFile } from "fs/promises";
+import { rename, writeFile } from "fs/promises";
 import { STATE_FILE_PATH, VAPI_ENV } from "./config.ts";
 import type { StateFile } from "./types.ts";
 
@@ -27,19 +27,37 @@ export function loadState(): StateFile {
     return createEmptyState();
   }
 
+  let content: unknown;
   try {
-    const content = JSON.parse(readFileSync(STATE_FILE_PATH, "utf-8"));
-    console.log(`📄 Loaded state file for environment: ${VAPI_ENV}`);
-    // Merge with empty state to ensure all keys exist (for backwards compatibility)
-    return { ...createEmptyState(), ...content } as StateFile;
+    content = JSON.parse(readFileSync(STATE_FILE_PATH, "utf-8"));
   } catch (error) {
-    console.error(`⚠️  Failed to parse state file, creating new: ${error}`);
-    return createEmptyState();
+    // Failing loudly here is deliberate. If we silently fall back to an empty
+    // state on a corrupted file, a subsequent push would treat every remote
+    // resource as new and create duplicates across the entire org.
+    const msg = error instanceof Error ? error.message : String(error);
+    throw new Error(
+      `Failed to parse state file at ${STATE_FILE_PATH}: ${msg}\n` +
+        `Refusing to continue — falling back to empty state would cause duplicate ` +
+        `resources on next push. Restore the file from git, or delete it manually ` +
+        `to start fresh after confirming the environment has no tracked resources.`,
+    );
   }
+
+  console.log(`📄 Loaded state file for environment: ${VAPI_ENV}`);
+  // Merge with empty state to ensure all keys exist (for backwards compatibility)
+  return {
+    ...createEmptyState(),
+    ...(content as Partial<StateFile>),
+  } as StateFile;
 }
 
 export async function saveState(state: StateFile): Promise<void> {
-  await writeFile(STATE_FILE_PATH, JSON.stringify(state, null, 2) + "\n");
+  // Atomic write: emit to a sibling temp file, then rename over the target.
+  // A crash or SIGINT mid-write leaves the original state intact rather than
+  // truncating it. A truncated state file would silently wipe all UUID
+  // mappings on the next load.
+  const tmpPath = `${STATE_FILE_PATH}.tmp`;
+  await writeFile(tmpPath, JSON.stringify(state, null, 2) + "\n");
+  await rename(tmpPath, STATE_FILE_PATH);
   console.log(`💾 Saved state file: ${STATE_FILE_PATH}`);
 }
-