Skip to content

fix(pair): WA Web compliant pairing QR + companion_platform_{id,display}#593

Merged
jlucaso1 merged 8 commits intomainfrom
fix-pair-qr-client-type
Apr 25, 2026
Merged

fix(pair): WA Web compliant pairing QR + companion_platform_{id,display}#593
jlucaso1 merged 8 commits intomainfrom
fix-pair-qr-client-type

Conversation

@jlucaso1
Copy link
Copy Markdown
Collaborator

@jlucaso1 jlucaso1 commented Apr 25, 2026

Summary

Two bugs in the pairing flows, fixed together because they share a derivation source.

QR pairing format (whatsmeow#1110, Baileys#2460 backport)

Primary devices started rejecting the legacy 4-field QR. WA Web's WAWebLinkDeviceQrcode.react now appends WAWebCompanionRegClientUtils.DEVICE_PLATFORM as a 5th field. Mirror that.

  • New CompanionWebClientType enum, 10 wire values matching WA Web exactly, plus Unrecognized(i32) fallback. Derived via WireEnum int (same pattern as TempBanReason / ConnectFailureReason).
  • make_qr_data auto-derives the client type from device_props.platform_type. make_qr_data_with_client_type is the override seam.
  • parse_qr_code accepts the legacy 4-field form, the new 5-field form, the native-camera https://wa.me/settings/linked_devices# prefix, and the jknob-770 FAQ URL suffix. Empty fields, malformed base64, and wrong key lengths still rejected.

companion_platform_{id,display} correctness (regression from 63210f7)

The pair-code IQ derivation introduced in 63210f7 had two wire-correctness bugs surfaced during the audit:

  1. companion_platform_id was the wrong enum's integer. WA Web sends WAWebCompanionRegClientUtils.DEVICE_PLATFORM (CompanionWebClientType, 10 values) per docs/captured-js/WAWeb/Alt/DeviceLinkingIq.js:35. We were formatting wa::device_props::PlatformType as i32 (proto, 25+ values). The two enums collide on indices 1 and 7 only. Firefox sent "2" instead of "3", Edge "6" instead of "2", IE "3" instead of "4", and mobile DeviceProps sent values like "16" outside the WA Web range entirely.
  2. companion_platform_display violated server validation. whatsmeow's PairPhone doc states the server validates this field strictly as Browser (OS) with browser ∈ {Chrome, Edge, Firefox, IE, Opera, Safari} and returns 400 otherwise. We were emitting Android (Android), iPhone (iOS), iPad (iPadOS) for mobile DeviceProps, breaking pair-code for any non-browser platform.

Fixes:

  • derive_companion_platform returns (CompanionWebClientType, String) and shares its derivation with the QR flow. Single source of truth.
  • companion_browser_name returns one of the 6 server-valid browsers, falling back to Chrome for non-browser variants. That's what WA Web's info().name emits even from Electron contexts.
  • companion_platform_display always emits <Browser> (<OS>), substituting Linux for empty OS since WA Web never sends bare browser names.

Breaking changes (pre-1.0, intentional)

  • wacore::pair_code::derive_companion_platform: (String, String)(CompanionWebClientType, String)
  • wacore::pair_code::resolve_companion_platform: same return-type change
  • wacore::pair_code::PairCodeOptions::platform_id: Option<wa::device_props::PlatformType>Option<CompanionWebClientType>
  • wacore::pair_code::PairCodeOptions::platform_display: removed (WA Web has no equivalent override; any free string risks server 400)
  • wacore::pair_code::platform_friendly_name: removed (only caller was the buggy derivation; the mixed browser/non-browser name table was the bug 2 vector)
  • wacore::pair::PairUtils::make_qr_data: now requires CompanionWebClientType parameter

The whatsapp-rust-bridge consumer only uses PairCodeOptions { phone_number, custom_code, ..Default::default() } and reads the QR string via Event::PairingQrCode. Both work transparently with the new behaviour.

Test plan

  • cargo test -p wacore --lib: 610 tests pass, including
    • 17 companion_reg tests: wire codes match WA Web, From<i32> round-trip, Unrecognized fallback, mapping totality, browser-name validity, display invariants
    • 14 pair::tests: QR 5-field format, retro-compat 4-field parse, native-camera prefix, FAQ suffix, round-trip auto-derive for every client type
    • rewritten pair_code::tests: per-browser correct wire, mobile → OtherWebClient + Chrome display fallback, total proto-PlatformType scan asserting display always uses one of 6 valid browsers, pair-code/QR id parity, override semantics
  • cargo clippy --workspace --exclude e2e-tests --tests clean
  • cargo fmt --all -- --check
  • Live pairing smoke against a real primary device, both QR (default Chrome/Linux + AndroidPhone DeviceProps) and pair-code (mobile DeviceProps to confirm the previous 400 path now works)

Primary devices started rejecting the legacy 4-field QR payload
(tulir/whatsmeow#1110, WhiskeySockets/Baileys#2460); WA Web's
WAWebLinkDeviceQrcode.react now emits a 5th field carrying
WAWebCompanionRegClientUtils.DEVICE_PLATFORM. Mirror that:

- New `CompanionWebClientType` enum with `#[repr(i32)]` discriminants
  pinned to the WA Web wire values.
- `companion_web_client_type_for_props` derives the QR client type
  from DeviceProps.PlatformType, falling through to OtherWebClient
  for non-web companions like WA Web does for unknown browser names.
- `make_qr_data` now appends the client type; the high-level wrapper
  auto-derives from `device_props` so callers don't have to wire it
  through. `make_qr_data_with_client_type` is available for overrides.
- `parse_qr_code` accepts legacy 4-field, current 5-field, the native-
  camera `https://wa.me/settings/linked_devices#` prefix, and the
  jknob-770 FAQ URL suffix; still rejects empty fields, malformed
  base64, and wrong key lengths.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 25, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

QR/pair flows now include a typed CompanionWebClientType derived from DeviceProps. A new public companion_reg module defines the enum and helpers; QR generation, parsing, and pair-code platform derivation were updated to emit, consume, and propagate the client-type across pairing flows.

Changes

Cohort / File(s) Summary
New Client Type Module
wacore/src/companion_reg.rs
Adds CompanionWebClientType enum (explicit i32 codes), companion_browser_name, companion_web_client_type_for_platform, companion_web_client_type_for_props, and companion_platform_display with unit tests.
Public Export
wacore/src/lib.rs
Exports new companion_reg module (pub mod companion_reg;).
Pairing QR: core
wacore/src/pair.rs
PairUtils::make_qr_data now takes ref_str: &str and an explicit CompanionWebClientType and emits 5-field payload ref,noise,identity,adv,client_type. PairUtils::parse_qr_code relaxed to accept legacy 4-field and new 5+ field shapes, tolerates optional native-camera prefix and trailing suffix, validates non-empty fields, and surfaces field-specific base64/key errors. Tests added/updated.
Pair-code platform derivation
wacore/src/pair_code.rs
derive_companion_platform / resolve_companion_platform now return (CompanionWebClientType, String); PairCodeOptions.platform_id changed to Option<CompanionWebClientType> and platform_display removed. Tests updated to new wire/display expectations.
Application integration
src/...
src/pair.rs, src/pair_code.rs, src/pair_code.rs
Call sites updated to compute CompanionWebClientType from device_props and pass it into QR builders (make_qr_data_with_client_type / PairUtils::make_qr_data). Minor comment/format cleanup.
Tests / Assertions
wacore/src/... (tests)
Expanded unit tests covering client-type suffix emission, parsing compatibility (prefix/suffix, 4 vs 5 fields), invalid inputs, key-length checks, and QR generation/parsing round-trips per client type.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Device as Device (DeviceProps)
  participant Reg as companion_reg
  participant App as App / PairUtils
  participant QR as QR Image
  participant Scanner as Mobile Scanner

  Device->>Reg: companion_web_client_type_for_props(props)
  Reg-->>Device: CompanionWebClientType
  Device->>App: make_qr_data_with_client_type(device_state, ref, client_type)
  App-->>QR: emit QR payload ("ref,noise,identity,adv,client_type")
  Scanner->>QR: scan
  Scanner->>App: parsed_string -> parse_qr_code(qr_body)
  App->>App: accept 4-field or 5+ field, validate base64/keys
  App-->>Scanner: (ref, noise, identity, adv, client_type?)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main changes: WA Web compliance for pairing QR format and correction of companion_platform derivation.
Description check ✅ Passed The description thoroughly documents both bugs fixed, the implementation approach, breaking changes, and test coverage with clear technical detail.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-pair-qr-client-type

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@wacore/src/companion_reg.rs`:
- Around line 13-25: The enum CompanionWebClientType is using manual
discriminants instead of the wire-format attributes; update the enum to derive
WireEnum and the standard set of derives (Debug, Clone, Copy, Default,
PartialEq, Eq, Hash, WireEnum), add #[wire(kind = "int")] on the enum, remove
all `= N` discriminants, annotate each variant with #[wire = N] corresponding to
its current numeric value and mark the default variant with #[wire_default]
(e.g., Unknown gets #[wire_default] and #[wire = 0]); keep the same variant
names and ordering so the wire values remain unchanged.
🪄 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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e42747c3-bdf6-4406-af7b-4c261986173c

📥 Commits

Reviewing files that changed from the base of the PR and between 63210f7 and bd80606.

📒 Files selected for processing (4)
  • src/pair.rs
  • wacore/src/companion_reg.rs
  • wacore/src/lib.rs
  • wacore/src/pair.rs

Comment thread wacore/src/companion_reg.rs
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 25, 2026

Benchmark Results

1 regression(s) detected (>2% threshold):

Benchmark Current Baseline Change
send_receive_benchmark::group_recv::bench_group_recv text:setup_group_recv() 12,808,213 12,489,647 +2.6%
66 unchanged benchmark(s)
Benchmark Current Baseline Change
reporting_token_benchmark::content_extraction_group::bench_content_extraction simple:setup_simple_message() 3,933 3,933 +0.0%
reporting_token_benchmark::content_extraction_group::bench_content_extraction extended:setup_extended_message() 12,038 12,038 +0.0%
reporting_token_benchmark::key_derivation_group::bench_key_derivation 43,514 43,514 +0.0%
reporting_token_benchmark::token_calculation_group::bench_token_calculation 19,365 19,365 +0.0%
reporting_token_benchmark::full_generation_group::bench_full_token_generation simple:setup_full_gen_simple() 68,579 68,579 +0.0%
reporting_token_benchmark::full_generation_group::bench_full_token_generation extended:setup_full_gen_extended() 76,679 76,679 +0.0%
reporting_token_benchmark::message_encoding_group::bench_message_encoding simple:setup_simple_message() 2,230 2,230 +0.0%
reporting_token_benchmark::message_encoding_group::bench_message_encoding extended:setup_extended_message() 5,988 5,988 +0.0%
send_receive_benchmark::dm_send::bench_dm_send text:setup_dm_send() 169,523 169,520 +0.0%
send_receive_benchmark::dm_recv::bench_dm_recv text:setup_dm_recv() 190,986 190,838 +0.1%
send_receive_benchmark::group_send::bench_group_send group_10:setup_group_send_10() 875,186 875,166 +0.0%
send_receive_benchmark::group_send::bench_group_send group_50:setup_group_send_50() 966,328 966,193 +0.0%
send_receive_benchmark::group_send::bench_group_send group_256:setup_group_send_256() 1,453,231 1,453,270 -0.0%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_10:setup_group_skdm_10() 2,575,110 2,575,130 -0.0%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_50:setup_group_skdm_50() 9,374,992 9,341,498 +0.4%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_256:setup_group_skdm_256() 44,454,427 44,455,718 -0.0%
binary_benchmark::marshal_group::bench_marshal_allocating 71,247 71,247 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_allocating 71,300 71,300 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_allocating 98,367 98,367 +0.0%
binary_benchmark::marshal_group::bench_marshal_reusing_buffer 78,801 78,801 +0.0%
binary_benchmark::marshal_group::bench_marshal_reusing_buffer_vec_writer 71,347 71,347 +0.0%
binary_benchmark::marshal_group::bench_marshal_long_string 7,518 7,518 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_long_string 7,561 7,561 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_long_string 9,273 9,273 +0.0%
binary_benchmark::marshal_group::bench_marshal_huge_bytes_allocating 530,504 530,504 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_huge_bytes_allocating 530,072 530,072 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_huge_bytes_allocating 531,427 531,427 +0.0%
binary_benchmark::marshal_group::bench_marshal_many_children_allocating 8,506,160 8,506,160 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_many_children_allocating 8,450,412 8,450,412 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_many_children_allocating 19,677,947 19,677,947 +0.0%
binary_benchmark::unmarshal_group::bench_unmarshal small:setup_small_marshaled() 2,468 2,468 +0.0%
binary_benchmark::unmarshal_group::bench_unmarshal large:setup_large_marshaled() 33,558 33,558 +0.0%
binary_benchmark::unpack_group::bench_unpack_uncompressed 787 787 +0.0%
binary_benchmark::unpack_group::bench_unpack_compressed 526,732 526,732 +0.0%
binary_benchmark::attr_parser_group::bench_attr_parser attr_lookup:setup_attr_marshaled() 4,986 4,986 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip small:setup_small_marshaled() 5,315 5,315 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip large:setup_large_marshaled() 61,874 61,874 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_auto small:setup_small_marshaled() 5,347 5,347 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_auto large:setup_large_marshaled() 61,942 61,942 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_exact small:setup_small_marshaled() 6,734 6,734 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_exact large:setup_large_marshaled() 85,564 85,564 +0.0%
binary_benchmark::child_iteration_group::bench_get_children_by_tag 477,570 477,570 +0.0%
binary_benchmark::jid_optimization_group::bench_jid_to_owned_access jid_access:setup_jid_heavy_marshaled() 11,563 11,563 +0.0%
numeric_attr_benchmark::bench_group::bench_baseline_u32 396 396 +0.0%
numeric_attr_benchmark::bench_group::bench_proposed_u32 120 120 +0.0%
numeric_attr_benchmark::bench_group::bench_baseline_u64 439 439 +0.0%
numeric_attr_benchmark::bench_group::bench_proposed_u64 153 153 +0.0%
numeric_attr_benchmark::bench_group::bench_baseline_i64 499 499 +0.0%
numeric_attr_benchmark::bench_group::bench_proposed_i64 162 162 +0.0%
numeric_attr_benchmark::bench_group::bench_baseline_loop_100_u64 44,624 44,624 +0.0%
numeric_attr_benchmark::bench_group::bench_proposed_loop_100_u64 16,424 16,424 +0.0%
libsignal_benchmark::dm_group::bench_dm_session_establishment setup:setup_dm_users() 17,295,400 17,345,239 -0.3%
libsignal_benchmark::dm_group::bench_dm_encrypt_first_message first_msg:setup_dm_session() 157,113 157,113 +0.0%
libsignal_benchmark::dm_group::bench_dm_decrypt_first_message decrypt_prekey:setup_dm_with_first_message() 5,510,200 5,510,200 +0.0%
libsignal_benchmark::dm_group::bench_dm_encrypt_subsequent_message subsequent:setup_established_dm_session() 157,827 157,827 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_create_distribution_message create:setup_group_sender() 296,767 296,767 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_encrypt_message encrypt:setup_group_with_distribution() 706,282 706,214 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_decrypt_message decrypt:setup_group_with_encrypted_message() 12,608,127 12,648,703 -0.3%
libsignal_benchmark::conversation_group::bench_full_dm_conversation full:setup_conversation_data() 27,683,988 27,433,511 +0.9%
libsignal_benchmark::signature_group::bench_signature_creation sign:setup_keypair_with_message() 3,467,011 3,467,011 +0.0%
libsignal_benchmark::signature_group::bench_signature_verification verify:setup_keypair_with_message() 125,813,013 125,704,943 +0.1%
libsignal_benchmark::signature_group::bench_key_generation keygen 2,830,452 2,830,452 +0.0%
libsignal_benchmark::session_optimization_group::bench_decrypt_with_previous_session previous_session:setup_with_archived_sessions() 46,003 46,003 +0.0%
libsignal_benchmark::session_optimization_group::bench_out_of_order_decryption out_of_order:setup_out_of_order_messages() 5,072,844 5,072,844 +0.0%
libsignal_benchmark::session_optimization_group::bench_promote_matching_session promote:setup_promote_matching_session() 316,083 316,083 +0.0%
libsignal_benchmark::session_optimization_group::bench_message_key_eviction eviction:setup_message_key_eviction() 14,255,917 14,255,917 +0.0%

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd8060610f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +11 to +13
#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Hash)]
#[repr(i32)]
pub enum CompanionWebClientType {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Derive this protocol enum with WireEnum

CompanionWebClientType is a wire-facing protocol enum, but it is implemented with #[repr(i32)] plus manual Display instead of #[derive(WireEnum)], which violates the repo convention in /workspace/whatsapp-rust/AGENTS.md (“Every protocol enum uses #[derive(WireEnum)] and #[wire = ...] is the single source of truth”). Keeping this as a manual special case makes future wire-tag updates easy to desynchronize from the rest of the protocol layer and can silently produce wrong QR wire values when enum definitions evolve.

Useful? React with 👍 / 👎.

Two pre-existing bugs in commit 63210f7's `derive_companion_platform`:

1. **id used the wrong enum**. The wire field
   `companion_platform_id` carries `WAWebCompanionRegClientUtils.DEVICE_PLATFORM`
   per `docs/captured-js/WAWeb/Alt/DeviceLinkingIq.js:35`, which is
   `CompanionWebClientType` (10 values). We were formatting
   `wa::device_props::PlatformType as i32`. The enums collide on indices 1
   and 7 only — every other browser produced the wrong wire integer
   (Firefox sent "2" instead of "3", Edge "6" instead of "2", etc.) and
   mobile DeviceProps emitted values like "16" that aren't in the WA Web
   enum at all.

2. **display violated server validation**. whatsmeow's `PairPhone` doc
   states the server validates `companion_platform_display` strictly
   as `Browser (OS)` with one of {Chrome, Edge, Firefox, IE, Opera,
   Safari} and 400s otherwise. We were emitting "Android (Android)",
   "iPhone (iOS)", "iPad (iPadOS)" for mobile DeviceProps, breaking
   pair-code for any non-browser platform_type.

Fixes:

- `CompanionWebClientType` is now a `WireEnum` int with `Unrecognized(i32)`
  fallback (consistent with `TempBanReason` / `ConnectFailureReason`),
  unblocking the bridge's TS codegen and giving forward-compat `From<i32>`.
- `derive_companion_platform` returns `(CompanionWebClientType, String)` and
  shares its derivation with the QR pairing flow — single source of truth.
- New `companion_browser_name` returns one of the 6 server-valid browsers,
  falling back to "Chrome" for non-browser variants (what WA Web's
  `info().name` would emit even from Electron).
- New `companion_platform_display(ct, os)` always emits `<Browser> (<OS>)`,
  substituting "Linux" for empty OS to avoid bare browser names which the
  server also rejects.
- `PairCodeOptions::platform_id` is now `Option<CompanionWebClientType>`
  (correct type for the wire field).
- `PairCodeOptions::platform_display` is dropped — WA Web has no equivalent
  override and any free-string value risks server rejection. Display is
  now a pure function of `(client_type, os)`; users customise via
  `DevicePropsOverride::with_os(...)` plus `platform_id`.
- `platform_friendly_name` removed — only caller was the buggy derivation
  and its mixed browser/non-browser table was the bug 2 vector.

Tests cover wire-integer correctness for every proto PlatformType, total
display validity (browser ∈ valid set, always parenthesised OS), and
parity between the pair-code id and the QR id for the same DeviceProps.
@jlucaso1 jlucaso1 changed the title fix(pair): append CompanionWebClientType to pairing QR fix(pair): emit server-valid companion_platform_{id,display} + QR client type Apr 25, 2026
@jlucaso1 jlucaso1 changed the title fix(pair): emit server-valid companion_platform_{id,display} + QR client type fix(pair): WA Web compliant pairing QR + companion_platform_{id,display} Apr 25, 2026
@jlucaso1
Copy link
Copy Markdown
Collaborator Author

@claude review nitpicky

@claude
Copy link
Copy Markdown

claude Bot commented Apr 25, 2026

Claude finished @jlucaso1's task in 2m 24s —— View job


Nitpicky Review of PR #593

  • Read all changed files
  • Review wacore/src/companion_reg.rs
  • Review wacore/src/pair.rs
  • Review wacore/src/pair_code.rs
  • Review src/pair.rs and src/pair_code.rs
  • Review wacore/src/lib.rs
  • Post final review

Overall this is a well-structured PR with excellent test coverage. The CodeRabbit WireEnum finding was already addressed in the follow-up commit. Here are the nitpicks:


wacore/src/companion_reg.rs

1. Display impl is surprising for readers (style, non-blocking)

impl std::fmt::Display for CompanionWebClientType {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        self.code().fmt(f)
    }
}

format!("{}", CompanionWebClientType::Chrome) yields "1", not "Chrome". This is only used in make_qr_data (client_type.to_string()) and pair_with_code (platform_id.to_string()). Using .code().to_string() at the two call sites would be more explicit and avoid a Display impl whose semantics surprise anyone who doesn't already know it's wire-code-oriented. If someone later adds debug logging like info!("pairing as {client_type}"), they'd get "1" instead of something human-readable.

2. companion_browser_name / companion_platform_display could be methods (style, non-blocking)

These free functions take CompanionWebClientType as their first argument. Making them inherent methods (ct.browser_name(), ct.platform_display(os)) would be more discoverable and idiomatic. The const fn constraint on companion_browser_name is preserved either way since inherent methods can also be const.

3. companion_web_client_type_for_platform — future-proofing the match (correctness, minor)

The match is exhaustive today, but if the proto adds a new PlatformType variant, this function will produce a compile error (good!). However, companion_web_client_type_for_props silently maps unknown proto integers to Unknown via .and_then(|v| try_from(v).ok()).unwrap_or_default(). This means a new proto variant that hasn't been regenerated into Rust will silently become Unknown rather than OtherWebClient. Not necessarily wrong, but worth noting the asymmetry: regenerated proto → compile error (forced decision); unregenerated integer → silent Unknown.


wacore/src/pair.rs

4. parse_qr_code validates emptiness of adv_secret_b64 but never decodes it (consistency, non-blocking)

let adv_secret_b64 = parts[3];
if pairing_ref.is_empty()
    || dut_noise_pub_b64.is_empty()
    || dut_identity_pub_b64.is_empty()
    || adv_secret_b64.is_empty()

The noise and identity fields get validated for base64 correctness and key length. The adv secret gets only an emptiness check — malformed base64 or wrong length in parts[3] passes silently. Since the return type doesn't include it, this is by-design (the old code had the same gap with // The ADV secret is not used by the phone side), but the inconsistency in validation depth could mislead a reader into thinking all four fields are fully validated.

5. concat_bytes uses .cloned() where .copied() is more precise (style, trivial)

fn concat_bytes(slices: &[&[u8]]) -> Vec<u8> {
    slices.iter().flat_map(|s| s.iter().cloned()).collect()
}

u8 is Copy, so .copied() is the idiomatic choice and signals to the reader that no Clone work is happening. Fix this →


wacore/src/pair_code.rs

6. Subtle logic duplication between derive_companion_platform and resolve_companion_platform (maintainability, non-blocking)

Both functions independently fetch os and call companion_platform_display(id, os):

// derive_companion_platform:
let os = props.os.as_deref().unwrap_or("");
let display = companion_platform_display(id, os);

// resolve_companion_platform:
let os = props.os.as_deref().unwrap_or("");
let display = companion_platform_display(id, os);

resolve_companion_platform could delegate to derive_companion_platform and just swap the id when an override is present, then rebuild display. Not a big deal at 3 lines, but if the OS-extraction logic ever gains a trim step or default change, there are two places to update.


src/pair.rs

7. make_qr_data_with_client_type is public but undocumented (docs, non-blocking)

make_qr_data has a doc comment mentioning make_qr_data_with_client_type as the override seam, but the latter function has no doc comment itself. Adding a one-liner would help IDE hover and generated docs.

/// Builds the QR payload with an explicit `CompanionWebClientType` override.
pub fn make_qr_data_with_client_type(

src/pair_code.rs

8. platform_id.to_string() relies on the wire-code Display impl (fragility, non-blocking)

let platform_id_str = platform_id.to_string();

This silently breaks if Display is ever changed to emit human-readable names (see nit 1). Using platform_id.code().to_string() would make the intent unambiguous. Same concern applies to wacore/src/pair.rs:75 where client_type.to_string() is used in the QR array.


Tests

9. Variant lists aren't tied to a count assertion (test robustness, trivial)

Several tests enumerate all 10 known variants in an array literal (e.g., from_i32_round_trips_known_values, round_trip_make_then_parse_for_every_client_type). If a new variant is added to the enum but not to these arrays, the tests still pass — they just don't cover the new variant. A const KNOWN_VARIANT_COUNT: usize = 10; with assert_eq!(array.len(), KNOWN_VARIANT_COUNT) would make forgetting a variant a test failure.


Summary

This is a solid PR. The WireEnum pattern is correctly followed (post the CodeRabbit-prompted fix), the single-source-of-truth derivation is clean, the test suite is comprehensive with good regression coverage, and the breaking changes are justified. All the above are genuine nitpicks — none are blocking.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7a7a84bd08

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread wacore/src/pair_code.rs
/// Override for `companion_platform_id`. `None` auto-derives from
/// `Device.device_props.platform_type` via the same mapping used by the
/// pairing QR (`companion_web_client_type_for_props`).
pub platform_id: Option<CompanionWebClientType>,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Re-export CompanionWebClientType in public API

PairCodeOptions::platform_id now uses CompanionWebClientType, but whatsapp-rust does not re-export that enum (the public layer only re-exports PairCodeOptions in src/pair_code.rs), so downstream users depending only on whatsapp-rust can no longer set platform_id without adding a new direct dependency on wacore. This turns an intentional override path into an effectively inaccessible API for existing consumers and is a regression from the previous PlatformType-based field that was reachable via whatsapp_rust::waproto.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@wacore/src/companion_reg.rs`:
- Around line 32-36: The Unrecognized(i32) variant of CompanionWebClientType is
intended decode-only but is currently exposed via PairCodeOptions::platform_id
and serialized by platform_id.to_string(), allowing callers to emit arbitrary
integers; fix by preventing Unrecognized from being produced or serialized:
either split the wire-only fallback into a private/internal enum used for
From<i32> and keep the public CompanionWebClientType without Unrecognized, or
ensure any public accessor used for generation (e.g.,
PairCodeOptions::platform_id, CompanionWebClientType::to_string()/Display)
rejects or maps Unrecognized to a safe value/Error before generation (returning
a Result or panicking/asserting as appropriate) so Unrecognized cannot be
serialized as companion_platform_id.

In `@wacore/src/pair_code.rs`:
- Around line 121-124: PairCodeOptions exposes a platform_id:
Option<CompanionWebClientType> but CompanionWebClientType isn’t re-exported from
the top-level crate, making the field unusable for downstream consumers; fix
this by re-exporting CompanionWebClientType alongside PairCodeOptions (e.g., add
a public re-export like `pub use wacore::pair_code::CompanionWebClientType;` in
the same module where PairCodeOptions is re-exported) so callers can import the
type from the crate root and set PairCodeOptions::platform_id without reaching
into wacore.
🪄 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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b55dd7be-d272-47b7-8200-1c789f714f92

📥 Commits

Reviewing files that changed from the base of the PR and between bd80606 and 7a7a84b.

📒 Files selected for processing (3)
  • src/pair_code.rs
  • wacore/src/companion_reg.rs
  • wacore/src/pair_code.rs

Comment thread wacore/src/companion_reg.rs Outdated
Comment thread wacore/src/pair_code.rs Outdated
- `companion_web_client_type_for_platform` is `const fn` again (lost it
  during the WireEnum migration).
- `PairUtils::make_qr_data` takes `ref_str: &str` instead of `String` and
  formats directly via `format!`, dropping the intermediate
  `client_type.to_string()` allocation. Top-level wrappers updated to
  match.
- Soften "server validates / 400s" claims in docstrings to "whatsmeow's
  PairPhone doc reports …" since the captured WA Web JS doesn't show the
  validation directly.
- Document that WA Web emits the native-camera prefix and FAQ-URL suffix
  mutually exclusively; `parse_qr_code` is permissive on purpose for
  e2e replay.
- Drop unused `adv_secret_b64` variable in `parse_qr_code`.
- Add docstring to `make_qr_data_with_client_type`.
- Add direct `Display` test for `CompanionWebClientType`, including the
  `Unrecognized(i32)` fallback paths (positive and negative integers).
…ection

WA Web's RegClientUtils.js short-circuits to UWP when isWindows is true,
but that flag is WAWebEnvironment's gkx("4112") — a GateKeeper
experiment that fires only when the JS is bundled inside the Microsoft
Store/UWP shell, not when the user's OS is Windows. Document why our
mapping doesn't replicate that branch (no analogous host-shell detection
in a Rust library; the user sets PlatformType::Uwp explicitly to claim
UWP). Same explanation applies to Electron — never emitted by WA Web's
runtime selector either, expressed via PlatformType::Desktop here.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
wacore/src/pair_code.rs (1)

120-124: ⚠️ Potential issue | 🟡 Minor

CompanionWebClientType needs to be re-exported for downstream users.

This was flagged in a previous review and remains valid. PairCodeOptions is publicly exported, and its platform_id field is typed as Option<CompanionWebClientType>. Downstream users who want to set this field can't import CompanionWebClientType without reaching into wacore directly.

Add a re-export in src/lib.rs or alongside PairCodeOptions:

pub use wacore::companion_reg::CompanionWebClientType;
#!/bin/bash
# Verify if CompanionWebClientType is already re-exported from the main crate
rg -n --type=rust 'pub use.*CompanionWebClientType' src/
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacore/src/pair_code.rs` around lines 120 - 124, PairCodeOptions exposes
platform_id: Option<CompanionWebClientType> but CompanionWebClientType is not
re-exported for downstream users; add a public re-export for
CompanionWebClientType (e.g., pub use
wacore::companion_reg::CompanionWebClientType) alongside the public API so
callers can import CompanionWebClientType without depending on wacore, ensuring
the symbol is available where PairCodeOptions is exposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@wacore/src/pair_code.rs`:
- Around line 120-124: PairCodeOptions exposes platform_id:
Option<CompanionWebClientType> but CompanionWebClientType is not re-exported for
downstream users; add a public re-export for CompanionWebClientType (e.g., pub
use wacore::companion_reg::CompanionWebClientType) alongside the public API so
callers can import CompanionWebClientType without depending on wacore, ensuring
the symbol is available where PairCodeOptions is exposed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 316b0841-9387-48a6-afd4-fc25594c0c00

📥 Commits

Reviewing files that changed from the base of the PR and between 7a7a84b and 1279782.

📒 Files selected for processing (4)
  • src/pair.rs
  • wacore/src/companion_reg.rs
  • wacore/src/pair.rs
  • wacore/src/pair_code.rs

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
wacore/src/companion_reg.rs (1)

32-43: ⚠️ Potential issue | 🟠 Major

We can’t let Unrecognized(i32) leak into outbound wire values

Line 41 serializes every variant as raw code, so a public CompanionWebClientType::Unrecognized(42) can become "42" in outbound pairing fields and trigger server 400s. Keep fallback decode-only on the wire-producing path.

Proposed hardening
+impl CompanionWebClientType {
+    pub const fn outbound_code(self) -> i32 {
+        match self {
+            Self::Unrecognized(_) => Self::Unknown.code(),
+            _ => self.code(),
+        }
+    }
+}
+
 impl std::fmt::Display for CompanionWebClientType {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.code().fmt(f)
+        self.outbound_code().fmt(f)
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacore/src/companion_reg.rs` around lines 32 - 43, The current Display impl
for CompanionWebClientType calls self.code() for every variant and thus
serializes Unrecognized(i32) into a raw numeric wire value; change fmt so it
only formats known variants (by delegating to self.code() or matching each real
variant) and explicitly refuse to format CompanionWebClientType::Unrecognized(_)
(e.g., return Err(std::fmt::Error) or otherwise avoid emitting a numeric string)
so the fallback remains decode-only and cannot be sent on the wire; update the
impl std::fmt::Display for CompanionWebClientType to match on self and handle
Unrecognized specially instead of calling self.code() unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@wacore/src/companion_reg.rs`:
- Around line 71-79: Trim the lengthy comment block in companion_reg.rs (the
multi-line doc comment explaining WA Web runtime selector) to a concise "why"
summary and move the detailed GateKeeper/WAWeb wiring and rationale into a
separate ADR or docs file; keep a short note referencing PlatformType::Uwp and
Desktop/Electron behavior in the code comment and add a TODO pointing to the new
ADR so callers can find the full explanation later.

---

Duplicate comments:
In `@wacore/src/companion_reg.rs`:
- Around line 32-43: The current Display impl for CompanionWebClientType calls
self.code() for every variant and thus serializes Unrecognized(i32) into a raw
numeric wire value; change fmt so it only formats known variants (by delegating
to self.code() or matching each real variant) and explicitly refuse to format
CompanionWebClientType::Unrecognized(_) (e.g., return Err(std::fmt::Error) or
otherwise avoid emitting a numeric string) so the fallback remains decode-only
and cannot be sent on the wire; update the impl std::fmt::Display for
CompanionWebClientType to match on self and handle Unrecognized specially
instead of calling self.code() unconditionally.
🪄 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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 37546352-4203-4f97-bd47-8520268b2525

📥 Commits

Reviewing files that changed from the base of the PR and between 1279782 and 506a0f9.

📒 Files selected for processing (1)
  • wacore/src/companion_reg.rs

Comment thread wacore/src/companion_reg.rs Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 506a0f93eb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/pair.rs
Comment on lines +25 to +28
pub fn make_qr_data_with_client_type(
store: &crate::store::Device,
ref_str: &str,
client_type: CompanionWebClientType,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Re-export companion enum for QR override API

make_qr_data_with_client_type is public but its parameter type (CompanionWebClientType) is not exposed by whatsapp-rust, so downstream crates that depend only on whatsapp-rust cannot call this new override path without adding a direct wacore dependency. That makes the advertised override seam effectively unusable for current consumers of the high-level crate.

Useful? React with 👍 / 👎.

…se comments

- Drop WireEnum on CompanionWebClientType, revert to #[repr(i32)]. The
  fallback Unrecognized(i32) was reachable via PairCodeOptions::platform_id
  and serialised by Display, letting callers emit arbitrary wire integers.
  Since this enum is encode-only (we never decode wire ints), the WireEnum
  pattern doesn't apply: all 10 valid values are nameable as variants and
  the user can't escape the spec.
- Re-export CompanionWebClientType from src/pair.rs and src/pair_code.rs
  so downstream callers can set PairCodeOptions::platform_id without
  reaching into wacore directly.
- Trim doc comments across companion_reg.rs / pair.rs / pair_code.rs:
  keep the why, drop the long-form rationale.
Forgotten in the previous commit. Downstream consumers can now write
`use whatsapp_rust::pair_code::{CompanionWebClientType, PairCodeOptions}`
or `use whatsapp_rust::pair::CompanionWebClientType` without reaching
into wacore directly.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
wacore/src/companion_reg.rs (1)

6-21: ⚠️ Potential issue | 🟠 Major

Put the wire mapping back on the enum itself.

This enum sits on the protocol boundary. #[repr(i32)] plus as i32 makes the wire contract implicit again, and this crate already has a stronger pattern for that. We should keep the numeric mapping attached to each variant with #[derive(WireEnum)] and #[wire = N].

Suggested fix
-#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Hash)]
-#[repr(i32)]
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Hash, WireEnum)]
+#[wire(kind = "int")]
 pub enum CompanionWebClientType {
     #[default]
-    Unknown = 0,
-    Chrome = 1,
-    Edge = 2,
-    Firefox = 3,
-    Ie = 4,
-    Opera = 5,
-    Safari = 6,
-    Electron = 7,
-    Uwp = 8,
-    OtherWebClient = 9,
+    #[wire_default]
+    #[wire = 0]
+    Unknown,
+    #[wire = 1]
+    Chrome,
+    #[wire = 2]
+    Edge,
+    #[wire = 3]
+    Firefox,
+    #[wire = 4]
+    Ie,
+    #[wire = 5]
+    Opera,
+    #[wire = 6]
+    Safari,
+    #[wire = 7]
+    Electron,
+    #[wire = 8]
+    Uwp,
+    #[wire = 9]
+    OtherWebClient,
 }

As per coding guidelines "Every protocol enum must use #[derive(WireEnum)]; the #[wire = "..."] or #[wire = NUM] attribute is the single source of truth for each variant's wire value; do not also derive serde::Serialize/Deserialize or add #[serde(rename_all)]".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacore/src/companion_reg.rs` around lines 6 - 21, The enum
CompanionWebClientType must declare its wire values explicitly: remove
#[repr(i32)] and instead add #[derive(WireEnum)] to CompanionWebClientType and
annotate each variant with its wire mapping (e.g., #[wire = 0] on Unknown,
#[wire = 1] on Chrome, etc.), keeping the #[default] on Unknown; also update any
call sites that relied on casting (as i32) to use the WireEnum-provided
conversion helpers if available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@wacore/src/pair.rs`:
- Around line 292-299: The QR parsing currently only checks pairing_ref,
dut_noise_pub_b64, dut_identity_pub_b64, and parts[3], so a 5-field WA Web QR
with an empty fifth field slips through; update the validation in the function
that contains pairing_ref / dut_noise_pub_b64 / dut_identity_pub_b64 /
parts[...] to also reject an empty fifth segment when parts.len() == 5 by
validating parts[4] (the client_type) is non-empty and returning the same
anyhow::anyhow! error on failure, and add a regression test that constructs a
5-field QR where the fifth field is empty and asserts the parser returns an
error.

---

Duplicate comments:
In `@wacore/src/companion_reg.rs`:
- Around line 6-21: The enum CompanionWebClientType must declare its wire values
explicitly: remove #[repr(i32)] and instead add #[derive(WireEnum)] to
CompanionWebClientType and annotate each variant with its wire mapping (e.g.,
#[wire = 0] on Unknown, #[wire = 1] on Chrome, etc.), keeping the #[default] on
Unknown; also update any call sites that relied on casting (as i32) to use the
WireEnum-provided conversion helpers if available.
🪄 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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c6eb10ed-41c8-4c3c-9d30-9b6fcb9b44ab

📥 Commits

Reviewing files that changed from the base of the PR and between 506a0f9 and 28485d2.

📒 Files selected for processing (4)
  • src/pair.rs
  • wacore/src/companion_reg.rs
  • wacore/src/pair.rs
  • wacore/src/pair_code.rs

Comment thread wacore/src/pair.rs
…ths from comments

- Promote the iOS native-camera QR prefix to a public constant
  (`wacore::companion_reg::NATIVE_CAMERA_DEEP_LINK_PREFIX`,
  re-exported from `whatsapp_rust::pair`) so callers can build a
  deep-link URL by prepending it to `make_qr_data` output.
- `parse_qr_code` now uses the same constant (single source of truth).
- Drop the `docs/captured-js/...` paths from comments per project
  rule .claude/rules/code-style.md — reference WA Web modules by name only.
5-field WA Web QR with an empty client_type slot would slip past the
existing parts[3] non-empty check. Extend validation to all parts
beyond the first four so any empty segment in the legacy or compliant
form is rejected with the same error.

Adds a regression test that builds a 5-field QR with an empty 5th
segment and asserts the parser fails.
@jlucaso1 jlucaso1 merged commit 2d82f2c into main Apr 25, 2026
9 of 10 checks passed
@jlucaso1 jlucaso1 deleted the fix-pair-qr-client-type branch April 25, 2026 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant