Skip to content

refactor(engine): extract shared slug + folder helpers into slug-utils#33

Closed
dhruva-reddy wants to merge 1 commit into
pull-rekey-one-way-bugfrom
engine-consolidation/01-shared-utils
Closed

refactor(engine): extract shared slug + folder helpers into slug-utils#33
dhruva-reddy wants to merge 1 commit into
pull-rekey-one-way-bugfrom
engine-consolidation/01-shared-utils

Conversation

@dhruva-reddy
Copy link
Copy Markdown
Contributor

Summary

Pure factoring PR. Consolidates duplicated helpers that accumulated across the gitops engine as symptom-fixes piled up. Zero behavior change at every reachable call site. Tier 1 of an engine consolidation series — surfaced by the post-mortem on the recanonicalize PR (#32, base of this stack).

What gets deduplicated

Helper Before After
`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 (`pull.ts`, `dep-dedup.ts`) 1
`FOLDER_MAP` 2 byte-identical copies (`pull.ts`, `resources.ts`) 1 in `resources.ts`
`UUID_SUFFIX_RE` Open-coded in 3 places 1 constant
recanonicalize precondition-2 (UUID prefix match) Inlined regex + comparison Extracted as `isEngineSuffixedSlug`

Why a separate module instead of "just import from pull.ts"

`src/slug-utils.ts` is config-free by design — no `./config.ts` import, no side effects at load. `config.ts` asserts `argv[2]` / `VAPI_TOKEN` at module load and `process.exit(1)`s under unit tests. Any test that imports a slug helper transitively via `pull.ts` would otherwise have to prime `process.argv` and `process.env.VAPI_TOKEN` (see `tests/recanonicalize.test.ts:7-8`).

The prior `dep-dedup.ts` comment claimed its local duplicates were "intentionally copied for testability" but `new-file-gate.ts` imported the helper from `pull.ts` anyway — so the testability claim didn't actually hold. The new `slug-utils.ts` makes it real: zero imports, importable from any test without ceremony.

Two-form contract: loose vs. strict

The helpers split into two semantically distinct forms that callers were previously mixing:

  • `extractBaseSlug(resourceId): string`loose. Best-effort strip of an engine-shape suffix; returns input unchanged on no-match. Used by `audit.ts`, `new-file-gate.ts`, `pull.ts`, `dep-dedup.ts` where there's no UUID handy to verify against.
  • `isEngineSuffixedSlug(stateKey, uuid): {base, suffix} | null`strict. Returns the parts only when the captured 8-hex matches the UUID's first 8 hex chars. Used by `recanonicalize.ts` to PROVE a key was engine-generated (rules out user-named resources coincidentally ending in `-abcd1234`).

Regex tightening (intentional)

Shared `UUID_SUFFIX_RE` uses `^(.+)-([0-9a-f]{8})$` (non-empty base). Prior pull/dep-dedup copies used `^(.*)-...` (allowed empty base). Strict improvement:

  • Engine-generated keys always have a non-empty base (`generateResourceId` produces `${slug}-${uuid8}` or `resource-${uuid8}`, both non-empty)
  • The only input class affected is the synthetic `-<8hex>` shape
  • Reachable call sites (`pull.ts:resourceIdMatchesName`, `audit.ts:checkSiblingBaseSlug`, `new-file-gate.ts:detectOrphanYamls`, `dep-dedup.ts:findExistingResourceByName`) all use `slugify(name)` upstream — `name` is non-empty by callsite guards, so `localSlug` is never `""`. Old behavior returned "no match"; new behavior returns "no match." No-op.
  • Pinned by `tests/slug-utils.test.ts:57-62` regression test

Files changed

File Change LOC
`src/slug-utils.ts` NEW (config-free utilities) +77
`tests/slug-utils.test.ts` NEW (20 cases) +170
`src/pull.ts` Delete `FOLDER_MAP`, `slugify`, `extractBaseSlug` locals; import from new modules -30
`src/dep-dedup.ts` Delete locals; import from `slug-utils`; re-export for back-compat -29
`src/audit.ts` Delete local `slugify`; re-route `extractBaseSlug` import -12
`src/setup.ts` Delete local `slugify`; import from `slug-utils` -9
`src/new-file-gate.ts` Re-route `extractBaseSlug` import (kept testability boundary clean) +-7
`src/recanonicalize.ts` Swap inlined precondition-2 check for `isEngineSuffixedSlug` -16

Net: +275 / -85 → +190. New file weight is mostly the test suite (170 lines) — source-code net is +105 / -85 = +20.

Test plan

  • `npm run build` (tsc --noEmit) — clean
  • `npm test` — 228/228 pass (208 prior + 20 new)
  • `npx @biomejs/biome check --write` — clean
  • Byte-equivalence: 4 `slugify` copies + 2 `extractBaseSlug` copies verified byte-identical to the new shared definitions (modulo `export` keyword)
  • Behavior preservation traced: every `extractBaseSlug` call site audited against the regex tightening — no observable change in realistic flows
  • Testability boundary: `tests/slug-utils.test.ts` does NOT prime `process.argv` / `VAPI_TOKEN` — proves the module is config-free
  • Back-compat: `dep-dedup.ts` re-exports `slugify` and `extractBaseSlug` so `tests/dep-dedup.test.ts` keeps importing via that path unchanged

Code review

In-branch review by code-reviewer subagent. No blocking findings. Four L-level cleanups addressed in-branch:

  • L1: removed dead `extractBaseSlug` re-export in `pull.ts` (no consumers)
  • L2: renamed stale "Slug helpers" section banner in `audit.ts` → "Resource name extraction"
  • L3: updated `new-file-gate.ts` JSDoc reference from "pull.ts helper" → "slug-utils helper"
  • L4: renamed `pull.ts` "Naming & Slug Generation" banner → "Resource naming" (slug generation moved out)

Stack

This is PR 1 of 2 in the engine-consolidation series. PR 2 stacks on this one and folds `ensureToolExists` / `ensureStructuredOutputExists` (both ~94 lines, structurally identical) into a generic `reconcileStateKeyForResource` helper. Reviewable alone — PR 2 is sequencing, not technical dependency.

Related

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).
@dhruva-reddy
Copy link
Copy Markdown
Contributor Author

Superseded by #35 (merged 2026-05-15). GitHub auto-closed this PR when the stack-parent base branch was deleted on #32's merge. Same diff, retargeted at main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant