From 9629b7d93812f50dae574fb8c9a70f28d778d9c0 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 2 Aug 2026 22:07:30 +0200 Subject: [PATCH] Write down how the phone will add hosts, before it adds any MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The + button the design has asked for twice needs three things that do not exist: a tag, a group's parent, and a group's defaults. Two of them are refused on the record — HostGroupSecret argues groups are flat, and the theme argues an unused style is a claim the control exists. So the plan goes in first, with the decisions and the reasons. Tags become a real item over the reserved slot, a host names them, and membership merges through ThreeWayMerge.Map rather than as a whole value, so two people tagging one host both keep theirs — which is what HostTag was going to buy. Group defaults inherit rather than copy, shown as the field's placeholder, which is what makes editing a group afterwards mean anything. A parent arrives with a visited-set walk, because with inheritance a cycle is no longer an undrawable sidebar — it is a shell that never opens. Also written down: that Port has to go nullable and everything that touches, the byte pin it will trip, and the schema-version branch whose absence would make an inheriting host unreadable rather than read-only on every client that has not been upgraded. Co-Authored-By: Claude Opus 5 (1M context) --- docs/adding-hosts-on-the-phone.md | 294 ++++++++++++++++++++++++++++++ 1 file changed, 294 insertions(+) create mode 100644 docs/adding-hosts-on-the-phone.md diff --git a/docs/adding-hosts-on-the-phone.md b/docs/adding-hosts-on-the-phone.md new file mode 100644 index 0000000..f472e82 --- /dev/null +++ b/docs/adding-hosts-on-the-phone.md @@ -0,0 +1,294 @@ +# Adding hosts on the phone + +The phone can read a keychain and connect through it. It cannot put anything in one. This document is the +plan for the change that fixes that, and it is written to be picked up cold — the decisions, the reasons, the +ordered work, and the traps that are already known. + +> **Status: planned, not started.** Nothing below is built. The only thing in the tree is this file. + +## What was asked for + +A floating `+` at the bottom right of the phone's HOSTS screen, opening a menu at the bottom offering **new +host** or **new group**. + +A **host** carries: which vault it is filed into (personal or a team's), an alias, a hostname or IP address, a +group, tags, an SSH port, and a username with either a password or an SSH key. + +A **group** carries: an alias, an optional parent group, and a default SSH port, username and +password-or-key — **defaults for the hosts inside it**. + +## The shape of the problem + +Three of those fields do not exist at any layer, and one of them is refused on the record. + +| Asked for | What exists today | +| --- | --- | +| The `+` and the editors behind it | Nothing. `Theme/Phone.axaml` has no `.fab` class, and its comment says an unused style would be "a claim that the control exists somewhere". `HostsScreen.axaml` and `ConnectionsScreen.axaml` both tell the user, in prose on screen, that hosts are created on the desktop and sync down. | +| Vault picker | **Built.** `VaultViewModel.TargetVaults` / `SelectedTargetVault` / `HasVaultChoice`, hidden at one vault. The desktop's `VaultScreen.axaml` already draws it. | +| Alias, hostname, port, username, key-or-password, group | **Built**, in the shared `VaultViewModel` host editor — `EditorLabel`, `EditorHostname`, `EditorPort`, `EditorUsername`, `EditorAuthenticationChoices`, `EditorGroupChoices`, `SaveHostCommand`. The phone has never bound any of it. | +| Tags | **Nothing.** `SyncEntityType.Tag = 5` and `HostTag = 6` are reserved slots with nothing behind them. `HostSecret` has no tag field. | +| A group's parent | **Refused on the record.** `HostGroupSecret`'s own remark says groups are flat because two clients can each re-parent A under B and B under A offline, a scalar merge accepts both, and the result is a cycle no reader can draw and the server cannot see, because it is inside the payload. | +| A group's defaults | **Nothing.** `HostGroupSecret` has exactly one field, `Label`. | + +So the phone half is mostly wiring. The domain half is three new capabilities, and one of them overturns a +written decision. + +## The three decisions + +**Tags are a real vault item, and a host names them.** A tag is a shared, reusable thing — the point of it is +to put the same tag on twenty machines and filter by it later — so it needs an identity of its own, not a +string repeated inside twenty payloads. `SyncEntityType.Tag = 5` becomes a live item kind. Membership is a +`TagIds` set on `HostSecret`, **not** the reserved `HostTag = 6` join. + +The reason to skip the join is that the merge does not need it. The argument for `HostSecret.GroupId` living on +the host is that filing two hosts into one group must be two writes to two items; the same holds here. What a +join would have bought beyond that is two machines tagging *the same* host without one of them losing — and +`ThreeWayMerge.Map` already gives that. It resolves a keyed collection key by key, honouring presence and +absence, which is set semantics with removals. `HostOptions` is merged through it today via `ToNameMap()`, and +a tag set keyed by tag id is the same shape. So `HostTag = 6` stays reserved and unused, and the second run +through the item-kind checklist is not spent. + +**Group defaults are inherited, not copied.** A host that leaves a field blank uses its group's value; a host +that fills one in overrides it. The editor shows the group's value as the field's placeholder, so the form +says what leaving it blank will get you. Copying the values into the host at create time was the alternative +and it was rejected: it makes a group a one-shot template rather than a live default, and changing a group +would then leave every host that had ever been created under it pinned to the old value. + +**Groups get a parent, and the resolver breaks cycles.** `HostGroupSecret` gains `ParentId`. The editor refuses +a parent that is already a descendant, which stops a cycle being made here; the resolver walks with a visited +set and stops at a repeat, which contains a cycle that arrives from somewhere else. A cycle therefore degrades +to a group that reads as a root — flat headings, defaults unresolved past that point — instead of hanging the +connect path, and clearing the parent in the editor is the repair. **This is the load-bearing part of the +decision:** with inheritance the chain is walked at connect time, so an unguarded cycle is not an undrawable +sidebar, it is a shell that never opens. + +`HostGroupSecret`'s "No parent. Groups are flat." remark and `HostGroupSecretTests`' class remark both become +false prose and must be rewritten to say what replaced the argument, not deleted. Same for the four rows in +`docs/design-import-gaps.md` recording the phone's missing `+`. + +## What the payload becomes + +``` +HostSecret HostGroupSecret + Label Label + Hostname ParentId ← new, optional + Port int? — null inherits DefaultPort ← new + Username null inherits DefaultUsername ← new + Notes DefaultSshKeyId ← new + JumpHostIds DefaultCredentialId ← new + Options + SshKeyId null inherits TagSecret + CredentialId null inherits Label + GroupId null = ungrouped + TagIds ← new + RelayEnabled +``` + +`Port` becoming `int?` is the change with the widest blast radius, and it is unavoidable: `int` with a default +of 22 has no way to say "I have no port of my own". `HostSecret.DefaultPort = 22` stays, as the last fallback +after the chain runs out. + +### Three states where there were two + +`SshKeyId` and `CredentialId` both null currently *means* "ask for a password each time" — a decision, not an +absence. `AuthenticationChoice`'s own remark argues at length that the two must never be conflated. +Inheritance adds a third state, so the picker needs an explicit **"Inherit from group"** entry beside +**"Password (ask each time)"**, and `Bound(...)` needs to distinguish them. `Username` has the same problem: +null means "no username" today and is refused at connect; it has to come to mean "inherit", with "no username" +still reachable and still refused. + +Mutual exclusion moves with it. `HostSecret.TryValidate` enforces "a key or a credential, never both" per +record; a host naming a credential under a group naming a key is two individually valid records that resolve to +two bindings. **Exclusion has to be enforced at resolution**, host-over-group, and the resolver must never +return both. + +## The order to build it in + +Each step compiles and its tests pass before the next one starts. Steps 1–3 are the domain and are +prerequisites for everything; step 4 is the only user-visible one. + +### 1. `HostGroupSecret` grows + +`src/DodoSSH.Client.Domain/HostGroupSecret.cs`, `HostGroupSecretCodec.cs`, `HostGroupSecretMerge.cs`. + +- Add `ParentId`, `DefaultPort`, `DefaultUsername`, `DefaultSshKeyId`, `DefaultCredentialId`. +- `TryValidate`: reject `Guid.Empty` for each id, reject a `ParentId` equal to the group's own id where that is + knowable, apply the same 1–65535 range rule to `DefaultPort`, and refuse a group naming both a default key + and a default credential. +- **The codec needs the host codec's rule, which it does not have today.** `HostGroupSecretCodec` stamps + `CurrentSchemaVersion` unconditionally. Copy `HostSecretCodec.SchemaVersionFor` — a *maximum over the fields + actually present*, not a ladder — or upgrading one machine makes every group read-only on every other. A + group carrying none of the new fields must still encode at version 1, byte for byte. +- `HostGroupSecretMerge` gains the five fields, each a scalar. +- Add a group byte pin mirroring `AddingTheKeyField_DidNotChangeTheBytesOfAHostWithoutOne`. The group codec has + none, and the same "old vaults must re-encode identically" argument applies to it. + +### 2. The `Tag` item kind + +`SyncEntityType.Tag = 5`, `CryptoSpec.AadResourceType.Tag = 8`, `ChangeEntityType.Tag = 5` and the +`PublicAPI.Unshipped.txt` line **already exist** — no contract change, no crypto-spec change, no `docs/crypto.md` +change. `LocalCacheProtector`'s AAD switch already maps `Tag`. + +New: `TagSecret.cs`, `TagSecretCodec.cs`, `TagSecretMerge.cs` in `DodoSSH.Client.Domain`; `TagCipher.cs`, +`TagRepository.cs` in `DodoSSH.Client.Sync`. Edited: `Client.Sync/ItemKinds.cs` (registry entry + `TagKind`), +`VaultSession.cs` (a `Tags` repository beside `HostGroups`), `Domain/Hosts.cs` (`VaultTag`), +`Infrastructure/DodoDbContext.cs`, `Infrastructure/Configurations/HostAndSyncConfigurations.cs`, +`Api/Features/Sync/ItemKinds.cs` (registry entry + server `TagKind`). + +**A server migration is required** — a tag is its own table. Generate it, do not hand-write it: +`dotnet ef migrations add AddTagItem --project src/DodoSSH.Infrastructure`, committing all three resulting file +changes. **No client migration**: `ClientCacheContext` has one `item` table keyed by `(VaultId, EntityType, +EntityId)`, so a new kind is new rows. + +Four traps, all of which pass a naive test suite: + +- **`TagCipher` must say `AadResourceType.Tag` as a constant, never cast from `SyncEntityType.Tag`.** Tag is 5 + on the wire and 8 in the crypto enum. A cast seals tags under `Credential`, which encrypts and decrypts + perfectly on the machine that wrote it and is a spec violation nothing notices until another implementation + refuses the item — and the AAD is frozen into stored ciphertext. `AadResourceTypeTests` is the test that + catches this, and it only catches it once `PinnedPairs` and `SealSample` have a Tag arm. +- **The `ItemKinds.Registry` entry is what `SyncEngine` pulls with.** `SyncedTypes` is projected from it. Miss + it and tags encrypt, merge and list perfectly on the machine that made them and are never once requested from + the server. +- **`TagKind.Fields` returns `null`, not an empty `SyncPlaintextFields`.** An empty record still serialises + `relayEnabled: false`, which invites a reader to believe the type has a relay setting that is off. +- **`ValidateFields` must refuse everything, with reasons.** There is no default; an unimplemented one accepts + whatever arrives. `RelatedId` matters more here than it did for a group — it is the field a future `HostTag` + would reach for, and `SyncPlaintextFields` is frozen so it cannot be removed. Follow `SnippetKind` and + `ObjectStoreKind`, which are more complete than `HostGroupKind`. + +### 3. `HostSecret` grows, and `Port` goes nullable + +`HostSecret.cs`, `HostSecretCodec.cs`, `HostSecretMerge.cs`, plus a new `TagSet` value type and a new resolver. + +- `TagSet` is a structural-equality wrapper in the shape of `JumpChain` — a plain `IReadOnlyList` on a + record gets reference equality from the compiler-generated `Equals`, which would make every host read as + changed on every sync pass and two identical edits register as a conflict. Unlike `JumpChain` it is a *set*: + order carries no meaning, so it sorts and dedupes, and it exposes a `ToIdMap()` for the merge. +- `HostSecretMerge` merges `TagIds` through `ThreeWayMerge.Map`, per tag, so two people each adding a different + tag to one host both keep theirs. It is not a scalar; a whole-value merge here would silently drop one side. +- `Port` becomes `int?`. `TryValidate`'s `is < 1 or > 65535` is already false for null, which is the wanted + behaviour but is silent — say so in a comment, and fix the message, which interpolates `{Port}` and renders + empty for null. +- **`SchemaVersionFor` needs branches for both new fields, and this is the one that loses data if forgotten.** + A host with `Port = null` written at version 1 omits the property (`WhenWritingNull`); an older client + deserialises `int Port` as 0, `TryValidate` refuses it, and the item reads as **unreadable rather than + read-only** — gone, not locked. Add `PortInheritSchemaVersion = 5` and `TagIdsSchemaVersion = 5` (or 5 and 6) + and make both bump the maximum. +- **`AddingTheKeyField_DidNotChangeTheBytesOfAHostWithoutOne` pins the exact bytes** of a minimal host, + including `"port":22`. A host that now inherits its port drops that key. Keep the pinned literal for a host + with an explicit port, and add a companion for the inheriting one — do not edit the old literal to match + whatever the code produces. +- `TagIds` must serialise **last** in `HostPayloadDocument`, and empty must serialise as absent, or every host + in every vault looks changed on the first sync after the upgrade. + +New file: the resolver. Given a host and the group list, walk `GroupId` then `ParentId` with a visited set, +returning resolved port, username and binding, plus which group each came from — the editor needs the second +half for its placeholders. It must never return both a key and a credential. + +### 4. The shared view model, and the two heads + +`VaultViewModel` is where nearly all of it lands, and both heads get it at once because both bind the same +properties. + +The choke points, from the connect-path trace: + +- **`TryBuildAuthentication(HostSecret, …)` and `TryBuildConnectionRequest(HostSecret, …)`** both take a bare + `HostSecret` — this signature is where group context is currently lost. Both must take the resolved value. + This is the single highest-leverage change: it is the only auth resolution in the product, and both heads + and both transports go through it. +- The credential-username fallback becomes three levels: credential → host → group chain. +- `Complete(...)` must refuse on an empty username only *after* the chain has been consulted. +- `HostRowViewModel.Address`, `Dialled`, `TransfersViewModel`'s connected-to label and + `TerminalWorkspace.Describe` must show the **dialled** address. `MainWindowViewModel.Rank` searches + `Address`, so an unresolved one makes hosts unfindable by their real port. +- **`HostFields.From` in `HostCipher` must emit the resolved port.** This is the one place where an unresolved + read is a wrong wire rather than a wrong label: it fills the relay's plaintext columns, which is what the + *server* dials. A relay host with a null port also fails server validation outright. +- Host-key pinning keys on host and port. `ForgetHostKeyAsync` and the "pin in use" endpoint set read + `row.Host.Port` directly — a host inheriting 2222 pins under 2222 while both look under 22. +- `HostRowViewModel.Authentication`, `SelectedHostAsksForAPassword` (in **both** `VaultViewModel` and + `TransfersViewModel`) and `SelectedHostAuthenticationNote` all switch on the host's own two ids and must + switch on the resolved binding, with new wording for a binding that came from a group. +- **`HostsBoundTo`** counts hosts naming a key explicitly. A key deleted while only a *group* names it warns + nobody and then refuses every host under that group at connect time. It must walk groups too. +- The editor: load must distinguish "unset" from "explicitly this", `BuildHost` must preserve that in both + directions, and each field needs the resolved value as its placeholder. +- `ImportViewModel.IsAlreadyPresent` compares port and username directly, so a stored host inheriting 22 will + never match an imported host at 22 and the import screen re-offers duplicates. Decide whether an imported + block with no `Port` should inherit rather than pin 22 — `SshConfigResolver` returns `DefaultPort` today. + +### 5. The phone + +`Views/HostsScreen.axaml` and `Theme/Phone.axaml`. No csproj edit — Avalonia globs `**/*.axaml`. Compiled +bindings are on, so a binding to a property that does not exist is a build error; `TreatWarningsAsErrors` is on +repo-wide, so any XAML warning fails CI. + +- A `Button.fab` style, and the comment that currently refuses one rewritten to say what it now opens. +- The `+` needs a `Panel` wrapping the screen's root `Grid` so it can sit bottom-right without moving. Placing + it inside the list row would put it above the connect bar, which appears and disappears with the selection. +- The bottom sheet follows `HostKeySheet.axaml`, the head's one existing sheet: a scrim + (`Border Background="#9E0E1220"`, a literal because the palette holds no alpha surface), a bottom-aligned + `Border` with `CornerRadius="22,22,0,0"`, a decorative grab handle, and **no tap-to-dismiss** unless it is + wired by hand, because nothing on this head implements it. +- **Scrim reach is a real choice.** Declared inside `HostsScreen`, a scrim darkens the screen area only and the + 64px nav bar stays lit and tappable. To cover the display the sheet has to sit in `PhoneShell.axaml` beside + `HostKeySheet`, which is exactly why that control lives there. +- **The back gesture needs a new guard**, above the `switch` in `PhoneShell.axaml.cs`, in the shape of the + host-key guard: back should close the sheet or the editor rather than leave the screen. The comment on that + method states an invariant — the two switch cases are the membership tests of `IsVaultsSurface` and + `IsConnectionsSurface` and must stay in step — so add a guard, do not extend the switch. +- The editors follow `SnippetsScreen.axaml`: a `Border Classes="card"` in the same grid row as the list, + toggled on `IsEditing`, with the list toggled on `!IsEditing`. There is no dialog, no modal and no + editor-screen anywhere on this head, and the reason is written down — a form stacked over the list hides what + it is about. +- Editing an existing host comes nearly free: the shared `IsEditing` panel serves both, so the connect bar gets + an EDIT button beside CONNECT. Not asked for, but a `+` that adds hosts with no way to correct one is a + strange thing to ship. + +### 6. Tests + +House style: `Subject_Predicate`, articles included, the predicate smuggling in the reason +(`_RefusesRatherThanFallingBackToThePassword`). No `Async` suffix on test methods; always on private helpers. +Every class carries a remark naming the class of failure it exists to catch. xunit.v3 + Shouldly, with the +because-string used as prose. + +- Three shared builders need the new fields: `Domain.Tests/HostFactory.cs`, `Sync.Tests/SyncHarness.cs`, + `Sync.Tests/HostCipherTests.cs`. There is no shared group factory and with five new fields there should be. +- Three guard tests fail by design and that is what they are for: + `HostSecretMergeTests.EveryScalarField_IsRoutedThroughAMerge` (note `GroupId` is missing from its `with` + block today — an existing gap worth closing while there), + `ValueSemanticsTests.TryValidate_RejectsWhatCannotBeStored`, and the two "groups are flat" remarks. +- `ItemKindsTests.ThePullFilterNamesEveryTypeThisBuildSynchronises` is an ordered list — add Tag in registry + order. `AadResourceTypeTests` needs `PinnedPairs`, a `SealSample` arm and a `NewTag()` builder. +- `SyncEndpointTests.AHostGroupCarryingAParent_IsRejected` asserts the server refuses a plaintext `ParentId` + with a reason containing "flat". Putting the parent **inside the payload** does not break it mechanically, + but its stated reason becomes wrong. It should survive with a rewritten comment — the parent lives in the + payload, the plaintext column stays refused, and ADR 0004 is why. Decide that deliberately rather than + letting it drift. +- `Push_AnUnsupportedEntityType_IsInvalidNotAFailedBatch` picks the first type with no kind registered, very + likely Tag today. It self-heals to `HostTag` via `Assert.SkipWhen`. +- `App.Layout.Tests` measures pixels and bends rather than breaks. Extend the seeders — nesting and a tag row + change what the widest realistic content is — rather than the assertions. `ScreenLayoutTests.cs:1310` pins + `ChangedFields = "Hostname, Port, Username, Options, Group"` as a literal; add `Tags` to keep it measuring + the worst case. +- `ShellFlowTests.ReadyToConnectAsync` gives its host both a username and a port, so the inheritance tests need + a host with neither, or the group is never the source. + +## Found on the way, and out of scope + +`LocalCacheProtector`'s AAD switch is missing `ConnectionLogEntry`, `ActivityLogEntry` and `ObjectStore`, so +`ConflictStore.Record` throws `ArgumentOutOfRangeException` on a conflict for any of those three. Pre-existing, +unrelated to any of this, and `Tag` is already in that switch. Worth a separate fix. + +## Prose that becomes false + +Not a tidy-up — these are the places the codebase currently tells a user or a maintainer that this feature is +deliberately absent, and each states a reason that will no longer hold. + +- `Theme/Phone.axaml` — the "No floating action button" block. +- `Views/HostsScreen.axaml` — the v2 and v3 notes on the missing `+`. +- `Views/ConnectionsScreen.axaml` — "hosts are created on the desktop and sync down", rendered on screen. +- `HostGroupSecret.cs` and `HostGroupSecretTests.cs` — "No parent. Groups are flat." +- `docs/design-import-gaps.md` — the **Add host** row, the two floating-button rows, the "Tag / HostTag still + reserved and unused" paragraph, and the tag-chips and groups-on-a-team's-hosts rows in the Hosts table. -- 2.54.0