TUI UX improvements

.github/workflows/ci-linux.yaml

@@ -25,13 +25,8 @@ jobs:
         with:
           go-version: ${{ matrix.go-version }}
 
-      # Only run in this linux based runner
-      - name: Check Formatting
-        run: |
-          if [ "$(gofmt -l . | wc -l)" -gt 0 ]; then
-            gofmt -l .
-            exit 1
-          fi
-
-      - name: Run tests
-        run: make test
+      # The Makefile is the one definition of the gate, so a green local
+      # `make check` means a green CI. The formatting check that used to be
+      # inlined here lives in `make lint`, which check depends on.
+      - name: Check
+        run: make check

AGENTS.md

@@ -0,0 +1,126 @@
+# aperture-cli
+
+## Design
+
+Domain-driven design is the primary design paradigm for this project. New work
+that introduces or reshapes a domain concept is modelled before it is written.
+
+The domain model, the data model and the contracts are one first step, not
+three stages. A domain model without its contracts is a description; contracts
+without a model have nothing to be complete about. Define them together, before
+the code, and let each correct the other.
+
+- Bounded contexts are sized by language, not by responsibility. Splitting a
+  context because two halves feel like different jobs is the usual mistake;
+  if the user experiences one thing, it is one context.
+- `domain` is never a package name. Packages and types are named after the
+  thing they are, by what they do in this program rather than by their
+  technical role. `Machine`, not `NodeManager`. Where the thing already has a
+  name in the system it wraps, take that name: a tailnet node registered by
+  `POST /machine/register` is a `Machine`.
+- Every domain object is classified entity, value object or enumeration, and
+  every field is listed. Behaviour lives on the object.
+- Vendor types never appear in domain signatures. `tsnet`, `ipn` and
+  `ipnstate` are confined to `internal/bridges`, which is the anti-corruption
+  layer for the tailnet.
+
+Artifacts, written before the code:
+
+- `docs/specs/<context>-context-map.md` — ubiquitous language, contexts,
+  relationships, ambiguous terms.
+- `docs/specs/<context>-domain-model.md` — one section per object, with
+  fields, behaviours, invariants, states and relationships.
+- `docs/specs/<context>-contracts.md` — the API, the domain events and the data
+  model, each to 100% and cross-checked against each other, so every aggregate
+  transition can be traced through all three. A contract this project does not
+  have is recorded as absent with its reason, never left blank.
+- `docs/adr/NNNN-<slug>.md` — the decision and the forcing reason.
+
+Mermaid diagrams in those files are rendered before the commit that adds them.
+
+### Writing them
+
+Progressive disclosure. Every document answers in its first paragraph and
+deepens from there, so a reader who stops early still leaves with the decision.
+Detail belongs in the spec; the ADR links to it.
+
+An ADR is one page, Nygard's shape with the reason made explicit:
+
+```
+# NNNN. Title            <- the decision, not the topic
+Status / Date
+## Why?                  <- the concrete failure, never the category
+## Decision              <- numbered, each one testable
+## Consequences          <- what this costs, not what it wins
+## Rejected              <- option, then the cost of taking it
+## Revisit when          <- the condition that reopens this
+```
+
+An ADR longer than that has a spec trying to get out of it. Both are read by
+someone with the code in front of them, so neither restates what the diff
+already says.
+
+Current: [Connection](docs/adr/0001-connection-bounded-context.md).
+
+## Conventions
+
+- Commit prefixes match the package touched: `tui:`, `bridges:`, `config:`.
+- `make check` is the gate, and it is what CI runs: lint, build, then the suite
+  under the race detector. `make test` is the fast loop, not the bar.
+
+## Workflow
+
+- Never commit to `main`. Every change gets a branch.
+- Prove a bug with a failing test first: reproduce it, watch it fail, fix to
+  green. No harness at that layer means adding the smallest one and wiring it
+  into `make check`.
+- Test through the real path before calling it fixed. Here that is the built
+  binary in a terminal, not only `go test`. State what was verified and what
+  could not be.
+- Reply to every addressed PR comment with the commit that fixed it: backticked
+  short hash, linked. No "done" without a hash.
+- Commit bodies and ADRs carry the why, because the what is in the diff: the
+  concrete failure, what the obvious alternative would have cost, and what
+  would justify revisiting. Same for PR descriptions.
+- Security-review any diff touching login links, tailnet identity, credentials
+  or the bridge state directory before it merges, as a fresh-context
+  adversarial pass by someone other than the author. Verify each finding; the
+  build is the arbiter.
+- Documents go in content-typed paths (`docs/specs/`, `docs/adr/`), never in a
+  path named after whatever produced them.
+
+## Writing the code
+
+Least code that solves it. Before writing any, in order: does it need to exist
+at all (skip it, and say so), is it already here (reuse it), does the stdlib do
+it, does a dependency already in `go.mod` do it, can it be one line, and only
+then the minimum new code. A new dependency has to be maintained and
+license-compatible, no GPL/AGPL; name the one chosen, or why none fit.
+
+- No abstraction nobody asked for: no interface with one implementation, no
+  wrapper around a single call, no config for a value that never changes.
+- Deletion over addition. Boring over clever.
+- Never grow a God object. When the natural home for new state is the struct
+  everything already hangs off, that is the signal to give it its own type.
+  Tells: unrelated field clusters, methods that ignore most of the fields, a
+  name that is a role rather than a thing, tests that cannot construct it
+  without stubbing the world.
+- Self-review each diff for duplication and misplaced logic before committing:
+  near-identical functions, repeated literals, a second copy of a helper that
+  already exists, code sitting in the wrong package.
+- Nil-check where a value enters: anything off an HTTP response, parsed input,
+  settings on disk or a function that can return nil is guarded at first
+  receipt. A check at every use site means the guard is missing at the door.
+- Handle errors where recovery is possible. Returning err to `main` moves every
+  failure to the top with no context and no recovery; propagate only when the
+  caller owns the decision, otherwise retry, default, wrap or degrade.
+- Fix the root cause, not the path the report names. Grep every caller first:
+  one guard in the shared function is a smaller diff than a guard in each.
+- Slow work is a `tea.Cmd`, never inline in `Update`. Back off on retries and
+  never poll the control plane or the LocalAPI in a tight loop.
+- Work consciously skipped is said out loud, never left as a TODO comment or
+  as speculative code.
+
+Being lazy about the solution is the goal. Being lazy about understanding it is
+not: trace the flow a change touches before picking an approach, because the
+smallest change in the wrong place is a second bug.

Makefile

@@ -1,4 +1,4 @@
-.PHONY: build test clean install
+.PHONY: build test lint check clean install
 
 BUILD_DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ")
 GIT_HEIGHT := $(shell git rev-list --count HEAD 2>/dev/null || echo 0)
@@ -16,6 +16,20 @@ build:
 test:
 	go test ./...
 
+# Nothing here needs installing: gofmt and vet ship with the toolchain, so a
+# clean checkout can run this.
+lint:
+	@out=$$(gofmt -l .); if [ -n "$$out" ]; then echo "gofmt:"; echo "$$out"; exit 1; fi
+	go vet ./...
+	go mod tidy -diff
+
+# The gate. Differs from test in the two ways that matter here: it runs the
+# suite under the race detector, and it builds. A bridge is several goroutines
+# racing a control plane, so a data race is the failure this project actually
+# has, and `go test` will not find one.
+check: lint build
+	go test -race ./...
+
 install:
 	go install -ldflags "$(LDFLAGS)" ./cmd/aperture
 

README.md

@@ -57,19 +57,49 @@ This is useful on machines where installing Tailscale is not practical, where Ap
 
 To use bridge mode:
 
-1. Open `Settings`, then open `Aperture Endpoints` and press `a` to add an endpoint.
-2. Choose `Bridge`, then select an existing bridge or choose `Add Bridge`.
-3. Enter the Aperture URL and follow the Tailscale login prompt for the bridge.
+1. Press `c` on the agent menu for `Change connection` (the same screen as `Settings`, then `Aperture Endpoints`).
+2. Choose `Add a connection`, then `Bridge`, then an existing bridge or `Add Bridge`.
+3. Authorize the bridge. Aperture CLI opens your browser at the Tailscale login link; if it cannot, the link stays on screen to open by hand. No URL is asked for: the bridge looks for Aperture at `http://ai`, the same location a direct connection starts from.
 4. Aperture CLI verifies `/v1/models`, makes the endpoint active, and returns to the agent menu.
 
-If verification fails, the endpoint remains configured for retry or editing, and any previous working endpoint remains active. Select a configured endpoint from `Aperture Endpoints` to switch to it.
+If your Aperture answers on a different hostname, type it on the connect screen while the default is being tried. That cancels the attempt and connects to what you typed. Esc abandons the attempt and leaves your current endpoint alone.
+
+### Choosing a connection
+
+`Change connection` lists everything this launcher can reach: each saved endpoint, and each bridge that has no endpoint yet, labelled with the tailnet it reaches. That is the screen to use when the launcher connected on its own and you wanted the other bridge.
+
+Selecting a row opens it. From there you can connect to it, change its URL, switch its tailnet, or remove it.
+
+`http://ai` can answer and still be the wrong Aperture, which is what happens when the bridge joins a tailnet that already has a host called `ai`. `Change URL` points the connection somewhere else; it keeps the bridge it is reached through and reconnects.
+
+### Switching tailnets
+
+A bridge is on one tailnet at a time. `Switch tailnet` logs it out, which removes its node from that tailnet, then prints a new login link: open it and pick the tailnet you want. Use a second bridge instead if you want to keep both logins and choose between them.
+
+If verification fails, the endpoint remains configured for retry or editing, and any previous working endpoint remains active.
 
 ### Flags
 
-| Flag | Description |
-|------|-------------|
-| `-version` | Print build version and exit |
-| `-debug` | Print environment variables set before launching the agent |
+| Flag | Environment | Description |
+|------|-------------|-------------|
+| `-version` | | Print build version and exit |
+| `-debug` | | Print environment variables set before launching the agent |
+| `-endpoint` | `APERTURE_ENDPOINT` | Aperture URL to open on, instead of the saved one |
+| `-bridge` | `APERTURE_BRIDGE` | Connect through the bridge with this name, creating it if there is none |
+
+A flag beats its environment variable, so a one-off run can override whatever
+the shell was started with. `-bridge` on its own starts at the well-known
+location, the same guess the connection picker makes:
+
+```sh
+aperture -bridge work                                   # http://ai over the "work" bridge
+aperture -bridge work -endpoint aperture.example.com    # that URL over the "work" bridge
+aperture -endpoint aperture.example.com                 # direct, no bridge
+```
+
+Neither is made the saved active endpoint until the connection works, so an
+unreachable URL passed on the command line does not displace the one that does
+work.
 
 ## Development
 

cmd/aperture/main.go

@@ -3,6 +3,7 @@ package main
 import (
 	"flag"
 	"fmt"
+	"io"
 	"log/slog"
 	"os"
 	"os/exec"
@@ -17,7 +18,7 @@ import (
 	"github.com/tailscale/aperture-cli/internal/profiles"
 	"github.com/tailscale/aperture-cli/internal/tui"
 
-	// Side-effect imports register each client with internal/clients.
+	// Each import registers its client with internal/clients.
 	_ "github.com/tailscale/aperture-cli/internal/clients/claudecode"
 	_ "github.com/tailscale/aperture-cli/internal/clients/codex"
 	_ "github.com/tailscale/aperture-cli/internal/clients/copilot"
@@ -29,8 +30,10 @@ import (
 )
 
 var (
-	flagVersion = flag.Bool("version", false, "print version and exit")
-	flagDebug   = flag.Bool("debug", false, "enable bridge diagnostics and print agent launch environment")
+	flagVersion  = flag.Bool("version", false, "print version and exit")
+	flagDebug    = flag.Bool("debug", false, "enable bridge diagnostics and print agent launch environment")
+	flagEndpoint = flag.String("endpoint", "", "Aperture URL to open on, instead of the saved one ($APERTURE_ENDPOINT)")
+	flagBridge   = flag.String("bridge", "", "connect through the bridge with this name, creating it if there is none ($APERTURE_BRIDGE)")
 
 	buildVersion = "B0-dev"
 	buildCommit  = "unknown"
@@ -51,7 +54,7 @@ func init() {
 		}
 	}
 
-	// Only fill in VCS info when ldflags haven't already set these values.
+	// ldflags may have set these already. Fill them from VCS info only then.
 	if buildCommit != "unknown" {
 		return
 	}
@@ -111,6 +114,62 @@ func gitCommitHeightInDir(dir string) string {
 	return height
 }
 
+// startRunLog points slog at the run log and returns a function that closes
+// it. Records are written straight through, so the os.Exit paths that skip
+// the close lose nothing. The close is tidiness, not a flush.
+//
+// A run that cannot open the file still runs. Diagnostics are not worth
+// refusing to start over. Such a run discards them rather than writing to
+// stderr, because stderr is the TUI's screen.
+//
+// verbose only raises the level. The log is on for every run. The run worth
+// reading back is the one that went wrong, and nobody knows to pass -debug
+// before it does.
+func startRunLog(verbose bool) func() {
+	f, err := config.OpenRunLog()
+	if err != nil {
+		slog.SetDefault(slog.New(slog.NewTextHandler(io.Discard, nil)))
+		return func() {}
+	}
+	level := slog.LevelInfo
+	if verbose {
+		level = slog.LevelDebug
+	}
+	slog.SetDefault(slog.New(slog.NewTextHandler(f, &slog.HandlerOptions{Level: level})))
+	slog.Info("aperture starting", "version", buildVersion, "commit", buildCommit, "pid", os.Getpid())
+	return func() {
+		slog.Info("aperture exiting")
+		f.Close()
+	}
+}
+
+// reportFailure prints err and the run log path to stderr. Every diagnostic
+// goes to the run log, which is right for a running TUI and wrong for a run
+// that just died. Without this, a failed launch prints nothing and exits 1.
+//
+// stderr is safe at both call sites. The TUI either never started or has
+// already given the terminal back.
+func reportFailure(err error) {
+	fmt.Fprintln(os.Stderr, "aperture:", err)
+	if path, pathErr := config.RunLogPath(); pathErr == nil {
+		fmt.Fprintln(os.Stderr, "details:", path)
+	}
+}
+
+// flagOrEnv returns flag name when it was passed, even empty, and otherwise
+// the environment variable key. The variable lets a dotfile, container or
+// systemd unit make the same selection a typed invocation can. The flag wins
+// so a one-off run can override the shell it started in: -bridge= means no
+// bridge.
+func flagOrEnv(fs *flag.FlagSet, name, key string) string {
+	passed := false
+	fs.Visit(func(f *flag.Flag) { passed = passed || f.Name == name })
+	if passed {
+		return fs.Lookup(name).Value.String()
+	}
+	return os.Getenv(key)
+}
+
 func main() {
 	flag.Parse()
 
@@ -123,26 +182,45 @@ func main() {
 		os.Exit(0)
 	}
 
+	// Start the log before anything that logs. slog's default handler writes
+	// to stderr, and on a TUI that owns the terminal that paints a line over
+	// the screen. Until this runs every diagnostic is either damage or lost.
+	closeLog := startRunLog(*flagDebug)
+	defer closeLog()
+
 	g, err := config.Load()
 	if err != nil {
 		slog.Error("loading launcher config", "err", err)
+		reportFailure(err)
 		os.Exit(1)
 	}
 	g.Debug = *flagDebug
 
 	// Register Claude Desktop on supported platforms (darwin, windows).
 	profiles.RegisterIfSupported()
 
-	bridgeManager := bridges.NewManager(g.Debug)
-	p := tea.NewProgram(tui.NewModel(g, buildVersion, bridgeManager))
+	// Resolve the flags before the TUI takes the terminal. A URL we cannot use
+	// then exits non-zero instead of painting an error the script that passed
+	// it will never see.
+	start, err := config.EndpointFromFlags(g, flagOrEnv(flag.CommandLine, "endpoint", "APERTURE_ENDPOINT"), flagOrEnv(flag.CommandLine, "bridge", "APERTURE_BRIDGE"))
+	if err != nil {
+		slog.Error("resolving the endpoint to open on", "err", err)
+		reportFailure(err)
+		os.Exit(1)
+	}
+
+	machines := bridges.NewMachines(g.Debug)
+	p := tea.NewProgram(tui.NewModel(g, buildVersion, machines, start))
 
 	var exitCode int
 	if _, err := p.Run(); err != nil {
 		slog.Error("launcher error", "err", err)
+		reportFailure(err)
 		exitCode = 1
 	}
-	if err := bridgeManager.Close(); err != nil {
+	if err := machines.Close(); err != nil {
 		slog.Error("shutting down bridges", "err", err)
+		reportFailure(err)
 		exitCode = 1
 	}
 	if exitCode != 0 {