[HELD for deploy window] fix(onboarding): key answers on uid so a failed step cannot wedge the wizard (#33 isolated) - #35
Open
EddyOne81 wants to merge 1 commit into
Conversation
* fix(onboarding): key answers on uid, let any step create the row, true reset
Backend and schema half of the onboarding reliability fix. Pairs with
onboarding-ui fix/onboarding-flow-reliability — the save_* procedures gain a
_uid parameter, so both must ship together.
session_id was the only write key. A session is transient: it rotates on
re-login, token refresh and expiry, and keying durable survey answers on it
meant the answers became unreachable the moment it changed. Worse, only
save_onboarding_user_info could INSERT — every other step was a bare UPDATE
that raised "Onboarding session not found. Start at step 1." So a single
failed or late step-1 wedged the whole wizard permanently, and a mid-flow
re-login did the same.
Adds a nullable uid column (additive, indexed, non-unique) and routes every
procedure through onboarding_resolve_row, which resolves in a fixed order:
the row for this session (unchanged behaviour for every existing record, and
it stamps uid as it goes, so rows migrate themselves on first touch), else
the user's most recent row re-pointed at the new session, else a fresh row.
Steps can now create the row they write to, so one failure no longer poisons
the rest of the flow. mark_onboarding_complete still validates firstname /
industry / role / team_size, so a stub row can never pass as complete.
Empty tool and challenge selections now overwrite instead of being rejected:
save_onboarding_tools no longer throws on an empty array, and
check_onboarding_completion NULL-tests rather than length-tests, so
"none of these" is a real answer rather than an unanswered step.
The tools "Other" free text moves into its own tools_other column, matching
industry_other / role_other, instead of being spliced into the current_tools
JSON array where nothing could distinguish a canonical key from user input.
Normalisation runs inside the procedure, so a client posting the old inline
shape still produces a clean array plus a populated tools_other; an idempotent
backfill converts existing rows without touching clean ones or their mtime.
reset() cleared authorization and nothing else: it discarded the session but
kept the data, orphaning a row per reset and restarting the wizard against a
dead session. It now deletes the user's answers (including orphans left by the
old behaviour) and leaves the login alone.
Security: fast_check "public-api" short-circuits the ACL to GRANTED before src
is ever evaluated, so "src": "anonymous" on these services was unreachable
rather than merely permissive. User-data endpoints move to src "owner" with no
fast_check; get_env, get_countries and save_signup_info stay public. Every
handler also gained a service-level identity guard, which does not depend on
the ACL being configured correctly. update_profile validated identity AFTER
destructuring this.user's profile, throwing for anonymous callers instead of
returning the "no-user" answer two lines below; it now checks first and
resolves its row by uid before falling back to email. Several handlers tested
`!this.uid`, which is truthy for ID_NOBODY and let anonymous callers through.
Verified against a scratch MariaDB: all 14 files apply to both a simulated
pre-migration v2 table and a fresh install; session rotation, step-out-of-order
creation, empty-selection clearing, legacy tools normalisation, uid stamping
and reset all behave as intended, and the validation guards still reject bad
values.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(schema): relax lastname to NULL, repairing pre-existing drift
Caught by a smoke test against stage, which failed every write with
"Field 'lastname' doesn't have a default value".
tables/onboarding_responses.sql has declared lastname NULL since the v2
rework — it is collected at signup, not by the wizard — but instances created
from the v1 definition still carry NOT NULL, and alter_onboarding_responses_v2
never relaxed it. Stage is one of them.
Under STRICT_TRANS_TABLES (the server default there) that makes any INSERT not
naming lastname fail outright. It breaks onboarding_resolve_row's stub insert,
and it equally breaks the v2 wizard's own step 1, which posts firstname only —
so this was already a latent bug for new users on any drifted instance, not
something the uid work introduced.
Widening NOT NULL -> NULL cannot lose data and MODIFY is idempotent, so this
is a no-op on correct instances. Ordered before the procedures by the manifest.
Verified by reproducing the exact stage schema and sql_mode locally: the call
fails before the migration, succeeds after, the migration re-runs clean, and
the full wizard suite (out-of-order step, session rotation, empty selection,
tools "other", completion, reset) passes on the repaired table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(acl): revert onboarding to public-api reachability; auth stays in service
src:owner denied every onboarding call on stage with PERMISSION_DENIED:
[onboarding.save_user_info][DENIED] to uid=650d04e8650d04ec
on hub_id=cd1cbbc8cd1cbbdc, nid: undefined
The caller is authenticated - a real uid, not ID_NOBODY - so this is a
privilege failure, not an identity one. Onboarding requests carry no hub_id
(the payload is firstname + socket_id + device_id), so the ACL resolves them
against the ENDPOINT's hub rather than the caller's. src:owner then asks
whether a user midway through onboarding owns the endpoint hub. They never do.
My justification for src:owner was contact.invite, which uses exactly that
shape and is called successfully from this same wizard. That reasoning was
wrong: contact.invite passes hub_id: Visitor.id, so it resolves to the
caller's OWN hub, where they are the owner. Same src value, different subject.
There is no fast_check meaning "any authenticated user" (the options are
user_permission, guest_permission, socket_bound, public-api), and the
MFS-based path cannot express it either, so the ACL layer cannot carry this
requirement at all for a plugin mounted on its own hub.
Reachability therefore goes back to what worked, and authentication stays
where it is actually enforceable - _identity() in service/onboarding.js, which
rejects anonymous callers with 401 and keys every row on the caller's uid.
That guard was written to not depend on ACL configuration, which is exactly
the property needed here.
This is still stronger than the original: before, these endpoints had no
authentication whatever and any caller with a session id could write or read
another user's answers; update_profile also touched this.user before checking
identity. Those remain fixed.
Each service now carries a doc field recording why src:owner must not be
reintroduced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(changelog): restore truncated history; record lastname repair and ACL note
Two things.
First, a repair. The 2026-08-04 entry added in d6a3b40 was written with
`open(p,'w').write(entry + open(p).read())`, which truncates the file before
the read in the same expression evaluates — so the read returned empty and the
prepend replaced the changelog instead of extending it. All six prior entries
(2025-11-17 through 2026-07-20) were lost in that commit and are restored here
from test, verified byte-identical before re-prepending. The diff against test
is now additions only.
Second, the entry itself gains what was learned applying this patch to stage:
- alter_onboarding_responses_identity.sql also repairs pre-existing drift,
relaxing lastname from NOT NULL to NULL. Under STRICT_TRANS_TABLES that
drift broke any insert not naming the column — the new stub insert, and
already the v2 wizard's own step 1.
- what was applied to stage, which database, and where the rollback set was
left.
- that acl/onboarding.json must keep fast_check public-api. src:owner was
tried and denied every call: onboarding requests carry no hub_id, so the
ACL resolves them against the endpoint's hub, which a user in onboarding
never owns. Authentication lives in _identity() instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(onboarding): accept whitespace-padded email; stop the role dead-end
Two production failures from 6_8ebdc3818ebdc382. Neither was fixed by the
uid work — and the new no-advance-on-failure behaviour turned both from
silent data loss into a hard block, so they had to be resolved together.
1. "Invalid email format" on save_onboarding_user_info
parameters: [..., 'exadim349@gmail.com ', '']
^ trailing space
The format check is anchored (^...$) and nothing trimmed, so one stray
space rejected an otherwise valid address. The address is never typed into
the wizard — it is carried from signup or backfilled from the account
profile — so the user had no field to correct and no way past step 1.
Now normalised before validation, in the procedure and in the service.
REGEXP_REPLACE rather than TRIM, because bare TRIM() strips spaces only and
a tab or newline from a paste or import would still fail. Genuinely
malformed addresses are still rejected; internal spaces are preserved.
2. "Step 3 (role) is incomplete." on mark_onboarding_complete
The footer grouped step 2 (Role) with step 5 (Goals) and so offered it a
"Tell me later", while mark_onboarding_complete treats role as mandatory —
its own comment even claims "no Tell me later in UI for these steps", which
this footer contradicted. Skipping role produced a wizard that could be
walked to the end and then refused to complete. Previously the refusal was
swallowed and the user was let into the workspace with onboarded unset;
now it is surfaced, and the done screen has no Back button, so they would
have been trapped on a screen whose only button fails forever.
Fixed on both sides. Role no longer offers "Tell me later", matching the
documented mandatory set (steps 4, 5 and 6 keep theirs). And a
mark_complete refusal now routes to the first unanswered mandatory step
with the reason shown there, so no future mismatch between the UI's
skippable steps and the procedure's required ones can strand anyone.
The server contract is deliberately unchanged: role is still required, and
mark_onboarding_complete still refuses without it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Drumee Dev <drumee@debian.local.drumee>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
Claude encountered an error after 2m 32s —— View job Claude QC Report — in progress
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-pick of #33 (
2df9472) ontopreview, isolated — #34 deliberately excluded.Why now
PROD keeps raising
ER_SIGNAL_EXCEPTION(1644) frommark_onboarding_complete. Measured across theretained prod logs: 65× "Step 3 (role) is incomplete", 39× "Onboarding session not found.
Start at step 1", 12× "User onboarding not found".
These are true positives — the procedure is correct and the users really are being dead-ended at
the last step of onboarding. #33 is the fix, and it has been on
testsince 2026-08-05 withoutreaching
previewormain.Root cause (as #33 describes it)
session_idwas the only write key, and a session is transient — it rotates on re-login, tokenrefresh and expiry. Worse, only
save_onboarding_user_infocould INSERT; every other step was abare
UPDATE, so a single failed or late step 1 wedged the whole wizard permanently.#33 adds a nullable indexed
uidand routes every step throughonboarding_resolve_row, whichresolves in a fixed order: this session's row (stamping
uidas it goes, so rows self-migrate) →else the user's most recent row re-pointed at the current session → else a fresh stub.
Verified on stage before queuing this
Stage already runs #33. I exercised the three cases that matter directly against it:
mark_onboarding_completeon a stubThat last one is the important guarantee: letting any step create a row does not weaken
completion validation, because the stub is inserted with
firstname = ''.Prod DB state
schemas/migrations/alter_onboarding_responses_identity.sqlis already applied to prod(2026-08-11):
uid,tools_otherandidx_uidare present. It was safe to apply early because it isadditive and the currently-deployed code never references those columns — verified inert (all live
read paths still exit 0, old signatures untouched).
Data safety was proven with a bounded checksum rather than
CHECKSUM TABLE(which changes when acolumn is added): an MD5 over only the pre-existing columns for
id <= MAX(id)was identical beforeand after, 1465 → 1465 rows.
MODIFY COLUMN lastname VARCHAR(128) NULL, not purely additive. Onprod
lastnamewas already nullable and its collation matched the table default, so it was a verifiedno-op.
The
save_*signatures gain_uidin position 2, so this is breaking in both directions: oldcode against new procedures fails, and new code against old procedures fails. The procedures and this
plugin must go out together.
Planned order for the window:
preview, thenpreview→main(git only;maindoes not auto-deploy)backfill_tools_other.sqlis legacy data cleanup and will be assessed separately — it is not neededfor the fix.
Why #34 is excluded
#34 (
030ab95, "push the referral row when onboarding completes") is a pure feature — +61/−1 inservice/onboarding.js— that does nothing for this alert. Its dependencies do exist on prod(
referral_live_sockets,referral_members), so it is not risky; it is left out purely to keep abreaking coordinated release minimal and post-deploy diagnosis unambiguous. It should ride the normal
promotion. Consequence: prod's
service/onboarding.jswill lagtestuntil #34 ships.Verified this branch carries #33's version of that file, not #34's, and that all 19 files are
byte-identical to
2df9472.