fix(pair-code): canonicalize companion_platform_display OS to a server-safe set - #979
Conversation
…r-safe set
The pair-code `companion_hello` server rejects a non-OS `companion_platform_display`
with `bad-request` — so a consumer that sets `DeviceProps::os` to a branding string
(e.g. "Veloz") could never pair by code, even though QR pairing tolerates it (QR
never sends this field). Verified live against the WhatsApp server.
WA Web only ever emits a real OS name from its UA parser (`WAWebBrowserInfo().os`).
Model that as a small, closed `CompanionOs` enum (Windows, Mac OS, Linux, Android,
iOS) with `wire_str()` (canonical ua-parser spelling) and `from_hint()` (fuzzy,
case-insensitive parse of the free-form os; unrecognized/branding/empty -> Linux,
the universally-accepted default). `companion_platform_display` now routes the OS
through it, so it can never emit a server-rejected string. Kept deliberately
conservative: the server is lenient toward real OS names today, but collapsing to a
guaranteed-accepted set is robust against that changing (iPad folds to iOS since
"iPadOS" isn't a confirmed-accepted value).
This is scoped to pair-code; QR pairing does not send this field, so its branding
path is untouched. Also:
- corrects the false doc ("server validates only length; no whitelist") that caused
the passthrough,
- warns (once, at pair time) when a branding os is coerced, so a consumer sees why
it didn't ride through,
- documents that a pair-code `bad-request` can be rate-limiting (throttled per phone
number), not invalid input — back off rather than treating every 400 as fatal.
Tests: `CompanionOs` wire/classify/from_hint table + "Veloz"->Linux regression; the
two os="Mac" display tests updated to the canonical "Mac OS". wacore companion_reg
(22) + pair_code (40) and whatsapp-rust pair_code (11) green; fmt/clippy clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthrough
ChangesCompanionOs Canonicalization
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pair_code.rs`:
- Around line 206-214: The warning in pair_with_code is emitted on every call
instead of being one-time, so update the OS coercion path in src/pair_code.rs to
gate the warn! behind a shared one-shot guard (for example a static Once or
equivalent flag) or otherwise lower its verbosity. Keep the check tied to the
existing CompanionOs::classify and device_snapshot.device_props.os handling so
unrecognized branding OS values only produce the coercion warning once across
retries, matching the documented behavior.
In `@wacore/src/companion_reg.rs`:
- Around line 139-146: CompanionOs is still using a manual wire-string path
instead of the generated wire enum support. Update the CompanionOs enum to
derive WireEnum and move the wire labels onto the variants so the generated
as_str() can replace the current wire_str() usage. Keep classify() as the
separate fuzzy parser for free-form OS hints, and adjust any CompanionOs call
sites to use the generated string accessor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 648d8395-7515-4774-8721-d7fe3299930f
📒 Files selected for processing (2)
src/pair_code.rswacore/src/companion_reg.rs
Review follow-up (#979): the classify Linux branch matched "chrome os"/"cros"/ "chromium" but not "chromeos" — Google's official no-space styling — so classify("ChromeOS") fell to None and fired a spurious branding-coercion warning (the wire value was already correctly Linux). Add "chromeos" and a test case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Modifies core pairing logic by introducing a canonical OS enum with fuzzy classification. Although targeted, this is a business logic change in a critical code path that could affect the behavior of pair-code linking for unrecognized OS strings.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 4/5
- In
wacore/src/companion_reg.rs, using a rawcontains("ios")check can misclassify non-iOS hints (for exampleKaiOS) as iOS, which may skip the intended unknown→Linux fallback and produce incorrect platform handling at runtime — tighten the match to a tokenized or anchored iOS check before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review follow-ups (#979): - The branding-OS coercion warn in pair_with_code fired on every call, so a rate-limited caller retrying (as the PairError::RequestFailed doc suggests) would get identical spam. Gate it behind a process-level `Once`, keeping it at WARN (discoverable) without repeating. - CompanionOs::classify used a bare `contains("ios")`, so "KaiOS" false-matched as iOS and skipped the unknown->Linux fallback. Match "ios" as a whole word instead. Added tests: "KaiOS" -> Linux, "iOS 17" -> iOS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Auto-approved: Canonicalizes a free-form OS string to a small server-safe set for pair-code linking. No business logic, infrastructure, or API changes; confined to a single enum in wacore. The companion_platform_display path is isolated to pair-code only (QR is unaffected).
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
The OS canonicalization is safe-by-default but had no escape hatch: a caller on Ubuntu/Fedora couldn't show their real distro even though the server accepts it (it only rejects non-OS branding). Add `PairCodeOptions::display_os: Option<String>` — when set (non-empty), the OS is sent verbatim, bypassing the canonical coercion; None keeps the safe default. Complements the existing `platform_id` (browser) override so an advanced caller controls both parts of "Browser (OS)". Documented as at-the-caller's-risk (the server rejects a non-OS string with bad-request); an all-whitespace value is ignored (falls back to coercion so we never emit an empty OS). The branding-coercion warn is skipped when the override is active. Splits out companion_platform_display_raw (verbatim formatter) so the default coerced path and the override share the "Browser (OS)" shape. Tests: verbatim override, override beats branding props.os, whitespace falls back. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Auto-approved: Adds OS canonicalization for pair-code, fixing server rejection of non-OS display strings. Well-tested, low-risk, and includes an opt-out override.
Re-trigger cubic
Branding strings like "March"/"Search"/"across" no longer false-match
the short Linux aliases via substring; they now stay unclassified and
default to Linux through the coercion fallback (surfacing the warn), same
wire value either way. Distinctive names ("linux"/"ubuntu"/"chromeos"/…)
keep substring matching; bare "Arch"/"CrOS" match whole-word.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: New canonical OS enum with classifying logic, nonce comparison, and warnings introduces risk of WhatsApp server rejection or subtle bugs in core pairing flow.
Re-trigger cubic
Trim the 8-line block to the "why" per AGENTS.md style; behavior unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EdZLyCWkYyEwPtgtdE6DFo
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Adds CompanionOs enum and updates wire format and error model; these core changes can break pairing and warrant domain-specific review.
Re-trigger cubic
What
Fixes a second, orthogonal blocker for phone-number pair-code linking, found by testing live against the WhatsApp server (a pre-auth
companion_helloprobe comparing byte-for-byte with a working WhatsApp Web capture). Follow-up to #976 (the nonce fix).The pair-code
companion_helloserver rejects a non-OScompanion_platform_displaywithbad-request. The lib derived that field's OS fromDeviceProps::os, which a consumer may set to an arbitrary branding string — so e.g.os = "Veloz"produced"Chrome (Veloz)"and pairing failed with a persistent 400. QR pairing is unaffected: it never sends this field, so it tolerates arbitrary branding.Empirically confirmed: with the #976 nonce fix alone (
os = "Veloz") →bad-request; changing only the OS to a real value ("Linux") → HTTP 200, code emitted, primary phone received the notification.The fix
WA Web only ever emits a real OS name from its UA parser (
WAWebBrowserInfo().os→ua-parser-jsgetOS().name). Model that as a small, closedCompanionOsenum inwacore/src/companion_reg.rs:wire_str()→ the canonical ua-parser spelling ("Windows","Mac OS","Linux","Android","iOS").classify()→ fuzzy, case-insensitive parse. Distinctive names match by substring; short ambiguous tokens ("ios","arch","cros") match whole-word so branding like"KaiOS","March","Search","across"doesn't false-match. Chrome OS / distros fold to Linux;from_hint()= classify-or-Linux.companion_platform_displayroutes the OS throughCompanionOs::from_hint(os).wire_str(), so the default path can never emit a server-rejected string.Kept deliberately conservative. Live testing showed the server is currently lenient (it also accepts
Ubuntu,Fedora,Mac,macOSverbatim), but collapsing to a small guaranteed-accepted set is robust against that leniency changing.iPadfolds toiOS, since older UA parsers report iPad as"iOS"and"iPadOS"isn't a confirmed-accepted value.Flexibility: opt-in OS override
Because the default coercion drops real-but-non-canonical names the server does accept (e.g.
Ubuntu), there's an escape hatch for advanced callers:PairCodeOptions::display_os: Option<String>. When set (non-empty) the OS is sent verbatim, bypassing coercion;Nonekeeps the safe default. It complements the existingplatform_id(browser) override, so a caller controls both parts ofBrowser (OS). Documented as at-the-caller's-risk (a non-OS string is rejected withbad-request); an all-whitespace value is ignored so we never emit an empty OS.Error-handling + docs (no speculative behavior)
osis coerced, so a consumer sees why their branding didn't ride through; skipped whendisplay_osoverrides.bad-request(400) can be rate-limiting (throttled per phone number), not invalid input — indistinguishable in the response, so back off and retry rather than treat every 400 as fatal. No auto-retry and no hardcoded threshold (both server-tunable / unreliable to detect); any serverbackoffhint is preserved on the wrappedIqError.Scope
Pair-code only. QR pairing does not send
companion_platform_display, so its branding path (DeviceProps::os) is untouched, and QR-only consumers are unaffected.Testing
CompanionOs:wire_strcanonical spellings, aclassify/from_hintalias table (Windows/Mac/darwin/Ubuntu/Fedora/Arch/Chrome OS(+no-space)/CrOS/Android/iOS/iPad…), the"Veloz"→Linuxregression, and the whole-word guards —"KaiOS","March","Search","across"all stay unclassified → Linux via fallback (never via a false substring match).display_os, override beats a brandingprops.os, all-whitespace falls back to coercion.os = "Mac"display tests updated to canonical"Mac OS".cargo test -p wacore --lib companion_reg(22) +pair_code,cargo test -p whatsapp-rust --lib pair_codegreen;cargo fmt --all --checkandcargo clippy -p wacore -p whatsapp-rust --testsclean.Not in scope
Whether the persistent linked-device name on the phone comes from this field or from post-auth
DeviceProps(which would decide if a dedicated branding field is worth adding) is left for a follow-up once probed — the coercion + opt-in override is the correct, safe behavior regardless.