chore: codebase-review cleanups — concurrency, keychain memoization, registry-owned tiers, postApply hook, dead code (#35)

src/api/ctClient.ts

@@ -97,8 +97,10 @@ export class CtClient {
       throw new CtApiError("Login succeeded but no session cookie was returned", res.status, null);
     }
     await this.refreshCsrfToken();
-    const body = (await res.json()) as { data: WhoAmI };
-    return body.data;
+    // Same tolerant unwrap as request(): prefer `.data`, but fall back to the raw body if the
+    // envelope is absent, so authenticate and request() agree on the shape.
+    const body = (await res.json()) as { data?: WhoAmI };
+    return (body.data ?? body) as WhoAmI;
   }
 
   async get<T = unknown>(path: string): Promise<T> {
@@ -155,20 +157,12 @@ export class CtClient {
   }
 
   private async refreshCsrfToken(): Promise<void> {
-    if (!this.cookie) {
-      return;
-    }
-    const res = await fetchWithRetry(
-      `${this.config.host}/api/csrftoken`,
-      { headers: { Accept: "application/json", Cookie: this.cookie } },
-      { isIdempotent: true },
-    );
-    this.captureCookie(res);
-    if (!res.ok) {
-      throw new CtApiError("Failed to fetch CSRF token", res.status, await safeBody(res));
-    }
-    const body = (await res.json()) as { data: string };
-    this.csrfToken = body.data;
+    // A plain authenticated GET: it rides the session cookie and GET skips the CSRF branch in
+    // request(), so this cannot recurse — and it reuses request()'s envelope unwrap + guarded 2xx
+    // parsing instead of duplicating the bootstrap fetch here. Callers only reach this once a cookie
+    // exists (authenticate sets it; request()'s write path guards on it), so the old empty-cookie
+    // early-return is unreachable and dropped.
+    this.csrfToken = await this.get<string>("/csrftoken");
   }
 
   /** Merge any Set-Cookie values into the stored cookie header. */

src/auth/tokenStore.ts

@@ -64,7 +64,25 @@ async function keychainSet(value: string): Promise<void> {
   ]);
 }
 
+/**
+ * Memoized keychain blob for this process. A single run resolves the host
+ * (via `resolveConfig`) AND the token (via `authedSession`) — each of which
+ * reaches for the stored credentials — so without this cache the same entry is
+ * fetched up to 3× per command, spawning `security find-generic-password`
+ * (and prompting to unlock a locked Keychain) every time. `undefined` = not yet
+ * read; `null` = read and absent. Invalidated on any write (`resetKeychainCache`).
+ */
+let cachedKeychainBlob: string | null | undefined;
+
+/** Drop the memoized keychain read. Called after every store/clear; exported for tests. */
+export function resetKeychainCache(): void {
+  cachedKeychainBlob = undefined;
+}
+
 async function keychainGet(): Promise<string | null> {
+  if (cachedKeychainBlob !== undefined) {
+    return cachedKeychainBlob;
+  }
   try {
     const { stdout } = await run("security", [
       "find-generic-password",
@@ -74,10 +92,11 @@ async function keychainGet(): Promise<string | null> {
       KEYCHAIN_ACCOUNT,
       "-w",
     ]);
-    return stdout.trim() || null;
+    cachedKeychainBlob = stdout.trim() || null;
   } catch {
-    return null;
+    cachedKeychainBlob = null;
   }
+  return cachedKeychainBlob;
 }
 
 async function keychainDelete(account: string): Promise<void> {
@@ -96,6 +115,7 @@ export async function storeCredentials(creds: Credentials): Promise<string> {
     );
   }
   await keychainSet(JSON.stringify(creds));
+  resetKeychainCache(); // a fresh login must invalidate any read the process already cached
   return `macOS Keychain (service "${KEYCHAIN_SERVICE}", account "${KEYCHAIN_ACCOUNT}")`;
 }
 
@@ -128,4 +148,5 @@ export async function clearCredentials(): Promise<void> {
     // Also drop the pre-host bare-token entry so an upgrade doesn't leave a secret behind.
     await keychainDelete(LEGACY_KEYCHAIN_ACCOUNT);
   }
+  resetKeychainCache(); // a later read in the same process must not return the cleared secret
 }

src/commands/apply.ts

@@ -1,16 +1,15 @@
 import { dirname, join } from "node:path";
 import { Command } from "commander";
-import type { CtClient } from "../api/ctClient.js";
 import { authedSession } from "../api/session.js";
 import { resolveConfig } from "../config.js";
-import { loadState, resolveStatePath, saveState, type State } from "../state/state.js";
+import { loadState, resolveStatePath, saveState } from "../state/state.js";
 import { loadConfig, resolveConfigPath } from "../config/load.js";
 import { buildPlan } from "../engine/build.js";
 import { executePlan } from "../engine/execute.js";
+import { runPostApplyHooks } from "../engine/synthetic.js";
 import { writeBackup } from "../engine/backup.js";
 import { renderPlan } from "../engine/render.js";
-import { summarize, type Plan } from "../engine/types.js";
-import { assertNotPeople } from "../engine/guard.js";
+import { summarize } from "../engine/types.js";
 import { buildPermissionPlan } from "../permissions/plan.js";
 import { renderPermissionPlan } from "../permissions/render.js";
 import { applyPermissionPlan } from "../permissions/apply.js";
@@ -26,49 +25,6 @@ interface ApplyOptions {
   refresh?: boolean;
 }
 
-interface RefreshResult {
-  created: number;
-  updated: number;
-  deleted: number;
-}
-
-/**
- * Post-apply dynamic-group refresh (opt-in via `--refresh`). For each applied
- * item whose changes touched the `dynamic` synthetic field, POST the
- * per-group `/dynamicgroups/{id}/refresh` endpoint to materialize computed
- * membership. Deliberately per-group only — the all-groups
- * `/dynamicgroups/refresh` endpoint has a huge blast radius and must never be
- * called from here.
- *
- * The id is read from state (post-apply, so creates have their real id) using
- * an explicit `undefined` check — CT ids can legitimately be `0`.
- */
-export async function refreshChangedDynamicGroups(
-  plan: Plan,
-  state: State,
-  client: Pick<CtClient, "request">,
-): Promise<void> {
-  for (const item of plan.items) {
-    if (item.action === "no-op" || item.action === "delete") continue;
-    const dynamicChange = item.changes.find((c) => c.field === "dynamic");
-    if (!dynamicChange) continue;
-    const to = dynamicChange.to as { status?: string } | undefined;
-    if (to?.status === "none") continue; // demoted to a non-dynamic group — nothing to refresh
-    const id = state.resources[item.key]?.id;
-    if (id === undefined) continue;
-    const path = `/dynamicgroups/${id}/refresh`;
-    assertNotPeople(path);
-    try {
-      const res = await client.request<RefreshResult[]>("POST", path);
-      const r = res?.[0];
-      if (r) info(`refreshed ${item.key}: +${r.created} ~${r.updated} -${r.deleted}`);
-    } catch (err) {
-      const message = err instanceof Error ? err.message : String(err);
-      warn(`Failed to refresh ${item.key} (#${id}): ${message}`);
-    }
-  }
-}
-
 /** backups/ dir: explicit flag → CT_BACKUP_DIR → `backups/` beside the state file. */
 export function resolveBackupDir(
   explicit: string | undefined,
@@ -97,8 +53,13 @@ export function applyCommand(): Command {
       const state = await loadState(statePath, config.host);
 
       const { client } = await authedSession();
-      const { plan, actual, fetchErrors } = await buildPlan(client, state, desired, { configDir });
-      const { items: permItems, fetchErrors: permFetchErrors } = await buildPermissionPlan(client, state, permissions, desired);
+      // Independent fetches: the resource plan and the permission plan (whose instance-wide
+      // /permissions/<domainType> reads are slow) run concurrently rather than back-to-back.
+      const [{ plan, actual, fetchErrors }, { items: permItems, fetchErrors: permFetchErrors }] =
+        await Promise.all([
+          buildPlan(client, state, desired, { configDir }),
+          buildPermissionPlan(client, state, permissions, desired),
+        ]);
 
       const allFetchErrors = [...fetchErrors, ...permFetchErrors];
       if (allFetchErrors.length > 0) {
@@ -163,9 +124,21 @@ export function applyCommand(): Command {
       if (permResult.granted > 0 || permResult.deleted > 0) {
         success(`Permissions applied: ${permResult.granted} granted, ${permResult.deleted} deleted.`);
       }
+      if (permResult.failed.length > 0) {
+        // Mirror executePlan's resumable stance: report which tuples failed (not a raw stack) and
+        // exit non-zero. Grants are reconciled statelessly, so a plain re-run resumes idempotently.
+        error(
+          `${permResult.failed.length} permission write(s) failed — re-run to resume (grant reconciliation is idempotent):`,
+        );
+        for (const f of permResult.failed) {
+          info(`    ${f.method} ${f.path} (authId ${f.authId}${f.dataId.length ? ` dataId ${f.dataId.join(",")}` : ""}): ${f.message}`);
+        }
+        process.exitCode = 1;
+        return;
+      }
 
       if (opts.refresh) {
-        await refreshChangedDynamicGroups(plan, state, client);
+        await runPostApplyHooks(plan, state, client);
       }
     });
 }

src/commands/get.ts

@@ -1,6 +1,6 @@
 import { Command } from "commander";
 import { authedSession } from "../api/session.js";
-import { loadCatalog } from "../permissions/catalog.js";
+import { CATALOG } from "../permissions/catalog.js";
 import { out } from "../ui.js";
 
 /**
@@ -40,9 +40,8 @@ export function getCommand(): Command {
     .command("permissions-catalog")
     .description("List the static permission-name → authId catalog (for use in config `grants`)")
     .action(() => {
-      const catalog = loadCatalog();
-      for (const name of Object.keys(catalog).sort()) {
-        const entry = catalog[name];
+      for (const name of Object.keys(CATALOG).sort()) {
+        const entry = CATALOG[name];
         if (!entry) continue;
         const scoped = entry.scopeField ? "scoped" : "unscoped";
         process.stdout.write(`${name} -> ${entry.authId} (${scoped})\n`);

src/commands/plan.ts

@@ -25,14 +25,16 @@ export function planCommand(): Command {
       const config = await resolveConfig();
       const configPath = resolveConfigPath(opts.config);
       const { resources: desired, permissions, configDir } = await loadConfig(configPath);
+      // loadState already refuses a host mismatch (state.ts) — no second guard needed here.
       const state = await loadState(resolveStatePath(opts.state), config.host);
-      if (state.host !== config.host) {
-        throw new Error(`State host (${state.host}) does not match CT_HOST (${config.host}).`);
-      }
 
       const { client } = await authedSession();
-      const { plan, fetchErrors } = await buildPlan(client, state, desired, { configDir });
-      const { items: permItems, fetchErrors: permFetchErrors } = await buildPermissionPlan(client, state, permissions, desired);
+      // Independent fetches run concurrently (see commands/apply.ts).
+      const [{ plan, fetchErrors }, { items: permItems, fetchErrors: permFetchErrors }] =
+        await Promise.all([
+          buildPlan(client, state, desired, { configDir }),
+          buildPermissionPlan(client, state, permissions, desired),
+        ]);
       if (opts.json) {
         out({ plan, permissions: permItems });
       } else {

src/engine/graph.ts

@@ -8,21 +8,18 @@
  * runs in the exact reverse.
  */
 import type { DesiredResource } from "./types.js";
+import { RESOURCES } from "../resources/registry.js";
 
-/** Lower tier is applied first. Delete runs highest tier first. */
-export const TYPE_TIER: Record<string, number> = {
-  campus: 0,
-  "group-type": 0,
-  "group-status": 0,
-  "age-group": 0,
-  "target-group": 0,
-  "relationship-type": 0,
-  group: 1,
-  "group-hierarchy": 2,
-  "group-role": 3,
-  permission: 4,
-  "dynamic-group": 5,
-};
+/**
+ * Lower tier is applied first; delete runs highest tier first. Derived from the resource registry
+ * (each entry owns its `tier`) rather than hand-maintained here — a new type gets ordered by adding
+ * one registry entry, and phantom types (`group-hierarchy`, `permission`, `dynamic-group`, …) that
+ * are synthetic sub-resources or separate plan domains, never `DesiredResource` types, cannot creep
+ * back in. The exported shape (`Record<string, number>`) is unchanged, so `computePlan` still reads it.
+ */
+export const TYPE_TIER: Record<string, number> = Object.fromEntries(
+  Object.entries(RESOURCES).map(([type, spec]) => [type, spec.tier]),
+);
 
 export function tierOf(type: string): number {
   return TYPE_TIER[type] ?? 0;

src/engine/hierarchy.ts

@@ -73,14 +73,17 @@ export function applyHierarchy(
     }
   }
 
-  // For each opted-in desired group that already exists in state, set its actual `parents` to the
-  // managed parent keys — mirroring the desired-side guard below (one pass, no separate opted-in set).
-  // A group not yet in state (fresh, first apply) has no actual to annotate; it is created instead.
+  // Single pass over the desired opt-ins (one copy of the predicate, mirroring the desired-side
+  // guard below). A group's actual gets a `parents` set only when it opted in AND is a managed
+  // GROUP in state — the managed-type guard from the old state-side iteration is preserved via the
+  // `state.resources[d.key]` lookup. A group not yet in state (fresh, first apply) has no actual to
+  // annotate; it is created instead.
   for (const d of desired) {
     if (d.type !== "group" || d.parents === undefined) continue;
     const managed = state.resources[d.key];
-    const a = managed && actual.get(d.key);
-    if (managed && a) {
+    if (!managed || managed.type !== "group") continue;
+    const a = actual.get(d.key);
+    if (a) {
       a.parents = managedParentKeys(parentIdsByGroup.get(managed.id) ?? [], groupIdToKey);
     }
   }

src/engine/plan.ts

@@ -20,7 +20,7 @@ import { orderKeys, isKnownType } from "./graph.js";
  * change (a `JSON.stringify` comparison would flag it, proposing an update that
  * can never converge).
  */
-function deepEqual(a: unknown, b: unknown): boolean {
+export function deepEqual(a: unknown, b: unknown): boolean {
   if (a === b) {
     return true;
   }
@@ -92,11 +92,23 @@ export function computePlan(
   for (const d of desired) {
     if (!isKnownType(d.type)) {
       throw new Error(
-        `Unknown resource type "${d.type}" for "${d.key}" — no apply tier defined. Add it to TYPE_TIER.`,
+        `Unknown resource type "${d.type}" for "${d.key}" — no apply tier defined. Add a registry entry in src/resources/registry.ts.`,
       );
     }
   }
 
+  // Reject duplicate desired keys up front. The DSL path (evaluateConfig) already dedups, but a
+  // programmatic caller (import command, test harness) that hands `computePlan` a raw array must not
+  // silently last-wins: `desiredByKey` would collapse the duplicates while the loop below emits both,
+  // corrupting the plan. Fail loudly instead.
+  const seen = new Set<string>();
+  for (const d of desired) {
+    if (seen.has(d.key)) {
+      throw new Error(`Duplicate desired key "${d.key}" — each resource must have a unique logical key.`);
+    }
+    seen.add(d.key);
+  }
+
   const desiredByKey = new Map(desired.map((d) => [d.key, d]));
   const creates: PlanItem[] = [];
   const updates: PlanItem[] = [];