feat: bind canonical managed-auth submissions - #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.
|
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>
Co-authored-by: Cursor <cursoragent@cursor.com>
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.
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.
dcruzeneil2
left a comment
There was a problem hiding this comment.
Approving — all four items from my last review are addressed.
Two non-blocking follow-ups:
-
Canonical fields lose
placeholderandhint.fieldsFromCanonicalcopies id/ref/name/type/label/required and nothing else, but the form also rendersplaceholderandhint— so both silently disappear whenever a step renders from canonical data.hintis the helper line like "Enter the phone ending in (***) ***-**92"; on a code-entry step it's the only thing telling the user where their code went. Easy to miss in manual testing unless the flow you run actually has hint text on a step. Fix is simple: canonicalrefmatches the legacy field'sname, so you can look up the correspondingdiscovered_fieldsentry by ref and carry the two properties over. -
The MFA
targetcarryover pairs by position.legacyMFATargetsgives the Nth canonical sms choice the Nth legacy sms target. That holds today but isn't guaranteed by the contract — if the orders ever diverge, we'd display one masked number on a button that actually selects a different destination, with no error anywhere. Canonical and legacy entries for the same option share a label, so matching on(type, label)is exact and removes the ordering assumption.
6565d3f to
f28a7d6
Compare
f28a7d6 to
e382373
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 03aa03a. Configure here.
| return; | ||
| } catch { | ||
| connectStream(jwt); | ||
| } |
There was a problem hiding this comment.
Stale refresh ignores session teardown
High Severity
The new stale_interaction resync awaits retrieveManagedAuth and then writes stateRef, calls setState, and may connectStream without checking generationRef (or exchangeRef.active). The other retrieve paths bail out after that await when the session effect has cleaned up. After unmount or a new sessionId/handoffCode, this path can still apply the old snapshot, fire success/error callbacks, and open an SSE connection that nothing will tear down.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 03aa03a. Configure here.
| } catch { | ||
| connectStream(jwt); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale refresh skips expiry handling
Medium Severity
If the stale_interaction GET fails, the catch always calls connectStream and then falls through to awaiting_input. resyncAndConnect treats 401/410 on the same GET as an expired session: it sets terminalRef, shows expired UI, and fires onError. Here an expired JWT instead reconnects SSE and puts the user back on the form, so they can keep submitting until a later stream error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 03aa03a. Configure here.


summary
interaction_idand reject incomplete canonical state instead of falling back to legacy inputsstale_interactiontesting
bun run testbun run typecheckbun run buildbun run format:checkcd packages/managed-auth-react && npm pack --dry-runbackend dependency
Deploy kernel/kernel#3185 and kernel/kernel#3380 before publishing this client update.