diff --git a/CHANGELOG.md b/CHANGELOG.md index a21ca1c..9b139b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,77 @@ current as you land changes. ## [Unreleased] +### Changed + +- **Contextual help lands on the key you asked about.** The **?** beside a + setting used to open one of four section anchors shared by every key in that + section; it now scrolls to that key's own row in the configuration reference. + The help renderer gives each documented key a row anchor, and each key's + anchor is derived from the key rather than hand-written, so the two cannot + drift — a key that loses its documentation row now fails the build by name. + The **?** button now says what it does on hover ("Open the documentation for + …"), which is the affordance a pointer user was missing — the key's own one-line + help is already on the control beside it, and visible under the toggles that + carry a caption. +- **Shorter action buttons, with one caption line under the row.** The Overview's + controls are now Block / Unblock / Switch VPN… / Pause / Guard down / Panic…, + with the window controls shortened to match — **Resume (m:ss left)** rather than + "Resume now", and one **Cancel** in place of "Cancel redial window" and "Cancel + VPN switch" (which window it closes is in the caption). The sentence each title + used to carry inline moved to a caption line beneath the row that follows the + pointer and the keyboard focus, and reads "Point at a button to see what it + does." when neither is on the row. Whichever of the two you used last wins, so + tabbing takes the caption from a resting pointer and moving the pointer takes it + back. The same sentence remains the tooltip and is also announced by VoiceOver as + the control's hint, and the caption wraps rather than truncating — its tail is + where the password expectation is stated, and that has to survive a narrow window. + The menubar's items are unchanged: a menu has no caption line to delegate to, and + a guided empty state's panic button keeps its full title for the same reason. +- **The advanced tunables table names its keys in full** (`vpn.advanced.redialBudget` + rather than `redialBudget`), matching every other table in the reference. + ### Fixed +- **Shutting the daemon down no longer buys one last lift-and-probe.** When + FULL BLOCK is held and the tunnel-scoped provider pass cannot be built (no + provider addresses resolved), the recovery probe lifts the guard, looks up + the exit country, and re-cuts. The run loop could take one of those on the + way out: with a cancelled context and a pending geo tick both ready, the + select chose between them at random. Losing that toss meant a stop briefly + opened egress through the forbidden-country exit — to observe a country + nothing was left to act on, moments before teardown removed the rules + anyway. A cancelled context now ends the loop instead, the same as any + other path out. +- **A contextual help link stays on the row it landed on.** The Help pane spends + a deep link's anchor as soon as WebKit has scrolled to it, and the view update + that followed asked for the same page without the fragment — which was loaded + again, putting the reader back at the top of the page a fraction of a second + after arriving. The pane now treats a target that differs only by a dropped + fragment as the page already on screen. Links *between* anchors on one page + still navigate, and clicking the same search hit again after scrolling away + still scrolls back to it. +- **The caption no longer follows a button that moved under a resting pointer.** + When a switch window opens, Cancel replaces Pause beneath wherever the mouse + happens to be; that counted as the pointer aiming at something and took the + caption away from the keyboard's own focused button. It now asks whether the + mouse actually moved, in screen coordinates, so only a hand that went somewhere + counts. A control that arrives already holding keyboard focus also writes its + own sentence now, rather than leaving the outgoing button's on screen. +- **Unblock says what it will actually release.** The button is offered both + while egress is cut by a standing block and while the guard is holding a downed + tunnel, and it described both as releasing a manual block and resuming + monitoring. Whenever the tunnel is down — under the guard *or* under a full + block — it now warns that enforcement stops and traffic uses your real IP until + the VPN reconnects, which is what `vpn.autoArm` does; with the tunnel up it + says the full block is lifted and the guard re-blocks the exit if it is still + forbidden. +- **Panic explains itself to VoiceOver and to a tooltip.** Shortening its title + to "Panic…" left the pane's one destructive control announcing nothing but its + name, with its explanation in an adjacent label no pointer or VoiceOver user + reached from the button. It now carries that sentence as both tooltip and + accessibility hint, with the visible copy hidden from VoiceOver so it is not + read twice — the same arrangement the action row already used. + - **"Open minimized" now actually decides whether the window opens.** The app used to infer a login launch from `NSApplication.launchIsDefaultUserInfoKey`, which reported wrong in both directions — the window appeared at login with diff --git a/README.md b/README.md index 9c33383..ab3182c 100644 --- a/README.md +++ b/README.md @@ -68,9 +68,10 @@ Gatekeeper friction** and asks for your password exactly once. Details, the Open the app and everything else happens by clicking: - **Menubar dropdown** — the safety core. One glance tells you the posture - (e.g. "Guard — NL via Mullvad"); **Block now** / **Unblock**, the VPN switch - window with a live countdown, and **Panic**. These never require the main - window to be open. + (e.g. "Guard — NL via Mullvad"); the VPN switch window with a live countdown, + **Pause**, and **Panic**. These never require the main window to be open. + Manual **Block** / **Unblock** are not here — they live in Overview, since + anyone who wants to cut their own internet can turn off Wi-Fi. - **Overview** — live status, the daily controls, and guided recovery: if the service isn't installed or is stopped, there's an inline button for exactly that, not an error message. diff --git a/cmd/dezhban/schemawire_test.go b/cmd/dezhban/schemawire_test.go new file mode 100644 index 0000000..4d696f4 --- /dev/null +++ b/cmd/dezhban/schemawire_test.go @@ -0,0 +1,122 @@ +package main + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/behnam-rk/dezhban/internal/config" +) + +// TestSchemaWireNamesTheAppDecodes pins the JSON field names `config schema --json` +// emits, because the macOS app decodes them by name and fails silently if one moves. +// +// `ConfigTunable`'s CodingKeys name `docAnchor` and `docKeyAnchor`, and +// `docKeyAnchor` is decoded with `decodeIfPresent` so an older CLI degrades to the +// section anchor. That tolerance is what would make a rename invisible: rename the Go +// tag and every key silently loses its row anchor, the `?` goes back to landing on +// section headings, and nothing on either side fails — the Swift test builds its +// fixture by hand, so it would keep passing too. +// +// Asserted against `schemaEntry`, not `config.Tunable`: the wire shape is the CLI's +// wrapper, which embeds the Tunable and adds `preset`. Marshalling the Tunable alone +// omits that field, so a test written against it both misses a name the app decodes +// and fails for a reason that is not a defect. This is where the tag would be +// changed, so this is where the assertion belongs. +func TestSchemaWireNamesTheAppDecodes(t *testing.T) { + tunables := config.Tunables() + if len(tunables) == 0 { + t.Fatal("no tunables — this test is pinning nothing") + } + + entries := make([]schemaEntry, len(tunables)) + written := presetWritten() + for i, tun := range tunables { + entries[i] = schemaEntry{Tunable: tun, Preset: written[tun.Key]} + } + + raw, err := json.Marshal(entries) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var decoded []map[string]json.RawMessage + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + // The names every tunable carries unconditionally. + required := []string{ + "key", "label", "kind", "default", "disablable", "advanced", "preset", + "help", "docAnchor", + } + for i, obj := range decoded { + for _, name := range required { + if _, ok := obj[name]; !ok { + t.Errorf("%s: no %q field on the wire", tunables[i].Key, name) + } + } + } + + // And the `omitempty` ones, asserted per tunable that actually has a value. + // + // Listing only the unconditional names and calling that "every CodingKey" was + // the first shape of this test, and it left the most consequential field + // unpinned: rename `restartReason` and `ConfigTunable.appliesLive` — which is + // `(restartReason ?? "").isEmpty` — returns true for every key, so Settings + // tells the user a restart-required key applies live. Nothing would have failed. + optional := map[string]func(config.Tunable) string{ + "capKey": func(t config.Tunable) string { return t.CapKey }, + "unit": func(t config.Tunable) string { return t.Unit }, + "restartReason": func(t config.Tunable) string { return t.RestartReason }, + } + covered := map[string]int{} + for i, obj := range decoded { + for name, value := range optional { + has := value(tunables[i]) != "" + _, onWire := obj[name] + switch { + case has && !onWire: + t.Errorf("%s: %q has a value but is missing from the wire", tunables[i].Key, name) + case !has && onWire: + t.Errorf("%s: %q is empty but present on the wire", tunables[i].Key, name) + case has: + covered[name]++ + } + } + } + for name := range optional { + if covered[name] == 0 { + t.Errorf("no tunable carries %q, so its wire name is not being pinned", name) + } + } + + // docKeyAnchor is omitempty, so it is absent exactly for the keys documented in + // prose — and present, naming a row on the reference page, for the rest. + withRow := 0 + for i, obj := range decoded { + key := tunables[i].Key + frag, present := obj["docKeyAnchor"] + if tunables[i].DocKeyAnchor == "" { + if present { + t.Errorf("%s: docKeyAnchor should be omitted when empty, got %s", key, frag) + } + continue + } + if !present { + t.Errorf("%s: docKeyAnchor missing from the wire", key) + continue + } + var s string + if err := json.Unmarshal(frag, &s); err != nil { + t.Errorf("%s: docKeyAnchor is not a string: %v", key, err) + continue + } + if !strings.Contains(s, "#key-") { + t.Errorf("%s: docKeyAnchor is %q, which does not name a key row", key, s) + } + withRow++ + } + if withRow == 0 { + t.Fatal("no tunable carried a row anchor — the app would have nothing to deep-link to") + } +} diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 68cc56b..adda918 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -213,6 +213,14 @@ Only a live host can prove these — CI cannot reach a printer. IP resolves → the daemon logs that recovery will briefly lift the guard, and recovery still works via lift-and-probe. A FULL BLOCK that can never observe its way out would be worse than the leak. +- [ ] **Shutdown takes no last probe.** Still on the unresolvable `providers` + above — the only configuration in which a probe lifts anything — hold FULL + BLOCK and stop the daemon (`Ctrl-C` in the foreground, or + `sudo dezhban stop`) with `pfctl -a dezhban -sr` polling in another shell. + The ruleset must go straight from FULL BLOCK to torn down: no lift-and-recut + in between. Buying a reading on the way out opens egress through the very + exit the block exists for, to observe a country nothing is left to act on. + `TestShutdownTakesNoProbeTick` covers the decision; only this shows the rules. ## Country check (exit country, not physical location) @@ -999,20 +1007,130 @@ task gui:build && open dist/Dezhban.app ### Actions -- [ ] **Routine ops are passwordless with a live daemon.** Block/Unblock and the - switch window complete over the control socket with **no** prompt, from both - the menubar and Overview; the switch countdown ticks in both surfaces and - matches. +- [ ] **The action row explains itself before the click.** Titles are short + (Block / Unblock / Switch VPN… / Pause / Guard down). Panic… is *not* part of + this row — it sits below with its own fixed caption, so hovering it changes + nothing, which is correct rather than a failure. Move the pointer across the + row: the caption line beneath it changes to that + control's sentence — in particular, hovering **Pause** must say it uses + your real ISP IP, which is the warning its old title carried — and the whole + sentence must be readable, including the password expectation at its tail, + which is what a single truncated line used to cut. Narrow the window until the + action row wraps and check it again there: two reserved lines were enough at a + comfortable width and put the ellipsis back on the password clause at a small + one, which is why the caption reserves three. Moving off the row leaves a + prompt ("Point at a button to see what it does."), *not* the posture headline — + the status hero already shows that a few lines above, and the caption used to + repeat it verbatim. + A disabled Block or Unblock says why it is disabled rather than describing the + action it will not perform. + + **Unblock names what it will actually release.** The rule is the tunnel + first, the posture second, because the daemon's unblock handler branches on + `AutoArm && !tunnelUp && !standby` without looking at why egress was cut. + So check three states, not two: (a) guard holding a downed tunnel — pull the + VPN; (b) `dezhban block` with the VPN **off**, which is a full block over a + downed tunnel; (c) `dezhban block` with the VPN **up**. (a) and (b) must both + warn that enforcement stops and traffic uses your **real IP** until the VPN + reconnects (`vpn.autoArm` is on by default, so both drop the daemon to + STANDBY); only (c) may say the block is lifted and the guard re-blocks. + It may not claim the block was manual or automatic in any of them: + `postureName` derives `full-block` from `blocked` alone, so both arrive as + the same posture string and nothing on the wire tells them apart. And read + the whole caption at a narrow width — these sentences share the three-line + reservation with the row's longest existing hint, so an overlong one + truncates the password clause off the tail. + + Tab through the row with Full Keyboard Access on and confirm focus drives the + caption too, **including with the pointer left resting on a different + button** — focus is meant to supersede a parked pointer. Then move the pointer + onto a control (the same one or another) and confirm it takes over again: + re-entering is what hands it back, deliberately, since jiggling inside the + control you are already on aims at nothing new. + + Then, still with the pointer parked on a button, Tab *out* of the row + altogether. The caption must go back to describing the button under the pointer + — not to the resting prompt. Focus outranks a parked pointer while it is in the + row; it does not erase where the pointer is. The line must never go blank or + change height either, which would reflow the row under the pointer. + + Two more, both about a caption outliving what it described, and both needing + the pointer to stay put — so trigger them from a terminal that already has + focus, with the command pre-typed. Reaching for the menubar moves the pointer + off the control, which fires a hover-exit and clears the state the step is + trying to observe. + + With the pointer parked on **Pause**, press Return on a waiting + `sudo dezhban switch --no-wait`: the button under the pointer becomes + **Cancel**, and the caption must stop describing Pause without waiting for the + mouse to move. Leave the pointer there through the next few countdown ticks — + the caption must stay on Cancel, and must not be handed back to the pointer if + you had tabbed elsewhere first, since retitling re-establishes the tracking area + under a stationary mouse. + + Run that swap **once more with keyboard focus on Block first**, pointer still + parked over Pause. Cancel replaces Pause underneath it, which is a *new* + control arriving rather than the same one retitling — but the hand has not + moved since Tab, so this is not the user aiming either: the caption must stay + on **Block**, where the focus ring and the Space key are. Then move the mouse + onto any control and confirm the pointer takes it back immediately. The same + applies when an enforcement-error banner appears above the row and shifts a + different button under a stationary pointer. + + Do that one twice more, because the reference point is *where the mouse was + when Tab moved the focus*, not where it was at the last hover event — and the + two differ in both directions. First **nudge the pointer a few points inside + Pause before tabbing** (no hover event fires, so a reading taken at the last + boundary would be stale): the caption must still stay on Block when Cancel + arrives. Then, with focus on Block, **flick the mouse quickly from one button + to the next** rather than easing across: the exit and the enter can be + dispatched from a single mouse-moved event, and the pointer must still take the + caption back. + + And with focus on **Pause**, open a window the same way: focus lands on the + replacement control, and the caption must describe **Cancel** rather than + keeping Pause's sentence — "uses your real ISP IP" under a button that ends a + window is the opposite of what it does. + + Then, with the pointer parked on **Block**, run a pre-typed action that ends in + a `refreshServiceState()` — any Overview action will do — with the control + socket having gone away underneath (`control.enabled=false` plus a restart + beforehand). The caption's password clause must follow the tooltip's rather + than keeping the answer it was given at hover-enter. It has to be an action + rather than simply waiting: the 1-second timer polls the state file and + repaints only, so `controlIsReachable` changes at launch, when either surface + opens, and after an action sequence — not on a tick. + + Do not reach for either by stopping the daemon: `state.isLive` goes false, + Overview renders its guided "stopped" layout, and the action row and its + caption line are gone before any of this could be observed. `routineHint` keys + on `controlIsReachable`, which is the socket, not the posture. +- [ ] **VoiceOver still hears what each button does.** With VoiceOver on, move + through the action row: each control announces its short title *and* its + consequence as a hint — Cancel in particular must say whether it closes the + automatic redial window or one you opened, since the titles no longer carry + that and the caption line is hidden from VoiceOver to avoid reading it twice. + **Panic… below the row too** — it is not in the action row and has no caption + line, so its own hint is the only thing that says it force-unblocks; its + visible sentence beside it is hidden from VoiceOver for the same + read-it-twice reason. Hovering Panic… must also produce a tooltip. +- [ ] **The degraded states keep their long panic title.** With the CLI missing + or the service not installed, the panic button still reads "Panic — force + unblock…" — there is no caption line there to carry the explanation. +- [ ] **Routine ops are passwordless with a live daemon.** Block/Unblock (Overview + only — they are deliberately not in the menubar) and the switch window (both + surfaces) complete over the control socket with **no** prompt; the switch + countdown ticks in both surfaces and matches. - [ ] **Pause and Resume, from both surfaces.** Pause opens with no password - (`control.allowPauseOps` default true); the app shows "Resume now (m:ss - left)" in place of the switch-window Cancel item, and the countdown agrees - between menubar and Overview. Resuming early re-arms the guard immediately. + (`control.allowPauseOps` default true); Overview shows "Resume (m:ss left)" + and the menubar "Resume now (m:ss left)" in place of the switch-window + Cancel item, and the countdown agrees between the two. Resuming early re-arms the guard immediately. Letting a pause expire re-arms it with no action needed. With `vpn.pauseMax: "0"`, Pause is disabled in both surfaces with a reason ("vpn.pauseMax is \"0\""), not just a silent no-op. - [ ] **Profile picker.** With `configs/dezhban.profiles.json`, Overview's details grid lists every configured profile and marks the one that - matched (`(active)`), matching `dezhban vpn list`; "Switching VPN…" + matched (`(active)`), matching `dezhban vpn list`; "Switch VPN…" becomes a menu with "Any known VPN" plus one item per profile, and picking a profile passes `--name ` (`dezhban vpn list` shows the learned endpoint attributed to it afterward). With no profiles @@ -1164,9 +1282,37 @@ traffic, so the check that matters is the one CI cannot run: with egress gone. shows this; the Go tests cannot. - [ ] Built from a checkout whose `docs/` was renamed under it, `task gui:build` **fails** rather than producing an app whose Help pane is missing a page. -- [ ] The **?** beside a Settings field opens Help scrolled to that key's own - heading — not the top of the configuration reference. Spot-check one field - per section, including one under Advanced. +- [ ] The **?** beside a Settings field opens Help scrolled to **that key's own + table row** — not to the section heading it shares with dozens of other + keys, and not to the top of the reference. Spot-check one field per + section, including one under Advanced (whose rows are anchored on the + fully-qualified `vpn.advanced.*` name). Its **tooltip** says what the button + does ("Open the documentation for …"): the key's own one-line help is already + on the control beside it, so repeating it here left nothing telling a pointer + user that the button navigates at all. + **And it stays there.** Watch the pane for a second after it opens: the row + must remain on screen, highlighted by `:target`. The deep link's anchor is + spent by the navigation it triggers, so the very next view update asks for the + same page with no fragment — answering that by loading again scrolled the + reader back to the top a fraction of a second after arriving + (`HelpNavigation.shouldLoad`). Then click a *different* key's **?** on the same + page and confirm it still moves: the fix must not turn into "never navigate + within a page again". Finally, search in the Help pane, click a heading hit, + scroll away by hand, and click **the same hit again** — it must scroll back. + An anchored request is honoured again on a repeat, including of the anchor + already showing; suppressing it as "already there" made the second click do + nothing. It must not be honoured *twice in one breath*, though: watch the pane + for a flash or a half-drawn page as it opens, which is a second `loadFileURL` + cancelling the first — `makeNSView` and `updateNSView` both run in the turn the + anchor is still pending, and only the served-anchor record tells that duplicate + from a real repeat. +- [ ] **A CLI newer than the bundled help still lands on the section.** The **?** + offers the key's row first and its section second, so an app bundle predating + row ids must land on the section heading rather than the top of the page. + Reproduce by running the built app against a help bundle from before this + change, or by checking that `dezhban config schema` prints a `docs:` anchor + that resolves as a heading on GitHub — that is the one a CLI reader follows, + and row ids exist only in the app's rendered help. - [ ] Against a CLI too old to know `config schema`, the **?** buttons are absent rather than present and inert. diff --git a/docs/usage/cli.md b/docs/usage/cli.md index cd15350..4212df9 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -621,11 +621,14 @@ promise. Build it with `task gui:build` (see [development.md](../contribute/deve Two surfaces, split by urgency: - **Menubar dropdown — the safety/glance core.** One status line (posture, exit - country/provider), **Open Dezhban…**, **Block now/Unblock**, the VPN switch - window (Switching VPN… / Cancel with a live countdown) when in VPN mode, - **Panic — force unblock…**, Quit. These are the time-critical and - lockout-recovery actions; they never depend on the main window opening. Items - enable/disable from the current state. + country/provider), **Open Dezhban…**, the VPN switch window (Switching VPN… / + Cancel VPN switch / Resume now, with a live countdown), **Pause — use my real + IP**, hold-the-line, **Panic — force unblock…**, Quit. These are the + time-critical and lockout-recovery actions; they never depend on the main window + opening. Items enable/disable from the current state. Manual **Block** and + **Unblock** are deliberately *not* here — somebody who wants to cut their own + internet can turn off Wi-Fi, so blocking by hand is a power-user affordance and + lives in the window's Overview (`AppDelegate` says so at the menu's top). - **Main window — everything else**, opened from the dropdown or by clicking the Dock icon (never automatically at launch). @@ -640,7 +643,18 @@ The main window's sidebar sections: banners above the grid — enforcement problems in red, failing exit checks in orange, first line only with the full text behind a disclosure — while an *expected* unknown exit stays a plain row. With profiles configured, - "Switching VPN…" becomes a menu so a switch window can target one by name. + "Switch VPN…" becomes a menu so a switch window can target one by name. + The action row's titles are deliberately short (Block, Unblock, Switch VPN…, + Pause, Guard down, and Resume / Cancel while a window is open) with a + **caption line beneath the row** + carrying the sentence each one used to hold inline — it follows the pointer + and the keyboard focus, and reads a prompt when neither is on the row (not the + posture headline, which the status hero already shows just above). The same + sentence is also the control's tooltip, so the two can never disagree. **Panic…** + is not in that row — it sits below with its own fixed caption, so hovering it + leaves the row's caption alone. The + menubar's items keep their long, self-explaining titles: a menu has no caption + line to delegate to. Degraded states are guided: CLI missing, service not installed, and daemon stopped each render an explanation with the one relevant action inline (Install service… / Guard up). diff --git a/docs/usage/config.md b/docs/usage/config.md index ec2c050..b820b85 100644 --- a/docs/usage/config.md +++ b/docs/usage/config.md @@ -308,7 +308,10 @@ An optional block for behaviors that are otherwise recommended defaults. Omit it entirely to keep the defaults; set only the knobs you need. Every field below is reachable with `dezhban config set vpn.advanced.=` — the same validated write-and-reload path as any other key — not just by hand-editing the -file. `switchWindowMax`, `redialWindowMax`, `redialMinUptime`, `redialBudget`, +file. Prose in this section drops the `vpn.advanced.` prefix once it is +established; the table below always writes it in full, because that is what its +row anchors are derived from. +`switchWindowMax`, `redialWindowMax`, `redialMinUptime`, `redialBudget`, `redialBudgetWindow`, `verifyInterval`, `livenessRedial`, and `windowDiscoveryInterval` apply live; the rest (built into something the run loop constructs once at startup, or — for `windowProtocols`/`windowPorts` — @@ -341,22 +344,22 @@ were on. Turning it off is fine; turning it off by accident is not. | Field | Default | What it controls | |---|---|---| -| `switchWindowMax` | `3m` | Hard cap on any MANUAL switch window (incl. `--for`). | -| `redialWindowMax` | `10m` | Hard cap on the AUTOMATIC redial window — kept independent of `switchWindowMax` so one trigger's budget never truncates the other's. | -| `commandFreshness` | `30s` | How recent a control command must be to be acted on (replay guard). | -| `windowDiscoveryInterval` | `1s` | How often the new server is looked for while a window is open. | -| `tunnelPruneAfter` | `60s` | How long a dynamically-detected tunnel must be gone before it's dropped. | -| `learnedEndpointTTL` | `720h` | How long an unused learned endpoint is kept. | -| `learnedMaxPerProfile` | `16` | Cap on learned endpoints per profile (LRU). | -| `promoteAfterRefreshes` | `3` | Consecutive sightings before a discovered endpoint is learned under normal guard. | -| `redialMinUptime` | `15s` | Backoff seed for the automatic redial window: a tunnel that was up for less than this, with no good exit confirmed during that uptime, still gets a window — but a shorter one for each consecutive fast drop, with a growing wait between them. The first drop after startup is exempt — uptime before the daemon started is unknowable. The wait between windows is cleared by a tunnel that proves itself (a confirmed exit, or an uptime past this value), not only by elapsing. `"0"` disables the backoff, so every qualifying drop gets a full window until the budget runs out. | -| `redialBudget` | `2m` | Total time automatic redial windows may leave the guard relaxed within `redialBudgetWindow`. Debited when a window opens and **credited back when it closes early**, so a redial that succeeded in three seconds costs three seconds — the budget measures the exposure actually taken, not the exposure offered. When it can no longer afford a window the guard simply holds and traffic stays cut. Not disablable (see below). | -| `redialBudgetWindow` | `15m` | The rolling period `redialBudget` is measured over. Each window's cost is returned as it falls out of the period, so a busy link recovers its allowance progressively rather than needing a full quiet stretch. Not disablable. | -| `endpointWarnThreshold` | `256` | Union size at which `doctor` warns about rule-list bloat. | -| `verifyInterval` | `1m` | How often the daemon re-reads the firewall to confirm the rules it believes are installed are still there, re-applying whatever is currently in force — the standing guard, a full block, or an open switch/redial window or pause — the instant they are not. Every other rule change dezhban makes is triggered by something the daemon itself did — this is the only one that notices a ruleset removed from OUTSIDE it (another firewall tool, `pfctl -F all`, `nft flush ruleset`, an OS ruleset reload). `"0"` disables the check, trusting the rules to stay put once applied. On Windows each check is a whole PowerShell invocation (one, not two: the group-existence test and the profile-default cross-check share a single script), so a very short interval has a real cost — the default is deliberately conservative. | -| `livenessRedial` | `false` | Lets a tunnel that reports up but has stopped passing traffic open an automatic redial window — see [ADR-0010](../adr/0010-tunnel-liveness.md). Off by default: an exit that censors the geo lookup produces the identical failure pattern as a genuinely hung tunnel, and turning this on lets that exit trigger a window on a tunnel that was never actually down. The diagnosis itself (`dezhban doctor`, the state file) is always on regardless of this key — only ACTING on it is gated. | -| `windowProtocols` | `[]` | Restrict a switch window to these protocols (e.g. `["udp"]`) instead of allowing all outbound. Empty allows all — only worth setting when every VPN you switch to uses a fixed protocol. | -| `windowPorts` | `[]` | Restrict a switch window to these ports (e.g. `[51820]`) instead of allowing all outbound. Empty allows all — only worth setting when every VPN you switch to uses a fixed port set (e.g. WireGuard on 51820). | +| `vpn.advanced.switchWindowMax` | `3m` | Hard cap on any MANUAL switch window (incl. `--for`). | +| `vpn.advanced.redialWindowMax` | `10m` | Hard cap on the AUTOMATIC redial window — kept independent of `vpn.advanced.switchWindowMax` so one trigger's budget never truncates the other's. | +| `vpn.advanced.commandFreshness` | `30s` | How recent a control command must be to be acted on (replay guard). | +| `vpn.advanced.windowDiscoveryInterval` | `1s` | How often the new server is looked for while a window is open. | +| `vpn.advanced.tunnelPruneAfter` | `60s` | How long a dynamically-detected tunnel must be gone before it's dropped. | +| `vpn.advanced.learnedEndpointTTL` | `720h` | How long an unused learned endpoint is kept. | +| `vpn.advanced.learnedMaxPerProfile` | `16` | Cap on learned endpoints per profile (LRU). | +| `vpn.advanced.promoteAfterRefreshes` | `3` | Consecutive sightings before a discovered endpoint is learned under normal guard. | +| `vpn.advanced.redialMinUptime` | `15s` | Backoff seed for the automatic redial window: a tunnel that was up for less than this, with no good exit confirmed during that uptime, still gets a window — but a shorter one for each consecutive fast drop, with a growing wait between them. The first drop after startup is exempt — uptime before the daemon started is unknowable. The wait between windows is cleared by a tunnel that proves itself (a confirmed exit, or an uptime past this value), not only by elapsing. `"0"` disables the backoff, so every qualifying drop gets a full window until the budget runs out. | +| `vpn.advanced.redialBudget` | `2m` | Total time automatic redial windows may leave the guard relaxed within `redialBudgetWindow`. Debited when a window opens and **credited back when it closes early**, so a redial that succeeded in three seconds costs three seconds — the budget measures the exposure actually taken, not the exposure offered. When it can no longer afford a window the guard simply holds and traffic stays cut. Not disablable (see below). | +| `vpn.advanced.redialBudgetWindow` | `15m` | The rolling period `redialBudget` is measured over. Each window's cost is returned as it falls out of the period, so a busy link recovers its allowance progressively rather than needing a full quiet stretch. Not disablable. | +| `vpn.advanced.endpointWarnThreshold` | `256` | Union size at which `doctor` warns about rule-list bloat. | +| `vpn.advanced.verifyInterval` | `1m` | How often the daemon re-reads the firewall to confirm the rules it believes are installed are still there, re-applying whatever is currently in force — the standing guard, a full block, or an open switch/redial window or pause — the instant they are not. Every other rule change dezhban makes is triggered by something the daemon itself did — this is the only one that notices a ruleset removed from OUTSIDE it (another firewall tool, `pfctl -F all`, `nft flush ruleset`, an OS ruleset reload). `"0"` disables the check, trusting the rules to stay put once applied. On Windows each check is a whole PowerShell invocation (one, not two: the group-existence test and the profile-default cross-check share a single script), so a very short interval has a real cost — the default is deliberately conservative. | +| `vpn.advanced.livenessRedial` | `false` | Lets a tunnel that reports up but has stopped passing traffic open an automatic redial window — see [ADR-0010](../adr/0010-tunnel-liveness.md). Off by default: an exit that censors the geo lookup produces the identical failure pattern as a genuinely hung tunnel, and turning this on lets that exit trigger a window on a tunnel that was never actually down. The diagnosis itself (`dezhban doctor`, the state file) is always on regardless of this key — only ACTING on it is gated. | +| `vpn.advanced.windowProtocols` | `[]` | Restrict a switch window to these protocols (e.g. `["udp"]`) instead of allowing all outbound. Empty allows all — only worth setting when every VPN you switch to uses a fixed protocol. | +| `vpn.advanced.windowPorts` | `[]` | Restrict a switch window to these ports (e.g. `[51820]`) instead of allowing all outbound. Empty allows all — only worth setting when every VPN you switch to uses a fixed port set (e.g. WireGuard on 51820). | ## Presets diff --git a/gui/macos/Sources/DezhbanCore/ActionCaption.swift b/gui/macos/Sources/DezhbanCore/ActionCaption.swift new file mode 100644 index 0000000..f15d5a2 --- /dev/null +++ b/gui/macos/Sources/DezhbanCore/ActionCaption.swift @@ -0,0 +1,96 @@ +import CoreGraphics +import Foundation + +/// What the single description line under the action row says. +/// +/// The row's buttons carry one- or two-word titles ("Pause", "Panic", "Guard +/// down"), so the sentence that used to live inside the title — "Pause — use my +/// real IP" — has to live somewhere the reader sees it *before* they click, not +/// only after a tooltip delay. That somewhere is one caption line beneath the +/// row, showing whatever the pointer or keyboard focus is on. +/// +/// Split out of the view for the same reason `PostureUI` and +/// `ActionRowPacking` are: the fallback rule has a right and a wrong answer, and +/// getting it wrong means a kill switch whose most dangerous button explains +/// itself to nobody. +public enum ActionCaption { + /// Which input the user reached for most recently. + public enum Aim: Equatable { + case pointer + case keyboard + } + + /// The caption to show. + /// + /// `hovered` is the hint of the control the pointer is over, `focused` the hint + /// of the keyboard-focused control, and `aim` says which of the two the user + /// touched last. Most-recent-interaction wins, which is the only rule that + /// satisfies both directions: a pointer parked on one button must not outrank the + /// keyboard indefinitely (tabbing moved the focus ring and the Space key while + /// the caption described something else), and tabbing must not erase the fact + /// that the pointer is still resting somewhere. + /// + /// Ranking rather than clearing, deliberately. The caller used to answer this by + /// setting `hovered` to nil when focus moved, which threw the pointer's position + /// away instead of deprioritising it: tab into the row and back out to anything + /// outside it and *both* were empty, so the caption fell to the prompt while the + /// pointer sat on Block, until it moved off and back on. + /// + /// With neither, `fallback` stands in — a prompt, in the app's case, not the + /// posture headline: the hero already renders that string in title2 just above, + /// so echoing it here said the same thing twice. Never empty and never a + /// placeholder like "—": the line must not collapse and reflow the row every + /// time the pointer crosses a gap between two buttons. + public static func text(hovered: String?, focused: String?, + aim: Aim, fallback: String) -> String { + let order = aim == .keyboard ? [focused, hovered] : [hovered, focused] + for candidate in order { + if let c = candidate, !c.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { + return c + } + } + return fallback + } + + /// Whether a hover-enter is the user aiming at something, or a control arriving + /// under a pointer that never moved. + /// + /// `onHover(true)` cannot tell the two apart on its own: it fires both when the + /// pointer crosses into a control and when a tracking area is established + /// beneath a stationary one. Two guards, and they cover different halves: + /// + /// - `previousHoverID != id` rejects the *same* control re-firing, which these + /// controls do constantly — a window's countdown retitles "Cancel (m:ss + /// left)" every second. + /// - the pointer comparison rejects a *different* control arriving underneath, + /// which the first guard cannot see. Pause becomes Cancel when a window + /// opens, and the replacement's enter arrives with a new id while the user's + /// hand has not moved — handing the caption back to the pointer a moment + /// after the keyboard took it, the very failure the first guard was added + /// for, through the other door. + /// + /// `pointer` must be in SCREEN coordinates (`NSEvent.mouseLocation`), which is + /// what makes this different from the local-space comparison that was tried and + /// removed: a banner appearing above the row moves a control under a stationary + /// mouse, so the pointer's position *within* that control changes while the + /// mouse itself has not. On screen it has not changed, which is the question + /// being asked. + /// + /// `keyboardAimedAt` is where the pointer was when the keyboard last took the + /// caption, and nil when it has not. That is the reference point, NOT the + /// pointer's position at the previous hover event, which was the first shape of + /// this and was wrong in both directions. `onHover` fires on enter and exit + /// only, so a reading taken there is the position at the last boundary crossing: + /// a hand that drifted a few points while reading (no hover event) then made a + /// swap look like movement, and a fast flick across the gap between two buttons + /// — whose exit and enter are dispatched from a single mouse-moved event, so + /// both sample the same location — looked like stillness and silently refused to + /// take the caption back. Anchored to the moment the keyboard took over, the + /// question is the one actually being asked: has the hand gone anywhere since? + public static func hoverIsAim(previousHoverID: String?, id: String, + pointer: CGPoint, keyboardAimedAt: CGPoint?) -> Bool { + guard previousHoverID != id else { return false } + guard let keyboardAimedAt else { return true } + return pointer != keyboardAimedAt + } +} diff --git a/gui/macos/Sources/DezhbanCore/ConfigSchema.swift b/gui/macos/Sources/DezhbanCore/ConfigSchema.swift index 50ef75c..c75a4c0 100644 --- a/gui/macos/Sources/DezhbanCore/ConfigSchema.swift +++ b/gui/macos/Sources/DezhbanCore/ConfigSchema.swift @@ -37,8 +37,14 @@ public struct ConfigTunable: Codable, Identifiable, Hashable { /// the config to Custom. public let preset: Bool public let help: String - /// Where this key is documented, as "#". + /// The *section* of the documentation covering this key, as "#". + /// A heading anchor, so it resolves in any markdown viewer as well as here. public let docAnchor: String + /// The key's own table row, as "#", or nil for a key documented + /// in prose. Row ids exist only in the rendered help, which is why this is + /// additional to `docAnchor` rather than a replacement — and why an older + /// bundle that predates row ids still lands the reader on the section. + public let docKeyAnchor: String? /// Why a running daemon cannot adopt this key in place; nil when it can. public let restartReason: String? @@ -46,17 +52,28 @@ public struct ConfigTunable: Codable, Identifiable, Hashable { private enum CodingKeys: String, CodingKey { case key, label, kind case defaultValue = "default" - case capKey, unit, disablable, advanced, preset, help, docAnchor, restartReason + case capKey, unit, disablable, advanced, preset, help, docAnchor, docKeyAnchor + case restartReason } /// True when a change to this key takes effect without restarting the daemon. public var appliesLive: Bool { (restartReason ?? "").isEmpty } - /// Where the Help pane should land for this key. Nil only if the schema - /// carried no anchor at all — Go's TestEveryTunableDocAnchorResolves keeps - /// that from shipping, and a control simply offers no link rather than a - /// dead one. - public var docTarget: HelpTarget? { HelpTarget(docAnchor: docAnchor) } + /// Where the Help pane should land for this key, best first. + /// + /// The key's row, then its section. Two entries rather than one because the + /// grain the app wants and the grain that always exists are different things: + /// a row id is present only in help rendered by a build that emits them, so a + /// CLI newer than the bundle would otherwise drop the reader at the top of a + /// forty-key reference. Falling back to the section is the middle step that + /// case needs. + /// + /// Empty only if the schema carried no anchor at all — Go's + /// TestEveryTunableDocAnchorResolves keeps that from shipping, and a control + /// then offers no link rather than a dead one. + public var docTargets: [HelpTarget] { + [docKeyAnchor, docAnchor].compactMap { $0.flatMap(HelpTarget.init(docAnchor:)) } + } /// Placeholder text for a text field: the label, then the real default. /// diff --git a/gui/macos/Sources/DezhbanCore/HelpIndex.swift b/gui/macos/Sources/DezhbanCore/HelpIndex.swift index c8d8a0d..481e9ba 100644 --- a/gui/macos/Sources/DezhbanCore/HelpIndex.swift +++ b/gui/macos/Sources/DezhbanCore/HelpIndex.swift @@ -27,12 +27,16 @@ public struct HelpPage: Codable, Identifiable, Hashable { /// material — reachable and searchable, but not part of the guided track. public let tutorial: Int public let headings: [HelpHeading] + /// Per-row anchors of the page's reference tables — one per documented + /// config key. Separate from `headings` because they are not headings: they + /// never appear in the sidebar outline, they only resolve deep links. + public let keys: [HelpHeading] /// The page stripped to words, so search runs in the app with no second /// pass over the HTML. public let text: String private enum CodingKeys: String, CodingKey { - case file, source, title, summary, tutorial, headings, text + case file, source, title, summary, tutorial, headings, keys, text } public init(from decoder: Decoder) throws { @@ -45,24 +49,38 @@ public struct HelpPage: Codable, Identifiable, Hashable { // common case: most pages are reference, not tutorial. tutorial = try c.decodeIfPresent(Int.self, forKey: .tutorial) ?? 0 headings = try c.decodeIfPresent([HelpHeading].self, forKey: .headings) ?? [] + // `omitempty` on the Go side, and absent entirely from a help bundle + // built before key anchors existed — so an older bundle decodes to a + // page with no key anchors rather than failing to decode at all. + keys = try c.decodeIfPresent([HelpHeading].self, forKey: .keys) ?? [] text = try c.decodeIfPresent(String.self, forKey: .text) ?? "" } public init(file: String, source: String, title: String, summary: String, - tutorial: Int = 0, headings: [HelpHeading] = [], text: String = "") { + tutorial: Int = 0, headings: [HelpHeading] = [], + keys: [HelpHeading] = [], text: String = "") { self.file = file self.source = source self.title = title self.summary = summary self.tutorial = tutorial self.headings = headings + self.keys = keys self.text = text } - /// True when this page has a heading with that fragment id — what a - /// contextual deep link depends on. + /// True when this page has a heading OR a documented-key row with that + /// fragment id — what a contextual deep link depends on. Keys count because a + /// `ConfigTunable.docKeyAnchor` names one directly + /// ("usage/config.md#key-vpnredialwindow"). + /// + /// Not `docAnchor`: that is the key's *section*, kept separate on purpose so a + /// CLI can print an anchor resolving in any markdown viewer, and so this index + /// has a step to fall back to when a bundle predates row ids. Collapsing the two + /// is the regression internal/config/schema.go spends a paragraph warning + /// against. public func hasAnchor(_ anchor: String) -> Bool { - headings.contains { $0.anchor == anchor } + headings.contains { $0.anchor == anchor } || keys.contains { $0.anchor == anchor } } } @@ -274,3 +292,52 @@ public enum HelpURL { return c?.url ?? absolute } } + +/// Whether a help URL has to be handed to the web view at all, given what is +/// already showing. +/// +/// Separate from the web view — and from the executable target, which cannot be +/// `@testable import`ed — because the answer is not "are these URLs equal". A +/// deep link's anchor is one-shot: it is spent by the navigation it triggered and +/// cleared on the next runloop turn, which re-evaluates the view and asks for the +/// *same page with no fragment* a moment later. Answering that second ask by +/// loading again reloads the page and puts the reader back at the top — undoing +/// the scroll the "?" button exists to perform, a fraction of a second after it +/// happened. +public enum HelpNavigation { + /// True when `target` is not already on screen. + /// + /// `loaded` is the last URL handed to the web view, fragment included. + /// + /// The rule is about the *fragment*, not about equality. A target that carries + /// one is an explicit "scroll here" and is honoured again on a repeat, because + /// between the two requests the reader may have scrolled away, and clicking the + /// same search hit again to get back is the obvious way to ask. Suppressing that + /// on "you are already there" made the second click do nothing at all. + /// + /// `servedAnchor` is what keeps "again" from meaning "twice in one breath". + /// `NSViewRepresentable` calls `updateNSView` immediately after `makeNSView`, in + /// the same runloop turn, while the anchor is still pending — it is cleared + /// asynchronously — so opening the pane on a deep link asks for the identical + /// anchored URL twice, and honouring both started a second load that cancelled + /// the first mid-flight. The caller records what it just served and clears it on + /// the following bare call, so a genuine repeat click (which arrives after that + /// clear) still navigates and the same-turn duplicate does not. + /// + /// A target with no fragment is suppressed when it names the page already + /// loaded, which is the spent-anchor follow-up this type exists for. Nothing is + /// lost: a bare request for the page on screen is a request to show what is + /// already shown. + public static func shouldLoad(target: URL, loaded: URL?, servedAnchor: URL?) -> Bool { + // Compared absolute, for the reason `HelpURL` spends a paragraph on: a URL + // built from `Bundle.main.resourceURL` carries a base, and the same page + // reached the two ways this method sees it — freshly derived (based) and + // remembered from a previous `appendingFragment` (absolute) — is otherwise + // unequal to itself. That inequality would silently restore the reload. + let here = target.absoluteURL + let bare = HelpURL.deletingFragment(target) + if bare != here { return here != servedAnchor } // carries a fragment + guard let loaded else { return true } + return bare != HelpURL.deletingFragment(loaded) + } +} diff --git a/gui/macos/Sources/DezhbanCore/PostureUI.swift b/gui/macos/Sources/DezhbanCore/PostureUI.swift index 1cb0fd7..0f9e0c1 100644 --- a/gui/macos/Sources/DezhbanCore/PostureUI.swift +++ b/gui/macos/Sources/DezhbanCore/PostureUI.swift @@ -96,6 +96,69 @@ public enum PostureUI { return !tuns.contains(where: { $0.up }) } + /// What Unblock actually releases, which is not a manual block in the state + /// that matters most. + /// + /// Overview enables it on `blocked || guardHoldsDownedTunnel`, and the sentence + /// describing it is no longer a tooltip a reader may never see but the caption + /// under the row — the primary pre-click explanation. "Releases a manual block + /// and resumes monitoring" covered both cases and was wrong about the second: + /// + /// - A tunnel that is not up. `runner`'s unblock handler branches on + /// `AutoArm && !tunnelUp && !standby` and does **not** look at why egress + /// was cut, so this has to be asked first, for every posture. With + /// `vpn.autoArm` (default on) an explicit unblock there is read as "the VPN + /// is off on purpose": the daemon drops to STANDBY, which installs no rules + /// at all. Monitoring is precisely what does *not* resume — the guard + /// re-arms when a tunnel comes back, not before. + /// - Otherwise `blocked`: egress cut by a standing block, which the snapshot + /// reports as posture "full-block" whether an operator asked for it or a + /// blocked-country reading did — `postureName` derives it from `blocked` + /// alone and nothing on the wire tells the two apart, so this must not claim + /// either. What is true of both: the daemon restores the guard and hands the + /// geo state machine back the wheel, so a still-forbidden exit carries + /// traffic until the next reading re-escalates. + /// + /// `vpn.autoArm` is assumed on, since the snapshot does not carry it and it is + /// the default. Turned off, the daemon restores the guard instead — which with + /// no tunnel is still a total cut, so the error is a caption warning about an + /// exposure that does not happen. That is the survivable direction; promising + /// "resumes monitoring" over a drop to standby is not. + /// + /// Returned bare, without the password expectation `AppState.routineHint` + /// appends — which is also why these stay short. That suffix is 60 characters, + /// and the caption reserves three lines sized for the longest existing hint + /// (~78 bare); a sentence that overruns it truncates, and the tail it drops is + /// the password clause. + public static func unblockConsequence(_ s: Snapshot?) -> String { + guard let s = s else { return "Releases the block and resumes monitoring." } + if releasingDropsToStandby(s) { + return "Stops enforcing until the VPN reconnects — traffic uses your real IP." + } + if s.posture == "full-block" { + return "Lifts the full block; traffic uses the exit until the guard re-blocks it." + } + return "Releases the block and resumes monitoring." + } + + /// Whether an explicit unblock would land in the daemon's autoArm branch and + /// leave nothing enforcing. + /// + /// `guardHoldsDownedTunnel` answers this for the guard posture and is preferred + /// where it applies, because the daemon's own Display is authoritative there. + /// It cannot answer for FULL BLOCK: it returns false for any posture but + /// "guard", and `fullBlockDisplay` reports the blocked key whether the tunnel is + /// up or down, so the tunnel list is the only thing left to read. + /// + /// Nothing reported as up counts as down, an absent list included — the same + /// reading `guardHoldsDownedTunnel` gives an empty list, and the direction that + /// errs toward the warning rather than toward the false promise. + private static func releasingDropsToStandby(_ s: Snapshot) -> Bool { + if guardHoldsDownedTunnel(s) { return true } + guard let tuns = s.tunnels, tuns.contains(where: { $0.up }) else { return true } + return false + } + /// SwiftUI accent for a brand state — used where the bundled bitmap isn't /// (SF Symbol fallback, text highlights). public static func color(for state: String) -> Color { diff --git a/gui/macos/Sources/DezhbanMenu/AppDelegate.swift b/gui/macos/Sources/DezhbanMenu/AppDelegate.swift index 476ea88..58ff7d5 100644 --- a/gui/macos/Sources/DezhbanMenu/AppDelegate.swift +++ b/gui/macos/Sources/DezhbanMenu/AppDelegate.swift @@ -105,7 +105,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSMenuDelegate { menu.delegate = self // We compute item enablement ourselves (see addAction); without this, AppKit's // automatic validation force-enables any item whose target responds to its - // selector, so the gating on "Block now" etc. would be ignored. + // selector, so the gating on "Pause — use my real IP" etc. would be ignored. menu.autoenablesItems = false statusItem.menu = menu watchdog.start() diff --git a/gui/macos/Sources/DezhbanMenu/AppState.swift b/gui/macos/Sources/DezhbanMenu/AppState.swift index f18f831..07424cc 100644 --- a/gui/macos/Sources/DezhbanMenu/AppState.swift +++ b/gui/macos/Sources/DezhbanMenu/AppState.swift @@ -216,12 +216,39 @@ final class AppState: ObservableObject { } } - /// Opens the Help pane at a specific place in the documentation, named the - /// way a `Tunable`'s docAnchor writes it ("usage/config.md#fields"). - /// A docAnchor that names nothing bundled still opens the pane — better a - /// reader lands in the documentation than on a dead control. - func openHelp(docAnchor: String) { - helpTarget = HelpTarget(docAnchor: docAnchor) + /// Decoded lazily on the first contextual help click and kept thereafter — see + /// `openHelp(preferring:)`. + private var cachedHelpBundle: HelpBundle? + + /// Opens the first of `targets` whose anchor actually exists in the bundle. + /// + /// The only entry point, deliberately. A second `openHelp(docAnchor:)` taking a + /// bare string survived this change with no callers, and it skipped the + /// resolution step this one exists for — so picking the shorter-looking overload + /// would have silently restored landing at the top of the page. + /// + /// Preference order, not alternatives: a key's own row first, then its + /// section. Resolving here rather than in the Help pane is what makes the + /// fallback a *section* rather than the top of the page — the pane's own + /// resolve() drops an unknown fragment and keeps the page, which for a + /// forty-key reference is not a useful place to land. + func openHelp(preferring targets: [HelpTarget]) { + guard !targets.isEmpty else { return } + // Decoded once and kept, rather than on every click of a `?`: `bundled()` + // reads the payload off disk and JSON-decodes every page's full text plus its + // per-row key list. + // + // It does not make that the app's only decode — `HelpView` evaluates + // `bundled()` in its own `@State` initialiser, so a second copy exists and is + // rebuilt more often than this one. Sharing them is worth doing and is not + // this change's business; the point here is only that resolving a target must + // not add a decode of its own. + // + // Nil (a bare SwiftPM binary with no help payload) leaves the preferred + // target, which is the right guess when nothing can be checked. + if cachedHelpBundle == nil { cachedHelpBundle = HelpBundle.bundled() } + let index = cachedHelpBundle + helpTarget = targets.first { index?.resolve($0)?.anchor != nil } ?? targets[0] selectedSection = .help } diff --git a/gui/macos/Sources/DezhbanMenu/HelpView.swift b/gui/macos/Sources/DezhbanMenu/HelpView.swift index ecdc438..8eaada6 100644 --- a/gui/macos/Sources/DezhbanMenu/HelpView.swift +++ b/gui/macos/Sources/DezhbanMenu/HelpView.swift @@ -254,6 +254,11 @@ struct HelpWebView: NSViewRepresentable { var parent: HelpWebView /// What was last handed to the web view, fragment included. private var loaded: URL? + /// The anchored URL served in the episode still in progress, cleared by the + /// bare call that ends it. Distinct from `loaded`, which never forgets — this + /// one exists only to tell a repeat click apart from the same-turn duplicate + /// `makeNSView` + `updateNSView` produce. See `HelpNavigation.shouldLoad`. + private var servedAnchor: URL? init(_ parent: HelpWebView) { self.parent = parent @@ -279,7 +284,22 @@ struct HelpWebView: NSViewRepresentable { let target = anchored.isFileURL ? anchored : url.absoluteURL defer { clearAnchor() } guard target.isFileURL, readAccess.isFileURL else { return } - guard target != loaded else { return } + // Not `target != loaded`. The anchor is one-shot (see clearAnchor), so + // every anchored load is followed one runloop turn later by an identical + // call with no anchor — which differs from `loaded` only by the dropped + // fragment, and reloading on that took the reader straight back to the + // top of the page the "?" had just scrolled into. `HelpNavigation` owns + // the rule so it can be tested; DezhbanMenu is an executable target and + // this method cannot be. + // + // That bare follow-up is also what ends an anchored episode: it is the + // one call that can only arrive after the pending anchor was cleared, so + // clearing `servedAnchor` here is what lets a later click on the SAME + // anchor navigate again while the same-turn duplicate below does not. + if anchor == nil { servedAnchor = nil } + guard HelpNavigation.shouldLoad(target: target, loaded: loaded, + servedAnchor: servedAnchor) else { return } + if anchor != nil { servedAnchor = target.absoluteURL } loaded = target web.loadFileURL(target, allowingReadAccessTo: readAccess) } diff --git a/gui/macos/Sources/DezhbanMenu/OverviewView.swift b/gui/macos/Sources/DezhbanMenu/OverviewView.swift index 60b9789..80b8081 100644 --- a/gui/macos/Sources/DezhbanMenu/OverviewView.swift +++ b/gui/macos/Sources/DezhbanMenu/OverviewView.swift @@ -8,6 +8,24 @@ import DezhbanCore struct OverviewView: View { @EnvironmentObject var state: AppState @State private var busy = false + /// The action control the pointer is over, and the sentence it explains. + /// Carried together so a control can only clear the caption it actually put + /// there — see `captioned`. + @State private var hoveredAction: (id: String, hint: String)? + /// Last hint handed over by keyboard focus. Read only while + /// `focusedAction != nil`, so blurring the row falls back to the resting + /// prompt instead of stranding the caption on a control nobody is on. + @State private var focusedHint = "" + /// Which input the user reached for last, so focus can outrank a parked pointer + /// without the pointer's position being thrown away. See `ActionCaption.Aim`. + @State private var aim: ActionCaption.Aim = .pointer + /// Where the mouse was when the keyboard last took the caption, in screen + /// coordinates — the reference that tells the pointer arriving somewhere from a + /// control arriving under a pointer that has not moved. Nil while the keyboard + /// has not taken it, and cleared once the pointer has proved it moved. See + /// `ActionCaption.hoverIsAim`. + @State private var keyboardAimPoint: CGPoint? + @FocusState private var focusedAction: String? var body: some View { Group { @@ -58,7 +76,10 @@ struct OverviewView: View { Divider() - actionButtons(s) + VStack(alignment: .leading, spacing: 6) { + actionButtons(s) + actionCaption() + } Spacer(minLength: 12) @@ -272,6 +293,11 @@ struct OverviewView: View { private func actionButtons(_ s: Snapshot) -> some View { let blocked = s.blocked let guardHolds = PostureUI.guardHoldsDownedTunnel(s) + // Titles are one or two words; the sentence each one used to carry + // inline ("Pause — use my real IP") is now the caption line below, fed + // by hover and keyboard focus. Same strings still go to `.help`, so the + // tooltip and the caption can never say different things. + // // ActionRow, not HStack: an HStack given less width than its children's // ideal sum compresses every one of them, so at a narrow window all // five labels truncated at once ("Block n…", "Switchin…", "Guard…"). @@ -279,83 +305,309 @@ struct OverviewView: View { // of whatever line it lands on — which is also why the Spacer that used // to sit before it is gone. return ActionRow(trailingCount: 1) { - Button("Block now") { AppActions.routine(["block"], "block") } - .disabled(blocked) - .help(state.routineHint("Cuts all traffic and holds it until you unblock.")) - Button("Unblock") { AppActions.routine(["unblock"], "unblock") } - .disabled(!(blocked || guardHolds)) - .help(state.routineHint("Releases a manual block and resumes monitoring.")) + // A disabled control says why, rather than describing an action it will + // not perform. This text was tooltip-only before the row's titles were + // shortened; promoting it to the primary visible explanation is what makes + // "Releases a manual block…" under a greyed-out button worth fixing. The + // Pause branch below already worked this way. + captioned("block", blocked + ? "Disabled — traffic is already blocked." + : state.routineHint("Cuts all traffic and holds it until you unblock.")) { + Button("Block") { AppActions.routine(["block"], "block") } + .disabled(blocked) + } + // Not one sentence for all three: this control is offered while a manual + // block is standing, while the guard holds a downed tunnel, and during + // FULL BLOCK, and it releases something different in each. See + // `PostureUI.unblockConsequence` — describing the other two as "a manual + // block" was survivable while this was a tooltip and is not now that it + // is the caption a user reads before clicking. + captioned("unblock", !(blocked || guardHolds) + ? "Disabled — there is no manual block or guard hold to release." + : state.routineHint(PostureUI.unblockConsequence(s))) { + Button("Unblock") { AppActions.routine(["unblock"], "unblock") } + .disabled(!(blocked || guardHolds)) + } if let sw = s.switch, sw.open, sw.isPause { // `switch --cancel` deliberately refuses to touch a pause (see the // glossary's Pause entry) — `resume` is the only way to end one early. - Button("Resume now" + sw.leftSuffix(asOf: state.now)) { - AppActions.routine(["resume"], "resume the guard") + captioned("resume", + state.routineHint("Ends the pause early and re-arms the guard.")) { + Button("Resume" + sw.leftSuffix(asOf: state.now)) { + AppActions.routine(["resume"], "resume the guard") + } } - .help(state.routineHint("Ends the pause early and re-arms the guard.")) } else if let sw = s.switch, sw.open { - Button("\(sw.isAutoRedial ? "Cancel redial window" : "Cancel VPN switch")" - + sw.leftSuffix(asOf: state.now)) { - AppActions.routine(["switch", "--cancel"], "cancel the switch window") + // Which window, not just "the window". Hovering this button replaces + // the posture headline — the only other place the distinction + // appears — so a caption that says neither leaves the user cancelling + // something unnamed, and the three triggers being distinct is most of + // what the guard's rules rest on. + captioned("cancel-window", + state.routineHint(sw.isAutoRedial + ? "Closes the automatic redial window and restores the guard." + : "Closes the switch window you opened and restores the guard.")) { + Button("Cancel" + sw.leftSuffix(asOf: state.now)) { + AppActions.routine(["switch", "--cancel"], "cancel the switch window") + } } - .help(state.routineHint("Closes the window and restores the guard.")) } else { switchMenu - Button("Pause — use my real IP") { AppActions.routine(["pause"], "pause the guard") } - .disabled(!state.pauseIsEnabled) - .help(state.pauseIsEnabled - ? state.routineHint("Deliberately drops to your real ISP IP, then re-arms the guard automatically.") - : "Disabled — vpn.pauseMax is \"0\" in your config.") + captioned("pause", state.pauseIsEnabled + ? state.routineHint("Uses your real ISP IP instead of the VPN, then re-arms the guard automatically.") + : "Disabled — vpn.pauseMax is \"0\" in your config.") { + Button("Pause") { AppActions.routine(["pause"], "pause the guard") } + .disabled(!state.pauseIsEnabled) + } + } + captioned("stop", + "Stops dezhban. Asks for your password — it can’t stop itself while running.") { + Button("Guard down") { AppActions.privileged(["stop"], "take the guard down") } } - Button("Guard down") { AppActions.privileged(["stop"], "take the guard down") } - .help("Stops dezhban. Asks for your password — it can’t stop itself while running.") } .frame(maxWidth: .infinity, alignment: .leading) } + /// Wraps one action control so pointing at it, or focusing it with the + /// keyboard, puts `hint` in the caption line — and puts the same string in + /// the tooltip, so the two surfaces cannot drift apart. + /// + /// `id` is what distinguishes controls in the hover/focus state; it is never + /// shown. Hover is cleared only by the control that owns the current hint, + /// so the pointer leaving A after it has already entered B does not blank + /// out B's caption. + /// + /// `focusable: false` skips the `.focused` binding, for a caller whose content is + /// a branch rather than a control. + /// + /// `.focused(_:equals:)` binds focus for a *focusable view*. Every slot but one + /// passes its Button straight through, so the binding lands on the control; the + /// switch slot passes an if/else, and a `_ConditionalContent` wrapper is not + /// something focus is defined to attach to. Rather than depend on whether it + /// happens to work, that caller applies the binding to the concrete Menu and + /// Button inside each branch and turns this off — the rest of the wrapper, which + /// is what must not disappear across the branch swap, is unaffected. + @ViewBuilder + private func captioned(_ id: String, _ hint: String, + focusable: Bool = true, + @ViewBuilder content: () -> Content) -> some View { + content() + .modifier(FocusBinding(id: focusable ? id : nil, focus: $focusedAction)) + .help(hint) + // And as an accessibility hint, not only as `help`. Shortening the titles + // moved the explanation into a caption line that is + // `accessibilityHidden` (it would otherwise be read twice), so VoiceOver + // was left with "Cancel", "Pause", "Guard down" and no disambiguation — + // worst for Cancel, which no longer says whether it closes an automatic + // redial window or one the user opened. A hint is where a control's + // consequence belongs, and it is announced after the label. + .accessibilityHint(hint) + .onHover { inside in + if inside { + // Whether this is the user aiming or a control arriving under a + // stationary pointer — see `ActionCaption.hoverIsAim`, which holds + // both halves of that question and is tested. It is asked here and + // nowhere else, so a control cannot opt out of it. + // + // The reference is where the mouse was when the KEYBOARD took the + // caption, in screen coordinates, not where it was at the last + // hover event. `onHover` fires on enter and exit only, so a + // reading taken here is the last boundary crossing rather than + // the hand's actual position, and both errors follow from that. + if ActionCaption.hoverIsAim(previousHoverID: hoveredAction?.id, id: id, + pointer: NSEvent.mouseLocation, + keyboardAimedAt: keyboardAimPoint) { + aim = .pointer + // Spent. The pointer has demonstrably moved since the keyboard + // took over, so there is nothing left to measure against — and + // a point kept past that would suppress a later entry that + // happened to land on the same pixel. + keyboardAimPoint = nil + } + hoveredAction = (id, hint) + } else if hoveredAction?.id == id { + hoveredAction = nil + } + } + .onChange(of: focusedAction) { _ in + guard focusedAction == id else { return } + focusedHint = hint + // Focus outranks a parked pointer, and does it by ranking rather + // than by clearing: `hoveredAction` used to be set to nil here, which + // threw the pointer's position away instead of deprioritising it — tab + // into the row and back out to anything outside it and both were empty, + // so the caption fell to the resting prompt while the pointer sat on a + // button, until it moved off and back on. + // + // The pointer takes the caption back by *entering* a control it was + // not already on, having actually moved to get there + // (`ActionCaption.hoverIsAim`). There is deliberately no "the mouse + // moved a little" re-arm on top of that: `onContinuousHover`'s + // `.active` fires when a tracking area is merely established, and + // these controls re-measure constantly (a window's countdown retitles + // "Cancel (m:ss left)" every second). The point it reports is in local + // space, so a banner appearing above the row moves the control under a + // stationary mouse and reads as movement — which is why the enter + // handler compares `NSEvent.mouseLocation` instead, on the screen, + // where a hand that has not moved has not moved. Jiggling inside the control + // you are already on aims at nothing new, so nothing is lost. + aim = .keyboard + // Where the mouse is at this instant, which is the reference the enter + // handler measures against. Sampled here rather than at each hover + // event on purpose: the question a later enter has to answer is "has + // the hand gone anywhere since the keyboard took over", and that is + // the only moment at which the answer's baseline exists. + keyboardAimPoint = NSEvent.mouseLocation + } + .onChange(of: hint) { newHint in + // The captured string has to track the live one. `state.routineHint` + // flips on `controlIsReachable`, and `.help` is recomputed every body + // pass while a hint captured at hover-enter is not — so the caption + // could say "No password needed" while the tooltip said the opposite, + // which is the one disagreement this wrapper exists to prevent. + // + // Not from the 1-second timer, which only polls the state file and + // repaints: `refreshServiceState()` runs at launch, when either + // surface opens, and after an action sequence. So the reachable + // window is narrower than "any moment" — an action completing while + // a control is hovered or focused is the realistic one. + if hoveredAction?.id == id { hoveredAction = (id, newHint) } + if focusedAction == id { focusedHint = newHint } + } + .onAppear { + // The mirror of `onDisappear` below, and needed for the same branch + // swap. `onChange` does not fire for a view inserted already matching + // the value it observes, so a control that arrives holding focus — + // AppKit can move first responder to the replacement before SwiftUI + // runs the outgoing view's `onDisappear`, which then declines to clear + // a hint it no longer owns — never wrote its own hint. The caption was + // left describing Pause while focus sat on Cancel, whose consequence is + // the opposite one. + if focusedAction == id { focusedHint = hint } + } + .onDisappear { + // A control can be replaced under a stationary pointer — Pause + // becomes Cancel the moment a window opens — and the removed view + // never receives a hover-exit, so its caption described a button that + // no longer exists until the mouse next moved. + if hoveredAction?.id == id { hoveredAction = nil } + if focusedAction == id { focusedHint = "" } + } + } + + /// The line that explains whatever the user is pointing at or has focused. + /// + /// Always present and always the same height — three reserved lines, see below. + /// A caption that collapsed between two buttons would reflow the row under the + /// pointer, which is the one thing it may never do. + private func actionCaption() -> some View { + // The resting text is a prompt, not the posture. `PostureUI.humanPosture` is + // exactly `display.headline`, which the hero already renders in title2 a + // hundred points above — so with the pointer off the row, which is the normal + // state, the pane said "Traffic cut" twice. The only requirement on the + // fallback is that the line never collapses and reflows the row. + Text(ActionCaption.text(hovered: hoveredAction?.hint, + focused: focusedAction == nil ? nil : focusedHint, + aim: aim, + fallback: "Point at a button to see what it does.")) + .font(.callout) + .foregroundStyle(.secondary) + // Three lines, always reserved. One line truncated the tail of every long + // caption, and the tail is where `routineHint` appends the password + // expectation — so Pause read "…Will ask for your pass…" and dropped the + // clause warning that a prompt is coming. Two was enough at a comfortable + // width and not at a narrow one: the pane is resizable, ActionRow is built + // to wrap, and the longest caption is ~137 characters, so the ellipsis + // landed back on the password clause exactly when the window was small. + // + // Reserving the space is what keeps the row from reflowing as the pointer + // crosses it, which is why this was capped at all; the cost of the third + // line is a little whitespace at wide widths, against a safety clause + // disappearing at narrow ones. The panic caption below makes the same + // trade by wrapping freely. + .lineLimit(3, reservesSpace: true) + .frame(maxWidth: .infinity, alignment: .leading) + .accessibilityHidden(true) + } + /// Opens a switch window, optionally targeted at a known VPN profile so the /// learned endpoint is attributed to it (`switch --no-wait --name `). /// Plain button when there are no profiles to pick from — a menu with a /// single "Any known VPN" entry would just be a worse button. - @ViewBuilder + /// One `captioned` wrapper around both shapes, not one per branch. + /// + /// Both used the id "switch", and `state.profiles` arrives asynchronously — so + /// the Button→Menu swap happens after launch, the outgoing branch's + /// `onDisappear` cleared the caption state for an id the incoming branch owns, + /// and the incoming one gets no hover-enter under a stationary pointer. Pointing + /// at "Switch VPN…" while profiles loaded therefore dropped the caption to the + /// resting prompt until the mouse moved. Wrapping once means the modifiers never + /// disappear, only their content changes. private var switchMenu: some View { - if let profiles = state.profiles, !profiles.profiles.isEmpty { - Menu("Switching VPN…") { - Button("Any known VPN") { - AppActions.routine(["switch", "--no-wait"], "open a switch window") - } - Divider() - ForEach(profiles.profiles) { p in - Button(p.name) { - AppActions.routine(["switch", "--no-wait", "--name", p.name], - "open a switch window for \(p.name)") + let hint = state.routineHint("Briefly relaxes the guard so a new VPN can connect.") + return captioned(Self.switchID, hint, focusable: false) { + if let profiles = state.profiles, !profiles.profiles.isEmpty { + Menu("Switch VPN…") { + Button("Any known VPN") { + AppActions.routine(["switch", "--no-wait"], "open a switch window") + } + Divider() + ForEach(profiles.profiles) { p in + Button(p.name) { + AppActions.routine(["switch", "--no-wait", "--name", p.name], + "open a switch window for \(p.name)") + } } } + .focused($focusedAction, equals: Self.switchID) + } else { + Button("Switch VPN…") { + AppActions.routine(["switch", "--no-wait"], "open a switch window") + } + .focused($focusedAction, equals: Self.switchID) } - .help(state.routineHint("Briefly relaxes the guard so a new VPN can connect.")) - } else { - Button("Switching VPN…") { AppActions.routine(["switch", "--no-wait"], "open a switch window") } - .help(state.routineHint("Briefly relaxes the guard so a new VPN can connect.")) } } + /// Shared by both switch shapes and by the caption wrapper around them, so the + /// two cannot drift apart. + private static let switchID = "switch" + private var panicRow: some View { - HStack(alignment: .firstTextBaseline, spacing: PaneMetrics.controlSpacing) { + // The sentence the shortened title no longer carries, said once and + // delivered three ways — visibly beside the button, as its tooltip, and as + // its VoiceOver hint. Held in a `let` so the three cannot drift. + let consequence = + "Force unblock: removes every dezhban firewall rule, even with dezhban not running." + return HStack(alignment: .firstTextBaseline, spacing: PaneMetrics.controlSpacing) { Button(role: .destructive) { guard AppActions.confirmPanic() else { return } AppActions.capturedPrivileged(["panic"]) { result in state.showInLogs(title: "dezhban — panic", text: result.output) } } label: { - Label("Panic — force unblock…", systemImage: "exclamationmark.octagon.fill") + Label("Panic…", systemImage: "exclamationmark.octagon.fill") } .tint(.red) .fixedSize() - Text("Removes every dezhban firewall rule, even with dezhban not running.") + // Same rule the action row's `captioned` applies, and for the same + // reason: shortening a title moves its explanation somewhere a pointer + // user and VoiceOver can each still reach it. Left off, the one + // destructive control on the pane announced "Panic…, button" and offered + // no tooltip at all — the gap `captioned` was written to close, open on + // the button where it costs the most. + .help(consequence) + .accessibilityHint(consequence) + Text(consequence) .font(.callout) .foregroundStyle(.secondary) // Wrap to a second line rather than truncate: this caption is // the sentence that stops someone pressing panic by accident. .fixedSize(horizontal: false, vertical: true) + // Hidden from VoiceOver because the button now carries it as a + // hint — exactly as the action row's caption line is hidden. Read + // from both, it would be announced twice. + .accessibilityHidden(true) } } @@ -429,6 +681,12 @@ struct OverviewView: View { .padding(.top, 4) // Panic stays reachable even from a degraded state — stale rules with // no daemon are exactly when the escape hatch matters. + // + // This one keeps its full title while the Overview's own panic + // button is just "Panic…": the rule is that a title may shed its + // explanation only where the explanation has somewhere else to + // live. Here there is no caption line and no action row — a lone + // "Panic…" in a guided empty state would explain itself to nobody. Button("Panic — force unblock…") { guard AppActions.confirmPanic() else { return } AppActions.capturedPrivileged(["panic"]) { result in @@ -441,3 +699,26 @@ struct OverviewView: View { .padding(24) } } + +/// Applies `.focused(_:equals:)` only when there is an id to bind. +/// +/// This does *not* make the branch identity-stable: `if let id { … } else { … }` in a +/// `ViewModifier.body` produces `_ConditionalContent` exactly as an inline `if` +/// would. It is safe because `focusable:` is a compile-time constant at every call +/// site, so `id` never flips for a given view. +/// +/// Which is the constraint to keep. Make `focusable` depend on state and the branch +/// swap destroys the wrapper's `onHover`/`onDisappear` state — the caption loss that +/// `switchMenu` was restructured to fix by wrapping both shapes once. +private struct FocusBinding: ViewModifier { + let id: String? + let focus: FocusState.Binding + + func body(content: Content) -> some View { + if let id { + content.focused(focus, equals: id) + } else { + content + } + } +} diff --git a/gui/macos/Sources/DezhbanMenu/SettingsView.swift b/gui/macos/Sources/DezhbanMenu/SettingsView.swift index d3c02fb..b0c6743 100644 --- a/gui/macos/Sources/DezhbanMenu/SettingsView.swift +++ b/gui/macos/Sources/DezhbanMenu/SettingsView.swift @@ -742,23 +742,30 @@ struct SettingsView: View { /// /// A tooltip has room for one sentence; the reason a setting exists, what it /// costs, and what happens when it is off often needs a page. This is the - /// bridge between the two — and it lands on the *heading*, from the key's - /// own `docAnchor`, so the answer is on screen rather than somewhere in a - /// long reference page. + /// bridge between the two — and it lands on the key's own table *row*, falling + /// back to its section, so the answer is on screen rather than somewhere in a + /// forty-key reference (`ConfigTunable.docTargets`). /// /// Absent when the schema is unavailable (a CLI too old to know /// `config schema`): a button that could only apologise is worse than none. @ViewBuilder private func docLink(_ key: String) -> some View { - if let tunable = schema?[key], !tunable.docAnchor.isEmpty { + if let tunable = schema?[key], !tunable.docTargets.isEmpty { Button { - state.openHelp(docAnchor: tunable.docAnchor) + state.openHelp(preferring: tunable.docTargets) } label: { Image(systemName: "questionmark.circle") } .buttonStyle(.borderless) .foregroundStyle(.secondary) - .help("Read about \(tunable.label) in the documentation") + // What the button does, because the key's own one-liner is already + // here: `schemaField`/`schemaToggle` put it on the control itself, and + // `schemaToggleWithCaption` shows it visibly underneath. Putting it on + // this button too hovered the same sentence twice and left nothing + // telling a pointer user that the control navigates — the only "opens + // the documentation" wording was the accessibility label, which a + // mouse never reaches. + .help("Open the documentation for \(tunable.label)") .accessibilityLabel("Documentation for \(tunable.label)") } } diff --git a/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift new file mode 100644 index 0000000..83b4d1b --- /dev/null +++ b/gui/macos/Tests/DezhbanCoreTests/ActionCaptionTests.swift @@ -0,0 +1,110 @@ +import CoreGraphics +import Testing +@testable import DezhbanCore + +struct ActionCaptionTests { + @Test func thePointerWinsWhileItIsWhatTheUserLastUsed() { + #expect(ActionCaption.text(hovered: "cut everything", + focused: "release the line", + aim: .pointer, + fallback: "point at a button") == "cut everything") + } + + /// Tabbing has to take the caption from a parked pointer, or the focus ring and + /// the Space key end up on one button while the caption describes another. + @Test func theKeyboardWinsAfterTabbing() { + #expect(ActionCaption.text(hovered: "cut everything", + focused: "release the line", + aim: .keyboard, + fallback: "point at a button") == "release the line") + } + + /// Ranking, not erasing. Tab into the row and back out of it — focus is gone, the + /// pointer has not moved — and the caption must still describe what it is on. + /// The caller used to clear the hover state instead, which left this case showing + /// the resting prompt over a control the pointer was physically resting on. + @Test func aParkedPointerSurvivesFocusLeavingTheRow() { + #expect(ActionCaption.text(hovered: "cut everything", + focused: nil, + aim: .keyboard, + fallback: "point at a button") == "cut everything") + } + + @Test func focusIsUsedWhenNothingIsHovered() { + #expect(ActionCaption.text(hovered: nil, + focused: "release the line", + aim: .pointer, + fallback: "point at a button") == "release the line") + } + + // MARK: - hoverIsAim + + private let p1 = CGPoint(x: 100, y: 200) + private let p2 = CGPoint(x: 140, y: 200) + + /// The pointer crossing from one control to another, having moved since the + /// keyboard took the caption. + @Test func movingOntoAnotherControlIsAim() { + #expect(ActionCaption.hoverIsAim(previousHoverID: "block", id: "pause", + pointer: p2, keyboardAimedAt: p1)) + } + + /// The keyboard never took the caption, so there is nothing for the pointer to + /// take it back from and no reading to swallow. + @Test func aHoverIsAimWhileTheKeyboardHasNotTakenIt() { + #expect(ActionCaption.hoverIsAim(previousHoverID: nil, id: "block", + pointer: p1, keyboardAimedAt: nil)) + } + + /// The same control re-firing. These retitle every second while a window + /// counts down, and each retitle re-establishes the tracking area. + @Test func theSameControlReFiringIsNotAim() { + #expect(!ActionCaption.hoverIsAim(previousHoverID: "cancel-window", + id: "cancel-window", + pointer: p2, keyboardAimedAt: p1)) + } + + /// The half the id check cannot see: Pause becomes Cancel when a window opens, + /// so the enter arrives with a *new* id while the hand has not moved. Read as + /// aiming, it handed the caption back to the pointer a moment after the keyboard + /// took it — the failure the id check was added for, through the other door. + @Test func aControlArrivingUnderAStationaryPointerIsNotAim() { + #expect(!ActionCaption.hoverIsAim(previousHoverID: nil, id: "cancel-window", + pointer: p1, keyboardAimedAt: p1)) + #expect(!ActionCaption.hoverIsAim(previousHoverID: "pause", id: "cancel-window", + pointer: p1, keyboardAimedAt: p1)) + } + + /// The reference is the moment the keyboard took over, not the last hover + /// event — which is why a hand that drifted a few points while reading, with no + /// hover event to record it, does not make the next swap look like aiming. Here + /// the pointer sits where it was when Tab moved the focus, having wandered + /// within the control since the enter that first put it there. + @Test func driftingInsideAControlDoesNotMakeALaterSwapLookLikeAiming() { + #expect(!ActionCaption.hoverIsAim(previousHoverID: nil, id: "cancel-window", + pointer: p2, keyboardAimedAt: p2)) + } + + /// And the other direction the old reference got wrong: a flick across the gap + /// between two buttons dispatches the exit and the enter from one mouse-moved + /// event, so both read the same location. Measured against the keyboard's + /// moment instead, the hand has plainly moved and the pointer takes the caption + /// back. + @Test func aFlickBetweenButtonsTakesTheCaptionBack() { + #expect(ActionCaption.hoverIsAim(previousHoverID: "block", id: "pause", + pointer: p2, keyboardAimedAt: p1)) + } + + /// The line must never go blank between two buttons: an empty caption + /// collapses its own height and reflows the row under the pointer. + @Test func fallbackFillsEveryGap() { + for aim: ActionCaption.Aim in [.pointer, .keyboard] { + #expect(ActionCaption.text(hovered: nil, focused: nil, + aim: aim, fallback: "point at a button") + == "point at a button") + #expect(ActionCaption.text(hovered: "", focused: " ", + aim: aim, fallback: "point at a button") + == "point at a button") + } + } +} diff --git a/gui/macos/Tests/DezhbanCoreTests/ActionRowPackingTests.swift b/gui/macos/Tests/DezhbanCoreTests/ActionRowPackingTests.swift index ff63cc0..6204f0e 100644 --- a/gui/macos/Tests/DezhbanCoreTests/ActionRowPackingTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/ActionRowPackingTests.swift @@ -3,9 +3,16 @@ import Testing @testable import DezhbanCore struct ActionRowPackingTests { - /// The Overview's standard-state row, measured at macOS 13pt bordered-button - /// metrics: Block now / Unblock / Switching VPN… (a Menu, so + chevron) / - /// Pause — use my real IP / Guard down. + /// The Overview's standard-state row: Block / Unblock / Switch VPN… (a Menu, + /// so + chevron) / Pause / Guard down. + /// + /// The widths are the ones measured at macOS 13pt bordered-button metrics + /// when those controls still carried their long titles ("Block now", + /// "Pause — use my real IP"). They are deliberately kept: what is under test + /// is the packing arithmetic and the break points these numbers exercise, + /// not the current text metrics. Shrinking them to match today's shorter + /// titles would make every row fit on one line and quietly stop testing the + /// wrapping this type exists for. Treat them as a fixture, not a measurement. private let overview: [CGFloat] = [94, 82, 141, 168, 110] private let spacing: CGFloat = 10 private let gutter: CGFloat = 24 @@ -85,11 +92,12 @@ struct ActionRowPackingTests { } /// The middle slot is state-dependent and can grow a live countdown - /// ("Cancel redial window (12:34 left)"), so the break point moves at - /// runtime. Packing must follow it rather than assume a fixed arity. + /// ("Cancel (12:34 left)"), so the break point moves at runtime. Packing + /// must follow it rather than assume a fixed arity. Same fixture caveat as + /// `overview` above: the widths exercise the break, they do not measure it. @Test func aLongerMiddleSlotMovesTheBreakPoint() { - let short: [CGFloat] = [94, 82, 141, 110] // "Cancel VPN switch" → 471 natural - let long: [CGFloat] = [94, 82, 215, 110] // "Cancel redial window (12:34 left)" → 545 + let short: [CGFloat] = [94, 82, 141, 110] // no countdown → 471 natural + let long: [CGFloat] = [94, 82, 215, 110] // with a live countdown → 545 // One pane width, two different answers — which is the point: nothing // here may be decided from the control COUNT, only from the measurements. #expect(pack(short, 500, pinnedFrom: 3).count == 1) diff --git a/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift b/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift index aaa731a..b9b873c 100644 --- a/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/HelpIndexTests.swift @@ -206,3 +206,78 @@ struct HelpIndexTests { == "file:///Applications/Dezhban.app/Contents/Resources/help/usage-config.html") } } + +/// The rule that keeps a contextual "?" from undoing its own scroll. +struct HelpNavigationTests { + private let page = URL(fileURLWithPath: "/A/help/usage-config.html") + private var anchored: URL { HelpURL.appendingFragment(page, "key-vpnredialwindow") } + + @Test func nothingLoadedMeansLoad() { + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: nil, servedAnchor: nil)) + } + + /// The same *bare* page is not reloaded. + @Test func theSamePageIsNotReloaded() { + #expect(!HelpNavigation.shouldLoad(target: page, loaded: page, servedAnchor: nil)) + } + + /// The same *anchor* is. An anchored target is an explicit "scroll here", and + /// the reader may have scrolled away since — clicking the same search hit again + /// to get back is how you ask. Answering it with "you are already there" made + /// the second click do nothing at all. + @Test func repeatingAnAnchorScrollsBackToIt() { + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: anchored, servedAnchor: nil)) + } + + /// …but not twice in one breath. `NSViewRepresentable` calls `updateNSView` + /// immediately after `makeNSView`, in the same runloop turn, while the anchor is + /// still pending — so opening the pane on a deep link asks for the identical + /// anchored URL twice, and honouring both started a load that cancelled the + /// first mid-flight. The caller records what it just served; the bare follow-up + /// clears it, which is what keeps the repeat click above working. + @Test func theSameTurnDuplicateIsNotServedTwice() { + #expect(!HelpNavigation.shouldLoad(target: anchored, loaded: anchored, + servedAnchor: anchored)) + // A different anchor is a different request, pending episode or not. + #expect(HelpNavigation.shouldLoad(target: HelpURL.appendingFragment(page, "key-other"), + loaded: anchored, servedAnchor: anchored)) + } + + /// The regression this exists for. The anchor is spent by the navigation it + /// triggered, so the very next view update asks for the same page with no + /// fragment — and answering it with a load put the reader back at the top of a + /// forty-key reference a fraction of a second after the "?" scrolled them to + /// their key. + @Test func droppingTheSpentAnchorDoesNotReload() { + #expect(!HelpNavigation.shouldLoad(target: page, loaded: anchored, servedAnchor: nil)) + } + + /// The other direction still navigates: a target that names an anchor is a + /// request to scroll somewhere, including from one key's row to another's. + @Test func aDifferentAnchorOnTheSamePageStillLoads() { + #expect(HelpNavigation.shouldLoad(target: HelpURL.appendingFragment(page, "key-other"), + loaded: anchored, servedAnchor: nil)) + #expect(HelpNavigation.shouldLoad(target: anchored, loaded: page, servedAnchor: nil)) + } + + @Test func anotherPageAlwaysLoads() { + let other = URL(fileURLWithPath: "/A/help/usage-cli.html") + #expect(HelpNavigation.shouldLoad(target: other, loaded: anchored, servedAnchor: nil)) + #expect(HelpNavigation.shouldLoad(target: other, loaded: page, servedAnchor: nil)) + } + + /// The based-URL trap `HelpURL` exists for, one layer up. `Bundle.main + /// .resourceURL` carries a base, so the page derived fresh each view update is + /// based while the one remembered from `appendingFragment` is absolute. Compared + /// as written they are unequal, and the reload — and the jump back to the top of + /// the page — comes straight back. + @Test func aBasedURLIsStillRecognisedAsTheLoadedPage() { + let app = URL(fileURLWithPath: "/Applications/Dezhban.app/", isDirectory: true) + let based = URL(string: "Contents/Resources/help/usage-config.html", relativeTo: app)! + #expect(based.baseURL != nil, "the fixture must reproduce a based URL, or it tests nothing") + + let anchoredAbsolute = HelpURL.appendingFragment(based, "key-vpnredialwindow") + #expect(!HelpNavigation.shouldLoad(target: based, loaded: anchoredAbsolute, servedAnchor: nil)) + #expect(!HelpNavigation.shouldLoad(target: based, loaded: based.absoluteURL, servedAnchor: nil)) + } +} diff --git a/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift b/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift index b85ed6a..4a63660 100644 --- a/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/PostureUITests.swift @@ -19,10 +19,13 @@ struct PostureUITests { obj["display"] = ["key": d.key, "headline": d.headline, "detail": d.detail] } if let e = enforcementErr { obj["enforcementErr"] = e } + // nil OMITS the key, rather than writing an empty array. `tunnels` is + // `omitempty` on the wire and `[Tunnel]?` here, so absent and `[]` decode to + // two different values and are two different cases — and writing `[]` for + // nil made the test for the absent one silently exercise the empty one, + // which another test already covered. Pass `[]` for the empty case. if let tuns = tunnels { obj["tunnels"] = tuns.map { ["name": $0.name, "up": $0.up] } - } else { - obj["tunnels"] = [] } let data = try! JSONSerialization.data(withJSONObject: obj) return StateReader.decode(data)! @@ -148,6 +151,92 @@ struct PostureUITests { snapshot(posture: "guard", display: nil, tunnels: [(name: "utun4", up: true)]))) } + // MARK: - unblockConsequence + + /// Overview enables Unblock in two states and its caption is now the primary + /// pre-click explanation, so the sentence has to name what is actually being + /// released. + /// + /// `postureName` derives "full-block" from `blocked` alone, so an operator's + /// own block and a blocked-country escalation are the same string on the wire. + /// The caption may therefore claim neither — only what is true of both. + @Test func unblockConsequenceNamesTheFullBlockItLifts() { + let text = PostureUI.unblockConsequence(snapshot(posture: "full-block", + display: Self.downedGuardDisplay)) + #expect(text.contains("full block")) + #expect(!text.contains("manual")) + } + + /// The posture check may not come first. `runner`'s unblock handler branches on + /// `AutoArm && !tunnelUp && !standby` without looking at *why* egress was cut, + /// so a full block standing over a downed tunnel — `dezhban block` with the VPN + /// off, or a tunnel that dropped under a geo block, which opens no redial window + /// — drops to STANDBY exactly like the guard case does. Answering that with + /// "resumes monitoring" is the same false promise this function removed from the + /// guard branch, in the branch the first version did not cover. + @Test func unblockConsequenceWarnsWhenAFullBlockSitsOverADownedTunnel() { + let text = PostureUI.unblockConsequence( + snapshot(posture: "full-block", display: Self.downedGuardDisplay, + tunnels: [(name: "utun4", up: false)])) + #expect(text.contains("real IP")) + #expect(!text.contains("resumes monitoring")) + } + + /// …and an absent tunnel list reads the same way. It is what the daemon sends + /// when it has none (`omitempty`), and guessing "up" there would put the false + /// promise back for the one snapshot that says least. + @Test func unblockConsequenceTreatsNoTunnelAsDown() { + let text = PostureUI.unblockConsequence( + snapshot(posture: "full-block", display: Self.downedGuardDisplay, tunnels: nil)) + #expect(text.contains("real IP")) + } + + /// The caption reserves three lines sized for the longest hint the row already + /// had, and `AppState.routineHint` appends 60 characters to whatever this + /// returns. A longer sentence truncates, and the tail that disappears is the + /// password clause — the exact failure the three-line reservation was + /// introduced to prevent. + @Test func everyUnblockSentenceFitsTheCaption() { + let states = [ + snapshot(posture: "guard"), + snapshot(posture: "guard", display: Self.downedGuardDisplay), + snapshot(posture: "full-block", display: Self.downedGuardDisplay), + snapshot(posture: "full-block", display: Self.downedGuardDisplay, + tunnels: [(name: "utun4", up: false)]), + ] + for s in states { + let text = PostureUI.unblockConsequence(s) + #expect(text.count <= 80, "too long for the caption (\(text.count)): \(text)") + } + #expect(PostureUI.unblockConsequence(nil).count <= 80) + } + + /// With `vpn.autoArm` on — the default — an explicit unblock with the tunnel + /// down drops the daemon to STANDBY, which installs nothing. Saying + /// "resumes monitoring" there was the opposite of what happens. + @Test func unblockConsequenceNamesTheRealIPExposure() { + let text = PostureUI.unblockConsequence( + snapshot(posture: "guard", display: Self.downedGuardDisplay)) + #expect(text.contains("real IP")) + #expect(!text.contains("resumes monitoring")) + } + + /// A healthy guard does not offer the button at all (`blocked` is false and the + /// guard is not holding), so this is the default rather than a live state — but + /// it must still be a sentence, and must not inherit either warning. + @Test func unblockConsequenceFallsBackToThePlainSentence() { + let text = PostureUI.unblockConsequence(snapshot(posture: "guard")) + #expect(text.contains("resumes monitoring")) + #expect(!text.contains("real IP")) + #expect(!text.contains("full block")) + } + + /// No snapshot is not a state the button is offered in, but the function must + /// still answer with a sentence rather than an empty caption. + @Test func unblockConsequenceHasATextForNoSnapshot() { + #expect(!PostureUI.unblockConsequence(nil).isEmpty) + } + // MARK: - dockState / mmss / agoString @Test func dockStateCoarsensToBlockedOrOn() { diff --git a/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift b/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift index ce9e132..7aa75f5 100644 --- a/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift @@ -5,6 +5,36 @@ import Testing /// Builds a schema good enough to exercise the pane's use of it, without /// shelling out to the CLI. Kinds and labels match what `config schema --json` /// reports for these keys. +/// Mirrors Go's `config.anchorSlug` / `help.Anchor`: lowercase, keep ASCII +/// letters, digits and hyphens, turn a space into a hyphen, drop everything else — +/// so a dot is **dropped**, not turned into a hyphen. +/// +/// Spelled out here because the fixture is the app-side reference for a +/// cross-language contract, and it had the rule wrong: it mapped "." to "-" and +/// then asserted `key-vpn-endpointrefresh`, which the pipeline never emits. Nothing +/// failed, because the test only exercised decoding and ordering — but the next +/// person deriving an anchor from this fixture would have got a fragment resolving +/// nowhere. Go's TestKeyAnchorSlugMatchesTheRenderer pins the two derivations that +/// actually ship; this keeps the fixture honest about them. +/// +/// Written over `unicodeScalars` with explicit ASCII ranges rather than +/// `isLowercase`/`isNumber`, which was the same near-miss one layer down: those +/// accept letters and digits outside ASCII that Go's `r >= '0' && r <= '9'` +/// rejects, and neither spelling handled the space at all. No config key exercises +/// either today — which is precisely why the fixture, not a key, has to carry the +/// rule correctly. +private func goAnchorSlug(_ key: String) -> String { + var out = "" + for scalar in key.lowercased().unicodeScalars { + switch scalar { + case "a"..."z", "0"..."9", "-": out.unicodeScalars.append(scalar) + case " ": out.append("-") + default: break + } + } + return out +} + private func testSchema() -> ConfigSchema { func tunable(_ key: String, _ label: String, _ kind: String, defaultValue: String = "", capKey: String? = nil, @@ -13,7 +43,8 @@ private func testSchema() -> ConfigSchema { {"key":"\(key)","label":"\(label)","kind":"\(kind)","default":"\(defaultValue)", \(capKey.map { "\"capKey\":\"\($0)\"," } ?? "") "disablable":\(disablable),"advanced":false,"preset":false, - "help":"help for \(key)","docAnchor":"usage/config.md#fields"} + "help":"help for \(key)","docAnchor":"usage/config.md#fields", + "docKeyAnchor":"usage/config.md#key-\(goAnchorSlug(key))"} """ return try! JSONDecoder().decode(ConfigTunable.self, from: Data(json.utf8)) } @@ -251,13 +282,33 @@ struct ConfigSchemaTests { /// Every control's help link is only as good as the anchor it carries. Go's /// TestEveryTunableDocAnchorResolves proves the anchors exist in the - /// bundled pages; this proves the app turns them into a page and a heading - /// rather than dropping the fragment and landing at the top. - @Test func docAnchorBecomesADeepLink() { + /// bundled pages; this proves the app turns them into a page and a fragment + /// rather than dropping the fragment and landing at the top — and that it + /// offers the key's own row *before* the section, with the section still there + /// as the step to fall back to when a bundle predates row ids. + @Test func docAnchorsBecomeDeepLinksRowFirst() { let schema = testSchema() - let target = try! #require(schema["vpn.endpointRefresh"]?.docTarget) - #expect(target.source == "usage/config.md") - #expect(target.anchor == "fields") + let targets = try! #require(schema["vpn.endpointRefresh"]?.docTargets) + #expect(targets.count == 2) + #expect(targets[0].source == "usage/config.md") + // Dots dropped, not hyphenated — see `goAnchorSlug`. + #expect(targets[0].anchor == "key-vpnendpointrefresh") + #expect(targets[1].anchor == "fields") + } + + /// A key documented in prose has no row to land on, so it offers the section + /// alone rather than a fabricated row id. Absent in the JSON, not empty: an + /// older CLI that does not know the field at all has to behave the same way. + @Test func aKeyWithoutARowOffersOnlyItsSection() { + let json = """ + {"key":"some.key","label":"Some key","kind":"text","default":"", + "disablable":false,"advanced":false,"preset":false, + "help":"","docAnchor":"usage/config.md#fields"} + """ + let tunable = try! JSONDecoder().decode(ConfigTunable.self, from: Data(json.utf8)) + #expect(tunable.docKeyAnchor == nil) + #expect(tunable.docTargets.count == 1) + #expect(tunable.docTargets[0].anchor == "fields") } /// The placeholder states the real default, which is the whole point: the diff --git a/internal/config/docdrift_test.go b/internal/config/docdrift_test.go index ee7623e..77cf1ae 100644 --- a/internal/config/docdrift_test.go +++ b/internal/config/docdrift_test.go @@ -169,9 +169,13 @@ func parseDocDefaults(t *testing.T, path string) map[string]string { for rawLine := range strings.SplitSeq(string(data), "\n") { line := strings.TrimSpace(rawLine) if !strings.HasPrefix(line, "|") { - // A heading sets the key prefix for the tables under it: the - // advanced table lists bare field names (`switchWindowMax`) because - // its heading already says which block they live in. + // A heading sets the key prefix for the tables under it. The advanced + // table now names its keys in full (`vpn.advanced.switchWindowMax`), + // which is what lets its rows carry anchors — so this prefix is + // currently unused by every table in the reference. Kept because it is + // the mechanism that makes a bare-name table legal at all, and a + // future block may want one; a table that reverted to bare names + // would otherwise fail the drift check for the wrong reason. if strings.HasPrefix(line, "#") { prefix = "" if strings.Contains(line, "vpn.advanced") { diff --git a/internal/config/keyanchororder_test.go b/internal/config/keyanchororder_test.go new file mode 100644 index 0000000..d8259c9 --- /dev/null +++ b/internal/config/keyanchororder_test.go @@ -0,0 +1,192 @@ +package config + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// The sections of the reference that *define* keys, as opposed to comparing, +// renaming or retiring them. +var definitionSections = map[string]bool{ + "Fields": true, + "`control` block": true, + "`vpn` block": true, + "Advanced tunables (`vpn.advanced`)": true, +} + +// A *lone* code span in the first cell, exactly as help.rowKey requires — nothing +// else in the cell. +// +// Looser than that, this test recorded rows the renderer never claims: a row +// written "| `vpn.redialWindow` (renamed) | … |", the natural way to annotate the +// Renamed table, would have been reported as that key's first row and failed the +// build even though claimKey had correctly anchored the definition. A spurious +// failure on the one test that exists to catch a silent one. +var firstCellKey = regexp.MustCompile("^\\|\\s*`([^`]+)`\\s*\\|") + +var sectionHeading = regexp.MustCompile(`^#{2,6}\s+(.*)$`) + +// A table's separator row, and the first cell of a header row. +var ( + tableSeparator = regexp.MustCompile(`^\|[\s:|-]+\|$`) + firstHeaderCell = regexp.MustCompile(`^\|([^|]*)\|`) +) + +// headsAKeyTable mirrors help.renderTable's gate: inline markup stripped, then a +// case-insensitive compare against "Field". +// +// A literal regex on the raw cell did not mirror it. `| `Field` |`, `| **Field** |` +// and `| FIELD |` all make the *renderer* mint row anchors from that table while a +// pattern matching bare "Field" stops treating it as a key table — so the guarantee +// this file exists to pin would silently stop covering it, and the `checked == 0` +// fatal does not help because it only fires when *every* key falls out, not a +// subset. Copied rather than called: internal/help's own tests import this package, +// so importing help from here would close an import cycle. +func headsAKeyTable(headerLine string) bool { + m := firstHeaderCell.FindStringSubmatch(headerLine) + if m == nil { + return false + } + // Links and images reduce to their text FIRST, then emphasis characters go. + // Both steps, because stripInline does both: with only the second, a header + // written `| [Field](../x.md) |` makes the renderer mint row anchors for that + // table while this file stops treating it as a key table — the coverage + // narrowing silently, and the `checked == 0` fatal no help because it fires + // only when EVERY key falls out, never a subset. The same near-miss the + // comment above warns about, one construct over. + cell := inlineImageText.ReplaceAllString(m[1], "$1") + cell = inlineLinkText.ReplaceAllString(cell, "$1") + cell = strings.Map(func(r rune) rune { + switch r { + case '`', '*', '_': + return -1 + } + return r + }, cell) + return strings.EqualFold(strings.TrimSpace(cell), "Field") +} + +// Mirroring help's own inlineImage/inlineLink, image first so `![a](b)` does not +// reduce to a stray `!`. +var ( + inlineImageText = regexp.MustCompile(`!\[([^\]]*)\]\([^)]+\)`) + inlineLinkText = regexp.MustCompile(`\[([^\]]*)\]\([^)]+\)`) +) + +// TestKeyRowsAnchorToTheirDefinitionSection pins the document arrangement that +// help.claimKey depends on. +// +// The renderer settles most of this itself now: only a table heading its first +// column "Field" defines keys, so config.md's presets and retired tables — headed +// "Key" — cannot claim an anchor however early they appear. What it still cannot +// settle is *two* Field-headed tables naming the same key, where the first one +// wins; refusing a duplicate loudly is not an option, since several keys are +// legitimately repeated. +// +// So this is the remaining guard, and it is a second line rather than the only one. +// For every settable key, the first Field-table row naming it must fall under one of +// the reference's definition headings. Nothing else notices if that changes: the +// resolution test stays green as long as some row carries the anchor. +func TestKeyRowsAnchorToTheirDefinitionSection(t *testing.T) { + // Read out of the anchors rather than restated here. Today that always yields + // exactly one page — docKeyAnchorFor builds every anchor from keyReferencePage — + // so this is not extra coverage; it is the test declining to hold a second copy + // of a constant it does not own. If the reference is ever split across pages, + // this reads the split instead of failing on it. + pages := map[string]bool{} + for _, tun := range Tunables() { + if tun.DocKeyAnchor == "" { + continue + } + page, _, _ := strings.Cut(tun.DocKeyAnchor, "#") + pages[page] = true + } + if len(pages) == 0 { + t.Fatal("no tunable carries a row anchor — this test is no longer pinning anything") + } + + // key -> the heading its first row appeared under, per page. + seen := map[string]map[string]string{} + for page := range pages { + path := filepath.Join("..", "..", "docs", filepath.FromSlash(page)) + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + seen[page] = firstRowSections(string(data)) + } + + checked := 0 + for _, tun := range Tunables() { + if tun.DocKeyAnchor == "" { + continue // documented in prose, so there is no row to order + } + page, _, _ := strings.Cut(tun.DocKeyAnchor, "#") + where, ok := seen[page][tun.Key] + if !ok { + t.Errorf("%s: no table row on %s names this key, so it has no anchor to claim", + tun.Key, page) + continue + } + if !definitionSections[where] { + t.Errorf("%s: its first table row on %s is under %q, so the help link would land "+ + "there rather than on its definition", tun.Key, page, where) + } + checked++ + } + if checked == 0 { + t.Fatal("checked no keys — this test is no longer pinning the document order") + } +} + +// firstRowSections maps each key to the heading under which its first table row +// appears, skipping fenced code the way the renderer does. +func firstRowSections(markdown string) map[string]string { + out := map[string]string{} + section := "" + inCode := false + // Only rows inside a real table, and only under a "Field" header — the same two + // conditions renderTable applies. Without the separator check a pipe-prefixed + // prose line counted as a row; without the header check a presets or retired row + // did, and neither is something claimKey would ever anchor. Both would have + // failed the build over a row the renderer ignores, which is the false-failure + // this test has already been fixed for once. + lines := strings.Split(markdown, "\n") + inFieldTable := false + for i, raw := range lines { + line := strings.TrimSpace(raw) + if strings.HasPrefix(line, "```") { + inCode = !inCode + continue + } + if inCode { + continue + } + if m := sectionHeading.FindStringSubmatch(line); m != nil { + section = strings.TrimSpace(m[1]) + inFieldTable = false + continue + } + if !strings.HasPrefix(line, "|") { + inFieldTable = false + continue + } + // A header row is one followed by a separator; that is what starts a table. + if i+1 < len(lines) && tableSeparator.MatchString(strings.TrimSpace(lines[i+1])) { + inFieldTable = headsAKeyTable(line) + continue + } + if tableSeparator.MatchString(line) || !inFieldTable { + continue + } + if m := firstCellKey.FindStringSubmatch(line); m != nil { + if _, dup := out[m[1]]; !dup { + out[m[1]] = section + } + } + } + return out +} diff --git a/internal/config/schema.go b/internal/config/schema.go index 03557c9..8b04cb5 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -1,6 +1,9 @@ package config -import "sort" +import ( + "sort" + "strings" +) // This file exists because a default stated in more than one place is a default // that will drift — and this one already had. Before it, every tunable's default @@ -85,12 +88,25 @@ type Tunable struct { // Help is one line explaining what the key does and what it costs. Help string `json:"help"` - // DocAnchor points at the section of the bundled documentation that covers - // this key, as "#". Section-level rather than per-key because - // that is the granularity docs/usage/config.md actually has; a test asserts - // every anchor resolves in the generated help bundle. + // DocAnchor points at the *section* of the documentation that covers this + // key, as "#". A heading anchor, so it resolves everywhere + // markdown does — on GitHub, in an editor's preview, and in the app's bundled + // help. This is what a CLI surface prints, because it is the only one a reader + // outside the app can follow. DocAnchor string `json:"docAnchor"` + // DocKeyAnchor points at the key's own table row, as "#", or is + // empty for a key documented in prose rather than a row. + // + // Additional to DocAnchor, never a replacement for it. Row ids exist only in + // the HTML tools/helpgen renders; markdown viewers generate heading anchors + // only. Overwriting DocAnchor with this therefore handed every CLI user a + // fragment that resolves nowhere, to fix a granularity problem only the app + // had — and it removed the app's own middle step on version skew, where a CLI + // newer than the bundled help finds no row id and would otherwise fall all the + // way back to the top of a forty-key reference instead of to the section. + DocKeyAnchor string `json:"docKeyAnchor,omitempty"` + // RestartReason is why a running daemon cannot adopt this key in place, or "" // when it can. Derived from restartReasonFor — never restated here, so a key // cannot claim to be live in one table and restart-required in another. @@ -100,8 +116,16 @@ type Tunable struct { // LiveAppliable reports whether a running daemon adopts this key in place. func (t Tunable) LiveAppliable() bool { return t.RestartReason == "" } -// Doc anchors. Keys are documented by section, so these are the four sections of -// docs/usage/config.md that between them cover every key. +// Doc anchors — the *section* of docs/usage/config.md that covers a group of keys, +// and every key's DocAnchor unconditionally. These are heading anchors, so they are +// what `dezhban config schema` prints for a reader who will open the file on GitHub, +// and the app's second choice after the key's own row (docKeyAnchorFor derives that +// separately, and it is additional rather than a replacement). +// +// Not a "fallback for keys documented in prose", which is what this said: that was +// true only of an intermediate design where the row anchor overwrote this one. +// keysDocumentedInProse is empty today, and even if it were not, these would still +// be every key's section. const ( anchorFields = "usage/config.md#fields" anchorControl = "usage/config.md#control-block" @@ -475,11 +499,78 @@ func Tunables() []Tunable { for i, t := range tunables { t.Default = defaults[t.Key] t.RestartReason = restartReasonFor(t.Key) + t.DocKeyAnchor = docKeyAnchorFor(t.Key) out[i] = t } return out } +// docKeyAnchorFor derives the anchor of a key's own row in the reference, to sit +// alongside its declared section anchor rather than in place of it. +// +// The reference documents each key as a table row opening with the key in a code +// span, and the help renderer gives every such row an id (help.KeyAnchor). So a +// contextual help link can land on the key the reader clicked rather than on a +// section heading that four dozen keys share, which is what the four constants +// above delivered on their own. +// +// Alongside, because a row id is not a markdown anchor: it exists only in the HTML +// tools/helpgen renders. Replacing the section anchor with it therefore broke every +// reader outside the app — `config schema` prints the anchor for someone who will +// open the file on GitHub — and cost the app its middle step when a CLI is newer +// than the bundled help. +// +// Derived rather than hand-written, for the same reason defaults are: forty-odd +// anchors restated by hand is forty-odd chances to drift. keysDocumentedInProse +// names the exceptions, and TestEveryTunableDocAnchorResolves fails the build +// naming any key whose derived anchor does not exist — so a key that loses its +// row cannot silently fall back to landing somewhere plausible and wrong. +func docKeyAnchorFor(key string) string { + if keysDocumentedInProse[key] { + return "" + } + return keyReferencePage + "#key-" + anchorSlug(key) +} + +// keyReferencePage is where the key *rows* live — the one page whose Field-headed +// tables the help renderer anchors (help.renderTable). +// +// Named here rather than taken from a key's section anchor, which is what this used +// to do. That coupled two things which need not agree: point one key's DocAnchor at +// a section on another page — a concept-heavy key at concepts/modes.md, say — and +// its derived row anchor named that page too, so the resolution test failed saying +// nothing there carries the anchor, even though the key does have a row, in +// config.md. The only escape was to declare the key "documented in prose", which is +// the wrong statement about it. +// +// docKeyAnchorFor takes no anchor argument at all, for the same reason: threading +// the section anchor in only to ignore it leaves the next edit one `_` away from +// reintroducing the coupling this constant exists to remove. +const keyReferencePage = "usage/config.md" + +// keysDocumentedInProse are the keys docs/usage/config.md covers outside a table +// row, which therefore have no row anchor to land on. They get an empty +// DocKeyAnchor and are reached through their section anchor alone. Adding a row +// for one of these is an improvement — delete it from here when you do. +var keysDocumentedInProse = map[string]bool{} + +// anchorSlug mirrors help.Anchor's rule (GitHub's), applied to a config key. +// Duplicated deliberately rather than imported: internal/help renders the docs +// and would import this package to check its work, so depending on it here would +// close a cycle. TestKeyAnchorSlugMatchesTheRenderer pins the two together. +func anchorSlug(key string) string { + var b strings.Builder + for _, r := range strings.ToLower(key) { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '-': + b.WriteRune(r) + case r == ' ': + b.WriteByte('-') + } + } + return b.String() +} + // TunableByKey looks one key up. The bool is false for a key that is not // settable, which callers should treat as "unknown key", not "no metadata". func TunableByKey(key string) (Tunable, bool) { diff --git a/internal/help/bundle.go b/internal/help/bundle.go index 4c34bdb..603f052 100644 --- a/internal/help/bundle.go +++ b/internal/help/bundle.go @@ -19,6 +19,11 @@ type IndexEntry struct { Summary string `json:"summary"` Tutorial int `json:"tutorial,omitempty"` Headings []Heading `json:"headings"` + // Keys are the per-row anchors of the page's reference tables, so a + // contextual help link can resolve to one config key rather than to the + // section heading it shares with dozens of others. Omitted for the pages + // that document no keys. + Keys []Heading `json:"keys,omitempty"` // Text is the page stripped to words, so search can run entirely in the app // with no second pass over the HTML. Text string `json:"text"` @@ -57,7 +62,8 @@ func Build(docsDir, outDir string) ([]IndexEntry, error) { index = append(index, IndexEntry{ File: name, Source: page.Source, Title: page.Title, Summary: page.Summary, - Tutorial: page.Tutorial, Headings: rendered.Headings, Text: rendered.Text, + Tutorial: page.Tutorial, Headings: rendered.Headings, Keys: rendered.Keys, + Text: rendered.Text, }) } diff --git a/internal/help/help_test.go b/internal/help/help_test.go index 3505944..89abbdd 100644 --- a/internal/help/help_test.go +++ b/internal/help/help_test.go @@ -54,35 +54,98 @@ func TestBundleBuilds(t *testing.T) { // TestEveryTunableDocAnchorResolves ties the settings schema to the bundle. A // contextual help link is a promise that the section exists; a stale anchor // silently lands the reader at the top of a long reference page instead. +// +// Both anchors a Tunable carries are checked, because they are load-bearing for +// different readers. DocAnchor is a *heading* anchor and must resolve everywhere +// markdown does, since a CLI prints it for someone reading the file on GitHub. +// DocKeyAnchor names the key's own row (config.docKeyAnchorFor) and exists only in +// the rendered HTML; it is what gives the app's contextual help its per-key grain, +// and a key that loses its documentation row fails here by name rather than +// degrading into a link to the top of a long reference. func TestEveryTunableDocAnchorResolves(t *testing.T) { index := buildInto(t) - anchors := map[string]map[string]bool{} + headings := map[string]map[string]bool{} + keyRows := map[string]map[string]bool{} for _, e := range index { - set := map[string]bool{} + hs := map[string]bool{} for _, h := range e.Headings { - set[h.Anchor] = true + hs[h.Anchor] = true + } + headings[e.Source] = hs + ks := map[string]bool{} + for _, k := range e.Keys { + ks[k.Anchor] = true } - anchors[e.Source] = set + keyRows[e.Source] = ks } - for _, tun := range config.Tunables() { - page, frag, found := strings.Cut(tun.DocAnchor, "#") + check := func(t *testing.T, key, field, value string, in map[string]map[string]bool) { + t.Helper() + page, frag, found := strings.Cut(value, "#") if !found { - t.Errorf("%s: DocAnchor %q has no fragment", tun.Key, tun.DocAnchor) - continue + t.Errorf("%s: %s %q has no fragment", key, field, value) + return } - set, ok := anchors[page] + set, ok := in[page] if !ok { - t.Errorf("%s: DocAnchor names %q, which is not a bundled page", tun.Key, page) - continue + t.Errorf("%s: %s names %q, which is not a bundled page", key, field, page) + return } if !set[frag] { - t.Errorf("%s: no heading in %s has the anchor %q", tun.Key, page, frag) + t.Errorf("%s: nothing in %s has the anchor %q (%s)", key, page, frag, field) + } + } + + for _, tun := range config.Tunables() { + // A heading, specifically: this is the one a CLI prints for a reader who + // will open the file on GitHub, where row ids do not exist. + check(t, tun.Key, "DocAnchor", tun.DocAnchor, headings) + if tun.DocKeyAnchor != "" { + // Key rows only, not rows-or-headings. Accepting either defeated this + // function's own promise: a key whose table row is deleted would still + // pass whenever some heading on the page happened to slug to the same + // `key-…` fragment — and such headings exist ("Key flags" in cli.md) — + // so the app would silently deep-link to a heading instead of failing + // here by name. + check(t, tun.Key, "DocKeyAnchor", tun.DocKeyAnchor, keyRows) } } } +// TestKeyAnchorSlugMatchesTheRenderer pins config.anchorSlug (which cannot +// import this package — internal/help imports internal/config to check its own +// work) to help.KeyAnchor. The two derive the same fragment id from opposite +// ends of the same link, and a divergence would break every contextual help +// link at once while both packages' own tests still passed. +func TestKeyAnchorSlugMatchesTheRenderer(t *testing.T) { + checked := 0 + for _, tun := range config.Tunables() { + // DocKeyAnchor, not DocAnchor: the latter is a *section* anchor, so reading it + // here made every iteration skip and the test could not fail — while its doc + // comment claimed to be the pin against exactly this divergence. + if tun.DocKeyAnchor == "" { + continue // documented in prose; covered by the resolution test above + } + // Compared directly, with no "is it in the bundle" gate. Such a gate is the + // second way this went inert: a slug that has *diverged* is by definition not + // among the rendered anchors, so gating on presence skipped precisely the + // keys it was meant to catch. Whether the anchor exists is the resolution + // test's question; whether the two derivations agree is this one's. + page, frag, _ := strings.Cut(tun.DocKeyAnchor, "#") + if want := KeyAnchor(tun.Key); frag != want { + t.Errorf("%s: schema derived %q, renderer derives %q (page %s)", + tun.Key, frag, want, page) + } + checked++ + } + // A test that silently checks nothing is worse than no test: this one already + // went inert once, when the field it reads stopped being the key anchor. + if checked == 0 { + t.Fatal("compared no anchors — the schema and the renderer are no longer being pinned together") + } +} + // TestBundleIsSelfContained — the pane opens when the kill switch has cut every // byte of egress, so a page that reaches for a CDN would render broken at // exactly the moment it is needed. diff --git a/internal/help/indentcollide_test.go b/internal/help/indentcollide_test.go new file mode 100644 index 0000000..509efd2 --- /dev/null +++ b/internal/help/indentcollide_test.go @@ -0,0 +1,26 @@ +package help + +import ( + "strings" + "testing" +) + +// An indented heading is still a heading, so a key row that collides with it must +// still be refused — the pre-scan and the main pass have to agree about what +// counts. They did not: the scan matched the raw line, the renderer the trimmed +// one, and a heading with up to three leading spaces slipped through to produce +// two elements sharing one id and a green build. +func TestIndentedHeadingStillBlocksACollidingKeyRow(t *testing.T) { + md := " ## Key flags\n\n| Field | Default |\n|---|---|\n| `flags` | `x` |\n" + r := Render("probe.md", md) + + if !strings.Contains(r.HTML, `

the key that claimed it, not merely "claimed", because + // those are two different situations. Refusing a *repeat of the same key* is + // benign and therefore silent: config.md documents several keys in more than one + // table and failing the build on that would be absurd. Two *different* keys + // landing on one anchor is a silently wrong deep link, and is reported the way a + // heading collision is. + // + // They can meet because `Anchor` lowercases and drops `.` (hyphens survive, so + // `vpn.pause-max` is not an example — checked). The real pairs are a case-only + // rename, `vpn.armAtBoot` against `vpn.armatboot`, and one that removes a dot, + // `vpn.pauseMax` against `vpnpauseMax`. + // + // What this cannot judge is which of two *Field* tables is the definition: it + // takes the first, and that is the definition because the reference is arranged + // that way. config.md's presets and retired tables are headed "Key" and its + // rename table "Old name", so none of them can claim an anchor at all — + // renderTable's header gate settles that. Two Field tables naming one key is the + // remaining case, and config.TestKeyRowsAnchorToTheirDefinitionSection pins it. + claimedKeys := map[string]string{} + // Heading anchors are collected up front, because a heading can appear *after* + // the row that would collide with it while r.Headings is filled in the same + // single pass below. + // + // The `key-` prefix was assumed to make collision impossible. It does not: a + // heading of "Key flags" — docs/usage/cli.md has one — slugs to exactly + // `key-flags`, which is also what a row for a key named `flags` would claim. + // Two elements with one id is invalid HTML and a browser jumps to whichever came + // first, so the reader lands on the heading and the contextual link is silently + // wrong. The heading wins, being the anchor markdown itself generates and the one + // written links depend on, and the row is *noted* rather than quietly skipped: + // Unsupported fails the bundle build (bundle.go), so this becomes a build error + // naming the page instead of a link that misses. + // + // The scan goes through headingRe and skips code fences, so it sees exactly what + // the main pass will call a heading. "Any trimmed line starting with #" was + // looser: `#nospace`, seven hashes, and — worst — a shell comment inside a fenced + // block all registered as phantom headings, and one that happened to slug onto a + // real row's anchor would refuse that row and fail the whole app build naming a + // heading nobody can find. + headingAnchors := map[string]bool{} + scanInCode := false + for _, line := range strings.Split(markdown, "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "```") { + scanInCode = !scanInCode + continue + } + if scanInCode { + continue + } + // The TRIMMED line, exactly as the main pass matches it. Markdown allows up + // to three leading spaces on an ATX heading, and this renderer honours that + // — so matching the raw line here let " ## Key flags" render as a heading + // that the pre-scan never saw, and a colliding row was then claimed anyway. + // Two elements with one id, the browser jumping to the heading, and a green + // build: precisely the failure the guard was added to turn into an error. + if m := headingRe.FindStringSubmatch(strings.TrimSpace(line)); m != nil { + if title := strings.TrimSpace(m[2]); title != "" { + headingAnchors[Anchor(title)] = true + } + } + } + claimKey := func(key string) (string, bool) { + anchor := KeyAnchor(key) + if owner, taken := claimedKeys[anchor]; taken { + if owner != key { + note("two different keys claim the anchor " + anchor + + " (" + owner + " and " + key + ")") + } + return "", false + } + if headingAnchors[anchor] { + note("a key row and a heading both claim the anchor " + anchor) + return "", false + } + claimedKeys[anchor] = key + r.Keys = append(r.Keys, Heading{Text: key, Anchor: anchor}) + return anchor, true + } + lines := strings.Split(markdown, "\n") inCode, inList, inQuote := false, false, false listTag := "" @@ -196,7 +290,7 @@ func Render(source, markdown string) Rendered { if strings.HasPrefix(trimmed, "|") && i+1 < len(lines) && isTableSeparator(lines[i+1]) { closeList() closeQuote() - consumed := renderTable(&out, &text, lines[i:], renderInline) + consumed := renderTable(&out, &text, lines[i:], renderInline, claimKey) i += consumed - 1 continue } @@ -366,7 +460,27 @@ func isTableSeparator(line string) bool { } // renderTable emits one table and reports how many lines it consumed. -func renderTable(out *strings.Builder, text *strings.Builder, lines []string, renderInline func(string) string) int { +// +// In a table whose first column is headed "Field", a row opening with a lone code +// span — `vpn.redialWindow` — gets an `id` on its , so a contextual help link +// can land on that key. Every other row is emitted exactly as before. claimKey +// decides the anchor and reports whether this row is the one that gets it. +// +// The header gate is what makes "a documented config key" mean something. Without +// it, any lone code span in any first cell minted a `key-` anchor: concepts/modes.md +// produced key-1006410 and key-fc007 from its private-range table, usage/cli.md +// produced key-panic and key-run from its subcommand tables — pages that document no +// config key at all, contradicting Rendered.Keys' own description. The cost was not +// only untidiness: the collision guard aborts the whole app build, so a row added to +// one of cli.md's flag tables could fail `task gui:build` complaining about a "key +// row and a heading" on a page with no keys in it. +// +// It also settles definition-versus-summary structurally. docs/usage/config.md heads +// its four defining tables "Field" and its presets and retired tables "Key", so a +// summary row can no longer claim an anchor at all — where before, the right row +// winning depended on the order the sections happened to appear in. +func renderTable(out *strings.Builder, text *strings.Builder, lines []string, + renderInline func(string) string, claimKey func(string) (string, bool)) int { header := splitRow(lines[0]) out.WriteString("
\n") for _, c := range header { @@ -376,14 +490,28 @@ func renderTable(out *strings.Builder, text *strings.Builder, lines []string, re out.WriteString("\n\n") text.WriteString("\n") + definesKeys := len(header) > 0 && + strings.EqualFold(strings.TrimSpace(stripInline(header[0])), "Field") + used := 2 // header + separator for _, line := range lines[2:] { s := strings.TrimSpace(line) if !strings.HasPrefix(s, "|") { break } - out.WriteString("") - for _, c := range splitRow(s) { + cells := splitRow(s) + anchor := "" + if definesKeys { + if key, ok := rowKey(cells); ok { + anchor, _ = claimKey(key) + } + } + if anchor != "" { + fmt.Fprintf(out, "", anchor) + } else { + out.WriteString("") + } + for _, c := range cells { out.WriteString("") text.WriteString(stripInline(c) + " ") } @@ -526,6 +654,38 @@ func stripInline(s string) string { return strings.TrimSpace(out) } +// rowKey reports the config key a table row documents, when its first cell is a +// lone code span and nothing else. The "nothing else" is deliberate: a row whose +// first cell is prose that happens to contain code (`"see `vpn.endpoints`"`) is +// not a definition of that key, and anchoring to it would send a help link to +// the wrong row — silently, which is the one failure mode this package refuses. +func rowKey(cells []string) (string, bool) { + if len(cells) == 0 { + return "", false + } + m := loneCode.FindStringSubmatch(strings.TrimSpace(cells[0])) + if m == nil { + return "", false + } + key := strings.TrimSpace(m[1]) + if key == "" { + return "", false + } + return key, true +} + +// KeyAnchor derives the fragment id a documented config key gets. +// +// Prefixed to keep key rows out of the way of most headings — but the prefix is +// not a guarantee, and it was documented as one. A heading of "Key flags", which +// docs/usage/cli.md has, slugs to `key-flags`, exactly what a row for a key named +// `flags` would claim: the two namespaces share one document and can meet. Render +// resolves that in the heading's favour and reports it, so the collision fails the +// bundle build rather than sending a help link somewhere plausible and wrong. +func KeyAnchor(key string) string { + return "key-" + Anchor(key) +} + // Anchor derives the fragment id a heading gets, matching GitHub's rule so an // anchor written in the docs (and in a Tunable's DocAnchor) resolves in the // rendered page too. diff --git a/internal/help/slugcollide_test.go b/internal/help/slugcollide_test.go new file mode 100644 index 0000000..a1104fc --- /dev/null +++ b/internal/help/slugcollide_test.go @@ -0,0 +1,48 @@ +package help + +import ( + "strings" + "testing" +) + +// Two different keys whose slugs collide is a silently wrong deep link, and has to +// be told apart from the same key appearing in two tables — which is normal and is +// refused quietly on purpose. +// +// Anchor lowercases and drops `.`, so the colliding pairs are a case-only rename +// (`vpn.armAtBoot` / `vpn.armatboot`, both `key-vpnarmatboot`) and one that removes +// a dot (`vpn.pauseMax` / `vpnpauseMax`). Hyphens survive, so `vpn.pause-max` is +// *not* one of them — checked, rather than assumed, when this test was written. +// +// Both have to be inside a Field-headed table to arise at all: renderTable's header +// gate means config.md's presets ("Key"), retired ("Key") and rename ("Old name") +// tables cannot claim an anchor however early they appear. So the live hazard is a +// rename or addition *within* the field reference itself. +func TestTwoKeysCollidingOnOneAnchorIsReported(t *testing.T) { + md := "## Fields\n\n| Field | Default |\n|---|---|\n" + + "| `vpn.armatboot` | old |\n| `vpn.armAtBoot` | new |\n" + r := Render("probe.md", md) + + if len(r.Unsupported) == 0 { + t.Fatal("a slug collision between two different keys was not reported") + } + if !strings.Contains(strings.Join(r.Unsupported, " "), "two different keys") { + t.Errorf("unexpected report: %v", r.Unsupported) + } +} + +// The same key in two tables stays silent: config.md does this legitimately, and +// failing the build on it would be absurd. +func TestTheSameKeyTwiceIsNotReported(t *testing.T) { + md := "## Fields\n\n| Field | Default |\n|---|---|\n| `pollInterval` | `5s` |\n\n" + + "## Presets\n\n| Field | strict |\n|---|---|\n| `pollInterval` | `1s` |\n" + r := Render("probe.md", md) + + if len(r.Unsupported) != 0 { + t.Errorf("a legitimately repeated key was reported: %v", r.Unsupported) + } + if strings.Count(r.HTML, `id="key-pollinterval"`) != 1 { + t.Errorf("expected exactly one anchored row, got %d", + strings.Count(r.HTML, `id="key-pollinterval"`)) + } +} diff --git a/internal/runner/runner.go b/internal/runner/runner.go index ed17604..1a41ae4 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -3016,6 +3016,24 @@ func (o Options) runGuard(ctx context.Context) error { snapshot() } case <-geoTick.C: + // The daemon is going down: Cleanup is deferred and about to remove + // every rule, so a reading taken now can change nothing. Returning is + // exactly what the ctx.Done() case above does — this only stops the + // answer depending on which of two ready cases select happened to pick, + // since it chooses uniformly among them and the ticker is ready whenever + // the previous tick's work outran the interval. + // + // It matters because a tick taken while blocked is not a passive read. + // `probe`'s fallback path — no provider addresses resolved, so the + // tunnel-scoped pass cannot be built — LIFTS the guard, looks, and + // re-cuts. On the way out of FULL BLOCK that is a shutdown that briefly + // opens egress through the forbidden-country exit, to observe a country + // nobody is left to act on. Same reasoning as the manualBlock and + // panic-disarm branches below: when recovery cannot act on what it + // learns, it must not pay a lift to learn it. + if ctx.Err() != nil { + return nil + } // Any state that suspends the geo state machine also ends an // accelerated episode: probing faster is pointless when no reading // will be taken, and this is the one place every such state is diff --git a/internal/runner/runner_test.go b/internal/runner/runner_test.go index 353e3d7..d154067 100644 --- a/internal/runner/runner_test.go +++ b/internal/runner/runner_test.go @@ -330,17 +330,138 @@ func TestVPNHoldsFullBlockOnProbeError(t *testing.T) { } // startup guard; IR → full block; each blocked tick probes (lift+re-cut) // but an error never restores guard. - want := []string{ - "apply-guard", // startup guard - "apply-fullblock", // IR → FULL BLOCK - "apply-guard", // probe 1 lift - "apply-fullblock", // probe 1 re-cut (error → hold block) - "apply-guard", // probe 2 lift - "apply-fullblock", // probe 2 re-cut (error → hold block) - "cleanup", + // + // The SHAPE, not an exact tick count. Asserting the literal seven-call + // sequence made this test depend on a race it does not test: fakeMonitor.Once + // cancels while handing out the last result, and the loop then has both + // ctx.Done() and a 1ms geo ticker ready at its next select — which picks + // uniformly among ready cases, so one more probe cycle is a coin flip whenever + // processing the previous one took longer than the interval. That is + // vanishingly rare on a fast machine and ordinary under -race on a loaded CI + // runner, where it failed with one extra "apply-guard apply-fullblock" pair. + // + // What the test is actually for survives intact and is now stated directly: a + // probe lift is ALWAYS followed by a re-cut, so a probe error never leaves the + // guard standing in place of a full block. An extra probe cycle satisfies that + // as fully as the expected number does; a lifted block does not, at any count. + assertProbeNeverLiftsTheBlock(t, be.calls) +} + +// assertProbeNeverLiftsTheBlock pins the call sequence of a daemon that entered +// FULL BLOCK and stayed there: the startup guard, the escalation, then some whole +// number of lift-and-probe pairs, then cleanup. Nothing may end on a lift. +// +// Timing-independent by construction — see the caller for why an exact length +// cannot be. The lower bound is what keeps it from passing vacuously if probing +// stops happening at all. +func assertProbeNeverLiftsTheBlock(t *testing.T, calls []string) { + t.Helper() + if n := len(calls); n < 7 { + t.Fatalf("calls = %v: want the startup guard, the escalation, at least two "+ + "probe pairs and cleanup", calls) + } + if last := calls[len(calls)-1]; last != "cleanup" { + t.Fatalf("calls = %v: last call is %q, want cleanup", calls, last) + } + body := calls[:len(calls)-1] + if len(body)%2 != 0 { + t.Fatalf("calls = %v: %d calls before cleanup is odd, so a lift went "+ + "un-recut — a probe error must not lift FULL BLOCK", calls, len(body)) + } + for i, call := range body { + want := "apply-fullblock" + if i%2 == 0 { + want = "apply-guard" + } + if call != want { + t.Fatalf("calls = %v: call %d is %q, want %q (a probe error must not "+ + "lift FULL BLOCK)", calls, i, call, want) + } } - if !equal(be.calls, want) { - t.Fatalf("calls = %v, want %v (a probe error must not lift FULL BLOCK)", be.calls, want) +} + +// slowLastMonitor cancels while handing out its last result and then stays inside +// Once past the geo interval, so the ticker is GUARANTEED ready when the loop +// returns to its select. That is the state the run loop's ctx.Err() check exists +// for, and the only way to reach it on purpose: with both cases ready, select +// picks uniformly, so the scenario is otherwise a coin flip that a fast machine +// almost never loses. +type slowLastMonitor struct { + results []monitor.Result + idx int + cancel context.CancelFunc + stall time.Duration +} + +func (m *slowLastMonitor) Poll(ctx context.Context) <-chan monitor.Result { + ch := make(chan monitor.Result) + close(ch) + return ch +} + +func (m *slowLastMonitor) Once(context.Context) (monitor.Reading, error) { + if m.idx >= len(m.results) { + if m.cancel != nil { + m.cancel() + } + return monitor.Reading{}, context.Canceled + } + r := m.results[m.idx] + m.idx++ + if m.idx >= len(m.results) && m.cancel != nil { + m.cancel() + // Outlast the interval, so the tick that must NOT be taken is pending + // rather than merely possible. + time.Sleep(m.stall) + } + return r.Reading, r.Err +} + +// TestShutdownTakesNoProbeTick pins that a cancelled context ends the loop instead +// of buying one more reading. +// +// A geo tick taken while blocked is not a passive read: with no provider addresses +// resolved — the fixture here, as in every runner test — `probe` lifts the guard, +// looks, and re-cuts. Taking one on the way down means a shutdown that briefly +// opens egress through the forbidden-country exit to observe a country nobody is +// left to act on. So the calls must end at the last real probe's re-cut, with +// nothing between it and cleanup. +// +// Repeated, deliberately. With the check in place this is exact every time; remove +// it and each run is an independent coin flip, so the repetition turns "usually +// caught" into "caught". The failure it guards is otherwise invisible on a +// developer machine and shows up as a flake in CI, which is where it was found. +func TestShutdownTakesNoProbeTick(t *testing.T) { + for i := range 12 { + be := &fakeBackend{} + ctx, cancel := context.WithCancel(context.Background()) + o := Options{ + Monitor: &slowLastMonitor{cancel: cancel, stall: 20 * time.Millisecond, results: []monitor.Result{ + reading("IR"), // enter FULL BLOCK + failResult(), // probe error → hold block + }}, + Decider: decision.New([]string{"IR"}, 1), + Backend: be, + Log: discardLog(), + Interval: time.Millisecond, + Tunnels: []string{"utun4"}, + Endpoints: []netip.Addr{netip.MustParseAddr("203.0.113.7")}, + } + if err := Run(ctx, o); err != nil { + t.Fatal(err) + } + want := []string{ + "apply-guard", // startup guard + "apply-fullblock", // IR → FULL BLOCK + "apply-guard", // probe lift + "apply-fullblock", // probe re-cut (error → hold block) + "cleanup", + } + if !equal(be.calls, want) { + t.Fatalf("run %d: calls = %v, want %v (shutdown must not buy one more "+ + "lift-and-probe, which opens egress through the blocked exit)", + i, be.calls, want) + } } }
" + renderInline(c) + "