Android: split the Spelunk sign-in and the phone's own sign-in out of ComposeShell, which MA0051 found too long

The sign-in rework added the SpelunkAccounts argument to ComposeShell and took it to 67 lines, over
the analyzer's 60, which failed the android head job of df64012. The two delegates' construction moves
into helpers of their own, with the comments that explained them; nothing they do changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs/manual-checks.md

@@ -3538,3 +3538,16 @@ Register a device, sync something, then `sudo dnf remove dodossh`.
 make an uninstall delete a user's un-synced work — the Linux spelling of the mistake ADR 0013 decision 2
 exists to prevent. Files left behind under `/opt/DodoSSH` are files the package did not own, which means
 something wrote into the install directory at runtime and would survive a reinstall.
+
+### 19.13 A copy at the same path that rpm does not own is offered nothing
+
+On a distribution without rpm — Arch, say, with a locally built package that puts the nightly tree at
+`/opt/DodoSSH.Nightly` — open PREFERENCES → UPDATES.
+
+**Pass:** **CHECK NOW** is disabled and the row says this copy was installed outside the `dodossh-nightly`
+RPM and should be updated with the package manager that installed it. `~/.local/share/dodossh.nightly/updates/`
+stays empty however long it runs.
+
+**Failure means:** a banner offering a nightly is the path being taken as proof the RPM is installed, or an
+rpm that cannot answer being read as an old version — the fault this check was written for, which offered
+every nightly as newer and fetched a 56 MB package the machine could never install, every six hours.

src/DodoSSH.Client.Android/App.axaml.cs

@@ -246,11 +246,7 @@ public sealed partial class DodoSshApp : Avalonia.Application
             knownHosts,
             deviceKeys,
 
-            // Difference 4: the system browser by intent, and the response by intent too rather than on a
+            SignInThroughBrowser(browser),
-            // loopback socket. See AndroidAuthorization for why the desktop's listener is not reused.
-            async (url, cancellationToken) => await ServerConnection
-                .SignInAsync(url, browser, TimeProvider.System, cancellationToken, ConfigureAndroidOidc)
-                .ConfigureAwait(false),
 
             TimeProvider.System,
             connections,
@@ -273,14 +269,7 @@ public sealed partial class DodoSshApp : Avalonia.Application
 
             updates: updates,
 
-            // Signing in to Spelunk comes back the way signing in to DodoSSH does — by intent, to the same
+            spelunkAccounts: PhoneSpelunkAccounts(browser));
-            // registered scheme, at the dev.dodotech.dodossh:/callback Spelunk allows — and names this phone as
-            // the account's device list does, for difference 5's reason.
-            spelunkAccounts: new SpelunkAccounts(
-                browser,
-                path => new AndroidRedirectCallback(path),
-                TimeProvider.System,
-                PhoneEnvironment.DeviceName));
 
         // Started rather than awaited: framework initialisation must not block on a schema migration. The
         // view model shows its own progress and handles its own failures.
@@ -289,6 +278,23 @@ public sealed partial class DodoSshApp : Avalonia.Application
         return viewModel;
     }
 
+    /// <summary>
+    /// Difference 4: the system browser by intent, and the response by intent too rather than on a loopback
+    /// socket. See AndroidAuthorization for why the desktop's listener is not reused.
+    /// </summary>
+    private static MainWindowViewModel.SignInHandler SignInThroughBrowser(AndroidBrowserLauncher browser) =>
+        async (url, cancellationToken) => await ServerConnection
+            .SignInAsync(url, browser, TimeProvider.System, cancellationToken, ConfigureAndroidOidc)
+            .ConfigureAwait(false);
+
+    /// <summary>
+    /// Signing in to Spelunk comes back the way signing in to DodoSSH does — by intent, to the same registered
+    /// scheme, at the dev.dodotech.dodossh:/callback Spelunk allows — and names this phone as the account's
+    /// device list does, for difference 5's reason.
+    /// </summary>
+    private static SpelunkAccounts PhoneSpelunkAccounts(AndroidBrowserLauncher browser) =>
+        new(browser, path => new AndroidRedirectCallback(path), TimeProvider.System, PhoneEnvironment.DeviceName);
+
     /// <summary>
     /// Difference 4: where the authorization response comes back to.
     /// </summary>

src/DodoSSH.Client.App/Platform/LinuxUpdateChannel.cs

@@ -127,17 +127,61 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
         this.installedVersion = installedVersion;
         this.handToInstaller = handToInstaller;
 
-        packageName = string.Equals(channel, "nightly", StringComparison.Ordinal)
+        packageName = PackageFor(channel);
-            ? "dodossh-nightly"
+    }
-            : "dodossh";
+
+    /// <summary>The channel for a copy under the RPM's install root, if rpm says the package is there.</summary>
+    internal static IUpdateChannel ForInstall(string channel) =>
+        ForInstall(channel, InstalledRpmVersion, () => new LinuxUpdateChannel(channel));
+
+    /// <summary>
+    /// The channel for a copy under the RPM's install root, with rpm supplied, which is how the tests
+    /// reach it.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// <b>The path says where this copy is and only rpm says who put it there.</b>
+    /// <c>UpdateChannels.LinuxChannel</c> has already established the first, and on an RPM distribution
+    /// that is the whole story. It is not on any other: a locally built pacman or dpkg package that mirrors
+    /// the spec puts the same tree at the same path, and this channel then offered every nightly as newer
+    /// than a version nobody could read, fetched a package that machine has no way to install, and did it
+    /// again every six hours.
+    /// </para>
+    /// <para>
+    /// Asked once, at composition, for the reason the rest of this decision is: who installed this copy
+    /// does not change while it runs. It costs one <c>rpm</c> launch at start on a machine whose path
+    /// already matched, and nothing at all anywhere else.
+    /// </para>
+    /// </remarks>
+    /// <param name="channel">The build's channel, which names the package.</param>
+    /// <param name="installedVersion">What rpm says is installed under a package name, or null.</param>
+    /// <param name="whenOwned">The channel to use when rpm does own this copy.</param>
+    internal static IUpdateChannel ForInstall(
+        string channel,
+        Func<string, string?> installedVersion,
+        Func<IUpdateChannel> whenOwned)
+    {
+        ArgumentException.ThrowIfNullOrEmpty(channel);
+        ArgumentNullException.ThrowIfNull(installedVersion);
+        ArgumentNullException.ThrowIfNull(whenOwned);
+
+        var package = PackageFor(channel);
+
+        return installedVersion(package) is null
+            ? new UnavailableUpdateChannel(
+                $"This copy of DodoSSH was installed outside the {package} RPM — rpm has no record of that "
+                + "package here — and an RPM is the only kind of update it can fetch. Update it with the "
+                + "package manager that installed it.")
+            : whenOwned();
     }
 
     /// <inheritdoc />
     /// <remarks>
     /// True unconditionally, because whether this copy is one that can be updated was already decided by
-    /// <see cref="UpdateChannels.ForThisMachine"/> — a build run from a checkout never reaches this class
+    /// <see cref="UpdateChannels.ForThisMachine"/> and <see cref="ForInstall(string)"/> — a build run from
-    /// at all. The same split Velopack's side uses, where <c>IsInstalled</c> is checked once at
+    /// a checkout, or installed by something other than rpm, never reaches this class at all. The same
-    /// composition rather than at every call site.
+    /// split Velopack's side uses, where <c>IsInstalled</c> is checked once at composition rather than at
+    /// every call site.
     /// </remarks>
     public bool IsSupported => true;
 
@@ -184,9 +228,20 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
                 $"The {channel} release's {manifestName} could not be read as a manifest.");
         }
 
+        // ◆ No answer from rpm is a failure, never a version. ForInstall only built this class because
+        // rpm answered, so silence now is a package removed under a running process or an rpm that
+        // stopped working — and any stand-in number would make every published build look newer and
+        // fetch a package for a machine that cannot install it, which is what this once did.
+        if (installedVersion(packageName) is not { } installed)
+        {
+            throw new InvalidOperationException(
+                $"rpm has no record of {packageName}, so there is no installed version to compare the "
+                + $"{channel} release against.");
+        }
+
         // The one place null is returned, and it means what null is documented to mean: this build is
         // current. Compared the way rpm compares, because rpm is what accepts or refuses the install.
-        if (RpmVersion.Compare(manifest.RpmVersion, installedVersion(packageName) ?? "0") <= 0)
+        if (RpmVersion.Compare(manifest.RpmVersion, installed) <= 0)
         {
             return null;
         }
@@ -324,6 +379,12 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
             ? $"{RepositoryApi}/releases/tags/nightly-desktop"
             : $"{RepositoryApi}/releases/latest";
 
+    /// <summary>The package a channel's builds are published as.</summary>
+    private static string PackageFor(string channel) =>
+        string.Equals(channel, "nightly", StringComparison.Ordinal)
+            ? "dodossh-nightly"
+            : "dodossh";
+
     private static string? Asset(LinuxForgeRelease? release, string name) =>
         release?.Assets?.FirstOrDefault(
             asset => string.Equals(asset.Name, name, StringComparison.Ordinal))?.DownloadUrl;
@@ -365,9 +426,11 @@ internal sealed class LinuxUpdateChannel : IUpdateChannel
     /// <c>scripts/ci-linux-rpm.sh</c> performs, and two copies of one rule is a pair that can disagree.
     /// </para>
     /// <para>
-    /// A machine that cannot answer gets null, which the caller reads as <c>0</c> and makes every
+    /// A machine that cannot answer gets null — no <c>rpm</c> at all, or one with no such package — and
-    /// published build look newer. That is the wrong way to fail, chosen deliberately: the failure is then
+    /// null means "rpm does not own this copy", never a version. It used to be read as <c>0</c>, on the
-    /// an install PackageKit declines rather than a security fix that never arrives.
+    /// argument that the worst case was an install PackageKit declined. That holds only where PackageKit
+    /// installs RPMs: on any other distribution it made every published build look newer, forever, and
+    /// fetched a package that machine could not install. See <see cref="ForInstall(string)"/>.
     /// </para>
     /// </remarks>
     private static string? InstalledRpmVersion(string packageName)

src/DodoSSH.Client.App/Platform/VelopackUpdateChannel.cs

@@ -66,21 +66,23 @@ internal static class UpdateChannels
         }
     }
 
-    /// <summary>The Linux channel, if this copy is one a package manager put here.</summary>
+    /// <summary>The Linux channel, if this copy is one the RPM put here.</summary>
     /// <remarks>
     /// <para>
     /// <b>The question Velopack answers with <c>IsInstalled</c>, answered by looking at where this process
-    /// lives.</b> The RPM owns <c>/opt/DodoSSH</c> or <c>/opt/DodoSSH.Nightly</c> — the spec puts it there
+    /// lives and then asking rpm.</b> The RPM owns <c>/opt/DodoSSH</c> or <c>/opt/DodoSSH.Nightly</c> — the
-    /// and nothing else writes to <c>/opt</c> under those names — so a build running from anywhere else is
+    /// spec puts it there — so a build running from anywhere else is a checkout somebody started, an
-    /// a checkout somebody started, an extracted archive, or a tree copied by hand. None of those can be
+    /// extracted archive, or a tree copied by hand. None of those can be updated by installing a package
-    /// updated by installing a package over them, and offering it would be an offer that cannot be carried
+    /// over them, and offering it would be an offer that cannot be carried out.
-    /// out.
     /// </para>
     /// <para>
-    /// Checked by path rather than by asking rpm which package owns the executable, which would be the
+    /// <b>The path is necessary and not sufficient.</b> It used to be the whole check, on the argument
-    /// more thorough answer and costs a process launch on every start for a distinction nothing acts on
+    /// that asking rpm cost a process launch for a distinction nothing acted on differently. Something
-    /// differently. The install root is a constant in two places — here and the packaging script — and
+    /// does: a locally built pacman package that mirrors the spec puts the same tree at the same path,
-    /// they are named in each other's comments.
+    /// and that copy was offered every nightly as an RPM it could never install. So a matching path is
+    /// then put to rpm — see <see cref="LinuxUpdateChannel.ForInstall(string)"/> — and the launch is paid
+    /// only where the path already matched. The install root is a constant in two places — here and the
+    /// packaging script — and they are named in each other's comments.
     /// </para>
     /// </remarks>
     [SupportedOSPlatform("linux")]
@@ -91,7 +93,7 @@ internal static class UpdateChannels
         var here = AppContext.BaseDirectory.TrimEnd(Path.DirectorySeparatorChar);
 
         return string.Equals(here, root, StringComparison.Ordinal)
-            ? new LinuxUpdateChannel(DesktopChannel.Name)
+            ? LinuxUpdateChannel.ForInstall(DesktopChannel.Name)
             : new UnavailableUpdateChannel();
     }
 }

src/DodoSSH.Client.App/Views/SettingsGeneralPage.axaml

@@ -84,7 +84,10 @@
                          IsVisible="{Binding Updates.Status, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />
               <TextBlock Classes="settingsrowcaption"
                          Text="This copy of DodoSSH cannot replace itself, so nothing above does anything. That is what a build run from a source checkout looks like, and also what a copy somebody unzipped by hand looks like — it is the installer that registers the update path."
-                         IsVisible="{Binding Updates.IsUnsupported}" />
+                         IsVisible="{Binding Updates.IsUnsupportedForTheUsualReason}" />
+              <TextBlock Classes="settingsrowcaption"
+                         Text="{Binding Updates.UnsupportedReason}"
+                         IsVisible="{Binding Updates.UnsupportedReason, Converter={x:Static StringConverters.IsNotNullOrEmpty}}" />
             </StackPanel>
           </Border>
 

src/DodoSSH.Client.Session/ClientUpdates.cs

@@ -141,9 +141,32 @@ public interface IUpdateChannel
 /// <see cref="UnavailableDeviceKeyStore"/>: the caller then has one shape to write against, and "no
 /// updater here" is a thing the interface can express rather than a case every call site has to remember.
 /// </para>
+/// <para>
+/// <b>One situation outside those four carries a <see cref="Reason"/>.</b> A Linux copy sitting exactly
+/// where the RPM puts it that no RPM owns — built locally and installed by another distribution's package
+/// manager. It looks installed, so the sentence about checkouts and unzipped copies would describe the
+/// wrong machine, and the person reading it needs telling which updater is theirs.
+/// </para>
 /// </remarks>
 public sealed class UnavailableUpdateChannel : IUpdateChannel
 {
+    /// <summary>Creates one for the situations that need no telling apart.</summary>
+    public UnavailableUpdateChannel()
+    {
+    }
+
+    /// <summary>Creates one whose situation is worth a sentence of its own.</summary>
+    /// <param name="reason">What to tell the user, as a sentence they can act on.</param>
+    public UnavailableUpdateChannel(string reason)
+    {
+        ArgumentException.ThrowIfNullOrWhiteSpace(reason);
+
+        Reason = reason;
+    }
+
+    /// <summary>Why this copy cannot replace itself, where that is not the usual why.</summary>
+    public string? Reason { get; }
+
     /// <inheritdoc />
     public bool IsSupported => false;
 

src/DodoSSH.Client.Shell/ViewModels/UpdateViewModel.cs

@@ -126,6 +126,7 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
         this.restart = restart;
 
         CurrentVersion = updates.CurrentVersion;
+        UnsupportedReason = (updates as UnavailableUpdateChannel)?.Reason;
         isAutomatic = settings.Read().AutomaticUpdateChecks;
         state = updates.IsSupported ? UpdateState.Idle : UpdateState.Unsupported;
     }
@@ -133,6 +134,16 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
     /// <summary>What this build calls itself.</summary>
     internal string CurrentVersion { get; }
 
+    /// <summary>Why this copy cannot replace itself, when the channel had something particular to say.</summary>
+    /// <remarks>
+    /// Null in the ordinary cases, where the preferences screen's own sentence about checkouts and unzipped
+    /// copies is the right one. See <see cref="UnavailableUpdateChannel"/> for the case where it is not.
+    /// </remarks>
+    internal string? UnsupportedReason { get; }
+
+    /// <summary>Unsupported, with nothing more particular to say than the screen's own sentence.</summary>
+    internal bool IsUnsupportedForTheUsualReason => IsUnsupported && UnsupportedReason is null;
+
     [ObservableProperty]
     private UpdateState state;
 
@@ -248,6 +259,7 @@ internal sealed partial class UpdateViewModel : ObservableObject, IAsyncDisposab
     {
         OnPropertyChanged(nameof(IsSupported));
         OnPropertyChanged(nameof(IsUnsupported));
+        OnPropertyChanged(nameof(IsUnsupportedForTheUsualReason));
         OnPropertyChanged(nameof(IsChecking));
         OnPropertyChanged(nameof(IsDownloading));
         OnPropertyChanged(nameof(IsReady));

tests/DodoSSH.Client.App.Tests/LinuxUpdateChannelTests.cs

@@ -86,19 +86,86 @@ public sealed class LinuxUpdateChannelTests : IDisposable
     }
 
     /// <summary>
-    /// A machine that cannot say what it has treats everything as newer.
+    /// A machine that cannot say what it has is told so, and is offered nothing.
     /// </summary>
     /// <remarks>
-    /// The wrong way to fail, chosen deliberately and for the reason the android channel gives: the cost is
+    /// This used to read the silence as <c>0</c> and offer everything, on the argument that the cost was
-    /// an install the package manager declines, and the cost of the other direction is a security fix that
+    /// an install the package manager declined. On a machine with no rpm at all — a pacman package of the
-    /// never arrives.
+    /// same tree at the same path — that was every nightly, every six hours, as a 56 MB download nothing
+    /// could install. Unknown is not older.
     /// </remarks>
     [Fact]
-    public async Task AnUnknownInstalledVersionTreatsEverythingAsNewer()
+    public async Task AnUnknownInstalledVersionThrowsRatherThanOfferingAnything()
     {
-        var channel = Channel(RandomNumberGenerator.GetBytes(16), published: "0.0.1~alpha.0.1-1", installed: null);
+        var handed = new List<string>();
+        var channel = Channel(
+            RandomNumberGenerator.GetBytes(16),
+            published: "0.0.1~alpha.0.200-1",
+            installed: null,
+            handToInstaller: handed.Add);
 
-        (await channel.CheckAsync(Token)).ShouldNotBeNull();
+        await Should.ThrowAsync<InvalidOperationException>(async () => await channel.CheckAsync(Token));
+
+        await Should.ThrowAsync<InvalidOperationException>(
+            async () => await channel.DownloadAsync(
+                new DodoSSH.Client.Session.AvailableUpdate("0.0.1-alpha.0.200"),
+                new Progress<int>(),
+                Token));
+
+        handed.ShouldBeEmpty();
+        Directory.Exists(Path.Combine(directory, "updates")).ShouldBeFalse();
+    }
+
+    /// <summary>
+    /// A copy rpm does not own gets no channel at all, and a sentence saying whose it is to update.
+    /// </summary>
+    /// <remarks>
+    /// The case that matters: the right path and no rpm — a locally built pacman package that mirrors the
+    /// spec. No binary and no such package both arrive here as null.
+    /// </remarks>
+    [Fact]
+    public void ACopyRpmDoesNotOwn_IsUnavailableAndSaysWhy()
+    {
+        var asked = new List<string>();
+
+        var updates = LinuxUpdateChannel.ForInstall(
+            "nightly",
+            package =>
+            {
+                asked.Add(package);
+
+                return null;
+            },
+            () => throw new InvalidOperationException("Built a channel for a copy rpm does not own."));
+
+        asked.ShouldBe(["dodossh-nightly"]);
+        updates.IsSupported.ShouldBeFalse();
+
+        var reason = updates.ShouldBeOfType<DodoSSH.Client.Session.UnavailableUpdateChannel>().Reason;
+
+        reason.ShouldNotBeNull();
+        reason.ShouldContain("outside the dodossh-nightly RPM");
+        reason.ShouldContain("package manager that installed it");
+    }
+
+    [Fact]
+    public void ACopyRpmOwns_GetsTheChannel()
+    {
+        var asked = new List<string>();
+        var owned = new FakeUpdateChannel();
+
+        var updates = LinuxUpdateChannel.ForInstall(
+            "release",
+            package =>
+            {
+                asked.Add(package);
+
+                return "0.0.1-1";
+            },
+            () => owned);
+
+        asked.ShouldBe(["dodossh"]);
+        updates.ShouldBeSameAs(owned);
     }
 
     [Fact]

tests/DodoSSH.Client.App.Tests/UpdateFlowTests.cs

@@ -80,6 +80,32 @@ public sealed class UpdateFlowTests : IDisposable
         Build().CurrentVersion.ShouldBe("0.4.2");
     }
 
+    [Fact]
+    public void ABuildThatCannotReplaceItself_ForAReasonOfItsOwn_SaysThatInstead()
+    {
+        var updates = new UpdateViewModel(
+            new UnavailableUpdateChannel("Update it with the package manager that installed it."),
+            new ClientSettingsStore(Paths),
+            TimeProvider.System,
+            () => 0,
+            restart: _ => Task.CompletedTask);
+
+        updates.IsUnsupported.ShouldBeTrue();
+        updates.UnsupportedReason.ShouldBe("Update it with the package manager that installed it.");
+        updates.IsUnsupportedForTheUsualReason.ShouldBeFalse();
+    }
+
+    [Fact]
+    public void ABuildThatCannotReplaceItself_ForTheUsualReasons_LeavesTheScreenItsOwnSentence()
+    {
+        channel.IsSupported = false;
+
+        var updates = Build();
+
+        updates.UnsupportedReason.ShouldBeNull();
+        updates.IsUnsupportedForTheUsualReason.ShouldBeTrue();
+    }
+
     [Fact]
     public async Task ABackgroundPassThatFindsNothing_SaysNothing()
     {