A capability that arrives after connect is now taken up

Asking `CAP LS 302` turns on `cap-notify`, so a server is entitled to send
`CAP NEW` when something becomes available and `CAP DEL` when it goes away.
Only `CAP ACK` was matched, so both fell through the router and the
capability set was whatever was true in the first second of the connection,
for the life of it.

`NEW` is now answered with the intersection of what is offered and what was
wanted, so a server offering something this daemon does not understand is
not answered with a request it would then have to honour. `DEL` takes the
capability back at once - it is the server stating a fact rather than
offering something, so there is no acknowledgement to wait for.

`DEL` is the half that was doing harm: moho went on believing it held a
capability the server had withdrawn, and kept using it.

SASL is deliberately left alone, and writing the test is what surfaced why:
it is not in WANTED_CAPS at all, because the handshake in sasl.rs requests
it before registration and drives the exchange with a blocking read on the
stream. Taking it up here would record it as held without authenticating
anything, making `irc_has_cap(.., "sasl")` true for a connection that is not
signed in. Re-authenticating an already-registered connection wants a state
machine in the router, which is its own piece of work.

Closes #176.

Co-Authored-By: Claude Opus 5 <[email protected]>

src/backend/irc/connect.rs

@@ -541,6 +541,31 @@ pub(super) fn cap_list<'a>(param: Option<&'a str>, suffix: Option<&'a str>) -> &
     param.or(suffix).unwrap_or("")
 }
 
+/// Which of a `CAP NEW` offer are worth asking for.
+///
+/// The intersection with `WANTED_CAPS`, because a server offering something
+/// this daemon does not understand should not be answered with a request it
+/// would then have to honour. Values are stripped the same way `grant_irc_caps`
+/// strips them: a server may offer `sasl=PLAIN,EXTERNAL`, and the name is what
+/// is being asked for.
+///
+/// `sasl` is deliberately not among them, and it is the interesting omission.
+/// It is not in `WANTED_CAPS` at all - the handshake in `sasl.rs` requests it
+/// on its own, before registration, and drives the exchange by reading the
+/// stream directly. Asking for it again here would record the capability as
+/// held without authenticating anything, which is worse than not asking:
+/// `irc_has_cap(.., "sasl")` would then be true for a connection that is not
+/// signed in. Re-authenticating an already-registered connection needs a state
+/// machine in the router rather than a blocking read, and is its own piece of
+/// work.
+pub(super) fn caps_worth_requesting(offered: &str) -> Vec<&str> {
+    offered
+        .split_whitespace()
+        .map(|cap| cap.split('=').next().unwrap_or(cap))
+        .filter(|cap| WANTED_CAPS.contains(cap))
+        .collect()
+}
+
 /// Closes capability negotiation and sends the ordinary NICK/USER pair.
 ///
 /// Shared by the authenticated path and the fell-back-to-nothing path because
@@ -888,6 +913,44 @@ mod quit_tests {
     }
 }
 
+#[cfg(test)]
+mod cap_new_tests {
+    use super::caps_worth_requesting;
+
+    /// A `CAP NEW` is answered with the intersection, not with everything.
+    ///
+    /// Asking for something never wanted is not harmless: a capability this
+    /// daemon does not understand still changes what the server sends once it
+    /// is granted, and nothing here would know what to do with it.
+    #[test]
+    fn only_what_was_wanted_in_the_first_place_is_asked_for() {
+        let asked = caps_worth_requesting("chghost vendor.example/thing away-notify");
+        assert_eq!(asked, vec!["chghost", "away-notify"]);
+    }
+
+    /// A capability offered with a value is asked for by name.
+    #[test]
+    fn a_capability_offered_with_a_value_is_asked_for_by_name() {
+        assert_eq!(caps_worth_requesting("draft/chathistory=50"), vec!["draft/chathistory"]);
+    }
+
+    /// SASL is left alone on purpose - see `caps_worth_requesting`. Recording
+    /// it as held without running the exchange would make
+    /// `irc_has_cap(.., "sasl")` true for a connection that is not signed in.
+    #[test]
+    fn sasl_is_not_taken_up_here() {
+        assert!(caps_worth_requesting("sasl=PLAIN,EXTERNAL").is_empty());
+    }
+
+    /// An offer of nothing recognisable produces no request, rather than an
+    /// empty `CAP REQ` for a server to puzzle over.
+    #[test]
+    fn an_offer_of_nothing_useful_is_not_answered() {
+        assert!(caps_worth_requesting("vendor.example/one vendor.example/two").is_empty());
+        assert!(caps_worth_requesting("").is_empty());
+    }
+}
+
 #[cfg(test)]
 mod cap_tests {
     use super::cap_list;

src/backend/irc/incoming.rs

@@ -632,6 +632,41 @@ pub(super) async fn handle_message(
             state.runtime.grant_irc_caps(account_id, caps);
         }
 
+        // Something became available after registration.
+        //
+        // Asking `CAP LS 302` turns on `cap-notify`, which is what entitles a
+        // server to send this at all - so a connection that negotiated at all
+        // is one that can be told, later, that history was switched on, or
+        // that a module offering `chghost` was loaded. Before this the
+        // capability set was whatever was true in the first second of the
+        // connection, for the life of the connection.
+        //
+        // Only what was wanted in the first place is asked for (and not
+        // `sasl`, which `caps_worth_requesting` explains). Nothing is recorded
+        // here: the ACK above is what records a capability, and a `NEW` the
+        // server then refuses should leave us exactly where we were.
+        Command::CAP(_, CapSubCommand::NEW, ref param, ref suffix) => {
+            let offered = cap_list(param.as_deref(), suffix.as_deref());
+            let wanted = super::connect::caps_worth_requesting(offered);
+            if wanted.is_empty() {
+                return;
+            }
+            tracing::debug!("irc[{account_id}]: newly offered, requesting: {}", wanted.join(" "));
+            let _ = sender.send(Command::CAP(None, CapSubCommand::REQ, None, Some(wanted.join(" "))));
+        }
+
+        // Something was taken away.
+        //
+        // The half that matters more than `NEW`: without it moho goes on
+        // believing it holds a capability the server has withdrawn, and keeps
+        // using it - sending a tag the server no longer reads, or asking for
+        // history from a server that just turned it off.
+        Command::CAP(_, CapSubCommand::DEL, ref param, ref suffix) => {
+            let caps = cap_list(param.as_deref(), suffix.as_deref());
+            tracing::debug!("irc[{account_id}]: capabilities withdrawn: {caps}");
+            state.runtime.revoke_irc_caps(account_id, caps);
+        }
+
         // The frame around a multiline message. Only multiline batches are
         // collected: a chathistory batch contains messages that are each
         // their own message, and folding those together would turn a replayed

src/runtime.rs

@@ -3116,6 +3116,20 @@ impl Runtime {
         }
     }
 
+    /// Takes back capabilities the server has withdrawn (`CAP DEL`).
+    ///
+    /// No acknowledgement is involved, which is what separates this from
+    /// `grant_irc_caps`: a `CAP DEL` is the server stating a fact, not
+    /// offering something to be asked for, so the capability is gone the
+    /// moment the line arrives and nothing is sent back.
+    pub fn revoke_irc_caps(&self, account_id: &str, caps: &str) {
+        let mut all = self.irc_caps.lock().unwrap();
+        let Some(granted) = all.get_mut(account_id) else { return };
+        for cap in caps.split_whitespace() {
+            granted.remove(cap.split('=').next().unwrap_or(cap));
+        }
+    }
+
     pub fn clear_irc_caps(&self, account_id: &str) {
         self.irc_caps.lock().unwrap().remove(account_id);
         self.irc_isupport.lock().unwrap().remove(account_id);
@@ -4134,6 +4148,50 @@ mod keyword_tests {
     }
 }
 
+#[cfg(test)]
+mod irc_cap_tests {
+    use super::Runtime;
+
+    /// A `CAP DEL` is the server stating a fact, so the capability is gone
+    /// the moment it arrives - no acknowledgement, and nothing left behind.
+    ///
+    /// This is the half that matters: before it, moho went on believing it
+    /// held a capability the server had withdrawn, and kept using it.
+    #[test]
+    fn a_withdrawn_capability_stops_being_held() {
+        let rt = Runtime::new();
+        rt.grant_irc_caps("a", "server-time away-notify chghost");
+        assert!(rt.irc_has_cap("a", "away-notify"));
+
+        rt.revoke_irc_caps("a", "away-notify");
+        assert!(!rt.irc_has_cap("a", "away-notify"));
+        // And only that one.
+        assert!(rt.irc_has_cap("a", "server-time"));
+        assert!(rt.irc_has_cap("a", "chghost"));
+    }
+
+    /// Withdrawn by name, whatever value it was granted with.
+    #[test]
+    fn a_value_does_not_stop_a_capability_being_withdrawn() {
+        let rt = Runtime::new();
+        rt.grant_irc_caps("a", "sasl=PLAIN,EXTERNAL");
+        assert!(rt.irc_has_cap("a", "sasl"));
+        rt.revoke_irc_caps("a", "sasl=PLAIN");
+        assert!(!rt.irc_has_cap("a", "sasl"));
+    }
+
+    /// A `CAP DEL` for an account that never negotiated anything, and for a
+    /// capability never held, are both nothing rather than a panic.
+    #[test]
+    fn withdrawing_what_was_never_held_is_quiet() {
+        let rt = Runtime::new();
+        rt.revoke_irc_caps("nobody", "away-notify");
+        rt.grant_irc_caps("a", "server-time");
+        rt.revoke_irc_caps("a", "batch");
+        assert!(rt.irc_has_cap("a", "server-time"));
+    }
+}
+
 #[cfg(test)]
 mod rename_tests {
     use super::*;