Skip to content

FILE instrument schema accepts file groups that can never be completed: min above max, or two groups sharing a basename #1706

Open

Description

@joshunrau

The FILE instrument schema validates each file group in isolation, so the API stores instruments that no one can complete. count.min and count.max are checked as two independent positive integers, and nothing requires min <= max. A group declared { min: 3, max: 1 } passes $AnyInstrument. The Dropzone then caps selection at max (1 file) while submit requires at least min (3), so every attempt fails with "You uploaded 1 file, but between 3 and 1 are required". Basenames are not checked for uniqueness across groups either. Two groups with the same basename share one uploadMap[basename] entry in the renderer, so choosing files in one dropzone replaces the other's. The API presigns both groups onto the same storage keys, and validateFiles pushes the same uploads once per group, so the record gets duplicate file rows. Both mistakes are accepted at upload, with no error for the author. They only surface when a subject tries to complete the instrument.

Where

packages/schemas/src/instrument/instrument.file.ts:12-22:

const $$FileGroup = <TLanguage extends InstrumentLanguage>(language?: TLanguage) => {
  return z.object({
    basename: z.string(),
    count: z.object({
      max: z.int().positive(),
      min: z.int().positive()
    }),
    label: $$InstrumentUIOption(z.string(), language),
    type: $FileType.nullable()
  }) satisfies z.ZodType<FileInstrument.FileGroup>;
};

content: z.object({ fileGroups: z.array($$FileGroup(language)) }) at :26-28 has no cross-group check. The consumers that assume both rules hold:

  • packages/react-core/src/components/FileInstrumentContent/Dropzone.tsx:51 and :61 key state by basename.
  • apps/api/src/instrument-records/files/files.service.ts:87 keys presigned URLs by basename, and :174-185 validates and collects files per basename.

Reproduce

  1. Upload a FILE instrument whose fileGroups is [{ basename: 'scan', count: { min: 3, max: 1 }, label: 'Scan', type: null }]. Upload a second one whose two groups are both basename: 'scan', each with { min: 1, max: 1 }.
  2. Start a session and open each instrument.

Actual: both instruments are accepted. The first can never be submitted. In the second, selecting a file in one dropzone shows it in both, and a completed upload stores the same files twice.
Expected: POST /v1/instruments rejects both bundles with a validation issue that points at the offending group.

Tests

packages/schemas/src/instrument/__tests__/instrument.file.test.ts (new, beside the other instrument.*.test.ts files):

  • it('should reject a file group whose minimum count exceeds its maximum, since no upload could satisfy it')
  • it('should reject two file groups with the same basename, since their uploads would overwrite each other')

E2E in testing/: with the admin token, POST /v1/instruments a bundle whose file groups share a basename, and assert a 422 (the path the playground's Upload Bundle dialog uses).

Suggested fix

Add a .refine to count for min <= max, and a .check on fileGroups that reports duplicate basenames by index. Both survive z.toJSONSchema, which apps/outreach requires. While here, decide whether min: 0 (an optional group) is legal. The schema's positive() rejects it, but FilesService.validateFiles, the Dropzone's "Between 0 and N" label and #1648 all treat it as valid.

Activity

  1. added
    BugType: existing behavior is wrong
    Difficulty: LowIsolated change, about 2 hours or less
    Area: InstrumentsInstrument authoring API, bundling, runtime, rendering, measures and the built-in library
    Area: SharedCross-tier packages (schemas, react-core, etc.) not covered by another area
    on Oct 7, 2026
  2. joshunrau commented on Oct 26, 2026

    @joshunrau
    CollaboratorAuthor

    Still reproduces on main (d86740e). No commit since this was filed touches packages/schemas/src/instrument/, FileInstrumentContent/, instrument-records/files/ or storage/, and no open PR changes the schema.

    min: 0

    I'd make it legal. Both bounds have been positive() since the FILE kind was added (21695b3), and every path that stores an instrument (the controller, repo sync, demo seeding) goes through InstrumentsService.create. So no stored group has min: 0, and relaxing it needs no migration. The zero-file case already works in the count checks at FileInstrumentContent/store.ts:26 and files.service.ts:177, and in the "No files" state at apps/web/src/routes/_app/datahub/subjects/$subjectId/table/$recordId.tsx:84-87. The one exception is the crash in #1648.

    Use min: z.int().nonnegative() and keep max positive(). react-dropzone 20.1.2 treats maxFiles: 0 as no limit, so max: 0 would remove the Dropzone's cap. Fix #1648 in the same PR. If groups should stay mandatory instead, close #1648 as unreachable and drop the min: 0 fixtures in files.service.test.ts (:146, :210, :310, :325).

    Notes on the fix

    • Put the refine on count with path: ['min'], and have the fileGroups check report each duplicate at [index, 'basename']. Through the discriminated union, these come back as content.fileGroups.<i>.count.min and content.fileGroups.<i>.basename (checked against the installed zod 3.25.76).
    • On the suggested fix: z.toJSONSchema does not throw on either rule, but it leaves both out of its output. The JSON Schema published by apps/outreach/src/pages/resources/schemas/[kind]-instrument-[lang].json.ts:25 will keep accepting these groups. JSON Schema cannot express either rule.
    • Stored instruments are never re-validated. The web render route passes no options to InstrumentRenderer (apps/web/src/routes/_app/instruments/render/$id.tsx:115-121), so a broken instrument already on an instance keeps rendering after the fix. Re-uploading the same edition returns 409, so the author has to bump internal.edition. The playground preview does validate (apps/playground/src/preview/PreviewApp.tsx:67).
    • Duplicate basenames also show up on the datahub record page. FilesService.find (files.service.ts:55-59) gives each group every row with that basename, so the page shows two sections and lists each file twice in each one.
    • Optional, same object: fileGroups accepts [], though the guidelines say "one or more" (packages/instrument-guidelines/AGENTS.md:575). max has no upper bound, and getPresignedUploadUrls signs count.max URLs on every request (files.service.ts:88). Whatever rules land, write them into the count comment at :584. That package is published, so the change ships with a release.

    Tests

    • Unit: build instrument.file.test.ts on bilingualFileInstrument from @opendatacapture/instrument-stubs/file, as instrument.core.test.ts does. Add a reject case for max: 0, and an accept case for min: 0 if it becomes legal.
    • E2E in testing/src/specs/admin-instruments.spec.ts, with adminToken and apiRequestContext as the archive test uses them (:63-67), so testing/src/support/api-client.ts (changed by feat: add the instrument hub and record provenance #1807) stays untouched. Fetch the seeded ARBITRARY_SINGLE_FILE bundle and post it back twice: once with count:{max:1,min:1} rewritten to min:3, and once with its group duplicated. On main both return 409, because validation passes and create reaches the duplicate-id check (instruments.service.ts:111-113). authorization.spec.ts:399-407 already uses this re-post pattern. After the fix both return 422 and nothing is stored, so the test needs no unique name. Assert the issue paths as well as the status.

    #1712 changes how $AnyInstrument is built. If it lands as the union alternative, zod reports these issues inside a single invalid_union at path [], so whichever PR lands second has to update the path assertions.

    Triage note written with Claude Code. Code references point at main @ d86740e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Area: InstrumentsInstrument authoring API, bundling, runtime, rendering, measures and the built-in libraryArea: SharedCross-tier packages (schemas, react-core, etc.) not covered by another areaBugType: existing behavior is wrongDifficulty: LowIsolated change, about 2 hours or lessPriority: MediumReal value; schedule normally

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions