From 876238473d91d518f98897c4eceebf8f383321b7 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Mon, 3 Aug 2026 15:05:59 -0400 Subject: [PATCH 1/2] fix(instrument-repos): import every instrument a repository provides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects made repo import report an incomplete and internally inconsistent set of instruments. For one repo: 9 directories on GitHub, 8 in the repo list, 4 in the "View instruments" dialog. **Series and file instruments were never imported.** `discoverInstrumentDirs` scanned a hardcoded `['forms', 'interactive']`, so `lib/series` and `lib/file` were skipped before discovery. Nothing was logged because nothing was attempted, even though both are real kinds in `$InstrumentKind` with a directory in the built-in library. Series carry an ordering constraint scalars do not: `validateSeriesInstrument` rejects a series unless every instrument it references is already stored. The category list is now ordered so `series` is scanned last and the scalars a repo provides are created first. A series referencing a *different* repository still fails — that needs a second resolution pass after all repos are imported, and is documented as a known limitation rather than half-built here. **A lost insert race silently dropped instruments.** `create` checks `instrumentModel.exists({ id })` and then inserts, which is not atomic. Two concurrent imports of an instrument provided by more than one repo both cleared the check; the loser got a driver-level unique-constraint error instead of the `ConflictException` that the caller knows how to recover an id from, so the instrument was dropped from that repo's `instrumentIds` entirely. The insert now re-checks existence on failure and reports the same conflict, which is exact (the insert failed and the row is present) and avoids coupling to Prisma error codes. Anything else still propagates. The conflict message is now produced in one place, since `InstrumentReposService` parses the id back out of it. **The dialog hid all but the latest edition.** The repo table counts `instrumentIds.length` while the dialog used `useInstrumentInfoQuery()` with no arguments, where `allEditions` defaults to false and results are keyed by instrument name, keeping only the highest edition. A page whose purpose is auditing what a repository contributed now asks for every edition. Tests: `discoverInstrumentDirs` gains coverage for all four categories, for the series-last ordering, and for ignoring an index-less directory; `create` gains coverage for the race and for not masking unrelated insert failures. The two category tests and the race test were each confirmed to fail before the fix. No e2e test accompanies this. `SetupService.seedDefaultInstrumentRepo` skips seeding when `NODE_ENV=test` because the suite must not reach GitHub, so the e2e environment contains no repo-sourced instruments and neither code path is reachable. Covering it needs a mockable GitHub layer. Co-Authored-By: Claude Opus 5 --- .../docs/architecture/instrument-pipeline.md | 18 +++--- .../instrument-repos.service.spec.ts | 55 ++++++++++++++++++- .../instrument-repos.service.ts | 18 +++++- .../__tests__/instruments.service.spec.ts | 33 +++++++++++ .../src/instruments/instruments.service.ts | 35 +++++++++--- .../_app/admin/instrument-repos/index.tsx | 4 +- 6 files changed, 144 insertions(+), 19 deletions(-) diff --git a/.agents/docs/architecture/instrument-pipeline.md b/.agents/docs/architecture/instrument-pipeline.md index 33cfe41df..05db4b730 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 @@ -215,6 +220,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/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 3055e038c..c73ee0490 100644 --- a/apps/api/src/instrument-repos/instrument-repos.service.ts +++ b/apps/api/src/instrument-repos/instrument-repos.service.ts @@ -171,7 +171,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; @@ -309,8 +313,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); From 315ddb03969f5e1a085fd890f81889176747c081 Mon Sep 17 00:00:00 2001 From: thomasbeaudry Date: Tue, 4 Aug 2026 23:36:53 -0400 Subject: [PATCH 2/2] docs(skills): correct the repo-discovery claim in odc-instruments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `odc-instruments/SKILL.md` still told agents that discovery scans only `lib/forms` and `lib/interactive` and silently skips `lib/file` and `lib/series` — the exact behaviour this branch changes. Left as-is it would actively mislead: an agent would conclude a missing series was expected rather than a bug. Replaces it with what is now true, including the ordering constraint and the cross-repository limitation, so the caveat travels with the capability rather than living only in the architecture doc. Found by applying the review note on #1482 (a fixed defect still listed as known in `odc-debugging/SKILL.md`) to this branch's own doc surface. Co-Authored-By: Claude Opus 5 --- .agents/skills/odc-instruments/SKILL.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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`