TUI UX improvements - #41
Open
guygrigsby wants to merge 70 commits into
Open
guygrigsby wants to merge 70 commits into
guygrigsby wants to merge 70 commits into
Conversation
Endpoint validation lived in internal/tui as endpointFromInput, so the rule for what counts as a reachable Aperture location was only enforced on the two screens that happened to call it. The inline URL override added next runs outside those screens and needs the same rule, and copying it would have left two definitions to drift apart. Moves Endpoint, Bridge and DefaultLocation into endpoint.go with ParseEndpoint, leaving settings.go holding the persisted Settings and its file IO. This is the first step of the DDD split: config becomes the domain package and its store moves out later. Revisit if the store split lands first, which would make endpoint.go the seed of a separate domain package instead.
Adding a bridge endpoint asked for a URL before it would connect, while a direct connection just tries the well-known location and only asks if that fails. Nobody adding their first bridge knows the hostname yet, so the flow stopped on a question the user had opened the bridge to answer. A bridge now probes DefaultLocation through the new node straight away. The guess is not free: someone who does know their hostname would be stuck watching it time out, so the connect screen carries a live URL field that cancels the running attempt and retargets it, and Esc abandons the attempt outright. Both go through the same cancellation, which is also the only exit from a bridge waiting on a login that will never come. Cancelling or retargeting removes the guessed endpoint it wrote to settings, so an abandoned attempt leaves nothing behind, and attempts carry an id so a cancelled probe's late result cannot take the screen back. The in-flight state moved off the model into an activation type rather than becoming six more model fields. Pasted input was dropped here: a multi-rune paste failed the old len(s)==1 check, and matching on KeyMsg.String() instead would have typed "up" into the field when someone pressed Up. textField keys off the message type.
A guessed URL that answers is not the same as the right Aperture. On a tailnet that already has a host called "ai", the default guess connects, so the two existing ways to change the URL both go missing: the connect screen's inline override is gone the moment the attempt succeeds, and the setup guide only appears on failure. Deleting the endpoint and adding it back guesses "ai" again and lands in the same place, so a user who wanted a different Aperture, or the same one through a bridge into another tailnet, has no way to say so. The endpoints menu takes "e" on the row under the cursor and edits its URL, keeping the bridge it is reached through, then reconnects. The setup guide's editor is now that same prompt rather than a second copy of it, and it still follows the failed endpoint through the rename so the failure screen keeps naming what is being tried. The alternative was asking for a URL again before every bridge connection, which is what the discovery flow removed: nobody adding their first bridge knows the hostname. The guess stays; correcting it no longer requires it to fail first.
Picking a bridge is picking a tailnet, and the connection picker has to say which one a bridge reaches before the user selects it. Nothing in settings knew: a bridge was an ID and a name, and the tailnet only existed in the running node's status, so a bridge that had not been started this session could not be labelled at all. Storing the name the node reported is a cache, not a source of truth, which is why SetBridgeTailnet treats an unknown bridge as a no-op and the accessor side prefers a live node's answer. The alternative, asking tailscaled or bringing every configured bridge up to read its status, costs a login per bridge to render a menu. Revisit if bridges ever hold more than one tailnet at a time; then this becomes a list and the picker needs to choose within a bridge.
A bridge holds one tailnet at a time, so letting the user change tailnets means logging the node out: tsnet reuses the credentials in its state dir on every start, so closing and reopening the node lands back on the same tailnet. SwitchTailnet therefore brings the node up before logging out, which is also what removes the device from that tailnet instead of orphaning it there, and drops the node so the next Activate builds a fresh one and prompts for a login. Up already returns the login status, so recording CurrentTailnet.Name costs no extra call. Doing it anywhere else would need a second LocalAPI round trip per bridge. The bring-up half of Activate moved into runningNode so the switch path shares it; the proxy half is unchanged.
With two bridges configured and a reachable http://ai, the launcher connects on its own at startup and nothing on screen leads to either bridge. The only path was Settings, Aperture Endpoints, "a", Bridge, pick: an add-an-endpoint flow used as a connect flow, two levels down and behind a key nobody is told about. Settings, Bridges looked like the right screen and its rows did nothing at all. So the picker is a visible row on the agent menu, and every action on it is a row too. Enter on a connection opens its page (connect, change URL, switch tailnet, remove) rather than connecting straight away: the "e" and "d" keys that did those things only worked if you already knew them, and a cursor-reading handler cannot be a visible row, because selecting it moves the cursor onto itself. It is the same screen as Settings, Aperture Endpoints rather than a second one, and the old a/e/d keys still work, so the existing flow is unchanged. A bridge with no endpoint gets a row as well, described by the endpoint it would create; that is how a second tailnet is reached the first time.
The bridge-mode walkthrough still sent readers to Settings and the "a" key, which is no longer how you reach a bridge, and nothing said a bridge is on one tailnet at a time or what switching costs.
A numbered row put picking an Aperture in the list of editors to launch, which is not what that list is for. It belongs with Settings and Install agents at the bottom, so [c] Change connection leads the hints there. Still on screen, so it is not a key you have to already know.
The footer promised it on every row and delivered on almost none. The handler indexed Settings.Endpoints by cursor position, which stopped being the row list when the picker grew rows for bridges that have no endpoint: the cursor on one of those indexed past the end and the key silently did nothing. A single saved endpoint was inert too, refused by a length guard before the branch that would have explained why. So both aliases resolve a row through connectionRows, the same list the screen is drawn from, and removal is the row's own action: delete the endpoint, or the bridge when nothing points at it. The active row now says why it stays instead of ignoring the key.
tsnet reprints "restart with TS_AUTHKEY set, or go to: <url>" every five seconds until the node is authorized, so the connect screen filled with the same URL seven times over and the only way forward was copying it out of a terminal by hand. The log tail now carries one "Authorize this bridge in your browser" line per distinct URL and the launcher starts the platform opener on it. Detection sits in the bridgeLogMsg case because that is the single point every bridge log line crosses; parsing it in the manager would mean a second sink next to the one the screen already reads. The dedupe key lives on the activation so a tailnet switch, which produces a new URL inside the same attempt, opens again. exec.Start, not Run: an opener can block for the life of the browser it launches. That means a missing opener is caught and a headless box that has xdg-open but no display is not, which is why the link stays on screen either way. Revisit if a dependency ever shows up that handles the display check.
A bridge start that took over a minute looked hung: the attempt line is static, and between "Listening on <local>" and the result the screen says nothing at all while the /v1/models request runs, which is up to 30s for a bridge. Nothing on screen distinguished that from a deadlock, and the run that prompted this did finish and launch its agent. So the attempt counts itself up once a second, and the bridge path logs the request it is waiting on before it makes it. A spinner was the alternative and carries less: the number is what tells you whether to keep waiting or press Esc. The tick is keyed to the attempt id and stops as soon as the screen changes, so a cancelled or superseded attempt cannot leave a timer running behind the menu. activateEndpoint now returns a batch, which is why the tests unwrap one.
A first connection through a freshly started bridge hung for the full 30s fetch timeout and only worked on a manual retry. A SIGQUIT dump caught it: the dial was parked in tsdial.SystemDial on a host-network connect that never completes. tsnet's UserDial resolves MagicDNS from the node's netmap and falls through to the host resolver when the netmap has not landed yet. On a machine that is itself on a tailnet, that fallback answers: this box's own tailnet has a node called "ai" at 100.81.69.95, while the bridge's tailnet has one at 100.105.9.12. tsnet has no route for the foreign address, so it system-dials it and blackholes. Had that node been listening on 80, the bridge would have quietly proxied to the wrong tailnet instead, which is the worse half of the bug. So resolve the target through the node itself: poll its status until the target shows up as a peer, then dial that IP. A name that never appears is not necessarily broken (a subnet router or the tailnet's DNS can serve it), so after the window we still hand the name to tsnet, now with a log line saying the dial may leave the tailnet. This replaces dialWithDNSRetry, which retried only *net.DNSError. That was aimed at the same window but never fired here: the leaked lookup succeeded. Measured against the real tailnet, cold node, the peer map takes ~1.7s to arrive, so the 5s window has room; raising it would only lengthen the wait for targets that are legitimately not peers. Revisit if a slow link pushes a real target past it.
…poll A bridge that had never logged in sat on "LocalBackend state is NeedsLogin" for 16s and got SIGQUIT'd as hung. It was not hung: its logtail buffer shows the control plane answered with an auth URL 300ms before the kill, and tsnet only prints that URL from printAuthURLLoop, a 5s poll. Measured against a fresh node, the bus has the link at 4.04s and tsnet prints it at 5.02s, so the link can be a full poll interval late on top of however long registration took, with nothing on screen saying what is being waited for. Watching the bus alongside Up costs one goroutine that ends with the activation. Leaving it to tsnet would have meant either living with the 5s window or polling Status ourselves, which is the same information arriving later. Revisit if tsnet grows a callback for this.
The browser open is unreachable over SSH and fails invisibly: xdg-open
exists on the remote box so Start succeeds, then it exits 3 ("no method
available") a moment later, by which time nothing is watching. The link
was a dim line in a log tail that tsnet keeps pushing around, so the one
thing the user has to act on looked like chatter.
The link now has the foot of the connect screen to itself, in the
palette's bright green on a dark terminal and its plain green on a light
one, with a copy button next to it. Copying goes over OSC 52 rather than
xclip/pbcopy: the clipboard that matters belongs to the terminal the
user is looking at, which over SSH is not the machine aperture runs on.
tmux and screen get their passthrough wrapping.
Mouse reporting is only on while that button is showing. Leaving it on
costs the terminal's own click-drag selection everywhere else, which is
a bad trade on screens full of URLs and error text.
The click hit test matches columns and ignores the row: this TUI renders
inline, so the row the footer landed on is not knowable from the model.
Worth revisiting if it ever moves to the alternate screen.
A bridge that had never connected took 29s to come up and the log gave no way to say where the time went. Three changes have now been aimed at that wait (the IPN bus watch, the peer-map resolve, the progress tick), each picked from a symptom, because the connect screen prints an unordered bag of strings: "waiting for a login link" and "Bridge connected." sit on adjacent lines whether the gap was 200ms or half a minute. The goroutine dump from this one shows the node parked in the control plane's first /machine/register with no followup URL, so the wait was upstream of every line we print, which is exactly the thing the log should have said. Stamped at the sink rather than where the message is handled: tsnet logs arrive in bursts and a stamp read after the channel queue attributes the queueing delay to the wrong line. Carrying the elapsed time as a field rather than a prefix keeps importantBridgeLog matching on text. Revisit when the log stream becomes typed events; the stamp belongs on the event then, not on a rendered line.
A bridge that had never connected took 29s and the screen showed only NeedsLogin. The dump puts the node in the control plane's first /machine/register with no follow-up URL, so it was waiting for a login link to exist. That is a different wait from waiting for the user to finish in the browser, and both are ipn.NeedsLogin, so nothing on screen could tell them apart. Three fixes have now been aimed at that wait: opening the browser at the link, reading the link off the IPN bus, resolving the target against the peer map. All correct, none in the phase the wait was in. Modelling first rather than patching again because the thing missing is a name. Four mechanisms carry progress out of internal/bridges (a func(string) sink, a chan bridgeLine, tsnet's own prose, an *ipnstate.Status return) and none says what the attempt is waiting on, so a fourth fix would be aimed the same way. Two defects fall out of the same shape: the TUI recovers the login link by matching a phrase inside tsnet's log text, and the link rides a 32-slot channel that drops on overflow while --debug puts the tsnet backend logger on the same channel. Writing it down rather than going straight to code because the decisions are the expensive part: Connection spans bring-up and the model fetch as one context, ApertureHost splits into Endpoint and Gateway, and owning the IPN bus watch means dropping tsnet.Server.Up and absorbing what it does beyond waiting for Running. That last one is reversible only at the cost of going back to two watchers on a LocalBackend whose own comments assume one. CLAUDE.md records the conventions these follow so they are reviewable in a diff rather than living in one person's tooling. All five Mermaid diagrams rendered before committing.
CLAUDE.md is one vendor's filename for a file that every agent in this repo reads. AGENTS.md is the cross-tool convention, and the contents are project conventions, not instructions to one assistant.
The model named six events in a three-column table, which is the failure the defining-contracts skill exists to catch: a name is not a contract. Filling every field found two holes that reading the model well did not. The domain service column is the anti-anemia check, and two events have a reaction with no owning object. TailnetJoined spans Crossing and Bridge and is resolved today by recordBridgeTailnet reaching from the TUI into the manager and then into settings. Ready spans the attempt and Client Launch and is resolved by assigning g.ApertureHost, a shared mutable global five client packages read whenever they run. Nothing owns "which Gateway is current", which is how that field came to mean two things without anyone deciding it should. Both go back to the model as open rather than getting a name here. API and DDL are recorded absent with reasons rather than skipped: the CLI has no callers to enumerate and no relational store. The schema rules still catch one thing, Bridge.Tailnet being empty until a crossing joins, which is a nullable "has not happened yet" in JSON clothing. Kept, with the exception recorded next to it, because a settings document rewritten whole does not want a collection to model an absence the picker already renders.
The file named a specific plugin and two skill names, which is one contributor's toolchain leaking into a project convention. What the repo needs to say is the rule, not what anyone runs to follow it. Also corrects the artifact list to match the discipline: domain model, data model and contracts are one first step rather than a model followed later by contracts. Writing them apart is what produces a model that reads well and a set of events that turn out to be names.
CI restated the formatting check inline and then called make test, so the workflow and the Makefile were two definitions of the gate and a clean local run did not mean a clean CI. check earns its place over test by running the suite under the race detector and by building. A bridge is several goroutines racing a control plane over channels nobody owns end to end, which is the failure this project actually has, and go test will not find one. Dropped redeploy: install covers it. lint is gofmt plus vet plus a tidy diff, all from the toolchain, so a clean checkout runs it without installing anything. Reaching for golangci-lint would have meant pinning a version and a config file to catch what vet already catches here. Cost worth knowing: the race build of the tailscale tree is slow, minutes not seconds, and the matrix runs it twice.
Crossing was invented. The thing already has a name in the system it wraps and we were ignoring it: the control plane registers it with POST /machine/register (controlclient/direct.go:839), keys it with a MachineKey, and reports ipn.NeedsMachineAuth when it is unauthorized. The user reads that same word in their admin console under Machines. An invented name costs a translation every time someone moves between this code, a tsnet trace and the console, and that translation is where "the bridge is not logged in" became unreadable in the first place. Node was the alternative and is worse: tsnet uses it for our node and for every peer in the netmap, so it is ambiguous in the one package that has to be exact. The word now collides with "the computer aperture runs on", which browser.go:69 uses when it explains that an SSH session writes to the wrong clipboard. The domain object takes the word, the computer is the host, and that comment gets reworded when its file is touched. Recorded in the ambiguous-terms table so the collision is decided rather than rediscovered. Also fixes the ADR's pointer to CLAUDE.md, renamed in 3b725d8. Revisit if the control plane renames it: the v2 API and /machine/set-device-attr already say "device" at the edges.
A first bridge connection spent 29 seconds with nothing on screen but tsnet's "NeedsLogin", and three separate fixes have now been aimed at that wait without anyone knowing which part of it was slow. The cause is that ipn.NeedsLogin covers two waits that are not the same problem: before a BrowseToURL arrives the control plane has not answered and there is nothing the user can do, and after it arrives everything is waiting on them. Reporting the backend state cannot tell them apart, so the screen could not either. Two other defects came from the same place. The login link travelled as a string on a channel whose sink dropped whatever arrived on a full buffer, and under -debug the tsnet backend logger shares that buffer, so a burst of chatter could discard the one line the user cannot proceed without. And the browser opened on a line matching "or go to: ", a phrase from inside a vendored package, which an upstream reword would have broken silently. internal/connection carries the vocabulary now: six Phases, a LoginLink that validates at the boundary, and an Event the producer cannot reword. The sink drops only diagnostics; anything else waits for room, bounded by the attempt's cancellation. Phases land in the existing timestamped pane, so the screen reads where the time went with no view changes. The alternative was a second channel for the link alongside the log, which keeps the string matching for everything else and gives the TUI two orderings to reconcile. Revisit the package if it only ever holds these three Kinds; the Ready and Failed events the ADR names still travel on endpointActivationResult and are deliberately not here yet. tsnet's UserLogf is silent unless -debug: it is mostly printAuthURLLoop reprinting a link the footer already shows, every five seconds. It is a no-op func rather than nil because tsnet falls back to log.Printf when it is unset, which writes over the TUI.
The contracts pass specified six domain events; three are in the code. A spec that reads as fully implemented when half of it is not is worse than no spec, because the next reader trusts it and goes looking for a TailnetJoined that does not exist. Each gap gets its reason rather than a status: two are blocked on ownership decisions this pass explicitly deferred, PhaseEntered dropped its Progress payload because the screen already computes elapsed time from one clock and a second copy can disagree with the first, and the three terminal Phases wait for the Attempt aggregate rather than ship as constants nothing writes. Also corrects the JoiningTailnet signal in the Phase table: ipn.Starting is one notification covering what the table described as LoginFinished then SelfChange.
A first bridge connection sat for over a minute showing "Starting the bridge" and nothing after it. The goroutine dump has controlclient parked in POST /machine/register with an empty loginOpt.URL, so this is the initial register, not a followup poll. The translation missed the state that covers it. A bridge that has never logged in is ipn.NoState for the whole register and only reaches NeedsLogin once control answers with a URL, because nextStateLocked returns NeedsLogin only when cc.AuthCantContinue() is true. Mapping NeedsLogin alone therefore named every wait except the long one. Tailscale's own comment on the state says UIs should print "Loading...", which is the same observation. Silence was survivable before because tsnet's UserLogf dribbled backend lines into the pane; the commit that took the vocabulary off prose also gated that behind -debug, so the gap became total. Mapping NoState to AwaitingLoginLink rather than adding a phase of its own is deliberate: it is the same wait for the same thing, the user cannot act in either, and the phase guard collapses the NoState-then-NeedsLogin pair to one line. ipn.Stopped and ipn.InUseOtherUser are still unmapped and still silent. Neither is reachable on the path this fixes.
A node is cached in Manager.nodes and its proxies in nodeRuntime.proxies for the life of the process. The connection that built them ends with the connect screen. runningNode gave the node's UserLogf and DebugLogf a closure over that connection's sink, and startProxy did the same for the transport's DialContext and the proxy's ErrorHandler, so from the second connection onward every "Bridge dial failed" and every "Bridge proxy error" was written to a channel nobody had read since the first one finished. That is the output most worth having: a bridge that breaks mid-session breaks in the proxy, not during bring-up. It was silent, and silently, which is why nothing caught it. nodeRuntime gains one field rather than Manager gaining state: the sink belongs to the node that reports through it, and Manager already holds more than it should. Passing the sink down per call was the alternative and does not work, because the closures are installed once at construction and run on goroutines the caller does not own. Nothing clears the sink when a connection ends, so a node with no connection in progress still holds the last one's. Harmless: that sink discards what it is given once its context is cancelled, which is the behaviour this replaces. Clearing needs a lifecycle hook the Attempt aggregate will own.
Mouse reporting was on for the whole time the login link showed, which is the one screen whose text people need to get out of the terminal. With reporting on the terminal forwards drags and ctrl-clicks to the app, so selection, copy and open-URL all died exactly where they mattered, and over SSH the click never arrived at all: tmux without `mouse on` and terminals with reporting off never send the events, leaving those users no copy path. The click was there because the override editor owns every printable key on that screen, so ctrl+y takes its place: `textField.insert` already drops control runes, so the chord costs the editor nothing. The link also had to come off the prose line. Bubble Tea's renderer truncates anything wider than the terminal, so a long URL wraps, and the old footer wrapped it with a "copy" label and the prose on the same lines, so a selection picked those up too. Alone on bare lines it pastes clean, browsers strip the newline. Each piece carries the same id-tagged OSC 8 hyperlink so the terminal rejoins them into one ctrl-click target. Keeping both was not an option: reporting on is what breaks selection, so the mouse had to go for the rest to work. Revisit if the override editor ever leaves this screen and the keyboard frees up.
A connect attempt that gets killed leaves nothing to read. The phases and notes it produced went to the connect screen and died with the process, and a 22 second kill this week left only a SIGQUIT dump, which says what the goroutines were parked on and nothing about what the run had already tried. slog's default handler made it worse: it writes to stderr, which under a TUI that owns the terminal is a line painted over the screen. So slog now points at <UserConfigDir>/aperture/aperture.log for every run, and `sink` tees every connection event into it on the way to the screen, including on the reuse path that passes no screen sink at all. Up is timed, because the number is what separates a slow control plane from a login link the user never saw. On for every run rather than behind -debug: the run worth reading back is the one that went wrong, and nobody knows to pass the flag before it does. -debug only raises the level to catch the tsnet backend chatter. The cost is a few hundred bytes per connect, capped by starting the file over at 2MB. Failures now print the error and the log path to stderr, since routing diagnostics to a file means a launch that dies would otherwise exit 1 in silence.
A run killed at 43 seconds logged "Waiting for a login link" and then nothing for 31 seconds. Three different failures produce exactly that trace and the log could not tell them apart: the IPN watch died, control sent a link that ParseLoginLink rejected, or control never answered the register. All three report through ev.note, and notes are logged at debug so the tsnet backend chatter stays out of a normal run, so all three were invisible on the run that hit one. The phase merge is deliberate and stays: NoState and NeedsLogin are one wait on screen because the user can do nothing about either. In a log they are the whole question, so the raw state goes to the file alongside the phase. A rejected link is logged with the URL, since "threw one away" and "never got one" want opposite fixes. A dead watch is an error, not a note: it leaves the attempt parked on its last phase forever. Also stamps the activation itself, so the gap before the first bridge line reads as what it is (someone choosing an endpoint) rather than startup.
… is waiting A bridge sat for 40 seconds showing "Waiting for a login link" while the control plane answered every register with `http 502: backend not found or not available; reqType=noise-register/machine-pubkey`. tsnet retried with growing backoff and a fresh nodekey each time, so registration never completed, no BrowseToURL was ever sent, and the screen's phase was accurate and useless: it named the wait without naming the reason the wait would not end. The reason reaches us only on ipn.Notify.Health, under the login-state warnable. ErrMessage stays nil because a register failure is not a vizerror, so watching it would have been the smaller change and would have caught nothing. Health is broadcast to every watcher on each change, which is why a passive read in notify is enough and no extra subscription is needed; NotifyInitialHealthState is added to the mask so a bridge that is already broken when we attach reports on the first notify rather than on the next change. Only login-state is surfaced. The other warnables fire for conditions the user cannot act on from this screen and cannot distinguish from noise mid-connect. Reporting is on the healthy to unhealthy transition, not on the text: the retry appends a fresh REQ id roughly once a second, so keying on the text would put a new line on the connect screen every second for the length of the outage. Note now flattens whitespace. That error arrives with its request ID on a second line, and Event.String promises one line of the activation log: the screen wraps and indents each line itself, so an embedded newline puts unindented text mid-block and miscounts the rows the renderer repaints.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Short-name resolution can escape the selected tailnet, endpoint credentials can enter logs, and removal has unresolved timeout and shutdown races.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (4)
Resolved since last review (8)
cmd.exe.ParseLoginLinkaccepts…m.nodesbeforeUp…AddBridgeaccepts duplicates—but this case-insensitive first-match…Makefile:30makescheckrun lint, build, and race…
guygrigsby
commented
Sep 21, 2026
One tui test built a model with no settings and ran an activation without t.Setenv, and once Begin started saving the candidate endpoint it overwrote ~/.config/aperture/settings.json on the developer's machine. Per-test Setenv relies on every author remembering; a TestMain in each package that writes settings makes forgetting harmless.
…ing in the TUI Manager grew from four fields to eight the day after ADR 0001 decision 6 said it would not, every behaviour the model gives Machine was a Manager method with the Machine passed in, and the lock was named acquire and documented as "a cancellable turn", a phrase in nobody's vocabulary that had reached ADR 0003 and the contracts. The TUI meanwhile decided the domain rules at twenty-eight sites: when a removal destroys a device, when the Bridge record goes, when the joined tailnet is recorded, when an edit commits. Machine now owns Open, RouteTo, LeaveTailnet, Destroy, Close and Tailnet with the one-at-a-time rule private. Machines is the collection. Bridging is the stateless domain service for what belongs to no single aggregate, and Attempt is the ConnectionAttempt entity; the TUI's activation keeps only presentation state and calls the service. Every service operation that waits on the network is split from the one that writes settings, because bubbletea runs commands off the update loop and config.Global has no lock; the race detector is the gate. Renaming acquire alone would have fixed the word and kept the shape. Leaving it for APT-330, which deletes Manager anyway, would have handed it a bigger rebase and left the model wrong on main meanwhile. ADR 0005 records the decision; revisit when APT-330 lands or an object owning the current Gateway exists.
… transitions An Endpoint carried a BridgeID that was empty for a direct connection, so every caller decided the kind by testing a string for emptiness and the two concepts shared one struct. Endpoint is now an interface with two closed implementations, DirectEndpoint and BridgeEndpoint; the kind is the type, the values compare with ==, and settings.json keeps the shape it had, with bridgeId present or absent, because the file predates the split and is not changing under existing users. The Bridging service from 8ce575d is gone. Its attempt half was the ConnectionAttempt's own transitions with the entity passed as an argument, so those are methods on Attempt now: BeginAttempt, Retarget, Run, Commit, Abandon. Commit rather than Succeed because it persists a result Run already produced and decides nothing; Fail is gone because whether the failing endpoint was the active one is knowable when the attempt begins, so it is a field. The removal half hangs off the real nouns it is about: Machines.Destroy and Machines.Tailnet on the collection, DestroysMachine and ForgetBridge as the two halves of removing a Bridge. No process object, no invented noun. The run log names the activated URL by scheme and host only: ParseEndpointURL accepts userinfo and a query, and the run log is the file people share.
…g out of it Kind duplicated what the populated field already said, so it is gone: a Phase, a Link or a Note is set and that is the kind. Text meant nothing and is Note. Login read as logging in and is LoginRequired. Phase.String and Event.String were the connect screen's sentences, and a screen is not a concept this package has; they are now the phase's name and the event's representation, and the sentences live in the TUI with the line flattening they exist for. Phase's zero value means no phase, which is what lets a field stand in for a kind.
Startup was a struct of two flag values with one method, named for when it ran rather than what it was. EndpointFromFlags is the function it was: given the flags, the Endpoint to open on. Nothing else held state, so nothing else needed a type.
ParseEndpointURL accepts userinfo and a query, so an endpoint like https://user:password@host or one carrying a token in its query wrote the credential into aperture.log, which is the file people share when asking for help. The activation record and the proxy error line now carry scheme and host and nothing else. url.Redacted was not enough: it masks the password and keeps the query.
Handing a bare name back to tsnet after the peer map failed to produce it reintroduced the wrong-tailnet resolution the peer-map lookup exists to prevent: tsnet's resolver falls through to the host resolver, and a host already on another tailnet answers with that tailnet's node of the same name. The five second wait only delayed the exposure. A bare name is a peer alias and nothing else, so it fails. A qualified name can be a subnet route or the tailnet's own DNS, which only tsnet can resolve, so it still falls through.
…Machine The removal's 45 second bound covered Logout, which takes the context, but not the node's Close after it, which does not. A close that hung held the removal past its deadline and the TUI with it. Destroy now runs its work on its own goroutine and returns when the work is done or the context ends. The Machine stays held until the work finishes, so the next operation on it waits for the close rather than opening the state directory underneath it.
Quitting during a removal closed the Machines, which cancelled the logout, and could exit before the outcome message dropped the settings records. The next run then named a device that was already gone. Ctrl+C during a removal now marks the intent and the quit happens once the outcome has been applied.
The model said Bridging and Removal; the code, after review, says Attempt, DestroysMachine, Machines.Destroy and ForgetBridge, and the domain model's Succeed/Fail/Cancel rows are Run/Commit/Abandon because that is what they do. ADR 0005 records the renames and why the first names went. ADR 0006 records the Endpoint split, whose forcing reason was a review comment and not a failure, so it is short.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved lifecycle and shutdown paths can reconnect the wrong tailnet, hide failed removals, and orphan bridge state.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 10
Open (11)
Comment on lines
+163
to
+167
| func orEnv(value, key string) string { | ||
| if value != "" { | ||
| return value | ||
| } | ||
| return os.Getenv(key) |
Contributor
Author
Comment on lines
+106
to
+113
| n := &Attempt{Endpoint: next, replaces: a.replaces, ephemeral: ephemeral, TargetsActive: next == g.ActiveEndpoint()} | ||
| if bridged, ok := next.(config.BridgeEndpoint); ok { | ||
| bridge, found := g.Bridge(bridged.BridgeID()) | ||
| if !found { | ||
| return nil, fmt.Errorf("bridge %s is not configured", bridged.BridgeID()) | ||
| } | ||
| n.bridge = bridge | ||
| } |
Contributor
Author
Comment on lines
+117
to
+123
| // Retry is the same attempt again. A tailnet switch is not repeated: it ran, | ||
| // or failed, the first time, and the retry is about reaching the Endpoint. | ||
| func (a *Attempt) Retry() *Attempt { | ||
| next := *a | ||
| next.switchTailnet = false | ||
| next.InvalidatesActive = false | ||
| return &next |
Contributor
Author
Comment on lines
+218
to
+224
| func (a *Attempt) Abandon(g *config.Global) error { | ||
| if a == nil || !a.ephemeral { | ||
| return nil | ||
| } | ||
| a.ephemeral = false | ||
| return g.DropEndpoint(a.Endpoint) | ||
| } |
Contributor
Author
| // AskingForModels phase and the verification everything else waits on. | ||
| func fetchProviders(ctx context.Context, host string, timeout time.Duration) ([]config.ProviderInfo, error) { | ||
| client := &http.Client{Timeout: timeout} | ||
| url := strings.TrimRight(host, "/") + "/v1/models" |
Contributor
Author
| peerWaitInterval: bridgePeerWaitInterval, | ||
| newNode: newTSNetNode(debug), | ||
| } | ||
| ms.shutdown = sync.OnceValue(ms.close) |
Contributor
Author
Comment on lines
+16
to
+17
| os.Setenv("HOME", tmp) | ||
| os.Setenv("XDG_CONFIG_HOME", tmp+"/.config") |
Contributor
Author
Comment on lines
+16
to
+17
| os.Setenv("HOME", tmp) | ||
| os.Setenv("XDG_CONFIG_HOME", tmp+"/.config") |
Contributor
Author
Comment on lines
+155
to
+159
| if m.quitAfterRemoval { | ||
| m.quitAfterRemoval = false | ||
| return m, m.quitCmd() | ||
| } | ||
| return m, cmd |
Contributor
Author
Comment on lines
+37
to
+41
| `Global.SetActiveEndpoint(ep Endpoint, replacing *Endpoint) error` atomically | ||
| persists `ep` first, removes duplicate `ep` entries and the optional original, | ||
| then updates in-memory settings. On a write error both settings and the runtime | ||
| host stay unchanged. Normal selection passes nil. The existing JSON schema is | ||
| unchanged; `activation.replaces` is a transient value, never persisted. |
Contributor
Author
guygrigsby
commented
Sep 21, 2026
| @@ -0,0 +1,44 @@ | |||
| # 0006. An Endpoint is one of two types, not a struct with an optional Bridge | |||
Contributor
Author
There was a problem hiding this comment.
This is not ADR worthy.
Contributor
Author
Comment on lines
+18
to
+21
| // Unconfirmed is a removal the tailnet did not confirm within the wait. The | ||
| // local records are gone; the device may not be, and the user has to be told | ||
| // where to look for it. | ||
| type Unconfirmed struct { |
Contributor
Author
There was a problem hiding this comment.
No. Unconfirmed is not a proper domain noun. Unconfirmed. isa modifier for another thing. In fact even this comment shows that this is nonsense.
Unconfirmed is a removal the tailnet did not confirm
What's a removal? This is all wrong. If we want to remove a thing then we call that on a function or method and return a success of failure. If it's async, return a channel.
Contributor
Author
There was a problem hiding this comment.
Gone. Destroy returns nil or an error and nothing else. A logout the tailnet did not answer in time is a failed removal like any other: the records stay, the failure screen names the device and removing the connection again retries the logout. That also closes the orphaned-login hole Copilot found at machine.go:319. 8c82bb5
Fixed in 6bfb938
| // nil, takes a Machine off a tailnet: ep is the Bridge's last Endpoint and the | ||
| // Bridge has started a Machine. A Bridge that never started has no device, and | ||
| // must not start one to find out. An error means it may not be removed at all. | ||
| func DestroysMachine(g *config.Global, bridge config.Bridge, ep config.Endpoint) (bool, error) { |
Contributor
Author
There was a problem hiding this comment.
don't use made up acronyms. call it an endpioint. This. this is also a bad name for a predicate function. DoesDestoyMachine
Contributor
Author
Comment on lines
+70
to
+73
| // Tailnet is the network a Bridge reaches, preferring what its running | ||
| // Machine reports to what was saved: a bridge that switched tailnets this | ||
| // session leaves a stale name on disk until the next verified connection | ||
| // rewrites it. |
Contributor
Author
There was a problem hiding this comment.
Clunky sentence.
Contributor
Author
| return bridge.Tailnet | ||
| } | ||
|
|
||
| // ForgetBridge drops the records a removal covers, endpoint first: a Bridge |
Contributor
Author
There was a problem hiding this comment.
This whole comment reads like a shredded newspaper glued together wrong. Use short sentences and write clearly.
Stop slapping together noun clauses.
Contributor
Author
There was a problem hiding this comment.
Rewritten and renamed RemoveFromSettings. Global.RemoveBridge deletes one bridge record and refuses while an endpoint still uses it. RemoveFromSettings deletes the endpoint first, then the bridge once nothing connects through it, because the picker shows the two as one row and deleting the endpoint alone left the bridge behind as a bare row. The destroyErr parameter went with Unconfirmed. 8c82bb5
Fixed in 6bfb938
| return LoginLink{url: raw}, nil | ||
| } | ||
|
|
||
| // Event is one thing a Machine reports while an attempt uses it. Exactly one |
Contributor
Author
There was a problem hiding this comment.
Event is one thing a Machine reports while an attempt uses it.
What's "it?" rewrite this. be clear.
Contributor
Author
| // Verified is what a successful attempt produced: the Gateway a client sends | ||
| // requests to, the providers it answered with and, through a Bridge, the | ||
| // tailnet the Machine joined. | ||
| type Verified struct { |
Contributor
Author
There was a problem hiding this comment.
Not a noun. Verfied is much more likely to be a function.
Contributor
Author
| // | ||
| // Nothing clears it when a connection ends: a finished sink discards what it is | ||
| // given, and a clear needs a lifecycle hook only the Attempt can own. | ||
| type liveEvents struct { |
Contributor
Author
There was a problem hiding this comment.
confusing description. andname
Contributor
Author
| } | ||
| } | ||
|
|
||
| // events is where a bridge reports what it is doing. This package translates |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| // events is where a bridge reports what it is doing. This package translates | |
| // events is where bridge status is repoorted. This package translates |
more confusing sentences.
Contributor
Author
| func (e events) enter(p connection.Phase) { e(connection.Entered(p)) } | ||
| func (e events) loginRequired(link connection.LoginLink) { e(connection.LoginRequired(link)) } | ||
|
|
||
| // redactURL is the part of an endpoint URL safe for the run log: scheme and |
Contributor
Author
There was a problem hiding this comment.
redactURL redacts a URL maybe would be a better comment? Or in fact because it says exactly what it does maybe a comment is not necesary.
Contributor
Author
Splitting Endpoint into two types is a refactor the code and its doc comments already explain. Nothing about it needed a decision record, and review on PR 41 said so.
The contract said *Endpoint while the signature has taken the interface, with nil for absence, since Endpoint became one. Whoever reads the contract first would write a pointer.
Both flags fell through to APERTURE_BRIDGE and APERTURE_ENDPOINT whenever their value was empty, so a shell with the variable exported had no way to ask for the saved endpoint. The README promises flag over environment. flag.Visit tells a passed flag from an absent one; the value cannot.
ParseEndpointURL accepts a query, so a saved endpoint can carry one, and string concatenation turned http://host?token=x into http://host?token=x/v1/models: a GET of / with a mangled token. url.JoinPath keeps the query where it was.
Clearing the flag before DropEndpoint meant a failed write left the candidate in settings with nothing that would ever try again: the next Abandon saw ephemeral false and returned nil.
os.UserConfigDir reads APPDATA on Windows and ignores HOME and XDG_CONFIG_HOME, so the TestMain isolation added after the 2026-09-21 overwrite protected only Linux and macOS.
Dropping settings on a timed-out logout orphaned the login on disk: the state directory survived, the bridge record naming it did not, and nothing was left to retry the cleanup through. Every failure now keeps the endpoint and bridge, reports the failure with the device name and lets the user remove the connection again, which retries the logout. Unconfirmed is gone with the special case. A Ctrl+C deferred past a removal that then fails shows the failure instead of quitting, since quitting would exit 0 with the only message naming the device gone. Removable, DestroysMachine, ForgetBridge, bridgeUsed and through are renamed CheckRemovable, WillDestroyMachine, RemoveFromSettings, endpointsThroughBridge and isReachableThroughBridge: predicates start with a verb and the settings function says which layer it writes.
Retry cleared switchTailnet unconditionally and Retarget never copied it, so a logout the control plane refused, or one cancelled by a typed URL, was followed by an attempt that opened the credentials still on disk and reconnected to the tailnet the user asked to leave. Run now records when LeaveTailnet succeeded and both Retry and Retarget carry the switch until then. Retry builds the new Attempt field by field because the flag is an atomic.Bool, written on Run's goroutine and read on the update loop. Verified becomes Gateway. Verified is a participle, not a thing; Gateway is the domain model's name for where a client sends requests, and the struct holds exactly that plus the tailnet and providers it came with.
…annot leave Machines.Close memoizes its result, so the error screen's q called Close again, got the same error and never quit. The TUI quits regardless and lets main call Close, which returns the memoized error and reports it on stderr with exit 1 once the terminal is back.
Review on PR 41 read liveEvents, events and Event as glued clauses. Each sentence now names its subject and gives it a finite verb. liveEvents is eventRelay and use is forwardTo, which is what it does.
Guy's review of PR 41 read the bridges comments as clauses glued together. Every sentence now names its subject and gives it a finite verb. Machine.of is Machine.machines (a preposition is not a name), notifier is ipnBusWatch (the thing bringUp reads, not a role) and loginReporter is bringUpProgress (the state it holds). fetchProviders no longer shadows the url package.
Same review as the bridges pass. cancelable, removing, overridable and important are canCancel, isRemoving, canOverride and isImportant.
DropEndpoint and RemoveEndpoint were two verbs for one action split by argument type. RemoveEndpoint now takes the Endpoint, which every caller outside the package had; the index form is removeEndpointAt and private.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bridge mode was two levels down behind a key nobody is told about, and a stalled
connection showed
NeedsLoginfor half a minute while the log kept nothing.Finding and picking a connection
con the agent menu opens Change connection, the same screen as Settingsthen Aperture Endpoints.
Adding a bridge no longer asks for a URL. It probes
http://aithrough thenew node, the same guess a direct connection starts from, and the connect
screen carries a live URL field that cancels the attempt and retargets it for
people who do know their hostname.
flags
-endpointand-bridgepick the connection from the invocation, withAPERTURE_ENDPOINTandAPERTURE_BRIDGEbehind them for the places nobodytypes it.
-bridgetakes a name and creates the bridge when there is none,since a flag that only worked after someone made the bridge by hand would
miss the first run:
Resolution happens in
mainbefore the TUI takes the terminal, so a URL we cannot use is a line on stderr
and exit 1 rather than a full-screen error a script never sees. Neither
becomes the saved active endpoint until the connection works.