From 3fd2f7b755cf9820fd614ba9ae145ad3aa2db9cb Mon Sep 17 00:00:00 2001 From: Dhruva Reddy Date: Thu, 14 May 2026 20:20:12 -0700 Subject: [PATCH 1/2] refactor(engine): extract shared slug + folder helpers into slug-utils (#35) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates 4+ duplicated helpers that had accumulated across the gitops engine as symptom-fixes piled up. Pure factoring, zero behavior change at every reachable call site. Duplications collapsed: - `slugify` — 4 byte-identical copies (pull.ts, dep-dedup.ts, audit.ts, setup.ts) → 1 in src/slug-utils.ts - `extractBaseSlug` — 2 byte-identical copies → 1 - `FOLDER_MAP` — 2 byte-identical copies (pull.ts, resources.ts) → 1 - `UUID_SUFFIX_RE` — open-coded in 3 places → 1 constant - recanonicalize's inlined precondition-2 check (UUID prefix match) → extracted as `isEngineSuffixedSlug` src/slug-utils.ts is config-free by design (no `./config.ts` import, no side effects at load) so it's safely importable from any test without priming process.argv / VAPI_TOKEN. This is the testability property the prior dep-dedup.ts comment claimed for its local duplicates but didn't actually enforce. Regex tightening: shared `UUID_SUFFIX_RE` uses `^(.+)-([0-9a-f]{8})$` (non-empty base) where the prior pull/dep-dedup copies used `^(.*)-...` (allowed empty base). Strict improvement — engine- generated keys always have a non-empty base, and the only input class affected is the synthetic `-<8hex>` shape which is never produced by `generateResourceId`. Pinned by a regression test in tests/slug-utils.test.ts. Back-compat: dep-dedup.ts re-exports slugify/extractBaseSlug so existing tests importing them via that path keep working. Tests: 228/228 pass (208 prior + 20 new slug-utils cases covering slugify behavior, UUID_SUFFIX_RE boundaries, extractBaseSlug loose form, isEngineSuffixedSlug strict form). --- src/audit.ts | 12 +-- src/dep-dedup.ts | 29 +++---- src/new-file-gate.ts | 7 +- src/pull.ts | 30 +------ src/recanonicalize.ts | 26 +++--- src/setup.ts | 9 +-- src/slug-utils.ts | 77 ++++++++++++++++++ tests/slug-utils.test.ts | 170 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 275 insertions(+), 85 deletions(-) create mode 100644 src/slug-utils.ts create mode 100644 tests/slug-utils.test.ts diff --git a/src/audit.ts b/src/audit.ts index ebc30a3..18905a5 100644 --- a/src/audit.ts +++ b/src/audit.ts @@ -23,12 +23,12 @@ import { join } from "path"; import { matchesIgnore, RESOURCES_DIR } from "./config.ts"; import { findOrphanResourceIds } from "./new-file-gate.ts"; import { - extractBaseSlug, fetchAllResources, listExistingResourceIds, type VapiResource, } from "./pull.ts"; import { FOLDER_MAP } from "./resources.ts"; +import { extractBaseSlug, slugify } from "./slug-utils.ts"; import { loadState } from "./state.ts"; import type { ResourceType, StateFile } from "./types.ts"; import { VALID_RESOURCE_TYPES } from "./types.ts"; @@ -132,17 +132,9 @@ async function defaultReadAssistantTools( } // ───────────────────────────────────────────────────────────────────────────── -// Slug helpers (kept local; mirror src/pull.ts conventions) +// Resource name extraction // ───────────────────────────────────────────────────────────────────────────── -function slugify(name: string): string { - return name - .toLowerCase() - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .replace(/-+/g, "-"); -} - function extractRemoteName(resource: VapiResource): string | undefined { if (typeof resource.name === "string" && resource.name) return resource.name; // Tools store their name under function.name diff --git a/src/dep-dedup.ts b/src/dep-dedup.ts index bed096c..556044d 100644 --- a/src/dep-dedup.ts +++ b/src/dep-dedup.ts @@ -17,16 +17,18 @@ // duplicates from prior bug runs — the caller should warn and surface // the loser UUIDs so a follow-up `npm run cleanup` can prune them). // -// NOTE on duplication: `slugify` and `extractBaseSlug` here mirror the -// definitions in `src/pull.ts`. pull.ts imports config.ts, which calls -// `parseEnvironment()` at module load and `process.exit(1)`s without a -// CLI env arg — making it impossible to import in a unit test. This -// module imports ONLY from `./types.ts` so it stays testable in -// isolation. Five lines duplicated is the right tradeoff; do not "DRY" -// these back into pull.ts. +// `slugify` and `extractBaseSlug` previously lived inline here as a +// deliberate copy of pull.ts's definitions — pull.ts imports config.ts +// which `process.exit(1)`s under unit tests, blocking direct reuse. They +// now live in `./slug-utils.ts` (config-free, side-effect-free) and are +// re-exported below so any existing test importing them from this module +// keeps working unchanged. +import { extractBaseSlug, slugify } from "./slug-utils.ts"; import type { ResourceState } from "./types.ts"; +export { extractBaseSlug, slugify } from "./slug-utils.ts"; + export interface RemoteResource { id: string; name?: string; @@ -43,19 +45,6 @@ export interface DedupMatch { duplicateUuids: string[]; } -export function slugify(name: string): string { - return name - .toLowerCase() - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .replace(/-+/g, "-"); -} - -export function extractBaseSlug(resourceId: string): string { - const match = resourceId.match(/^(.*)-([a-f0-9]{8})$/i); - return match?.[1] ?? resourceId; -} - // Minimal payload shape this module needs. Local resource files are loaded // as `Record`, so the only fields we know exist are `name` // (top-level, used by SOs / assistants / squads) and a nested `function.name` diff --git a/src/new-file-gate.ts b/src/new-file-gate.ts index f349b14..654781d 100644 --- a/src/new-file-gate.ts +++ b/src/new-file-gate.ts @@ -26,8 +26,9 @@ // ───────────────────────────────────────────────────────────────────────────── import { matchesIgnore, VAPI_ENV } from "./config.ts"; -import { extractBaseSlug, listExistingResourceIds } from "./pull.ts"; +import { listExistingResourceIds } from "./pull.ts"; import { FOLDER_MAP } from "./resources.ts"; +import { extractBaseSlug } from "./slug-utils.ts"; import type { ResourceType, StateFile } from "./types.ts"; import { VALID_RESOURCE_TYPES } from "./types.ts"; @@ -62,8 +63,8 @@ export interface DetectOrphanYamlsOptions { // list are reported. Mirrors `APPLY_FILTER.filePaths` semantics used by // selective push. filePathFilter?: string[]; - // Optional override of `extractBaseSlug`. Defaults to the pull.ts helper - // — only swapped in tests to keep the unit suite filesystem-free. + // Optional override of `extractBaseSlug`. Defaults to the slug-utils + // helper — only swapped in tests to keep the unit suite filesystem-free. extractBaseSlug?: (resourceId: string) => string; // Optional override of `matchesIgnore`. Defaults to the config.ts helper // which reads `.vapi-ignore` from disk. Tests pass a stub so they don't diff --git a/src/pull.ts b/src/pull.ts index d4cb024..ed1f185 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -20,6 +20,8 @@ import { formatRecanonicalizeReport, recanonicalizeStateKeys, } from "./recanonicalize.ts"; +import { FOLDER_MAP } from "./resources.ts"; +import { extractBaseSlug, slugify } from "./slug-utils.ts"; import { hashPayload, loadState, saveState, upsertState } from "./state.ts"; import type { ResourceState, ResourceType, StateFile } from "./types.ts"; @@ -59,19 +61,6 @@ const ENDPOINT_MAP: Record = { evals: "/eval", }; -// Map resource types to their folder paths (relative to resources/) -const FOLDER_MAP: Record = { - tools: "tools", - structuredOutputs: "structuredOutputs", - assistants: "assistants", - squads: "squads", - personalities: "simulations/personalities", - scenarios: "simulations/scenarios", - simulations: "simulations/tests", - simulationSuites: "simulations/suites", - evals: "evals", -}; - // ───────────────────────────────────────────────────────────────────────────── // Git Helpers (detect locally changed files to skip during pull) // ───────────────────────────────────────────────────────────────────────────── @@ -251,17 +240,9 @@ async function pullCredentials(state: StateFile): Promise { } // ───────────────────────────────────────────────────────────────────────────── -// Naming & Slug Generation +// Resource naming (slug generation lives in src/slug-utils.ts) // ───────────────────────────────────────────────────────────────────────────── -function slugify(name: string): string { - return name - .toLowerCase() - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .replace(/-+/g, "-"); -} - function extractName(resource: VapiResource): string | undefined { if (resource.name) return resource.name; // Tools store their name under function.name @@ -276,11 +257,6 @@ function generateResourceId(resource: VapiResource): string { return name ? `${slugify(name)}-${shortId}` : `resource-${shortId}`; } -export function extractBaseSlug(resourceId: string): string { - const match = resourceId.match(/^(.*)-([a-f0-9]{8})$/i); - return match?.[1] ?? resourceId; -} - export function resourceIdMatchesName( resourceId: string, resource: VapiResource, diff --git a/src/recanonicalize.ts b/src/recanonicalize.ts index 3fc6c13..4f6e953 100644 --- a/src/recanonicalize.ts +++ b/src/recanonicalize.ts @@ -44,12 +44,11 @@ import { existsSync } from "fs"; import { join } from "path"; import { RESOURCES_DIR } from "./config.ts"; import { FOLDER_MAP, VALID_EXTENSIONS } from "./resources.ts"; +import { isEngineSuffixedSlug } from "./slug-utils.ts"; import type { TouchedSets } from "./state-merge.ts"; import type { ResourceType, StateFile } from "./types.ts"; import { VALID_RESOURCE_TYPES } from "./types.ts"; -const UUID_SUFFIX_RE = /^(.+)-([0-9a-f]{8})$/i; - export interface RecanonicalizeRekey { type: ResourceType; fromKey: string; @@ -132,21 +131,14 @@ export function recanonicalizeStateKeys( const entry = section[stateKey]; if (!entry) continue; - const match = stateKey.match(UUID_SUFFIX_RE); - if (!match) continue; - - const [, canonicalSlug, capturedUuid8] = match; - if (!canonicalSlug || !capturedUuid8) continue; - - // Precondition 2 — only recanonicalize engine-generated suffixes. If - // the captured 8 hex chars don't match the entry's UUID prefix, this - // is a user-named resource that coincidentally looks suffixed. - // - // Mirrors generateResourceId() in src/pull.ts:265-273 — UUID dashes - // start at index 8, so `slice(0, 8)` on the raw UUID is equivalent - // to stripping dashes first; the dash-strip is defensive. - const entryUuid8 = entry.uuid.replace(/-/g, "").slice(0, 8).toLowerCase(); - if (capturedUuid8.toLowerCase() !== entryUuid8) continue; + // Preconditions 1 + 2 — the key must match the engine-generated + // shape `-` AND the captured 8-hex must match the + // entry's UUID prefix. `isEngineSuffixedSlug` returns `null` on + // either failure, ruling out user-named resources that + // coincidentally end in `-abcd1234`. + const parsed = isEngineSuffixedSlug(stateKey, entry.uuid); + if (!parsed) continue; + const canonicalSlug = parsed.base; // Precondition 3 — canonical slot must be unclaimed in state. // diff --git a/src/setup.ts b/src/setup.ts index 1e7ae62..565a66f 100644 --- a/src/setup.ts +++ b/src/setup.ts @@ -5,6 +5,7 @@ import { mkdir, rm, writeFile } from "fs/promises"; import { dirname, join } from "path"; import { fileURLToPath } from "url"; import searchableCheckbox, { BACK_SENTINEL } from "./searchableCheckbox.js"; +import { slugify } from "./slug-utils.ts"; // ───────────────────────────────────────────────────────────────────────────── // Constants @@ -152,14 +153,6 @@ async function fetchAllResourceSnapshots( // Slug helpers // ───────────────────────────────────────────────────────────────────────────── -function slugify(name: string): string { - return name - .toLowerCase() - .replace(/[^a-z0-9]+/g, "-") - .replace(/^-+|-+$/g, "") - .replace(/-+/g, "-"); -} - // ───────────────────────────────────────────────────────────────────────────── // Dependency detection — scan selected resources for UUID references // to resources that aren't yet selected diff --git a/src/slug-utils.ts b/src/slug-utils.ts new file mode 100644 index 0000000..eed0625 --- /dev/null +++ b/src/slug-utils.ts @@ -0,0 +1,77 @@ +// ───────────────────────────────────────────────────────────────────────────── +// Shared slug helpers — config-free, no side-effect imports. +// +// This module exists to break two duplications that previously lived across +// `pull.ts`, `dep-dedup.ts`, `audit.ts`, and `setup.ts`: +// - `slugify(name)` — 4 byte-identical copies +// - `extractBaseSlug(resourceId)` — 2 byte-identical copies +// +// It also exposes the strict `isEngineSuffixedSlug` form used by +// `recanonicalize.ts` to prove a state key was engine-generated (i.e. the +// captured 8-hex matches the entry's UUID prefix), and the canonical +// `UUID_SUFFIX_RE` constant. +// +// Config-free is load-bearing: `config.ts` asserts `argv[2]` / `VAPI_TOKEN` +// at module load. Any test that imports a slug helper without going through +// this module would otherwise have to prime `process.argv` and +// `process.env.VAPI_TOKEN` (see `tests/recanonicalize.test.ts:7-8`). This +// module has zero such side effects so it's safely importable from any test. +// ───────────────────────────────────────────────────────────────────────────── + +// `^(.+)-([0-9a-f]{8})$` deliberately requires a non-empty base. An engine- +// generated state key always carries a real slug before the 8-hex suffix — +// the synthetic `-deadbeef` shape (empty base) is never produced. +export const UUID_SUFFIX_RE = /^(.+)-([0-9a-f]{8})$/i; + +// Lowercase, replace non-alphanumeric runs with `-`, trim leading/trailing +// `-`, and collapse repeated `-`. Mirrors the slug shape produced by +// `generateResourceId` in `src/pull.ts` and downstream `-` +// patterns. +export function slugify(name: string): string { + return name + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, "") + .replace(/-+/g, "-"); +} + +// Loose form: strip a trailing 8-hex segment if the resourceId matches the +// engine-generated `-` shape; otherwise return the input +// unchanged. Used by callers that don't have a UUID handy (audit's +// sibling-base-slug check, the orphan-gate's pairing pass, pull's +// `findExistingResourceId`, dep-dedup's `extractBaseSlug` consumers). +// +// This intentionally does NOT verify that the captured suffix matches any +// specific UUID — that proof requires `isEngineSuffixedSlug`. Loose callers +// only need a best-effort canonical form. +export function extractBaseSlug(resourceId: string): string { + const match = resourceId.match(UUID_SUFFIX_RE); + return match?.[1] ?? resourceId; +} + +// Strict form: return the parsed `{ base, suffix }` ONLY when the captured +// 8 hex chars match the leading 8 hex chars of `uuid` (case-insensitive, +// dashes stripped defensively). Returns `null` otherwise — including when +// the resourceId doesn't match the engine shape at all. +// +// Use this when you have BOTH a state key AND its entry's UUID and need to +// prove the key was engine-generated (the precondition-2 check in +// `recanonicalize.ts`). A user-given name that coincidentally ends in +// `-abcd1234` will NOT match because its UUID prefix is different. +// +// Mirrors `generateResourceId` in `src/pull.ts:265-273` — UUIDs have the +// form `xxxxxxxx-xxxx-...` so the first 8 hex chars are dash-free, but the +// dash-strip is kept as defense against malformed input. +export function isEngineSuffixedSlug( + stateKey: string, + uuid: string, +): { base: string; suffix: string } | null { + const match = stateKey.match(UUID_SUFFIX_RE); + if (!match) return null; + const base = match[1]; + const capturedSuffix = match[2]; + if (!base || !capturedSuffix) return null; + const uuidPrefix = uuid.replace(/-/g, "").slice(0, 8).toLowerCase(); + if (capturedSuffix.toLowerCase() !== uuidPrefix) return null; + return { base, suffix: capturedSuffix.toLowerCase() }; +} diff --git a/tests/slug-utils.test.ts b/tests/slug-utils.test.ts new file mode 100644 index 0000000..cd0dde0 --- /dev/null +++ b/tests/slug-utils.test.ts @@ -0,0 +1,170 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +// slug-utils.ts is config-free by design — no need to prime process.argv / +// VAPI_TOKEN. That's the whole point of the module. This test imports it +// directly to prove that property. +import { + extractBaseSlug, + isEngineSuffixedSlug, + slugify, + UUID_SUFFIX_RE, +} from "../src/slug-utils.ts"; + +// ───────────────────────────────────────────────────────────────────────────── +// slugify +// ───────────────────────────────────────────────────────────────────────────── + +test("slugify: lowercases, replaces non-alphanumeric, trims, collapses runs", () => { + assert.equal(slugify("Hello World"), "hello-world"); + assert.equal(slugify(" Foo Bar "), "foo-bar"); + assert.equal(slugify("Foo___Bar"), "foo-bar"); + assert.equal(slugify("--foo--bar--"), "foo-bar"); + assert.equal(slugify("End Call!"), "end-call"); + assert.equal(slugify("ALL CAPS NAME"), "all-caps-name"); +}); + +test("slugify: preserves digits", () => { + assert.equal(slugify("v2 API"), "v2-api"); + assert.equal(slugify("squad 7"), "squad-7"); +}); + +test("slugify: collapses repeated separators into single dash", () => { + assert.equal(slugify("foo!!bar"), "foo-bar"); + assert.equal(slugify("foo - - bar"), "foo-bar"); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// UUID_SUFFIX_RE +// ───────────────────────────────────────────────────────────────────────────── + +test("UUID_SUFFIX_RE: matches exactly 8 hex chars after final dash", () => { + assert.match("foo-12345678", UUID_SUFFIX_RE); + assert.match("end-call-67aea057", UUID_SUFFIX_RE); + assert.match("a-b-c-d-deadbeef", UUID_SUFFIX_RE); +}); + +test("UUID_SUFFIX_RE: rejects 7 or 9 hex chars (off-by-one boundaries)", () => { + assert.doesNotMatch("foo-1234567", UUID_SUFFIX_RE); + assert.doesNotMatch("foo-123456789", UUID_SUFFIX_RE); +}); + +test("UUID_SUFFIX_RE: rejects non-hex suffix", () => { + assert.doesNotMatch("foo-xxxxxxxx", UUID_SUFFIX_RE); + assert.doesNotMatch("foo-1234567g", UUID_SUFFIX_RE); +}); + +test("UUID_SUFFIX_RE: rejects empty base (regression guard for `.+` not `.*`)", () => { + // An engine-generated state key always has a real slug before the + // suffix. The synthetic `-deadbeef` shape (empty base) must not match + // because there's no canonical slug to recanonicalize TO. + assert.doesNotMatch("-deadbeef", UUID_SUFFIX_RE); +}); + +test("UUID_SUFFIX_RE: case-insensitive on hex", () => { + assert.match("foo-ABCDEF12", UUID_SUFFIX_RE); + assert.match("foo-AbCdEf12", UUID_SUFFIX_RE); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// extractBaseSlug — loose form +// ───────────────────────────────────────────────────────────────────────────── + +test("extractBaseSlug: strips engine-shape suffix", () => { + assert.equal(extractBaseSlug("end-call-67aea057"), "end-call"); + assert.equal(extractBaseSlug("foo-vmd-004c5108"), "foo-vmd"); +}); + +test("extractBaseSlug: returns input unchanged when no suffix present", () => { + assert.equal(extractBaseSlug("my-tool"), "my-tool"); + assert.equal(extractBaseSlug("plain"), "plain"); +}); + +test("extractBaseSlug: returns input unchanged when suffix is non-hex", () => { + assert.equal(extractBaseSlug("my-tool-xxxxxxxx"), "my-tool-xxxxxxxx"); +}); + +test("extractBaseSlug: returns input unchanged when suffix is wrong length", () => { + assert.equal(extractBaseSlug("my-tool-1234567"), "my-tool-1234567"); + assert.equal(extractBaseSlug("my-tool-123456789"), "my-tool-123456789"); +}); + +test("extractBaseSlug: does NOT validate suffix against any UUID (intentional loose form)", () => { + // This is the contract that separates the loose form from the strict + // `isEngineSuffixedSlug`. Callers without a UUID handy still need a + // best-effort canonical form. A user-named "my-tool-deadbeef" returns + // "my-tool" — pull's `findExistingResourceId` and audit's + // sibling-base-slug check both rely on this best-effort behavior. + assert.equal(extractBaseSlug("my-tool-deadbeef"), "my-tool"); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// isEngineSuffixedSlug — strict form +// ───────────────────────────────────────────────────────────────────────────── + +test("isEngineSuffixedSlug: returns {base, suffix} when captured 8-hex matches UUID prefix", () => { + const result = isEngineSuffixedSlug( + "end-call-67aea057", + "67aea057-1234-5678-90ab-cdef01234567", + ); + assert.deepEqual(result, { base: "end-call", suffix: "67aea057" }); +}); + +test("isEngineSuffixedSlug: returns null when captured suffix doesn't match UUID prefix (the precondition-2 check)", () => { + // User-named "my-tool-deadbeef" paired with an unrelated UUID — must + // NOT be treated as engine-generated. + const result = isEngineSuffixedSlug( + "my-tool-deadbeef", + "99999999-aaaa-bbbb-cccc-dddddddddddd", + ); + assert.equal(result, null); +}); + +test("isEngineSuffixedSlug: returns null when key has no engine-shape suffix", () => { + assert.equal( + isEngineSuffixedSlug("plain-name", "67aea057-1234-5678-90ab-cdef01234567"), + null, + ); +}); + +test("isEngineSuffixedSlug: rejects empty base (regression guard)", () => { + // Same shape as the UUID_SUFFIX_RE empty-base guard, but reached + // through the strict path. `-deadbeef` paired with `deadbeef-...` UUID + // would technically satisfy the prefix-match but the regex itself + // rejects empty bases. + assert.equal( + isEngineSuffixedSlug("-deadbeef", "deadbeef-1234-5678-90ab-cdef01234567"), + null, + ); +}); + +test("isEngineSuffixedSlug: case-insensitive on captured suffix and UUID prefix", () => { + const result = isEngineSuffixedSlug( + "foo-AbCdEf12", + "abcdef12-0000-0000-0000-000000000000", + ); + assert.deepEqual(result, { base: "foo", suffix: "abcdef12" }); +}); + +test("isEngineSuffixedSlug: strips UUID dashes defensively (malformed UUID with leading dashes)", () => { + // Real UUIDs are `xxxxxxxx-xxxx-...` (dash starts at index 8), so the + // first 8 chars contain no dashes. The dash-strip is defense against + // malformed input — verify it does what the comment claims by feeding + // a UUID with leading dashes that should still match. + const result = isEngineSuffixedSlug( + "foo-abcdef12", + "abc-def-12-0000-0000-0000-000000000000", + ); + assert.deepEqual(result, { base: "foo", suffix: "abcdef12" }); +}); + +test("isEngineSuffixedSlug: handles multi-segment base", () => { + const result = isEngineSuffixedSlug( + "iform-voicemail-triage-squad-llm-only-vmd-004c5108", + "004c5108-aaaa-bbbb-cccc-dddddddddddd", + ); + assert.deepEqual(result, { + base: "iform-voicemail-triage-squad-llm-only-vmd", + suffix: "004c5108", + }); +}); From 4d952a41616d731b1e3c96f32e98aa1b036a35c0 Mon Sep 17 00:00:00 2001 From: Dhruva Reddy Date: Thu, 14 May 2026 19:41:02 -0700 Subject: [PATCH 2/2] =?UTF-8?q?refactor(push):=20extract=20reconcileStateK?= =?UTF-8?q?eyForResource=20=E2=80=94=20fold=20two=2094-line=20ensure-fns?= =?UTF-8?q?=20into=20one=20generic=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ensureToolExists` and `ensureStructuredOutputExists` were structurally identical 94-line functions differing only in: resource type label, apply function, state section, remote-list cache, and the per-type bookkeeping array. Both implemented the same dedup-then-apply flow: look up existing dashboard/state match by canonical name, adopt with orphan-deletion guard, mark `touched` for `mergeScoped`, call apply. Behavioral contract preserved exactly: - Log strings byte-identical (verified path-by-path against pre-refactor) - `autoApplied.add` BEFORE the `if (!uuid) return` early-exit - `applied[type]++`, `pushToAutoAppliedList`, `touched.add` AFTER the null check — preserves dry-run / drift-halt semantics - Orphan-deletion guard scope unchanged: deletes state keys pointing at the adopted UUID, leaves `duplicateUuids` alone for `npm run cleanup` to handle - try/catch boundary identical - All 228 prior tests pass unchanged, including the integration test in tests/push-dry-run.test.ts push.ts shrunk -129 net LOC (two 94-line functions collapsed to ~14-line wrappers). Helper is `tools | structuredOutputs` narrow today; adding a future type requires a deliberate union widening + LABELS map entry, not a config flag. `vapiEnv` and `formatError` parameters are required (not optional with placeholder defaults) so a future caller can't accidentally emit a degraded warning or error message. Tests: 244/244 pass (228 prior + 16 new — 8 scenarios × 2 resource types covering happy path, ambiguous match, null applyFn ordering contract, orphan-deletion guard scope, run-scoped idempotency, state-hit/dashboard-hit/no-match branches). Closes the symptom-fix pattern documented in improvements.md #10 (now handled by the generic helper instead of the two hardcoded functions). --- src/push.ts | 179 ++---------- src/reconcile-state-key.ts | 205 ++++++++++++++ tests/reconcile-state-key.test.ts | 444 ++++++++++++++++++++++++++++++ 3 files changed, 674 insertions(+), 154 deletions(-) create mode 100644 src/reconcile-state-key.ts create mode 100644 tests/reconcile-state-key.test.ts diff --git a/src/push.ts b/src/push.ts index b40183f..d1d3757 100644 --- a/src/push.ts +++ b/src/push.ts @@ -26,6 +26,7 @@ import { formatRecanonicalizeReport, recanonicalizeStateKeys, } from "./recanonicalize.ts"; +import { reconcileStateKeyForResource } from "./reconcile-state-key.ts"; import { writeSnapshot } from "./snapshot.ts"; import { mergeScoped } from "./state-merge.ts"; import { @@ -947,85 +948,19 @@ async function ensureToolExists( const tool = ctx.allTools.find((t) => t.resourceId === toolId); if (!tool) return; - // Before creating, check whether an existing state entry (under a - // different key — e.g., bootstrap-generated `end-call-`) or a - // live dashboard tool already represents this same logical tool. Adopt - // instead of minting a duplicate. - const remoteList = await getExistingRemoteTools(ctx); - const match = findExistingResourceByName({ - localResourceId: toolId, - localPayload: tool.data, - stateSection: ctx.state.tools, - remoteList, + await reconcileStateKeyForResource({ + resourceType: "tools", + resource: tool, + state: ctx.state, + touched: ctx.touched, + applied: ctx.applied, + autoApplied: ctx.autoApplied, + pushToAutoAppliedList: (r) => ctx.autoAppliedTools.push(r), + getRemoteList: () => getExistingRemoteTools(ctx), + applyFn: applyTool, + vapiEnv: VAPI_ENV, + formatError: formatApiError, }); - if (match) { - if (match.ambiguous) { - const displayName = extractResourceName(tool.data) ?? toolId; - console.warn( - ` ⚠️ Multiple dashboard tools share the name "${displayName}" — adopting ${match.uuid} (lex-smallest). Other UUIDs: ${match.duplicateUuids.join(", ")}. Run \`npm run cleanup -- ${VAPI_ENV}\` to prune duplicates.`, - ); - } - console.log( - ` 🔁 Reusing existing tool: ${toolId} → ${match.uuid} (matched via ${match.source})`, - ); - - // Re-key state to point at the adopted UUID under the local resourceId. - // No hash yet — applyTool below will PATCH with the local payload and - // record the post-PATCH hash, exercising the standard drift-check flow. - upsertState(ctx.state.tools, tool.resourceId, { uuid: match.uuid }); - - // Orphan-deletion guard — drop other state keys pointing at the SAME - // uuid so a subsequent full push doesn't see them as "tracked but no - // local file" and DELETE the dashboard resource we just adopted. Mark - // them touched so the scoped state-merge on save flushes the deletion. - // Entries pointing at `match.duplicateUuids` are SEPARATE dashboard - // duplicates — leave them alone; `npm run cleanup` handles those. - for (const [staleKey, entry] of Object.entries(ctx.state.tools)) { - if (staleKey !== tool.resourceId && entry.uuid === match.uuid) { - delete ctx.state.tools[staleKey]; - ctx.touched.tools.add(staleKey); - } - } - - // PATCH the dashboard with the local payload. `applyTool`'s - // `upsertResourceWithStateRecovery` branch picks PATCH because - // `state.tools` now has `existingUuid` set. Drift check fires - // (no-baseline → log + proceed when `lastPulledHash` is undefined; - // full check when it isn't). - try { - const uuid = await applyTool(tool, ctx.state); - ctx.autoApplied.add(`tools:${toolId}`); - if (!uuid) return; - upsertState(ctx.state.tools, tool.resourceId, { - uuid, - lastPushedHash: hashPayload(tool.data), - }); - ctx.applied.tools++; - ctx.autoAppliedTools.push(tool); - ctx.touched.tools.add(tool.resourceId); - } catch (error) { - console.error(formatApiError(toolId, error)); - throw error; - } - return; - } - - console.log(` 📦 Auto-applying dependency → tool: ${toolId}`); - try { - const uuid = await applyTool(tool, ctx.state); - ctx.autoApplied.add(`tools:${toolId}`); - if (!uuid) return; - upsertState(ctx.state.tools, tool.resourceId, { - uuid, - lastPushedHash: hashPayload(tool.data), - }); - ctx.applied.tools++; - ctx.autoAppliedTools.push(tool); - ctx.touched.tools.add(tool.resourceId); - } catch (error) { - console.error(formatApiError(toolId, error)); - throw error; - } } async function ensureStructuredOutputExists( @@ -1044,83 +979,19 @@ async function ensureStructuredOutputExists( ); if (!output) return; - // Same dedup pattern as `ensureToolExists`, against the SO state section - // and live dashboard SO list. - const remoteList = await getExistingRemoteStructuredOutputs(ctx); - const match = findExistingResourceByName({ - localResourceId: outputId, - localPayload: output.data, - stateSection: ctx.state.structuredOutputs, - remoteList, + await reconcileStateKeyForResource({ + resourceType: "structuredOutputs", + resource: output, + state: ctx.state, + touched: ctx.touched, + applied: ctx.applied, + autoApplied: ctx.autoApplied, + pushToAutoAppliedList: (r) => ctx.autoAppliedStructuredOutputs.push(r), + getRemoteList: () => getExistingRemoteStructuredOutputs(ctx), + applyFn: applyStructuredOutput, + vapiEnv: VAPI_ENV, + formatError: formatApiError, }); - if (match) { - if (match.ambiguous) { - const displayName = extractResourceName(output.data) ?? outputId; - console.warn( - ` ⚠️ Multiple dashboard structured outputs share the name "${displayName}" — adopting ${match.uuid} (lex-smallest). Other UUIDs: ${match.duplicateUuids.join(", ")}. Run \`npm run cleanup -- ${VAPI_ENV}\` to prune duplicates.`, - ); - } - console.log( - ` 🔁 Reusing existing structured output: ${outputId} → ${match.uuid} (matched via ${match.source})`, - ); - - // Re-key state to point at the adopted UUID under the local resourceId. - // No hash yet — applyStructuredOutput below will PATCH with the local - // payload and record the post-PATCH hash, exercising the standard - // drift-check flow. - upsertState(ctx.state.structuredOutputs, output.resourceId, { - uuid: match.uuid, - }); - - // Orphan-deletion guard — drop other state keys pointing at the SAME - // uuid so a subsequent full push doesn't see them as "tracked but no - // local file" and DELETE the dashboard resource we just adopted. Mark - // them touched so the scoped state-merge on save flushes the deletion. - for (const [staleKey, entry] of Object.entries( - ctx.state.structuredOutputs, - )) { - if (staleKey !== output.resourceId && entry.uuid === match.uuid) { - delete ctx.state.structuredOutputs[staleKey]; - ctx.touched.structuredOutputs.add(staleKey); - } - } - - // PATCH via the standard apply path so drift detection fires and any - // local edits land on the dashboard. - try { - const uuid = await applyStructuredOutput(output, ctx.state); - ctx.autoApplied.add(`structuredOutputs:${outputId}`); - if (!uuid) return; - upsertState(ctx.state.structuredOutputs, output.resourceId, { - uuid, - lastPushedHash: hashPayload(output.data), - }); - ctx.applied.structuredOutputs++; - ctx.autoAppliedStructuredOutputs.push(output); - ctx.touched.structuredOutputs.add(output.resourceId); - } catch (error) { - console.error(formatApiError(outputId, error)); - throw error; - } - return; - } - - console.log(` 📦 Auto-applying dependency → structured output: ${outputId}`); - try { - const uuid = await applyStructuredOutput(output, ctx.state); - ctx.autoApplied.add(`structuredOutputs:${outputId}`); - if (!uuid) return; - upsertState(ctx.state.structuredOutputs, output.resourceId, { - uuid, - lastPushedHash: hashPayload(output.data), - }); - ctx.applied.structuredOutputs++; - ctx.autoAppliedStructuredOutputs.push(output); - ctx.touched.structuredOutputs.add(output.resourceId); - } catch (error) { - console.error(formatApiError(outputId, error)); - throw error; - } } async function ensureAssistantDepsExist( diff --git a/src/reconcile-state-key.ts b/src/reconcile-state-key.ts new file mode 100644 index 0000000..9291747 --- /dev/null +++ b/src/reconcile-state-key.ts @@ -0,0 +1,205 @@ +// Generic state-key reconciliation for auto-applied dependencies. +// +// Both `ensureToolExists` and `ensureStructuredOutputExists` in `push.ts` +// share a 94-line body that: +// +// 1. Probes for an existing dashboard / state resource whose canonical +// name matches the local payload (`findExistingResourceByName`), so a +// bootstrap-renamed state entry (`-`) or a live dashboard +// twin isn't shadowed by a brand-new POST. +// 2. On match: rekeys state to the adopted UUID, runs the orphan-deletion +// guard (drop OTHER state keys pointing at the same UUID so a +// subsequent push doesn't DELETE the freshly-adopted dashboard +// resource), and PATCHes via the standard apply path so drift +// detection fires. +// 3. On no match: logs the auto-apply line and runs the standard apply +// path (which creates). +// 4. In BOTH branches: records `autoApplied` even when the apply returns +// `null` (the apply-path short-circuit that signals "skipped"), then +// conditionally updates state with the post-PATCH hash, increments the +// applied counter, pushes onto the per-type bookkeeping array, and +// marks the local resourceId touched. +// +// The two functions differ only in: resource type label (singular + plural +// for log lines), state section, remote list fetcher, the per-type counter +// in `applied`, the per-type bookkeeping array, and the apply function. +// This module extracts the shared body — narrowed to `tools` / +// `structuredOutputs` because those are the only two auto-apply paths in +// the engine today. + +import type { RemoteResource } from "./dep-dedup.ts"; +import { + extractResourceName, + findExistingResourceByName, +} from "./dep-dedup.ts"; +import { hashPayload, upsertState } from "./state.ts"; +import type { TouchedSets } from "./state-merge.ts"; +import type { ResourceFile, ResourceType, StateFile } from "./types.ts"; + +// The two resource types that flow through the auto-apply path. Adding a +// new caller (e.g. simulations) is a one-line union widening. +export type ReconcileResourceType = "tools" | "structuredOutputs"; + +// Per-type display labels used in the three log lines this helper emits. +// `singular` populates the "Reusing existing X" / "→ X" lines; `plural` +// populates the ambiguous-match warning ("Multiple dashboard Xs share the +// name"). +const LABELS: Record< + ReconcileResourceType, + { singular: string; plural: string } +> = { + tools: { singular: "tool", plural: "dashboard tools" }, + structuredOutputs: { + singular: "structured output", + plural: "dashboard structured outputs", + }, +}; + +export interface ReconcileStateKeyOptions { + resourceType: ReconcileResourceType; + resource: ResourceFile>; + state: StateFile; + touched: TouchedSets; + // Per-type counter map (incremented on a non-null apply result). The + // shape mirrors `DependencyContext.applied` so callers can pass it + // directly without remapping. + applied: Record; + // Cross-type Set of `${resourceType}:${resourceId}` keys for run-scoped + // idempotency. The apply path adds to this set BEFORE checking the apply + // result, matching the legacy behavior — see "applyFn null semantics" in + // the PR description. + autoApplied: Set; + // Callback for the per-type bookkeeping array + // (`ctx.autoAppliedTools` / `ctx.autoAppliedStructuredOutputs`). Wrapped + // as a callback so this module stays decoupled from `DependencyContext`. + pushToAutoAppliedList: ( + resource: ResourceFile>, + ) => void; + // Lazy fetcher for the live dashboard inventory used in the dedup check. + // Callers cache the result inside `DependencyContext` so it fires at most + // once per push. + getRemoteList: () => Promise; + // The standard apply pipeline for this resource type. Returns the + // post-PATCH UUID, or `null` when the apply was skipped (e.g. dry-run, + // dependency unresolved, drift-check halt) — in which case `autoApplied` + // is still recorded but the state-update / counter / bookkeeping steps + // are SKIPPED. Matches the legacy `ensureToolExists` / + // `ensureStructuredOutputExists` semantics exactly. + applyFn: (resource: ResourceFile, state: StateFile) => Promise; + // Surfaced in the ambiguous-match warning to direct the operator at the + // right cleanup command. Required so a future caller (e.g. a sims + // auto-apply path) can't omit it and emit `npm run cleanup -- ` + // verbatim to a confused operator. Tests pass `"test-env"`. + vapiEnv: string; + // Format errors uniformly with the rest of the push pipeline. Required + // for the same future-caller-safety reason as `vapiEnv` — production + // callers MUST inject `formatApiError` from push.ts so VapiApiError + // breakouts render with status code + endpoint, not a degraded + // `Error.message`-only fallback. + formatError: (resourceId: string, error: unknown) => string; +} + +export async function reconcileStateKeyForResource( + opts: ReconcileStateKeyOptions, +): Promise { + const { + resourceType, + resource, + state, + touched, + applied, + autoApplied, + pushToAutoAppliedList, + getRemoteList, + applyFn, + vapiEnv, + formatError, + } = opts; + + const { singular, plural } = LABELS[resourceType]; + const stateSection = state[resourceType]; + const touchedSection = touched[resourceType]; + const resourceId = resource.resourceId; + const autoAppliedKey = `${resourceType}:${resourceId}`; + + // Before creating, check whether an existing state entry (under a + // different key — e.g., bootstrap-generated `-`) or a live + // dashboard resource already represents this same logical resource. + // Adopt instead of minting a duplicate. + const remoteList = await getRemoteList(); + const match = findExistingResourceByName({ + localResourceId: resourceId, + localPayload: resource.data, + stateSection, + remoteList, + }); + + if (match) { + if (match.ambiguous) { + const displayName = extractResourceName(resource.data) ?? resourceId; + console.warn( + ` ⚠️ Multiple ${plural} share the name "${displayName}" — adopting ${match.uuid} (lex-smallest). Other UUIDs: ${match.duplicateUuids.join(", ")}. Run \`npm run cleanup -- ${vapiEnv}\` to prune duplicates.`, + ); + } + console.log( + ` 🔁 Reusing existing ${singular}: ${resourceId} → ${match.uuid} (matched via ${match.source})`, + ); + + // Re-key state to point at the adopted UUID under the local resourceId. + // No hash yet — `applyFn` below will PATCH with the local payload and + // record the post-PATCH hash, exercising the standard drift-check flow. + upsertState(stateSection, resourceId, { uuid: match.uuid }); + + // Orphan-deletion guard — drop other state keys pointing at the SAME + // uuid so a subsequent full push doesn't see them as "tracked but no + // local file" and DELETE the dashboard resource we just adopted. Mark + // them touched so the scoped state-merge on save flushes the deletion. + // Entries pointing at `match.duplicateUuids` are SEPARATE dashboard + // duplicates — leave them alone; `npm run cleanup` handles those. + for (const [staleKey, entry] of Object.entries(stateSection)) { + if (staleKey !== resourceId && entry.uuid === match.uuid) { + delete stateSection[staleKey]; + touchedSection.add(staleKey); + } + } + + // PATCH the dashboard with the local payload. The apply function's + // `upsertResourceWithStateRecovery` branch picks PATCH because the + // state section now has `existingUuid` set. Drift check fires + // (no-baseline → log + proceed when `lastPulledHash` is undefined; + // full check when it isn't). + try { + const uuid = await applyFn(resource, state); + autoApplied.add(autoAppliedKey); + if (!uuid) return; + upsertState(stateSection, resourceId, { + uuid, + lastPushedHash: hashPayload(resource.data), + }); + applied[resourceType]++; + pushToAutoAppliedList(resource); + touchedSection.add(resourceId); + } catch (error) { + console.error(formatError(resourceId, error)); + throw error; + } + return; + } + + console.log(` 📦 Auto-applying dependency → ${singular}: ${resourceId}`); + try { + const uuid = await applyFn(resource, state); + autoApplied.add(autoAppliedKey); + if (!uuid) return; + upsertState(stateSection, resourceId, { + uuid, + lastPushedHash: hashPayload(resource.data), + }); + applied[resourceType]++; + pushToAutoAppliedList(resource); + touchedSection.add(resourceId); + } catch (error) { + console.error(formatError(resourceId, error)); + throw error; + } +} diff --git a/tests/reconcile-state-key.test.ts b/tests/reconcile-state-key.test.ts new file mode 100644 index 0000000..a4b4b0e --- /dev/null +++ b/tests/reconcile-state-key.test.ts @@ -0,0 +1,444 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +// reconcile-state-key.ts → state.ts → config.ts: config.ts asserts argv[2] +// and VAPI_TOKEN at module load. Prime both before dynamic import. +process.argv = ["node", "test", "test-fixture-org"]; +process.env.VAPI_TOKEN = process.env.VAPI_TOKEN || "test-token-not-used"; + +const { reconcileStateKeyForResource } = await import( + "../src/reconcile-state-key.ts" +); + +import type { RemoteResource } from "../src/dep-dedup.ts"; +import type { TouchedSets } from "../src/state-merge.ts"; +import type { + ResourceFile, + ResourceState, + ResourceType, + StateFile, +} from "../src/types.ts"; + +// ───────────────────────────────────────────────────────────────────────────── +// Test fixtures — every scenario is run twice (tools + structuredOutputs) +// to prove the helper is genuinely uniform. Each scenario builds the inputs +// with the helpers below, runs reconcileStateKeyForResource, then asserts on +// state mutations, touched sets, applied counters, and apply-fn invocations. +// ───────────────────────────────────────────────────────────────────────────── + +type RType = "tools" | "structuredOutputs"; + +function emptyState(): StateFile { + return { + credentials: {}, + assistants: {}, + structuredOutputs: {}, + tools: {}, + squads: {}, + personalities: {}, + scenarios: {}, + simulations: {}, + simulationSuites: {}, + evals: {}, + }; +} + +function emptyTouched(): TouchedSets { + return { + tools: new Set(), + structuredOutputs: new Set(), + assistants: new Set(), + squads: new Set(), + personalities: new Set(), + scenarios: new Set(), + simulations: new Set(), + simulationSuites: new Set(), + evals: new Set(), + credentials: new Set(), + }; +} + +function emptyApplied(): Record { + return { + tools: 0, + structuredOutputs: 0, + assistants: 0, + squads: 0, + personalities: 0, + scenarios: 0, + simulations: 0, + simulationSuites: 0, + evals: 0, + }; +} + +// Construct a payload whose canonical name slugifies to the same value for +// both types. Tools use `function.name`, SOs use top-level `name`. +function makePayload(rtype: RType, name: string): Record { + if (rtype === "tools") return { function: { name } }; + return { name }; +} + +function makeResource( + rtype: RType, + resourceId: string, + name: string, +): ResourceFile> { + return { + resourceId, + filePath: `/fake/${rtype}/${resourceId}.yml`, + data: makePayload(rtype, name), + }; +} + +interface Harness { + state: StateFile; + touched: TouchedSets; + applied: Record; + autoApplied: Set; + autoAppliedList: ResourceFile>[]; + applyCalls: { resourceId: string }[]; +} + +function makeHarness(): Harness { + return { + state: emptyState(), + touched: emptyTouched(), + applied: emptyApplied(), + autoApplied: new Set(), + autoAppliedList: [], + applyCalls: [], + }; +} + +interface RunOpts { + rtype: RType; + resource: ResourceFile>; + remoteList?: RemoteResource[]; + applyResult?: string | null; + applyThrows?: Error; + harness: Harness; +} + +async function runReconcile(opts: RunOpts): Promise { + const { rtype, resource, remoteList = [], harness } = opts; + await reconcileStateKeyForResource({ + resourceType: rtype, + resource, + state: harness.state, + touched: harness.touched, + applied: harness.applied, + autoApplied: harness.autoApplied, + pushToAutoAppliedList: (r) => harness.autoAppliedList.push(r), + getRemoteList: async () => remoteList, + applyFn: async (r, _state) => { + harness.applyCalls.push({ resourceId: r.resourceId }); + if (opts.applyThrows) throw opts.applyThrows; + // Default: return a UUID matching whatever state has under the + // resourceId, falling back to a synthetic uuid. Mirrors what the real + // applyTool would return after a successful PATCH/POST. + if (opts.applyResult !== undefined) return opts.applyResult; + const existing = harness.state[rtype][r.resourceId]?.uuid; + return existing ?? `uuid-${r.resourceId}-created`; + }, + vapiEnv: "test-env", + }); +} + +const RTYPES: RType[] = ["tools", "structuredOutputs"]; + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 1 — State hit, SAME uuid, redundant alias. +// State already has `-` pointing at the same UUID we'd be +// adopting from the dashboard. The alias should be dropped and marked +// touched; the canonical key keeps the adopted UUID. applyFn is called. +// ───────────────────────────────────────────────────────────────────────────── + +for (const rtype of RTYPES) { + test(`[${rtype}] scenario 1: state hit, same uuid, redundant alias dropped`, async () => { + const h = makeHarness(); + const sharedUuid = "00000000-0000-0000-0000-000000000aaa"; + // Pre-existing alias under a bootstrap-renamed key, same UUID. + h.state[rtype]["my-resource-12345678"] = { uuid: sharedUuid }; + const resource = makeResource(rtype, "my-resource", "my-resource"); + + await runReconcile({ + rtype, + resource, + remoteList: [{ id: sharedUuid, name: "my-resource" }], + harness: h, + }); + + // Canonical key now points at the adopted UUID (with lastPushedHash + // populated after applyFn success). + assert.equal(h.state[rtype]["my-resource"]?.uuid, sharedUuid); + assert.ok(h.state[rtype]["my-resource"]?.lastPushedHash); + // Alias was deleted. + assert.equal(h.state[rtype]["my-resource-12345678"], undefined); + // Both keys marked touched: the deletion AND the new canonical entry. + assert.ok(h.touched[rtype].has("my-resource-12345678")); + assert.ok(h.touched[rtype].has("my-resource")); + // applyFn was called once. + assert.equal(h.applyCalls.length, 1); + // applied counter incremented, bookkeeping populated. + assert.equal(h.applied[rtype], 1); + assert.equal(h.autoAppliedList.length, 1); + assert.ok(h.autoApplied.has(`${rtype}:my-resource`)); + }); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 2 — State hit, DIFFERENT uuid, base-slug match. +// State has `-` pointing at uuid A; local file wants the +// canonical slot. We adopt uuid A, rekey state to the canonical key, and +// delete the suffixed entry. +// ───────────────────────────────────────────────────────────────────────────── + +for (const rtype of RTYPES) { + test(`[${rtype}] scenario 2: state hit, different uuid, adopts and rekeys`, async () => { + const h = makeHarness(); + const adoptedUuid = "11111111-1111-1111-1111-111111111aaa"; + h.state[rtype]["end-call-67aea057"] = { uuid: adoptedUuid }; + const resource = makeResource(rtype, "end-call", "end-call"); + + await runReconcile({ + rtype, + resource, + remoteList: [{ id: adoptedUuid, name: "end-call" }], + harness: h, + }); + + assert.equal(h.state[rtype]["end-call"]?.uuid, adoptedUuid); + assert.equal(h.state[rtype]["end-call-67aea057"], undefined); + assert.ok(h.touched[rtype].has("end-call-67aea057")); + assert.ok(h.touched[rtype].has("end-call")); + assert.equal(h.applyCalls.length, 1); + assert.equal(h.applied[rtype], 1); + }); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 3 — Dashboard hit only (state empty, dashboard has same-name). +// Pure dashboard adoption: no prior state entry, but a live dashboard +// resource shares the slugified name. We adopt and PATCH. +// ───────────────────────────────────────────────────────────────────────────── + +for (const rtype of RTYPES) { + test(`[${rtype}] scenario 3: dashboard hit only, adopts remote UUID`, async () => { + const h = makeHarness(); + const dashboardUuid = "22222222-2222-2222-2222-222222222aaa"; + const resource = makeResource(rtype, "my-resource", "My Resource"); + + await runReconcile({ + rtype, + resource, + remoteList: [{ id: dashboardUuid, name: "My Resource" }], + harness: h, + }); + + assert.equal(h.state[rtype]["my-resource"]?.uuid, dashboardUuid); + assert.equal(h.applyCalls.length, 1); + assert.equal(h.applied[rtype], 1); + assert.ok(h.autoApplied.has(`${rtype}:my-resource`)); + }); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 4 — Ambiguous dashboard match. +// Two dashboard resources share the slugified name. We pick the +// lex-smallest, surface ambiguous=true via the warning log, and DO NOT +// touch the other UUIDs' state entries. +// ───────────────────────────────────────────────────────────────────────────── + +for (const rtype of RTYPES) { + test(`[${rtype}] scenario 4: ambiguous dashboard match, picks lex-smallest`, async () => { + const h = makeHarness(); + const uuidA = "11111111-1111-1111-1111-111111111aaa"; + const uuidB = "22222222-2222-2222-2222-222222222aaa"; + const resource = makeResource(rtype, "dup", "dup"); + + // Capture warning output to confirm the ambiguous path fired. + const warnings: string[] = []; + const origWarn = console.warn; + console.warn = (msg: string) => { + warnings.push(msg); + }; + try { + await runReconcile({ + rtype, + resource, + remoteList: [ + { id: uuidB, name: "dup" }, + { id: uuidA, name: "dup" }, + ], + harness: h, + }); + } finally { + console.warn = origWarn; + } + + // Lex-smallest wins → uuidA. + assert.equal(h.state[rtype]["dup"]?.uuid, uuidA); + assert.ok(warnings.some((w) => w.includes("Multiple"))); + assert.ok(warnings.some((w) => w.includes(uuidB))); + assert.equal(h.applied[rtype], 1); + }); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 5 — No match, pure create path. +// State empty, dashboard empty. We fall through to the create branch: +// applyFn called, state populated with the fresh UUID, applied++, +// autoApplied set populated, bookkeeping array pushed. +// ───────────────────────────────────────────────────────────────────────────── + +for (const rtype of RTYPES) { + test(`[${rtype}] scenario 5: no match, pure create path`, async () => { + const h = makeHarness(); + const resource = makeResource(rtype, "brand-new", "brand-new"); + + await runReconcile({ + rtype, + resource, + applyResult: "33333333-3333-3333-3333-333333333aaa", + harness: h, + }); + + assert.equal( + h.state[rtype]["brand-new"]?.uuid, + "33333333-3333-3333-3333-333333333aaa", + ); + assert.ok(h.state[rtype]["brand-new"]?.lastPushedHash); + assert.equal(h.applied[rtype], 1); + assert.equal(h.autoAppliedList.length, 1); + assert.ok(h.autoApplied.has(`${rtype}:brand-new`)); + assert.ok(h.touched[rtype].has("brand-new")); + assert.equal(h.applyCalls.length, 1); + }); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 6 — Idempotency. +// The caller (push.ts ensureToolExists / ensureStructuredOutputExists) is +// responsible for the autoApplied short-circuit; this helper itself runs +// unconditionally. We verify the caller's guard works by simulating two +// invocations from a wrapper that mimics the production short-circuit. +// ───────────────────────────────────────────────────────────────────────────── + +for (const rtype of RTYPES) { + test(`[${rtype}] scenario 6: caller-side autoApplied short-circuit prevents 2nd reconcile`, async () => { + const h = makeHarness(); + const resource = makeResource(rtype, "once", "once"); + const autoAppliedKey = `${rtype}:once`; + + async function callOnce(): Promise { + if (h.autoApplied.has(autoAppliedKey)) return; + await runReconcile({ + rtype, + resource, + applyResult: "44444444-4444-4444-4444-444444444aaa", + harness: h, + }); + } + + await callOnce(); + await callOnce(); + + // Second call must short-circuit before reconcile runs. + assert.equal(h.applyCalls.length, 1); + assert.equal(h.applied[rtype], 1); + assert.equal(h.autoAppliedList.length, 1); + }); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 7 — applyFn returns null. +// Matches push.ts:998 / push.ts:1093 semantics exactly: autoApplied IS +// recorded, but state hash, applied counter, bookkeeping array, and +// touched set are NOT updated for the resourceId. +// ───────────────────────────────────────────────────────────────────────────── + +for (const rtype of RTYPES) { + test(`[${rtype}] scenario 7: applyFn returns null — autoApplied recorded, counter not incremented`, async () => { + const h = makeHarness(); + const resource = makeResource(rtype, "skipme", "skipme"); + + await runReconcile({ + rtype, + resource, + applyResult: null, + harness: h, + }); + + // autoApplied IS recorded (even on null result) — this is the + // "we tried; don't try again this run" signal. + assert.ok(h.autoApplied.has(`${rtype}:skipme`)); + // State NOT updated with hash, applied NOT incremented, bookkeeping + // NOT populated, touched NOT marked for the resourceId. + assert.equal(h.state[rtype]["skipme"], undefined); + assert.equal(h.applied[rtype], 0); + assert.equal(h.autoAppliedList.length, 0); + assert.equal(h.touched[rtype].has("skipme"), false); + assert.equal(h.applyCalls.length, 1); + }); +} + +// ───────────────────────────────────────────────────────────────────────────── +// Scenario 8 — Orphan-deletion guard scope. +// Only state keys pointing at the ADOPTED UUID are dropped. Keys pointing +// at OTHER UUIDs (including those flagged as match.duplicateUuids — real +// on-dashboard duplicates left for `npm run cleanup`) MUST be left alone. +// ───────────────────────────────────────────────────────────────────────────── + +for (const rtype of RTYPES) { + test(`[${rtype}] scenario 8: orphan-deletion only drops keys pointing at the adopted uuid`, async () => { + const h = makeHarness(); + const adoptedUuid = "11111111-1111-1111-1111-111111111aaa"; + const duplicateUuid = "22222222-2222-2222-2222-222222222aaa"; + + // Three state entries: + // - alias A pointing at the adopted uuid → MUST be deleted + // - alias B pointing at the duplicate uuid → MUST be preserved + // - alias C pointing at an unrelated uuid → MUST be preserved + h.state[rtype]["dup-aaaaaaaa"] = { uuid: adoptedUuid }; + h.state[rtype]["dup-bbbbbbbb"] = { uuid: duplicateUuid }; + h.state[rtype]["unrelated-cccccccc"] = { + uuid: "99999999-9999-9999-9999-999999999aaa", + }; + + const resource = makeResource(rtype, "dup", "dup"); + + // Capture warning so the ambiguous test path doesn't pollute output. + const origWarn = console.warn; + console.warn = (): void => {}; + try { + await runReconcile({ + rtype, + resource, + remoteList: [ + { id: adoptedUuid, name: "dup" }, + { id: duplicateUuid, name: "dup" }, + ], + harness: h, + }); + } finally { + console.warn = origWarn; + } + + // Canonical key adopts the lex-smallest UUID. + assert.equal(h.state[rtype]["dup"]?.uuid, adoptedUuid); + // Alias pointing at the adopted UUID: deleted. + assert.equal(h.state[rtype]["dup-aaaaaaaa"], undefined); + assert.ok(h.touched[rtype].has("dup-aaaaaaaa")); + // Alias pointing at the duplicate UUID: preserved. + const dupEntry: ResourceState | undefined = h.state[rtype]["dup-bbbbbbbb"]; + assert.equal(dupEntry?.uuid, duplicateUuid); + assert.equal(h.touched[rtype].has("dup-bbbbbbbb"), false); + // Unrelated entry: preserved. + assert.equal( + h.state[rtype]["unrelated-cccccccc"]?.uuid, + "99999999-9999-9999-9999-999999999aaa", + ); + assert.equal(h.touched[rtype].has("unrelated-cccccccc"), false); + }); +}