fix(signal): route username targets as usernames, not phone numbers#96308
fix(signal): route username targets as usernames, not phone numbers#96308ly-wang19 wants to merge 2 commits into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 3:32 AM ET / 07:32 UTC. Summary PR surface: Source +12, Tests +31. Total +43 across 2 files. Reproducibility: yes. Source inspection shows current main strips Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the plugin-owned route fix after maintainers accept that ambiguous pre-fix username sessions should not be migrated. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main strips Is this the best way to solve the issue? Yes. The Signal plugin outbound-session resolver is the narrowest maintainable fix because core delegates route semantics to channel plugins and the existing Signal send path already has a username target kind. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b63e06f68aa0. Label changesLabel justifications:
Evidence reviewedPR surface: Source +12, Tests +31. Total +43 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
proof: sufficient
resolveSignalOutboundTarget stripped the username:/u: prefix to the bare name and then fed it to the phone/uuid resolver, where normalizeE164 digit-strips the value — so "username:alice.42" resolved to phone "+42", "username:kevin" to "+", corrupting both the delivery target and the session key. Add a dedicated username branch (twin of send.ts parseTarget) that keeps the username grammar intact; group/phone/uuid targets are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
0524e48 to
8fdf1e7
Compare
clawsweeper
Bot
added
the
merge-risk: 🚨 session-state
|
Superseded by #107365, now landed as 4ea1c71. The landed fix keeps username targets out of phone normalization for both outbound session routing and direct sends, while preserving phone, UUID, and group behavior. It also carries @ly-wang19 as co-author. Thank you for finding and driving this bug. |
2 participants
Summary
resolveSignalOutboundTarget(extensions/signal/src/outbound-session.ts) handled the documentedusername:/u:Signal send grammar by stripping the prefix to the bare name — and then fell through to the phone/uuid resolver.resolveSignalSendertreats any non-emptysourceNumberas a phone and runs it throughnormalizeE164, which strips all non-digits. So:username:alice.42→ phone+42username:kevin→+(empty)signal:username:bob.99→+99Both the returned
to/fromand theRoutePeerid (the session key) are corrupted, so an agent replying/sending to a Signal username delivers to a wrong/empty phone number (or fails) and is keyed to the wrong session. Group and phone/uuid targets resolve correctly; only the username grammar was mis-dispatched.Usernames are a first-class, documented send target (
docs/channels/signal.md), and the siblingsend.ts parseTargetmapsusername:/u:to a distinct username send —resolveSignalOutboundTargetwas the one target-grammar surface that didn't honor it.Changes
outbound-session.ts— add a dedicated username branch (after thegroup:branch) that returns{ peer: { kind: "direct", id: "username:<name>" }, chatType: "direct", to/from: "signal:username:<name>" }, and remove the prefix-stripping that fell through into the phone resolver.outbound-session.test.ts— new colocated test (username + group + phone).Real behavior proof
Behavior addressed: a Signal
username:/u:target was digit-stripped into a bogus phone number, corrupting delivery and the session key. After the fix the username grammar is preserved.Real environment tested: Node 22.22.1, macOS, no network/model. Vitest exercises the real exported
resolveSignalOutboundTarget. BEFORE isorigin/main's file (swapped viagit show).Exact steps or command run after this patch:
Evidence after fix:
Observed result after fix: username targets keep their grammar (correct delivery target + session key); group/phone/uuid paths are unchanged. The new test fails on
origin/mainand passes after.What was not tested: a live Signal send (the test drives the real
resolveSignalOutboundTarget, which the outbound route + agent-delivery paths consume to set the delivery target).Additional checks
node scripts/run-vitest.mjs extensions/signal/src/outbound-session.test.tspasses; fail-before verified by swapping inorigin/main's file. Extensionoxlintclean.