test(cli): migrate comprehensive test suite from test/cli-integration-coverage

cli/test/integration/auth-resolution.test.ts

@@ -0,0 +1,159 @@
+/**
+ * End-to-end integration coverage for token / registry priority resolution.
+ *
+ * The unit test in test/unit/services/registry-service.test.ts pins the
+ * resolution function in isolation. These tests verify the same priorities
+ * are wired through the actual CLI subprocess: --flag > SKILLHUB_* env >
+ * stored config / credentials > built-in default.
+ *
+ * Why this matters: a regression in the wiring (e.g. command forgets to
+ * forward `process.env`) would silently downgrade users to the wrong
+ * registry / token without surfacing in unit tests.
+ */
+import { mkdir, writeFile } from 'node:fs/promises'
+import { join } from 'node:path'
+import { afterEach, describe, expect, test } from 'bun:test'
+import { startFakeRegistry } from '../helpers/fake-registry'
+import { runCli } from '../helpers/run-cli'
+import { createTempHome } from '../helpers/temp-env'
+
+let registry: Awaited<ReturnType<typeof startFakeRegistry>> | undefined
+let registryB: Awaited<ReturnType<typeof startFakeRegistry>> | undefined
+
+afterEach(() => {
+  registry?.stop(); registry = undefined
+  registryB?.stop(); registryB = undefined
+})
+
+async function seedCredentials(home: string, registryUrl: string, token: string): Promise<void> {
+  await mkdir(join(home, '.skillhub'), { recursive: true })
+  await writeFile(
+    join(home, '.skillhub', 'credentials.json'),
+    JSON.stringify({ tokens: { [registryUrl]: token } })
+  )
+}
+
+async function seedConfig(home: string, registryUrl: string): Promise<void> {
+  await mkdir(join(home, '.skillhub'), { recursive: true })
+  await writeFile(
+    join(home, '.skillhub', 'config.json'),
+    JSON.stringify({ registry: registryUrl })
+  )
+}
+
+// ---------------------------------------------------------------------------
+// Token priority: --token > SKILLHUB_TOKEN > stored
+// ---------------------------------------------------------------------------
+
+describe('auth resolution — token priority', () => {
+  test('--token flag wins over SKILLHUB_TOKEN env', async () => {
+    const env = await createTempHome()
+    registry = await startFakeRegistry({
+      token: 'sk_from_flag',
+      user: { handle: 'flag-user', displayName: 'Flag' }
+    })
+
+    const result = await runCli(
+      ['whoami', '--registry', registry.url, '--token', 'sk_from_flag'],
+      { HOME: env.home, USERPROFILE: env.home, SKILLHUB_TOKEN: 'sk_wrong_from_env' }
+    )
+    expect(result.exitCode).toBe(0)
+    expect(result.stdout).toContain('flag-user')
+  })
+
+  test('SKILLHUB_TOKEN env wins over stored token', async () => {
+    const env = await createTempHome()
+    registry = await startFakeRegistry({
+      token: 'sk_from_env',
+      user: { handle: 'env-user', displayName: 'Env' }
+    })
+    await seedCredentials(env.home, registry.url, 'sk_wrong_from_storage')
+
+    const result = await runCli(
+      ['whoami', '--registry', registry.url],
+      { HOME: env.home, USERPROFILE: env.home, SKILLHUB_TOKEN: 'sk_from_env' }
+    )
+    expect(result.exitCode).toBe(0)
+    expect(result.stdout).toContain('env-user')
+  })
+
+  test('stored token used when neither --token nor env is set', async () => {
+    const env = await createTempHome()
+    registry = await startFakeRegistry({
+      token: 'sk_from_storage',
+      user: { handle: 'storage-user', displayName: 'Storage' }
+    })
+    await seedCredentials(env.home, registry.url, 'sk_from_storage')
+
+    const result = await runCli(
+      ['whoami', '--registry', registry.url],
+      { HOME: env.home, USERPROFILE: env.home }
+    )
+    expect(result.exitCode).toBe(0)
+    expect(result.stdout).toContain('storage-user')
+  })
+})
+
+// ---------------------------------------------------------------------------
+// Registry priority: --registry > SKILLHUB_REGISTRY > config.json
+// ---------------------------------------------------------------------------
+
+describe('auth resolution — registry priority', () => {
+  test('--registry flag wins over SKILLHUB_REGISTRY env', async () => {
+    const env = await createTempHome()
+    // Each registry only authenticates its own token. The wrong registry
+    // would 401, so a successful whoami proves the right one was used.
+    registry = await startFakeRegistry({
+      token: 'sk_a',
+      user: { handle: 'a-user', displayName: 'A' }
+    })
+    registryB = await startFakeRegistry({
+      token: 'sk_b',
+      user: { handle: 'b-user', displayName: 'B' }
+    })
+
+    const result = await runCli(
+      ['whoami', '--registry', registry.url, '--token', 'sk_a'],
+      { HOME: env.home, USERPROFILE: env.home, SKILLHUB_REGISTRY: registryB.url }
+    )
+    expect(result.exitCode).toBe(0)
+    expect(result.stdout).toContain('a-user')
+  })
+
+  test('SKILLHUB_REGISTRY env wins over config.registry', async () => {
+    const env = await createTempHome()
+    registry = await startFakeRegistry({
+      token: 'sk_env',
+      user: { handle: 'env-reg', displayName: 'EnvReg' }
+    })
+    registryB = await startFakeRegistry({
+      token: 'sk_config',
+      user: { handle: 'config-reg', displayName: 'ConfigReg' }
+    })
+    await seedConfig(env.home, registryB.url)
+
+    const result = await runCli(
+      ['whoami', '--token', 'sk_env'],
+      { HOME: env.home, USERPROFILE: env.home, SKILLHUB_REGISTRY: registry.url }
+    )
+    expect(result.exitCode).toBe(0)
+    expect(result.stdout).toContain('env-reg')
+  })
+
+  test('config.registry used when no --registry / env present', async () => {
+    const env = await createTempHome()
+    registry = await startFakeRegistry({
+      token: 'sk_config',
+      user: { handle: 'config-only-user', displayName: 'CfgOnly' }
+    })
+    await seedConfig(env.home, registry.url)
+    await seedCredentials(env.home, registry.url, 'sk_config')
+
+    const result = await runCli(
+      ['whoami'],
+      { HOME: env.home, USERPROFILE: env.home }
+    )
+    expect(result.exitCode).toBe(0)
+    expect(result.stdout).toContain('config-only-user')
+  })
+})

cli/test/integration/concurrency.test.ts

@@ -0,0 +1,164 @@
+/**
+ * Concurrency tests for inventory.json bookkeeping.
+ *
+ * inventory-store.ts uses an OS-level lock file with retry + stale-lock
+ * detection. These tests exercise that path through real CLI subprocesses
+ * (Bun.spawn) running in parallel — the same way users hit it when scripts
+ * fan out installs.
+ *
+ * The unit test in test/unit/stores/inventory-store.test.ts pins the
+ * single-process lock recovery; here we cover the cross-process case.
+ */
+import { mkdir, readFile, writeFile } from 'node:fs/promises'
+import { join } from 'node:path'
+import { afterEach, describe, expect, test } from 'bun:test'
+import { zipSync, strToU8 } from 'fflate'
+import { startFakeRegistry } from '../helpers/fake-registry'
+import { runCli } from '../helpers/run-cli'
+import { createTempHome } from '../helpers/temp-env'
+
+let registry: Awaited<ReturnType<typeof startFakeRegistry>> | undefined
+
+afterEach(() => {
+  registry?.stop(); registry = undefined
+})
+
+function makeSkillZip(): Uint8Array {
+  return zipSync({ 'SKILL.md': strToU8('# c') })
+}
+
+describe('cross-process concurrency on inventory.json', () => {
+  // KNOWN BUG (documented here, not yet fixed):
+  //   inventory-store.upsertTarget() reads inventory, modifies in memory,
+  //   then writeAtomic() acquires the lock only over the write half. Two
+  //   concurrent installs each read the (empty) inventory, each adds their
+  //   own item, and the second writer overwrites the first — a classic
+  //   lost-update.
+  //
+  //   When the fix lands (lock spans read+write, or upsertTarget acquires
+  //   the lock first and re-reads), tighten the inventory assertion to
+  //   `expect(slugs).toEqual(['first', 'second'])`.
+  test('two parallel installs of distinct slugs: filesystem is correct, inventory has at least one (lost-update bug pinned)', async () => {
+    const env = await createTempHome()
+    registry = await startFakeRegistry({
+      token: 'sk_ok',
+      user: { handle: 'u', displayName: 'U' },
+      skills: [
+        { namespace: 'global', slug: 'first', version: '1.0.0', zipBytes: makeSkillZip() },
+        { namespace: 'global', slug: 'second', version: '1.0.0', zipBytes: makeSkillZip() }
+      ]
+    })
+    await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home })
+
+    const dirA = join(env.cwd, 'A')
+    const dirB = join(env.cwd, 'B')
+    await mkdir(dirA, { recursive: true })
+    await mkdir(dirB, { recursive: true })
+
+    const [r1, r2] = await Promise.all([
+      runCli(
+        ['install', 'first', '--dir', dirA, '--registry', registry.url, '--token', 'sk_ok'],
+        { HOME: env.home, USERPROFILE: env.home }
+      ),
+      runCli(
+        ['install', 'second', '--dir', dirB, '--registry', registry.url, '--token', 'sk_ok'],
+        { HOME: env.home, USERPROFILE: env.home }
+      )
+    ])
+
+    // Both subprocess installs report success — neither errored at the
+    // protocol level even though the inventory bookkeeping race ate one of
+    // their inventory writes.
+    expect(r1.exitCode).toBe(0)
+    expect(r2.exitCode).toBe(0)
+
+    // Filesystem is correct: both bundles extracted independently.
+    expect(await Bun.file(join(dirA, 'first', 'SKILL.md')).exists()).toBe(true)
+    expect(await Bun.file(join(dirB, 'second', 'SKILL.md')).exists()).toBe(true)
+
+    const inv = JSON.parse(
+      await readFile(join(env.home, '.skillhub', 'inventory.json'), 'utf-8')
+    ) as { items: Array<{ slug: string }> }
+    const slugs = inv.items.map(i => i.slug).sort()
+    // Today: at least one slug always lands; under the lost-update race
+    // both may NOT be there. When the lock widens to cover read+write,
+    // upgrade this to `toEqual(['first', 'second'])`.
+    expect(slugs.length).toBeGreaterThanOrEqual(1)
+    const lastSlug = slugs[slugs.length - 1]!
+    expect(['first', 'second']).toContain(lastSlug)
+  })
+
+  test('two parallel installs of the same slug to the same dir: exactly one wins, one conflicts', async () => {
+    const env = await createTempHome()
+    registry = await startFakeRegistry({
+      token: 'sk_ok',
+      user: { handle: 'u', displayName: 'U' },
+      skills: [{ namespace: 'global', slug: 'race', version: '1.0.0', zipBytes: makeSkillZip() }]
+    })
+    await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home })
+
+    const installDir = join(env.cwd, 'race-dir')
+    await mkdir(installDir, { recursive: true })
+
+    const [r1, r2] = await Promise.all([
+      runCli(
+        ['install', 'race', '--dir', installDir, '--registry', registry.url, '--token', 'sk_ok'],
+        { HOME: env.home, USERPROFILE: env.home }
+      ),
+      runCli(
+        ['install', 'race', '--dir', installDir, '--registry', registry.url, '--token', 'sk_ok'],
+        { HOME: env.home, USERPROFILE: env.home }
+      )
+    ])
+
+    // Two valid outcomes: (a) both succeed because the loser's existence
+    // check ran BEFORE the winner extracted, OR (b) one succeeds and the
+    // other reports already-installed (EXIT.filesystem).
+    // Either way, inventory must end up coherent (single item, single
+    // target — no duplicates).
+    const codes = [r1.exitCode, r2.exitCode].sort((a, b) => a - b)
+    expect(codes[0]).toBe(0) // at least one succeeded
+    const otherCode = codes[1]!
+    expect([0, 4]).toContain(otherCode) // other either succeeded or got conflict
+
+    const inv = JSON.parse(
+      await readFile(join(env.home, '.skillhub', 'inventory.json'), 'utf-8')
+    ) as { items: Array<{ slug: string; targets: Array<{ installDir: string }> }> }
+    const item = inv.items.find(i => i.slug === 'race')
+    expect(item).toBeDefined()
+    expect(item!.targets).toHaveLength(1) // no duplicate targets
+  })
+
+  test('install proceeds after a stale lock file from a dead process', async () => {
+    const env = await createTempHome()
+    registry = await startFakeRegistry({
+      token: 'sk_ok',
+      user: { handle: 'u', displayName: 'U' },
+      skills: [{ namespace: 'global', slug: 'after-stale', version: '1.0.0', zipBytes: makeSkillZip() }]
+    })
+    await runCli(['login', '--registry', registry.url, '--token', 'sk_ok'], { HOME: env.home, USERPROFILE: env.home })
+
+    // Plant a stale lock file: PID 1 (init, never the same as our test
+    // child, and won't match the spawned subprocess's PID), with a very
+    // old timestamp so the store treats it as stale.
+    const skillhubDir = join(env.home, '.skillhub')
+    await mkdir(skillhubDir, { recursive: true })
+    const lockPath = join(skillhubDir, 'inventory.json.lock')
+    const ancientTimestamp = Date.now() - 600_000 // 10 minutes ago — past the 30s stale threshold
+    await writeFile(lockPath, JSON.stringify({ pid: 1, timestamp: ancientTimestamp }))
+
+    const installDir = join(env.cwd, 'stale')
+    await mkdir(installDir, { recursive: true })
+
+    const result = await runCli(
+      ['install', 'after-stale', '--dir', installDir, '--registry', registry.url, '--token', 'sk_ok'],
+      { HOME: env.home, USERPROFILE: env.home }
+    )
+    expect(result.exitCode).toBe(0)
+
+    const inv = JSON.parse(
+      await readFile(join(skillhubDir, 'inventory.json'), 'utf-8')
+    ) as { items: Array<{ slug: string }> }
+    expect(inv.items.find(i => i.slug === 'after-stale')).toBeDefined()
+  })
+})