OTTER-690: refactor the Set Up page and move the study title to Step 1

[therealmarv]

see OTTER-690

Rebuilds the Set Up page (Step 1) as a single card and moves the study title onto it, so every saved draft has a title from the moment it is created.

The study title moved from Step 2 to Step 1

Step 2 no longer renders a title field. The field, its validation, its persistence and its error handling all live on the Set Up page now.

Who owns study.title

Ownership is scoped by flow, not by page:

Status	Owner	How it is written
DRAFT	Step 1 (Set Up)	the Step 1 draft actions
CHANGE-REQUESTED	edit-and-resubmit	unchanged: still a collaborative Yjs field, still mirrored into the column

Nothing was deleted to get there, only scoped. The resubmit flow shares the proposal schema, the Yjs field map, buildStudyInfo and the title mirror, so dropping the title from any of them would break resubmission. In detail:

• useYjsFormMap takes an optional collabKeys set. The DRAFT Step 2 editor passes a reduced set without title; the resubmit flow passes nothing and behaves as before. Without this, a cold fields-doc would seed a blank title over the Step 1 one, and a warm one would seed a stale title.
• The title mirror is narrowed from DRAFT, CHANGE-REQUESTED to CHANGE-REQUESTED only. Legacy fields-docs still hold a title key that the DRAFT client no longer maintains, so leaving DRAFT in would flush that stale key over the Step 1 value on the next debounce.
• buildStudyInfo takes a required TitleMode ('send' | 'omit' | 'omitIfBlank') instead of two independent booleans. Required rather than defaulted, so a new caller has to choose instead of inheriting one by accident. All four write paths are covered: DRAFT save-on-navigate and DRAFT final submit omit the title, resubmit sends it, and resubmit autosave omits it only when blank (a NULL title on a non-DRAFT row violates study_title_required_when_not_draft).
• Step 2's DRAFT resolver drops the title rule. That page no longer renders the field, and a required rule on a field nobody can see is a submit blocker nothing can clear (the OTTER-647 failure mode).

The server-side rule is picked by workflow rather than by action, because every entry point except study creation is shared. onUpdateDraftStudyAction serves both Step 1 updates and the resubmit autosave, so its params schema stays permissive and the handler applies the 60-character rule only when the persisted row is DRAFT. Its schema is now declared standalone instead of derived from the create schema, so tightening creation cannot leak into it and reject a resubmit payload before the handler sees the status.

The /proposal redirect for CHANGE-REQUESTED, and why it is safe

/proposal now redirects a CHANGE-REQUESTED study to /edit-and-resubmit.

Nothing routes a study there today. The only two ways into /proposal are Step 1, which is always a DRAFT, and the dashboard resume link, which is gated on isDraft. A change-requested study goes to /submitted and then to /edit-and-resubmit, the page built for that state: it carries the reviewer feedback and the resubmission note that /proposal has no UI for. Anyone arriving on a stale bookmark now lands on the working page instead of a half-working one.

That redirect is what lets ProposalProvider be unconditionally DRAFT. Without it, four separate things would each need their own status branch: the collaborative key set, the form resolver, the title mode on three buildStudyInfo call paths, and the reviewer preview's title source. proposal/page.test.tsx pins the redirect, so reverting it fails loudly.

Drafts created before this change

The migration that made study.title nullable also cleared every 'Untitled Draft' placeholder, so drafts with no title at all exist wherever it has run. Step 2 used to give them one; it no longer has the field. Two guards cover them:

• /proposal sends a draft with a blank title back to Step 1. This matters because the dashboard routes any draft with Step 2 progress straight to /proposal, so without it those drafts would never see a title field again.
• finalizeStudySubmissionAction works out the title it is about to write (the submitted one if the caller sent it, otherwise the persisted one) and returns a normal validation error when it is blank. Before this, the status change hit study_title_required_when_not_draft and surfaced as an unhandled database error with no way to recover in the UI.

The 60-character limit

The old rule was 20 words, enforced on Step 2 and shared with the resubmit form. The new Step 1 rule is 60 characters:

• Emptiness is measured trimmed; the cap is measured raw. Mixing the two would let a 60-character title with a trailing space read 61/60 in the counter and still validate, so the counter and the validator can never disagree.
• Typing past the limit stays possible, with the error shown, rather than the input silently swallowing keystrokes.
• The over-limit error is live while typing, and it is the one field error announced through aria-live="polite", because it can appear before any blur or click. The blank-title error stays on blur and on click, so clearing the box does not flash an error mid-edit.
• Existing drafts with a longer title are never truncated. They show the error until the researcher shortens the title.

The resubmit flow deliberately keeps its 20-word rule, so the two flows now enforce different title rules. That inconsistency is real and worth a follow-up card, but harmonizing it is a bigger product change than this one.

Deferred acceptance criteria

• The post-submission read-only Set Up screen is not reachable. The derivation is implemented and unit-tested by rendering the view directly with a non-DRAFT study, so the rule itself is proven. But /edit still returns not-found for any non-DRAFT study and nothing links there, so the criteria about the disabled title surviving navigation, a reload and a new session cannot be demonstrated. Relaxing that gate is out of scope here and belongs with the later navigation cards. Please treat these as deferred rather than ticking them.
• The single "Next step" footer shown in the locked-state mockups is not in scope. All three states keep today's two-button footer; collapsing it belongs to whichever in-content-navigation card lands first.
• The page header / H1 is untouched, as the card requires.

Open question

The card says the title is disabled "permanently" once submitted, and that it should appear "only on the set up page". Read literally, that reaches the edit-and-resubmit page, which by definition renders a study that was submitted and sent back, and which today has a fully editable collaborative title. This PR leaves that page alone: the card never names it, and the rules sit under headings scoped to Set Up and Proposal. If the intent is to lock it there too, that is a separate and larger change. Worth tightening the wording either way.

Other notes

• Changing the Data Partner clears a language the new partner cannot run. Previously the stale value survived and still satisfied the enum, so validation could pass on an environment that does not exist. That sync only runs while the field is editable: once a language is locked, the persisted value wins, because a locked field renders read-only text with no error slot and a failed click has nothing to focus there.
• The Data Partner and programming language lock only once a value actually exists, not merely because a studyId does. Locking on the id alone would leave a draft that never got a language permanently uncompletable.
• The Data Partner select stays disabled until Clerk has resolved the session. A query held back by enabled reports fetchStatus: 'idle', so isLoading is false and the control would otherwise be openable with an empty list.
• Save & continue is never disabled. Clicking it is what surfaces the errors, and a disabled button explains nothing. It also switches from variant="primary", which resolves to no styles at all, to the filled variant the card asks for.
• A blank title no longer blocks Submit on Step 2, since Step 1 owns it by then. The tests asserting the old behavior were replaced rather than deleted.
• Accepted behavior change: the title is no longer live-collaborative on a DRAFT. Two co-authors editing it on Step 1 get last-write-wins instead of a CRDT merge.
• renderWithProviders accepts an optional queryClient, so a test can prime a query before the first render. A locked field renders nothing derived from its query, which leaves no DOM signal to wait on, and without priming the regression test raced the interaction it was meant to assert on.
• onUpdateDraftStudyAction gates the title-length rule on lab membership, so the message cannot tell a caller that a guessed id is a real, currently-DRAFT study. CASL scopes update Study to the caller's own labs, so a lab member outside the submitting lab is already denied before the handler runs; the gate is defense in depth against that rule widening, and it is what stops a caller who passed the ability check on a broader grant. submittedByOrgId already arrives from the middleware's read, so the gate costs no extra query and the single atomic UPDATE stays intact. The length check still runs before that UPDATE rather than after it, which is what keeps an over-long title from being written and only then rejected.
• Fixed a pre-existing flake in acknowledgements-table.test.tsx, unrelated to this card. Two tests looked up a faker-named user inside a 25-row page listing every user in the shared test database, so they failed whenever enough users happened to sort ahead of it.
• No migration: study.title is already nullable with the right check constraint.
• No screen-rules change, so docs/study-screens-logic.md needs no update.

Worth noting on the Step 2 refactor card: the title field was removed here, and Step 2 now assumes a non-blank persisted title.

Review follow-ups

• Save & continue now decides on the fields the researcher can act on, rather than on the resolver's schema-wide error flag. The resolver covers locked fields too, and a locked field renders read-only text with no error slot and nothing focusable, so a locked failure used to stop the click with no message anywhere and no field to correct. focusFirstInvalid returning null is the gate now, which is exactly "nothing on this page is failing". A locked value is the persisted server one and authoritative, so it is not the researcher's to fix.
• Creating a study requires a non-blank title. step1DraftStudyApiSchema belongs to onSaveDraftStudyAction alone, so requiring it there does not reach the update or resubmit paths, whose titles are owned elsewhere. This stops new untitled rows being minted; the /proposal and finalizeStudySubmissionAction guards stay, because rows predating this card still need them.
• CharacterCounter delegates to WordCounter rather than repeating its render, so the over-limit styling has one definition and the two cannot drift.
• finalizeStudySubmissionAction reads the persisted title in its handler, after the ability check. It cannot be folded into the middleware's read of the same row: middleware output becomes the ability subject, and requireAbilityTo serializes that subject into the permission_denied it returns to a caller it just refused, so a title carried that far would travel back to anyone who guessed a study id. getInfoForStudyId is middleware for many actions, so it now carries identifiers only. The read also cannot move into the claiming UPDATE's returning: by then the status has left DRAFT and the check constraint has already produced the raw error this guard exists to replace.
• The cross-lab title test was renamed to what it proves. CASL rejects a cross-lab caller before the handler runs, so the test pins the CASL-level outcome, not the handler's check ordering, and it now also asserts that the stored title appears nowhere in the refusal.
• The programming-language effect excludes form from its dependency array, with the same note and disable use-yjs-form-map.ts uses. Listing it re-ran the effect on every render, with only setFieldValue short-circuiting on an unchanged value standing between that and a loop.

Testing

pnpm run checks and the unit suite pass: 295 files, 3521 tests.

The end-to-end specs were updated but not executed locally. They needed updating regardless of the feature, since they drove UI this change removes: the "Proceed to Step 2" button, the STEP 1A eyebrow and the Step 2 "Study Title" field. uniqueTitle also had to be capped at 60 characters, since it could previously generate around 85 and every UI-driven flow would have stopped on the first page.

[nathanstitt]

seems to be on the right track but we need to fix the disabled button with silent error case flagged, that seems dangerous

[nathanstitt]

form.validate() runs the whole step1FieldsSchema, including the fields that are currently locked, but visibleFieldIds() deliberately drops the locked ids. So if a locked field is the only thing failing, hasErrors is true, focusFirstInvalid finds nothing to focus, and we return without opening the modal — a dead button with no visible error anywhere.

The concrete path: the PR says existing drafts with a longer title are never truncated, and a locked title renders through ReadOnlyField, which has no error slot at all. A submitted study whose title is 61+ characters would land exactly here. Same shape for a locked language once the partner's supported list changes underneath it.

That's the OTTER-647 failure mode this hook works hard to avoid everywhere else. It's unreachable today because /edit 404s anything non-DRAFT, but the whole visibleFieldIds design already says locked fields shouldn't gate, so it'd be nice if the validation agreed. Could we skip locked paths when deciding hasErrors — something like checking whether any unlocked path has an error, rather than the schema-wide flag?

[therealmarv]

Fixed, and you are right that the design already said so. The gate is now "did a field the researcher can act on fail" rather than the schema-wide hasErrors: focusFirstInvalid returns the id it flagged, or null when nothing visible failed, so its return value is the decision.

management-app/src/app/[orgSlug]/study/request/use-setup-form.ts

Lines 70 to 82 in 52e1e38

	const attemptContinue = useCallback(() => {
	const { errors } = form.validate()
	const invalidFieldId = focusFirstInvalid(visibleFieldIds(), (fieldId) => {
	const path = FIELD_ID_TO_FORM_PATH[fieldId as keyof typeof FIELD_ID_TO_FORM_PATH]
	return !!errors[path]
	})
	if (invalidFieldId) return
	openConfirm()
	}, [form, visibleFieldIds, openConfirm])

Since the resolver is step1FieldsSchema alone, validate() can only key errors under title, orgSlug or language, and all three are in FIELD_ID_TO_FORM_PATH, so the narrower check has no fourth category to swallow. Only locked paths stop gating, which is the intent.

Regression test uses your first case, a submitted study whose stored title is 61 characters: the modal opens and no message is rendered anywhere.

management-app/src/app/[orgSlug]/study/request/setup-form.test.tsx

Lines 774 to 789 in 52e1e38

	it('still continues when the only failing field is locked', async () => {
	const user = userEvent.setup()
	const fixtures = await setupFixtures()
	const overLimitTitle = 'a'.repeat(61)
	const draftData = draftFor(fixtures, { status: 'PENDING-REVIEW', title: overLimitTitle })
	renderSetup(fixtures, { studyId: draftData.id, draftData })
	expect(await screen.findByText(overLimitTitle)).toBeInTheDocument()
	expect(screen.queryByRole('textbox')).not.toBeInTheDocument()
	await user.click(continueButton())
	expect(await screen.findByText('Continue to the next step?')).toBeInTheDocument()
	expect(screen.queryByText(OVER_LIMIT_ERROR)).not.toBeInTheDocument()
	})
	})

[nathanstitt]

This is byte-for-byte the same render as WordCounter — same Text size="xs", same c={isOverLimit ? 'red' : 'dimmed'}, same {count}/{max} — and WordCounter already grew a unit prop for exactly this kind of variation.

The doc comment argues that a shared component would let a call site pass a word count against a character limit. That's true, but it's true of the split version too: nothing stops someone passing countWords(x) into count. The type is number either way, so the split buys naming, not enforcement.

If the naming is what we want, could we get it more cheaply — e.g. export const CharacterCounter = ({count, maxCharacters}) => <WordCounter wordCount={count} maxWords={maxCharacters} />? That keeps one implementation of the over-limit styling so the two can't drift, which feels like the thing actually worth protecting.

[therealmarv]

Took your cheap version. CharacterCounter keeps its name and props and delegates the render, so the over-limit styling has one definition:

management-app/src/components/character-counter.tsx

Lines 21 to 23 in 52e1e38

	export const CharacterCounter: FC<CharacterCounterProps> = ({ count, maxCharacters }) => (
	<WordCounter wordCount={count} maxWords={maxCharacters} />
	)

Agreed the split buys naming and not enforcement, and the doc comment now says that instead of claiming otherwise.

Worth knowing for direction: OTTER-737 (#991, stacked two up from here) deletes word-counter.tsx outright along with countWords and the WordCountPlugin, and CharacterCounter becomes the only counter with its body inlined again. So this delegation is deliberately pointing at the file that is going away rather than the other way round, which costs nothing since #991 rewrites this file anyway.

[nathanstitt]

Worth a look: this throws on the title length before the lab-ownership row filter further down runs, so a lab member outside the study's lab can distinguish "known id, currently DRAFT" from everything else just by sending an over-long title and reading which error comes back.

The PR notes this under "left alone on purpose", and the reasoning about not splitting the atomic UPDATE is fair. But status here already comes from getInfoForStudyId in the middleware, so we've done the read regardless — the round trip isn't the cost. The cheap version might be to move this check after verified, keeping the single UPDATE and just reordering when we complain. Happy to be told it's not worth it, but it seemed worth having the option on the record rather than only in the description.

[therealmarv]

Fixed, and thanks for putting it on the record rather than leaving it in the description. You were right that the read is already paid for.

One adjustment to the mechanics: moving the check after verified would have written the over-long title before complaining, because the UPDATE is what produces verified. So instead of reordering past it, the rule is now gated on the same fact the row filter gates on. getInfoForStudyId already selects submittedByOrgId, so the gate is free, the single atomic UPDATE is untouched, and validation still precedes the write:

management-app/src/server/actions/study-request.ts

Lines 245 to 264 in 52e1e38

	.map((org) => org.id)
	// The title rule is selected by workflow, not by action: a DRAFT row's title is owned by
	// Step 1 and capped at 60 characters, while a CHANGE-REQUESTED row's is owned by the
	// resubmit form and still governed by its 20-word rule.
	//
	// Gated on lab membership so the message cannot be used as an oracle: answering "title too
	// long" would otherwise tell any lab member that a guessed id exists and is currently a
	// DRAFT. `submittedByOrgId` comes from the middleware's read, so the gate costs no extra
	// query, and the length check still runs before the UPDATE rather than after it, which is
	// what keeps an over-long title from being written and only then complained about. A caller
	// outside the lab falls through to the generic rejection below.
	if (
	userLabOrgIds.includes(submittedByOrgId) &&
	status === 'DRAFT' &&
	(studyInfo.title?.length ?? 0) > STUDY_TITLE_MAX_CHARACTERS
	) {
	throw new ActionFailure({ title: STUDY_TITLE_OVER_LIMIT_ERROR })
	}

A caller outside the lab now falls through to the generic Study is not editable or you do not have access instead of getting a distinct title error. Test asserts the failure carries no title key and the stored title is unchanged:

management-app/src/server/actions/study-request.test.ts

Lines 722 to 742 in 52e1e38

	it('onUpdateDraftStudyAction does not leak DRAFT status to a cross-lab caller via the title rule', async () => {
	const { enclave, studyId } = await createTestProposalDraft({
	enclaveSlug: 'title-cap-cross-lab',
	studyInfo: { title: 'LabA Draft' },
	})
	const labB = await insertTestOrg({ slug: `${enclave.slug}-lab-b`, type: 'lab' })
	await mockSessionWithTestData({ orgSlug: labB.slug, orgType: 'lab' })
	const result = await onUpdateDraftStudyAction({ studyId, studyInfo: { title: OVER_LIMIT } })
	expect(result).toHaveProperty('error')
	expect(result).not.toMatchObject({ error: expect.objectContaining({ title: expect.any(String) }) })
	const after = await db
	.selectFrom('study')
	.select(['title'])
	.where('id', '=', studyId)
	.executeTakeFirstOrThrow()
	expect(after.title).toBe('LabA Draft')
	})

[nathanstitt]

Small thing: with titleMode: 'omit' now the common DRAFT path, 'title' in snapshotFields is false almost every time, so this extra selectFrom('study') fires on essentially every submit.

We could fold it into the middleware's existing study read (getInfoForStudyId) or into the claiming UPDATE's returning, and drop a round trip from the hot path. Not a correctness issue — the guard itself is a good catch and I like that it turns a raw constraint violation into a real validation error.

[therealmarv]

Fixed via the middleware route. getInfoForStudyId already reads this row, so the title now comes back with it and the second lookup is gone:

management-app/src/server/db/queries.ts

Lines 354 to 370 in 52e1e38

	export const getInfoForStudyId = async (studyId: string) => {
	return await Action.db
	.selectFrom('study')
	.innerJoin('org', 'org.id', 'study.orgId')
	.select([
	'orgId',
	'org.slug as orgSlug',
	'study.researcherId',
	'study.status',
	'study.submittedByOrgId',
	// OTTER-690: finalizeStudySubmissionAction needs the persisted title on the common
	// submit path, where the caller omits it. Selected here so that path reuses this read
	// instead of issuing a second lookup of the same row.
	'study.title',
	])
	.where('study.id', '=', studyId)
	.executeTakeFirstOrThrow()

management-app/src/server/actions/study-request.ts

Lines 425 to 431 in 52e1e38

	// /proposal redirects such a draft to Step 1 before it can reach this point.
	//
	// The persisted title comes from the middleware's read of this same row, so the common
	// omitted-title path costs no extra query. It cannot be deferred to the UPDATE's
	// `returning` instead: by then the status has already left DRAFT and the check constraint
	// has fired, which is the raw error this guard exists to replace.
	const submittedTitle = 'title' in snapshotFields ? (snapshotFields.title as string | null) : persistedTitle

The returning option turns out not to work here: by the time the claiming UPDATE returns, the status has already left DRAFT, so study_title_required_when_not_draft has fired and produced exactly the raw error this guard exists to replace. That is now written down next to the check so nobody tries it later.

[therealmarv]

Reversing part of this, and the round trip you flagged comes back. Sorry for the churn.

Folding the read into getInfoForStudyId put study.title into the ability subject, and requireAbilityTo serializes that subject into the permission_denied it returns to a caller it just refused (action.ts:102-118, then :210). The note above that code spells out the rule I broke: middleware may return ids and slugs, never row content, because the subject is handed to someone who has no right to the record. It is middleware for many actions, so every one of them was returning the study's title to anyone who guessed an id. Your cross-lab oracle comment on the other thread is what surfaced it.

So the shared query carries identifiers only again, and the persisted title is read in the handler, after the ability check:

management-app/src/server/db/queries.ts

Lines 398 to 409 in 7b6a42f

	.select([
	'orgId',
	'org.slug as orgSlug',
	'study.researcherId',
	'study.status',
	'study.submittedByOrgId',
	// No row content here, however convenient it would be for a handler: this is middleware
	// for many actions, its output becomes their ability subject, and requireAbilityTo
	// serializes that subject into the permission_denied it returns to a caller it just
	// refused (OTTER-724 / MA-6). A title selected here would travel back to anyone who
	// guessed a study id. Read content in the handler, which only runs after the check.
	])

management-app/src/server/actions/study-request.ts

Lines 430 to 440 in 7b6a42f

	// Read here rather than folded into the middleware's read of this same row: middleware
	// output becomes the ability subject, and requireAbilityTo serializes that subject into the
	// permission_denied it returns to a caller it just refused, so a title carried that far
	// would travel back to anyone who guessed a study id (OTTER-724 / MA-6). It cannot be
	// deferred to the UPDATE's `returning` either: by then the status has already left DRAFT
	// and the check constraint has fired, which is the raw error this guard exists to replace.
	const submittedTitle =
	'title' in snapshotFields
	? (snapshotFields.title as string | null)
	: ((await db.selectFrom('study').select('title').where('id', '=', studyId).executeTakeFirst())?.title ??
	null)

Your returning finding still stands unchanged and is still written down there. The lookup is one extra query per submit, on a path that is already doing an UPDATE plus three signed-URL calls, so I think it is the right trade against the disclosure. A middleware chained after requireAbilityTo would keep the handler clean and read just as late, but nothing in the codebase does that today and it seemed the wrong PR to introduce the pattern. Say the word if you would rather have it that way.

[nathanstitt]

form in the dep array is a fresh object on every render (Mantine rebuilds the return value each time), so this effect re-runs on every render rather than only when selectedOrgSlug / data / isLocked change.

It happens to be harmless because setFieldValue short-circuits when the value is unchanged — that's the only thing standing between this and a render loop. The codebase elsewhere handles exactly this by excluding form with an exhaustive-deps disable and a note about Mantine ref semantics (use-yjs-form-map.ts does it twice). Could we do the same here so the safety isn't resting on a library implementation detail? The carried-over version had the same shape, so this is inherited rather than introduced — just a good moment to fix it while the file is being rewritten anyway.

[therealmarv]

Done, matching the use-yjs-form-map.ts shape (prose note, then the bare disable on the line before the array, which is where the rule actually reports):

management-app/src/app/[orgSlug]/study/request/fields/programming-language-field.tsx

Lines 89 to 93 in 52e1e38

	// form intentionally excluded: Mantine rebuilds it every render, so listing it would re-run
	// this every render, with only setFieldValue's no-op-on-unchanged-value standing between
	// that and a loop. Stable via Mantine ref semantics, as in use-yjs-form-map.
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [selectedOrgSlug, data, isLocked])

Fair point that it was inherited rather than introduced. Good moment to fix it, agreed.

[nathanstitt]

The || undefined here means a blank title is dropped from the payload entirely rather than sent. On the update path that's exactly right — it preserves the stored title.

On the create path it means we can persist a brand-new study row with no title at all, since step1DraftStudyApiSchema accepts title as nullable/optional. The comment says the Save & continue gate makes that unreachable, and it does today, but it's the same class of untitled-draft row that this PR then has to add two separate recovery guards for (/proposal redirect, finalizeStudySubmissionAction).

Since onSaveDraftStudyAction is Step 1's alone, would it be worth making title required (non-empty) in step1DraftStudyApiSchema for the create case, so we stop minting new rows that need those guards?

[therealmarv]

Worth it, done. step1DraftStudyApiSchema now requires a non-blank title, and since it is onSaveDraftStudyAction's alone the tightening does not reach the update or resubmit paths:

management-app/src/app/[orgSlug]/study/request/form-schemas.ts

Lines 146 to 163 in 52e1e38

	// Step 1 study creation only. `draftStudyApiSchema` replaces `title` outright, so tightening
	// its parent would silently be a no-op; the rule has to be applied to the override.
	//
	// Required and non-blank, unlike the parent's nullable/optional title: this is the one entry point
	// that mints a study row, and every untitled row it creates is one the recovery guards in
	// /proposal and `finalizeStudySubmissionAction` then have to rescue. Step 1's Save & continue gate
	// already makes a blank create unreachable through the UI; requiring it here means a future caller
	// cannot reintroduce the case by forgetting. Rows predating OTTER-690 still need those guards.
	//
	// Cap before blank, so the message matches what the user did: 61 characters reports the limit,
	// while whitespace-only reports the blank rule. Emptiness is measured trimmed, matching
	// `studyTitleField`; the client trims before sending.
	export const step1DraftStudyApiSchema = draftStudyApiSchema.extend({
	title: z
	.string()
	.max(STUDY_TITLE_MAX_CHARACTERS, { message: STUDY_TITLE_OVER_LIMIT_ERROR })
	.refine((val) => val.trim().length > 0, { message: STUDY_TITLE_BLANK_ERROR }),
	})

Cap is checked before blankness so the message matches what the caller did, and emptiness is measured trimmed like studyTitleField.

The || undefined had to split, since one payload can no longer serve both calls. Update keeps it for the reason you gave; create requires a title and the guard gives TypeScript the narrowing:

management-app/src/contexts/study-request/hooks/use-save-draft.ts

Lines 33 to 69 in 52e1e38

	piName: formValues.piName || undefined,
	language: formValues.language || undefined,
	}
	let result
	if (studyId) {
	// `undefined` rather than `null` on update: an accidental blank save must never
	// clear a stored title, and this action also serves the resubmit autosave, whose
	// title is owned elsewhere.
	result = actionResult(
	await onUpdateDraftStudyAction({
	studyId,
	studyInfo: { ...draftInfo, title },
	}),
	)
	} else {
	if (!formValues.orgSlug) {
	throw new Error('Data Partner is required to create a study')
	}
	// Creation cannot fall back to omitting the title: an untitled row is what the
	// /proposal and finalize guards exist to rescue, so `step1DraftStudyApiSchema`
	// requires one. The Save & continue gate means this is unreachable in practice.
	if (!title) {
	throw new Error('Study title is required to create a study')
	}
	result = actionResult(
	await onSaveDraftStudyAction({
	orgSlug: formValues.orgSlug,
	studyInfo: { ...draftInfo, title },
	submittingOrgSlug,
	}),
	)
	}
	return { studyId: result.studyId }
	},
	onSuccess({ studyId: newStudyId }) {

The two recovery guards stay, because rows predating this card exist regardless. This only stops new ones joining them. Something that argued for it: the tests needing an untitled row already manufacture one with a direct update study set title = null, rather than getting one out of the action.

One existing test changed as a result. The OpenStax flow test created its draft with language only and asserted title came back NULL, which described the pre-OTTER-690 flow where Step 2 supplied the title. It now passes one at create, matching what Step 1 actually sends:

management-app/src/server/actions/study-request.test.ts

Lines 699 to 720 in 52e1e38

	it('onSaveDraftStudyAction rejects a create with no usable title', async () => {
	const enclave = await insertTestOrg({ type: 'enclave', slug: 'title-required-enclave' })
	const lab = await insertTestOrg({ slug: 'title-required-lab', type: 'lab' })
	await mockSessionWithTestData({ orgSlug: lab.slug, orgType: 'lab' })
	const blank = await onSaveDraftStudyAction({
	orgSlug: enclave.slug,
	submittingOrgSlug: lab.slug,
	studyInfo: { title: ' ', language: 'R' as const },
	})
	expect('error' in blank).toBe(true)
	const untitled = await db
	.selectFrom('study')
	.select('id')
	.where('submittedByOrgId', '=', lab.id)
	.executeTakeFirst()
	expect(untitled).toBeUndefined()
	})
	// The title rule must not answer before the ownership filter does: a distinct
	// "title too long" would tell any lab member that a guessed id is a real, currently-DRAFT

[nathanstitt]

This test passes with or without the fix it is meant to protect.

I reverted the handler gate to the old unconditional form:

if (status === 'DRAFT' && (studyInfo.title?.length ?? 0) > STUDY_TITLE_MAX_CHARACTERS) {

and this test still passed. A console.log at the top of the handler never printed. The actual rejection is:

{"error":{"permission_denied":"in onUpdateDraftStudyAction action; cannot update Study..."}}

The cross-lab caller is stopped by CASL in requireAbilityTo('update', 'Study') before the handler body runs at all, because permissions.ts:104 is row-scoped:

permit('update', 'Study', { submittedByOrgId: { $in: usersResearcherOrgIds } })

That scoping landed in f1b7761 ("Enforce the submitting lab through CASL instead of a handler check"), which is already in this PR's base. So a cross-lab caller can never reach the title check regardless of its position, and the not.toMatchObject assertion is satisfied by the permission_denied shape rather than by the ordering.

Two consequences worth separating:

1. The reordering itself is still the right shape — defence in depth, and it costs nothing since submittedByOrgId comes from the middleware read. Keep it.
2. The test's name promises it guards the oracle, and it doesn't. As written it would stay green if someone moved the title check back above the lab gate, which is exactly the revert it exists to catch.

If you want it to pin the behaviour, it needs a caller who passes CASL but fails the lab-membership gate — e.g. a user who belongs to some lab in session.orgs but not the study's submittedByOrgId, so the handler is actually entered. Otherwise I'd suggest renaming it to reflect what it really asserts (that a cross-lab update is rejected without a title-specific message) so the coverage isn't overstated.

No objection to merging either way — the production behaviour is correct.

[therealmarv]

You are correct. The test does not protect the gate. I put the gate back to its old form, and the test still passed.

But you cannot make the caller that you suggest. CASL builds usersResearcherOrgIds from the lab orgs in the session (permissions.ts:43). The handler builds userLabOrgIds from the same orgs. Each one then compares its list to the same submittedByOrgId. This is one rule, written two times.

So CASL refuses a member of a different lab before the handler starts. Only an SI admin who holds manage all can pass CASL and then fail the gate.

I did not write a test for the SI admin. The oracle is not a risk for that user, and such a test would make a rule from a condition that nobody selected. I used your second option instead. The gate stays, the test name tells what the test does, and the comment gives CASL as the cause of the refusal.

management-app/src/server/actions/study-request.test.ts

Lines 719 to 741 in 7b6a42f

	// CASL denies a cross-lab caller before the handler runs, so what this pins is the
	// CASL-level outcome rather than the handler's check ordering: the refusal carries no
	// title-specific message, tells the caller nothing about the stored title, and writes
	// nothing. The last assertion matters because requireAbilityTo serializes the ability
	// subject into the message it returns, so anything the middleware reads goes back to a
	// caller who was just refused.
	it('onUpdateDraftStudyAction rejects a cross-lab update without disclosing the stored title', async () => {
	const { enclave, studyId } = await createTestProposalDraft({
	enclaveSlug: 'title-cap-cross-lab',
	studyInfo: { title: 'LabA Draft' },
	})
	const labB = await insertTestOrg({ slug: `${enclave.slug}-lab-b`, type: 'lab' })
	await mockSessionWithTestData({ orgSlug: labB.slug, orgType: 'lab' })
	const result = await onUpdateDraftStudyAction({ studyId, studyInfo: { title: OVER_LIMIT } })
	expect(result).toHaveProperty('error')
	expect(result).not.toMatchObject({ error: expect.objectContaining({ title: expect.any(String) }) })
	expect(JSON.stringify(result)).not.toContain('LabA Draft')
	const after = await db
	.selectFrom('study')
	.select(['title'])

I also corrected the comments at study-request.ts:239 and :396. They said that CASL update Study is "org-type-scoped (any lab member)". Commit f1b7761 made that text incorrect.

Your comment showed a more serious problem. In my change for your comment about the second lookup, I added study.title to the middleware. The data from the middleware becomes the ability subject. requireAbilityTo then puts that subject into the permission_denied message, and the action sends this message to the caller that it refused. In this test, the caller from the other lab received "title": "LabA Draft". The old assertion did not find it, because the assertion examined only the title key.

getInfoForStudyId now gives identifiers only. The handler reads the title after the permission check. The test also makes sure that the refusal shows no part of the stored title. This change adds again the second query that you asked me to remove, and I give the reasons on that thread.

management-app/src/server/db/queries.ts

Lines 398 to 409 in 7b6a42f

	.select([
	'orgId',
	'org.slug as orgSlug',
	'study.researcherId',
	'study.status',
	'study.submittedByOrgId',
	// No row content here, however convenient it would be for a handler: this is middleware
	// for many actions, its output becomes their ability subject, and requireAbilityTo
	// serializes that subject into the permission_denied it returns to a caller it just
	// refused (OTTER-724 / MA-6). A title selected here would travel back to anyone who
	// guessed a study id. Read content in the handler, which only runs after the check.
	])

management-app/src/server/actions/study-request.ts

Lines 430 to 440 in 7b6a42f

	// Read here rather than folded into the middleware's read of this same row: middleware
	// output becomes the ability subject, and requireAbilityTo serializes that subject into the
	// permission_denied it returns to a caller it just refused, so a title carried that far
	// would travel back to anyone who guessed a study id (OTTER-724 / MA-6). It cannot be
	// deferred to the UPDATE's `returning` either: by then the status has already left DRAFT
	// and the check constraint has fired, which is the raw error this guard exists to replace.
	const submittedTitle =
	'title' in snapshotFields
	? (snapshotFields.title as string | null)
	: ((await db.selectFrom('study').select('title').where('id', '=', studyId).executeTakeFirst())?.title ??
	null)

I recommend a different card for one more problem. The subject always contained status and researcherId, for all actions that use this middleware. The correction is to keep the serialized subject in the logger and in Sentry, and to stop sending it to the caller. The work looks small: 33 of the 35 test references to permission_denied use objectContaining, and one uses stringContaining. Tell me if you agree, and I will make the card.

[github-actions]

Total coverage

Lines	Branches	Functions	Statements
96.52% (+0.07%)	77.28% (+0.09%)	93.03% (+0.17%)	95.80% (+0.07%)

Detailed report

3 files with a coverage regression

	File	Lines	Branches	Functions	Statements
✅	src/app/[orgSlug]/study/[studyId]/proposal/form.tsx	82.05% (+1.20%)	50.00%	36.36% (+3.03%)	82.05% (+1.20%)
✅	src/app/[orgSlug]/study/[studyId]/proposal/page.test.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/app/[orgSlug]/study/[studyId]/proposal/page.tsx	95.65% (+95.65%)	75.00% (+75.00%)	100.00% (+100.00%)	95.65% (+95.65%)
✅	src/app/[orgSlug]/study/[studyId]/proposal/reviewer-preview.tsx	100.00%	80.00% (+10.00%)	75.00%	100.00%
✅	src/app/[orgSlug]/study/request/fields/data-partner-field.tsx	100.00% (+100.00%)	88.88% (+88.88%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/app/[orgSlug]/study/request/fields/field-ids.ts	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/app/[orgSlug]/study/request/fields/programming-language-field.tsx	94.44% (+94.44%)	89.47% (+89.47%)	77.77% (+77.77%)	94.64% (+94.64%)
✅	src/app/[orgSlug]/study/request/fields/study-title-field.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
🚫	src/app/[orgSlug]/study/request/form-schemas.test.ts	100.00%	50.00%	100.00%	95.00% (-5.00%)
✅	src/app/[orgSlug]/study/request/form-schemas.ts	82.75% (+12.17%)	57.14% (+32.14%)	37.50% (+20.84%)	80.00% (+13.34%)
✅	src/app/[orgSlug]/study/request/proposal.tsx	100.00% (+23.08%)	100.00% (+25.00%)	85.71% (+52.38%)	100.00% (+23.08%)
✅	src/app/[orgSlug]/study/request/setup-form.test.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/app/[orgSlug]/study/request/setup-form.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/app/[orgSlug]/study/request/use-setup-form.ts	100.00% (+100.00%)	87.50% (+87.50%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/app/admin/safeinsights/legal/tos-pn/acknowledgements-table.test.tsx	100.00%	50.00%	93.33% (+0.48%)	100.00%
✅	src/components/character-counter.test.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/components/character-counter.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
🚫	src/components/editable-text/collaborative-editor.tsx	88.18% (-3.94%)	68.33% (-10.00%)	72.41% (-6.90%)	84.28% (-5.72%)
✅	src/components/form-field.tsx	100.00%	96.87% (+0.32%)	100.00%	97.72% (+0.29%)
✅	src/components/read-only-field.test.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/components/read-only-field.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/components/study/proposal-step-header.test.tsx	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
🚫	src/components/study/proposal-step-header.tsx	100.00%	88.88% (-11.12%)	100.00%	93.75% (-6.25%)
✅	src/lib/realtime/yjs-websocket-context.test.tsx	100.00%	100.00%	96.62% (+1.12%)	100.00%
✅	src/server/actions/study-request.ts	96.74% (+0.08%)	76.59% (+0.99%)	92.30%	95.04% (+0.11%)