Pull request overview

Improves KiwiSDR connection failure reporting by decoding additional server/proxy denial states into more specific user-facing error messages, and by preserving more detail from the status-page preflight checks.

Changes:

• Added IpLimitNotice + parseIpLimitNotice() (with tests) to decode ip_limit denial payloads (including percent-encoded comma cases).
• Enhanced status preflight failure reporting to retain the first HTTP attempt’s status/error when falling back from HTTP → HTTPS.
• Expanded MSG key handling in KiwiSdrClient to map more denial conditions (down, wb_only, exclusive_use, ip_limit, inactivity_timeout, password_timeout, kiwi_kick, richer badp mapping) into targeted errors.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Thanks for this, @rfoust — this is a well-scoped, high-quality improvement. The denial states are mapped to clear, bounded operator-facing messages, every error path calls cleanupSockets() before returning, and the new parseIpLimitNotice parser is properly defensive (rejects non-numeric / non-positive minutes) with good unit coverage for the encoded, minute-only, and invalid cases. Preserving the HTTP denial status through the preflight retry instead of collapsing it to "unreachable" is a real UX win. Files are all within scope.

A couple of notes:

1. Copilot's down / reason_disabled finding is valid (minor). The reason_disabled handler now looks up down via messageValue(...), but the down handler always passes QString() as the reason:

setState(State::Error, downMessage(downOk ? downValue : 0, QString()));

Because the loop returns on the first matching key, a message like down=3 reason_disabled=<operator text> loses the operator's reason if down is encountered first. It's low-severity — for down=1/down=2 the reason is intentionally ignored (fixed update/backup text), so the gap only affects other non-zero down values that also carry a reason_disabled. Mirroring the lookup in the down handler (messageValue("reason_disabled"), percent-decoded) would make the two paths symmetric and order-independent. Worth a one-line fix if you're touching it again, but not a blocker.

2. Minor i18n nit (optional). statusPreflightFailureMessage and the downMessage lambda use QObject::tr(...), which files these strings under the QObject translation context rather than KiwiSdrClient. Harmless functionally; just slightly inconsistent with the surrounding tr(...) calls. Not worth changing on its own.

Nice work — the defensive decoding and test coverage are exactly right for a boundary-input change.

_{🤖 aethersdr-agent · cost: $1.9058 · model: claude-opus-4-8}

Thanks @rfoust — the defensive decoding, the cleanupSockets() discipline on every error path, and the test coverage are all the right instincts, and a couple of pieces here are clear wins I'd like to land regardless of the rest:

• Status-preflight HTTP-status preservation (statusPreflightFailureMessage) — keeping a 401/403 from the first HTTP attempt instead of collapsing to "unreachable" is a real, version-safe UX improvement.
• parseIpLimitNotice mechanics — properly validated (rejects non-numeric / non-positive minutes), good unit coverage.

Before this lands in full, though, there's one correctness problem and one provenance question I need to resolve, because this PR translates protocol codes into confident, specific user-facing text — and a wrong-but-authoritative message is worse than the generic one it replaces.

1. badp=5 contradicts our own clean-room observations

This PR maps badp=5 → "does not allow multiple connections from the same IP address." But our clean-room record in docs/kiwisdr-cleanroom-design.md shows badp=5 returned for ordinary public-auth rejection, on single fresh probes to three different servers:

L290: "zl2j.proxy.kiwisdr.com:8073 … returned badp=5 and closed before sending audio for both p=# and empty public-password auth"
L297: "k9czi-1.proxy.kiwisdr.com:8073 … returned badp=5 for public auth before closing"
L310: "162.199.177.108:8073 … Sending public auth first returned badp=0 and normal setup"

That's the most common real failure (public access denied). Today it hits the generic "may require a password or site-specific login" hint; under this PR the same servers would tell users "does not allow multiple connections from the same IP address" — wrong, and it drops the password hint that actually helps. Three different servers returning that on a first connection can't be a multi-connection condition.

2. Provenance of the rest (clean-room / Principle IV)

Our KiwiSDR integration is deliberately clean-room, and the design doc took the conservative, version-agnostic stance (badp/down/reason_disabled → generic local errors). This PR adds specific semantics we have no observations for — badp 1, 2, 3, 4, 6, 7, and six new MSG keys never seen in our captures (wb_only, exclusive_use, inactivity_timeout, password_timeout, kiwi_kick, and the ip_limit wire format itself) — with no provenance cited and no THIRD_PARTY_LICENSES change. The badp=5 miss matters beyond itself: the one code we can check against reality failed, so I can't take the others on faith.

What I need from you, Robbie:

1. Where did the badp 1–7 meanings and the new MSG keys come from? A KiwiSDR source file (which version/commit)? A published spec? Live observation? This decides clean-room handling — if it's source-derived we need attribution recorded, not landed silently.
2. Can you reconcile badp=5? Is the numbering version-dependent (i.e., did you read a newer KiwiSDR where 5 means something different than the builds our probes hit)? A citable source + a note on version skew would settle it.
3. For ip_limit / the new keys — observed on a live server, or inferred? A frame capture we can point to would let us treat the wire format as confirmed rather than assumed.

Two ways forward

• (A) Narrow to the version-safe subset now: keep statusPreflightFailureMessage, the ip_limit parser, and the generic improved badp text; drop the per-code badp 1–7 switch and the unobserved MSG keys. Lands the real value with zero accuracy/provenance risk, and the specific codes can return once verified.
• (B) Verify in full: cite an authoritative source for the codes + keys, reconcile badp=5, and add attribution if source-derived — then it all lands.

Minor (non-blocking, already noted by the bot)

• down/reason_disabled ordering: the down branch passes an empty reason, so down=N reason_disabled=… loses the operator text if down parses first. Mirror messageValue("reason_disabled") (percent-decoded) in the down branch to make the paths order-independent.
• QObject::tr vs member tr() context — cosmetic.

Happy to take path (A) and merge the safe parts immediately if you'd rather verify the codes separately — your call on which is less work given your in-flight split-VFO/KiwiSDR sweep.

Re-reviewed the amended PR — path (A) executed cleanly. All concerns resolved; clearing my changes-requested.

Verified against the new diff:

• badp per-code switch removed, including the incorrect badp=5 mapping. Nonzero badp now stays generic, preserves the numeric code (badp=%1) and keeps the password / site-login / access-policy hint via identityDiagnosticText(). ✓
• Unobserved, source-derived MSG keys dropped — zero references to wb_only / exclusive_use / inactivity_timeout / password_timeout / kiwi_kick. Provenance concern moot. ✓
• ip_limit kept (the observed wire failure) with the defensive parser that validates minutes/address and falls back to generic text — fails safe. ✓
• down / reason_disabled order-independent — both branches now look up and percent-decode the other key, so message selection no longer depends on field order. ✓
• i18n context fixed — trKiwiSdrClient() routes the free-function strings through QCoreApplication::translate("KiwiSdrClient", …); no more QObject::tr. ✓

The version-safe wins (status-preflight HTTP-status preservation, defensive ip_limit decode, improved generic badp/down text) all land with no accuracy or provenance risk. CI green on all six checks. Thanks for the quick, precise turnaround, @rfoust.