test(twilio): unit test the controller, which had no spec at all

[chrispaskvan]

The follow-up to #738 and #740: twilio/twilio.controller.js is the core of the SMS interface and had no spec file at all. Its 54.5% branch coverage came incidentally, from the routes spec exercising it over HTTP.

Coverage

	before	after
twilio/twilio.controller.js branches	60/110 (54.5%)	99/110 (90.0%)
global branches	80.00%	83.61%

Sixty-five tests, over three runs to confirm they are not flaky.

What it covers

The units the routes spec reaches only sideways:

• the consent worker's subscription and the rules it applies — a superseded intent discarded, a tie applied, a superseded etag retried, a permanent fault not retried, and the retries exhausting on fake timers rather than fifteen seconds of real waiting
• request's unregistered, opted-out and already-told branches; keyword handling without any sender lookup, which is the carrier-compliance property; the inline fallback when the queue will not take a change
• media filtering, including a mixed attachment where only the image survives
• every mapped emoji intent, the default for an unmapped one, and an emoji riding along with text still reaching search
• the more keyword with and without a remembered item
• item search across no match, one match, distinct matches and duplicates collapsed to one; damage type present and absent; the landline path that withholds an icon
• getXur across failed authentication, no characters, weapon filtering, a DestinyError relayed verbatim, and an unexpected error that must not be
• statusCallback across a missing recipient, a missing status, an unknown number, the legacy SmsStatus field, and the claim-check update

Thresholds

Left where autoUpdate ratcheted them: statements 82.54 → 88.63, functions 79.76 → 86.15, lines 82.54 → 88.78. Branches stays at 88.99, still above the 83.61 the suite reaches — the remaining debt is mcp/mcp.routes.js and notifications/notification.routes.js at zero, and helpers/queryBuilder.js at 58.8%.

Two notes on the tests

One assertion deliberately avoids an item's name. The no-results reply is drawn at random from a list, and one of them reads "Does it look like a Gjallarhorn?" — so a test asserting the name is absent fails intermittently. It asserts on the item's detail instead, with a comment saying why.

Two tests characterise dead code rather than a rule. The itemName.includes('Catalyst') guard in queryItem does nothing: it is unreachable from request(), which lowercases the term before passing it to a case-sensitive check, and redundant anyway — every catalyst in the manifest is itemType 19, 20, 0 or 12, while the filter beside it admits only 2, 3 and 4. Both tests need a weapon-typed item named 'Catalyst', which the manifest does not contain, and that is itself the evidence. Recorded in #741 rather than changed here, since deleting it is a separate call.

🤖 Generated with Claude Code

https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf

[Copilot]

Copilot review overview

🔵 Needs a closer look

The MMS test does not yet verify the advertised non-blocking behavior.

Review effort: Lite
Findings: None

What changed in this PR

Adds comprehensive unit coverage for TwilioController and raises coverage thresholds.

Changes:

• Tests consent, messaging, searches, Xur responses, callbacks, and fallback behavior.
• Updates statement, function, and line coverage thresholds.

File	Description
vitest.config.js	Updates coverage thresholds.
twilio/​twilio.controller.spec.js	Adds comprehensive controller unit tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chrispaskvan]

Right, and worth acting on even though it came through as a summary line rather than a finding. Fixed in a8721bc.

The test was named should hand images off and acknowledge without waiting and could not see whether anything waited. mmsService.process is a double that settles immediately, so asserting it was called and that the reply came back passes identically when the source awaits the handoff — which is the one thing the test existed to rule out. I checked: changing void this.mms.process(...) to await failed nothing in the file.

Split in two. The first keeps the handoff assertion under a name that claims only that. The second leaves the analysis outstanding via Promise.withResolvers() and asks for the reply anyway, so awaiting the handoff now hangs the request and fails the test. Verified by re-running that mutation.

This is the same shape as the ordering test on #740: a claim about when something happens needs the dependency left pending, because no call-order assertion can tell awaited from merely fast. Two for two on that pattern, which is a useful thing to have learned twice.

896 passed, 1 skipped; lint and typecheck clean.

[Copilot]

Copilot review overview

🟢 Approval recommended

The only review comment is a minor test-specificity nit; no blocking issues were identified.

Review effort: Lite
Findings: None