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<Guid>` 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.