Skip to content

fix(registration): stop a Swiss trunk-0 phone number from breaking the signature - #910

Open
joshuakrueger-dfx wants to merge 13 commits into
stagingfrom
fix/905-ch-phone-signature-mismatch
Open

fix(registration): stop a Swiss trunk-0 phone number from breaking the signature#910
joshuakrueger-dfx wants to merge 13 commits into
stagingfrom
fix/905-ch-phone-signature-mismatch

Conversation

@joshuakrueger-dfx

@joshuakrueger-dfx joshuakrueger-dfx commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

EN:
The app now composes the registration phone number as canonical E.164 before it signs it, so the signed value is the one the backend accepts and forwards to the share register. A Swiss 079… typed with its customary trunk zero no longer breaks the signature, and neither does a French, British or Dutch one. The canonical form is derived from the same libphonenumber metadata the API uses, not from a hand-kept list of dial codes. Closes #912.

DE:
Die App bildet die Telefonnummer der Registrierung als kanonisches E.164, bevor sie sie signiert — damit ist der signierte Wert der, den das Backend akzeptiert und ans Aktienbuch weiterreicht. Eine Schweizer 079… mit der gewohnten Verkehrsausscheidungsziffer bricht die Signatur nicht mehr, eine französische, britische oder niederländische ebenso wenig. Die kanonische Form stammt aus denselben libphonenumber-Metadaten, die die API benutzt, statt aus einer handgepflegten Vorwahlliste. Schliesst #912.

Details

Why this PR exists

Symptom (verbatim): Invalid signature on POST /v1/realunit/register/complete, reported in #905 from production. A Swiss user completes the whole registration wizard, presses Abschliessen, and loses the submit; nothing is stored. The snackbar blames «your entries» and so points at the tax step the user happens to be looking at — the tax data is a red herring, the signature is verified before any tax check.

Scale: Every user who types their national number the way it is written in their own country. The trunk zero is the customary spelling in roughly sixty numbering plans, Switzerland, Germany, Austria, France, the UK and the Netherlands among them. The field defaults to +41, so the Swiss case is the default path through the registration wizard; the failure is total (registration cannot complete) and misattributes its own cause.

Smaller fix considered: Strip a leading 0 for +41 only — about five lines in updatePhoneNumber, and the first revision of this PR. Insufficient because the same backend rule rejects +330612345678, +4407911123456 and +310612345678 exactly as it rejects +410791234567: a per-dial-code list would have to grow to roughly sixty entries and would be a second, drifting copy of libphonenumber's metadata — the shape CONTRIBUTING.md:112-118 tells us to avoid.

Summary

  • Derives the canonical form from the same libphonenumber metadata the API uses, instead of a hand-kept list of dial codes. Every numbering plan with a national trunk zero is covered, not three.
  • Keeps significant leading zeroes (Italian landlines) untouched, because the metadata says they are significant.
  • Passes the raw value through unchanged while the national part is still incomplete — the app must not gate on validity or length. Only the parse failure is caught; any other exception propagates.
  • Rejects a surviving second leading zero in the field for the main markets, so it cannot advance through the registration form.
  • Applies the same composition to a pre-filled value before any user interaction.
  • Names the leading zero in the error message instead of reusing "Phone number is required".

Why

DFXswiss/backend#5073 stops rewriting signed fields before signature verification and rejects a non-canonical signed phone on /register/complete. It cannot rewrite the payload after verification, because Aktionariat re-verifies the same EIP-712 signature. The only way the share register sees +4179… is if this client composes that form before Eip712Signer.signRegistration.

This is the app half TaprootFreak asked for when reopening on 2026-08-26. The Invalid signature copy stays follow-up work (#911) and is deliberately not part of this revision.

It also fixes #905 on its own. Against the deployed dev API, a registration signed with +410791234567 is rejected at the signature check, while the same registration signed with +41791234567 passes it and is stopped only by a later, unrelated precondition. Today's backend normalises +41791234567 to itself, so a canonical value verifies without any API change — this PR does not wait on backend#5073.

Why metadata instead of a longer list

The first revision stripped the trunk zero for +41, +49 and +43. That leaves every other trunk-zero plan composing a value the backend rejects: +330612345678 (FR) must be +33612345678, +4407911123456 (UK) must be +447911123456, +310612345678 (NL) must be +31612345678. There are roughly sixty such plans; extending the list would have been a second, drifting copy of the metadata.

dlibphonenumber was chosen over phone_numbers_parser because it agrees with the API's libphonenumber-js on every case in a comparison built from libphonenumber's own example numbers (per country the canonical mobile number, and the same number with a zero inserted after the dial code), where phone_numbers_parser diverges on a handful. It adds one transitive dependency (protobuf); fixnum and collection were already present, and the lockfile's sdks: constraint is unchanged.

Scope

Effective diff against staging, fourteen files:

  • lib/widgets/form/phone_number_field.dart
  • test/widgets/form/phone_number_field_test.dart
  • assets/languages/strings_de.arb, strings_en.arb — one new key each
  • pubspec.yaml, pubspec.lock — the new dependency and its transitive protobuf
  • test/goldens/widgets/form/phone_number_field_golden_test.dart and two new baselines, phone_number_field_leading_zero_error{,_en}.png — the error state the new key introduces, in both languages
  • docs/handbook/de/index.html, docs/screens.md — the descriptions this change makes stale
  • test/screens/kyc/steps/kyc_registration_page_test.dart — pins that the canonical value reaches the submit call, not just the widget's notifier
  • test/packages/service/dfx/real_unit_registration_service_happy_test.dart — pins that the signed and the submitted number are the same string, by recovering the signer from the transmitted body
  • test/screens/kyc/steps/registration/cubits/registration_submit/kyc_registration_submit_cubit_test.dart — pins the value across the cubit, the one handover neither neighbouring test covered

No service, signing or generated-artifact changes, and no existing baseline is touched: phone_number_field_default.png is byte-identical to the committed one. The lockfile change is the two added packages, not a range-wide re-resolution.

On the new goldens. They earn their place rather than padding the diff: rendering the error state showed the German message being cut off in the field (Telefonnummer ohne führende …), which the four widget tests could not see — find.text matches the resolved string whether or not it fits the row. Both strings were shortened until they render in full, and the second baseline covers English, so that half is measured rather than assumed. Together they keep the next over-long message from shipping truncated.

On the documentation. Slot 268 still described a prefix dropdown offering +41 or +49, as did the section intro above it and block 237 for the settings screen; #909 replaced that control with a free one-to-three-digit field. Block 268 additionally described an error message that its own screenshot does not show, and claimed the leading zero is always removed — it is not: Italian numbers keep theirs, and so does any number that stripping would leave at an impossible length. These are corrected here rather than deferred to #913 because this PR changes that widget and adds the message in question; the remaining spots #913 lists are untouched.

On Closes #912: that issue was filed by the reviewer against this PR on 2026-08-14 and asks for exactly what this revision does — derive canonicality from the numbering plan instead of stripping unconditionally. The reviewer recorded it as addressed in the 2026-08-20 re-review ("#912 (prefix-agnostic strip) — closed by this head"). It is closed here rather than deferred because the change that resolves it is the change this PR exists for.

Rebased, deliberately

An earlier head of this branch was a merge commit carrying a fix(...) subject line, and it introduced the core logic of the PR rather than any of the seven commits doing so: _canonicalize appeared 0× in both parents and 3× in the merge itself, and the test count went 12 (staging) → 20 → 24 (the merge). staging had never touched the file since the branch point, so the merge should have been a no-op for it. Reading the branch commit by commit therefore showed a history that did not produce the shipped code.

The branch is now four commits on current staging, force-pushed with --force-with-lease; nobody else pushes to this branch.

Known limits, stated rather than hidden

  • A double leading zero is only reported in the field for +41/+49/+43. Canonicalisation does not remove it on its own: for +41 the value is a fixed point (+4100791234567 stays as typed, because no shorter form is a possible length), while for +49/+43 one of the two zeroes is dropped and the second survives (+4900691234567+490691234567). Either way a zero remains directly after the dial code, and that is the only thing _trunkZeroPrefixes still does. For other dial codes the value reaches the API and comes back as a named 400 rather than a field error.

  • A bare 0 as the national part validates. Prefix +41, national part 0 composes to +41 and passes the field, so a naked dial code is signed. The API rejects it with @IsDfxPhone, so it never reaches the share register — but the field claims it is valid.

  • The field shows what was typed, not what is signed — and the field boundary is not preserved. Canonicalisation writes to the shared ValueNotifier and never back into the two text fields, so +41 / 079… on screen submits +41791234567. libphonenumber also receives the two fields already concatenated and may read the boundary differently than the user drew it. Measured with the pinned dlibphonenumber 1.1.70: prefix 410 + 791234567 composes +41791234567, the intended Swiss number — that is the repair this PR exists for. But prefix 4 + 791234567 composes +4791234567, a valid Norwegian number, and prefix 3 + 612345678 composes +3612345678, a valid Hungarian one. The user sees neither, and the KYC correction form asks them to check a value it does not display in its final form.

    This is not a regression. staging builds the same '$prefix$number' string and would submit exactly those values unchanged; canonicalisation leaves them alone because they are already canonical. What the PR changes is only that a non-canonical variant now gets repaired instead of rejected by the API. Closing the gap would mean either validating the dial code against known country codes or writing the canonical value back into the two fields — both are UI changes well beyond a signature fix, and both deserve their own decision.

  • initState now writes to the shared ValueNotifier. None of the three consumers listens to the controller, so this cannot throw today. It becomes setState() called during build the moment someone wraps that controller in a ValueListenableBuilder.

  • Only the parse failure is pinned, not the propagation of anything else. _canonicalize deliberately catches NumberParseException alone, so a defect inside the library surfaces instead of silently yielding the raw value. That the other exceptions propagate has no test: PhoneNumberUtil.instance is a static singleton, so pinning it would mean injecting the parser into the widget — production structure changed for testability, in a PR about a signature. The narrow catch is verified by reading, not by a red test.

  • The canonical form is now computed twice in two codebases. App and API agree on every case measured today; they can drift when either side updates its metadata. Having the API return the value to sign would remove the client copy entirely — that is an API change and not in this PR.

Not built

Review history

The 2026-08-14 approval lapsed with the rebase that followed. The 2026-08-20 re-review asked for an explicit human grant for the named CONTRIBUTING deviation before approving — a client-side allow-list is the shape CONTRIBUTING.md:83-86 and :112-118 warn about. TaprootFreak's reopen of 2026-08-26 states the product rule directly: "a national trunk 0 after +41 is always wrong … This PR is the app half of the fix, not a workaround." This revision also shrinks the deviation: the list no longer decides canonicality, only which markets get a field-level message instead of an API error.

A re-review was requested on 2026-08-27. It has not concluded: two of the reviewer's four lanes reported rejected at that head for vendor reasons ("Codex not logged in … HTTP 401"), which is an unavailable check rather than a finding, and no review verdict has been recorded since.

@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/905-ch-phone-signature-mismatch branch from f0525d5 to aaaffe4 Compare August 14, 2026 14:43
@joshuakrueger-dfx
joshuakrueger-dfx marked this pull request as ready for review August 14, 2026 14:52
@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

@marassteiner please review

This is a PR, not an issue: 15 files against staging, head aaaffe4c, 5/5 checks green.

@marassteiner

Copy link
Copy Markdown

EN: Working on this now — job realunitch__app__910__pr-review.
DE: Ich arbeite jetzt daran — Auftrag realunitch__app__910__pr-review.

@marassteiner marassteiner left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EN: Approve — the trunk-0 strip fixes #905; two follow-ups are tracked in #911 and #912, neither blocks merge.
DE: Approve — der Trunk-0-Strip behebt #905; zwei Follow-ups stehen in #911 und #912 und blockieren den Merge nicht.

Details

Recommendation

Approve. No merge-blocker. CI on aaaffe4c is green (Analyze & Test, Coverage Floor Gate, Visual Regression, BitBox quirks audit, handbook smoke).

Findings

Follow-up — lib/packages/service/dfx/real_unit_registration_service.dart:215

The new exception split matches the free-text API message 'Invalid signature' instead of a structured code. If that wording ever changes, this 400 would again become RegistrationRejectedException and show “check your entries”. Tracked in #911.

Not a blocker: merged today this only stops attributing a known non-content 400 to the user’s entries. A message-text drift would restore the previous generic-failure path; it does not re-break registration. The #905 fix is the trunk-0 strip, not this mapping.

Follow-up — lib/widgets/form/phone_number_field.dart:48

The strip is prefix-agnostic. That is correct for the current ['+41', '+49'] list. Parallel work on feat/open-phone-country-prefix already accepts a free 1–3 digit dial code; combining both without an allow-list would mutate numbers whose leading 0 is significant (e.g. +39). Tracked in #912.

Not a blocker: on the surface this PR ships, CH/DE trunk-0 stripping is the intended #905 fix and is covered by the new widget tests (input, prefix switch, seed).

Local run

Documented CONTRIBUTING path on this host:

  • flutter pub get
  • dart run tool/generate_localization.dart
  • dart run tool/generate_release_info.dart
  • flutter pub run build_runner build --delete-conflicting-outputs
  • flutter analyze on the five changed Dart files — No issues found
  • flutter test test/widgets/form/phone_number_field_test.dart test/packages/service/dfx/real_unit_registration_service_test.dart23/23 passed (includes the new trunk-0 and Invalid signature cases)

A GUI flutter run was not possible on this review host. That is environmental and outside the diff (Case 2):

  • flutter run -d macosError: No macOS desktop project configured (repo has no macos/ tree; this PR does not add one)
  • flutter run -d chrome → compile failed in package:ffi (Only JS interop members may be 'external'); repo has no web/ tree and this PR does not touch web/ffi
  • flutter doctor: no Android SDK; Xcode is Command Line Tools only (simctl unavailable); no iOS Simulator / Android emulator
  • The diff is phone-field normalisation + exception mapping + goldens — no platform runner, plugin, or toolchain change

Comment thread lib/packages/service/dfx/real_unit_registration_service.dart Outdated
Comment thread lib/widgets/form/phone_number_field.dart Outdated
@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/905-ch-phone-signature-mismatch branch 2 times, most recently from 13893c4 to 1955133 Compare August 20, 2026 07:57
@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

Ready after 3 review passes.
Strips a leading trunk zero from the composed phone number only for dial codes whose numbering plan has one, so a Swiss 079… no longer breaks the registration signature and an Italian 06… landline stays intact.

Details

Pass 1 — 6 findings, 3 acted on:

Pass 2 — 4 findings, 3 acted on:

  • national stayed statically String? because mutable fields do not promote through a != null guard; copied into locals with an early return instead.
  • The strip comment claimed the API drops a trunk 0 "only for CH/DE/AT". It does not — it drops it wherever the numbering plan has one, France included. That restriction is this client's decision.
  • Two test gaps of the widening mutation class: nothing distinguished startsWith('0') from contains('0'), and nothing pinned that exactly one leading zero is removed.

Pass 3 — 1 finding, no code finding: the counter-check table in the description undercounted the +41 mutation. The whole table was re-measured at this head rather than recalculated.

Comments: both review threads answered and resolved — #912 fixed by this PR including the +39 preservation test it asks for, #911 left open by design with the decision recorded in-thread.

Mergeability: MERGEABLE at ee8d2f70, no conflicts against staging.

Verification: 34/34 locally with the CI toolchain (Flutter 3.41.6) and the full workflow step order; 4963 tests in CI (4961 before the last two, so the delta matches exactly); flutter analyze clean. Every entry in the trunk-zero list is pinned by its own named test. Golden baselines regenerated on the self-hosted runner; the bot commit was taken over as a signed commit, byte-identical by tree hash.

Not verified: live end-to-end against the deployed API with a real EIP-712 signature, and a device run of the registration wizard.

@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

@marassteiner please review

This is a PR, not an issue: 14 files against staging, head ee8d2f70, 5/5 checks green.

Re-review requested — the branch was rebased onto staging after #909 merged and has changed substantially since your approval on aaaffe4c: 3 new commits, and both of your inline threads now have a reply.

@marassteiner

Copy link
Copy Markdown

EN: Working on this now — job realunitch__app__910__pr-review__r2.
DE: Ich arbeite jetzt daran — Auftrag realunitch__app__910__pr-review__r2.

@marassteiner marassteiner left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EN: Re-review of ee8d2f70 after the staging rebase: the scoped trunk-0 strip and tests look correct, #912 is addressed and #911 stays open, but the named CONTRIBUTING workaround still needs an explicit human grant before I can approve.
DE: Re-Review von ee8d2f70 nach dem Staging-Rebase: der begrenzte Trunk-0-Strip und die Tests sind korrekt, #912 ist erledigt und #911 bleibt offen, aber die benannte CONTRIBUTING-Abweichung braucht noch eine ausdrückliche menschliche Freigabe, bevor ich approven kann.

Details

Recommendation

Comment, not approve. The rebase onto staging after #909, the allow-list, and the new tests address the previous follow-up. I am not re-approving while the PR-declared CONTRIBUTING workaround (_trunkZeroPrefixes) is still ungranted.

Previous approval was on aaaffe4c and does not apply to this head.

What changed since aaaffe4c

  • After #909 replaced the +41/+49 dropdown with a free 1–3 digit prefix field, a prefix-agnostic strip would have mutated Italian landlines (leading 0 is significant). The strip is now limited to _trunkZeroPrefixes (+41, +49, +43). +423 is excluded: Liechtenstein has no national trunk 0.
  • Same strip on seed (initState), so a pre-filled legacy +41079… is rewritten before sign/submit.
  • Widget tests pin startsWith('0') vs contains('0'), exactly one leading zero, each list entry, +39 preservation, +423 preservation, prefix switch, and seed.
  • Invalid signature 400 still stays a plain ApiException (not RegistrationRejectedException). Unchanged from the last review; still free-text. #911 remains open by design.
  • Goldens regenerated; Visual Regression is green on this head.

Follow-ups from the last review

  • #912 (prefix-agnostic strip) — closed by this head. The +39 preservation test is present.
  • #911 (structured INVALID_SIGNATURE code) — still not in this PR. Failure mode if the API message text changes: generic error instead of “check your entries”; registration is not blocked. Thread already records that decision.

CONTRIBUTING

The PR body names _trunkZeroPrefixes as a deviation from “the API decides” / “do not add app-side workarounds”. The list does not reject any number (validators stay format-only). It only chooses whether to drop a trunk 0 so the signed bytes match the API’s E.164 form for CH/DE/AT.

That is still client-side numbering-plan knowledge. The fully compliant fix is DFXswiss/api#5073 (verify the signature against the submitted bytes, then normalize). Until a human maintainer grants this named workaround — or #5073 lands and the list is deleted — this review does not approve.

Residual, documented, not a regression of this head: trunk-0 countries outside the list (FR, UK, NL) still hit Invalid signature if the user types a leading 0. Lengthening the client list would copy libphonenumber into the app.

Checks on ee8d2f70

  • Analyze & Test — pass
  • Coverage Floor Gate — pass
  • Visual Regression — pass
  • BitBox quirks audit — pass
  • Build handbook image + container smoke — pass
  • Maestro handbook flows — skipped (expected without tier3:full / not targeting main)

Not re-run locally in this pass: flutter test / flutter analyze / a device wizard. CI on this head is the evidence.

@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

Closing in favour of DFXswiss/api#5073.

Why

The root cause is server-side: register/complete normalizes phoneNumber to E.164 before verifying the EIP-712 signature, so any client that signs a non-canonical form fails verification. api#5073 removes that transform and verifies against the exact submitted bytes.

That fixes the problem for every country and every client at once. This PR fixed it app-side for three dial codes (+41, +49, +43) via _trunkZeroPrefixes — France, UK and the Netherlands would have stayed broken, and the list would have needed maintenance as a second, drifting copy of libphonenumber's data.

CONTRIBUTING is explicit about this shape of fix:

When the API doesn't yet expose what we need

Extend the API, then change the app. Do not add app-side workarounds. … Temporary local logic is technical debt that stays.

Audit

Current violations of this rule are tracked internally. New PRs must not add to that list.

A server fix also reaches users who have not updated the app; an app fix does not.

Not covered by api#5073 — worth a small follow-up PR

Two changes in this branch are independent of the signature problem and are lost by closing:

  1. Hint text '1231234567''791234567' (phone_number_field.dart). The old hint is not a Swiss number shape, which is part of why users typed the trunk 0 in the first place.
  2. Invalid signature error mapping (real_unit_registration_service.dart). Today that 400 becomes RegistrationRejectedException, which renders "check your entries" — pointing the user at data that is correct. This is proposal 4 of Registration complete: +41 plus a leading-0 national number fails with Invalid signature #905 and stays wrong even after api#5073, for any other cause of a signature mismatch.

Both are small and carry tests on this branch.

State of this branch

fix/905-ch-phone-signature-mismatch is left in place at ee8d2f70 — rebased onto staging, 4963 tests green in CI, goldens regenerated. Anything worth keeping can be cherry-picked from there.

Issues

@TaprootFreak

Copy link
Copy Markdown
Contributor

EN:
Reopen this: the app must compose E.164 before signing, because a leading 0 after +41 is never a valid number.

DE:
Wieder öffnen: die App muss vor dem Signieren E.164 bilden, denn eine führende 0 nach +41 ist nie eine gültige Nummer.

Details

Product rule: a national trunk 0 after +41 is always wrong. The signed registration must carry E.164 (+41791234567). +410791234567 must not be signed, submitted, or stored.

Closing this in favour of the API-only change does not hold. The API can stop rewriting signed fields before verification, but it cannot rewrite the payload afterwards: the share register re-verifies the same EIP-712 signature. The only way the share register sees +4179… is if this client composes that form before Eip712Signer.signRegistration.

What to keep from this branch: strip a leading trunk 0 when composing (+41 / +49 / +43; keep the Italian leading 0). Hint and Invalid signature copy can stay in a follow-up; they do not block this.

#905 stays the report. This PR is the app half of the fix, not a workaround.

@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/905-ch-phone-signature-mismatch branch from 51e0d11 to de756e8 Compare August 27, 2026 09:31
@joshuakrueger-dfx joshuakrueger-dfx added the tier3:full Opt-in: run Tier 3 Maestro handbook flows on this PR label Aug 27, 2026
@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

@marassteiner please re-review — the branch changed substantially since your last pass on ee8d2f70.

This is a PR, not an issue: 6 files against staging, head 7a433200, 7/7 checks green, none skipped.

What changed since your 2026-08-20 comment:

  • Your blocking point is answered. You asked for an explicit human grant for the named CONTRIBUTING deviation before approving. TaprootFreak gave it when reopening on 2026-08-26: "a national trunk 0 after +41 is always wrong … This PR is the app half of the fix, not a workaround."
  • The deviation is also smaller now. The dial-code list no longer decides canonicality. The app derives the canonical form from the same libphonenumber metadata the API uses (dlibphonenumber), measured at 490 of 490 agreement with the API's libphonenumber-js 1.12.25 across 245 countries. _trunkZeroPrefixes survives only to name a double leading zero in the field for the main markets.
  • Scope PhoneNumberField trunk-0 strip to prefixes that use a national trunk zero #912 is closed by this PR, and the French, British and Dutch cases it hinted at are covered rather than deferred.
  • The history was rebased. The previous head was a merge commit that introduced the core logic itself; the branch is now four signed commits on current staging.

Counter-checks on this head with the pinned Flutter 3.41.6: 28/28 pass, 12 of 28 fail without the fix, 5 fail when canonicalisation is rolled back to the dial-code list, 2 fail when the new error key is reverted, 1 fails when the parse fallback is broken.

Known limits, the bare-0 edge and the metadata-drift risk are in the PR body under Known limits — none of them blocks, but I would rather you see them than not.

@marassteiner

Copy link
Copy Markdown

EN: Working on this now — job realunitch__app__910__pr-review__r3.
DE: Ich arbeite jetzt daran — Auftrag realunitch__app__910__pr-review__r3.

@marassteiner

Copy link
Copy Markdown

codex-pr / quality (codex) rejected at 7a433200efabf76381c0ce8ed3d1d1f5beb52f39

Vendor cannot run. Official lane: agent run --spec-file STATUS=unavailable rc=1. Direct: codex login status = Not logged in (rc=1); no auth.json. Direct codex exec: HTTP 401 Unauthorized on wss://api.openai.com/v1/responses. No approved recorded. No vendor substitute.

@marassteiner

Copy link
Copy Markdown

codex-pr / logic (codex) rejected at 7a433200efabf76381c0ce8ed3d1d1f5beb52f39

Vendor cannot run. Codex not logged in (no auth.json). Official grok-pr both approved on 7a43320; codex-pr/logic cannot start. Official sibling lane (codex-pr/quality) STATUS=unavailable rc=1; direct exec HTTP 401 Unauthorized. No approved recorded. No vendor substitute.

…ation

The registration phone number was composed exactly as typed, so the customary
Swiss trunk-zero spelling (prefix `+41`, national part `079…`) was EIP-712
signed as `+410791234567`. The server normalises the field to E.164 before it
verifies the signature, so verification ran over different bytes than the
client signed and a valid submission was rejected.

Compose the canonical value in the field instead:

- Remove exactly one national trunk zero, and only for the dial codes whose
  numbering plan has one (`+41`, `+49`, `+43`).
- Match on the fully composed value rather than per field, so the result is
  the same however the digits are split between the free-form dial-code field
  and the national part.
- Apply the same composition to a pre-filled value before any user
  interaction.
- Reject a residual `+410…`, `+490…` or `+430…` so it cannot advance through
  the form.

Italian and Liechtenstein leading zeroes are significant and stay untouched;
`+423` is deliberately absent from the trunk-zero list because Liechtenstein
has no national trunk zero.
The trunk-zero rejection returned `registerPhoneNumberInvalid`, which reads
"Phone number is required" / "Telefonnummer ist erforderlich" — the same
string the empty-field branch twelve lines above uses. Someone who typed
`00791234567` therefore saw "is required" over a visibly filled field, with
no hint that the leading zero was the cause.

Add `registerPhoneNumberLeadingZero` and use it for that branch only; the
empty-field branch keeps the old key.

The two rejection tests now assert the new string. That also sharpens them:
while both branches returned the same text, neither test could tell a
trunk-zero rejection from an empty-field one.
…metadata

The trunk-zero strip was driven by a hand-kept list of three dial codes, so
every other numbering plan with a national trunk zero still composed a value
the backend rejects. Measured with `libphonenumber-js` 1.12.25, the version
the API resolves: `+330612345678` must be `+33612345678`, `+4407911123456`
must be `+447911123456`, `+310612345678` must be `+31612345678`.

Derive the canonical form from the same metadata the API uses instead, via
`dlibphonenumber`. Measured over 490 cases built from libphonenumber's own
example numbers across 245 countries — the canonical mobile number per
country and the same number with a zero inserted after the dial code — the
package agrees with `libphonenumber-js` on 490 of 490. `phone_numbers_parser`
was measured too and differs in 5 of those cases, which is why it is not the
one used here.

`parse` throws while the national part is still being typed, so the raw value
is passed through unchanged; the app must not gate on validity or length.

`_trunkZeroPrefixes` stays, but only to report a surviving second leading
zero in the field rather than letting it become a 400: `+4100791234567` is a
fixed point of the canonicalisation, so the metadata alone does not catch it.
`_canonicalize` caught every exception and returned the raw value. The
comment named the one case that is expected — incomplete input while the
national part is still being typed — but the code also swallowed a genuine
defect in the metadata library, which would then reach the API as a 400
instead of failing visibly.

Narrow it to `NumberParseException`, the type measured for `+41`, `+417`,
`+410`, `+49` and `+423`. Anything else now propagates. The incomplete-input
test stays green, which is what pins that this is the type actually thrown.
The double-zero guard listed +41, +49 and +43, but only +41 was covered by a
test: removing the other two entries left the suite green. Adds a case for each.

The Liechtenstein test claimed LI has no national trunk 0. It has one — the
zero is stripped as soon as the result has a possible length. The test only
passed because its number was too short for that, so the comment described a
rule that does not exist. Corrects it and pins the actual behaviour.

Also moves the dlibphonenumber import behind the flutter imports, per the
import order in CONTRIBUTING.md.
@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/905-ch-phone-signature-mismatch branch from 7a43320 to 1ad452d Compare September 2, 2026 15:46
The new message was cut off in the field: German rendered as "Telefonnummer
ohne fuehrende ...", and the English string was longer still. The four widget
tests could not see it — find.text matches the resolved string whether or not
it fits on screen.

Shortens both strings and pins the error state with a golden, so the next
message that outgrows the row fails a check instead of shipping truncated.
Block 268 still described a prefix dropdown offering +41 or +49. #909 replaced
that with a free field taking one to three digits. Also names the leading-zero
message, which this branch introduces.
@joshuakrueger-dfx
joshuakrueger-dfx marked this pull request as draft September 3, 2026 09:00
@joshuakrueger-dfx
joshuakrueger-dfx marked this pull request as ready for review September 3, 2026 09:02
The golden only covered German, so the English string was shortened on an
estimate rather than a measurement. wrapForGolden already takes a locale;
this pins both. Each test now holds its own form key instead of sharing one.
The section intro still called the prefix a selection, and block 237 carried
the same wording #909 made obsolete. Block 268 described an error message its
own screenshot does not show, and stated the leading zero is always removed:
it stays for Italy, and whenever stripping would leave an impossible length.
screens.md counted one baseline under the shared-widget path; there are three,
of which one carries the handbook slot.
…call

Every test so far stopped at the widget's ValueNotifier. The submit tests
captured phoneNumber as any(), so replacing the notifier with a raw value in
kyc_registration_page would have gone unnoticed and quietly undone the fix.

Types a Swiss number with its trunk zero into the page and asserts the cubit
receives exactly +41791234567. Also drops two documentation claims that this
branch made untrue: screens.md said it covers every tested state variant while
the same file now lists two unmapped error baselines, and the handbook made the
leading-zero rule depend on the resulting length, which is not how the metadata
decides.
…one string

This is the sentence the whole PR exists for, and nothing pinned it. The page
test stops at a mocked cubit, and the service tests only checked the signature's
length — so appending a space to the value handed to the signer would have gone
unnoticed while the request still carried the correct number. That is exactly
the failure #905 reports.

Signs with a fixed key, rebuilds the EIP-712 message from the transmitted body
and recovers the signer. It only matches if both sides hung on the same bytes.
@joshuakrueger-dfx
joshuakrueger-dfx marked this pull request as draft September 4, 2026 19:54
… match

The signature test inherited this file's fixture, `+41 79 000 00 00` — which is
not canonical E.164. It proved signed and sent bytes were equal, and would have
stayed green if no canonicalisation had happened at all, so it covered half of
what it was written for.

Uses the canonical number in that one case and asserts the transmitted value
literally; the other tests keep the old fixture through a defaulted parameter.
Also moves the eth_sig_util_plus import behind the flutter ones, per the import
order in CONTRIBUTING.md.
The value travels widget to page to cubit to service. The page test stops at a
mocked cubit and the service test starts at the service, so the cubit itself
was the one station where the number could change unnoticed: appending a space
where it builds the Registration would have kept both neighbouring tests green.

The existing cubit test already captures that object; this asserts its phone
number is exactly the canonical string.
@joshuakrueger-dfx
joshuakrueger-dfx marked this pull request as ready for review September 5, 2026 15:24
@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

EN:
Ready after 5 review passes.
The app composes the registration phone number as canonical E.164 before signing it, so the signed value is the one the backend accepts and forwards to the share register.

DE:
Bereit nach 5 Review-Durchläufen.
Die App bildet die Telefonnummer der Registrierung als kanonisches E.164, bevor sie sie signiert, damit der signierte Wert der ist, den das Backend akzeptiert und ans Aktienbuch weiterreicht.

Details

Review passes

Pass 1 — 7 findings, 3 acted on:

  • docs/screens.md claimed to cover every tested state variant while the same file now lists two error baselines that carry no handbook slot.
  • The handbook made the leading-zero rule depend on the resulting length; that is not how the metadata decides.
  • No test showed the canonical value reaching the submit call — every test stopped at the widget's notifier.
  • Rejected with evidence: the API-authority objection (the deviation carries an explicit maintainer grant from the 2026-08-26 reopen) and the text-scale objection — rendered at scale 3.0, the existing message is cut off exactly as the new one is, so it is a property of the shared field, not of this change.

Pass 2 — 1 finding, acted on: nothing pinned the sentence this PR exists for. Appending a space to the value handed to the signer would have kept the request correct while the signature hung on different bytes — precisely the failure #905 reports. Now pinned by rebuilding the EIP-712 message from the transmitted body and recovering the signer.

Pass 3 — 2 findings, both acted on: that new test used this file's fixture +41 79 000 00 00, which is not canonical, so it proved equality but not canonicality; and an import sat before the flutter group.

Pass 4 — 1 finding, acted on: the value travels widget → page → cubit → service, and the cubit was the one handover neither neighbouring test covered.

Pass 5 — 0 findings acted on. Two repeat objections were rejected with evidence and are named in the description: the field-boundary reinterpretation (measured as not a regression — staging composes the identical string) and the untested propagation of non-parse exceptions (pinning it would mean injecting the parser into the widget, i.e. changing production structure for testability).

Comments

All four sources re-read at the final head: both review threads resolved, no CHANGES_REQUESTED, nothing new since 2026-08-27. The two entries from that date report that the reviewer's own vendor lanes could not start — an unavailable checker, not a finding.

Mergeability and checks

mergeable: MERGEABLE at 4022a3f8. Six checks green at that head. The seventh, BitBox02 simulator (real firmware), was skipped by its draft guard while the PR was a draft and does not re-trigger on ready_for_review; it was re-run via workflow_dispatch on the same head and passed with the full job executed — the skipped run had run 0 steps, the replacement ran checkout plus the pinned testkit action.

@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

@marassteiner please re-review — head 4022a3f8.

EN: Your last pass was on 7a433200; two of your four lanes could not start then (codex-pr vendor unavailable), so no verdict was recorded. The branch has changed substantially since: rebased onto current staging, and the canonicalisation is now pinned across the whole path — widget, page, cubit and service — after seven review passes. 7 of 7 checks green, MERGEABLE.

DE: Dein letzter Durchgang lief auf 7a433200; zwei deiner vier Lanes konnten damals nicht starten (codex-pr vendor unavailable), es wurde also kein Verdikt erfasst. Der Branch hat sich seither deutlich geändert: auf aktuelles staging gerebased, und die Kanonisierung ist jetzt über den ganzen Pfad gepinnt — Widget, Page, Cubit und Service — nach sieben Review-Durchläufen. 7 von 7 Checks grün, MERGEABLE.

@marassteiner

marassteiner commented Sep 7, 2026

Copy link
Copy Markdown

Job realunitch__app__910__pr-review__r4: Der Lauf ist mit Fehlercode 1 beendet worden; es wurde nichts veröffentlicht.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tier3:full Opt-in: run Tier 3 Maestro handbook flows on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants