refactor: simplify lockfile parsers and core logic for maintainability

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@patchstack/connect",
-  "version": "0.2.5",
+  "version": "0.2.6",
   "description": "Patchstack connector for JavaScript applications. Scans your lockfile and reports installed packages to Patchstack for vulnerability monitoring.",
   "keywords": [
     "patchstack",

src/cli.ts

@@ -1,7 +1,7 @@
 import { scanLockfile } from './parsers/index.js';
 import { buildWirePayload } from './normalize.js';
 import { buildClaimUrl, postManifest } from './client.js';
-import { persistSiteUuid, resolveConfig, writeConfigFile } from './config.js';
+import { isUuid, persistSiteUuid, resolveConfig, writeConfigFile } from './config.js';
 import { PatchstackError } from './types.js';
 
 const HELP = `@patchstack/connect — scan your lockfile and report packages to Patchstack.
@@ -87,7 +87,7 @@ async function runInit(args: ParsedArgs): Promise<number> {
     console.error('Usage: patchstack-connect init <site-uuid>');
     return 1;
   }
-  if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(uuid)) {
+  if (!isUuid(uuid)) {
     console.error(`Error: "${uuid}" does not look like a valid UUID.`);
     return 1;
   }
@@ -142,11 +142,18 @@ async function runScan(args: ParsedArgs): Promise<number> {
 
   const response = await postManifest(config, payload);
 
-  // The server always returns the UUID. If we didn't have one, persist it so
-  // every subsequent scan targets the same site.
-  if (provisioning && response.uuid !== undefined && response.uuid.length > 0) {
-    const target = await persistSiteUuid(process.cwd(), response.uuid);
-    console.log(`Provisioned site ${response.uuid}. Saved UUID to ${target}.`);
+  // When provisioning, the server returns a fresh UUID for this site. (null in
+  // every other case — either we already had one, or the server didn't send one.)
+  const provisionedUuid =
+    provisioning && response.uuid !== undefined && response.uuid.length > 0
+      ? response.uuid
+      : null;
+
+  // If we didn't have a UUID, persist the provisioned one so every subsequent
+  // scan targets the same site.
+  if (provisionedUuid !== null) {
+    const target = await persistSiteUuid(process.cwd(), provisionedUuid);
+    console.log(`Provisioned site ${provisionedUuid}. Saved UUID to ${target}.`);
   }
 
   if (response.stored) {
@@ -160,10 +167,10 @@ async function runScan(args: ParsedArgs): Promise<number> {
   // On the first scan (provisioning), surface the claim URL so the user can
   // attach this site to their Patchstack account. `npx @patchstack/connect status`
   // re-displays it any time.
-  if (provisioning && response.uuid !== undefined && response.uuid.length > 0) {
+  if (provisionedUuid !== null) {
     console.log('');
     console.log('Claim this site to view vulnerability reports in your Patchstack dashboard:');
-    console.log(`  ${buildClaimUrl(config.endpoint, response.uuid)}`);
+    console.log(`  ${buildClaimUrl(config.endpoint, provisionedUuid)}`);
   }
 
   return 0;

src/client.ts

@@ -79,7 +79,7 @@ export async function postManifest(
     );
   }
 
-  if (response.status < 200 || response.status >= 300) {
+  if (!isSuccessStatus(response.status)) {
     throw new PatchstackError(
       `Patchstack returned ${response.status}: ${text.slice(0, 200)}`,
       'SERVER_ERROR',
@@ -93,6 +93,10 @@ export async function postManifest(
   return body;
 }
 
+function isSuccessStatus(status: number): boolean {
+  return status >= 200 && status < 300;
+}
+
 function isTimeoutError(cause: unknown): boolean {
   if (cause instanceof Error) {
     return cause.name === 'TimeoutError' || cause.name === 'AbortError';

src/config.ts

@@ -126,6 +126,6 @@ function readEnv(): ConfigFile {
   };
 }
 
-function isUuid(value: string): boolean {
+export function isUuid(value: string): boolean {
   return /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(value);
 }

src/normalize.ts

@@ -101,37 +101,47 @@ function compareSegments(a: string[], b: string[]): number {
   for (let i = 0; i < max; i++) {
     const aPart = a[i];
     const bPart = b[i];
+    // A shorter segment list sorts before a longer one (e.g. `1.0` before `1.0.1`).
     if (aPart === undefined) {
       return -1;
     }
     if (bPart === undefined) {
       return 1;
     }
-    const aNum = /^\d+$/.test(aPart);
-    const bNum = /^\d+$/.test(bPart);
-    if (aNum && bNum) {
-      const diff = Number(aPart) - Number(bPart);
-      if (diff !== 0) {
-        return diff < 0 ? -1 : 1;
-      }
-      continue;
-    }
-    if (aNum) {
-      return -1;
-    }
-    if (bNum) {
-      return 1;
-    }
-    if (aPart < bPart) {
-      return -1;
-    }
-    if (aPart > bPart) {
-      return 1;
+    const cmp = comparePartTokens(aPart, bPart);
+    if (cmp !== 0) {
+      return cmp;
     }
   }
   return 0;
 }
 
+/**
+ * Compares two version tokens. Numeric tokens sort numerically and ahead of
+ * non-numeric ones; otherwise tokens fall back to lexical comparison.
+ */
+function comparePartTokens(a: string, b: string): number {
+  const aNum = /^\d+$/.test(a);
+  const bNum = /^\d+$/.test(b);
+  if (aNum && bNum) {
+    const diff = Number(a) - Number(b);
+    return diff === 0 ? 0 : diff < 0 ? -1 : 1;
+  }
+  if (aNum) {
+    return -1;
+  }
+  if (bNum) {
+    return 1;
+  }
+  if (a < b) {
+    return -1;
+  }
+  if (a > b) {
+    return 1;
+  }
+  return 0;
+}
+
 export function findPackageInManifest(
   manifest: Manifest,
   name: string,

src/parsers/index.ts

@@ -26,55 +26,26 @@ interface DetectedLockfile {
   strategy: DetectionStrategy;
 }
 
-export async function detectLockfile(cwd: string): Promise<DetectedLockfile> {
-  const npmLock = path.join(cwd, 'package-lock.json');
-  if (await exists(npmLock)) {
-    return {
-      ecosystem: 'npm',
-      filePath: npmLock,
-      filename: 'package-lock.json',
-      strategy: 'npm-lockfile',
-    };
-  }
-
-  const bunLock = path.join(cwd, 'bun.lock');
-  if (await exists(bunLock)) {
-    return {
-      ecosystem: 'npm',
-      filePath: bunLock,
-      filename: 'bun.lock',
-      strategy: 'node-modules-walk',
-    };
-  }
-
-  const bunLockB = path.join(cwd, 'bun.lockb');
-  if (await exists(bunLockB)) {
-    return {
-      ecosystem: 'npm',
-      filePath: bunLockB,
-      filename: 'bun.lockb',
-      strategy: 'node-modules-walk',
-    };
-  }
-
-  const pnpmLock = path.join(cwd, 'pnpm-lock.yaml');
-  if (await exists(pnpmLock)) {
-    return {
-      ecosystem: 'npm',
-      filePath: pnpmLock,
-      filename: 'pnpm-lock.yaml',
-      strategy: 'pnpm-lockfile',
-    };
-  }
+// Probed in order; the first match wins, so more specific lockfiles must come
+// before fallbacks. bun has no standalone parser, so it falls back to walking
+// node_modules.
+const LOCKFILE_SPECS: ReadonlyArray<{
+  filename: LockfileFilename;
+  strategy: DetectionStrategy;
+}> = [
+  { filename: 'package-lock.json', strategy: 'npm-lockfile' },
+  { filename: 'bun.lock', strategy: 'node-modules-walk' },
+  { filename: 'bun.lockb', strategy: 'node-modules-walk' },
+  { filename: 'pnpm-lock.yaml', strategy: 'pnpm-lockfile' },
+  { filename: 'yarn.lock', strategy: 'yarn-lockfile' },
+];
 
-  const yarnLock = path.join(cwd, 'yarn.lock');
-  if (await exists(yarnLock)) {
-    return {
-      ecosystem: 'npm',
-      filePath: yarnLock,
-      filename: 'yarn.lock',
-      strategy: 'yarn-lockfile',
-    };
+export async function detectLockfile(cwd: string): Promise<DetectedLockfile> {
+  for (const { filename, strategy } of LOCKFILE_SPECS) {
+    const filePath = path.join(cwd, filename);
+    if (await exists(filePath)) {
+      return { ecosystem: 'npm', filePath, filename, strategy };
+    }
   }
 
   throw new PatchstackError(

src/parsers/npm.ts

@@ -1,5 +1,4 @@
 import { readFile } from 'node:fs/promises';
-import path from 'node:path';
 import { PatchstackError, type PackageEntry } from '../types.js';
 
 interface LockfileV2Package {
@@ -96,11 +95,11 @@ function extractFromV1(
 }
 
 function extractNameFromPath(pkgPath: string): string | null {
-  const segments = pkgPath.split('node_modules' + path.sep === pkgPath ? path.sep : '/');
+  // npm lockfile keys always use forward slashes, regardless of platform.
   const parts = pkgPath.split('/');
   const nmIndex = parts.lastIndexOf('node_modules');
   if (nmIndex === -1 || nmIndex >= parts.length - 1) {
-    return segments[segments.length - 1] ?? null;
+    return parts[parts.length - 1] ?? null;
   }
   const tail = parts.slice(nmIndex + 1);
   if (tail.length === 0) {

src/parsers/pnpm.ts

@@ -1,5 +1,6 @@
 import { readFile } from 'node:fs/promises';
 import { PatchstackError, type PackageEntry } from '../types.js';
+import { unquote } from './strings.js';
 
 /**
  * Parses pnpm-lock.yaml without pulling in a YAML library. We don't need full
@@ -62,12 +63,7 @@ export function parsePackageKey(rawKey: string): ParsedKey | null {
     return null;
   }
 
-  if (
-    (k.startsWith("'") && k.endsWith("'")) ||
-    (k.startsWith('"') && k.endsWith('"'))
-  ) {
-    k = k.slice(1, -1);
-  }
+  k = unquote(k);
 
   if (k.startsWith('/')) {
     k = k.slice(1);
@@ -343,15 +339,6 @@ function extractLeafName(trimmed: string): string | null {
   if (colonIdx < 0) {
     return null;
   }
-  let name = trimmed.slice(0, colonIdx).trim();
-  if (name.length === 0) {
-    return null;
-  }
-  if (
-    (name.startsWith("'") && name.endsWith("'")) ||
-    (name.startsWith('"') && name.endsWith('"'))
-  ) {
-    name = name.slice(1, -1);
-  }
+  const name = unquote(trimmed.slice(0, colonIdx).trim());
   return name.length > 0 ? name : null;
 }

src/parsers/strings.ts

@@ -0,0 +1,10 @@
+/** Strips a single matching pair of surrounding single or double quotes. */
+export function unquote(s: string): string {
+  if (
+    (s.startsWith("'") && s.endsWith("'")) ||
+    (s.startsWith('"') && s.endsWith('"'))
+  ) {
+    return s.slice(1, -1);
+  }
+  return s;
+}

src/parsers/yarn.ts

@@ -1,6 +1,7 @@
 import { readFile } from 'node:fs/promises';
 import path from 'node:path';
 import { PatchstackError, type PackageEntry } from '../types.js';
+import { unquote } from './strings.js';
 
 /**
  * Parses yarn.lock (yarn classic v1 and yarn berry v2+) without a YAML
@@ -173,16 +174,10 @@ export function splitDescriptors(keyLine: string): string[] {
  * version comes from the `version` field of the block.
  */
 export function extractName(rawSpec: string): string | null {
-  let s = rawSpec.trim();
+  const s = unquote(rawSpec.trim());
   if (s.length === 0) {
     return null;
   }
-  if (
-    (s.startsWith('"') && s.endsWith('"')) ||
-    (s.startsWith("'") && s.endsWith("'"))
-  ) {
-    s = s.slice(1, -1);
-  }
   // Position-0 `@` belongs to a scope, so we want the last `@` after it.
   const atIdx = s.lastIndexOf('@');
   if (atIdx <= 0) {
@@ -204,16 +199,7 @@ function parseVersionField(content: string): string | null {
     return null;
   }
   let rest = firstChar === ':' ? after.slice(1) : after;
-  rest = rest.trim();
-  if (rest.length === 0) {
-    return null;
-  }
-  if (
-    (rest.startsWith('"') && rest.endsWith('"')) ||
-    (rest.startsWith("'") && rest.endsWith("'"))
-  ) {
-    rest = rest.slice(1, -1);
-  }
+  rest = unquote(rest.trim());
   return rest.length > 0 ? rest : null;
 }