feat: prefer canonical awaiting-input contract#20
Conversation
Read top-level fields/choices from managed-auth state events and responses when present, mapping them into the existing form rendering model. Keep falling back to legacy discovered_fields/pending_sso_buttons/mfa_options/sign_in_options so older API responses remain supported during the deprecation window. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
When canonical fields/choices are present, submit field_values by canonical field ID and selected_choice_id by canonical choice ID. Continue supporting legacy fields, SSO selectors, MFA option IDs, and sign-in option IDs when canonical data is absent. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
| () => | ||
| button.id | ||
| ? submitSelectedChoice(sessionId, jwt, button.id, options) | ||
| : submitSSOButton(sessionId, jwt, button.selector, options), |
There was a problem hiding this comment.
MFA ignores canonical choice submit
Medium Severity
MFA options built from canonical mfa_method choices still submit via mfa_option_id, while SSO clicks with button.id use selected_choice_id. If the server expects canonical choice ids on submit, MFA selection can fail even when the UI was rendered from choices.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 30a2d0b. Configure here.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| fields: ev.fields ?? base.fields ?? null, | ||
| choices: ev.choices ?? base.choices ?? null, | ||
| discovered_fields: ev.discovered_fields ?? base.discovered_fields ?? null, | ||
| pending_sso_buttons: | ||
| ev.pending_sso_buttons ?? base.pending_sso_buttons ?? null, | ||
| mfa_options: ev.mfa_options ?? base.mfa_options ?? null, | ||
| sign_in_options: ev.sign_in_options ?? base.sign_in_options ?? null, |
There was a problem hiding this comment.
mergeStateEvent's ?? base.* fallback on the canonical/legacy input arrays leaks a previous AWAITING_INPUT step's content (e.g. login SSO buttons rendering on the MFA step) because canonical snapshots express "category no longer applies" by omission.
Co-authored-by: Cursor <cursoragent@cursor.com>
masnwilliams
requested review from
dcruzeneil2
and removed request for
dcruzeneil2
State events are complete snapshots: the server omits fields/choices when the current step has none, so retaining the prior values leaks a previous step's inputs (e.g. login SSO buttons rendering on the MFA step).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aa4d14b. Configure here.
| .map((choice) => ({ | ||
| type: normalizeMFAChoiceId(choice.id), | ||
| label: choice.label, | ||
| description: choice.description ?? undefined, |
There was a problem hiding this comment.
MFA canonical mapping drops display text
Medium Severity
mfaOptionsFromCanonical only copies description from each MFA choice, while signInOptionsFromCanonical also falls back to context and display_text. Masked phone numbers or similar MFA hints often live in those fields, so canonical MFA steps can show labels without the extra detail legacy target used to provide.
Reviewed by Cursor Bugbot for commit aa4d14b. Configure here.
| ssoButtonsFromCanonical(state.choices) ?? state.pending_sso_buttons, | ||
| mfa_options: mfaOptionsFromCanonical(state.choices) ?? state.mfa_options, | ||
| sign_in_options: | ||
| signInOptionsFromCanonical(state.choices) ?? state.sign_in_options, |
There was a problem hiding this comment.
Legacy fallback ignores canonical presence
Medium Severity
normalizeManagedAuthState treats a failed or empty canonical conversion as “no canonical data” and falls back to legacy discovered_fields, pending_sso_buttons, mfa_options, and sign_in_options. When the response includes a non-null fields or choices key (including [] or choices with only some types), legacy arrays on the same payload can still render, so a step can show inputs the canonical contract meant to omit.
Reviewed by Cursor Bugbot for commit aa4d14b. Configure here.
There was a problem hiding this comment.
The final shape is close and the hard parts are right: canonical-first with legacy fallback is the correct migration posture, the client only sends canonical submit payloads after the server has sent canonical data first (so rollout ordering is safe), and the snapshot-replace fix in mergeStateEvent landed on the correct semantics. CI is green.
I checked the types and submit payloads here against the upcoming API behavior and most of it lines up: the field and choice type enums match, field_values keyed by canonical field id round-trips correctly, and SSO submit via selected_choice_id works. Two things don't line up, and one behavioral gap will show up as soon as the API starts emitting canonical data. Requesting changes on the first two, the rest are non-blocking.
Blocking
-
The submit strategy is split down the middle, and the MFA half loses information. Fields and SSO now submit canonically, but MFA and sign-in selections rendered from canonical choices still go through the legacy helpers. For MFA this is lossy:
mfaOptionsFromCanonicalsquasheschoice.idthroughnormalizeMFAChoiceId, and sinceMFAOptionhas noid, the original choice id is gone by submit time. Anything unrecognized collapses to"other", so two unfamiliar MFA methods become indistinguishable to the server. This happens to work right now because of how the server currently builds these ids, but that's a coincidence, not a contract. The fix I'd like to see: add an optionalidtoMFAOption(andSignInOption) and submitselected_choice_idwhenever the option came from canonical data, same assubmitSSOalready does. If we'd rather not do that yet, then revert fields/SSO to legacy submits too so the switch happens as one coherent change. Half-and-half is the one state we shouldn't ship. Also worth noting the PR description still says everything submits through legacy helpers, which stopped being true a few commits ago. -
ManagedAuthChoice.display_textandcontextdon't exist in the API contract. The fallback chain insignInOptionsFromCanonical(description ?? context ?? display_text) can never fire, and the type definitions will convince the next reader that the wire carries fields it doesn't. Please drop both, or hold this until they actually exist on the API side if that's the plan.
Should fix
-
MFA options rendered from canonical lose the masked destination. Legacy
MFAOption.targetcarries hints like***-***-5678, the canonical choice has no slot for it, and this PR prefers canonical when present. So the moment the API ships canonical data, MFA steps stop showing the masked phone/email even though the legacy arrays in the same payload still have it. This is deterministic, not an edge case. A reasonable client-side fix while the dual contract exists: carrytargetover from the matching legacymfa_optionsentry during normalization. -
The legacy fallback should key off whether canonical data is present, not whether the conversion produced anything. Right now a payload with canonical
choicescontaining only SSO buttons plus a stale legacymfa_optionsarray would render both, mixing content from two different steps. Not reachable with the current server since both views come from the same source, but the fix is one line per category: ifchoicesis non-null, derive every choice-based category from canonical only, and same forfields.
Nits
-
The snapshot-vs-merge question took three review rounds to settle and the answer now lives only in a comment.
mergeStateEventand the*FromCanonicalhelpers are pure functions, so a small test file would lock in "absent key means cleared" before someone re-adds the?? base.fieldsfallback because it looks like the safe thing to do. -
ManagedAuthResponseis exported and now referencesManagedAuthFieldandManagedAuthChoice, which aren't exported fromindex.ts, so consumers can see the shape but can't name it. And given the new public type surface and new submit behavior, this feels like aminorrather than apatch.
Happy to take another look as soon as 1 and 2 are in. Everything else here is small.
2 participants
Summary
fieldsandchoices.discovered_fields,pending_sso_buttons,mfa_options, andsign_in_optionsso old API responses keep working during the deprecation window.Notes
This PR is forward-compatible with the API dual-contract migration. It does not remove legacy support and it still submits through the existing legacy submit helpers until the API accepts canonical submit payloads.
Test plan
bun run typecheckbun run buildMade with Cursor