diff --git a/.agents/docs/architecture/instrument-pipeline.md b/.agents/docs/architecture/instrument-pipeline.md index 3e6d9b929..bbcce77ad 100644 --- a/.agents/docs/architecture/instrument-pipeline.md +++ b/.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}//` | `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. diff --git a/.agents/skills/odc-instruments/SKILL.md b/.agents/skills/odc-instruments/SKILL.md index 4ae0ee39f..3251632a6 100644 --- a/.agents/skills/odc-instruments/SKILL.md +++ b/.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` diff --git a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.spec.ts b/apps/api/src/instrument-repos/__tests__/instrument-repos.service.spec.ts index e17136892..b440b9fd1 100644 --- a/apps/api/src/instrument-repos/__tests__/instrument-repos.service.spec.ts +++ b/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']); + }); + }); }); diff --git a/apps/api/src/instrument-repos/instrument-repos.service.ts b/apps/api/src/instrument-repos/instrument-repos.service.ts index e49e51fe6..b41df9f8a 100644 --- a/apps/api/src/instrument-repos/instrument-repos.service.ts +++ b/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; } diff --git a/apps/api/src/instruments/__tests__/instruments.service.spec.ts b/apps/api/src/instruments/__tests__/instruments.service.spec.ts index eb6d878c5..223e38d3b 100644 --- a/apps/api/src/instruments/__tests__/instruments.service.spec.ts +++ b/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); diff --git a/apps/api/src/instruments/instruments.service.ts b/apps/api/src/instruments/instruments.service.ts index 6be89fdcc..8cce8adf5 100644 --- a/apps/api/src/instruments/instruments.service.ts +++ b/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, diff --git a/apps/web/src/routes/_app/admin/instrument-repos/index.tsx b/apps/web/src/routes/_app/admin/instrument-repos/index.tsx index 6303d6376..c3b1fcb80 100644 --- a/apps/web/src/routes/_app/admin/instrument-repos/index.tsx +++ b/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);