Skip to content

Importing instruments from a GitHub repository reports an incomplete and internally inconsistent set of instruments. #1483

Description

@thomasbeaudry

What Happened?

Summary

These are three independent defects that happen to compound. Observed on a production
Docker deployment; the numbers are reproducible.


1. Series and file instruments are never imported

Observed

A repository containing lib/series/* syncs successfully and reports no error, but no
series instrument ever appears. Nothing is logged, because nothing is attempted.

Root cause

discoverInstrumentDirs (apps/api/src/instrument-repos/instrument-repos.service.ts:174)
iterates a hardcoded category list:

for (const category of ['forms', 'interactive']) {
  const libDir = path.join(repoDir, 'lib', category);

lib/series/ and lib/file/ are skipped before discovery, even though both are real kinds —
$InstrumentKind is ['FILE', 'FORM', 'INTERACTIVE', 'SERIES']
(packages/schemas/src/instrument/instrument.base.ts:31) and the built-in library has a
directory for each (packages/instrument-library/src/{file,forms,interactive,series}/).

This is currently documented as intended behaviour in
.agents/docs/architecture/instrument-pipeline.md:49-51:

Repo discovery only scans lib/forms and lib/interactive. A lib/file or
lib/series directory in an external repo is silently ignored.

Why this is more than a one-line change

validateSeriesInstrument (apps/api/src/instruments/instruments.service.ts:691) resolves
every series item to an existing instrument id and rejects the entire series if any is
missing (Cannot find instrument '<name>' with edition '<edition>'). Consequences:

  • Ordering becomes load-bearing. Series must be created after the scalars they
    reference. Appending 'series' to the category array happens to get this right, since
    forms and interactive are pushed first — but nothing enforces or documents that.
  • Cross-repo series cannot work today. A series in repo A referencing a form in repo B
    only imports if B was synced first. There is no dependency resolution and no retry, so
    the result depends on the order repos were added. Making this reliable needs a second
    resolution pass after all repos are imported.
  • Group ownership is undecided. Repo imports pass no seriesGroupId, so a series would
    be created with seriesGroup: null and access would flow through repo assignment like
    other repo-sourced instruments. Consistent, but should be a deliberate choice.

Suggested fix

Add 'series' (and 'file') to the scanned categories, make the ordering explicit rather
than incidental, log a clear reason when a series is skipped for a missing item, and update
the two doc locations pinning this behaviour (instrument-pipeline.md:47-51 and :219).
Decide explicitly whether cross-repo series references must work.


2. Import silently under-counts instruments

Observed

Found 9 instrument directories in DouglasNeuroInformatics/ODC_Restricted_Instruments
Imported 8 instruments from DouglasNeuroInformatics/ODC_Restricted_Instruments

Two distinct causes were found in the logs.

Cause A — an instrument that fails to evaluate is reported only as a generic message

Failed to import instrument from SHEEHAN_DISABILITY_SCALE:
UnprocessableEntityException: Failed to interpret instrument bundle

The instrument bundles, then fails when the API evaluates it
(instruments.service.ts:93-99). The underlying reason is logged separately via
this.loggingService.error(result.error) and is not correlated with the instrument, so an
operator sees only "Failed to interpret" with no indication of which file or why. If the
cause is module-scope DOM access, the bundler's GLOBALS proxies already produce a message
naming the offending file — that detail should reach the import log.

Cause B — check-then-act race drops instruments already present

Failed to import instrument from GAD_7: PrismaClientKnownRequestError:
Invalid `prisma.instrument.create()` invocation:
Unique constraint failed on the constraint: `_id_`

Also seen for OLDER_AMERICANS_RESOURCES_AND_SERVICES, TEN_ITEM_PERSONALITY_INVENTORY,
TMB_DIGIT_SYMBOL_MATCHING, TMB_FLANKER_ATTENTION, TMB_VERBAL_PA_STUDY — all names
provided by more than one repository.

create() guards with instrumentModel.exists({ id }) at instruments.service.ts:111,
then inserts at :137. Between those two points another import can insert the same id, so
the insert collides. The ConflictException handler in importInstrumentFromDir
(instrument-repos.service.ts:310-313) recovers the existing id from the message and still
associates the instrument with the repo — but a PrismaClientKnownRequestError is not a
ConflictException, so it escapes to the outer catch and the instrument is dropped from
that repo's instrumentIds entirely.

Note: the concurrency trigger is inferred from the log pattern (duplicate names across
repos) and has not been confirmed by reproduction.

There is also a genuinely silent path

importInstrumentFromDir returns null and importInstruments does
if (!result) continue; (:338) with no log above debug. One of those branches —
ConflictException where the id regex fails to match (:313) — would drop an instrument
with no trace at default log level.

Suggested fix

Treat a Prisma unique-constraint violation the same as ConflictException (recover the id,
count the instrument). Attach the underlying cause to the per-instrument failure log.
Replace the silent null/continue path with an explicit warning, or make it unreachable.


3. "View instruments" dialog hides all but the latest edition

Observed

A repo whose column reads 8 instruments opens a dialog listing 4.

Root cause

The column reads the stored count — row.instrumentIds.length
(apps/web/src/routes/_app/admin/instrument-repos/index.tsx:184) — while the dialog is
populated from useInstrumentInfoQuery() called with no arguments (:114) and filtered by
sourceRepo.id (:276).

With no arguments, allEditions defaults to false
(apps/api/src/instruments/instruments.service.ts:362), and that path keys results by
instrument name, keeping only the highest edition
(instruments.service.ts:427-432):

2. Import silently under-counts instruments

Observed

Found 9 instrument directories in DouglasNeuroInformatics/ODC_Restricted_Instruments
Imported 8 instruments from DouglasNeuroInformatics/ODC_Restricted_Instruments

Two distinct causes were found in the logs.

Cause A — an instrument that fails to evaluate is reported only as a generic message

Failed to import instrument from SHEEHAN_DISABILITY_SCALE:
UnprocessableEntityException: Failed to interpret instrument bundle

The instrument bundles, then fails when the API evaluates it
(instruments.service.ts:93-99). The underlying reason is logged separately via
this.loggingService.error(result.error) and is not correlated with the instrument, so an
operator sees only "Failed to interpret" with no indication of which file or why. If the
cause is module-scope DOM access, the bundler's GLOBALS proxies already produce a message
naming the offending file — that detail should reach the import log.

Cause B — check-then-act race drops instruments already present

Failed to import instrument from GAD_7: PrismaClientKnownRequestError:
Invalid `prisma.instrument.create()` invocation:
Unique constraint failed on the constraint: `_id_`

Also seen for OLDER_AMERICANS_RESOURCES_AND_SERVICES, TEN_ITEM_PERSONALITY_INVENTORY,
TMB_DIGIT_SYMBOL_MATCHING, TMB_FLANKER_ATTENTION, TMB_VERBAL_PA_STUDY — all names
provided by more than one repository.

create() guards with instrumentModel.exists({ id }) at instruments.service.ts:111,
then inserts at :137. Between those two points another import can insert the same id, so
the insert collides. The ConflictException handler in importInstrumentFromDir
(instrument-repos.service.ts:310-313) recovers the existing id from the message and still
associates the instrument with the repo — but a PrismaClientKnownRequestError is not a
ConflictException, so it escapes to the outer catch and the instrument is dropped from
that repo's instrumentIds entirely.

Note: the concurrency trigger is inferred from the log pattern (duplicate names across
repos) and has not been confirmed by reproduction.

There is also a genuinely silent path

importInstrumentFromDir returns null and importInstruments does
if (!result) continue; (:338) with no log above debug. One of those branches —
ConflictException where the id regex fails to match (:313) — would drop an instrument
with no trace at default log level.

Suggested fix

Treat a Prisma unique-constraint violation the same as ConflictException (recover the id,
count the instrument). Attach the underlying cause to the per-instrument failure log.
Replace the silent null/continue path with an explicit warning, or make it unreachable.


3. "View instruments" dialog hides all but the latest edition

Observed

A repo whose column reads 8 instruments opens a dialog listing 4.

Root cause

The column reads the stored count — row.instrumentIds.length
(apps/web/src/routes/_app/admin/instrument-repos/index.tsx:184) — while the dialog is
populated from useInstrumentInfoQuery() called with no arguments (:114) and filtered by
sourceRepo.id (:276).

With no arguments, allEditions defaults to false
(apps/api/src/instruments/instruments.service.ts:362), and that path keys results by
instrument name, keeping only the highest edition
(instruments.service.ts:427-432):

const currentEntry = results.get(info.internal.name);
if (!currentEntry || !('internal' in currentEntry) || info.internal.edition > currentEntry.internal.edition) {
  results.set(info.internal.name, info);
}

So 8 instruments at two editions each collapse to 4 rows. Nothing is missing from the
database — the dialog simply isn't asking for it. This matches the observed data
(WHOQOL-BREF listed at edition 2, with its edition 1 absorbed).

Why it's wrong here

The dialog is titled "Instruments in <repo>" and exists to audit what a repository
contributed. Silently hiding earlier editions makes it disagree with the count shown one
click earlier, with no indication that filtering occurred.

Suggested fix

Pass { allEditions: true } at index.tsx:114. This affects only the dialog; the column
count comes from instrumentIds and is unchanged.


Related

PR #1482 fixes a separate defect that made all of the above harder to diagnose: repo import
failed entirely in bundled/production builds, and two layers of error handling discarded the
underlying cause. The improved error logging in that PR is a prerequisite for diagnosing
items 1 and 2 in the field.

Two notes on using this: the concurrency explanation in 2B is inferred from the log pattern, not reproduced, so I flagged it as such in the text rather than
asserting it. And item 1 contains a genuine design question (cross-repo series references) that probably needs a decision before anyone starts implementing
— it may deserve to be its own issue so that discussion doesn't get buried under the two smaller bugs.

What Did You Expect?

For the instrument repo sync to work properly

Operating System

ubuntu 24.04

Browser (if applicable)

No response

Steps to Reproduce

No response

Anything Else?

No response

Metadata

Metadata

Assignees

Labels

BugSomething isn't working

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions