TUI UX improvements - #41
Open
guygrigsby wants to merge 46 commits into
Open
guygrigsby wants to merge 46 commits into
guygrigsby wants to merge 46 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.
Deleting a bridge-backed connection took two presses. The picker draws one row per endpoint and then one per bridge no endpoint claims, so removing the endpoint left the bridge to be re-listed as a bare "Connect via" row at the bottom of the same list. Nothing said an object had been deleted and a different one had appeared, so it read as the row moving instead of going. The cascade is safe to make silent because RemoveBridge is config only: it drops the settings entry without logging the node out of the tailnet or touching its state directory. The alternative, leaving both objects and labelling the second row so the user could tell it apart, keeps a two-step delete for something presented as one thing. Only endpoints are cascaded from, and only when no other endpoint reaches through the bridge, so a shared bridge survives. Left config.RemoveEndpoint alone: discardActivation calls it to clean up a cancelled attempt, and a bridge the user made earlier should not disappear because they backed out of one connection through it. This overturns a deliberate choice. TestConnectionPicker_RemovesInactiveConnection asserted the bridge row coming back, with a comment saying so.
A bridge connection can only be started by hand: open the picker, pick or add a bridge, wait. Nothing about that is scriptable, so a machine image, a container or a dotfile cannot ship a working launcher, and every new user retypes the same two answers on first run. -endpoint and -bridge resolve to the endpoint Init opens on, with APERTURE_ENDPOINT and APERTURE_BRIDGE behind them for the places nobody types the invocation. -bridge takes a name and creates the bridge when there is none, because a flag that only worked after someone had made the bridge by hand would not help the first run, which is the run that needs it. Resolution happens in main before the TUI takes the terminal: a URL we cannot use is a line on stderr and exit 1, which a script can see, rather than a full-screen error it cannot. Init routes through connectVia rather than a new path, so a named endpoint is saved for the failure screen to name and taken back out if the attempt is abandoned, and the saved active endpoint keeps working exactly as before because it is already configured. The obvious alternative, SetActiveEndpoint before starting the TUI, is two lines shorter and makes an unreachable -endpoint displace the one that works.
Two dead aperture-cli-bridge-* machines are in the maintainer's tailnet because RemoveBridge drops the settings entry and nothing else: the node is not ephemeral, so the control plane keeps the device, and nothing in the repo removes the tsnet state dir either. SwitchTailnet already works out that closing a node without logging out orphans the device; that reasoning was never applied to deleting the bridge outright. Writing it down now rather than fixing it because b2a6bc3 changed the odds, not the bug: before it, orphaning took a deliberate second press on a bridge-named row, and now deleting an endpoint cascades into it. The fix is not small, since logout needs the node running and a control-plane round trip that was hanging past 90s yesterday, so delete stops being instant and infallible and needs a bounded wait and an escape. The obvious cheap alternative, making bridges ephemeral, would close the leak and cost a new device and a new interactive login on every run, which is the thing persistent bridges exist to avoid. Revisit when tsnet can deregister a node without bringing it up first.
A 200 line ADR is a spec that did not get written. 0001 carried its own context map, ubiquitous language, ACL research and a deferred-work table, so the decision it exists to record was six screens in and nobody reading it for the decision got there. Nygard's four sections with the reason named as "Why?", capped at a page, recorded in AGENTS.md so the next one starts there. Detail moves to the spec that owns it rather than being cut: the Up table and the printAuthURLLoop finding to the context map's ACL section, the plumbing knots to the contracts spec. 0002 loses the same way and links out. 0002's decision also changes shape: it proposed Manager.Forget, which contradicts 0001 decision 6 and grows the pile Manager already is. Destruction belongs on Machine, the aggregate that owns the node, as Destroy alongside the LeaveTailnet the domain model already has.
StartupEndpoint and bridgeNamed went onto Global because Global was already threaded everywhere the flags had to reach. Global is the pile every client package reads: five unrelated fields, twenty methods, and now two more that touch none of the existing ones. Hanging the next thing off whatever is nearest is how it got that way. Startup owns what the invocation asked for and nothing else, so the resolution rules are testable against a value rather than against the whole loaded config, and Global loses two methods that were never about live app state. The alternative was leaving it: one more pair of methods on a struct that already has twenty is invisible in review, which is the cost. Revisit if Startup grows a second reason to exist, at which point it wants the bridge lookup as a real repository rather than a *Global.
… say Every block here was written to justify a decision and then kept explaining it: fifteen lines on liveEvents, thirteen on health, twelve on authFooter. A comment that long stops being read, and the reason it holds gets skipped with it. Each keeps its forcing fact (the 29 second register, the 502 loop, the dropped login link, Bubble Tea truncating an over-width line) and loses the retelling. Nothing that explains a non-obvious choice was removed; what went was narration of what the code already shows. Comments in main and unchanged files are left alone: trimming them would grow a diff this was meant to shrink.
The appended block came from a cross-project rule set and a third-party "lazy senior developer" persona. Two problems with it here. Most of it has nothing to act on: this is a single-binary Go CLI with no jobs, no schedules, no relational store and no GPUs, so event-driven and durable-by-default, the JSON-in-Postgres rule and the multi-card parallelism rule are instructions about infrastructure that does not exist. Rules that never fire train readers to skim the ones that do. What survives is rewritten against what this repo has: tea.Cmd instead of "async", the bridge state directory instead of "secrets", make check instead of "the project gate". The persona text was also copied verbatim from outside with no license attached, which is not something to carry in a Tailscale repo. Its substance is kept and reworded; the copied prose is gone, along with the web, hardware and Python examples that came with it. Revisit the cut rules if any of that infrastructure shows up.
guygrigsby
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent activation, bridge cleanup, endpoint fallback, and login-link security issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves connection selection, bridge setup, diagnostics, and TUI feedback.
Changes:
- Adds connection flags, picker, URL retargeting, and tailnet switching.
- Introduces typed connection events, persistent logs, and browser/clipboard integration.
- Expands tests, architecture documentation, and CI checks.
File summaries
| File | Description |
|---|---|
README.md |
Documents connection workflows and flags. |
Makefile |
Adds lint and comprehensive check targets. |
.github/workflows/ci-linux.yaml |
Runs the new check target. |
AGENTS.md |
Adds repository engineering guidance. |
cmd/aperture/main.go |
Resolves startup flags and configures run logging. |
go.mod |
Makes OSC 52 a direct dependency. |
internal/tui/tui.go |
Implements cancellable connection attempts and progress UI. |
internal/tui/tui_test.go |
Tests connection and authorization workflows. |
internal/tui/menus.go |
Adds the connection picker and management actions. |
internal/tui/browser.go |
Opens and copies authorization links. |
internal/connection/event.go |
Defines typed connection events and phases. |
internal/connection/event_test.go |
Tests event invariants and URL validation. |
internal/config/state_test.go |
Tests persisted bridge tailnets. |
internal/config/startup.go |
Resolves startup endpoint and bridge selection. |
internal/config/startup_test.go |
Tests startup resolution. |
internal/config/settings.go |
Moves endpoint types to a dedicated file. |
internal/config/runlog.go |
Adds bounded persistent logging. |
internal/config/runlog_test.go |
Tests append and truncation behavior. |
internal/config/global.go |
Persists bridge tailnet metadata. |
internal/config/endpoint.go |
Defines and parses endpoints. |
internal/config/endpoint_test.go |
Tests endpoint parsing. |
internal/bridges/manager.go |
Adds event reporting, login monitoring, and peer resolution. |
internal/bridges/manager_test.go |
Tests bridge lifecycle and diagnostics. |
docs/specs/connection-domain-model.md |
Defines the connection domain model. |
docs/specs/connection-contracts.md |
Documents connection contracts and events. |
docs/specs/connection-context-map.md |
Maps connection bounded contexts. |
docs/specs/bridge-resource-lifecycle.md |
Documents bridge resource cleanup requirements. |
docs/adr/0001-connection-bounded-context.md |
Records the connection architecture decision. |
docs/adr/0002-bridge-removal-destroys-the-machine.md |
Records bridge destruction requirements. |
Review details
Suppressed comments (1)
internal/bridges/manager.go:440
- This starts a second IPN-bus watcher while
rt.node.Up(ctx)below still runs tsnet's own watcher. ADR 0001 explicitly rejects this combination because a lagging watcher can be evicted withIPN bus consumer fell behind, turning an event burst into an unrelated activation failure. The custom phase reporting and bring-up should share one watcher, as required by decision 4, rather than layering another watcher aroundUp.
watchCtx, stopWatch := context.WithCancel(ctx)
defer stopWatch()
go rt.node.WatchLogin(watchCtx, ev)
- Files reviewed: 29/29 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case connection.PhaseEntered: | ||
| slog.Info("bridge phase", "phase", e.Phase) | ||
| case connection.LoginRequired: | ||
| slog.Info("bridge needs login", "url", e.Link.String()) |
Contributor
Author
There was a problem hiding this comment.
Fixed. LoginRequired now logs bridge needs login with no URL, and notes, which carry tsnet's backend logger under -debug, go through redactDiagnostic so any link the backend repeats is replaced with [redacted URL]. The link survives only in the interactive event the connect screen renders. 608b85c, internal/bridges/manager.go.
Fixed in 608b85c
Comment on lines
+401
to
+405
| if rt != nil { | ||
| m.mu.Unlock() | ||
| // The node and its proxies were built by an earlier connection whose | ||
| // sink is long gone. Point them at this one before returning. | ||
| rt.ev.use(ev) |
Contributor
Author
There was a problem hiding this comment.
Fixed differently: the node is still cached before Up returns, but nothing else can reach it while it is starting. Machine now carries a one-slot turn channel, and every operation goes through Manager.acquire/release (internal/bridges/machine.go), so attempt B blocks until A has finished and released rather than dialling through a node A is closing. The sink is re-pointed through liveEvents on acquire, so the login watcher reports to whoever holds the turn now, not to A's cancelled sink. Publishing only after Up succeeds would not help here: the whole point of the wait is the interactive login, which happens during Up. 316b57d.
Fixed in c56b2d9
| case "darwin": | ||
| cmd = exec.Command("open", url) | ||
| case "windows": | ||
| cmd = exec.Command("cmd", "/c", "start", "", url) |
Contributor
Author
There was a problem hiding this comment.
Fixed. The Windows opener is internal/tui/browser_windows.go now and calls windows.ShellExecute with the URL as the file argument, so cmd.exe is out of the path entirely and shell punctuation stays URL data. browser_windows_test.go pins the arguments handed to ShellExecute. 316b57d.
Fixed in c56b2d9
Comment on lines
+29
to
+34
| if name != "" { | ||
| bridge, err := s.bridge(g, name) | ||
| if err != nil { | ||
| return Endpoint{}, err | ||
| } | ||
| bridgeID = bridge.ID |
Contributor
Author
There was a problem hiding this comment.
Comment on lines
+46
to
+48
| for _, b := range g.Settings.Bridges { | ||
| if strings.EqualFold(b.Name, name) { | ||
| return b, nil |
Contributor
Author
There was a problem hiding this comment.
Took the second option: an ambiguous -bridge name is now an error rather than a silent first match. Startup.bridge collects every case-insensitive match and, on more than one, fails naming the count and the bridge IDs so the user can tell which is which and rename one. Enforcing uniqueness at AddBridge would have to reject or rename bridges people already have on disk, and the name is the user's own label with nothing keying off it. ac07745.
Fixed in 7ba574f
| return nil | ||
| } | ||
| } | ||
| return m.g.RemoveBridge(id) |
Contributor
Author
There was a problem hiding this comment.
Implemented the ADR 0002 lifecycle rather than backing the cascade out. Two commits.
2929492 gives the aggregate the destroy: Machine.destroy does Logout, Close, then discards the state directory, and Manager.Destroy is the way in because the turn and the cache entry are Manager state. bridges.MachineName and bridges.HasMachine are exported so the TUI can name the device and can tell a bridge that never registered from one that did, without starting a node to find out.
d38ff81 routes all five removal sites, including this cascade, through one remove in internal/tui/removal.go. It confirms, naming aperture-cli-<bridge-id> and the tailnet, runs the logout on the connect screen with its log tail, and only then drops settings, endpoint first. The logout is bounded at 45s; on timeout the local records go and the error names the device still on the tailnet. A failed logout keeps the connection so removing it again retries.
Tests cover destroy ordering (settings still present inside the destroy callback), a refused logout keeping the records, an unstarted bridge removing instantly, both delete UIs, and the timeout path.
Fixed in bad77c4
Comment on lines
+690
to
+692
| if err := m.g.ReplaceEndpoint(ep, next); err != nil { | ||
| return simpleErrorCmd(err) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed. promptEditEndpoint no longer replaces anything up front: it starts the activation and records the old endpoint on the attempt as act.replaces. The swap happens in SetActiveEndpoint(msg.endpoint, m.act.replaces) once the candidate has passed /v1/models, so a failed edit leaves the previous active row in place to fall back to. A second edit of an in-flight attempt retargets it rather than stacking another replacement. 608b85c.
Fixed in 608b85c
| ## Conventions | ||
|
|
||
| - Commit prefixes match the package touched: `tui:`, `bridges:`, `config:`. | ||
| - `make test` is the gate. |
Contributor
Author
608b85c committed the callers without the new files, so HEAD does not build: manager.go acquires a turn on a Machine that is not there, and browser.go calls a platformOpenURL with no implementation on any platform. machine.go gives one bridge one cancellable turn at a time, so an inline override cannot dial through a node the cancelled attempt is still closing. browser_windows.go opens the login link with ShellExecuteW, because cmd.exe reads the & in a control-plane URL as a command separator. Reasoning in ADR 0003 and ADR 0004; the tests here are what holds both.
`aperture -bridge work -endpoint ftp://host` exited with a usage error and left a bridge called work on disk, because Resolve looked the bridge up (creating it) before parsing the URL. Parsing first costs nothing: a bridge is only needed once the URL is known to be usable. A name matching two bridges now fails naming both IDs rather than taking the first. Nothing keeps names unique, so first-match made the second bridge unaddressable from the command line with no way to tell. Enforcing uniqueness in AddBridge was the alternative and it invalidates configs that are already on disk, for a label the program itself never keys off.
`make test` skips the race detector and the build, which is where a bridge fails: several goroutines racing a control plane. A contributor following AGENTS.md could land red CI from a green local run.
Two dead aperture-cli-bridge-* machines in the maintainer's tailnet came from deleting a bridge: the settings entry went and the registered device stayed. SwitchTailnet already knew a close without a logout orphans the device; removal never got that reasoning, and b2a6bc3 made it the common path by cascading endpoint deletion into it. Destroy holds the Machine's turn like every other operation on that node, because a second Machine for one bridge would open the state directory a running attempt is still writing. It initializes the node but never brings it up: bring-up is the interactive login being removed, so demanding one to leave would make an expired identity unremovable. The state directory goes last and only on success, since it holds the node key a later attempt needs to deregister the device. HasMachine reads that directory rather than Bridge.Tailnet, which is a display hint written after verification and cleared before a switch, so it is empty for machines that do exist.
Deleting a connection dropped settings and left the device on the tailnet. Settings are the only record the device exists, so they have to outlast the logout: removal confirms, logs the node out, discards its state directory, and only then drops the endpoint and the orphaned bridge. Five of the six removal sites now describe the delete as a bridgeRemoval and hand it to one function, so the picker, its row page, the bridges page and the setup guide cannot disagree about what removing means. discardActivation is the sixth and stays as it is: it abandons an ephemeral endpoint for a bridge that never finished a login, which has nothing to deregister. The wait reuses the connect screen and keeps no cancel handle. Logout is a round trip to a control plane that was hanging past 90s on 2026-09-17, and Esc half way through it is how the record and the device start disagreeing. Bounded at 45s instead, after which the local records go and the message names the surviving device and its tailnet, which is what the user needs to finish the job in the admin console. A bridge with no state directory never registered, and is removed with no confirmation and no round trip.
A login ran two IPN bus consumers: tsnet.Server.Up watches for Running while WatchLogin watched the same bus for the phase and the link. The backend evicts a consumer that falls behind with a terminal ErrMessage, which Up returns as "IPN bus consumer fell behind", so a burst of notifications during registration could fail a bring-up the user did nothing wrong in. ADR 0001 decision 4 rejected this shape up front and the code kept it. BringUp replaces Up and WatchLogin on the node, running one watch that names each wait and returns the status when Running arrives. Taking the wait means taking what Up did with it: ErrMessage is terminal, and a Running node with no TailscaleIPs is refused. resetServeStateOnce is dropped, nothing here sets a serve config. The alternative was keeping both watchers and hoping the login watch stays ahead of the bus, which is the same bet with no way to observe losing it: eviction surfaces as an unrelated bring-up error. Revisit if tsnet publishes phases itself, which would make an owned bring-up loop pure cost.
The knot is still real, a cached node re-logs in with nothing watching, but it was described by a symbol 9c61d05 removed, so a reader grepping for WatchLogin finds nothing and cannot tell whether the knot went with it.
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.