fix(api): keep an uploaded file instrument record pending

[gdevenyi]

This is the test failure I've been flagging as pre-existing in #1418, #1419, #1420 and #1421. It turns out to be a real defect, not a stale test.

What happened

InstrumentRecordsService.upload builds its records without a pending field at all, so every bulk-uploaded record is persisted with the field absent rather than false.

PR #1404 ("fix pending tag issue which cause old data not to be visible") fixed the resulting invisibility by teaching find to match records where the field is missing:

// records created before the file instrument feature do not have the pending field at all,
// and prisma NOT filters on mongodb exclude documents where the field is missing entirely
{ OR: [{ pending: { isSet: false } }, { pending: null }, { pending: false }] }

and in the same commit added a test asserting that upload sets pending — but never made that change to upload. The test has been red on main since that PR merged (8998d811d).

So the test was correct and expressed the intent; the production half of the fix was simply missing.

Why it still matters

The comment on the compatibility clause says it exists for "records created before the file instrument feature". But because upload still omits the field, that clause is not a legacy shim — it is load-bearing for newly created data, and any query that filters on pending the natural way silently misses every uploaded record.

Confirmed against a live instance, before the fix:

record exists:                            true
has pending field:                        false
matched by a plain {pending:false} query: 0
matched by the compat OR clause:          1

Why this mirrors create instead of hardcoding false

My first cut was pending: false, which is literally what the test asserts. Then I checked whether upload can receive a file instrument — it does not reject them the way it rejects series instruments — and it can:

upload of a FILE instrument: HTTP 201
persisted: 1

Hardcoding false would therefore mark a record with no attached files as complete, and FilesService.getAssociations would then reject any attempt to attach them with Upload already completed. So this uses the same expression create already uses one function above:

pending: instrument.kind === 'FILE',

which satisfies the test (its fixture is a FORM) and keeps the two creation paths consistent.

Verified live, after the fix

form upload      pending = false     visible in the datahub
file upload      pending = true      correctly excluded until its files arrive
main behaviour   pending = ABSENT    still visible via the compatibility clause

No regression for existing data: records created before this change are still matched by the isSet: false clause, which stays exactly as it is.

Tests

The existing test now passes. Added one covering the file-instrument branch, so the create/upload consistency is pinned rather than incidental.

vitest run across the whole workspace is now fully green — 37/37 files, 254 passed, 1 skipped, 0 failed. It has had this one failure throughout.

• tsc --noEmit on apps/api — clean
• eslint src — clean
• prettier --check — clean

Note

While verifying this I re-confirmed the separate issue filed as #1414: upload's return value is findMany({ groupId, instrumentId }), so uploading one record returned all 8 records for that instrument. Not touched here.

---

Rebased onto main — the scope is now much narrower than this title suggests

Rebased onto 26ec89168; conflict resolved, CI green.

You were right that this needed deciding rather than replaying, but the history is not quite
"main took the opposite approach". Reconstructing it:

• When 722f0a0b0 reframed the create-side test, upload genuinely did not write pending — I
  confirmed at 722f0a0b0^ that only create did. So that reasoning was correct at the time.
• Separately and on a different branch, 573f307c9 ("resolve series regressions after main merge")
  added pending: false to upload. It reached main after the test was reframed, so nothing
  re-asserted it.

Net effect on today's main: upload writes pending: false unconditionally, and no test covers it.
So the "write the field at all" half of this PR has already arrived by another route. What remains
is only the FILE case, which is still a real defect:

	create	upload on main	this branch
FORM	false	false	false
FILE	true	false ❌	true

FilesService clears pending once the upload lands, and refuses to attach a file to a record that
is not pending — so a bulk-uploaded file record is born settled and its files can never be attached.
upload does not reject file instruments the way it rejects series instruments; verified live, a
file instrument upload returns 201.

The diff is now one expression plus its tests, and the title and commit message were rewritten to say
so. The comment 722f0a0b0 left on the create-side test is corrected, since on this branch it
asserts the opposite of what the code does, and the FORM case gets its assertion back.

🤖 Generated with Claude Code

https://claude.ai/code/session_014ZEJLwa7jwD8sKzcE4PfSc

[Copilot]

Pull request overview

This PR fixes a production defect in the API bulk-upload path (InstrumentRecordsService.upload) where newly created instrument records were persisted without a pending field, causing them to be missed by any query filtering on pending (unless it used the legacy compatibility OR clause). The change aligns upload with the existing create behavior so newly uploaded records always have an explicit pending value.

Changes:

• Set pending explicitly during bulk upload, matching the create logic (pending: instrument.kind === 'FILE').
• Add a unit test to pin the FILE-instrument branch (FILE uploads remain pending: true so files can be attached later).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
apps/api/src/instrument-records/instrument-records.service.ts	Ensures bulk-uploaded records always persist an explicit pending value, consistent with the single-record create path.
apps/api/src/instrument-records/tests/instrument-records.service.spec.ts	Adds coverage to ensure FILE instrument uploads produce pending: true (and existing pending assertion for FORM remains satisfied).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[joshunrau]

This branch conflicts with main, so I have not reviewed it yet.

CI is green, but GitHub reports mergeable: CONFLICTING.

For context on the conflict: apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts and the surrounding upload/pending code were changed on main after this branch was cut — see 7e0ef1a0f ("fix: fix pending tag issue which cause old data not to be visible") and 722f0a0b0 ("test(api): update stale pending-on-create assertion for instrument records"). Worth reading both before resolving, since they touch the same behaviour this PR changes.

Please rebase or merge main into fix/upload-sets-pending and push. I'll pick the review back up once it merges cleanly.

[joshunrau]

Suggested model: Opus 5

Sizing the remaining work on this PR for whoever picks it up, human or agent. The conflict resolution here isn't mechanical: main took the opposite approach in 7e0ef1a0f and 722f0a0b0, so resolving it means deciding whether this PR's premise still holds rather than just replaying the diff.

[joshunrau]

Requesting changes so this tracks as needing author action.

Blocking: conflicts with main. CI is green. Details in my earlier comment.

Worth reading 7e0ef1a0f and 722f0a0b0 before resolving — main addressed the same pending visibility problem from the other direction, treating missing and false alike via an OR filter on the find side rather than writing the field on create. So the resolution here is less a merge than a question of whether this PR's approach is still the one you want.

No content review has been done on this branch yet — it's gated on the conflict being cleared.

[joshunrau]

Thanks — the create/upload divergence you found is real, and the diff is clean: lint passes, the
api suite is green, and removing the stale comment is correct since 573f307c9 did make upload
write the field.

The question this raised — should upload accept file instruments at all? — has now been answered,
and unfortunately it goes against this diff. Joshua's decision is that upload should reject
file instruments with UnprocessableEntityException, alongside the SERIES rejection already there,
rather than write them as pending. The reasoning is the one your own investigation surfaced: the
bulk-upload page lists only FORM instruments, createUploadTemplateCSV casts to a form instrument,
and $UploadInstrumentRecordsData cannot carry a file — so a record created on that path can never
be completed, whichever way pending is set. Better to fail loudly than to write something
incomplete.

That makes pending: instrument.kind === 'FILE' dead code, so this PR is being closed in favour of a
guard rather than reworked. Concretely, what replaces it:

1. In apps/api/src/instrument-records/instrument-records.service.ts:379-391, reject
   instrument.kind === 'FILE' with UnprocessableEntityException next to the existing SERIES
   rejection.
2. Replace the two pending unit tests with one asserting that rejection. Please do carry over the
   deletion of the stale spec comment — you were right that 573f307c9 made it contradict the code,
   and that should not be lost with the rest of the diff.
3. Add the end-to-end test the root CLAUDE.md asks for. This is easier now than it was for a
   pending record: the testing/ api fixture can call the upload endpoint with a file instrument
   and assert the 422, with no object storage needed.

Worth knowing: #1427 sets the same expression in the same method and is being reworked to this same
decision, so coordinate which of the two lands the guard.

If you hand this to Claude Code, Sonnet 5 is the right size — one guard, one unit test and one
e2e assertion, in files these items name outright, with the design decision already made.

Reviewed at commit 1ae1043.

[joshunrau]

superseded by #1427

[joshunrau]

PR #1422 — fix(api): keep an uploaded file instrument record pending

CLOSE · confidence: high · gdevenyi · +19/-3, 2 files
#1422 · reviewed at 1ae1043

What it does

• Changes one expression in InstrumentRecordsService.upload: pending: false becomes
  pending: instrument.kind === 'FILE', mirroring what create already writes.
• Adds two unit tests pinning both branches (FORM settles, FILE stays pending).
• Deletes a now-stale comment in the spec claiming upload does not write pending at all —
  573f307c9 made it write false unconditionally, so the comment contradicted the code.
• Net behavioural change is confined to bulk-uploading a file instrument, which today produces a
  record that FilesService.getAssociations will refuse to attach files to (Upload already completed).

Why this verdict

The inconsistency the PR names is real and the diff is minimal, clean and tested — but the decision
went the other way. Joshua has settled that upload should reject file instruments with
UnprocessableEntityException, alongside the existing SERIES rejection, rather than write them as
pending. The bulk-upload UI is FORM-only, createUploadTemplateCSV casts to
AnyUnilingualFormInstrument, and $UploadInstrumentRecordsData cannot carry a file, so a file
record created there is incomplete however pending is set. That makes this PR's one expression dead
code: the fix is a guard, not a different value, so the diff is replaced rather than reworked. Its two
unit tests go with it — a rejection test replaces them.

Settled (2026-07-28)

• upload rejects FILE instruments. Fail loudly with UnprocessableEntityException where SERIES
  is already rejected, rather than creating a record that cannot be completed. Recorded on perf(api): batch session creation and scope the upload response (alternative to #1424) #1427
  too, which carries the same expression.

Action items

1. Close this PR in favour of a guard. apps/api/src/instrument-records/instrument-records.service.ts:379-391
   — reject instrument.kind === 'FILE' with UnprocessableEntityException next to the existing
   SERIES rejection, and replace this PR's two pending tests with one asserting the rejection. The
   stale spec comment this PR deletes is a genuine find and should be carried over.
2. Whoever writes that guard should note perf(api): batch session creation and scope the upload response (alternative to #1424) #1427 sets pending: instrument.kind === 'FILE' in the same
   method and is being reworked to the same decision — only one of the two should land the guard.
3. Whatever lands needs an end-to-end test, which the root CLAUDE.md requires. A rejection is far
   easier to pin end to end than a pending record was: the testing/ api fixture can call the
   upload endpoint with a file instrument and assert the 422, with no object storage needed.

Suggested model: Sonnet 5 — one guard, one unit test and one e2e assertion, all in files the
items name, with the design decision already made.

Evidence

ok: pnpm install --frozen-lockfile, pnpm generate:env and pnpm lint all clean in a worktree on
the merge result (33/33 turbo tasks); vitest --project api green (15 files, 145 tests); read both
changed files whole in the merged tree plus files.service.ts, files.controller.ts,
packages/schemas/src/instrument-records/instrument-records.ts, apps/web/src/routes/_app/upload/*
and apps/web/src/routes/_app/instruments/render/$id.tsx; conformance checked against
apps/api/AGENTS.md, testing/AGENTS.md and the root CLAUDE.md; commit claims about 573f307c9
and 722f0a0b0 verified against history.

not checked: no live instance, so the author's runtime observations (file-instrument upload returning
201, records hidden from the datahub) were reasoned from the code rather than reproduced; the
Playwright suite was not run.

Reply to author

Thanks — the create/upload divergence you found is real, and the diff is clean: lint passes, the
api suite is green, and removing the stale comment is correct since 573f307c9 did make upload
write the field.

The question this raised — should upload accept file instruments at all? — has now been answered,
and unfortunately it goes against this diff. Joshua's decision is that upload should reject
file instruments with UnprocessableEntityException, alongside the SERIES rejection already there,
rather than write them as pending. The reasoning is the one your own investigation surfaced: the
bulk-upload page lists only FORM instruments, createUploadTemplateCSV casts to a form instrument,
and $UploadInstrumentRecordsData cannot carry a file — so a record created on that path can never
be completed, whichever way pending is set. Better to fail loudly than to write something
incomplete.

That makes pending: instrument.kind === 'FILE' dead code, so this PR is being closed in favour of a
guard rather than reworked. Concretely, what replaces it:

1. In apps/api/src/instrument-records/instrument-records.service.ts:379-391, reject
   instrument.kind === 'FILE' with UnprocessableEntityException next to the existing SERIES
   rejection.
2. Replace the two pending unit tests with one asserting that rejection. Please do carry over the
   deletion of the stale spec comment — you were right that 573f307c9 made it contradict the code,
   and that should not be lost with the rest of the diff.
3. Add the end-to-end test the root CLAUDE.md asks for. This is easier now than it was for a
   pending record: the testing/ api fixture can call the upload endpoint with a file instrument
   and assert the 422, with no object storage needed.

Worth knowing: #1427 sets the same expression in the same method and is being reworked to this same
decision, so coordinate which of the two lands the guard.

If you hand this to Claude Code, Sonnet 5 is the right size — one guard, one unit test and one
e2e assertion, in files these items name outright, with the design decision already made.

Reviewed at commit 1ae1043.

[gdevenyi]

Rebased onto 5bfe4b2dc; CI green. Kept open pending @gdevenyi's call rather than closed — flagging that so it is not read as disagreement with the decision.

The FILE-rejection guard has landed in #1427 as you directed, so this branch's pending: instrument.kind === 'FILE' is superseded there. The deletion of the stale spec comment 722f0a0b0 left behind is carried over into #1427 too, so it will not be lost whichever way this PR goes.

Nothing here needs re-reviewing — #1427 is where the decision is implemented.