From eae50eb4dedf5d0f6174b1a54780f9e8fc66bcd8 Mon Sep 17 00:00:00 2001 From: Dhruva Reddy Date: Thu, 14 May 2026 19:30:28 -0700 Subject: [PATCH] refactor(engine): extract shared slug + folder helpers into slug-utils 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", + }); +});