From 27bb1deb5ded4cc26678039633c16f71051af7e9 Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Tue, 4 Aug 2026 10:15:02 +0200 Subject: [PATCH 1/2] Say what the docs describe, in the order they describe it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tidy across docs/, and every change is either something that had drifted from the code or something a reader would trip over. Nothing here is a rewrite: the audit documents keep their audit-time text, because that is what the scope was decided against, and corrections are marked in place the way each file already marks them. The one that matters is crypto.md. Section 4.3's resourceType table stopped at 13 while CryptoSpec.AadResourceType has carried ConnectionLogEntry = 14, ActivityLogEntry = 15 and ObjectStore = 16 since the logs and the buckets shipped — and items have been sealed under all three. That file is normative, so a table three item kinds behind the code is the sort of gap somebody reimplementing this would build against. Nothing on the wire or on disk changes; the list now says what the code has been doing, and it notes the thing that makes it worth reading carefully — 14, 15 and 16 are the first members that are not a fixed offset from their SyncEntityType counterparts, because 12 and 13 closed a hole and pushed the two enums apart. manual-checks.md had Phase 3 in an order nobody could follow: 3.2b, then 3.2a, then 3.4a, then 3.3. The letters are not the problem — they exist so a check can keep its number for life, which is what lets HostGridTests cite 7.6 and platform-flags.md cite 3.6-3.8 — so the sections are reordered and the numbers are untouched, and the intro now says the convention out loud rather than leaving the next person to infer it from two orphaned letters. Phases 8 and 9 got the horizontal rule every other phase has. The Phase 3 preamble still said "a sidebar that now draws headings", which has been two different interfaces for a while: the desktop's grid of cards and the phone's flat list. 7.6 cited MovingAHostToAGroup_FilesItAndLeavesItSelected, which was renamed in the commit before this one. Every other Type.Member citation across all six files was checked against src/ and tests/ the same way; those two were the only ones pointing at nothing. design-import-gaps.md's v3 section described the grid of cards without saying it holds one level, and its Groups row still said the sidebar emits one heading per group — true of the phone and not of the desktop. The team-groups row said a host a teammate filed "shows under UNGROUPED", which is now only half the story and the interesting half is why: the chip resolves through groupsById, which is every readable vault, while the level resolves through Groups, which is the active vault alone — so the card sits at the outermost level with its group's name still on it. Verified against both call sites rather than reasoned about. android-port.md's destination inventory is the desktop as it stood when the audit was taken and is deliberately left that way, with a note at its head saying so and two inline corrections where a reader would otherwise be misled: the rail lost FILES and S3 to fixed tabs, and the hosts sidebar became a grid — which is exactly why the headings this head draws are the phone's alone now. adding-hosts-on-the-phone.md is a plan marked built, so its two open questions are answered in place: GroupId did get into the merge test's with block, and the server test that asserts a plaintext ParentId is refused survived under a name that says what it means. Its "found on the way" note is marked still open, because it is: LocalCacheProtector.For has no arm for ConnectionLogEntry, ActivityLogEntry or ObjectStore, so ConflictStore.Record still throws for all three. That outlived the phases that shipped them, which is the drift a note like that exists to prevent. No code changed, so the suite is untouched at 1522 passing. --- docs/adding-hosts-on-the-phone.md | 10 ++++- docs/android-port.md | 17 +++++++- docs/crypto.md | 13 ++++++- docs/design-import-gaps.md | 11 +++++- docs/manual-checks.md | 65 ++++++++++++++++++------------- 5 files changed, 83 insertions(+), 33 deletions(-) diff --git a/docs/adding-hosts-on-the-phone.md b/docs/adding-hosts-on-the-phone.md index ce1d417..8c95db9 100644 --- a/docs/adding-hosts-on-the-phone.md +++ b/docs/adding-hosts-on-the-phone.md @@ -299,7 +299,7 @@ because-string used as prose. `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), + block today — an existing gap worth closing while there) *(closed: `GroupId` is in that block now)*, `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. @@ -307,7 +307,9 @@ because-string used as prose. 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. + letting it drift. *(Decided that way, and the name says it now: + `SyncEndpointTests.AHostGroupCarryingAPlaintextParent_IsRejected`. The column is still refused and the + reason is the trust model rather than flatness.)* - `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 @@ -323,6 +325,10 @@ because-string used as prose. `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. +> **Still open.** All three are on `SyncEntityType` and on `CryptoSpec.AadResourceType`, and all three are +> still absent from `LocalCacheProtector.For` — so this outlived the phases that shipped the logs and the +> buckets, which is exactly the drift a note like this is meant to prevent. + ## 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 diff --git a/docs/android-port.md b/docs/android-port.md index f7f12fc..bb9e5d9 100644 --- a/docs/android-port.md +++ b/docs/android-port.md @@ -311,7 +311,11 @@ that appear rarely and matter most. Those are marked **◆**. - **Status bar** — the selected terminal's live dot and address, the vault's last status sentence, the sync label again, and the search hint. - **Nav rail** — eight destinations: `HOSTS FILES KEYS PINS SNIPS LOGS TEAM PREFS`. Five characters is a - desktop constraint, not a product one; the phone can use words. + desktop constraint, not a product one; the phone can use words. ⚠️ **Corrected:** the desktop's v3 moved + FILES and the S3 buckets out of the rail and onto fixed tabs beside the terminals, leaving seven — they are + the two destinations you *stay in* while something runs, and a rail entry is for somewhere you go and come + back from. The phone reached the same count by a different route: three bottom-bar destinations with the + rest behind Settings. - **Terminal strip** — always visible, above every screen. Tabs with a close cross *inside* each tab, a `+`, and a sentence when there are none. This is what makes a terminal a surface the window switches to rather than a screen you navigate away from, and it is the single most desktop-shaped idea in the product. @@ -319,10 +323,21 @@ that appear rarely and matter most. Those are marked **◆**. ### The nine destinations +> ⚠️ **This is the desktop as it stood when the audit was taken, and it is left that way on purpose** — it is +> the checklist the scope was decided against, so rewriting it would erase what was actually weighed. Two +> things have moved since, and both are noted where they belong below: the rail lost FILES and S3 to fixed +> tabs in the desktop's v3, and the hosts sidebar became a grid of cards. Neither changed what a phone needs +> somewhere to put, which is what this list is for. + **1. Hosts** — the list of machines, and what is known about the selected one. - *Sidebar:* filter box; group headings with a chevron and a count, **shown only when groups exist**; host rows carrying a connected dot, name, sync badge, address and one word for how it authenticates. + ⚠️ **Corrected:** the desktop replaced this column with a grid of cards and a right-hand drawer, and its + grid holds one level of the groups at a time — so the headings, their chevrons and UNGROUPED are now the + *phone's* alone, drawn from `SidebarRows`. That is what shipped here, and it is why this head flattens the + whole tree rather than navigating into it: there is no room for a row of group cards at 360dp and nothing + to open one into. See `docs/design-import-gaps.md` on the desktop's v3. - *Editor* (doubles as "add"): name, hostname, port, username, notes, one authentication picker covering typed password / key / stored credential, a group picker, a relay checkbox with the sentence explaining that relay puts the address on the server in plain text, and **◆ forget host key** — the only way back from diff --git a/docs/crypto.md b/docs/crypto.md index e7f16e5..b514afd 100644 --- a/docs/crypto.md +++ b/docs/crypto.md @@ -286,7 +286,7 @@ value can forge a field boundary. UUIDs must be serialised in RFC 4122 order — `1` User, `2` Device, `3` Vault, `4` Host, `5` Credential, `6` SshKey, `7` HostGroup, `8` Tag, `9` Snippet, `10` PortForward, `11` KnownHostKey, `12` HostTag, -`13` HostCredential. +`13` HostCredential, `14` ConnectionLogEntry, `15` ActivityLogEntry, `16` ObjectStore. > **Added 2026-07-29:** `12` and `13`. `Contracts.SyncEntityType` has listed `HostTag` and > `HostCredential` as syncable since the contract was frozen, but this table had no value for @@ -294,6 +294,17 @@ value can forge a field boundary. UUIDs must be serialised in RFC 4122 order — > implementation to need one would have had to invent a value or reuse a neighbour's. Append > only, and no such item has been stored. +> **Recorded 2026-08-04:** `14`, `15` and `16`. Unlike `12` and `13` these were not a hole — they +> were added to `CryptoSpec.AadResourceType` when the two logs and the buckets shipped, and items +> **have** been sealed under them. What was missing was this table, which had gone three item kinds +> without being updated. Nothing changes on the wire or on disk; the normative list now says what +> the code has been doing. +> +> **They are the first members that are not a fixed offset from their `SyncEntityType` +> counterparts** — the two enums drifted apart when `12` and `13` closed the hole above, so +> `ConnectionLogEntry` is `11` there and `14` here. Nothing casts between them, and +> `AadResourceTypeTests` pins each pairing by name for exactly that reason. + `0` means not applicable and is legal only where the table in §4.2 implies no resource. ### 4.4 What this prevents diff --git a/docs/design-import-gaps.md b/docs/design-import-gaps.md index c2b2a79..cf55bb8 100644 --- a/docs/design-import-gaps.md +++ b/docs/design-import-gaps.md @@ -105,6 +105,13 @@ the chrome, hosts and terminals, file transfer, the vault, teams, and preference > double-pressing one opens it, and a breadcrumb trail above the cards is where you are and the way back — > the same trail the transfers screen draws over a directory. > +> **And the grid holds one level of that trail rather than every host.** A host filed under a group is drawn +> inside that group and not also on the screen the group's card sits on, which is what makes a card a place +> instead of a label: while the level above held everything anyway, opening a group could only ever subtract. +> The find box is the one thing that crosses a boundary — it searches the open group and everything under it, +> so a machine can be found without first remembering where it was filed, and the chip on a card is what says +> which group a result came out of. See `VaultViewModel.Matches`. +> > **The type scale went up a point and the text ramp went white.** `#E3E7F4` was a blue-tinted white on > blue-black surfaces, which costs contrast twice — once for being darker than white and once for sharing > a hue with what it is drawn on. Every step of the ramp moved with the top, so the intervals the design @@ -298,9 +305,9 @@ caption buttons and window title drawn on top of the application's own — two s | Design element | Layer | What it would take | What ships instead | | --- | --- | --- | --- | | Tag chips (`nginx`, `eu`, `pg16`) | client-domain | A tag item type, and a way to put one on a host. | **Shipped.** `Tag = 5` is a live item kind and `HostSecret.TagIds` names them, merged per tag so two people tagging one host both keep theirs — which is what `HostTag = 6` was going to buy, so it stays reserved and unused. Chips are drawn on host rows on both heads and toggled in the host editor, which also creates one inline; renaming and deleting are a TAGS category on the keychain screen. The filter box still searches name, address and notes only — a chip is read rather than typed. | -| Groups `PRODUCTION` / `STAGING` / `PERSONAL` | client-domain | A host-group item type (`HostGroup = 4`, reserved) or a group field on `HostSecret`. | **Shipped**, as both: `VaultHostGroup` is a synced item kind and `HostSecret.GroupId` names one. Nested in the data and drawn flat — a group carries a `ParentId` and the defaults its hosts inherit, and the sidebar still emits one heading per group in label order with no indentation. A keychain with no groups renders exactly as it did before — one flat list, no headings. | +| Groups `PRODUCTION` / `STAGING` / `PERSONAL` | client-domain | A host-group item type (`HostGroup = 4`, reserved) or a group field on `HostSecret`. | **Shipped**, as both: `VaultHostGroup` is a synced item kind and `HostSecret.GroupId` names one. A group carries a `ParentId` and the defaults its hosts inherit, and the two heads draw the nesting differently on purpose. The desktop is a grid of cards holding one level at a time, the way a directory pane holds one directory; the phone has no room for a row of cards, so it flattens the whole tree into one heading per group in label order with no indentation. A keychain with no groups renders exactly as it did before on both — one flat list, no headings and no cards. | | Group badge `TEAM·PLATFORM` | server | **Built in M3.** | The vault's name on each row, and the personal vault ordered first. Not the team's name: two of a team's vaults would then carry the same badge and the badge would be naming the wrong thing. Distinct from the groups above, and deliberately so — a group is a shelf the user chose, a vault is who can read the item. | -| Groups on a **team's** hosts | client-domain | A vault id on each group row for rename and delete, and a way to tell two vaults' identically-named groups apart in a list with one heading per group. | **Half shipped, and the half that shipped had to.** Every readable vault's groups are now read into the resolution map, because a group lends a port, a username and a binding — so a host whose group went unread would silently dial 22 as nobody while the machine is on 2222 as `deploy`. A missing heading is cosmetic; a missing port is a connection to the wrong place. The editable list is still the active vault's alone, so a host a teammate filed still shows under UNGROUPED and cannot be renamed or unfiled from here. | +| Groups on a **team's** hosts | client-domain | A vault id on each group row for rename and delete, and a way to tell two vaults' identically-named groups apart in a list with one heading per group. | **Half shipped, and the half that shipped had to.** Every readable vault's groups are now read into the resolution map, because a group lends a port, a username and a binding — so a host whose group went unread would silently dial 22 as nobody while the machine is on 2222 as `deploy`. A missing heading is cosmetic; a missing port is a connection to the wrong place. The editable list is still the active vault's alone, so a group a teammate made has no card and no heading and cannot be renamed from here. A host filed into one is drawn at the outermost level of the desktop's grid and under UNGROUPED on the phone — reachable either way, which is the point — with the chip on its card still naming the group, because the label is resolved through the wide map while the level is decided from the narrow list. | | Per-host status dot, three colours | client-ssh | The amber state would mean "reachable but not connected", and nothing here ever probes a host. | Two states, both real: green when a terminal is open on that host, grey when not. | | `· ⤷ bastion-eu` in the host subtitle | client-ssh | **Jump hosts are data-only.** `HostSecret.JumpHostIds` is a `JumpChain` that is stored, encrypted, synced and three-way merged — and nothing reads it at connect time. `SshConnectionRequest` carries one host. | Omitted. The stored chain is preserved untouched by every edit. | | `SPLIT ⌘D` and side-by-side panes | client-ssh + ui | The renderer stacks panes and shows one (`terminal.css`: `.pane { position:absolute; inset:0; display:none }`). Tiling needs a real pane geometry and a splitter. | Omitted. Tabs ship instead, over the same one-WebView multiplexing. | diff --git a/docs/manual-checks.md b/docs/manual-checks.md index caa51eb..7af5f01 100644 --- a/docs/manual-checks.md +++ b/docs/manual-checks.md @@ -16,6 +16,12 @@ Three constraints put things on this list, and they are worth knowing before add Each item says what to do, what a pass looks like, and what a failure would mean. +**On the numbering.** A check keeps its number for life, because code comments and other documents cite them +— `HostGridTests` sends a reader to 7.6, `platform-flags.md` to 3.6–3.8. A check inserted later therefore +takes a letter rather than pushing its neighbours along: 3.2a and 3.2b sit between 3.2 and 3.3 and always +will. Add in the same way, and keep each one next to the check it belongs beside; a gap in the numbers means +a phase had nothing left for a person to do, which is the good outcome rather than an omission. + --- ## Phase 1 — the shell and the tab strip @@ -290,11 +296,12 @@ surface. Use the ← button, or drag the file to the local pane first. ## Phase 3 — Host groups and snippets -Two synced item kinds, a sidebar that now draws headings, and one new frame between the host process and the -renderer. The data half of all of that is covered: the payloads round-trip, the server refuses the plaintext -fields, the sidebar's grouping and the snippet policy are in `ShellFlowTests`, and both new screens are -measured. What is left here is the part that only exists inside a WebView, plus the two-machine cases no -single-process test can reach. +Two synced item kinds, two ways of drawing a group — the desktop's grid of cards, one level at a time, and +the phone's flat list under headings — and one new frame between the host process and the renderer. The data +half of all of that is covered: the payloads round-trip, the server refuses the plaintext fields, both ways +of drawing a group and the snippet policy are in `ShellFlowTests`, and both new screens are measured. What is +left here is the part that only exists inside a WebView, plus the two-machine cases no single-process test +can reach. ### 3.1 A keychain with no groups looks exactly as it did @@ -334,19 +341,6 @@ a filed host is worse than either: it is the box answering "no host matches that keychain has got. A full-width bar with a chevron between the cards is the old grouping coming back through `SidebarRows`; the desktop grid binds `VisibleHosts`. See `HostsScreen.axaml`. -### 3.2b Making something while standing inside a group - -Open a group, then press **+ NEW HOST**, and afterwards **+ NEW GROUP**. - -**Pass:** the host editor opens with that group already chosen in its group picker, and the saved host is on -the screen it was made on rather than somewhere the trail is not. The group editor likewise opens with that -group as its parent, so the new group is a card inside the one that is open. - -**Failure means:** anything created inside a group disappearing the moment it is saved. That is the papercut -a level-at-a-time grid comes with, and `NewHost` / `NewGroup` are where it is answered. Note the deliberate -difference between them: the host editor also takes a merely *selected* card as its group, the group editor -takes only the group that is open. - ### 3.2a A group inside a group, and the way back out Make two groups and file one under the other with the parent picker in the group editor. @@ -361,17 +355,18 @@ Pressing the **middle** crumb goes back one level rather than all the way out. the flat `Groups` is the phone's and the lookups'. A group that cannot be reached at all is worse and is the case `EffectiveParents` promotes: see 3.4a. -### 3.4a A parent that dangles, or a cycle · **needs two machines** +### 3.2b Making something while standing inside a group -Rare, and the reason it is here is that the repair for both is inside the group's own editor, which is -opened from its card. On A, file group X under group Y and sync. On B, sync, delete Y, sync. Back on A, sync. -For the cycle: with both machines offline, file X under Y on A and Y under X on B, then sync both. +Open a group, then press **+ NEW HOST**, and afterwards **+ NEW GROUP**. -**Pass:** in both cases every group still has a card at the outermost level, so every one of them can be -opened and its parent cleared. Nothing is missing from the grid, and the trail never loops. +**Pass:** the host editor opens with that group already chosen in its group picker, and the saved host is on +the screen it was made on rather than somewhere the trail is not. The group editor likewise opens with that +group as its parent, so the new group is a card inside the one that is open. -**Failure means:** a group drawn nowhere is a group that can never be repaired — the fix is locked inside the -state that needs fixing. See `VaultViewModel.EffectiveParents`. +**Failure means:** anything created inside a group disappearing the moment it is saved. That is the papercut +a level-at-a-time grid comes with, and `NewHost` / `NewGroup` are where it is answered. Note the deliberate +difference between them: the host editor also takes a merely *selected* card as its group, the group editor +takes only the group that is open. ### 3.3 Deleting a group with hosts in it @@ -394,6 +389,18 @@ the group picker shows "(a group that is no longer here)" and *keeps it selected **Failure means:** if the picker opened on "No group", saving has just unfiled the host — quietly, as a side effect of an unrelated edit. That is the case `BuildGroupChoices` adds the placeholder for. +### 3.4a A parent that dangles, or a cycle · **needs two machines** + +Rare, and the reason it is here is that the repair for both is inside the group's own editor, which is +opened from its card. On A, file group X under group Y and sync. On B, sync, delete Y, sync. Back on A, sync. +For the cycle: with both machines offline, file X under Y on A and Y under X on B, then sync both. + +**Pass:** in both cases every group still has a card at the outermost level, so every one of them can be +opened and its parent cleared. Nothing is missing from the grid, and the trail never loops. + +**Failure means:** a group drawn nowhere is a group that can never be repaired — the fix is locked inside the +state that needs fixing. See `VaultViewModel.EffectiveParents`. + ### 3.5 A grouped host stays editable on an older build · **needs two builds** Only worth doing before a release that ships alongside an older client. A host filed into a group is written @@ -753,7 +760,7 @@ wheel. The pointer has to keep moving inside the band — a stationary pointer g **Failure means:** headless Avalonia cannot synthesise a platform drag, so the picking up, the cursor and the drop are covered by nothing. What *is* automated is the decision each drag event takes — `HostGridTests.TheGroupCardsAreWhatAcceptsADroppedHost` raises a real `DragOver` over both kinds of card — -and the write at the end, `ShellFlowTests.MovingAHostToAGroup_FilesItAndLeavesItSelected`. +and the write at the end, `ShellFlowTests.MovingAHostToAGroup_FilesItAndTakesItOffTheLevelItCameFrom`. ### 7.7 A click still selects, and a double click still connects @@ -804,6 +811,8 @@ add a bucket — then come back. **Failure means:** the screen has gone back to copying the vault's lists once at unlock. Covered by `TheTransfersScreen_FollowsTheVaultsHostList`; this is the version of it with a real picker in front of it. +--- + ## Phase 8 — Adding hosts on the phone Nine checks, and the reason there are nine rather than none is worth stating: **the layout suite cannot see @@ -902,6 +911,8 @@ Walk the sheet's two rows, the editor's fields, both pickers, the checkbox and t **Failure means:** a control took its size from its glyph. The desktop suite's only tap-target assertion has a floor of 20 pixels — set for a mouse — so copying it here would certify a target no thumb can hit. +--- + ## Phase 9 — Tag chips and the picker The desktop half is measured by the layout suite, which now seeds three tags and puts all of them on one From 6ae1912c34660f67b6041d352caf155c5341edaa Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Tue, 4 Aug 2026 10:24:47 +0200 Subject: [PATCH 2/2] Give the two logs and the buckets a resource type, so a conflict can be written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AadResourceTypes.For maps a syncable type onto the AAD resource type its cache records bind to, and it had no arm for ConnectionLogEntry, ActivityLogEntry or ObjectStore. All three are on both enums, in the reconciler registry and in the cipher pinning; only this switch was missed, and it throws rather than falling back — so a merge conflict on a connection log, an activity log or a bucket raised ArgumentOutOfRangeException on the path that records what the merge discarded. The conflict log is the whole reason the merge is allowed to pick a winner, so the one item kind whose conflicts could not be recorded was a bucket: an editable item two machines can genuinely disagree about. Worth writing down why it lasted two phases. Of the three callers, ItemStore and OutboxStore reach the mapping only when an item carries plaintext fields, and none of these three kinds does — so they never touched the gap. ConflictStore calls it unconditionally, but a test only reaches that by causing a real merge conflict, and every existing one raised its conflict against a Host. Three arms missing, and no path in the suite crossed any of them. So the tests are the point of this commit as much as the arms are. The guard is AadResourceTypeTests.EverySyncableType_HasAnArmInTheStorageMapping: it walks the whole wire enum, and for each type asserts both that there is an arm and that the arm returns the same-named resource type, which is the mistake the file's cipher half already guards against on the server side. Written over the full enum rather than over ItemKinds.SyncedTypes, because that is the stronger claim and the one the switch really makes — the two reserved association types have arms too. Beside it, CacheStoreTests.AConflict_CanBeRecordedForEveryKindOfItem records a conflict per kind and reads the detail back, since an arm returning the wrong resource type seals under one AAD and opens under another, which surfaces as an empty detail rather than as a throw. Both were confirmed to fail with the arms removed: the theory fails on exactly ConnectionLogEntry, ActivityLogEntry and ObjectStore and passes on the other three, and the guard names those three and no others. The note in docs/adding-hosts-on-the-phone.md that recorded this as out of scope is marked fixed, with what let it survive, since that is the part worth knowing next time an item kind is added. 1529 tests pass, seven of them new. --- docs/adding-hosts-on-the-phone.md | 11 +++- .../LocalCacheProtector.cs | 25 ++++++++ .../CacheStoreTests.cs | 43 ++++++++++++++ .../AadResourceTypeTests.cs | 58 +++++++++++++++++++ 4 files changed, 134 insertions(+), 3 deletions(-) diff --git a/docs/adding-hosts-on-the-phone.md b/docs/adding-hosts-on-the-phone.md index 8c95db9..802856c 100644 --- a/docs/adding-hosts-on-the-phone.md +++ b/docs/adding-hosts-on-the-phone.md @@ -325,9 +325,14 @@ because-string used as prose. `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. -> **Still open.** All three are on `SyncEntityType` and on `CryptoSpec.AadResourceType`, and all three are -> still absent from `LocalCacheProtector.For` — so this outlived the phases that shipped the logs and the -> buckets, which is exactly the drift a note like this is meant to prevent. +> **Fixed 2026-08-04**, having outlived the phases that shipped the logs and the buckets — which is exactly +> the drift a note like this is meant to prevent, so it is worth saying what let it last. Of the three callers +> of `AadResourceTypes.For`, two reach it only when an item carries plaintext fields and none of these three +> does; the third, `ConflictStore.RecordAsync`, calls it unconditionally but is reached only by a real merge +> conflict, which every existing test raised against a `Host`. The arms are in, and two tests now hold them +> there: `CacheStoreTests.AConflict_CanBeRecordedForEveryKindOfItem` records one per kind, and +> `AadResourceTypeTests.EverySyncableType_HasAnArmInTheStorageMapping` fails on the *next* item type added +> without one, by name rather than by a list kept by hand. ## Prose that becomes false diff --git a/src/DodoSSH.Client.Storage/LocalCacheProtector.cs b/src/DodoSSH.Client.Storage/LocalCacheProtector.cs index d300c71..d9352a0 100644 --- a/src/DodoSSH.Client.Storage/LocalCacheProtector.cs +++ b/src/DodoSSH.Client.Storage/LocalCacheProtector.cs @@ -98,10 +98,27 @@ public sealed class LocalCacheProtector : IDisposable /// Maps a syncable entity type onto the resource type its AAD binds. /// /// +/// /// A switch rather than a cast, even though the two enums happen to be adjacent. They are not the /// same list: also covers users, devices and vaults, so the /// numbers do not line up, and a cast would bind an item's ciphertext to the wrong resource type /// without failing anywhere a test would notice. +/// +/// +/// It has to name every syncable type, and the throw is not a safety net. +/// ConflictStore.RecordAsync calls this unconditionally, so a type with no arm here is a type +/// whose conflicts cannot be recorded — and the conflict log is the only reason the merge is +/// allowed to pick a winner at all. Three types went two phases without one: the two logs and the +/// buckets were added to both enums and to the reconciler registry, and this switch was not touched, +/// so a merge over any of them turned a recorded conflict into an +/// . The two other callers hid it — +/// ItemStore and OutboxStore only reach this when an item carries plaintext fields, and +/// none of those three does. +/// +/// +/// AadResourceTypeTests.EverySyncableType_HasAnArmInTheStorageMapping is what says so now, and it +/// is a name comparison rather than a list to keep by hand. +/// /// internal static class AadResourceTypes { @@ -117,6 +134,14 @@ internal static class AadResourceTypes SyncEntityType.Snippet => CryptoSpec.AadResourceType.Snippet, SyncEntityType.PortForward => CryptoSpec.AadResourceType.PortForward, SyncEntityType.KnownHostKey => CryptoSpec.AadResourceType.KnownHostKey, + + // Appended in the order the enums grew, which is why these three are not beside their + // neighbours by number. See docs/crypto.md §4.3 on why 14–16 are not one-behind their wire + // counterparts the way the arms above are. + SyncEntityType.ConnectionLogEntry => CryptoSpec.AadResourceType.ConnectionLogEntry, + SyncEntityType.ActivityLogEntry => CryptoSpec.AadResourceType.ActivityLogEntry, + SyncEntityType.ObjectStore => CryptoSpec.AadResourceType.ObjectStore, + _ => throw new ArgumentOutOfRangeException( nameof(entityType), entityType, "No AAD resource type is defined for this entity type."), }; diff --git a/tests/DodoSSH.Client.Storage.Tests/CacheStoreTests.cs b/tests/DodoSSH.Client.Storage.Tests/CacheStoreTests.cs index 90353a2..2360613 100644 --- a/tests/DodoSSH.Client.Storage.Tests/CacheStoreTests.cs +++ b/tests/DodoSSH.Client.Storage.Tests/CacheStoreTests.cs @@ -394,6 +394,49 @@ public sealed class CacheStoreTests : IAsyncLifetime all.Detail.ShouldBe(new byte[] { 1, 2, 3 }); } + /// + /// A conflict can be recorded against any kind of item, not only the kinds that were here first. + /// + /// + /// + /// Every other test in this section uses , and that is how three item + /// kinds shipped with no way to record a conflict at all: the two logs and the buckets were added to both + /// enums, to the reconciler registry and to the cipher pinning, while AadResourceTypes.For — which + /// ConflictStore.RecordAsync calls unconditionally — kept throwing for them. The two other callers + /// of that mapping only reach it when an item carries plaintext fields, which none of the three does, so + /// nothing else so much as touched the gap. + /// + /// + /// A theory over the types rather than one more Host case, because the failure was never about + /// conflicts and always about which types the layer below had been taught. Recording is asserted through + /// a read-back rather than by "it did not throw": an arm returning the wrong resource type would seal + /// under one AAD and open under another, which is a null detail rather than an exception. + /// + /// + [Theory] + [InlineData(SyncEntityType.Host)] + [InlineData(SyncEntityType.HostGroup)] + [InlineData(SyncEntityType.Snippet)] + [InlineData(SyncEntityType.ConnectionLogEntry)] + [InlineData(SyncEntityType.ActivityLogEntry)] + [InlineData(SyncEntityType.ObjectStore)] + public async Task AConflict_CanBeRecordedForEveryKindOfItem(SyncEntityType entityType) + { + var detail = System.Text.Encoding.UTF8.GetBytes($$"""{"kind":"{{entityType}}"}"""); + + var id = await harness.Conflicts.RecordAsync( + VaultId, entityType, Guid.CreateVersion7(), ConflictKind.FieldOverridden, detail, Token); + + var listed = (await harness.Conflicts.ListAsync(VaultId, false, Token)).ShouldHaveSingleItem(); + + listed.Id.ShouldBe(id); + listed.EntityType.ShouldBe(entityType); + listed.Detail.ShouldBe( + detail, + "an empty detail here means the record was sealed under one resource type and opened under " + + "another, which ListAsync reports as nothing rather than as a failure"); + } + [Fact] public async Task AnUnacknowledgedConflict_CannotBeDiscarded() { diff --git a/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs b/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs index 28c86de..a51df87 100644 --- a/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs +++ b/tests/DodoSSH.Client.Sync.Tests/AadResourceTypeTests.cs @@ -1,5 +1,6 @@ using System.Security.Cryptography; using DodoSSH.Client.Domain; +using DodoSSH.Client.Storage; using DodoSSH.Client.Sync; using DodoSSH.Contracts; using DodoSSH.Crypto; @@ -151,6 +152,63 @@ public sealed class AadResourceTypeTests .ShouldBe(ItemKinds.SyncedTypes, ignoreOrder: true); } + /// + /// The same pairing, made a second time in the storage layer, and every type must be in it. + /// + /// + /// + /// AadResourceTypes.For is the cache's copy of the table above: the ciphers seal an item for the + /// server, and this seals the two things the local cache holds in the clear — a relay host's + /// address, and the values a merge overrode. A type missing from it throws rather than mis-seals, which + /// sounds like the safe failure and is not: ConflictStore.RecordAsync calls it unconditionally, so + /// the exception lands on the path that records what a merge discarded. + /// + /// + /// This is written after finding three types missing from it — ConnectionLogEntry, + /// ActivityLogEntry and ObjectStore went two shipping phases without an arm, because the + /// only unconditional caller is one a test suite reaches solely by causing a real merge conflict. Asserted + /// over the whole wire enum rather than over ItemKinds.SyncedTypes, which is the stronger claim and + /// the one the switch actually makes: the two reserved association types have arms too. + /// + /// + [Fact] + public void EverySyncableType_HasAnArmInTheStorageMapping() + { + var missing = new List(); + + foreach (var wire in Enum.GetValues()) + { + if (wire == SyncEntityType.Unspecified) + { + continue; + } + + CryptoSpec.AadResourceType resource; + + try + { + resource = AadResourceTypes.For(wire); + } + catch (ArgumentOutOfRangeException) + { + missing.Add(wire); + continue; + } + + // Same name, as the cipher table demands — a wrong-but-present arm is the failure this half + // would otherwise wave through. + Enum.GetName(resource).ShouldBe( + Enum.GetName(wire), + $"AadResourceTypes.For({wire}) returns {resource}, which binds this type's cache records " + + "to another type's resource."); + } + + missing.ShouldBeEmpty( + "every syncable type needs an arm in AadResourceTypes.For, or a conflict recorded against one " + + "of these throws instead of being written — and the conflict log is what justifies the merge " + + "picking a winner."); + } + private static EncryptedPayload SealSample( SyncEntityType wire, byte[] vaultKey,