From 3e2fa8305b16da8ffc85a4477c89edd586f806d9 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Fri, 21 Aug 2026 11:59:53 +0330 Subject: [PATCH 1/6] feat(setup): two-step wizard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Countries, then one "Use automatic VPN detection?" tickbox with the manual fields — tunnel interfaces, self-hosted config files, endpoints — hanging off it. The opening "Configure your VPN now?" question is gone, and both wizards read the same question set, so the CLI changes with the app. Dropping configureVPN removes the thing that stood between a re-run and a working config, so two guards replace it. autoMode's default is seeded from the config: a config with pinned vpn.tunnelInterfaces starts on manual, because Apply clears TunnelInterfaces under AutoMode on purpose and a wizard defaulting to automatic would silently unpin them. Input.Endpoints becomes *[]string, nil when the question was never shown. On macOS the endpoint question is gated behind "not automatic", so a re-run that leaves detection on reaches Apply with no answer — writing that as an empty list would delete the user's server. Same convention, and the same reasoning, as the existing AutoDiscover *bool. The Swift side already had this property via shouldAsk. Off macOS the question is ungated instead: there is no live discovery, so an endpoint is required whichever mode is chosen. The CLI now asks a group in waves. A huh form binds every field before any is answered, so a question gated on another question in the same group would be decided by that question's seeded default. Rather than split the shared question set to suit one renderer, the terminal asks the ungated questions, re-evaluates, and asks whatever that opened up — the app shows step 2 as one screen because it re-evaluates gates as answers change. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 13 ++ cmd/dezhban/setup.go | 61 ++++++-- docs/contribute/testing.md | 26 +++- docs/usage/cli.md | 42 +++-- .../Sources/DezhbanCore/SetupQuestions.swift | 17 ++- .../SetupQuestionsTests.swift | 118 ++++++++++----- internal/setup/answers.go | 62 +++++--- internal/setup/questions.go | 58 ++++--- internal/setup/setup_test.go | 143 ++++++++++++++---- 9 files changed, 396 insertions(+), 144 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fa6be2..5fe47dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,19 @@ current as you land changes. ### Changed +- **The setup wizard is two steps.** Blocked countries, then one "Use automatic + VPN detection?" tickbox with the manual fields — tunnel interfaces, self-hosted + config files, endpoints — revealed underneath it when you untick it. The + opening "Configure your VPN now?" question is gone; a run always writes the + VPN keys, so instead the detection answer is **seeded from your config** and a + config with pinned `vpn.tunnelInterfaces` starts on manual, meaning a re-run + clicked straight through preserves your pins. A question that is not asked + still writes no key, so leaving automatic detection on does not blank + endpoints you set by hand. Off macOS, where there is no live discovery, the + endpoint question is asked whichever mode you pick. Both wizards read the same + question set, so `dezhban setup` changes with the app — in a terminal step 2 + appears as two consecutive prompts, since a form cannot react to an answer + given inside itself. - **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. diff --git a/cmd/dezhban/setup.go b/cmd/dezhban/setup.go index 7704b97..2af3bb5 100644 --- a/cmd/dezhban/setup.go +++ b/cmd/dezhban/setup.go @@ -66,19 +66,37 @@ func cmdSetup(args []string) int { // Asked a screenful at a time, in Group order, so a gate can be evaluated // against answers already given — which is exactly what makes the VPN // branch a branch. + // + // Within a group, in waves. A huh form binds every field before any of them + // is answered, so a question gated on another question in the SAME group + // would be decided by that question's seeded default rather than by what + // the user just typed. The macOS app has no such problem — it re-evaluates + // gates as answers change and shows the whole step at once, which is what + // makes step 2 a single screen there — so rather than splitting the shared + // question set to suit one renderer, this one asks the ungated questions, + // re-evaluates, and asks whatever that opened up. for _, group := range groupsOf(qs) { - var fields []huh.Field - for _, q := range qs { - if q.Group != group || !answers.ShouldAsk(q) { - continue + asked := map[string]bool{} + for { + var fields []huh.Field + for _, q := range qs { + if q.Group != group || asked[q.ID] || !answers.ShouldAsk(q) { + continue + } + // Defer anything still waiting on an unanswered question in + // this same group; the next wave picks it up. + if q.Gated() && !asked[q.RequiresID] && gateIsInGroup(qs, q, group) { + continue + } + asked[q.ID] = true + fields = append(fields, field(q, answers)) + } + if len(fields) == 0 { + break + } + if err := runForm(huh.NewForm(huh.NewGroup(fields...))); err != nil { + return formExit(err) } - fields = append(fields, field(q, answers)) - } - if len(fields) == 0 { - continue - } - if err := runForm(huh.NewForm(huh.NewGroup(fields...))); err != nil { - return formExit(err) } } @@ -86,7 +104,7 @@ func cmdSetup(args []string) int { // reported but doesn't abort the wizard). Reading files is the caller's job, // not internal/setup's. var profiles []config.Profile - if answers.Bool("configureVPN") { + { for _, f := range setup.SplitList(answers.Text("profileFiles")) { eps, format, ierr := vpnimport.Extract(f) if ierr != nil { @@ -112,7 +130,7 @@ func cmdSetup(args []string) int { } // --- lockout guard: warn if an endpoint sits inside a tunnel subnet --- - if answers.Bool("configureVPN") { + { if warn := setup.EndpointLockoutWarning(cfg); warn != "" { var proceed bool fmt.Fprintln(os.Stderr, warn) @@ -176,9 +194,7 @@ func cmdSetup(args []string) int { } else { fmt.Println("later, enable it with: sudo dezhban install && sudo dezhban start") } - if answers.Bool("configureVPN") { - fmt.Println("to connect a brand-new VPN whose server isn't known yet: dezhban switch, then connect it.") - } + fmt.Println("to connect a brand-new VPN whose server isn't known yet: dezhban switch, then connect it.") return 0 } @@ -304,3 +320,16 @@ func isInteractive() bool { func isTerminal(f *os.File) bool { return term.IsTerminal(f.Fd()) } + +// gateIsInGroup reports whether the question q depends on lives in the same +// group — the case the wave loop above has to defer, because a huh form cannot +// react to an answer given inside itself. A gate pointing at an EARLIER group is +// already decided by the time this group runs and needs no deferral. +func gateIsInGroup(qs []setup.Question, q setup.Question, group int) bool { + for _, other := range qs { + if other.ID == q.RequiresID { + return other.Group == group + } + } + return false +} diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index eeb7494..a9335ff 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -724,18 +724,38 @@ macOS only, privileged (`dezhban upgrade download`/`apply`). See forms are bound to the same answers. - [ ] Re-running it **without** naming profile files keeps the profiles already imported (`dezhban vpn list`). +- [ ] **Two steps.** Step 1 is countries. Step 2 opens with "Use automatic VPN + detection?"; leaving it ticked ends the wizard there, unticking it asks for + tunnel interfaces, self-hosted config files, and endpoints. In a terminal + that second half arrives as a follow-up prompt, not the same screen. +- [ ] **A re-run on a pinned config keeps its pins.** With + `vpn.tunnelInterfaces` set, re-run and press Enter through everything: + automatic detection must arrive **already unticked**, and + `dezhban config show` must still list the same interfaces. This is what + replaced the old "Configure your VPN now?" escape, so it is the check that + matters most in this section. +- [ ] **A re-run under automatic detection keeps configured endpoints.** With + `vpn.endpoints` set and no pinned interfaces, re-run, leave automatic + detection ticked, finish: the endpoints are unchanged. The question was + never asked, so nothing may have been written. +- [ ] **Off macOS the endpoint question always appears.** On Linux or Windows, + leave automatic detection ticked — you must still be asked for an endpoint, + because there is no live discovery to find one. - [ ] `dezhban setup --questions --json` runs with no TTY, no root, and no config file present, and lists the same questions the wizard asks. ### First-run wizard (macOS app) -- [ ] With no VPN configured and `defaults delete com.dezhban.menu +- [ ] With no VPN configured and `defaults delete com.behnam-rk.dezhban.app dezhban.firstRunCompleted`, launching the app opens the window **and** the wizard. With a VPN already configured from the CLI, it does not — the questions were already answered. - [ ] The questions, their order, and the gating match `dezhban setup` run in a - terminal on the same host. Declining "Configure your VPN now?" skips the - whole VPN branch in both. + terminal on the same host. Unticking "Use automatic VPN detection?" reveals + the same three manual fields in both — in the app they appear **on the same + screen**, without paging forward. +- [ ] **Two steps, labelled as two.** The step counter reads "Step 1 of 2" and + "Step 2 of 2"; unticking automatic detection must not add a third. - [ ] Saving writes through one `config set` (one password prompt, or none with a token enrolled) and the values land in `dezhban config show`. Choosing automatic detection leaves `vpn.tunnelInterfaces` **empty**. diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 4212df9..0b60dec 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -331,15 +331,39 @@ succeeds and says so; the new values are read the next time it starts. validation, and ruleset preview as `detect-vpn`/`validate`/`print-rules`. Writes to the system path need root (hence `sudo`); a permission error prints a `sudo` hint. -The wizard asks only what has no safe default: blocked countries (plus a -free-text field for other codes), whether to configure the VPN now, automatic -vs. manual detection, and — when configuring — tunnel interfaces (manual mode -only), self-hosted config files to import, and endpoints. Everything it used -to also ask (poll interval, log level, provider quorum, physical DNS, -auto-discovery) ships with a sane default and lives in the app's Settings or -`config set`; a wizard run leaves those keys untouched, so re-running setup -never clobbers a tuned value. The one silent defaulting decision it kept: a -brand-new macOS config gets live endpoint discovery turned on. +The wizard is **two steps**, and asks only what has no safe default: + +1. **Blocked countries** — a checklist of the common ones, plus a free-text + field for any other ISO codes. +2. **Automatic VPN detection?** — on by default. Leave it on and dezhban finds + the tunnel and, on macOS, learns the server address itself. Untick it and the + manual fields appear: tunnel interfaces, self-hosted config files to import, + and endpoints. + +Everything it used to also ask (poll interval, log level, provider quorum, +physical DNS, auto-discovery) ships with a sane default and lives in the app's +Settings or `config set`; a wizard run leaves those keys untouched, so +re-running setup never clobbers a tuned value. **A question that is not asked +writes no key** — so leaving automatic detection on does not blank the endpoints +of someone who set them by hand. The exception is off macOS, where there is no +live discovery: the endpoint question is asked whichever detection mode you +pick, because without it the config cannot enforce. + +Two consequences of there being no "configure your VPN now?" question, which +this wizard used to open with. A run always writes the VPN keys, so the +automatic-detection answer is **seeded from your config**: a config with pinned +`vpn.tunnelInterfaces` starts on manual, and pressing Enter through the wizard +preserves the pins rather than clearing them. And choosing automatic detection +deliberately clears those pins, because a leftover pin is precisely what stops +autodetection from happening. + +The one silent defaulting decision it kept: a brand-new macOS config gets live +endpoint discovery turned on. + +In a terminal, step 2 is shown as two consecutive prompts rather than one +screen — the form library binds every field before any is answered, so a +question that appears only when you untick another has to come after it. The +macOS app re-evaluates as you type and shows step 2 as a single screen. `setup --questions` is the exception: it prints what the wizard *would* ask — each question, what it writes, its seeded answer, and which earlier answer diff --git a/gui/macos/Sources/DezhbanCore/SetupQuestions.swift b/gui/macos/Sources/DezhbanCore/SetupQuestions.swift index e183bd4..d578029 100644 --- a/gui/macos/Sources/DezhbanCore/SetupQuestions.swift +++ b/gui/macos/Sources/DezhbanCore/SetupQuestions.swift @@ -141,14 +141,20 @@ public struct SetupAnswers { /// The `key=value` pairs one batched `config set` should write. /// - /// Three rules are not derivable from a question's `key` alone, and they + /// Two rules are not derivable from a question's `key` alone, and they /// mirror Go's `setup.Apply` exactly: /// - the free-text country codes fold into `blockedCountries`; - /// - answering "no" to the VPN branch writes none of its keys, so a VPN - /// somebody already configured is left alone; /// - choosing automatic detection CLEARS pinned interfaces rather than /// skipping the key, because a leftover pin is what makes autodetect not /// happen. + /// + /// The third rule is `shouldAsk` itself, and it is load-bearing now that + /// there is no "configure your VPN now?" question to skip the branch: a key + /// whose question was never shown must not be written. On macOS the + /// endpoint question is gated behind "not automatic", so a re-run that + /// leaves automatic detection on produces no `vpn.endpoints=` pair at all — + /// which is what keeps it from blanking a configured server. Go's `Apply` + /// achieves the same with a nil `Input.Endpoints`. public func configPairs(for questions: [SetupQuestion]) -> [String] { var pairs: [String] = [] for q in questions where shouldAsk(q) && !q.key.isEmpty { @@ -162,14 +168,15 @@ public struct SetupAnswers { pairs.append("\(q.key)=\(self[q.id])") } } - if bool("configureVPN") && bool("autoMode") { + if bool("autoMode") { pairs.append("vpn.tunnelInterfaces=") } return pairs } /// The VPN config files to import, which are not a config key at all — they - /// become profiles through `dezhban vpn import`. + /// become profiles through `dezhban vpn import`. Empty under automatic + /// detection, where the question is not asked. public var profileFiles: [String] { list("profileFiles") } } diff --git a/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift b/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift index 99f1ada..2633d7b 100644 --- a/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift @@ -19,16 +19,15 @@ struct SetupQuestionsTests { "options":[{"label":"Iran (IR)","value":"IR"},{"label":"Russia (RU)","value":"RU"}], "selected":["IR"],"group":1}, {"id":"otherCountries","kind":"list","title":"Other country codes","default":"AQ","group":1}, - {"id":"configureVPN","kind":"bool","title":"Configure your VPN now?","default":"true","group":1}, {"id":"autoMode","kind":"bool","title":"Use automatic VPN detection? (recommended)", - "default":"true","group":2,"requiresId":"configureVPN","requiresValue":"true"}, + "default":"true","group":2}, {"id":"tunnels","key":"vpn.tunnelInterfaces","kind":"multiselect","title":"Tunnel interface(s)", "options":[{"label":"utun4","value":"utun4"},{"label":"utun7","value":"utun7"}], - "selected":["utun4"],"group":3,"requiresId":"autoMode","requiresValue":"false"}, - {"id":"profileFiles","kind":"list","title":"Self-hosted VPN config files","group":4, - "requiresId":"configureVPN","requiresValue":"true"}, + "selected":["utun4"],"group":2,"requiresId":"autoMode","requiresValue":"false"}, + {"id":"profileFiles","kind":"list","title":"Self-hosted VPN config files","group":2, + "requiresId":"autoMode","requiresValue":"false"}, {"id":"endpoints","key":"vpn.endpoints","kind":"list","title":"VPN endpoint(s)", - "default":"203.0.113.7","group":4,"requiresId":"configureVPN","requiresValue":"true"} + "default":"203.0.113.7","group":2,"requiresId":"autoMode","requiresValue":"false"} ] """ @@ -38,13 +37,16 @@ struct SetupQuestionsTests { @Test func decodesTheDaemonsQuestions() throws { let qs = try Self.questions() - #expect(qs.count == 8) + #expect(qs.count == 7) let countries = try #require(qs.first { $0.id == "blockedCountries" }) #expect(countries.selected == ["IR"]) #expect(countries.options.map(\.value) == ["IR", "RU"]) // Absent `key` decodes as "no config key", not as a decode failure. #expect(try #require(qs.first { $0.id == "otherCountries" }).key.isEmpty) - #expect(try #require(qs.first { $0.id == "autoMode" }).isGated) + // autoMode is the gate now, not a gated question — everything manual + // hangs off it, and it hangs off nothing. + #expect(!(try #require(qs.first { $0.id == "autoMode" }).isGated)) + #expect(try #require(qs.first { $0.id == "endpoints" }).isGated) #expect(!(try #require(qs.first { $0.id == "pollInterval" }).isGated)) } @@ -52,23 +54,46 @@ struct SetupQuestionsTests { let a = SetupAnswers(questions: try Self.questions()) #expect(a["pollInterval"] == "15s") #expect(a.list("blockedCountries") == ["IR"]) - #expect(a.bool("configureVPN")) + #expect(a.bool("autoMode")) + } + + /// Two steps, matching Go's TestTheWizardIsTwoGroups. The app renders one + /// group per step, so a third group is a third screen. + @Test func theWizardIsTwoSteps() throws { + #expect(Set(try Self.questions().map(\.group)) == [1, 2]) } + /// Step 2 is one automatic-detection tickbox with every manual field hanging + /// off it — the reveal-in-place the app renders, and the same gate the CLI + /// evaluates. @Test func gatingMatchesTheCLI() throws { let qs = try Self.questions() var a = SetupAnswers(questions: qs) - a["configureVPN"] = "false" - for q in qs where q.requiresID == "configureVPN" { - #expect(!a.shouldAsk(q), "\(q.id) should be hidden when the VPN branch is declined") + a["autoMode"] = "true" + for q in qs where q.requiresID == "autoMode" { + #expect(!a.shouldAsk(q), "\(q.id) should be hidden under automatic detection") } - a["configureVPN"] = "true" - a["autoMode"] = "true" - #expect(!a.shouldAsk(try #require(qs.first { $0.id == "tunnels" }))) a["autoMode"] = "false" - #expect(a.shouldAsk(try #require(qs.first { $0.id == "tunnels" }))) + for id in ["tunnels", "profileFiles", "endpoints"] { + #expect(a.shouldAsk(try #require(qs.first { $0.id == id })), + "\(id) should be asked once automatic detection is unticked") + } + } + + /// The rule that replaced "configure your VPN now?": a question that was + /// never shown writes no key. Under automatic detection on macOS the + /// endpoint question is hidden, so a re-run must produce no + /// `vpn.endpoints=` pair — writing an empty one would delete a configured + /// server. Mirrors Go's TestAnUnaskedEndpointListTouchesNoEndpoint. + @Test func anUnaskedQuestionWritesNoKey() throws { + let qs = try Self.questions() + var a = SetupAnswers(questions: qs) + a["autoMode"] = "true" + + let pairs = a.configPairs(for: qs) + #expect(!pairs.contains { $0.hasPrefix("vpn.endpoints=") }) } /// The free-text codes fold into the same key as the checkboxes — they are @@ -91,7 +116,6 @@ struct SetupQuestionsTests { @Test func automaticDetectionClearsPinnedInterfaces() throws { let qs = try Self.questions() var a = SetupAnswers(questions: qs) - a["configureVPN"] = "true" a["autoMode"] = "true" let pairs = a.configPairs(for: qs) @@ -101,7 +125,6 @@ struct SetupQuestionsTests { @Test func pinningWritesTheChosenInterfaces() throws { let qs = try Self.questions() var a = SetupAnswers(questions: qs) - a["configureVPN"] = "true" a["autoMode"] = "false" a["tunnels"] = "utun4,utun7" @@ -111,17 +134,26 @@ struct SetupQuestionsTests { "a pinned config must not also be cleared") } - /// Declining the VPN branch writes none of its keys, so a VPN somebody - /// already configured is left alone — the same rule as Go's setup.Apply. - @Test func decliningTheVPNBranchWritesNoVPNKey() throws { - let qs = try Self.questions() - var a = SetupAnswers(questions: qs) - a["configureVPN"] = "false" + /// A wizard seeded with `autoMode: false` — which the daemon does whenever + /// vpn.tunnelInterfaces is pinned — and clicked straight through must write + /// those pins back, not clear them. This is the consumer side of Go's + /// TestAutoModeSeedsFalseWhenInterfacesArePinned: the app renders whatever + /// default arrives, so the guard only holds if seeding drives it. + @Test func aSeededManualModeReWritesItsPins() throws { + let qs = try Self.questions().map { q -> SetupQuestion in + guard q.id == "autoMode" else { return q } + return SetupQuestion(questionID: q.questionID, key: q.key, kind: q.kind, + title: q.title, description: q.description, + options: q.options, defaultValue: "false", + selected: q.selected, group: q.group, + requiresID: q.requiresID, requiresValue: q.requiresValue) + } + let a = SetupAnswers(questions: qs) + #expect(!a.bool("autoMode"), "the seeded default must drive the answer") let pairs = a.configPairs(for: qs) - #expect(!pairs.contains { $0.hasPrefix("vpn.") }) - // The answers that were given still apply. - #expect(pairs.contains { $0.hasPrefix("pollInterval=") }) + #expect(pairs.contains("vpn.tunnelInterfaces=utun4")) + #expect(!pairs.contains("vpn.tunnelInterfaces=")) } /// Profile files are not a config key: they become profiles through @@ -147,8 +179,7 @@ struct SetupQuestionsTests { // MARK: - the shrunk wizard /// The daemon's question list after the 2026-08 shrink: blocked countries, - /// configure-VPN?, auto-vs-manual, and the gated VPN details — nothing - /// else. Everything above must keep working with this list, because the + /// auto-vs-manual and the gated VPN details — nothing else. Everything above must keep working with this list, because the /// view renders whatever arrives, and the id-keyed special cases /// (blockedCountries+otherCountries fold, autoMode's tunnel clearing) must /// hold with the surrounding questions gone. @@ -158,16 +189,15 @@ struct SetupQuestionsTests { "options":[{"label":"Iran (IR)","value":"IR"},{"label":"Russia (RU)","value":"RU"}], "selected":["IR"],"group":1}, {"id":"otherCountries","kind":"list","title":"Other country codes","default":"AQ","group":1}, - {"id":"configureVPN","kind":"bool","title":"Configure your VPN now?","default":"true","group":1}, {"id":"autoMode","kind":"bool","title":"Use automatic VPN detection? (recommended)", - "default":"true","group":2,"requiresId":"configureVPN","requiresValue":"true"}, + "default":"true","group":2}, {"id":"tunnels","key":"vpn.tunnelInterfaces","kind":"multiselect","title":"Tunnel interface(s)", - "options":[{"label":"utun4","value":"utun4"}],"selected":[],"group":3, + "options":[{"label":"utun4","value":"utun4"}],"selected":[],"group":2, "requiresId":"autoMode","requiresValue":"false"}, - {"id":"profileFiles","kind":"list","title":"Self-hosted VPN config files","group":4, - "requiresId":"configureVPN","requiresValue":"true"}, - {"id":"endpoints","key":"vpn.endpoints","kind":"list","title":"VPN endpoint(s)","group":4, - "requiresId":"configureVPN","requiresValue":"true"} + {"id":"profileFiles","kind":"list","title":"Self-hosted VPN config files","group":2, + "requiresId":"autoMode","requiresValue":"false"}, + {"id":"endpoints","key":"vpn.endpoints","kind":"list","title":"VPN endpoint(s)","group":2, + "requiresId":"autoMode","requiresValue":"false"} ] """ @@ -177,10 +207,10 @@ struct SetupQuestionsTests { @Test func shrunkListDecodesAndGates() throws { let qs = try Self.shrunkQuestions() - #expect(qs.count == 7) + #expect(qs.count == 6) var a = SetupAnswers(questions: qs) - // Default flow: configure yes, automatic yes — the tunnel question is - // never asked. + // Default flow: automatic detection on — the tunnel question is never + // asked. let tunnels = try #require(qs.first { $0.id == "tunnels" }) #expect(!a.shouldAsk(tunnels)) a["autoMode"] = "false" @@ -205,9 +235,15 @@ struct SetupQuestionsTests { let qs = try Self.shrunkQuestions() var a = SetupAnswers(questions: qs) a["endpoints"] = "203.0.113.7" - let pairs = a.configPairs(for: qs) - // configureVPN + autoMode both default true → pinned interfaces cleared. + var pairs = a.configPairs(for: qs) + // autoMode defaults true → pinned interfaces cleared, and the endpoint + // question is not asked, so its answer is not written even though one + // was set. #expect(pairs.contains("vpn.tunnelInterfaces=")) + #expect(!pairs.contains { $0.hasPrefix("vpn.endpoints=") }) + + a["autoMode"] = "false" + pairs = a.configPairs(for: qs) #expect(pairs.contains("vpn.endpoints=203.0.113.7")) } } diff --git a/internal/setup/answers.go b/internal/setup/answers.go index bfecefd..383fc10 100644 --- a/internal/setup/answers.go +++ b/internal/setup/answers.go @@ -143,13 +143,20 @@ func (a *Answers) ShouldAsk(q Question) bool { // Input is the collected answers, in the shape the config wants them. type Input struct { - Hysteresis string - Countries []string - ConfigureVPN bool + Hysteresis string + Countries []string // AutoMode is automatic tunnel detection: no pinned interface names. - AutoMode bool - Tunnels, Endpoints []string - Profiles []config.Profile + AutoMode bool + Tunnels []string + // Endpoints is nil when the wizard never asked — on macOS the question is + // gated behind "not automatic", because live discovery learns the server + // address there. Nil rather than empty for the same reason AutoDiscover is + // a pointer: Apply must leave an unasked key ALONE, and an empty slice is + // indistinguishable from "asked, and cleared on purpose". Writing it + // unconditionally would blank the endpoints of anyone who re-ran setup and + // left automatic detection on. + Endpoints *[]string + Profiles []config.Profile // AutoDiscover is nil when nothing answered it — the wizard no longer asks; // a surface that still collects an explicit answer can set it. Nil rather // than false because Apply must leave an unanswered key ALONE: writing @@ -178,26 +185,44 @@ func (a *Answers) Input(hysteresis string, profiles []config.Profile) Input { // The no-tunnels-detected form of the question is free text. tunnels = SplitList(a.Text("tunnels")) } + var endpoints *[]string + if a.wasAsked("endpoints") { + eps := SplitList(a.Text("endpoints")) + endpoints = &eps + } return Input{ - Hysteresis: hysteresis, - Countries: countries, - ConfigureVPN: a.Bool("configureVPN"), - AutoMode: a.Bool("autoMode"), - Tunnels: tunnels, - Endpoints: SplitList(a.Text("endpoints")), - Profiles: profiles, + Hysteresis: hysteresis, + Countries: countries, + AutoMode: a.Bool("autoMode"), + Tunnels: tunnels, + Endpoints: endpoints, + Profiles: profiles, // Nil unless a surface asked the (no-longer-offered) question anyway; // nil leaves the configured value untouched in Apply. AutoDiscover: a.OptionalBool("autoDiscover"), } } +// wasAsked reports whether the question with this id exists and its gate was +// satisfied by the answers collected — i.e. whether the user actually saw it. +// The question set is retained by NewAnswers precisely so this does not have to +// be re-derived by every caller. +func (a *Answers) wasAsked(id string) bool { + for _, q := range a.asked { + if q.ID == id { + return a.ShouldAsk(q) + } + } + return false +} + // Apply writes collected answers onto cfg. Validation happens after, by the // caller: this only assembles. // // A question the user never reached leaves its part of the config alone. That -// is why the VPN keys are written only when ConfigureVPN is true — answering -// "no" must not blank out a tunnel someone configured earlier. +// is why Endpoints is a pointer: on macOS the question is gated behind "not +// automatic", and an unasked endpoint list must not blank out a server someone +// configured earlier. // Keys the wizard no longer asks about (pollInterval, logLevel, // providerQuorum, vpn.allowPhysicalDNS) are deliberately not assigned at all: // unasked means untouched, so re-running setup can never clobber a value tuned @@ -208,9 +233,6 @@ func Apply(cfg *config.Config, in Input) { } cfg.BlockedCountries = in.Countries // config.Normalize upper-cases and de-dupes on save - if !in.ConfigureVPN { - return - } if in.AutoMode { // Automatic detection: no pinned interface names (Normalize implies // autodetect), plus live discovery where supported. @@ -218,7 +240,9 @@ func Apply(cfg *config.Config, in Input) { } else { cfg.VPN.TunnelInterfaces = in.Tunnels } - cfg.VPN.Endpoints = in.Endpoints + if in.Endpoints != nil { + cfg.VPN.Endpoints = *in.Endpoints + } cfg.VPN.Profiles = mergeProfiles(cfg.VPN.Profiles, in.Profiles) switch { case in.AutoDiscover != nil: diff --git a/internal/setup/questions.go b/internal/setup/questions.go index a3a9ad7..f7987fe 100644 --- a/internal/setup/questions.go +++ b/internal/setup/questions.go @@ -145,12 +145,38 @@ func Questions(opts Options) []Question { endpointDesc = "Server IP(s)/hostname(s), comma-separated. Required on this platform (no live discovery)." } + // Automatic detection is the recommendation, but never at the cost of + // silently unpinning interfaces someone chose on purpose: a config with + // pinned vpn.tunnelInterfaces seeds this to false, so clicking straight + // through a re-run preserves them. This is load-bearing now that there is + // no "configure your VPN?" question to skip the whole branch — + // TestAutoModeSeedsFalseWhenInterfacesArePinned pins it. + autoModeDefault := "true" + if len(cfg.VPN.TunnelInterfaces) > 0 { + autoModeDefault = "false" + } + + // Endpoints are gated behind "not automatic" on macOS, where live discovery + // learns the server address. Everywhere else there is no discovery, so the + // endpoint is required whichever detection mode is chosen and the question + // is ungated — a Linux host that picked automatic detection and was never + // asked for a server would end up with a config that cannot enforce. + endpointsRequires, endpointsRequiresValue := "autoMode", "false" + if !macOS { + endpointsRequires, endpointsRequiresValue = "", "" + } + // The wizard asks only what has no safe default: what to block, and how to // find the VPN. Everything it used to also ask (poll interval, log level, // provider quorum, physical DNS, auto-discovery) ships with a sane default, // lives in Settings/`config set`, and — critically — is left UNTOUCHED by a // wizard run, so re-running setup never clobbers a tuned value // (TestAnUnaskedQuestionLeavesItsKeyAlone pins this). + // + // Two groups, which is two steps: what to block, then how to find the VPN. + // Everything in group 2 hangs off the one automatic-detection question, so + // unticking it reveals the manual fields in place rather than paging to + // another screen. return []Question{ { ID: "blockedCountries", Key: "blockedCountries", Kind: KindMultiSelect, Group: 1, @@ -165,37 +191,29 @@ func Questions(opts Options) []Question { Description: "Comma-separated ISO codes not listed above (optional).", Default: strings.Join(extra, ","), }, - { - ID: "configureVPN", Kind: KindBool, Group: 1, - Title: "Configure your VPN now?", - Description: "dezhban only enforces once it knows your VPN's tunnel and server. " + - "Say no and it starts in standby — fully open, nothing blocked — until you " + - "run 'dezhban setup' again or edit the config.", - Default: "true", - }, { ID: "autoMode", Kind: KindBool, Group: 2, Title: "Use automatic VPN detection? (recommended)", Description: "dezhban finds your tunnel and, on macOS, learns the server address " + - "itself — works with any VPN and survives redials.", - Default: "true", - RequiresID: "configureVPN", - RequiresValue: "true", + "itself — works with any VPN and survives redials. Untick it to name your " + + "tunnel and server yourself.", + Default: autoModeDefault, }, tunnelQuestion(opts.DetectedTunnels, cfg.VPN.TunnelInterfaces), { - ID: "profileFiles", Kind: KindList, Group: 4, + ID: "profileFiles", Kind: KindList, Group: 2, Title: "Self-hosted VPN config files", Description: "Comma-separated paths to WireGuard/.conf, OpenVPN/.ovpn, or V2Ray " + "JSON to import as profiles (optional).", - RequiresID: "configureVPN", RequiresValue: "true", + RequiresID: "autoMode", RequiresValue: "false", }, { - ID: "endpoints", Key: "vpn.endpoints", Kind: KindList, Group: 4, - Title: "VPN endpoint(s)", - Description: endpointDesc, - Default: strings.Join(cfg.VPN.Endpoints, ","), - RequiresID: "configureVPN", RequiresValue: "true", + ID: "endpoints", Key: "vpn.endpoints", Kind: KindList, Group: 2, + Title: "VPN endpoint(s)", + Description: endpointDesc, + Default: strings.Join(cfg.VPN.Endpoints, ","), + RequiresID: endpointsRequires, + RequiresValue: endpointsRequiresValue, }, } } @@ -204,7 +222,7 @@ func Questions(opts Options) []Question { // none were — the same split the CLI's tunnelSelector used to make on its own. func tunnelQuestion(detected, configured []string) Question { q := Question{ - ID: "tunnels", Key: "vpn.tunnelInterfaces", Group: 3, + ID: "tunnels", Key: "vpn.tunnelInterfaces", Group: 2, Title: "Tunnel interface(s)", RequiresID: "autoMode", RequiresValue: "false", } diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go index d2f1f4b..c225adf 100644 --- a/internal/setup/setup_test.go +++ b/internal/setup/setup_test.go @@ -15,9 +15,9 @@ func TestApplyAutoMode(t *testing.T) { cfg := config.Default() Apply(&cfg, Input{ Hysteresis: "3", - ConfigureVPN: true, AutoMode: true, + AutoMode: true, Tunnels: []string{"utun9"}, // must be ignored in auto mode - Endpoints: []string{"vpn.example.com"}, + Endpoints: eps("vpn.example.com"), Profiles: []config.Profile{{Name: "home", Endpoints: []string{"203.0.113.7"}}}, AutoDiscover: boolPtr(true), }) @@ -93,26 +93,33 @@ func TestAnUnaskedQuestionLeavesItsKeyAlone(t *testing.T) { func TestApplyAdvancedPin(t *testing.T) { cfg := config.Default() Apply(&cfg, Input{ - Hysteresis: "3", - ConfigureVPN: true, AutoMode: false, - Tunnels: []string{"utun4"}, - Endpoints: []string{"203.0.113.7"}, + Hysteresis: "3", + AutoMode: false, + Tunnels: []string{"utun4"}, + Endpoints: eps("203.0.113.7"), }) if len(cfg.VPN.TunnelInterfaces) != 1 || cfg.VPN.TunnelInterfaces[0] != "utun4" { t.Errorf("advanced mode should pin utun4, got %v", cfg.VPN.TunnelInterfaces) } } -// Answering "no" to "configure your VPN now?" must leave a VPN somebody already -// set up completely alone — the wizard is also how people change their -// blocked-country list. -func TestDecliningTheVPNBranchTouchesNoVPNKey(t *testing.T) { +// An UNASKED endpoint question must leave a configured server alone. +// +// This replaced the "configure your VPN now?" question as the thing standing +// between a re-run and someone's working config. On macOS the endpoint question +// is gated behind "not automatic", so a user who re-runs setup to change their +// blocked-country list — and leaves automatic detection on, as recommended — +// reaches Apply with no endpoint answer at all. Writing that as an empty list +// would delete their server. +func TestAnUnaskedEndpointListTouchesNoEndpoint(t *testing.T) { cfg := config.Default() cfg.VPN.TunnelInterfaces = []string{"utun4"} cfg.VPN.Endpoints = []string{"203.0.113.7"} cfg.VPN.AllowPhysicalDNS = true - Apply(&cfg, Input{Countries: []string{"IR", "SY"}, ConfigureVPN: false}) + // Endpoints nil is what Input produces when the question was never shown. + Apply(&cfg, Input{Countries: []string{"IR", "SY"}, AutoMode: false, + Tunnels: []string{"utun4"}}) if !reflect.DeepEqual(cfg.VPN.TunnelInterfaces, []string{"utun4"}) { t.Errorf("tunnels changed: %v", cfg.VPN.TunnelInterfaces) @@ -139,13 +146,13 @@ func TestImportedProfilesAddToTheSavedOnes(t *testing.T) { } // A run that imported nothing keeps both. - Apply(&cfg, Input{ConfigureVPN: true, AutoMode: true}) + Apply(&cfg, Input{AutoMode: true}) if len(cfg.VPN.Profiles) != 2 { t.Fatalf("a run importing nothing must keep saved profiles, got %+v", cfg.VPN.Profiles) } // A run that re-imports one replaces that one and keeps the other. - Apply(&cfg, Input{ConfigureVPN: true, AutoMode: true, + Apply(&cfg, Input{AutoMode: true, Profiles: []config.Profile{{Name: "work", Endpoints: []string{"192.0.2.5"}}}}) if len(cfg.VPN.Profiles) != 2 { t.Fatalf("re-importing a profile must not drop the others, got %+v", cfg.VPN.Profiles) @@ -194,21 +201,21 @@ func TestQuestionsSeedFromTheConfig(t *testing.T) { func TestAutoDiscoverDefaultsOnlyForANewMacConfig(t *testing.T) { fresh := config.Default() fresh.VPN.AutoDiscoverEndpoints = false // Default() has it on; force the observable flip - Apply(&fresh, Input{ConfigureVPN: true, AutoMode: true, MacOS: true, ConfigExisted: false}) + Apply(&fresh, Input{AutoMode: true, MacOS: true, ConfigExisted: false}) if !fresh.VPN.AutoDiscoverEndpoints { t.Error("a brand-new macOS config should get discovery on") } existing := config.Default() existing.VPN.AutoDiscoverEndpoints = false - Apply(&existing, Input{ConfigureVPN: true, AutoMode: true, MacOS: true, ConfigExisted: true}) + Apply(&existing, Input{AutoMode: true, MacOS: true, ConfigExisted: true}) if existing.VPN.AutoDiscoverEndpoints { t.Error("an existing config's explicit false must be preserved") } linux := config.Default() linux.VPN.AutoDiscoverEndpoints = false - Apply(&linux, Input{ConfigureVPN: true, AutoMode: true, MacOS: false, ConfigExisted: false}) + Apply(&linux, Input{AutoMode: true, MacOS: false, ConfigExisted: false}) if linux.VPN.AutoDiscoverEndpoints { t.Error("discovery is macOS-only; a new Linux config must not have it defaulted on") } @@ -217,7 +224,7 @@ func TestAutoDiscoverDefaultsOnlyForANewMacConfig(t *testing.T) { answered := config.Default() answered.VPN.AutoDiscoverEndpoints = true off := false - Apply(&answered, Input{ConfigureVPN: true, AutoMode: true, MacOS: true, ConfigExisted: false, AutoDiscover: &off}) + Apply(&answered, Input{AutoMode: true, MacOS: true, ConfigExisted: false, AutoDiscover: &off}) if answered.VPN.AutoDiscoverEndpoints { t.Error("an explicit false answer must win over the new-config default") } @@ -248,32 +255,107 @@ func TestTunnelQuestionFollowsDetection(t *testing.T) { // --- gating --- -func TestGatingHidesTheWholeVPNBranch(t *testing.T) { +// Automatic detection is the one gate left, and everything manual hangs off it. +func TestAutomaticDetectionGatesEveryManualField(t *testing.T) { qs := Questions(Options{GOOS: "darwin"}) a := NewAnswers(qs) - a.Set("configureVPN", "false") + a.Set("autoMode", "true") for _, q := range qs { - if q.RequiresID == "configureVPN" && a.ShouldAsk(q) { - t.Errorf("%s should not be asked when the VPN branch was declined", q.ID) + if q.RequiresID == "autoMode" && a.ShouldAsk(q) { + t.Errorf("%s should not be asked under automatic detection", q.ID) } } + for _, id := range []string{"tunnels", "endpoints", "profileFiles"} { + if !gatedOnAutoMode(qs, id) { + t.Errorf("%s is not gated on autoMode; on macOS it must be", id) + } + } + + a.Set("autoMode", "false") + for _, id := range []string{"tunnels", "endpoints", "profileFiles"} { + if !asked(qs, a, id) { + t.Errorf("declining automatic detection must ask %s", id) + } + } +} - a.Set("configureVPN", "true") +// Off macOS there is no live discovery, so the endpoint is required whichever +// detection mode is chosen. Gating it would let a Linux host finish the wizard +// with a config that cannot enforce. +func TestEndpointsAreUngatedWhereThereIsNoDiscovery(t *testing.T) { + qs := Questions(Options{GOOS: "linux"}) + a := NewAnswers(qs) a.Set("autoMode", "true") + if !asked(qs, a, "endpoints") { + t.Error("endpoints must be asked under automatic detection off macOS") + } + if gatedOnAutoMode(qs, "endpoints") { + t.Error("endpoints is gated on autoMode off macOS") + } +} + +// Two steps, which is the whole shape of the wizard: what to block, then how to +// find the VPN. A third group would mean a third screen in the app. +func TestTheWizardIsTwoGroups(t *testing.T) { + for _, goos := range []string{"darwin", "linux", "windows"} { + groups := map[int]bool{} + for _, q := range Questions(Options{GOOS: goos}) { + groups[q.Group] = true + } + if len(groups) != 2 || !groups[1] || !groups[2] { + t.Errorf("%s: groups = %v, want exactly {1, 2}", goos, groups) + } + } +} + +// The guard that replaced "configure your VPN now?". Without it, a re-run on a +// config with pinned interfaces would default to automatic detection, and +// clicking straight through would silently unpin them — Apply clears +// TunnelInterfaces under AutoMode on purpose. +func TestAutoModeSeedsFalseWhenInterfacesArePinned(t *testing.T) { + pinned := config.Default() + pinned.VPN.TunnelInterfaces = []string{"utun4"} + if got := defaultOf(Questions(Options{Config: &pinned, GOOS: "darwin"}), "autoMode"); got != "false" { + t.Errorf("autoMode default with pinned interfaces = %q, want \"false\"", got) + } + + fresh := config.Default() + fresh.VPN.TunnelInterfaces = nil + if got := defaultOf(Questions(Options{Config: &fresh, GOOS: "darwin"}), "autoMode"); got != "true" { + t.Errorf("autoMode default with no pinned interfaces = %q, want \"true\"", got) + } +} + +func gatedOnAutoMode(qs []Question, id string) bool { for _, q := range qs { - if q.ID == "tunnels" && a.ShouldAsk(q) { - t.Error("automatic detection must not ask which interface to pin") + if q.ID == id { + return q.RequiresID == "autoMode" && q.RequiresValue == "false" } } - a.Set("autoMode", "false") + return false +} + +func asked(qs []Question, a *Answers, id string) bool { + for _, q := range qs { + if q.ID == id { + return a.ShouldAsk(q) + } + } + return false +} + +func defaultOf(qs []Question, id string) string { for _, q := range qs { - if q.ID == "tunnels" && !a.ShouldAsk(q) { - t.Error("declining automatic detection must ask which interface to pin") + if q.ID == id { + return q.Default } } + return "" } +func eps(v ...string) *[]string { return &v } + // Walking the wizard and pressing Enter on every question must land on the // config you started with. Anything else means a default is stated in one place // and applied differently in another — the drift Phase M exists to prevent, @@ -289,10 +371,9 @@ func TestAnsweringNothingChangesNothing(t *testing.T) { qs := Questions(Options{Config: &cfg, GOOS: "darwin", DetectedTunnels: []string{"utun4"}}) a := NewAnswers(qs) - // The one answer with no config to seed it: the VPN branch is offered, and - // its own sub-answers are seeded, so accepting them must be a no-op too. - a.Set("configureVPN", "true") - a.Set("autoMode", "false") + // Nothing is Set here on purpose. autoMode seeds itself to false from the + // pinned interfaces above, which is exactly the guard being tested: pressing + // Enter through the whole wizard must not unpin them. after := cfg // Hysteresis has no question; the wizard carries the current value through, From 3a72c7828ed0d57de45c93f0b9872a96c5116299 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 5 Sep 2026 10:03:20 +0330 Subject: [PATCH 2/6] fix(setup): a pinned tunnel stays pinned when its tunnel is down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seeding autoMode to false from pinned vpn.tunnelInterfaces closes only half of "a re-run clicked straight through preserves your pins". The other half is the pick list it lands on: tunnelQuestion built its options from DetectedTunnels alone, and detection only sees tunnels that are UP. So a re-run while the VPN is down offered a list the configured interface was not in, with nothing preselected. Pressing Enter through that answers "none of them", which Apply writes as an empty vpn.tunnelInterfaces — silently unpinning an interface someone chose deliberately, which is the exact failure the autoMode seed exists to prevent. Configured interfaces are now options in their own right, appended after the detected ones and deduplicated, and preselected as before. TestAPinnedTunnelSurvivesADetectionMiss checks both the question shape and that the whole click-through is a no-op; it fails on the unfixed code with vpn.tunnelInterfaces changing from "utun9" to "". Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 3 ++- docs/contribute/testing.md | 5 ++++- docs/usage/cli.md | 4 +++- internal/setup/questions.go | 29 ++++++++++++++++++++------ internal/setup/setup_test.go | 40 ++++++++++++++++++++++++++++++++++++ 5 files changed, 72 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fe47dd..c03996c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,8 @@ current as you land changes. opening "Configure your VPN now?" question is gone; a run always writes the VPN keys, so instead the detection answer is **seeded from your config** and a config with pinned `vpn.tunnelInterfaces` starts on manual, meaning a re-run - clicked straight through preserves your pins. A question that is not asked + clicked straight through preserves your pins — even when that tunnel is down, + which previously dropped it from the pick list and so cleared it. A question that is not asked still writes no key, so leaving automatic detection on does not blank endpoints you set by hand. Off macOS, where there is no live discovery, the endpoint question is asked whichever mode you pick. Both wizards read the same diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index a9335ff..9d6b0e7 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -733,7 +733,10 @@ macOS only, privileged (`dezhban upgrade download`/`apply`). See automatic detection must arrive **already unticked**, and `dezhban config show` must still list the same interfaces. This is what replaced the old "Configure your VPN now?" escape, so it is the check that - matters most in this section. + matters most in this section. Run it **twice: once with the VPN up and once + with it down.** Detection only sees tunnels that are up, so the down run is + the one where the pinned interface has to appear in the pick list — and stay + ticked — on its own. - [ ] **A re-run under automatic detection keeps configured endpoints.** With `vpn.endpoints` set and no pinned interfaces, re-run, leave automatic detection ticked, finish: the endpoints are unchanged. The question was diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 0b60dec..1680a73 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -353,7 +353,9 @@ Two consequences of there being no "configure your VPN now?" question, which this wizard used to open with. A run always writes the VPN keys, so the automatic-detection answer is **seeded from your config**: a config with pinned `vpn.tunnelInterfaces` starts on manual, and pressing Enter through the wizard -preserves the pins rather than clearing them. And choosing automatic detection +preserves the pins rather than clearing them — including while that tunnel is +down, since detection only sees tunnels that are up and the pick list would +otherwise not contain your own pinned interface. And choosing automatic detection deliberately clears those pins, because a leftover pin is precisely what stops autodetection from happening. diff --git a/internal/setup/questions.go b/internal/setup/questions.go index f7987fe..920064a 100644 --- a/internal/setup/questions.go +++ b/internal/setup/questions.go @@ -232,16 +232,33 @@ func tunnelQuestion(detected, configured []string) Question { q.Default = strings.Join(configured, ",") return q } + q.Kind = KindMultiSelect + q.Description = "Detected tunnels — pick the VPN's." + + // A pinned interface is an option even when it is not detected right now. + // Detection only sees tunnels that are UP, so a re-run while the VPN is + // down would otherwise offer a list its own configured interface is not in + // — and pressing Enter through that list answers "none of them", which + // Apply writes as an empty vpn.tunnelInterfaces. That silently unpins an + // interface someone chose deliberately, which is the very thing seeding + // autoMode to false from those pins exists to prevent; offering the pins + // but leaving them unselectable would close only half of it. + // TestAPinnedTunnelSurvivesADetectionMiss pins this. + seen := map[string]bool{} + for _, t := range append(append([]string(nil), detected...), configured...) { + if seen[t] { + continue + } + seen[t] = true + q.Options = append(q.Options, Option{Label: t, Value: t}) + } cfgSet := map[string]bool{} for _, t := range configured { cfgSet[t] = true } - q.Kind = KindMultiSelect - q.Description = "Detected tunnels — pick the VPN's." - for _, t := range detected { - q.Options = append(q.Options, Option{Label: t, Value: t}) - if cfgSet[t] { - q.Selected = append(q.Selected, t) + for _, o := range q.Options { + if cfgSet[o.Value] { + q.Selected = append(q.Selected, o.Value) } } return q diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go index c225adf..5644d0c 100644 --- a/internal/setup/setup_test.go +++ b/internal/setup/setup_test.go @@ -253,6 +253,46 @@ func TestTunnelQuestionFollowsDetection(t *testing.T) { } } +// Detection only sees tunnels that are UP. A re-run while the VPN is down must +// still offer — and preselect — the interface the config pins, or pressing +// Enter through the pick list answers "none of them" and unpins it. +func TestAPinnedTunnelSurvivesADetectionMiss(t *testing.T) { + cfg := config.Default() + cfg.BlockedCountries = []string{"IR"} + cfg.VPN.Endpoints = []string{"203.0.113.7"} + cfg.VPN.TunnelInterfaces = []string{"utun9"} + config.Normalize(&cfg) + before := config.KeyValues(&cfg) + + // utun9 is absent from the detected set: that tunnel is not up right now. + qs := Questions(Options{Config: &cfg, GOOS: "darwin", DetectedTunnels: []string{"utun0", "utun4"}}) + + q := byID(qs)["tunnels"] + var offered []string + for _, o := range q.Options { + offered = append(offered, o.Value) + } + if !reflect.DeepEqual(offered, []string{"utun0", "utun4", "utun9"}) { + t.Errorf("options = %v, want the detected tunnels plus the pinned one", offered) + } + if !reflect.DeepEqual(q.Selected, []string{"utun9"}) { + t.Errorf("selected = %v, want the pinned tunnel preselected", q.Selected) + } + + // And the whole click-through must be a no-op, as it is when the pin is up. + after := cfg + in := NewAnswers(qs).Input(strconv.Itoa(cfg.Hysteresis), nil) + in.MacOS = true + in.ConfigExisted = true + Apply(&after, in) + config.Normalize(&after) + for key, want := range before { + if got := config.KeyValues(&after)[key]; got != want { + t.Errorf("%s changed by answering nothing: %q -> %q", key, want, got) + } + } +} + // --- gating --- // Automatic detection is the one gate left, and everything manual hangs off it. From 241dc2ea2511373f1b2845ea761015417d3aeb86 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 5 Sep 2026 12:26:27 +0330 Subject: [PATCH 3/6] fix(setup): step 2's second wave was folded into the first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wave loop's deferral never fired. It marked a question asked inside the same selection pass that read `asked[q.RequiresID]`, so a question gated on one that appears EARLIER in the question set — the normal way to write one, and how autoMode sits relative to its three manual fields — saw its gate already marked and was never held back. It went unnoticed because the fresh-config path looks right for the wrong reason: autoMode seeds to true there, so the manual fields fail their gate outright and the loop happens to produce two waves. The bug shows on a re-run against a pinned config, where autoMode seeds to FALSE and so every manual field satisfies its gate before the user has touched anything: all four questions arrive on one huh form. Ticking "use automatic detection" on that form then retracts the gate for the endpoint field beside it, so `wasAsked` returns false and the endpoint answer the same form collected is silently dropped — the failure `Input.Endpoints` became a pointer to prevent, one layer up. It also made three docs describe a two-prompt step 2 that a pinned config never got. Selection is now a plain function over (questions, answers) that treats `asked` as read-only and leaves marking to the caller, which is both the fix and the only reason it is testable without driving a terminal. Also from the same review: - Deferring now requires the gate question to be genuinely still coming. A question gated on a same-group question that this run will never show would otherwise wait for a wave that never arrives, and be silently dropped when the loop ran out of fields. Latent at today's depth of one. - The profile-file import reads its answer only when that question was shown, matching the macOS app, whose profileFiles is empty for the same reason. - The two bare blocks left where `if answers.Bool("configureVPN")` was removed are unindented, and a doc comment naming that dead question is corrected. TestStepTwoArrivesInWaves pins wave membership for both seedings and fails on the old code with [[autoMode tunnels profileFiles endpoints]] where two waves were wanted. TestEveryGatedQuestionIsReachable pins that nothing is stranded. cmd/dezhban had no test for this loop at all. Co-Authored-By: Claude Opus 5 --- cmd/dezhban/setup.go | 115 +++++++++++++++++++++++---------- cmd/dezhban/setup_wave_test.go | 99 ++++++++++++++++++++++++++++ internal/setup/questions.go | 4 +- 3 files changed, 181 insertions(+), 37 deletions(-) create mode 100644 cmd/dezhban/setup_wave_test.go diff --git a/cmd/dezhban/setup.go b/cmd/dezhban/setup.go index 2af3bb5..0c06831 100644 --- a/cmd/dezhban/setup.go +++ b/cmd/dezhban/setup.go @@ -78,22 +78,15 @@ func cmdSetup(args []string) int { for _, group := range groupsOf(qs) { asked := map[string]bool{} for { + wave := nextWave(qs, group, asked, answers) + if len(wave) == 0 { + break + } var fields []huh.Field - for _, q := range qs { - if q.Group != group || asked[q.ID] || !answers.ShouldAsk(q) { - continue - } - // Defer anything still waiting on an unanswered question in - // this same group; the next wave picks it up. - if q.Gated() && !asked[q.RequiresID] && gateIsInGroup(qs, q, group) { - continue - } + for _, q := range wave { asked[q.ID] = true fields = append(fields, field(q, answers)) } - if len(fields) == 0 { - break - } if err := runForm(huh.NewForm(huh.NewGroup(fields...))); err != nil { return formExit(err) } @@ -103,8 +96,13 @@ func cmdSetup(args []string) int { // Import any named config files into profiles (best-effort; a bad file is // reported but doesn't abort the wizard). Reading files is the caller's job, // not internal/setup's. + // Only what the user was actually shown: the question is gated behind manual + // mode, and reading it unconditionally would import files from a field this + // run never rendered — the same "unasked means untouched" rule Apply follows + // for keys, and what keeps this in step with the macOS app, whose + // profileFiles is empty for exactly that reason. var profiles []config.Profile - { + if q, ok := questionByID(qs, "profileFiles"); ok && answers.ShouldAsk(q) { for _, f := range setup.SplitList(answers.Text("profileFiles")) { eps, format, ierr := vpnimport.Extract(f) if ierr != nil { @@ -130,20 +128,18 @@ func cmdSetup(args []string) int { } // --- lockout guard: warn if an endpoint sits inside a tunnel subnet --- - { - if warn := setup.EndpointLockoutWarning(cfg); warn != "" { - var proceed bool - fmt.Fprintln(os.Stderr, warn) - if err := runForm(huh.NewForm(huh.NewGroup( - huh.NewConfirm().Title("Save anyway?"). - Description("The flagged endpoint would very likely lock you out.").Value(&proceed), - ))); err != nil { - return formExit(err) - } - if !proceed { - fmt.Fprintln(os.Stderr, "setup cancelled — fix the endpoint (see 'dezhban doctor').") - return 1 - } + if warn := setup.EndpointLockoutWarning(cfg); warn != "" { + var proceed bool + fmt.Fprintln(os.Stderr, warn) + if err := runForm(huh.NewForm(huh.NewGroup( + huh.NewConfirm().Title("Save anyway?"). + Description("The flagged endpoint would very likely lock you out.").Value(&proceed), + ))); err != nil { + return formExit(err) + } + if !proceed { + fmt.Fprintln(os.Stderr, "setup cancelled — fix the endpoint (see 'dezhban doctor').") + return 1 } } @@ -321,15 +317,64 @@ func isTerminal(f *os.File) bool { return term.IsTerminal(f.Fd()) } -// gateIsInGroup reports whether the question q depends on lives in the same -// group — the case the wave loop above has to defer, because a huh form cannot -// react to an answer given inside itself. A gate pointing at an EARLIER group is -// already decided by the time this group runs and needs no deferral. -func gateIsInGroup(qs []setup.Question, q setup.Question, group int) bool { - for _, other := range qs { - if other.ID == q.RequiresID { - return other.Group == group +// questionByID finds a question in the set the wizard is running, so a caller +// can ask whether the user was actually shown it. +func questionByID(qs []setup.Question, id string) (setup.Question, bool) { + for _, q := range qs { + if q.ID == id { + return q, true + } + } + return setup.Question{}, false +} + +// nextWave picks the questions of this group to put on the next form: those +// whose gate is already satisfied, minus any whose gate is about to be answered +// on that very form. +// +// It takes `asked` as read-only and leaves the marking to the caller, which is +// what makes the deferral work at all. Marking inside the selection pass — as +// this did — defeats it silently whenever a gating question appears BEFORE its +// dependents in the question set, which is the normal way to write one: by the +// time the dependent is examined, the gate it is waiting on has been marked +// asked earlier in the same pass, so it is never held back. The visible symptom +// was a re-run on a pinned config, where autoMode seeds to false and so all of +// step 2 satisfied its gate up front and arrived as one form instead of two — +// and ticking automatic detection on that form then retracted the endpoint +// answer the same form had just collected. +// +// Being a plain function over (questions, answers) rather than a loop body is +// also the only reason this is testable without driving a terminal; +// TestStepTwoArrivesInWaves covers it. +func nextWave(qs []setup.Question, group int, asked map[string]bool, a *setup.Answers) []setup.Question { + var wave []setup.Question + for _, q := range qs { + if q.Group != group || asked[q.ID] || !a.ShouldAsk(q) { + continue + } + if q.Gated() && stillToAsk(qs, q.RequiresID, group, asked, a) { + continue + } + wave = append(wave, q) + } + return wave +} + +// stillToAsk reports whether the question a gate points at is in this same group +// and is genuinely still coming — the only case where deferring is right. +// +// A gate pointing at an EARLIER group is already decided by the time this group +// runs. A gate pointing at a question this run will never show — because that +// question's own gate is unmet — is fixed at its seeded default and will never +// change, so deferring for it would strand the dependent question forever: the +// wave it waits for never arrives, the loop runs out of fields and breaks, and +// the question is silently never asked. +func stillToAsk(qs []setup.Question, id string, group int, asked map[string]bool, a *setup.Answers) bool { + for _, q := range qs { + if q.ID != id { + continue } + return q.Group == group && !asked[q.ID] && a.ShouldAsk(q) } return false } diff --git a/cmd/dezhban/setup_wave_test.go b/cmd/dezhban/setup_wave_test.go new file mode 100644 index 0000000..5575ec0 --- /dev/null +++ b/cmd/dezhban/setup_wave_test.go @@ -0,0 +1,99 @@ +package main + +import ( + "reflect" + "testing" + + "github.com/behnam-rk/dezhban/internal/config" + "github.com/behnam-rk/dezhban/internal/setup" +) + +// ids of the questions a wave puts on one form. +func waveIDs(qs []setup.Question) []string { + var out []string + for _, q := range qs { + out = append(out, q.ID) + } + return out +} + +// drive runs the wave loop for one group the way cmdSetup does, without a +// terminal: each wave is recorded, then answered by `answer` before the next. +func drive(qs []setup.Question, group int, a *setup.Answers, answer func(id string)) [][]string { + var waves [][]string + asked := map[string]bool{} + for { + wave := nextWave(qs, group, asked, a) + if len(wave) == 0 { + return waves + } + waves = append(waves, waveIDs(wave)) + for _, q := range wave { + asked[q.ID] = true + answer(q.ID) + } + } +} + +// A huh form binds every field before any is answered, so a question gated on +// another question on the SAME form would be decided by that question's seeded +// default. Step 2 must therefore arrive as two waves — and crucially it must do +// so whichever way autoMode is seeded, because a pinned config seeds it to +// false and so satisfies the manual fields' gate before the user touches it. +func TestStepTwoArrivesInWaves(t *testing.T) { + for _, tc := range []struct { + name string + pinned []string + want [][]string + }{ + { + name: "fresh config, automatic seeded on", + want: [][]string{{"autoMode"}}, + }, + { + name: "pinned config, automatic seeded off", + pinned: []string{"utun9"}, + want: [][]string{{"autoMode"}, {"tunnels", "profileFiles", "endpoints"}}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := config.Default() + cfg.VPN.TunnelInterfaces = tc.pinned + qs := setup.Questions(setup.Options{Config: &cfg, GOOS: "darwin"}) + + // The user answers nothing: every question keeps its seeded value. + got := drive(qs, 2, setup.NewAnswers(qs), func(string) {}) + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("waves = %v, want %v", got, tc.want) + } + }) + } +} + +// The wave loop must never strand a question. Whatever the user answers, every +// question whose gate ends up satisfied has to have been put on some form. +func TestEveryGatedQuestionIsReachable(t *testing.T) { + cfg := config.Default() + cfg.VPN.TunnelInterfaces = []string{"utun9"} // seeds autoMode false + qs := setup.Questions(setup.Options{Config: &cfg, GOOS: "darwin"}) + a := setup.NewAnswers(qs) + + // The user unticks nothing but re-affirms manual mode when asked. + seen := map[string]bool{} + for _, w := range drive(qs, 2, a, func(id string) { + seen[id] = true + if id == "autoMode" { + a.Set("autoMode", "false") + } + }) { + _ = w + } + for _, q := range qs { + if q.Group != 2 { + continue + } + if a.ShouldAsk(q) && !seen[q.ID] { + t.Errorf("%s has a satisfied gate but was never asked", q.ID) + } + } +} diff --git a/internal/setup/questions.go b/internal/setup/questions.go index 920064a..5bf72aa 100644 --- a/internal/setup/questions.go +++ b/internal/setup/questions.go @@ -51,8 +51,8 @@ type Question struct { // JSON use it, so it is a compatibility surface: do not rename one. ID string `json:"id"` // Key is the dotted config key this answer writes, empty when the question - // only steers the flow (ConfigureVPN, AutoMode) or is folded into another - // key's value (OtherCountries). + // only steers the flow (AutoMode) or is folded into another key's value + // (OtherCountries). Key string `json:"key,omitempty"` Kind string `json:"kind"` Title string `json:"title"` From a891dd72482bdd603dc93f693c01c379ccfae5de Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 5 Sep 2026 18:46:58 +0330 Subject: [PATCH 4/6] fix(setup): the app imported profile files the user had withdrawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of the review loop, and the headline finding is an asymmetry the loop itself created: gating the Go wizard's profile-file import on "was this question shown?" left the macOS app ungated, so the two wizards disagreed for identical answers. The app reveals step 2 in place and re-evaluates gates live, so unticking automatic detection, choosing files, then ticking it again makes the field vanish while the answer it collected stays in the dictionary. `save()` then ran `dezhban vpn import` on files the user had visibly withdrawn, which the CLI no longer does. `profileFiles` is now `profileFiles(for:)` and returns nothing unless its question is being asked, matching `configPairs`, which has always skipped an unasked key. The property's doc comment claimed this behaviour already; now it is true. Also from round 2: - The tunnel pick list labels an interface it did not detect. A list headed "Detected tunnels" with a ticked interface that is plainly down reads as a broken detector; the label now says "(configured, not up right now)" and the description explains it. Values are untouched, so nothing changes in what gets written. - Two comments claimed more than the code delivers, both about gate depth. `stillToAsk` treats a gate question that will never be shown as fixed at its seed, which is only sound while gates are one deep; `wasAsked` stands in "gate satisfied by the final answers" for "the user saw it", which holds only because no gate points at a later group. Both are properties of the question set rather than of those functions, so TestGatesAreShallowAndPointBackwards pins the shape across all three platforms and both comments now say what is actually relied on. - TestEveryGatedQuestionIsReachable could not fail — its fixture answered autoMode false, which every plausible implementation gets right. It is joined by TestTickingAutomaticRetractsTheManualFields, which drives the flip and asserts the three manual questions are neither asked nor written. - TestLeavingAutomaticOnKeepsAConfiguredEndpoint drives the PR's headline guarantee through Answers instead of a hand-built Input. The existing TestAnUnaskedEndpointListTouchesNoEndpoint pinned Apply's half with a nil Endpoints but never showed the wizard producing that nil. - cli.md and the changelog said step 2 arrives as two prompts without qualification; that is only so when you untick automatic detection. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 +- cmd/dezhban/setup.go | 15 +++-- cmd/dezhban/setup_wave_test.go | 57 ++++++++++++++---- docs/usage/cli.md | 10 ++-- .../Sources/DezhbanCore/SetupQuestions.swift | 18 +++++- .../Sources/DezhbanMenu/FirstRunView.swift | 2 +- .../SetupQuestionsTests.swift | 17 +++++- internal/setup/answers.go | 15 +++-- internal/setup/questions.go | 23 ++++++- internal/setup/setup_test.go | 60 +++++++++++++++++++ 10 files changed, 189 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c03996c..baf5e6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,8 +44,9 @@ current as you land changes. still writes no key, so leaving automatic detection on does not blank endpoints you set by hand. Off macOS, where there is no live discovery, the endpoint question is asked whichever mode you pick. Both wizards read the same - question set, so `dezhban setup` changes with the app — in a terminal step 2 - appears as two consecutive prompts, since a form cannot react to an answer + question set, so `dezhban setup` changes with the app — in a terminal, + unticking automatic detection brings the manual fields as a second prompt + rather than revealing them in place, since a form cannot react to an answer given inside itself. - **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 diff --git a/cmd/dezhban/setup.go b/cmd/dezhban/setup.go index 0c06831..7d3d4ce 100644 --- a/cmd/dezhban/setup.go +++ b/cmd/dezhban/setup.go @@ -365,10 +365,17 @@ func nextWave(qs []setup.Question, group int, asked map[string]bool, a *setup.An // // A gate pointing at an EARLIER group is already decided by the time this group // runs. A gate pointing at a question this run will never show — because that -// question's own gate is unmet — is fixed at its seeded default and will never -// change, so deferring for it would strand the dependent question forever: the -// wave it waits for never arrives, the loop runs out of fields and breaks, and -// the question is silently never asked. +// question's own gate is unmet — is fixed at its seeded default, so deferring +// for it would strand the dependent question forever: the wave it waits for +// never arrives, the loop runs out of fields and breaks, and the question is +// silently never asked. +// +// That second case is only sound while gates are ONE deep. A gate question that +// is itself gated could become askable later, and releasing its dependent now +// would evaluate it against a seed — the very bug this loop was fixed for, one +// level down. Depth 1 is a property of the question set, not of this function, +// so it is pinned there by TestGatesAreShallowAndPointBackwards; make this +// predicate transitive before adding a gated gate. func stillToAsk(qs []setup.Question, id string, group int, asked map[string]bool, a *setup.Answers) bool { for _, q := range qs { if q.ID != id { diff --git a/cmd/dezhban/setup_wave_test.go b/cmd/dezhban/setup_wave_test.go index 5575ec0..aecc4c2 100644 --- a/cmd/dezhban/setup_wave_test.go +++ b/cmd/dezhban/setup_wave_test.go @@ -2,6 +2,7 @@ package main import ( "reflect" + "strconv" "testing" "github.com/behnam-rk/dezhban/internal/config" @@ -70,29 +71,61 @@ func TestStepTwoArrivesInWaves(t *testing.T) { } } -// The wave loop must never strand a question. Whatever the user answers, every -// question whose gate ends up satisfied has to have been put on some form. -func TestEveryGatedQuestionIsReachable(t *testing.T) { +// Ticking automatic detection retracts the manual half of step 2. The three +// gated questions must then never be put on a form at all — the wave loop is +// what makes that true, and it is the case the old mark-inside-the-pass loop got +// wrong by shipping all four on one form. +func TestTickingAutomaticRetractsTheManualFields(t *testing.T) { cfg := config.Default() cfg.VPN.TunnelInterfaces = []string{"utun9"} // seeds autoMode false + cfg.VPN.Endpoints = []string{"203.0.113.7"} qs := setup.Questions(setup.Options{Config: &cfg, GOOS: "darwin"}) a := setup.NewAnswers(qs) - // The user unticks nothing but re-affirms manual mode when asked. seen := map[string]bool{} - for _, w := range drive(qs, 2, a, func(id string) { + drive(qs, 2, a, func(id string) { seen[id] = true if id == "autoMode" { - a.Set("autoMode", "false") + a.Set("autoMode", "true") // the user ticks it after all } - }) { - _ = w + }) + + if !seen["autoMode"] { + t.Fatal("autoMode was never asked") } - for _, q := range qs { - if q.Group != 2 { - continue + for _, id := range []string{"tunnels", "profileFiles", "endpoints"} { + if seen[id] { + t.Errorf("%s was put on a form despite automatic detection being on", id) + } + } + + // And nothing it would have written may reach the config. + after := cfg + in := a.Input(strconv.Itoa(cfg.Hysteresis), nil) + in.MacOS, in.ConfigExisted = true, true + setup.Apply(&after, in) + config.Normalize(&after) + if got := after.VPN.Endpoints; !reflect.DeepEqual(got, []string{"203.0.113.7"}) { + t.Errorf("endpoints = %v, want the configured one untouched", got) + } +} + +// Every question whose gate ends up satisfied must have been put on some form. +func TestEveryGatedQuestionIsReachable(t *testing.T) { + cfg := config.Default() + cfg.VPN.TunnelInterfaces = []string{"utun9"} + qs := setup.Questions(setup.Options{Config: &cfg, GOOS: "darwin"}) + a := setup.NewAnswers(qs) + + seen := map[string]bool{} + drive(qs, 2, a, func(id string) { + seen[id] = true + if id == "autoMode" { + a.Set("autoMode", "false") } - if a.ShouldAsk(q) && !seen[q.ID] { + }) + for _, q := range qs { + if q.Group == 2 && a.ShouldAsk(q) && !seen[q.ID] { t.Errorf("%s has a satisfied gate but was never asked", q.ID) } } diff --git a/docs/usage/cli.md b/docs/usage/cli.md index 1680a73..7bdbebb 100644 --- a/docs/usage/cli.md +++ b/docs/usage/cli.md @@ -362,10 +362,12 @@ autodetection from happening. The one silent defaulting decision it kept: a brand-new macOS config gets live endpoint discovery turned on. -In a terminal, step 2 is shown as two consecutive prompts rather than one -screen — the form library binds every field before any is answered, so a -question that appears only when you untick another has to come after it. The -macOS app re-evaluates as you type and shows step 2 as a single screen. +In a terminal, unticking automatic detection brings the manual fields as a +second prompt rather than revealing them in place — the form library binds every +field before any is answered, so a question that appears only when you untick +another has to come after it. Leave automatic detection on and step 2 is the one +prompt. The macOS app re-evaluates as you type and shows step 2 as a single +screen either way. `setup --questions` is the exception: it prints what the wizard *would* ask — each question, what it writes, its seeded answer, and which earlier answer diff --git a/gui/macos/Sources/DezhbanCore/SetupQuestions.swift b/gui/macos/Sources/DezhbanCore/SetupQuestions.swift index d578029..5a0b029 100644 --- a/gui/macos/Sources/DezhbanCore/SetupQuestions.swift +++ b/gui/macos/Sources/DezhbanCore/SetupQuestions.swift @@ -175,9 +175,21 @@ public struct SetupAnswers { } /// The VPN config files to import, which are not a config key at all — they - /// become profiles through `dezhban vpn import`. Empty under automatic - /// detection, where the question is not asked. - public var profileFiles: [String] { list("profileFiles") } + /// become profiles through `dezhban vpn import`. + /// + /// Gated, for the same reason `configPairs` skips an unasked key: this step + /// reveals in place and re-evaluates as answers change, so someone can untick + /// automatic detection, choose files, then tick it again — the field goes + /// away but the answer it collected does not. Importing those would enact a + /// choice the user visibly withdrew, and Go's wizard does not (it reads this + /// answer only when `ShouldAsk` holds). Taking the question set rather than + /// reading the stored answer blind is what keeps the two in step. + public func profileFiles(for questions: [SetupQuestion]) -> [String] { + guard let q = questions.first(where: { $0.id == "profileFiles" }), shouldAsk(q) else { + return [] + } + return list("profileFiles") + } } /// When the first-run wizard should be offered. diff --git a/gui/macos/Sources/DezhbanMenu/FirstRunView.swift b/gui/macos/Sources/DezhbanMenu/FirstRunView.swift index 7cf9b99..e85e7ef 100644 --- a/gui/macos/Sources/DezhbanMenu/FirstRunView.swift +++ b/gui/macos/Sources/DezhbanMenu/FirstRunView.swift @@ -248,7 +248,7 @@ struct FirstRunView: View { } return } - let files = answers.profileFiles + let files = answers.profileFiles(for: questions) FirstRun.markComplete() guard !files.isEmpty else { done(true) diff --git a/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift b/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift index 2633d7b..30cefd9 100644 --- a/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift +++ b/gui/macos/Tests/DezhbanCoreTests/SetupQuestionsTests.swift @@ -163,10 +163,25 @@ struct SetupQuestionsTests { var a = SetupAnswers(questions: qs) a["profileFiles"] = "/tmp/home.conf, /tmp/work.ovpn" - #expect(a.profileFiles == ["/tmp/home.conf", "/tmp/work.ovpn"]) + a["autoMode"] = "false" + #expect(a.profileFiles(for: qs) == ["/tmp/home.conf", "/tmp/work.ovpn"]) #expect(!a.configPairs(for: qs).contains { $0.contains("profileFiles") }) } + /// The field is revealed in place and re-evaluated live, so a user can pick + /// files and then tick automatic detection again. The answer survives in the + /// dictionary; acting on it would import files the user visibly withdrew. + @Test func withdrawnProfileFilesAreNotImported() throws { + let qs = try Self.questions() + var a = SetupAnswers(questions: qs) + a["autoMode"] = "false" + a["profileFiles"] = "/tmp/home.conf" + #expect(a.profileFiles(for: qs) == ["/tmp/home.conf"]) + + a["autoMode"] = "true" // the field disappears again + #expect(a.profileFiles(for: qs).isEmpty) + } + @Test func firstRunIsOfferedOnlyWhenNothingIsKnownYet() { // Whether the flag is set is UserDefaults' business; whether the wizard // should be offered given that flag is a rule, and lives in the core. diff --git a/internal/setup/answers.go b/internal/setup/answers.go index 383fc10..831bb76 100644 --- a/internal/setup/answers.go +++ b/internal/setup/answers.go @@ -203,10 +203,17 @@ func (a *Answers) Input(hysteresis string, profiles []config.Profile) Input { } } -// wasAsked reports whether the question with this id exists and its gate was -// satisfied by the answers collected — i.e. whether the user actually saw it. -// The question set is retained by NewAnswers precisely so this does not have to -// be re-derived by every caller. +// wasAsked reports whether the question with this id exists and its gate is +// satisfied by the FINAL answers collected. The question set is retained by +// NewAnswers precisely so this does not have to be re-derived by every caller. +// +// That stands in for "the user saw it", and the two agree only because a gate +// never points forward: every gate names an ungated question in the same or an +// earlier group, so by the time a gated question is put to the user its gate is +// already answered and cannot move afterwards. A gate pointing at a LATER +// group's answer would be read here against that question's seeded default, +// and this would report a question asked that nobody was shown — writing its +// key from a seed. TestGatesAreShallowAndPointBackwards pins the shape. func (a *Answers) wasAsked(id string) bool { for _, q := range a.asked { if q.ID == id { diff --git a/internal/setup/questions.go b/internal/setup/questions.go index 5bf72aa..c83a9b0 100644 --- a/internal/setup/questions.go +++ b/internal/setup/questions.go @@ -233,7 +233,6 @@ func tunnelQuestion(detected, configured []string) Question { return q } q.Kind = KindMultiSelect - q.Description = "Detected tunnels — pick the VPN's." // A pinned interface is an option even when it is not detected right now. // Detection only sees tunnels that are UP, so a re-run while the VPN is @@ -244,13 +243,33 @@ func tunnelQuestion(detected, configured []string) Question { // autoMode to false from those pins exists to prevent; offering the pins // but leaving them unselectable would close only half of it. // TestAPinnedTunnelSurvivesADetectionMiss pins this. + // + // Those extras are labelled, because a list headed "detected" containing a + // ticked interface that is plainly not up reads as a bug in the detector. + // The label carries it; the Value stays the bare interface name, which is + // what gets written. + detectedSet := map[string]bool{} + for _, t := range detected { + detectedSet[t] = true + } seen := map[string]bool{} + offDuty := false for _, t := range append(append([]string(nil), detected...), configured...) { if seen[t] { continue } seen[t] = true - q.Options = append(q.Options, Option{Label: t, Value: t}) + label := t + if !detectedSet[t] { + label = t + " (configured, not up right now)" + offDuty = true + } + q.Options = append(q.Options, Option{Label: label, Value: t}) + } + q.Description = "Detected tunnels — pick the VPN's." + if offDuty { + q.Description = "Pick the VPN's. Interfaces you already configured are listed " + + "and kept ticked even while they are down." } cfgSet := map[string]bool{} for _, t := range configured { diff --git a/internal/setup/setup_test.go b/internal/setup/setup_test.go index 5644d0c..626747b 100644 --- a/internal/setup/setup_test.go +++ b/internal/setup/setup_test.go @@ -432,6 +432,66 @@ func TestAnsweringNothingChangesNothing(t *testing.T) { } } +// The PR's headline guarantee, driven through Answers rather than a hand-built +// Input: leaving automatic detection on must not blank a server set by hand. +// TestAnUnaskedEndpointListTouchesNoEndpoint pins Apply's half of this with a +// nil Endpoints; this pins that the wizard actually produces that nil. +func TestLeavingAutomaticOnKeepsAConfiguredEndpoint(t *testing.T) { + cfg := config.Default() + cfg.BlockedCountries = []string{"IR"} + cfg.VPN.Endpoints = []string{"203.0.113.7"} + config.Normalize(&cfg) + + qs := Questions(Options{Config: &cfg, GOOS: "darwin"}) + a := NewAnswers(qs) + a.Set("autoMode", "true") // the recommended answer, and the default + + in := a.Input(strconv.Itoa(cfg.Hysteresis), nil) + if in.Endpoints != nil { + t.Errorf("Endpoints = %v, want nil: the question was gated away", *in.Endpoints) + } + after := cfg + in.MacOS, in.ConfigExisted = true, true + Apply(&after, in) + config.Normalize(&after) + if got := after.VPN.Endpoints; !reflect.DeepEqual(got, []string{"203.0.113.7"}) { + t.Errorf("endpoints = %v, want the configured one untouched", got) + } +} + +// Two separate pieces of machinery assume gates are shallow and never point +// forward: the CLI's wave loop (a gate question this run will never show is +// treated as fixed at its seed, which is only safe when gates cannot nest) and +// Answers.wasAsked (which re-evaluates a gate against the FINAL answers, so a +// question gated on a LATER group's answer would be judged against a seed). +// Both are correct only because of the shape pinned here. Adding a gated gate, +// or a gate pointing at a later group, means fixing those two first. +func TestGatesAreShallowAndPointBackwards(t *testing.T) { + for _, goos := range []string{"darwin", "linux", "windows"} { + cfg := config.Default() + qs := Questions(Options{Config: &cfg, GOOS: goos}) + byid := byID(qs) + for _, q := range qs { + if !q.Gated() { + continue + } + gate, ok := byid[q.RequiresID] + if !ok { + t.Errorf("%s/%s: gate points at unknown question %q", goos, q.ID, q.RequiresID) + continue + } + if gate.Gated() { + t.Errorf("%s/%s: gate %q is itself gated; the wave loop assumes depth 1", + goos, q.ID, gate.ID) + } + if gate.Group > q.Group { + t.Errorf("%s/%s: gate %q is in a LATER group (%d > %d); wasAsked would read a seed", + goos, q.ID, gate.ID, gate.Group, q.Group) + } + } + } +} + func TestValidDuration(t *testing.T) { for _, ok := range []string{"30s", "5m", " 1h "} { if err := ValidDuration(ok); err != nil { From 0257c104a283d2e813f8a15b9408148a18e42605 Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 5 Sep 2026 18:56:02 +0330 Subject: [PATCH 5/6] docs(setup): the two-step check was written for macOS only Round 3 of the review loop. No defects in the wizard itself this time; the findings are about what the branch tells people it does. - The "Two steps" on-host check sits in the cross-platform section but describes macOS behaviour: it says leaving automatic detection ticked ends the wizard, which off macOS is false. There is no live discovery there, so the endpoint question is ungated and rides on the first prompt beside the tickbox, and unticking brings only tunnel interfaces and config files. A tester on Linux would have marked correct code as failed. The item now splits the two platforms. - The changelog bullet claimed "a question that is not asked writes no key" without the exception it shares a paragraph with: choosing automatic detection deliberately CLEARS pinned interfaces, because a leftover pin is what stops autodetection happening. cli.md already stated this; the changelog now does too. - `setup --questions` printed option values only, so the "(configured, not up right now)" label added last round was visible in --json and invisible in the plain text a human actually reads to answer "why is that interface on the list?". Options now render their label where it adds anything, which also turns the country list from "IR, RU, CN" into "Iran (IR), Russia (RU)". Nothing pins that output and the app reads --json, so this is display only. TestOffMacOSTheEndpointQuestionRidesTheFirstWave closes the coverage gap the same round named: every wave test was GOOS darwin, leaving the platform where the wave shape actually differs unpinned. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 +++++--- cmd/dezhban/setup.go | 16 +++++++++++++++- cmd/dezhban/setup_wave_test.go | 22 ++++++++++++++++++++++ docs/contribute/testing.md | 9 ++++++--- 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baf5e6d..99cd3a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,9 +40,11 @@ current as you land changes. VPN keys, so instead the detection answer is **seeded from your config** and a config with pinned `vpn.tunnelInterfaces` starts on manual, meaning a re-run clicked straight through preserves your pins — even when that tunnel is down, - which previously dropped it from the pick list and so cleared it. A question that is not asked - still writes no key, so leaving automatic detection on does not blank - endpoints you set by hand. Off macOS, where there is no live discovery, the + which previously dropped it from the pick list and so cleared it. A question + that is not asked still writes no key, so leaving automatic detection on does + not blank endpoints you set by hand; the one deliberate exception is that + *choosing* automatic detection clears pinned interfaces, since a leftover pin + is what stops autodetection happening. Off macOS, where there is no live discovery, the endpoint question is asked whichever mode you pick. Both wizards read the same question set, so `dezhban setup` changes with the app — in a terminal, unticking automatic detection brings the manual fields as a second prompt diff --git a/cmd/dezhban/setup.go b/cmd/dezhban/setup.go index 7d3d4ce..526db8f 100644 --- a/cmd/dezhban/setup.go +++ b/cmd/dezhban/setup.go @@ -278,9 +278,23 @@ func printQuestions(qs []setup.Question, asJSON bool) int { fmt.Printf(" selected: %s\n", strings.Join(q.Selected, ", ")) } if len(q.Options) > 0 { + // Value is what gets written, but an option whose label says more + // than its value — a tunnel offered because it is configured rather + // than because it was detected — has to show that here too. This is + // the form a human reads to answer "why is that one on the list?"; + // --json already carries both. vals := make([]string, 0, len(q.Options)) for _, o := range q.Options { - vals = append(vals, o.Value) + switch { + case o.Label == "" || o.Label == o.Value: + vals = append(vals, o.Value) + case strings.Contains(o.Label, o.Value): + // The label already carries the value, as the tunnel list's + // "utun9 (configured, not up right now)" does. + vals = append(vals, o.Label) + default: + vals = append(vals, fmt.Sprintf("%s (%s)", o.Value, o.Label)) + } } fmt.Printf(" options: %s\n", strings.Join(vals, ", ")) } diff --git a/cmd/dezhban/setup_wave_test.go b/cmd/dezhban/setup_wave_test.go index aecc4c2..7ae9bd2 100644 --- a/cmd/dezhban/setup_wave_test.go +++ b/cmd/dezhban/setup_wave_test.go @@ -71,6 +71,28 @@ func TestStepTwoArrivesInWaves(t *testing.T) { } } +// Off macOS there is no live discovery, so the endpoint question is ungated and +// rides on the FIRST prompt beside the tickbox — a Linux user who leaves +// automatic detection on must still be asked for a server, or the config cannot +// enforce. That makes the wave shape genuinely platform-dependent, which is why +// it is pinned rather than left to the darwin cases above. +func TestOffMacOSTheEndpointQuestionRidesTheFirstWave(t *testing.T) { + for _, goos := range []string{"linux", "windows"} { + t.Run(goos, func(t *testing.T) { + cfg := config.Default() + qs := setup.Questions(setup.Options{Config: &cfg, GOOS: goos}) + a := setup.NewAnswers(qs) + + // Automatic detection left on, the recommended answer. + got := drive(qs, 2, a, func(string) {}) + want := [][]string{{"autoMode", "endpoints"}} + if !reflect.DeepEqual(got, want) { + t.Errorf("waves = %v, want %v", got, want) + } + }) + } +} + // Ticking automatic detection retracts the manual half of step 2. The three // gated questions must then never be put on a form at all — the wave loop is // what makes that true, and it is the case the old mark-inside-the-pass loop got diff --git a/docs/contribute/testing.md b/docs/contribute/testing.md index 9d6b0e7..84d23ad 100644 --- a/docs/contribute/testing.md +++ b/docs/contribute/testing.md @@ -725,9 +725,12 @@ macOS only, privileged (`dezhban upgrade download`/`apply`). See - [ ] Re-running it **without** naming profile files keeps the profiles already imported (`dezhban vpn list`). - [ ] **Two steps.** Step 1 is countries. Step 2 opens with "Use automatic VPN - detection?"; leaving it ticked ends the wizard there, unticking it asks for - tunnel interfaces, self-hosted config files, and endpoints. In a terminal - that second half arrives as a follow-up prompt, not the same screen. + detection?". **On macOS**, leaving it ticked ends the wizard there, and + unticking it asks for tunnel interfaces, self-hosted config files, and + endpoints as a follow-up prompt rather than on the same screen. + **Off macOS** the endpoint question is ungated — there is no live discovery + to find a server — so it rides on that first prompt beside the tickbox, and + unticking brings only tunnel interfaces and config files. - [ ] **A re-run on a pinned config keeps its pins.** With `vpn.tunnelInterfaces` set, re-run and press Enter through everything: automatic detection must arrive **already unticked**, and From 8bb6d1c4d1cad2c53864a82e3a34aa786a5f33dd Mon Sep 17 00:00:00 2001 From: Behnam RK Date: Sat, 5 Sep 2026 19:02:48 +0330 Subject: [PATCH 6/6] fix(setup): a comma inside an option label read as two options Round 4, and the only finding is the loop's own. Printing option labels last round put prose into a comma-joined list, and the one multi-word label in the tree contains a comma: a re-run with the VPN down printed options: utun0, utun4, utun9 (configured, not up right now) which reads as four options, one of them an interface named "utun9 (configured". Joining with semicolons fixes the class rather than this instance, so a future label carrying a comma cannot reintroduce it. Co-Authored-By: Claude Opus 5 --- cmd/dezhban/setup.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/dezhban/setup.go b/cmd/dezhban/setup.go index 526db8f..7ae80cf 100644 --- a/cmd/dezhban/setup.go +++ b/cmd/dezhban/setup.go @@ -296,7 +296,11 @@ func printQuestions(qs []setup.Question, asJSON bool) int { vals = append(vals, fmt.Sprintf("%s (%s)", o.Value, o.Label)) } } - fmt.Printf(" options: %s\n", strings.Join(vals, ", ")) + // Semicolons, not commas: labels are prose and may contain a comma + // themselves ("utun9 (configured, not up right now)"), which in a + // comma-joined list reads as two options, one of them an interface + // named "utun9 (configured". + fmt.Printf(" options: %s\n", strings.Join(vals, "; ")) } if q.Gated() { fmt.Printf(" asked only when %s is %s\n", q.RequiresID, q.RequiresValue)