fix(instrument-repos): import every instrument a repository provides

.agents/docs/architecture/instrument-pipeline.md

@@ -44,11 +44,16 @@ Three places produce instrument source:
 | ---------------------------------- | ------------------------------------------------------------------------- | ------------------------------------------------------- |
 | Built-in catalog                   | `packages/instrument-library/src/{file,forms,interactive,series}/<NAME>/` | `instrument-bundler` CLI at package build time          |
 | Playground examples and user edits | `apps/playground/src/instruments/{examples,templates}/`                   | `bundle()` in the browser, via `esbuild-wasm`           |
-| External GitHub repos              | `lib/forms/*` and `lib/interactive/*` in the remote repo                  | `InstrumentReposService` on the API at import/sync time |
+| External GitHub repos              | `lib/{file,forms,interactive,series}/*` in the remote repo                | `InstrumentReposService` on the API at import/sync time |
 
-**Repo discovery only scans `lib/forms` and `lib/interactive`** (`discoverInstrumentDirs` in
-`apps/api/src/instrument-repos/instrument-repos.service.ts`). A `lib/file` or `lib/series` directory
-in an external repo is silently ignored.
+**Repo discovery returns `series` last, and callers must import in that order** (`discoverInstrumentDirs`
+in `apps/api/src/instrument-repos/instrument-repos.service.ts`). `InstrumentsService.create` rejects a
+series unless every instrument it references is already stored, so the scalars a repo provides have to
+be created before its series.
+
+**A series that references another repository still fails to import.** Nothing resolves dependencies
+across repositories or retries afterwards, so whether it works depends on the order the repos were
+added. Making this reliable needs a second resolution pass once every repo has been imported.
 
 ## 2. Bundling
 
@@ -217,6 +222,5 @@ instrument repository so an agent working _there_ can read it.
 
 Editing it changes a published package that third parties consume. It describes how to author an
 instrument in a standalone repo; it says nothing about working in this monorepo, and none of its
-rules apply to code outside an instrument directory. Its instruction to place instruments in
-`lib/forms` and `lib/interactive` is coupled to `discoverInstrumentDirs` on the API — change one and
-you must change the other.
+rules apply to code outside an instrument directory. Its instruction on where to place instruments is
+coupled to `discoverInstrumentDirs` on the API — change one and you must change the other.

.agents/skills/odc-instruments/SKILL.md

@@ -54,9 +54,10 @@ emitted directory exists under `runtime/v1/dist` before believing the build.
 **A built instrument is not a loaded one.** The built-in catalog takes one hand-written default import and one
 `create` call per instrument in `apps/api/src/demo/demo.service.ts` (`.agents/docs/playbooks/add-instrument.md`
 is where that step lives); `src/interactive/DNP_STROOP_TASK` is the standing proof — it builds, `pnpm run available`
-lists it, and it is in no demo instance. For an external repository, discovery scans only `lib/forms` and
-`lib/interactive` (`.agents/docs/architecture/instrument-pipeline.md`), so a `lib/file` or `lib/series`
-directory is skipped silently.
+lists it, and it is in no demo instance. For an external repository, discovery scans
+`lib/{file,forms,interactive,series}` and returns `series` last, because a series is rejected unless the
+instruments it references are already stored — so a series whose items live in a _different_ repository still
+fails to import (`.agents/docs/architecture/instrument-pipeline.md`).
 
 **`/runtime/v1/zod@3.x` is the Zod v3 API.** `vendor/zod@3.x`'s `.` export re-exports `zod/v3`; the v4 API is a
 separate subpath, `/runtime/v1/zod@3.x/v4`. The repo-wide `no-restricted-imports` ban on bare `zod`

apps/api/src/instrument-repos/__tests__/instrument-repos.service.spec.ts

@@ -1,10 +1,14 @@
+import * as fs from 'node:fs';
+import * as os from 'node:os';
+import * as path from 'node:path';
+
 import { ConfigService, getModelToken, LoggingService } from '@douglasneuroinformatics/libnest';
 import type { Model } from '@douglasneuroinformatics/libnest';
 import { MockFactory } from '@douglasneuroinformatics/libnest/testing';
 import type { MockedInstance } from '@douglasneuroinformatics/libnest/testing';
 import { BadRequestException, ConflictException, NotFoundException } from '@nestjs/common';
 import { Test } from '@nestjs/testing';
-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
 import { InstrumentsService } from '../../instruments/instruments.service';
 import { InstrumentReposService } from '../instrument-repos.service';
@@ -14,6 +18,7 @@ import { InstrumentReposService } from '../instrument-repos.service';
 // intersection with the class, whose private members would collapse the type to `never`.
 type InternalService = {
   decrypt(value: string): string | undefined;
+  discoverInstrumentDirs(repoDir: string): string[];
   encrypt(plaintext: string): string;
   importInstruments(
     owner: string,
@@ -313,4 +318,52 @@ describe('InstrumentReposService', () => {
       expect(instrumentModel.update).not.toHaveBeenCalled();
     });
   });
+
+  describe('discoverInstrumentDirs', () => {
+    let repoDir: string;
+
+    const writeInstrument = (category: string, name: string) => {
+      const dir = path.join(repoDir, 'lib', category, name);
+      fs.mkdirSync(dir, { recursive: true });
+      fs.writeFileSync(path.join(dir, 'index.ts'), 'export default {};');
+    };
+
+    beforeEach(() => {
+      repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'odc-repo-'));
+    });
+
+    afterEach(() => {
+      fs.rmSync(repoDir, { force: true, recursive: true });
+    });
+
+    it('should discover every instrument category, so a repo does not silently lose its series', () => {
+      writeInstrument('file', 'SCAN');
+      writeInstrument('forms', 'HAPPINESS');
+      writeInstrument('interactive', 'CLICK');
+      writeInstrument('series', 'HAPPINESS_WITH_CONSENT');
+
+      const found = internal.discoverInstrumentDirs(repoDir).map((dir) => path.basename(dir));
+
+      expect(found.sort()).toStrictEqual(['CLICK', 'HAPPINESS', 'HAPPINESS_WITH_CONSENT', 'SCAN']);
+    });
+
+    it('should return series after every scalar, because a series cannot be stored before its items', () => {
+      writeInstrument('series', 'HAPPINESS_WITH_CONSENT');
+      writeInstrument('forms', 'HAPPINESS');
+      writeInstrument('interactive', 'CLICK');
+
+      const found = internal.discoverInstrumentDirs(repoDir).map((dir) => path.basename(dir));
+
+      expect(found.at(-1)).toBe('HAPPINESS_WITH_CONSENT');
+    });
+
+    it('should ignore a directory without an index file', () => {
+      writeInstrument('forms', 'HAPPINESS');
+      fs.mkdirSync(path.join(repoDir, 'lib', 'forms', 'README_ONLY'), { recursive: true });
+
+      const found = internal.discoverInstrumentDirs(repoDir).map((dir) => path.basename(dir));
+
+      expect(found).toStrictEqual(['HAPPINESS']);
+    });
+  });
 });

apps/api/src/instrument-repos/instrument-repos.service.ts

@@ -167,7 +167,11 @@ export class InstrumentReposService implements OnModuleInit {
 
   private discoverInstrumentDirs(repoDir: string): string[] {
     const dirs: string[] = [];
-    for (const category of ['forms', 'interactive']) {
+    // `series` is scanned last, and callers must import in the returned order: a series is rejected
+    // unless every instrument it references is already stored, so the scalars a repo provides have to
+    // be created first. A series referencing a *different* repo still fails; see
+    // `.agents/docs/architecture/instrument-pipeline.md`.
+    for (const category of ['file', 'forms', 'interactive', 'series']) {
       const libDir = path.join(repoDir, 'lib', category);
       if (!fs.existsSync(libDir)) {
         continue;
@@ -294,8 +298,18 @@ export class InstrumentReposService implements OnModuleInit {
       // from the conflict message so we still associate the existing instrument with this repo.
       if (err instanceof ConflictException) {
         const idMatch = /ID '([^']+)'/.exec(err.message);
+        if (!idMatch?.[1]) {
+          // The message is the only channel carrying the existing id. Losing it drops the instrument
+          // from this repo's list, so fail loudly rather than returning an empty result.
+          this.loggingService.error({
+            conflictMessage: err.message,
+            error: 'Cannot recover instrument id from conflict',
+            instrumentDir: dirName
+          });
+          return null;
+        }
         this.loggingService.debug(`Instrument from ${dirName} already exists, skipping creation`);
-        return idMatch?.[1] ? { created: false, id: idMatch[1] } : null;
+        return { created: false, id: idMatch[1] };
       }
       throw err;
     }

apps/api/src/instruments/__tests__/instruments.service.spec.ts

@@ -439,6 +439,39 @@ describe('InstrumentsService', () => {
     });
   });
 
+  describe('create', () => {
+    // The id `create` derives for `existingSeries` when no owning group is supplied, as a repo import
+    // does. `seriesGroupId` is undefined, so `JSON.stringify` drops the key entirely.
+    const seriesId = `__V2__hash:${JSON.stringify({
+      content: existingSeries.content,
+      title: existingSeries.details.title
+    })}`;
+
+    beforeEach(() => {
+      vi.spyOn(cryptoService, 'hash').mockImplementation((value) => `hash:${value}`);
+      virtualizationService.eval.mockResolvedValue({ isErr: () => false, value: existingSeries } as any);
+      instrumentModel.findMany.mockResolvedValue([{ id: 'hash:FORM_A-1' }, { id: 'hash:FORM_B-1' }] as any);
+    });
+
+    it('should report a lost insert race as a conflict, so a concurrent import can still recover the id', async () => {
+      // The guard passes, then a competing import wins the insert: the driver rejects the duplicate id
+      // and the row is present on re-check.
+      instrumentModel.exists.mockResolvedValueOnce(false).mockResolvedValueOnce(true);
+      instrumentModel.create.mockRejectedValueOnce(new Error('Unique constraint failed on the constraint: `_id_`'));
+
+      await expect(instrumentsService.create({ bundle: '__BUNDLE__' })).rejects.toThrowError(
+        new ConflictException(`Instrument with ID '${seriesId}' already exists!`)
+      );
+    });
+
+    it('should rethrow an insert failure that is not a duplicate, so real errors are not masked', async () => {
+      instrumentModel.exists.mockResolvedValue(false);
+      instrumentModel.create.mockRejectedValueOnce(new Error('connection reset'));
+
+      await expect(instrumentsService.create({ bundle: '__BUNDLE__' })).rejects.toThrowError('connection reset');
+    });
+  });
+
   describe('generateSeriesInstrumentId', () => {
     it('uses a versioned prefix and includes the title so confirmed duplicate form sets can be distinct', () => {
       vi.spyOn(cryptoService, 'hash').mockImplementation((value) => value);

apps/api/src/instruments/instruments.service.ts

@@ -109,7 +109,7 @@ export class InstrumentsService {
 
     const id = this.generateInstrumentId(instance, seriesGroupId);
     if (await this.instrumentModel.exists({ id })) {
-      throw new ConflictException(`Instrument with ID '${id}' already exists!`);
+      throw this.instrumentExistsConflict(id);
     }
 
     if (instance.kind === 'SERIES') {
@@ -134,14 +134,24 @@ export class InstrumentsService {
       });
     }
 
-    await this.instrumentModel.create({
-      data: {
-        bundle,
-        groups: seriesGroupId ? { connect: { id: seriesGroupId } } : undefined,
-        id,
-        seriesGroup: seriesGroupId ? { connect: { id: seriesGroupId } } : undefined
+    try {
+      await this.instrumentModel.create({
+        data: {
+          bundle,
+          groups: seriesGroupId ? { connect: { id: seriesGroupId } } : undefined,
+          id,
+          seriesGroup: seriesGroupId ? { connect: { id: seriesGroupId } } : undefined
+        }
+      });
+    } catch (err) {
+      // Two concurrent imports both clear the check above and then one loses the insert on the unique
+      // id. Surface that as the same conflict as losing the check, so a caller has one case to handle
+      // rather than a driver-specific error that reads as an unrelated failure.
+      if (await this.instrumentModel.exists({ id })) {
+        throw this.instrumentExistsConflict(id);
       }
-    });
+      throw err;
+    }
     return { ...instance, id };
   }
 
@@ -616,6 +626,15 @@ export class InstrumentsService {
     );
   }
 
+  /**
+   * The single source of this message. `InstrumentReposService.importInstrumentFromDir` reads the id
+   * back out of it to associate an already-stored instrument with the repository that provides it, so
+   * the id must stay quoted and the two throw sites must stay identical.
+   */
+  private instrumentExistsConflict(id: string): ConflictException {
+    return new ConflictException(`Instrument with ID '${id}' already exists!`);
+  }
+
   /**
    * Whether an instrument visible to the target group already uses the given title (case-insensitive,
    * across every language it is defined in). Used to enforce that newly-created series have a unique,

apps/web/src/routes/_app/admin/instrument-repos/index.tsx

@@ -111,7 +111,9 @@ const RouteComponent = () => {
   const createRepoMutation = useCreateInstrumentRepoMutation();
   const deleteRepoMutation = useDeleteInstrumentRepoMutation();
   const syncRepoMutation = useSyncInstrumentRepoMutation();
-  const instrumentInfoQuery = useInstrumentInfoQuery();
+  // Every edition, because this page audits what a repository contributed. The default collapses an
+  // instrument's editions to the latest one, which would disagree with the count in the table.
+  const instrumentInfoQuery = useInstrumentInfoQuery({ params: { allEditions: true } });
   const [isDialogOpen, setIsDialogOpen] = useState(false);
   const [isConfirmDeleteOpen, setIsConfirmDeleteOpen] = useState(false);
   const [isViewOpen, setIsViewOpen] = useState(false);